Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1643623004:
A variety of SkPngCodec clean-ups (Closed)
DescriptionA variety of SkPngCodec clean-ups
(1) Remove #ifdefs that extend support to > 1.2.
Using nullptr everywhere should work on all versions.
(2) Use png_get_valid(tRNS) to check for transparency
It does the same thing we were doing previously in less
work.
(3) Remove image size check
Clients allocate their own memory, and they have the option
to perform scaled and partial decodes. So we don't need
to arbitrarily fail on large images.
(4) Remove FIXME for subsitute trans color
libpng is already doing this for us.
(5) Remove #ifdef PNG_READ_PACK_SUPPORTED
We don't have a fallback, so we'll fail to compile if this
isn't supported.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1643623004
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
Committed: https://skia.googlesource.com/skia/+/13a912354875a747aefb1f00de50eb3ec454ab1f
Patch Set 1 : #
Total comments: 38
Patch Set 2 : Response to comments #
Total comments: 2
Patch Set 3 : Changes to decodePalette() #Patch Set 4 : Change comment #Messages
Total messages: 24 (16 generated)
Description was changed from ========== Clean up SkPngCodec BUG=skia: ========== to ========== Clean up SkPngCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + scroggo@google.com
This shouldn't change much, but IMO it makes things a little more understandable and removes a little unnecessary work/code. May be some small perf improvements, I'll update with numbers if there are any. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (left): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:38: // Helper macros IMO these don't help much. We need to be compiled 1.6+ anyway to optimize the filter functions. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:118: SkOpts::RGB_to_RGB1(colorPtr, palette, numColors); It's a little frustrating to optimize this knowing that we are going to memcpy() it (possibly twice I think), later on. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:127: for (int i = 0; i < numColorsWithAlpha; i++) { I don't SIMD premuls will do much here for two reasons. (1) We need to touch each pixel anyway to set a new alpha value. (2) I would guess that numColorsWithAlpha is often "1" or small.
https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (left): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:571: // FIXME: do we need substituteTranspColor? Note that we cannot do it for Substituting in a "transparent color" is necessary when a transparent color is specified in a tRNS chunk. However, we always tell libpng to handle this by calling png_set_tRNS_to_alpha(png_ptr) and getting the output as RGBA. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:118: SkOpts::RGB_to_RGB1(colorPtr, palette, numColors); On 2016/01/28 01:26:18, msarett wrote: > It's a little frustrating to optimize this knowing that we are going to memcpy() > it (possibly twice I think), later on. Here we treat a pointer to the png_color struct as uint8_t*. I think this is safe unless png changes the implementation of the png_color struct. Currently it is: struct { uint8 red; uint8 green; uint8 blue; } https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:127: for (int i = 0; i < numColorsWithAlpha; i++) { On 2016/01/28 01:26:18, msarett wrote: > I don't SIMD premuls will do much here for two reasons. > > (1) We need to touch each pixel anyway to set a new alpha value. > (2) I would guess that numColorsWithAlpha is often "1" or small. Maybe we should check if alpha is zero (which it will often be) before multiplying? Maybe we ought to call SkMulDiv255?
https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (left): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:38: // Helper macros On 2016/01/28 01:26:18, msarett wrote: > IMO these don't help much. We need to be compiled 1.6+ anyway to optimize the > filter functions. They were added to the original SkImageDecoder in c1c5695f8f0cda7cbabb490d274d742ca0e26eed. I copied them over to SkCodec. Does removing these give us problems when using the older versions of libpng? Do we care? https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:296: int64_t size = sk_64_mul(origWidth, origHeight); Looks like this got removed. Can you add that to the commit message? https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:307: #ifdef PNG_READ_PACK_SUPPORTED Why did you remove this? https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:571: // FIXME: do we need substituteTranspColor? Note that we cannot do it for On 2016/01/28 02:06:46, msarett wrote: > Substituting in a "transparent color" is necessary when a transparent color is > specified in a tRNS chunk. > > However, we always tell libpng to handle this by calling > png_set_tRNS_to_alpha(png_ptr) and getting the output as RGBA. Good to know. This would be a good thing to specify in the commit message. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:118: SkOpts::RGB_to_RGB1(colorPtr, palette, numColors); On 2016/01/28 02:06:45, msarett wrote: > On 2016/01/28 01:26:18, msarett wrote: > > It's a little frustrating to optimize this knowing that we are going to > memcpy() > > it (possibly twice I think), later on. > > Here we treat a pointer to the png_color struct as uint8_t*. I think this is > safe unless png changes the implementation of the png_color struct. Currently > it is: > > struct { > uint8 red; > uint8 green; > uint8 blue; > } Can you add these comments plus some asserts here? Something like #ifdef SK_DEBUG for (int i = 0; i < numColors; i++) { png_colorp color = &png_colorp[i]; SkASSERT(color->red == color[0]); ... } #endif Or do you think that's overkill? Or maybe just a static_assert that checks the struct itself? (Not sure if that's possible.) https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:118: SkOpts::RGB_to_RGB1(colorPtr, palette, numColors); On 2016/01/28 01:26:18, msarett wrote: > It's a little frustrating to optimize this knowing that we are going to memcpy() > it (possibly twice I think), later on. I can think of two places we memcpy: - Into the SkColorTable Maybe we should fix SkColorTable's API? - Into the client's memory in getPixels This is a problem with the SkCodec/SkImageGenerator API, which we should reconsider There may be others. I think it makes sense to optimize them separately. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:120: SkOpts::RGB_to_BGR1(colorPtr, palette, numColors); Should we also consider the case where the client wanted the non-native swizzle? I suppose we read the colors before that (and even could, if they call a second time). https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:127: for (int i = 0; i < numColorsWithAlpha; i++) { On 2016/01/28 02:06:45, msarett wrote: > On 2016/01/28 01:26:18, msarett wrote: > > I don't SIMD premuls will do much here for two reasons. > > > > (1) We need to touch each pixel anyway to set a new alpha value. > > (2) I would guess that numColorsWithAlpha is often "1" or small. > > Maybe we should check if alpha is zero (which it will often be) before > multiplying? > > Maybe we ought to call SkMulDiv255? SkPremultiplyARGBInline does. It also calls SkMulDiv255Round. I'm in favor of being consistent. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:128: // Note that we inline this because we need to make sure that we don't I didn't understand this comment at first. I think you're saying that you *already* did the swap, if it was necessary? I find this a little less clear than the old code, which handles the transparent colors first, then does the rest. You could use the same logic here, so you wouldn't have this weird (to me, at least) logic of "we don't need to swap this time". https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:144: // Pad the color table with the last color in the table (or black) in the case that But you forgot to include "BUGGY IMAGE WORKAROUND"! ;) I do wonder why we chose black. I guess it is arbitrary for a bad image, so it probably doesn't matter. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:238: // Tell libpng to strip 16 bit/color files down to 8 bits/color. I mentioned this in the RAW code review - would it make sense to add a swizzler option which takes 16 bit/color as an input? Perhaps this would speed up libpng's work? https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:254: png_set_packing(png_ptr); Could we instead use SkSwizzler::kIndex1 etc? https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:259: alphaType = png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS) ? So we no longer check to see how many transparent colors there are. Is that okay? Are there ever images which have a tRNS chunk but it doesn't have any transparent colors in it? Or is that a bogus/rare case that we don't care about? The end result would that such an image can no longer be decoded to 565, and we'll draw it slightly slower in 8888. If we do make this change, it would be good to state in the commit message that it's changed (and why). https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:275: png_set_expand_gray_1_2_4_to_8(png_ptr); Swizzler instead? (I'd be fine with a TODO for these, if you think it makes sense, but don't want it to hold this CL up). https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:389: // We will always recommend one the above colorTypes. one of*
Description was changed from ========== Clean up SkPngCodec BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Clean up SkPngCodec and remove maximum image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Clean up SkPngCodec and remove maximum image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support past 1.2. Using nullptr everywhere should work on all versions. (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support past 1.2. Using nullptr everywhere should work on all versions. (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support past 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support past 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support past 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Optimize reading colors into a color table WIP Perf results coming soon. (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support past 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Optimize reading colors into a color table WIP Perf results coming soon. (1) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Optimize reading colors into a color table WIP Perf results coming soon. (4) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (5) Remove FIXME for subsitute trans color libpng is already doing this for us. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Optimize reading colors into a color table WIP Perf results coming soon. (4) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (5) Remove FIXME for subsitute trans color libpng is already doing this for us. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Optimize reading colors into a color table WIP Perf results coming soon. (4) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (5) Remove FIXME for subsitute trans color libpng is already doing this for us. (6) Remove #ifdef PNG_READ_PACK_SUPPORTED We don't have a fallback, so we'll fail to compile if this isn't supported. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The commit message has been improved :). https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (left): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:38: // Helper macros On 2016/01/28 21:07:32, scroggo wrote: > On 2016/01/28 01:26:18, msarett wrote: > > IMO these don't help much. We need to be compiled 1.6+ anyway to optimize the > > filter functions. > > They were added to the original SkImageDecoder in > c1c5695f8f0cda7cbabb490d274d742ca0e26eed. I copied them over to SkCodec. > > Does removing these give us problems when using the older versions of libpng? Thanks for the reference. Looking at those notes, I actually I think they won't. If they do, Google3 should break and tell us. > Do we care? I don't :). https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:296: int64_t size = sk_64_mul(origWidth, origHeight); On 2016/01/28 21:07:32, scroggo wrote: > Looks like this got removed. Can you add that to the commit message? Done. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:307: #ifdef PNG_READ_PACK_SUPPORTED On 2016/01/28 21:07:32, scroggo wrote: > Why did you remove this? AFAICT if READ_PACK isn't supported we'll decode the PNGs wrong (and leave memory uninitialized). I think it's best to fail to compile in this case. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:571: // FIXME: do we need substituteTranspColor? Note that we cannot do it for On 2016/01/28 21:07:32, scroggo wrote: > On 2016/01/28 02:06:46, msarett wrote: > > Substituting in a "transparent color" is necessary when a transparent color is > > specified in a tRNS chunk. > > > > However, we always tell libpng to handle this by calling > > png_set_tRNS_to_alpha(png_ptr) and getting the output as RGBA. > > Good to know. This would be a good thing to specify in the commit message. Done. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:118: SkOpts::RGB_to_RGB1(colorPtr, palette, numColors); On 2016/01/28 21:07:33, scroggo wrote: > On 2016/01/28 02:06:45, msarett wrote: > > On 2016/01/28 01:26:18, msarett wrote: > > > It's a little frustrating to optimize this knowing that we are going to > > memcpy() > > > it (possibly twice I think), later on. > > > > Here we treat a pointer to the png_color struct as uint8_t*. I think this is > > safe unless png changes the implementation of the png_color struct. Currently > > it is: > > > > struct { > > uint8 red; > > uint8 green; > > uint8 blue; > > } > > Can you add these comments plus some asserts here? Something like > > #ifdef SK_DEBUG > for (int i = 0; i < numColors; i++) { > png_colorp color = &png_colorp[i]; > SkASSERT(color->red == color[0]); > ... > } > #endif > > Or do you think that's overkill? Or maybe just a static_assert that checks the > struct itself? (Not sure if that's possible.) Yes not going to worry about the memcpys() in this CL. I've added a static assert and a check for RGB order. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:120: SkOpts::RGB_to_BGR1(colorPtr, palette, numColors); On 2016/01/28 21:07:33, scroggo wrote: > Should we also consider the case where the client wanted the non-native swizzle? > I suppose we read the colors before that (and even could, if they call a second > time). I'm not sure I completely understand. I think SkCodec as a whole ought to support the non-native swizzle. I don't think this code makes that issue any better or worse. Same as before, we just always provide the native swizzle. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:127: for (int i = 0; i < numColorsWithAlpha; i++) { On 2016/01/28 21:07:32, scroggo wrote: > On 2016/01/28 02:06:45, msarett wrote: > > On 2016/01/28 01:26:18, msarett wrote: > > > I don't SIMD premuls will do much here for two reasons. > > > > > > (1) We need to touch each pixel anyway to set a new alpha value. > > > (2) I would guess that numColorsWithAlpha is often "1" or small. > > > > Maybe we should check if alpha is zero (which it will often be) before > > multiplying? > > > > Maybe we ought to call SkMulDiv255? > > SkPremultiplyARGBInline does. It also calls SkMulDiv255Round. I'm in favor of > being consistent. Changed to use SkMulDiv255Round. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:128: // Note that we inline this because we need to make sure that we don't On 2016/01/28 21:07:32, scroggo wrote: > I didn't understand this comment at first. I think you're saying that you > *already* did the swap, if it was necessary? > > I find this a little less clear than the old code, which handles the transparent > colors first, then does the rest. You could use the same logic here, so you > wouldn't have this weird (to me, at least) logic of "we don't need to swap this > time". Yes this is unclear. I'll fix the comment for now, and I'll come back to the other stuff. I changed the order because I think it *should* be faster this way. But now I'm realizing that Android does weird things when they encode their PNGs, and I'd like to fix that first. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:144: // Pad the color table with the last color in the table (or black) in the case that On 2016/01/28 21:07:32, scroggo wrote: > But you forgot to include "BUGGY IMAGE WORKAROUND"! ;) > > I do wonder why we chose black. I guess it is arbitrary for a bad image, so it > probably doesn't matter. Agreed. I almost removed the black part completely, but it wasn't clear to me that libpng checked for zero sized palettes, so I figured we should keep being safe. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:238: // Tell libpng to strip 16 bit/color files down to 8 bits/color. On 2016/01/28 21:07:33, scroggo wrote: > I mentioned this in the RAW code review - would it make sense to add a swizzler > option which takes 16 bit/color as an input? Perhaps this would speed up > libpng's work? We could certainly handle this case faster in the swizzler. It's low priority since I think these images are rare. So for now, we let libpng handle it. However, if this would also help RAW, we should consider it more seriously. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:254: png_set_packing(png_ptr); On 2016/01/28 21:07:32, scroggo wrote: > Could we instead use SkSwizzler::kIndex1 etc? Yes. Adding a TODO. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:259: alphaType = png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS) ? On 2016/01/28 21:07:32, scroggo wrote: > So we no longer check to see how many transparent colors there are. Is that > okay? Are there ever images which have a tRNS chunk but it doesn't have any > transparent colors in it? Or is that a bogus/rare case that we don't care about? > The end result would that such an image can no longer be decoded to 565, and > we'll draw it slightly slower in 8888. > In libpng, a "valid" tRNS chunk has at least one color. So there should be no behavior change. > If we do make this change, it would be good to state in the commit message that > it's changed (and why). Yep sorry about the commit message. I think it's better now. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:275: png_set_expand_gray_1_2_4_to_8(png_ptr); On 2016/01/28 21:07:32, scroggo wrote: > Swizzler instead? (I'd be fine with a TODO for these, if you think it makes > sense, but don't want it to hold this CL up). Yes I'll add TODOs. I haven't gotten to these rarer cases yet and am unsure that we will be faster. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:389: // We will always recommend one the above colorTypes. On 2016/01/28 21:07:32, scroggo wrote: > one of* Done.
lgtm https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:120: SkOpts::RGB_to_BGR1(colorPtr, palette, numColors); On 2016/01/28 22:19:25, msarett wrote: > On 2016/01/28 21:07:33, scroggo wrote: > > Should we also consider the case where the client wanted the non-native > swizzle? > > I suppose we read the colors before that (and even could, if they call a > second > > time). > > I'm not sure I completely understand. > > I think SkCodec as a whole ought to support the non-native swizzle. I don't > think this code makes that issue any better or worse. > > Same as before, we just always provide the native swizzle. I think this is a good change, I just wanted to think ahead to when we do support the non-native swizzle. Ideally, we would decode the palette differently if they want non-native swizzle, which would imply changing this code some. (So maybe there should be a TODO here.) My caveat ("I suppose we read...", above) was that I was thinking we called this method inside the constructor, meaning we don't yet know which swizzle they want. But it looks like I was mistaken. We call it in onGetPixels/onStartScanlineDecode. So we *do* know the order they want. (On another note, we always redecode the palette on a second call to getPixels. It would probably be more efficient to skip this if we already have a color table. But that will require some extra work to make sure that we correct on a second call if the client swapped their preference.) https://codereview.chromium.org/1643623004/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:141: uint8_t b = SkMulDiv255Round(colorPtr[i] >> 16, a); Are these names misleading? It sounds like this may not actually be the blue component?
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Optimize reading colors into a color table WIP Perf results coming soon. (4) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (5) Remove FIXME for subsitute trans color libpng is already doing this for us. (6) Remove #ifdef PNG_READ_PACK_SUPPORTED We don't have a fallback, so we'll fail to compile if this isn't supported. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (4) Remove FIXME for subsitute trans color libpng is already doing this for us. (5) Remove #ifdef PNG_READ_PACK_SUPPORTED We don't have a fallback, so we'll fail to compile if this isn't supported. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
PTAL I made decodePalette() look more similar to the original design. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:120: SkOpts::RGB_to_BGR1(colorPtr, palette, numColors); On 2016/01/29 15:40:27, scroggo wrote: > On 2016/01/28 22:19:25, msarett wrote: > > On 2016/01/28 21:07:33, scroggo wrote: > > > Should we also consider the case where the client wanted the non-native > > swizzle? > > > I suppose we read the colors before that (and even could, if they call a > > second > > > time). > > > > I'm not sure I completely understand. > > > > I think SkCodec as a whole ought to support the non-native swizzle. I don't > > think this code makes that issue any better or worse. > > > > Same as before, we just always provide the native swizzle. > > I think this is a good change, I just wanted to think ahead to when we do > support the non-native swizzle. Ideally, we would decode the palette differently > if they want non-native swizzle, which would imply changing this code some. (So > maybe there should be a TODO here.) > > My caveat ("I suppose we read...", above) was that I was thinking we called this > method inside the constructor, meaning we don't yet know which swizzle they > want. But it looks like I was mistaken. We call it in > onGetPixels/onStartScanlineDecode. So we *do* know the order they want. (On > another note, we always redecode the palette on a second call to getPixels. It > would probably be more efficient to skip this if we already have a color table. > But that will require some extra work to make sure that we correct on a second > call if the client swapped their preference.) Thanks, adding a TODO. https://codereview.chromium.org/1643623004/diff/20001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:128: // Note that we inline this because we need to make sure that we don't On 2016/01/28 22:19:25, msarett wrote: > On 2016/01/28 21:07:32, scroggo wrote: > > I didn't understand this comment at first. I think you're saying that you > > *already* did the swap, if it was necessary? > > > > I find this a little less clear than the old code, which handles the > transparent > > colors first, then does the rest. You could use the same logic here, so you > > wouldn't have this weird (to me, at least) logic of "we don't need to swap > this > > time". > > Yes this is unclear. I'll fix the comment for now, and I'll come back to the > other stuff. I changed the order because I think it *should* be faster this > way. > > But now I'm realizing that Android does weird things when they encode their > PNGs, and I'd like to fix that first. I think you're right. My version was less clear and less optimal. I've changed this again to be more similar to how it was before. https://codereview.chromium.org/1643623004/diff/40001/src/codec/SkPngCodec.cpp File src/codec/SkPngCodec.cpp (right): https://codereview.chromium.org/1643623004/diff/40001/src/codec/SkPngCodec.cp... src/codec/SkPngCodec.cpp:141: uint8_t b = SkMulDiv255Round(colorPtr[i] >> 16, a); On 2016/01/29 15:40:27, scroggo wrote: > Are these names misleading? It sounds like this may not actually be the blue > component? This has been changed.
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (4) Remove FIXME for subsitute trans color libpng is already doing this for us. (5) Remove #ifdef PNG_READ_PACK_SUPPORTED We don't have a fallback, so we'll fail to compile if this isn't supported. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (4) Remove FIXME for subsitute trans color libpng is already doing this for us. (5) Remove #ifdef PNG_READ_PACK_SUPPORTED We don't have a fallback, so we'll fail to compile if this isn't supported. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ==========
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1643623004/#ps100001 (title: "Change comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1643623004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1643623004/100001
Message was sent while issue was closed.
Description was changed from ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (4) Remove FIXME for subsitute trans color libpng is already doing this for us. (5) Remove #ifdef PNG_READ_PACK_SUPPORTED We don't have a fallback, so we'll fail to compile if this isn't supported. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot ========== to ========== A variety of SkPngCodec clean-ups (1) Remove #ifdefs that extend support to > 1.2. Using nullptr everywhere should work on all versions. (2) Use png_get_valid(tRNS) to check for transparency It does the same thing we were doing previously in less work. (3) Remove image size check Clients allocate their own memory, and they have the option to perform scaled and partial decodes. So we don't need to arbitrarily fail on large images. (4) Remove FIXME for subsitute trans color libpng is already doing this for us. (5) Remove #ifdef PNG_READ_PACK_SUPPORTED We don't have a fallback, so we'll fail to compile if this isn't supported. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot Committed: https://skia.googlesource.com/skia/+/13a912354875a747aefb1f00de50eb3ec454ab1f ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://skia.googlesource.com/skia/+/13a912354875a747aefb1f00de50eb3ec454ab1f |
