Define CHROME_PNG_READ_iCCP_sRGB_cHRM_gAMA

Define CHROME_PNG_READ_iCCP_sRGB_cHRM_gAMA

Detect if cHRM and gAMA chunks are present in the PNG image headers when
iCCP sRGB cHRM gAMA chunk reading is enabled by pngusr.h config.

Store the detections as PNG info flags in a PNG info struct value called
chroma_or_gamma. Define CHROME_PNG_READ_iCCP_sRGB_cHRM_gAMA to guard the
various code changes.

No change in behavior, no new tests.

BUG=354883

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

noel@chromium.org changed reviewers:
+ inferno@chromium.org

[Noel Gordon, 2014-12-02 20:45:01]

noel@chromium.org changed reviewers:
+ kbr@chromium.org
- inferno@chromium.org

[Noel Gordon, 2014-12-02 20:46:49]

Ken, if you're comfortable reviewing this one ...  Clusterfuzz image fuzzers
have our back.

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

What's the real bug here? Is it that libPNG isn't honoring the statements in
http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html saying for example:

"When the sRGB chunk is present, applications that recognize it and are capable
of color management [ICC] must ignore the gAMA and cHRM chunks and use the sRGB
chunk instead."

Is the bug that libPNG is synthesizing the gAMA and cHRM chunks when it sees the
sRGB chunk? Is it that it's reading these chunks from the file instead of
ignoring them properly? Is it that Chromium is doing the wrong thing with the
chunks that are read from the file?

[Noel Gordon, 2014-12-02 23:57:26]

On 2014/12/02 23:50:56, Ken Russell wrote:
> What's the real bug here? Is it that libPNG isn't honoring the statements in
> http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html saying for example:
> 
> "When the sRGB chunk is present, applications that recognize it and are
capable
> of color management [ICC] must ignore the gAMA and cHRM chunks and use the
sRGB
> chunk instead."
> 
> Is the bug that libPNG is synthesizing the gAMA and cHRM chunks when it sees
the
> sRGB chunk?

Yes, exactly. And I want to ignore the synthesized the gAMA and cHRM chunks.

> Is it that it's reading these chunks from the file instead of
> ignoring them properly?
> Is it that Chromium is doing the wrong thing with the
> chunks that are read from the file?

Chromium currently ignores them.  I'd like to make it spot the sRGB chunk and
use it correctly.

[Ken Russell (switch to Gerrit), 2014-12-03 00:00:18]

On 2014/12/02 23:57:26, noel gordon wrote:
> On 2014/12/02 23:50:56, Ken Russell wrote:
> > What's the real bug here? Is it that libPNG isn't honoring the statements in
> > http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html saying for example:
> > 
> > "When the sRGB chunk is present, applications that recognize it and are
> capable
> > of color management [ICC] must ignore the gAMA and cHRM chunks and use the
> sRGB
> > chunk instead."
> > 
> > Is the bug that libPNG is synthesizing the gAMA and cHRM chunks when it sees
> the
> > sRGB chunk?
> 
> Yes, exactly. And I want to ignore the synthesized the gAMA and cHRM chunks.

Would it be possible to instead fix libPNG to stop synthesizing the fake gAMA
and cHRM chunks? It seems to me that would be a more correct fix instead of
working around the problem, as this CL seems to do.

> > Is it that it's reading these chunks from the file instead of
> > ignoring them properly?
> > Is it that Chromium is doing the wrong thing with the
> > chunks that are read from the file?
> 
> Chromium currently ignores them.  I'd like to make it spot the sRGB chunk and
> use it correctly.

I'll defer to your judgment, but would like to understand the answer to the
above question first.

[Noel Gordon, 2014-12-03 00:13:56]

On 2014/12/03 00:00:18, Ken Russell wrote:
> On 2014/12/02 23:57:26, noel gordon wrote:
> > On 2014/12/02 23:50:56, Ken Russell wrote:
> > > What's the real bug here? Is it that libPNG isn't honoring the statements
in
> > > http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html saying for example:
> > > 
> > > "When the sRGB chunk is present, applications that recognize it and are
> > capable
> > > of color management [ICC] must ignore the gAMA and cHRM chunks and use the
> > sRGB
> > > chunk instead."
> > > 
> > > Is the bug that libPNG is synthesizing the gAMA and cHRM chunks when it
sees
> > the
> > > sRGB chunk?
> > 
> > Yes, exactly. And I want to ignore the synthesized the gAMA and cHRM chunks.
> 
> Would it be possible to instead fix libPNG to stop synthesizing the fake gAMA
> and cHRM chunks? It seems to me that would be a more correct fix instead of
> working around the problem, as this CL seems to do.

