Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2325223002:
Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder (Closed)
DescriptionSupport RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder
BUG=skia:5616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2325223002
Committed: https://skia.googlesource.com/skia/+/f17b71f6c9b94e93f88e103b3b6cab61cbee76d7
Patch Set 1 #
Total comments: 2
Patch Set 2 : Changed signature of transform_scanline() procs #Patch Set 3 : Fix bug flipping R and B #
Total comments: 30
Patch Set 4 : Response to comments #
Total comments: 4
Patch Set 5 : A few more fixes #Patch Set 6 : Rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 37 (26 generated)
Description was changed from ========== Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder BUG=skia: ========== to ========== Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2325223002 ==========
msarett@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/2325223002/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2325223002/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:72: { kRGBA_8888_SkColorType, kUnpremul_SkAlphaType, transform_scanline_RGBA }, can transform_scanline_RGBA be the same as memcpy?
Changed signature of transform_scanline procs. Added tests for kIndex8. https://codereview.chromium.org/2325223002/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2325223002/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:72: { kRGBA_8888_SkColorType, kUnpremul_SkAlphaType, transform_scanline_RGBA }, On 2016/09/09 21:07:37, reed1 wrote: > can transform_scanline_RGBA be the same as memcpy? Yes!
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
msarett@google.com changed reviewers: + scroggo@google.com
https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:82: for (int i = SK_ARRAY_COUNT(gMap) - 1; i >= 0; --i) { nit: Can you update this to a for each loop? https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:115: png_byte* SK_RESTRICT trans, SkAlphaType alphaType) { I'm confused. Didn't you change this method differently in crrev.com/2330053002 ? https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:205: SkAlphaType alphaType = bitmap->alphaType(); nit: could be const? https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... File src/images/transform_scanline.h (right): https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:19: * Function template for transforming scanlines. This should probably state what "bpp" means. I'm guessing it's bits or bytes per pixel. Is that the source or the dest? (Looks like source?) https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:23: typedef void (*transform_scanline_proc)(uint8_t* dst, const void* src, int width, int bpp); Just out of curiosity, why did you switch to uint8_t? (It seems like we are generally inconsistent, and sometimes use char. Maybe one is better, but I do want to make sure we don't change a particular use back and forth, even though we'll probably have the bad choice used in various places.) Also, I asked this elsewhere, but why the removal of SK_RESTRICT? https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:51: static void transform_scanline_RGB1(uint8_t* dst, const void* src, int width, int) { How'd you choose the "1" in the name? (I might be tempted to call it RGBX, since we ignore the alpha.) https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:68: SkPMColor c = *srcP++; One of these uses of SkPMColor is a lie, but I suppose we don't know which one unless we look at the pm color macro... https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:125: unsigned r = (c >> 16) & 0xFF; Can we share code between these two methods (maybe with a template)? I think only this block is different? https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp File tests/CodecTest.cpp (right): https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1105: SkCodec::Result result = origCodec->getPixels(info, bm1.getPixels(), bm1.rowBytes(), nullptr, Won't this leave colorTable (which the bitmap points to) uninitialized? Is this tested on an index8 bitmap? https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1119: result = codec->getPixels(info, bm2.getPixels(), bm2.rowBytes(), nullptr, colors, &numColors); Once again, bm2's colorTable will remain uninitialized. I'm guessing the only reason the md5s match down below is because they are using the same uninitialized colorTable. (But I would hope that one of our *SAN bots would catch it?) https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1134: SkImageInfo info565 = codec->getInfo().makeColorType(kRGB_565_SkColorType); nit: Can you fold these three into one for loop? Something like: for (ct : { kRGB_565_SkColorType, kRGBA_8888_SkColorType, kBGRA_8888_SkColorType }) { SkImageInfo info = codec->getInfo().makeColorType(ct); check_round_trip(r, codec.get(), info); } https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1155: // Test RGBA/BGRA with alpha Can this be another for loop? https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1158: .makeColorSpace(nullptr); I'm guessing the color space needs to be removed because of premultiplication? https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1181: //SkImageInfo infoIndex8Premul = codec->getInfo().makeAlphaType(kPremul_SkAlphaType) Comment explaining why this one is commented out? (Also, this could be another for loop?)
Description was changed from ========== Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2325223002 ========== to ========== Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2325223002 ==========
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:82: for (int i = SK_ARRAY_COUNT(gMap) - 1; i >= 0; --i) { On 2016/09/12 17:16:49, scroggo wrote: > nit: Can you update this to a for each loop? Done. https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:115: png_byte* SK_RESTRICT trans, SkAlphaType alphaType) { On 2016/09/12 17:16:50, scroggo wrote: > I'm confused. Didn't you change this method differently in crrev.com/2330053002 > ? Sorry I've sent my code reviews backwards. That CL is based on this CL. The changes to pack_palette() here aren't super interesting since they will be overwritten anyway. https://codereview.chromium.org/2325223002/diff/40001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:205: SkAlphaType alphaType = bitmap->alphaType(); On 2016/09/12 17:16:50, scroggo wrote: > nit: could be const? Done. https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... File src/images/transform_scanline.h (right): https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:19: * Function template for transforming scanlines. On 2016/09/12 17:16:50, scroggo wrote: > This should probably state what "bpp" means. I'm guessing it's bits or bytes per > pixel. Is that the source or the dest? (Looks like source?) Done. It is dst bytesPerPixel, though src would be fine as well. https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:23: typedef void (*transform_scanline_proc)(uint8_t* dst, const void* src, int width, int bpp); On 2016/09/12 17:16:50, scroggo wrote: > Just out of curiosity, why did you switch to uint8_t? (It seems like we are > generally inconsistent, and sometimes use char. Maybe one is better, but I do > want to make sure we don't change a particular use back and forth, even though > we'll probably have the bad choice used in various places.) > > Also, I asked this elsewhere, but why the removal of SK_RESTRICT? You're right, back to char and SK_RESTRICT. https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:51: static void transform_scanline_RGB1(uint8_t* dst, const void* src, int width, int) { On 2016/09/12 17:16:50, scroggo wrote: > How'd you choose the "1" in the name? (I might be tempted to call it RGBX, since > we ignore the alpha.) I think RGBX is a better name, switching. https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:68: SkPMColor c = *srcP++; On 2016/09/12 17:16:50, scroggo wrote: > One of these uses of SkPMColor is a lie, but I suppose we don't know which one > unless we look at the pm color macro... Let's use uint32_t... https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:125: unsigned r = (c >> 16) & 0xFF; On 2016/09/12 17:16:50, scroggo wrote: > Can we share code between these two methods (maybe with a template)? I think > only this block is different? SGTM https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp File tests/CodecTest.cpp (right): https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1105: SkCodec::Result result = origCodec->getPixels(info, bm1.getPixels(), bm1.rowBytes(), nullptr, On 2016/09/12 17:16:50, scroggo wrote: > Won't this leave colorTable (which the bitmap points to) uninitialized? > > Is this tested on an index8 bitmap? Yes, it's just irrelevant because the digest comparison will still work. https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1119: result = codec->getPixels(info, bm2.getPixels(), bm2.rowBytes(), nullptr, colors, &numColors); On 2016/09/12 17:16:50, scroggo wrote: > Once again, bm2's colorTable will remain uninitialized. I'm guessing the only > reason the md5s match down below is because they are using the same > uninitialized colorTable. (But I would hope that one of our *SAN bots would > catch it?) I had thought that the md5 would be comparing the indices (and the not the color table itself). Still, I'll fix this anyway. https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1134: SkImageInfo info565 = codec->getInfo().makeColorType(kRGB_565_SkColorType); On 2016/09/12 17:16:50, scroggo wrote: > nit: Can you fold these three into one for loop? Something like: > > for (ct : { kRGB_565_SkColorType, kRGBA_8888_SkColorType, kBGRA_8888_SkColorType > }) { > SkImageInfo info = codec->getInfo().makeColorType(ct); > check_round_trip(r, codec.get(), info); > } Done. https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1155: // Test RGBA/BGRA with alpha On 2016/09/12 17:16:50, scroggo wrote: > Can this be another for loop? Done. https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1158: .makeColorSpace(nullptr); On 2016/09/12 17:16:50, scroggo wrote: > I'm guessing the color space needs to be removed because of premultiplication? Yes, uggh... https://codereview.chromium.org/2325223002/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1181: //SkImageInfo infoIndex8Premul = codec->getInfo().makeAlphaType(kPremul_SkAlphaType) On 2016/09/12 17:16:50, scroggo wrote: > Comment explaining why this one is commented out? (Also, this could be another > for loop?) Because I forgot to comment it back in :).
lgtm https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... File src/images/transform_scanline.h (right): https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:19: * Function template for transforming scanlines. On 2016/09/12 19:39:20, msarett wrote: > On 2016/09/12 17:16:50, scroggo wrote: > > This should probably state what "bpp" means. I'm guessing it's bits or bytes > per > > pixel. Is that the source or the dest? (Looks like source?) > > Done. It is dst bytesPerPixel, though src would be fine as well. It looks like you pass the src's bytes per pixel. But this parameter is ignored, except in the case of memcpy, where they are the same. (In some other cases the bytes per pixel do not match, for example when we are ignoring the alpha byte.) https://codereview.chromium.org/2325223002/diff/120001/src/images/SkPNGImageE... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2325223002/diff/120001/src/images/SkPNGImageE... src/images/SkPNGImageEncoder.cpp:332: SkColorTable* ct = bitmap.getColorTable(); Not your doing, but this variable hides the SkColorType ct, and threw me off for a second. https://codereview.chromium.org/2325223002/diff/120001/tests/CodecTest.cpp File tests/CodecTest.cpp (right): https://codereview.chromium.org/2325223002/diff/120001/tests/CodecTest.cpp#ne... tests/CodecTest.cpp:1107: &numColors); numColors gets updated, but colorTable1's count does not :( I don't think this causes any problems though...
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... File src/images/transform_scanline.h (right): https://codereview.chromium.org/2325223002/diff/40001/src/images/transform_sc... src/images/transform_scanline.h:19: * Function template for transforming scanlines. On 2016/09/12 19:54:31, scroggo wrote: > On 2016/09/12 19:39:20, msarett wrote: > > On 2016/09/12 17:16:50, scroggo wrote: > > > This should probably state what "bpp" means. I'm guessing it's bits or bytes > > per > > > pixel. Is that the source or the dest? (Looks like source?) > > > > Done. It is dst bytesPerPixel, though src would be fine as well. > > It looks like you pass the src's bytes per pixel. > > But this parameter is ignored, except in the case of memcpy, where they are the > same. (In some other cases the bytes per pixel do not match, for example when we > are ignoring the alpha byte.) Done, thanks https://codereview.chromium.org/2325223002/diff/120001/src/images/SkPNGImageE... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2325223002/diff/120001/src/images/SkPNGImageE... src/images/SkPNGImageEncoder.cpp:332: SkColorTable* ct = bitmap.getColorTable(); On 2016/09/12 19:54:31, scroggo wrote: > Not your doing, but this variable hides the SkColorType ct, and threw me off for > a second. Fixing this... https://codereview.chromium.org/2325223002/diff/120001/tests/CodecTest.cpp File tests/CodecTest.cpp (right): https://codereview.chromium.org/2325223002/diff/120001/tests/CodecTest.cpp#ne... tests/CodecTest.cpp:1107: &numColors); On 2016/09/12 19:54:31, scroggo wrote: > numColors gets updated, but colorTable1's count does not :( > > I don't think this causes any problems though... Adding a comment. I think the real fix is dangerous_overwriteColors(), but I don't want to friend class SkColorTable for this test case when we don't really need to...
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) Build-Ubuntu-Clang-arm-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-arm64-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-a...) Build-Ubuntu-Clang-mips64el-Debug-GN_Android-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-m...) Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...) Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...) Build-Win-MSVC-x86_64-Release-Vulkan-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2325223002 ========== to ========== Support RGBA/BGRA Premul/Unpremul from SkPNGImageEncoder BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2325223002 Committed: https://skia.googlesource.com/skia/+/f17b71f6c9b94e93f88e103b3b6cab61cbee76d7 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001) as https://skia.googlesource.com/skia/+/f17b71f6c9b94e93f88e103b3b6cab61cbee76d7 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
