Set sRGB flag for PNGs with an sRGB chunk

Set sRGB flag for PNGs with an sRGB chunk

BUG=skia:3471
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1667823004

Committed: https://skia.googlesource.com/skia/+/a87d6de6a5018eab0a484af510338601f2976772

[msarett, 2016-02-04 16:25:55]

Description was changed from

==========
Set sRGB flag for PNGs with an sRGB chunk

BUG=skia:3471
==========

to

==========
Set sRGB flag for PNGs with an sRGB chunk

BUG=skia:3471
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-02-04 16:27:26]

Patchset #1 (id:1) has been deleted

[msarett, 2016-02-04 16:32:49]

msarett@google.com changed reviewers:
+ herb@google.com, reed@google.com, scroggo@google.com

[msarett, 2016-02-04 16:32:49]

This is more information than we had before, but also a very small subset of the
amount of color information that can be stored in PNGs.

PNG recognizes four types of sRGB.
PNG_sRGB_INTENT_SATURATION
PNG_sRGB_INTENT_PERCEPTUAL
PNG_sRGB_INTENT_ABSOLUTE
PNG_sRGB_INTENT_RELATIVE

I need to look a little deeper to see if Skia should also.

Additionally, a PNG may also have chromaticity or gamma chunks.  These are
overridden by an sRGB chunk, but are intended to provide custom color
information in the absence of sRGB.

[scroggo, 2016-02-04 16:37:52]

lgtm

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:337: *imageInfo = SkImageInfo::Make(origWidth,
origHeight, colorType, alphaType, profileType);
conversion_possible checks to make sure the profile types match. So if someone
requests something like:

codec->getInfo().make<...>(...)

this will work, but if they call some versions of SkImageInfo::Make (without
selecting the right profile type), getPixels will fail.

This may be correct (?), but we should make sure that our existing tests use the
former and not the latter so we don't suddenly start failing. (Our tests should
catch us, though, I think.)

[msarett, 2016-02-04 16:44:35]

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:337: *imageInfo = SkImageInfo::Make(origWidth,
origHeight, colorType, alphaType, profileType);
On 2016/02/04 16:37:52, scroggo wrote:
> conversion_possible checks to make sure the profile types match. So if someone
> requests something like:
> 
> codec->getInfo().make<...>(...)
> 
> this will work, but if they call some versions of SkImageInfo::Make (without
> selecting the right profile type), getPixels will fail.
> 
> This may be correct (?), but we should make sure that our existing tests use
the
> former and not the latter so we don't suddenly start failing. (Our tests
should
> catch us, though, I think.)

You're right.  This would break us all over the place (including in Android).
I would suggest that we change the check in conversion_possible to:
*** Always support linear (since right now we blindly output linear color
regardless of the encoded image)
*** Support sRGB if the encoded image is sRGB

What do you think?

[scroggo, 2016-02-04 16:53:01]

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:337: *imageInfo = SkImageInfo::Make(origWidth,
origHeight, colorType, alphaType, profileType);
On 2016/02/04 16:44:35, msarett wrote:
> On 2016/02/04 16:37:52, scroggo wrote:
> > conversion_possible checks to make sure the profile types match. So if
someone
> > requests something like:
> > 
> > codec->getInfo().make<...>(...)
> > 
> > this will work, but if they call some versions of SkImageInfo::Make (without
> > selecting the right profile type), getPixels will fail.
> > 
> > This may be correct (?), but we should make sure that our existing tests use
> the
> > former and not the latter so we don't suddenly start failing. (Our tests
> should
> > catch us, though, I think.)
> 
> You're right.  This would break us all over the place (including in Android).
> I would suggest that we change the check in conversion_possible to:
> *** Always support linear (since right now we blindly output linear color
> regardless of the encoded image)
> *** Support sRGB if the encoded image is sRGB
> 
> What do you think?

I think that's a fine choice for now. I think we have three choices for what to
do when linear is requested from an sRGB image:
- do a conversion to linear
- disallow linear
- silently give the sRGB version (this choice)

I don't know which one is correct, but maybe we should file a bug or a TODO to
fix to the right one?

[msarett, 2016-02-04 17:09:53]

I've updated conversion_possible to always allow kLinear.

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cpp
File src/codec/SkPngCodec.cpp (right):

https://codereview.chromium.org/1667823004/diff/20001/src/codec/SkPngCodec.cp...
src/codec/SkPngCodec.cpp:337: *imageInfo = SkImageInfo::Make(origWidth,
origHeight, colorType, alphaType, profileType);
On 2016/02/04 16:53:01, scroggo wrote:
> On 2016/02/04 16:44:35, msarett wrote:
> > On 2016/02/04 16:37:52, scroggo wrote:
> > > conversion_possible checks to make sure the profile types match. So if
> someone
> > > requests something like:
> > > 
> > > codec->getInfo().make<...>(...)
> > > 
> > > this will work, but if they call some versions of SkImageInfo::Make
(without
> > > selecting the right profile type), getPixels will fail.
> > > 
> > > This may be correct (?), but we should make sure that our existing tests
use
> > the
> > > former and not the latter so we don't suddenly start failing. (Our tests
> > should
> > > catch us, though, I think.)
> > 
> > You're right.  This would break us all over the place (including in
Android).
> > I would suggest that we change the check in conversion_possible to:
> > *** Always support linear (since right now we blindly output linear color
> > regardless of the encoded image)
> > *** Support sRGB if the encoded image is sRGB
> > 
> > What do you think?
> 
> I think that's a fine choice for now. I think we have three choices for what
to
> do when linear is requested from an sRGB image:
> - do a conversion to linear
> - disallow linear
> - silently give the sRGB version (this choice)
> 
> I don't know which one is correct, but maybe we should file a bug or a TODO to
> fix to the right one?

sgtm

[scroggo, 2016-02-04 17:11:50]

lgtm in patch set 2

[msarett, 2016-02-04 17:12:44]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-02-04 17:12:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1667823004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1667823004/40001

[commit-bot: I haz the power, 2016-02-04 17:35:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-04 17:35:17]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[msarett, 2016-02-04 22:59:21]

The CQ bit was checked by msarett@google.com

[msarett, 2016-02-04 22:59:22]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1667823004/#ps60001
(title: "Fix")

[commit-bot: I haz the power, 2016-02-04 22:59:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1667823004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1667823004/60001

[commit-bot: I haz the power, 2016-02-04 23:38:01]

Description was changed from

==========
Set sRGB flag for PNGs with an sRGB chunk

BUG=skia:3471
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Set sRGB flag for PNGs with an sRGB chunk

BUG=skia:3471
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/a87d6de6a5018eab0a484af510338601f2976772
==========

[commit-bot: I haz the power, 2016-02-04 23:38:02]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/a87d6de6a5018eab0a484af510338601f2976772