The synthesis happens via png_set_sRGB_gAMA_and_cHRM

 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libpng...

but conflated with sRGB setting also.  Ditching that line, I lost the ability to
detect the sRGB chunk.
 
> > > Is it that it's reading these chunks from the file instead of
> > > ignoring them properly?
> > > Is it that Chromium is doing the wrong thing with the
> > > chunks that are read from the file?
> > 
> > Chromium currently ignores them.  I'd like to make it spot the sRGB chunk
and
> > use it correctly.
> 
> I'll defer to your judgment, but would like to understand the answer to the
> above question first.

Let me know if I answered your question.

[Ken Russell (switch to Gerrit), 2014-12-03 00:47:38]

On 2014/12/03 00:13:56, noel gordon wrote:
> On 2014/12/03 00:00:18, Ken Russell wrote:
> > On 2014/12/02 23:57:26, noel gordon wrote:
> > > On 2014/12/02 23:50:56, Ken Russell wrote:
> > > > What's the real bug here? Is it that libPNG isn't honoring the
statements
> in
> > > > http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html saying for
example:
> > > > 
> > > > "When the sRGB chunk is present, applications that recognize it and are
> > > capable
> > > > of color management [ICC] must ignore the gAMA and cHRM chunks and use
the
> > > sRGB
> > > > chunk instead."
> > > > 
> > > > Is the bug that libPNG is synthesizing the gAMA and cHRM chunks when it
> sees
> > > the
> > > > sRGB chunk?
> > > 
> > > Yes, exactly. And I want to ignore the synthesized the gAMA and cHRM
chunks.
> > 
> > Would it be possible to instead fix libPNG to stop synthesizing the fake
gAMA
> > and cHRM chunks? It seems to me that would be a more correct fix instead of
> > working around the problem, as this CL seems to do.
> 
> The synthesis happens via png_set_sRGB_gAMA_and_cHRM
> 
>  
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libpng...
> 
> but conflated with sRGB setting also.  Ditching that line, I lost the ability
to
> detect the sRGB chunk.

That's clear, because it won't call png_set_sRGB.

From reading png_handle_sRGB, png_handle_gAMA and png_handle_cHRM, it seems to
me that the presence of PNG_INFO_sRGB in the "valid" bits of the header should
be sufficient information. Even though libpng synthesizes the gAMA and cHRM
information when it finds the sRGB chunk, the file either didn't contain those
chunks in the first place, or the data in the file was valid. Either way I think
Chromium ought to be able to act as though gAMA and cHRM were absent if it finds
sRGB and that should have the same effect as your CL (plus updating Chromium to
look for the new field). Am I missing something?

[Noel Gordon, 2014-12-03 01:46:46]

On 2014/12/03 00:47:38, Ken Russell wrote:

> > but conflated with sRGB setting also.  Ditching that line, I lost the
ability to
> > detect the sRGB chunk.
> 
> That's clear, because it won't call png_set_sRGB.

Yeap, exactly.

And if one changed the png_set_sRGB_gAMA_and_cHRM() line to call png_set_sRGB()
instead, then two other places in chromium maybe change behavior: the imagediff
tool, and gfx/png_code.cc

https://code.google.com/p/chromium/codesearch#chromium/src/tools/imagediff/im...
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/codec/png_c...

so I suspect such a change might be a lot more exciting. I've not tried it
though, but could if you think it's worth it.

> From reading png_handle_sRGB, png_handle_gAMA and png_handle_cHRM, it seems to
> me that the presence of PNG_INFO_sRGB in the "valid" bits of the header should
> be sufficient information. Even though libpng synthesizes the gAMA and cHRM
> information when it finds the sRGB chunk, the file either didn't contain those
> chunks in the first place, or the data in the file was valid.

Correct, and so you can't distinguish if the gAMA and cHRM were synthesized or
present in file.

> Either way I think Chromium ought to be able to act as though gAMA and cHRM
were absent if it finds
> sRGB and that should have the same effect as your CL (plus updating Chromium
to
> look for the new field). Am I missing something?

Agree and I did try that: 162 layout tests break [1]. Many of our test resource
images have all three chunks (sRGB gAMA cHRM). Wonderful. Given the sad state of
png and gamma correction [2], that's not so surprising and so I'd expect there
are many of these multi chromatic chunk PNG in the wild.

