Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716533002:
[PNGImageDecoder] Clean up readColorSpace, TODOs, cruft (Closed)
|
Created:
3 years, 10 months ago by Noel Gordon Modified:
3 years, 9 months ago CC:
chromium-reviews, blink-reviews, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[PNGImageDecoder] Clean up readColorSpace, TODOs, cruft
inline sk_sp<SkColorSpace> readColorSpace
Prefer the early return to prevent the code from dancing off to the
right of the page. Clarify comment about cHRM+gAMA: we should never
guess sRGB if there is no gAMA, that could be just as wrong. Images
with no profile could be assumed to be sRGB but it's not the job of
this lower-layer code to ever decide this matter: a system run-time
flag or similar should be used.
PNGImageDecoder::headerAvailable
Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible
to bool and also move it into the callee, remove TODOs done or that
aren't worth doing, add TODO about GRAY CMYK profiles, state why we
"Update our info now", use DCHECK instead of ASSERT, and remove all
unused transparency variables.
PNGImageDecoder::rowAvailable
Fix up the blink formatting of our libpng comments, ditch a C-style
cast per style, move the alpha tracking code into the associated if
branch to match an upcoming change to this code.
No change in behavior, no new tests.
BUG=439655
Review-Url: https://codereview.chromium.org/2716533002
Cr-Commit-Position: refs/heads/master@{#452553}
Committed: https://chromium.googlesource.com/chromium/src/+/1a37300047e6ddefeb685d021e353ed08e3caffc
Patch Set 1 #Patch Set 2 : Moar cleaning, ASSERT is DCHECK. #
Total comments: 16
Patch Set 3 : Remove unused transparency variables. #Messages
Total messages: 40 (25 generated)
The CQ bit was checked by noel@chromium.org 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.
Description was changed from ========== [PNGDecoder] Clean up readColorSpace BUG= ========== to ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft BUG= ==========
The CQ bit was checked by noel@chromium.org 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...
Description was changed from ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft BUG= ========== to ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the caller, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. BUG= ==========
noel@chromium.org changed reviewers: + scroggo@chromium.org
noel@chromium.org changed reviewers: + msarett@chromium.org
The came up in connection with the APNG patch. We should remove old cruft and TODO to simplify the APNG patch review.
Description was changed from ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the caller, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. BUG= ========== to ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. BUG= ========== to ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 ==========
https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:173: // transfer function is not necessarily a 2.2 exponential. Here be dragons for reference. The sad story of PNG and gamma [1] and the story of the Apple and the Pear [2]. Perhaps let sleeping dogs lie. [1] https://hsivonen.fi/png-gamma/ [2] https://bugs.chromium.org/p/chromium/issues/detail?id=517481 https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:177: // and do a full color space transformation if it is present. Fixed a while back. Though the Android pixels results seem to be back-the front (R swapped with B), started when color correction was enabled for Android. I don't know what's going one there.
https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:104: // However, the current behavior matches Safari and Firefox. "... that unmarked PNGs should be treated as sRGB" is a matter for some higher-layer to tell us, via a blink config flag or similar. We should not try to guess about profiles, nor try to patch over their bogus-ness, we should just deem them invalid. There is only one exception to this that know of: when a color profile has degenerate gamma curves. Refer to https://crbg.com/492379
code lgtm I have nits about the comments, but those are of secondary importance. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:104: // However, the current behavior matches Safari and Firefox. On 2017/02/23 10:53:41, noel gordon wrote: > "... that unmarked PNGs should be treated as sRGB" is a matter for some > higher-layer to tell us, via a blink config flag or similar. > > We should not try to guess about profiles, nor try to patch over their > bogus-ness, we should just deem them invalid. > > There is only one exception to this that know of: when a color profile has > degenerate gamma curves. Refer to https://crbg.com/492379 I don't completely agree. It is widely accepted that untagged images should be treated as sRGB - this is how Safari behaves and how Chrome intends to behave. Images with only cHRM are a strange case - I would argue that they are not "invalid". Right now, we ignore the tags, which seems fine. When we start treating untagged images as sRGB, these would naturally be treated as sRGB as well. This, to me, seems worse than at least paying attention to the gamut specified by the cHRM. Of course, if our goal is to match Safari, I understand that as well. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:173: // transfer function is not necessarily a 2.2 exponential. On 2017/02/23 10:43:56, noel gordon wrote: > Here be dragons for reference. The sad story of PNG and gamma [1] and the story > of the Apple and the Pear [2]. Perhaps let sleeping dogs lie. > > [1] https://hsivonen.fi/png-gamma/ > [2] https://bugs.chromium.org/p/chromium/issues/detail?id=517481 This comment seems valid to me? [1] seems to point out that browsers handle gamma wrong - which is a problem that we can/should fix. [2] the apple/pear image "works" regardless of whether we correct to 2.2 or the monitor gamma. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:177: // and do a full color space transformation if it is present. On 2017/02/23 10:43:56, noel gordon wrote: > Fixed a while back. Though the Android pixels results seem to be back-the front > (R swapped with B), started when color correction was enabled for Android. I > don't know what's going one there. Yes this comment is out of date, thanks for removing. Not sure what you're referring to with Android, but that seems like a serious issue. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:157: // TODO(msarret): Add GRAY, CMYK profile support? nit: msarett nit: CMYK doesn't seem relevant here since we can't get it from a PNG. FWIW, Gray support is ready. Thanks for the reminder.
https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:177: // and do a full color space transformation if it is present. On 2017/02/23 14:07:03, msarett1 wrote: > On 2017/02/23 10:43:56, noel gordon wrote: > > Fixed a while back. Though the Android pixels results seem to be back-the > front > > (R swapped with B), started when color correction was enabled for Android. I > > don't know what's going one there. > Yes this comment is out of date, thanks for removing. > > Not sure what you're referring to with Android, but that seems like a serious > issue. Oops, I see the bug now. Thanks.
On 2017/02/23 07:11:13, noel gordon wrote: > The came up in connection with the APNG patch. We should remove old cruft and > TODO to simplify the APNG patch review. +1. My drafts in APNG suggest moving some of this to a separate CL. I plan to have another patch set of APNG ready for review today. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:144: png_get_tRNS(png, info, &trns, &trnsCount, 0); Another issue you brought up in the APNG patch - trns and trnsCount are never used. trnsCount was used before [1], but trns was not used when it was introduced [2]. And if we remove those, there is no reason to call png_get_tRNS, which looks to just return values that we ignore [3]. Or would you prefer to land that as a separate CL? [1] crrev.com/2426723005 [2] https://chromium.googlesource.com/chromium/src/+/1861a9cf28b51732a362539a36c3... [3] https://cs.chromium.org/chromium/src/third_party/libpng/pngget.c?q=png_get_tr... https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:311: As long as you're moving where alphaMask is referenced, its declaration can be moved here.
The CQ bit was checked by noel@chromium.org 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...
On 2017/02/23 15:11:51, scroggo_chromium wrote: > On 2017/02/23 07:11:13, noel gordon wrote: > > The came up in connection with the APNG patch. We should remove old cruft and > > TODO to simplify the APNG patch review. > > +1. My drafts in APNG suggest moving some of this to a separate CL. I plan to > have another patch set of APNG ready for review today. Yeah, was thinking we should just get this orthogonal stuff out of the way so we can focus on APNG in terms of the work and the review.
New patch uploaded, PTAL https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:177: // and do a full color space transformation if it is present. On 2017/02/23 14:09:34, msarett1 wrote: > On 2017/02/23 14:07:03, msarett1 wrote: > > On 2017/02/23 10:43:56, noel gordon wrote: > > > Fixed a while back. Though the Android pixels results seem to be back-the > > front > > > (R swapped with B), started when color correction was enabled for Android. > I > > > don't know what's going one there. > > Yes this comment is out of date, thanks for removing. K. > > Not sure what you're referring to with Android, but that seems like a serious > > issue. Yeap it looked somewhat serious to me. > Oops, I see the bug now. Thanks. Issue 676561 Yeah, the test is not flaky. It has an <img src="...">, no timers or nothing. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/images/pa... The actual layout image is drawn, but output looks to me like the R and G channels have been swapped. Weird. Try loading the test URL in your phone and see if looks color correct? https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (right): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:144: png_get_tRNS(png, info, &trns, &trnsCount, 0); On 2017/02/23 15:11:51, scroggo_chromium wrote: > Another issue you brought up in the APNG patch - trns and trnsCount are never > used. Ah yes, that's orthogonal to your APNG patch too. > trnsCount was used before [1], but trns was not used when it was introduced [2]. Yes, IIRC I added trnsCount because QCMS needed it. > And if we remove those, there is no reason to call png_get_tRNS, which looks to > just return values that we ignore [3]. Right, we can remove that part. I think have to retain the png_set_expand(png); if there is a PNG_INFO_tRNS. > Or would you prefer to land that as a separate CL? Nah, let's do it here and let the png-suite tests tell us if we are wrong. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:157: // TODO(msarret): Add GRAY, CMYK profile support? On 2017/02/23 14:07:03, msarett1 wrote: > nit: msarett > nit: CMYK doesn't seem relevant here since we can't get it from a PNG. Yes but reading SkColorSpace::MakeICC, it seems to accept a CMYK profile if it was present, eg., someone added a CMYK profile to the PNG :) Silly thing to do sure, and probably rare, but we just need some way to check for CMYK profile, and so ignore CMYK profiles here. I think the same issue can happen in the JPEG decoder now (I've seen JPEG with CMYK profiles), and maybe in the other decoders too. Basic issue is SkColorSpace::MakeICC accepts CMYK profiles, and the image decoders haven't been changed to handle that :( Changed the comment to say, ... block CMYK? > FWIW, Gray support is ready. Thanks for the reminder. Ah cool. It would be nice have a layout tests for gray profiled images since they have uses in medical applications, for example. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:311: On 2017/02/23 15:11:51, scroggo_chromium wrote: > As long as you're moving where alphaMask is referenced, its declaration can be > moved here. Done.
Description was changed from ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 ========== to ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT, and remove all unused transparency variables. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 ==========
lgtm
https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp (left): https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:104: // However, the current behavior matches Safari and Firefox. On 2017/02/23 14:07:03, msarett1 wrote: > On 2017/02/23 10:53:41, noel gordon wrote: > > "... that unmarked PNGs should be treated as sRGB" is a matter for some > > higher-layer to tell us, via a blink config flag or similar. > > > > We should not try to guess about profiles, nor try to patch over their > > bogus-ness, we should just deem them invalid. Not sure you understood this point, but the crux is about following the specs. > > There is only one exception to this that know of: when a color profile has > > degenerate gamma curves. Refer to https://crbg.com/492379 > > I don't completely agree. It is widely accepted that untagged images should be > treated as sRGB - this is how Safari behaves and how Chrome intends to behave. Sure, in a browser, per the CSS specs. The comment as written seemed to suggest to me that we just do it, but talking about CSS needs in an image decoder is probably not the right place to mention it. This image decoder does what is told by the system we are in (the browser), perhaps by a run-time flag, or perhaps via an decoder argument. We do have an argument that enables/disables color correction, and WebGL needs that (a disable), but we don't talk/comment about WebGL in this code, right? So no need to add comments about CSS spec issues like sRGB, I feel. > Images with only cHRM are a strange case - I would argue that they are not > "invalid". Right now, we ignore the tags, which seems fine. When we start > treating untagged images as sRGB, these would naturally be treated as sRGB as > well. This, to me, seems worse than at least paying attention to the gamut > specified by the cHRM. Yes strange case cHRM: and no matter what we do, we'll not have good compat here. The lone cHRM is not well-specified by the spec, and that results in different interpretations in different implementations. Treating it as sRGB might gain some compat, but I agree, the render might be worse as you suggest, which creates yet another compat issue :) > Of course, if our goal is to match Safari, I understand that as well. Our goal is compat in browsers, but lack of decent spec text (lone cHRM handling here) leads to compat problems. https://codereview.chromium.org/2716533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp:173: // transfer function is not necessarily a 2.2 exponential. On 2017/02/23 14:07:03, msarett1 wrote: > On 2017/02/23 10:43:56, noel gordon wrote: > > Here be dragons for reference. The sad story of PNG and gamma [1] and the > story > > of the Apple and the Pear [2]. Perhaps let sleeping dogs lie. > > > > [1] https://hsivonen.fi/png-gamma/ > > [2] https://bugs.chromium.org/p/chromium/issues/detail?id=517481 > > This comment seems valid to me? > > [1] seems to point out that browsers handle gamma wrong - which is a problem > that we can/should fix. I don't think the lone gAMA tag is a problem worth fixing: you'd need to change Windows (IE draws a Pear, the Windows Image Viewer draws a Pear, or vice-sersa, fix OSX, and so on). > [2] the apple/pear image "works" regardless of whether we correct to 2.2 or the > monitor gamma. If you remove the block of code, you get an Apple, if you retain it you get a Pear. Poor spec text, and we get compat problems in implementations. I would not describe it as "works". Moral of this story: good specs can be followed and we should do so to gain compat. Are we following the ICC spec? I don't see anywhere in it that suggests we patch over problems in color profiles or describe exactly how. But reading the Skia impl code, I see cases were do patch over, rather than declaring a profile bogus as conforming implementations should. Taking the PNG spec as an example, we know that road can lead to compat problems, I'm sad to say.
Ahem ... IE draws a Pear, the Windows Image Viewer draws an _Apple_, or vice-versa.
On 2017/02/23 16:58:06, scroggo_chromium wrote: > lgtm OK thanks. Will try to land.
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 noel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org Link to the patchset: https://codereview.chromium.org/2716533002/#ps40001 (title: "Remove unused transparency variables.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487873567571660,
"parent_rev": "0ac7dd29d6b7537444599db5342de36c8f5c6a69", "commit_rev":
"1a37300047e6ddefeb685d021e353ed08e3caffc"}
Message was sent while issue was closed.
Description was changed from ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT, and remove all unused transparency variables. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 ========== to ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT, and remove all unused transparency variables. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 Review-Url: https://codereview.chromium.org/2716533002 Cr-Commit-Position: refs/heads/master@{#452553} Committed: https://chromium.googlesource.com/chromium/src/+/1a37300047e6ddefeb685d021e35... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1a37300047e6ddefeb685d021e35...
Message was sent while issue was closed.
Description was changed from ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT, and remove all unused transparency variables. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 Review-Url: https://codereview.chromium.org/2716533002 Cr-Commit-Position: refs/heads/master@{#452553} Committed: https://chromium.googlesource.com/chromium/src/+/1a37300047e6ddefeb685d021e35... ========== to ========== [PNGDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT, and remove all unused transparency variables. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 Review-Url: https://codereview.chromium.org/2716533002 Cr-Commit-Position: refs/heads/master@{#452553} Committed: https://chromium.googlesource.com/chromium/src/+/1a37300047e6ddefeb685d021e35... ==========
Description was changed from ========== [PNGDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT, and remove all unused transparency variables. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 Review-Url: https://codereview.chromium.org/2716533002 Cr-Commit-Position: refs/heads/master@{#452553} Committed: https://chromium.googlesource.com/chromium/src/+/1a37300047e6ddefeb685d021e35... ========== to ========== [PNGImageDecoder] Clean up readColorSpace, TODOs, cruft inline sk_sp<SkColorSpace> readColorSpace Prefer the early return to prevent the code from dancing off to the right of the page. Clarify comment about cHRM+gAMA: we should never guess sRGB if there is no gAMA, that could be just as wrong. Images with no profile could be assumed to be sRGB but it's not the job of this lower-layer code to ever decide this matter: a system run-time flag or similar should be used. PNGImageDecoder::headerAvailable Remove unneeded filter variable, sk_sp<SkColorSpace> is convertible to bool and also move it into the callee, remove TODOs done or that aren't worth doing, add TODO about GRAY CMYK profiles, state why we "Update our info now", use DCHECK instead of ASSERT, and remove all unused transparency variables. PNGImageDecoder::rowAvailable Fix up the blink formatting of our libpng comments, ditch a C-style cast per style, move the alpha tracking code into the associated if branch to match an upcoming change to this code. No change in behavior, no new tests. BUG=439655 Review-Url: https://codereview.chromium.org/2716533002 Cr-Commit-Position: refs/heads/master@{#452553} Committed: https://chromium.googlesource.com/chromium/src/+/1a37300047e6ddefeb685d021e35... ========== |
