Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(952)

Issue 2212563003: Modify SkPngCodec to recognize 565 images from the sBIT chunk (Closed)

Created:
4 years, 1 month ago by msarett
Modified:
4 years, 1 month ago
Reviewers:
hcm, mtklein, reed1
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

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

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -21 lines) Patch
M include/codec/SkCodec.h View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
M src/codec/SkCodec.cpp View 1 2 3 4 5 6 1 chunk +12 lines, -0 lines 0 comments Download
M src/codec/SkPngCodec.h View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M src/codec/SkPngCodec.cpp View 1 2 3 4 5 6 3 chunks +29 lines, -19 lines 0 comments Download
M tests/CodecTest.cpp View 1 2 3 4 2 chunks +32 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 39 (23 generated)
msarett
4 years, 1 month ago (2016-08-04 17:37:25 UTC) #4
mtklein
https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncodedInfo.h File include/codec/SkEncodedInfo.h (left): https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncodedInfo.h#oldcode108 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 ...
4 years, 1 month ago (2016-08-04 17:52:51 UTC) #5
msarett
https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncodedInfo.h File include/codec/SkEncodedInfo.h (left): https://codereview.chromium.org/2212563003/diff/40001/include/codec/SkEncodedInfo.h#oldcode108 include/codec/SkEncodedInfo.h:108: default: On 2016/08/04 17:52:51, mtklein wrote: > No need ...
4 years, 1 month ago (2016-08-04 19:04:10 UTC) #6
mtklein
That's probably another good thing to check in with them about. Last I recalled they ...
4 years, 1 month ago (2016-08-04 19:22:10 UTC) #8
msarett
On 2016/08/04 19:22:10, mtklein wrote: > That's probably another good thing to check in with ...
4 years, 1 month ago (2016-08-04 19:33:46 UTC) #9
msarett
On 2016/08/04 19:33:46, msarett wrote: > On 2016/08/04 19:22:10, mtklein wrote: > > That's probably ...
4 years, 1 month ago (2016-08-17 02:04:10 UTC) #19
mtklein
lgtm
4 years, 1 month ago (2016-08-17 12:49:36 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2212563003/120001
4 years, 1 month ago (2016-08-17 12:55:45 UTC) #23
commit-bot: I haz the power
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/builds/12691)
4 years, 1 month ago (2016-08-17 12:57:45 UTC) #25
msarett
Oops, forgot I need an API review. reed@ can you take a look?
4 years, 1 month ago (2016-08-17 13:09:49 UTC) #26
reed1
https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncodedInfo.h File include/codec/SkEncodedInfo.h (right): https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncodedInfo.h#newcode183 include/codec/SkEncodedInfo.h:183: case k565_Color: What is the interpretation of bytsPerPixel() in ...
4 years, 1 month ago (2016-08-17 13:13:06 UTC) #27
msarett
https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncodedInfo.h File include/codec/SkEncodedInfo.h (right): https://codereview.chromium.org/2212563003/diff/120001/include/codec/SkEncodedInfo.h#newcode183 include/codec/SkEncodedInfo.h:183: case k565_Color: On 2016/08/17 13:13:06, reed1 wrote: > What ...
4 years, 1 month ago (2016-08-17 13:17:35 UTC) #28
msarett
I've backed out the change to SkEncodedInfo. Now SkPngCodec can set the recommended SkImageInfo directly. ...
4 years, 1 month ago (2016-08-17 13:53:44 UTC) #29
reed1
lgtm
4 years, 1 month ago (2016-08-17 14:01:37 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2212563003/140001
4 years, 1 month ago (2016-08-17 15:53:11 UTC) #37
commit-bot: I haz the power
4 years, 1 month ago (2016-08-17 15:54:12 UTC) #39
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as
https://skia.googlesource.com/skia/+/549ca329cfbd0ffaa436c7208a67d9af8b2e6e7d

Powered by Google App Engine
This is Rietveld 408576698