With my change, only 25 layout tests break.  My change is more restrictive:
there must only be an sRGB chunk. We could loosen the restriction later once the
svg changes are sorted out.  Or am I being too conservative here?

[1] https://code.google.com/p/chromium/issues/detail?id=354883#c6
[2] https://hsivonen.fi/png-gamma

[Ken Russell (switch to Gerrit), 2014-12-03 03:16:04]

On 2014/12/03 01:46:46, noel gordon wrote:
> On 2014/12/03 00:47:38, Ken Russell wrote:
> 
> > > but conflated with sRGB setting also.  Ditching that line, I lost the
> ability to
> > > detect the sRGB chunk.
> > 
> > That's clear, because it won't call png_set_sRGB.
> 
> Yeap, exactly.
> 
> And if one changed the png_set_sRGB_gAMA_and_cHRM() line to call
png_set_sRGB()
> instead, then two other places in chromium maybe change behavior: the
imagediff
> tool, and gfx/png_code.cc
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/imagediff/im...
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/codec/png_c...
> 
> so I suspect such a change might be a lot more exciting. I've not tried it
> though, but could if you think it's worth it.
> 
> > From reading png_handle_sRGB, png_handle_gAMA and png_handle_cHRM, it seems
to
> > me that the presence of PNG_INFO_sRGB in the "valid" bits of the header
should
> > be sufficient information. Even though libpng synthesizes the gAMA and cHRM
> > information when it finds the sRGB chunk, the file either didn't contain
those
> > chunks in the first place, or the data in the file was valid.
> 
> Correct, and so you can't distinguish if the gAMA and cHRM were synthesized or
> present in file.
> 
> > Either way I think Chromium ought to be able to act as though gAMA and cHRM
> were absent if it finds
> > sRGB and that should have the same effect as your CL (plus updating Chromium
> to
> > look for the new field). Am I missing something?
> 
> Agree and I did try that: 162 layout tests break [1]. Many of our test
resource
> images have all three chunks (sRGB gAMA cHRM). Wonderful. Given the sad state
of
> png and gamma correction [2], that's not so surprising and so I'd expect there
> are many of these multi chromatic chunk PNG in the wild.
> 
> With my change, only 25 layout tests break.  My change is more restrictive:
> there must only be an sRGB chunk. We could loosen the restriction later once
the
> svg changes are sorted out.  Or am I being too conservative here?
> 
> [1] https://code.google.com/p/chromium/issues/detail?id=354883#c6
> [2] https://hsivonen.fi/png-gamma

On the whole I think we should just bite the bullet and change Chrome's
behavior, if the new behavior is more correct. The tests can and should be
rebaselined.

We should see if doing so affects the behavior of
http://ie.microsoft.com/testdrive/graphics/colorprofiles/default.html and other
real-world color profile tests.

[Noel Gordon, 2014-12-03 03:53:35]

On 2014/12/03 03:16:04, Ken Russell wrote:
> > With my change, only 25 layout tests break.  My change is more restrictive:
> > there must only be an sRGB chunk. We could loosen the restriction later once
the
> > svg changes are sorted out.  Or am I being too conservative here?
> > 
> > [1] https://code.google.com/p/chromium/issues/detail?id=354883#c6
> > [2] https://hsivonen.fi/png-gamma
> 
> On the whole I think we should just bite the bullet and change Chrome's
> behavior, if the new behavior is more correct. The tests can and should be
> rebaselined.

I will post a blink patch that just corrects if it see sRGB, and we can look at
the SVG test changes.

I recall that it was mentioned elsewhere that "filter tests" broke when the
browser started to color correct CSS [1], and I'm seeing SVG filter tests break.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=25643#c10

> We should see if doing so affects the behavior of
> http://ie.microsoft.com/testdrive/graphics/colorprofiles/default.html and
other
> real-world color profile tests.

http://www.libpng.org/pub/png/colorcube/colorcube-pngs-sRGB-CSS.html

Load in chrome, all color matched

Load in safari, all color matched

Load in firefox: _color mismatches_

Load in chrome with my libpng change & blink-side too: _color mismatches_

So we'd regress here, but we'd be more "correct". The whole situation irks me,
but why I took the more conservative approach in this patch /shrug.

[Ken Russell (switch to Gerrit), 2014-12-04 00:01:28]

