Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2212563003:
Modify SkPngCodec to recognize 565 images from the sBIT chunk (Closed)
DescriptionModify SkPngCodec to recognize 565 images from the sBIT chunk
Conveniently, SkPngImageEncoder already writes the sBIT chunk
appropriately.
BUG=skia:5616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003
Committed: https://skia.googlesource.com/skia/+/549ca329cfbd0ffaa436c7208a67d9af8b2e6e7d
Patch Set 1 #Patch Set 2 : Don't even try on CG and WIC #Patch Set 3 : Clean up test #
Total comments: 4
Patch Set 4 : Rebase #Patch Set 5 : Enable test on all platforms #Patch Set 6 : Fix for dumb compiler #
Total comments: 2
Patch Set 7 : Allow SkPngCodec to set SkImageInfo directly #
Depends on Patchset: Messages
Total messages: 39 (23 generated)
Description was changed from ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. Will be interesting to see how this works with CG. This will not work with WIC. BUG=skia: ========== to ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. Will be interesting to see how this works with CG. This will not work with WIC. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ==========
Description was changed from ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. Will be interesting to see how this works with CG. This will not work with WIC. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ========== to ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. This will not work on WIC or CG. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ==========
msarett@google.com changed reviewers: + mtklein@google.com, reed@google.com
https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncoded... File include/codec/SkEncodedInfo.h (left): https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncoded... include/codec/SkEncodedInfo.h:108: default: No need for defaults anymore? https://codereview.chromium.org/2212563003/diff/40001/tests/CodecTest.cpp File tests/CodecTest.cpp (right): https://codereview.chromium.org/2212563003/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1049: // We need to disable this test on Mac and Windows because CG and WIC do not encode 565 as 565. encode -> tag? We can't ask them to nicely can we? Is this feature worth having if it only works on Linux?
https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncoded... File include/codec/SkEncodedInfo.h (left): https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncoded... include/codec/SkEncodedInfo.h:108: default: On 2016/08/04 17:52:51, mtklein wrote: > No need for defaults anymore? Yeah, all values of the enum are handled. This used to not be true, before I deleted kUnknown. https://codereview.chromium.org/2212563003/diff/40001/tests/CodecTest.cpp File tests/CodecTest.cpp (right): https://codereview.chromium.org/2212563003/diff/40001/tests/CodecTest.cpp#new... tests/CodecTest.cpp:1049: // We need to disable this test on Mac and Windows because CG and WIC do not encode 565 as 565. On 2016/08/04 17:52:51, mtklein wrote: > encode -> tag? > > We can't ask them to nicely can we? Is this feature worth having if it only > works on Linux? I would be in favor of using our encoder as the default on Mac/Win instead of CG/WIC. But then I guess we have to keep around the CG/WIC studd for some clients (ex: Adobe).
mtklein@google.com changed reviewers: + hcm@google.com
That's probably another good thing to check in with them about. Last I recalled they were interested in WIC/CG image decoding... not clear to me about encoding.
On 2016/08/04 19:22:10, mtklein wrote: > That's probably another good thing to check in with them about. Last I recalled > they were interested in WIC/CG image decoding... not clear to me about encoding. IIRC they do use SkImageEncoder (the CG implementation). I still think we can keep SkImageEncoderCG but have the option to choose CG or our regular encoder.
Patchset #5 (id:80001) has been deleted
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-Release-Shared-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 msarett@google.com
Description was changed from ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. This will not work on WIC or CG. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ========== to ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ==========
On 2016/08/04 19:33:46, msarett wrote: > On 2016/08/04 19:22:10, mtklein wrote: > > That's probably another good thing to check in with them about. Last I > recalled > > they were interested in WIC/CG image decoding... not clear to me about > encoding. > > IIRC they do use SkImageEncoder (the CG implementation). > > I still think we can keep SkImageEncoderCG but have the option to choose CG or > our regular encoder. Please take a look. We've switched to SkPNGImageEncoder on Mac and Win. I think this change looks a lot better now that it works on all platforms.
lgtm
Description was changed from ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ========== to ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: skia_presubmit-Trybot on master.client.skia.fyi (JOB_FAILED, http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)
Oops, forgot I need an API review. reed@ can you take a look?
https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncode... File include/codec/SkEncodedInfo.h (right): https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncode... include/codec/SkEncodedInfo.h:183: case k565_Color: What is the interpretation of bytsPerPixel() in this class? decompressed size?
https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncode... File include/codec/SkEncodedInfo.h (right): https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncode... include/codec/SkEncodedInfo.h:183: case k565_Color: On 2016/08/17 13:13:06, reed1 wrote: > What is the interpretation of bytsPerPixel() in this class? decompressed size? Encoded size. In this case, even though we recognize that the image is 565, the encoded bits per pixel are 3*8. Because the png is encoded with 8-bit channels.
I've backed out the change to SkEncodedInfo. Now SkPngCodec can set the recommended SkImageInfo directly. I think this looks better.
lgtm
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.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2212563003/#ps140001 (title: "Allow SkPngCodec to set SkImageInfo directly")
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 ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 ========== to ========== Modify SkPngCodec to recognize 565 images from the sBIT chunk Conveniently, SkPngImageEncoder already writes the sBIT chunk appropriately. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2212563003 Committed: https://skia.googlesource.com/skia/+/549ca329cfbd0ffaa436c7208a67d9af8b2e6e7d ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://skia.googlesource.com/skia/+/549ca329cfbd0ffaa436c7208a67d9af8b2e6e7d |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
