color: Make images without embedded profiles sRGB by default

color: Make images without embedded profiles sRGB by default

This is only under the --enable-color-correct-rendering flag.

BUG=44872
Committed: https://crrev.com/1614d49c8ff84f43b6d5abdfa6e78beec18101c4
Cr-Commit-Position: refs/heads/master@{#420157}

[ccameron, 2016-09-20 22:54:40]

ccameron@chromium.org changed reviewers:
+ scroggo@chromium.org

[ccameron, 2016-09-20 22:54:40]

I somehow missed this my first time through adding SkColorSpaces to decoded
images...

[scroggo, 2016-09-21 11:46:06]

scroggo@google.com changed reviewers:
+ msarett@chromium.org, scroggo@google.com

[scroggo, 2016-09-21 11:46:07]

lgtm

[msarett, 2016-09-21 12:06:40]

msarett@google.com changed reviewers:
+ msarett@google.com

[msarett, 2016-09-21 12:06:40]

https://codereview.chromium.org/2359643002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2359643002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:109: colorSpace
= SkColorSpace::NewICC(newIccProfile.data(), newIccProfile.size());
NewICC() might fail (return nullptr)  - if the input data is bad or if the
profile is one that we don't yet recognize.  Should we set also set the
colorSpace to sRGB if NewICC() returns nullptr?

[ccameron, 2016-09-21 18:23:02]

Thanks!

https://codereview.chromium.org/2359643002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2359643002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:109: colorSpace
= SkColorSpace::NewICC(newIccProfile.data(), newIccProfile.size());
On 2016/09/21 12:06:40, msarett wrote:
> NewICC() might fail (return nullptr)  - if the input data is bad or if the
> profile is one that we don't yet recognize.  Should we set also set the
> colorSpace to sRGB if NewICC() returns nullptr?

I was considering to add an ASSERT for that, rather than silently failing. This
is still behind a flag, so the more landmines the better IMO.

Changed this to an ASSERT.

[ccameron, 2016-09-21 18:23:06]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-09-21 18:23:07]

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/2359643002/#ps20001
(title: "Use DCHECK")

[commit-bot: I haz the power, 2016-09-21 18:23:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarett, 2016-09-21 18:28:43]

https://codereview.chromium.org/2359643002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp (right):

https://codereview.chromium.org/2359643002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageFrame.cpp:109: colorSpace
= SkColorSpace::NewICC(newIccProfile.data(), newIccProfile.size());
On 2016/09/21 18:23:02, ccameron wrote:
> On 2016/09/21 12:06:40, msarett wrote:
> > NewICC() might fail (return nullptr)  - if the input data is bad or if the
> > profile is one that we don't yet recognize.  Should we set also set the
> > colorSpace to sRGB if NewICC() returns nullptr?
> 
> I was considering to add an ASSERT for that, rather than silently failing.
This
> is still behind a flag, so the more landmines the better IMO.
> 
> Changed this to an ASSERT.

ASSERT seems good for now - especially if it helps us recognize profiles that we
aren't parsing correctly.  I would love to see those :).

When this gets turned on, we'll obviously need to do something different (I
think setting to sRGB makes sense).  There certainly are images on the web that
say they have ICC profiles and then give us garabage...

[commit-bot: I haz the power, 2016-09-21 18:34:21]

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

[commit-bot: I haz the power, 2016-09-21 18:34:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2016-09-21 19:43:22]

On 2016/09/21 18:34:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

scroggo, could you lgtm from your @chromium.org account?

[scroggo_chromium, 2016-09-21 19:51:06]

On 2016/09/21 19:43:22, ccameron wrote:
> On 2016/09/21 18:34:22, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> scroggo, could you lgtm from your @chromium.org account?

D'oh! LGTM, too!

[ccameron, 2016-09-21 20:04:32]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-09-21 20:05:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 20:47:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-21 20:50:08]

Description was changed from

==========
color: Make images without embedded profiles sRGB by default

This is only under the --enable-color-correct-rendering flag.

BUG=44872
==========

to

==========
color: Make images without embedded profiles sRGB by default

This is only under the --enable-color-correct-rendering flag.

BUG=44872

Committed: https://crrev.com/1614d49c8ff84f43b6d5abdfa6e78beec18101c4
Cr-Commit-Position: refs/heads/master@{#420157}
==========

[commit-bot: I haz the power, 2016-09-21 20:50:10]

Patchset 2 (id:??) landed as
https://crrev.com/1614d49c8ff84f43b6d5abdfa6e78beec18101c4
Cr-Commit-Position: refs/heads/master@{#420157}