Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1699953002:
Make png filter functions compatible with libpng (Closed)
DescriptionMake png filter functions compatible with libpng
We hope to upstream these functions to libpng (a C library).
Let's make the code that Skia tests on look like the code
that we are submitting.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1699953002
Committed: https://skia.googlesource.com/skia/+/c29cd72df2ac355201486be27c90e60b0a946e4b
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add loadX() and storeX() functions #
Total comments: 11
Patch Set 3 : Return to Skia style #Messages
Total messages: 21 (9 generated)
Description was changed from ========== Make png filter functions compatible with libpng We hope to upstream these functions to libpng (a C library). Let's make the code that Skia tests on look like the code that we are submitting. BUG=skia: ========== to ========== Make png filter functions compatible with libpng We hope to upstream these functions to libpng (a C library). Let's make the code that Skia tests on look like the code that we are submitting. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
msarett@google.com changed reviewers: + mtklein@google.com
You can view the patch that I've prepared for libpng here. https://github.com/mattsarett/libpng/commits/libpng16 https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp File src/codec/SkPngFilters.cpp (right): https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp#... src/codec/SkPngFilters.cpp:138: #if 0 && defined(__SSE4_1__) // Make sure we have a bot testing this before enabling. I dropped the SSE4 code for libpng patch.
You can view the patch that I've prepared for libpng here. https://github.com/mattsarett/libpng/commits/libpng16 If this lands and doesn't cause any bot/Gold issues, I'll go ahead and make the pull request.
https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp File src/codec/SkPngFilters.cpp (right): https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp#... src/codec/SkPngFilters.cpp:29: a = d; memcpy(&d, row, 3); Seems worth keeping load() and store() for readability, even if they're all implemented with memcpy: static __m128i load3(const void*) { ... } static __m128i load4(const void*) { ... } static void store3(void*, __m128i) { ... } static void store4(void*, __m128i) { ... }
https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp File src/codec/SkPngFilters.cpp (right): https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp#... src/codec/SkPngFilters.cpp:138: #if 0 && defined(__SSE4_1__) // Make sure we have a bot testing this before enabling. On 2016/02/15 19:50:20, msarett wrote: > I dropped the SSE4 code for libpng patch. Let's set up a bot to test this tomorrow. It'd actually be somewhat of a shame to drop this for Android, given that they will be building with -march=native, and new mobile x86 chips do support SSE 4.1.
https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp File src/codec/SkPngFilters.cpp (right): https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp#... src/codec/SkPngFilters.cpp:29: a = d; memcpy(&d, row, 3); On 2016/02/15 20:16:32, mtklein wrote: > Seems worth keeping load() and store() for readability, even if they're all > implemented with memcpy: > > static __m128i load3(const void*) { ... } > static __m128i load4(const void*) { ... } > > static void store3(void*, __m128i) { ... } > static void store4(void*, __m128i) { ... } Done. https://codereview.chromium.org/1699953002/diff/1/src/codec/SkPngFilters.cpp#... src/codec/SkPngFilters.cpp:138: #if 0 && defined(__SSE4_1__) // Make sure we have a bot testing this before enabling. On 2016/02/15 20:27:33, mtklein wrote: > On 2016/02/15 19:50:20, msarett wrote: > > I dropped the SSE4 code for libpng patch. > > Let's set up a bot to test this tomorrow. > > It'd actually be somewhat of a shame to drop this for Android, given that they > will be building with -march=native, and new mobile x86 chips do support SSE > 4.1. Dropping the #if 0 in order to actually run this code. Let's land it this way, if we are confident that we can get a bot set-up today.
I think we might want to just make this C compatible as a first step, then translate any style changes only when converting it to a libpng patch. If libpng's got some sort of style guide, we should find a way to rewrite the code so that it's still clear in that style... just re-whitespacing our style seems to really hurt readability here. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.cpp File src/codec/SkPngFilters.cpp (right): https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:28: return _mm_cvtsi32_si128(packed); Now that we've split these apart, we might consider writing this more simply: return _mm_cvtsi32_si128(*(const int*)p); and same for store4: *(int*)p = _mm_cvtsi128_si32(v); https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:41: void sk_sub3_sse2(png_row_infop row_info, uint8_t* row, Why do these guys go to two lines? Wouldn't it only be 78 columns to write void sk_sub3_sse2(png_row_infop row_info, uint8_t* row, const uint8_t* prev) { ? https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:87: __m128i b; We might want to scoot b over a few columns right to keep the c b a d layout. Same deal below with the load: b = load3(prev); a = d; d = load3(row); https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:188: d = load3(row); I think this breaks things by loading new values for b and d before we rotate the old values into c and a. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:192: // (p-a) == (a+b-c - a) == (b-c) Moving these comments around and changing the alignment here has made things harder to read. If there's no way to have everything under formatting constraints, let's sacrifice the comments, or move them out of line. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:206: if_then_else(_mm_cmpeq_epi16(smallest, pb), b, I think this hurts readability to indent like this. It's lost the alignment that made it clear.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Newest patch restores the old style. I probably shouldn't have just used a "whitespace fixer" to move code from libpng to Skia. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.cpp File src/codec/SkPngFilters.cpp (right): https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:41: void sk_sub3_sse2(png_row_infop row_info, uint8_t* row, On 2016/02/16 14:11:57, mtklein wrote: > Why do these guys go to two lines? Wouldn't it only be 78 columns to write > > void sk_sub3_sse2(png_row_infop row_info, uint8_t* row, const uint8_t* prev) { > > ? Not sure. I copied the style of the signatures from libpng's NEON opts. Note that libpng uses png_bytep so it's different. Irrelevant since I'm returning this to Skia style. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:87: __m128i b; On 2016/02/16 14:11:57, mtklein wrote: > We might want to scoot b over a few columns right to keep the > c b > a d > layout. Same deal below with the load: > > b = load3(prev); > a = d; d = load3(row); Done. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:188: d = load3(row); On 2016/02/16 14:11:57, mtklein wrote: > I think this breaks things by loading new values for b and d before we rotate > the old values into c and a. Done. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:192: // (p-a) == (a+b-c - a) == (b-c) On 2016/02/16 14:11:57, mtklein wrote: > Moving these comments around and changing the alignment here has made things > harder to read. > > If there's no way to have everything under formatting constraints, let's > sacrifice the comments, > or move them out of line. Done. https://codereview.chromium.org/1699953002/diff/20001/src/codec/SkPngFilters.... src/codec/SkPngFilters.cpp:206: if_then_else(_mm_cmpeq_epi16(smallest, pb), b, On 2016/02/16 14:11:57, mtklein wrote: > I think this hurts readability to indent like this. It's lost the alignment > that made it clear. Done.
lgtm
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699953002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699953002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to request the patch to try. Please note that binary files are still unsupported at the moment, this is being worked on. Thanks for your patience. Transient error: No JSON object could be decoded
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1699953002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1699953002/80001
Message was sent while issue was closed.
Description was changed from ========== Make png filter functions compatible with libpng We hope to upstream these functions to libpng (a C library). Let's make the code that Skia tests on look like the code that we are submitting. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make png filter functions compatible with libpng We hope to upstream these functions to libpng (a C library). Let's make the code that Skia tests on look like the code that we are submitting. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/c29cd72df2ac355201486be27c90e60b0a946e4b ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://skia.googlesource.com/skia/+/c29cd72df2ac355201486be27c90e60b0a946e4b |
