Color-correct PNG images that have an sRGB chunk

Color-correct PNG images that have an sRGB chunk

When a PNG has an sRGB chunk, the color space of the image RGB pixels  
is sRGB (per the PNG spec) and gAMA and cHRM chunks should be ignored
in systems that support image color-correction. Such images should be
color-corrected so do that, and gain compat with other browsers.

Note: per the spec, a PNG image should have an sRGB chunk, or an iCCP
chunk, but never both.

Changes in color behaviour here (obviously). Add TestExpectations for
layout test results needing rebaselines, ref-test fixes, etc.

BUG=354883
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187101

[Ken Russell (switch to Gerrit), 2014-12-03 23:50:02]

kbr@chromium.org changed reviewers:
+ kbr@chromium.org

[Ken Russell (switch to Gerrit), 2014-12-03 23:50:03]

+pdr. Philip, can you suggest other people who should review this change to
colorspace handling?

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/png/PNGImageDecoder.cpp (right):

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:202: if
(!colorProfile.isEmpty())
I think this should read "if (sRGB) { ... } else {
ASSERT(!colorProfile.isEmpty()); ... }" so that sRGB is preferred over other
color profiles that might be embedded in the file, per the PNG spec.

[pdr., 2014-12-03 23:54:26]

pdr@chromium.org changed reviewers:
+ senorblanco@chromium.org, skyostil@chromium.org

[pdr., 2014-12-03 23:55:21]

On 2014/12/03 at 23:54:26, pdr wrote:
> pdr@chromium.org changed reviewers:
> + senorblanco@chromium.org, skyostil@chromium.org

@Sami, would you be up for reviewing this one?

[Sami, 2014-12-04 12:51:24]

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/png/PNGImageDecoder.cpp (right):

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:283: static void
readColorProfile(png_structp png, png_infop info, ColorProfile& colorProfile,
bool& sRGB)
Generally output parameters should be pointers rather than references.

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:300: sRGB =
png_get_valid(png, info, PNG_INFO_sRGB);
Could we instead encode this information in |colorProfile| instead of adding a
redundant-seeming flag?

[Noel Gordon, 2014-12-05 04:12:43]

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/png/PNGImageDecoder.cpp (right):

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:202: if
(!colorProfile.isEmpty())
On 2014/12/03 23:50:02, Ken Russell wrote:
> I think this should read "if (sRGB) { ... } else {
> ASSERT(!colorProfile.isEmpty()); ... }" so that sRGB is preferred over other
> color profiles that might be embedded in the file, per the PNG spec.

I think the spec says that a PNG should have an sRGB or an iCCP, but not both. 
Doesn't say how to tie-break, so interesting point.

The preference selection is done in readColorProfile, so if I reading that you'd
prefer sRGB, I can add it there.

Note: mozilla prefers iCCP over sRGB [1].  I don't think it matters though. 
Thoughts?

http://lxr.mozilla.org/mozilla-central/source/image/decoders/nsPNGDecoder.cpp...

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:283: static void
readColorProfile(png_structp png, png_infop info, ColorProfile& colorProfile,
bool& sRGB)
On 2014/12/04 12:51:23, Sami wrote:
> Generally output parameters should be pointers rather than references.

Blink style is that output parameters are references, no?

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:300: sRGB =
png_get_valid(png, info, PNG_INFO_sRGB);
On 2014/12/04 12:51:23, Sami wrote:
> Could we instead encode this information in |colorProfile| instead of adding a
> redundant-seeming flag?

Not easily Sami. ColorProfile is not a class, it's a typedef to WTF::Vector.

[Sami, 2014-12-05 11:50:43]

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/png/PNGImageDecoder.cpp (right):

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:283: static void
readColorProfile(png_structp png, png_infop info, ColorProfile& colorProfile,
bool& sRGB)
On 2014/12/05 04:12:43, noel gordon wrote:
> On 2014/12/04 12:51:23, Sami wrote:
> > Generally output parameters should be pointers rather than references.
> 
> Blink style is that output parameters are references, no?

Ah, sorry, I've spent too much time in Chromium land recently :)

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:300: sRGB =
png_get_valid(png, info, PNG_INFO_sRGB);
On 2014/12/05 04:12:43, noel gordon wrote:
> On 2014/12/04 12:51:23, Sami wrote:
> > Could we instead encode this information in |colorProfile| instead of adding
a
> > redundant-seeming flag?
> 
> Not easily Sami. ColorProfile is not a class, it's a typedef to WTF::Vector.

Right, I was thinking it was a string identifier but looks like it's just a blob
of data. Nevermind then.

[Ken Russell (switch to Gerrit), 2014-12-05 19:24:53]

LGTM from my standpoint.

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/png/PNGImageDecoder.cpp (right):

https://codereview.chromium.org/774103002/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/png/PNGImageDecoder.cpp:202: if
(!colorProfile.isEmpty())
On 2014/12/05 04:12:43, noel gordon wrote:
> On 2014/12/03 23:50:02, Ken Russell wrote:
> > I think this should read "if (sRGB) { ... } else {
> > ASSERT(!colorProfile.isEmpty()); ... }" so that sRGB is preferred over other
> > color profiles that might be embedded in the file, per the PNG spec.
> 
> I think the spec says that a PNG should have an sRGB or an iCCP, but not both.

> Doesn't say how to tie-break, so interesting point.
> 
> The preference selection is done in readColorProfile, so if I reading that
you'd
> prefer sRGB, I can add it there.
> 
> Note: mozilla prefers iCCP over sRGB [1].  I don't think it matters though. 
> Thoughts?
> 
>
http://lxr.mozilla.org/mozilla-central/source/image/decoders/nsPNGDecoder.cpp...

I see. Thanks for clarifying that. This looks fine then.

[Noel Gordon, 2014-12-13 06:57:22]

Syncing up, to run try jobs and gather layout failures.

[Noel Gordon, 2014-12-13 16:02:02]

Patchset #6 (id:100001) has been deleted

[Noel Gordon, 2014-12-14 04:17:57]

Green bubbles at last (hard to come by).  Ken, assuming #8 holds, wrote change
log and will try to submit.

[Noel Gordon, 2014-12-14 04:18:08]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-12-14 04:19:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/774103002/120001

[commit-bot: I haz the power, 2014-12-14 04:21:11]

Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187101

[Noel Gordon, 2015-01-05 12:45:54]

Patchset #7 (id:140001) has been deleted