|
|
DescriptionFix WIC encoder to support kJPEG_Type
(1) Add support for kJPEG to WIC
(2) Add encoding test.
(3) Turn on WIC jpeg encoder on Windows and CG jpeg
encoder on Mac.
A follow-up may make Skia's encoders the default on all
platforms. But, in order to do that, I think we need
to write better encoding unit tests for CG and WIC.
BUG=skia:3969
BUG=skia:5632
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002
Committed: https://skia.googlesource.com/skia/+/b3a7ef1fc0adc24859d2498aee54d3ec2cbcac3a
Committed: https://skia.googlesource.com/skia/+/36c38cbb29744e0b5390a38367e47c0c74287c2d
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add gm instead of test #
Total comments: 4
Patch Set 3 : Fix gm message #Patch Set 4 : Bug fix #
Dependent Patchsets: Messages
Total messages: 50 (38 generated)
Description was changed from ========== Fix wic BUG=skia: ========== to ========== Fix wic BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 ==========
Description was changed from ========== Fix wic BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 ========== to ========== Fix WIC encoder to support kJPEG_Type BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 ==========
Patchset #1 (id:1) 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: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Fix WIC encoder to support kJPEG_Type BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 ========== to ========== Fix WIC encoder to support kJPEG_Type (1) Add support for kJPEG to WIC (2) Add encoding test. (3) Turn on WIC jpeg encoder on Windows and CG jpeg encoder on Mac. A follow-up may make Skia's encoders the default on all platforms. But, in order to do that, I think we need to write better encoding unit tests for CG and WIC. BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 ==========
msarett@google.com changed reviewers: + mtklein@google.com
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp File tests/EncodeTest.cpp (right): https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp#ne... tests/EncodeTest.cpp:14: return SkTAbs((int) a - (int) b) <= 30; That's a pretty generous definition of almost equal... https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp#ne... tests/EncodeTest.cpp:22: sk_sp<SkData> jpegData(SkImageEncoder::EncodeData(orig, SkImageEncoder::kJPEG_Type, 100)); Does the quality setting cause us to go down any different code paths when encoding a JPEG? That is, should we test more quality settings? https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp#ne... tests/EncodeTest.cpp:40: REPORTER_ASSERT(r, almost_equal(SkGetPackedA32(pngPixel), SkGetPackedA32(jpegPixel))); Sort of silly to test JPEG's alpha?
Patchset #2 (id:60001) 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...
Changed unit test to a gm. We can inspect the encoder correctness visually. https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp File tests/EncodeTest.cpp (right): https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp#ne... tests/EncodeTest.cpp:14: return SkTAbs((int) a - (int) b) <= 30; On 2016/08/15 15:11:37, mtklein_C wrote: > That's a pretty generous definition of almost equal... Acknowledged. https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp#ne... tests/EncodeTest.cpp:22: sk_sp<SkData> jpegData(SkImageEncoder::EncodeData(orig, SkImageEncoder::kJPEG_Type, 100)); On 2016/08/15 15:11:37, mtklein_C wrote: > Does the quality setting cause us to go down any different code paths when > encoding a JPEG? That is, should we test more quality settings? The jpeg library handles the quality, so the code path is the same for us. I think this is fine. https://codereview.chromium.org/2245453002/diff/40001/tests/EncodeTest.cpp#ne... tests/EncodeTest.cpp:40: REPORTER_ASSERT(r, almost_equal(SkGetPackedA32(pngPixel), SkGetPackedA32(jpegPixel))); On 2016/08/15 15:11:37, mtklein_C wrote: > Sort of silly to test JPEG's alpha? Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2245453002/diff/80001/gm/encode.cpp File gm/encode.cpp (right): https://codereview.chromium.org/2245453002/diff/80001/gm/encode.cpp#newcode39 gm/encode.cpp:39: const char text[] = "Images should look identical."; ...more-or-less the same. ?
https://codereview.chromium.org/2245453002/diff/80001/gm/encode.cpp File gm/encode.cpp (right): https://codereview.chromium.org/2245453002/diff/80001/gm/encode.cpp#newcode40 gm/encode.cpp:40: canvas->drawText(text, sizeof(text) - 1, 450.0f, 550.0f, SkPaint()); It can be nice to call sk_tool_utils::set_portable_typeface() to make more GMs similar to each other.
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/2245453002/diff/80001/gm/encode.cpp File gm/encode.cpp (right): https://codereview.chromium.org/2245453002/diff/80001/gm/encode.cpp#newcode39 gm/encode.cpp:39: const char text[] = "Images should look identical."; On 2016/08/15 19:50:53, mtklein wrote: > ...more-or-less the same. ? Done. https://codereview.chromium.org/2245453002/diff/80001/gm/encode.cpp#newcode40 gm/encode.cpp:40: canvas->drawText(text, sizeof(text) - 1, 450.0f, 550.0f, SkPaint()); On 2016/08/15 19:54:11, mtklein wrote: > It can be nice to call sk_tool_utils::set_portable_typeface() to make more GMs > similar to each other. Done.
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/2245453002/#ps140001 (title: "Fix gm message")
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 ========== Fix WIC encoder to support kJPEG_Type (1) Add support for kJPEG to WIC (2) Add encoding test. (3) Turn on WIC jpeg encoder on Windows and CG jpeg encoder on Mac. A follow-up may make Skia's encoders the default on all platforms. But, in order to do that, I think we need to write better encoding unit tests for CG and WIC. BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 ========== to ========== Fix WIC encoder to support kJPEG_Type (1) Add support for kJPEG to WIC (2) Add encoding test. (3) Turn on WIC jpeg encoder on Windows and CG jpeg encoder on Mac. A follow-up may make Skia's encoders the default on all platforms. But, in order to do that, I think we need to write better encoding unit tests for CG and WIC. BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 Committed: https://skia.googlesource.com/skia/+/b3a7ef1fc0adc24859d2498aee54d3ec2cbcac3a ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://skia.googlesource.com/skia/+/b3a7ef1fc0adc24859d2498aee54d3ec2cbcac3a
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:140001) has been created in https://codereview.chromium.org/2246203002/ by msarett@google.com. The reason for reverting is: Broken Windows bot..
Message was sent while issue was closed.
Description was changed from ========== Fix WIC encoder to support kJPEG_Type (1) Add support for kJPEG to WIC (2) Add encoding test. (3) Turn on WIC jpeg encoder on Windows and CG jpeg encoder on Mac. A follow-up may make Skia's encoders the default on all platforms. But, in order to do that, I think we need to write better encoding unit tests for CG and WIC. BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 Committed: https://skia.googlesource.com/skia/+/b3a7ef1fc0adc24859d2498aee54d3ec2cbcac3a ========== to ========== Fix WIC encoder to support kJPEG_Type (1) Add support for kJPEG to WIC (2) Add encoding test. (3) Turn on WIC jpeg encoder on Windows and CG jpeg encoder on Mac. A follow-up may make Skia's encoders the default on all platforms. But, in order to do that, I think we need to write better encoding unit tests for CG and WIC. BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 Committed: https://skia.googlesource.com/skia/+/b3a7ef1fc0adc24859d2498aee54d3ec2cbcac3a ==========
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.
Patchset #4 (id:160001) 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: 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/2245453002/#ps180001 (title: "Bug fix")
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 ========== Fix WIC encoder to support kJPEG_Type (1) Add support for kJPEG to WIC (2) Add encoding test. (3) Turn on WIC jpeg encoder on Windows and CG jpeg encoder on Mac. A follow-up may make Skia's encoders the default on all platforms. But, in order to do that, I think we need to write better encoding unit tests for CG and WIC. BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 Committed: https://skia.googlesource.com/skia/+/b3a7ef1fc0adc24859d2498aee54d3ec2cbcac3a ========== to ========== Fix WIC encoder to support kJPEG_Type (1) Add support for kJPEG to WIC (2) Add encoding test. (3) Turn on WIC jpeg encoder on Windows and CG jpeg encoder on Mac. A follow-up may make Skia's encoders the default on all platforms. But, in order to do that, I think we need to write better encoding unit tests for CG and WIC. BUG=skia:3969 BUG=skia:5632 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2245453002 Committed: https://skia.googlesource.com/skia/+/b3a7ef1fc0adc24859d2498aee54d3ec2cbcac3a Committed: https://skia.googlesource.com/skia/+/36c38cbb29744e0b5390a38367e47c0c74287c2d ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://skia.googlesource.com/skia/+/36c38cbb29744e0b5390a38367e47c0c74287c2d |