On 2014/12/03 03:53:35, noel gordon wrote:
> On 2014/12/03 03:16:04, Ken Russell wrote:
> > > With my change, only 25 layout tests break.  My change is more
restrictive:
> > > there must only be an sRGB chunk. We could loosen the restriction later
once
> the
> > > svg changes are sorted out.  Or am I being too conservative here?
> > > 
> > > [1] https://code.google.com/p/chromium/issues/detail?id=354883#c6
> > > [2] https://hsivonen.fi/png-gamma
> > 
> > On the whole I think we should just bite the bullet and change Chrome's
> > behavior, if the new behavior is more correct. The tests can and should be
> > rebaselined.
> 
> I will post a blink patch that just corrects if it see sRGB, and we can look
at
> the SVG test changes.
> 
> I recall that it was mentioned elsewhere that "filter tests" broke when the
> browser started to color correct CSS [1], and I'm seeing SVG filter tests
break.
> 
> [1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=25643#c10
> 
> > We should see if doing so affects the behavior of
> > http://ie.microsoft.com/testdrive/graphics/colorprofiles/default.html and
> other
> > real-world color profile tests.
> 
> http://www.libpng.org/pub/png/colorcube/colorcube-pngs-sRGB-CSS.html
> 
> Load in chrome, all color matched
> 
> Load in safari, all color matched
> 
> Load in firefox: _color mismatches_

What platform are you testing on? 

> Load in chrome with my libpng change & blink-side too: _color mismatches_

What about with just the Blink-side change in
https://codereview.chromium.org/774103002 ? I assume that mismatches too?

> So we'd regress here, but we'd be more "correct". The whole situation irks me,
> but why I took the more conservative approach in this patch /shrug.

Argh.

Why does Chrome mismatch in that case? Will your work on color correction in the
compositor fix that mismatch in the long run?

Is the intermediate state better or worse for end users?

[Noel Gordon, 2014-12-09 05:44:30]

On 2014/12/04 00:01:28, Ken Russell wrote:

> > Load in firefox: _color mismatches_
> 
> What platform are you testing on? 

OSX & Win32, on a wide-gamut monitor (wider than sRGB).

> > Load in chrome with my libpng change & blink-side too: _color mismatches_
> 
> What about with just the Blink-side change in
> https://codereview.chromium.org/774103002 ? I assume that mismatches too?

Yes. That change honors the sRGB chunk (absent an iCCP chunk, and regardless of
cHRM or
gAMA chunks) and color-corrects the image.

That can cause CSS color mismatches for some users: depends on their viewing
conditions.

> > So we'd regress here, but we'd be more "correct". The whole situation irks
me,
> > but why I took the more conservative approach in this patch /shrug.
> 
> Argh.
>
> Why does Chrome mismatch in that case?

Because Chrome would begin to respect the sRGB chunk. Users with wide-gamut
viewing conditions would see a mismatch.

> Will your work on color correction in the
> compositor fix that mismatch in the long run?

Yes, but it will require that chrome color-corrects CSS color also.  Only then
will the color mismatch go away, and regardless of user viewing conditions.

[Ken Russell (switch to Gerrit), 2014-12-11 01:30:12]

On 2014/12/09 05:44:30, noel gordon wrote:
> On 2014/12/04 00:01:28, Ken Russell wrote:
> 
> > > Load in firefox: _color mismatches_
> > 
> > What platform are you testing on? 
> 
> OSX & Win32, on a wide-gamut monitor (wider than sRGB).
> 
> > > Load in chrome with my libpng change & blink-side too: _color mismatches_
> > 
> > What about with just the Blink-side change in
> > https://codereview.chromium.org/774103002 ? I assume that mismatches too?
> 
> Yes. That change honors the sRGB chunk (absent an iCCP chunk, and regardless
of
> cHRM or
> gAMA chunks) and color-corrects the image.
> 
> That can cause CSS color mismatches for some users: depends on their viewing
> conditions.
> 
> > > So we'd regress here, but we'd be more "correct". The whole situation irks
> me,
> > > but why I took the more conservative approach in this patch /shrug.
> > 
> > Argh.
> >
> > Why does Chrome mismatch in that case?
> 
> Because Chrome would begin to respect the sRGB chunk. Users with wide-gamut
> viewing conditions would see a mismatch.
> 
> > Will your work on color correction in the
> > compositor fix that mismatch in the long run?
> 
> Yes, but it will require that chrome color-corrects CSS color also.  Only then
> will the color mismatch go away, and regardless of user viewing conditions.

Please provide your best recommendation about the path forward here. I'm happy
to l g t m either of these patches but am not really qualified to make the
decision. It sounds like this patch tries to reject bad images but I don't know
how widespread they might be.

[Noel Gordon, 2014-12-12 03:12:14]

On 2014/12/11 01:30:12, Ken Russell wrote:
> On 2014/12/09 05:44:30, noel gordon wrote:
> > On 2014/12/04 00:01:28, Ken Russell wrote:
> > 
> > > > Load in firefox: _color mismatches_
> > > 
> > > What platform are you testing on? 
> > 
> > OSX & Win32, on a wide-gamut monitor (wider than sRGB).
> > 
> > > > Load in chrome with my libpng change & blink-side too: _color
mismatches_
> > > 
> > > What about with just the Blink-side change in
> > > https://codereview.chromium.org/774103002 ? I assume that mismatches too?
> > 
> > Yes. That change honors the sRGB chunk (absent an iCCP chunk, and regardless
> of
> > cHRM or
> > gAMA chunks) and color-corrects the image.
> > 
> > That can cause CSS color mismatches for some users: depends on their viewing
> > conditions.
> > 
> > > > So we'd regress here, but we'd be more "correct". The whole situation
irks
> > me,
> > > > but why I took the more conservative approach in this patch /shrug.
> > > 
> > > Argh.
> > >
> > > Why does Chrome mismatch in that case?
> > 
> > Because Chrome would begin to respect the sRGB chunk. Users with wide-gamut
> > viewing conditions would see a mismatch.
> > 
> > > Will your work on color correction in the
> > > compositor fix that mismatch in the long run?
> > 
> > Yes, but it will require that chrome color-corrects CSS color also.  Only
then
> > will the color mismatch go away, and regardless of user viewing conditions.
> 
> Please provide your best recommendation

my recommendation is my patch :)
  - doesn't change anything
  - does allow us control how we would respond to sRGB in Blink.

Will say more about this shortly.

> about the path forward here. I'm happy
> to l g t m either of these patches but am not really qualified to make the
> decision. It sounds like this patch tries to reject bad images but I don't
know
> how widespread they might be.

Yeap, bad images being these multi-chunked-sRGB-gAMA-chRM png.

@firefrog could you provide data on how prevalent the bad png are?

[Noel Gordon, 2014-12-13 00:14:36]

On 2014/12/12 03:12:14, noel gordon wrote:

> > Please provide your best recommendation

> Will say more about this shortly.

Mulled some more. If Chrome respects the sRGB chunk, it brings us into line with
how other browsers handle the chunk (modulo our lack of CSS color correction,
which is a known bug).

Users with wide-gamut viewing conditions may see color differences in Chrome
(and in other browsers) and often use color profile switchers as a workaround.
Browsers don't serve wide-gamut users uniformly well yet, so nothing new there
if we begin to respect the sRGB chunk.

The sRGB chunk can be used to communicate color correction intent, with very low
overhead, and that use-case would improve. On balance, I think we should stash
this patch and bite the bullet, for some compat.

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

On 2014/12/13 00:14:36, noel gordon wrote:
> On 2014/12/12 03:12:14, noel gordon wrote:
> 
> > > Please provide your best recommendation
> 
> > Will say more about this shortly.
> 
> Mulled some more. If Chrome respects the sRGB chunk, it brings us into line
with
> how other browsers handle the chunk (modulo our lack of CSS color correction,
> which is a known bug).
> 
> Users with wide-gamut viewing conditions may see color differences in Chrome
> (and in other browsers) and often use color profile switchers as a workaround.
> Browsers don't serve wide-gamut users uniformly well yet, so nothing new there
> if we begin to respect the sRGB chunk.
> 
> The sRGB chunk can be used to communicate color correction intent, with very
low
> overhead, and that use-case would improve. On balance, I think we should stash
> this patch and bite the bullet, for some compat.

OK. Let's please close this then and pursue the other option.

[Noel Gordon, 2014-12-14 04:53:27]

On 2014/12/13 00:23:03, Ken Russell wrote:
 
> OK. Let's please close this then and pursue the other option.

Done, and for additional patina, see #1
https://code.google.com/p/page-speed/issues/detail?id=1187 to see how difficult
it is to decide which PNG chunks to believe, retain, etc, and #2
http://superuser.com/questions/579216/why-does-this-png-image-display-differe...
(the strange story of the apple and the pear). png is a cluster.