color: Don't use QCMS for transforms unless necessary

color: Don't use QCMS for transforms unless necessary

Don't use QCMS for color space transformations unless the ColorSpace is
not a faithful representation of the ICCProfile.

With this in place, we can make a few housekeeping changes.

Actually create ICCProfiles for ColorSpaces that weren't created
from an ICCProfile to begin with.

Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which
can fail for unrepresentable results.

Add an IsValid method to ICCProfile to indicate if SkICC was able
to parse the profile.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2691213007
Cr-Commit-Position: refs/heads/master@{#451244}
Committed: https://chromium.googlesource.com/chromium/src/+/3b6fe6da979b8d9e084160ce6d8b7bd83f3aa878

[ccameron, 2017-02-15 20:38:52]

Description was changed from

==========
color: Don't use QCMS for transforms unless necessary

Don't use QCMS for color space transformations unless the ColorSpace is
not a faithful representation of the ICCProfile.

With this in place, we can make a few housekeeping changes.

Actually create ICCProfiles for ColorSpaces that weren't created
from an ICCProfile to begin with.

Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which
can fail for unrepresentable results.

Add an IsValid method to ICCProfile to indicate if SkICC was able
to parse the profile.

BUG=667966
==========

to

==========
color: Don't use QCMS for transforms unless necessary

Don't use QCMS for color space transformations unless the ColorSpace is
not a faithful representation of the ICCProfile.

With this in place, we can make a few housekeeping changes.

Actually create ICCProfiles for ColorSpaces that weren't created
from an ICCProfile to begin with.

Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which
can fail for unrepresentable results.

Add an IsValid method to ICCProfile to indicate if SkICC was able
to parse the profile.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2017-02-15 20:39:05]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 20:40:13]

Dry run: 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, 2017-02-15 22:13:27]

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

[commit-bot: I haz the power, 2017-02-15 22:13:28]

Dry run: Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ccameron, 2017-02-15 22:55:23]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 22:56:42]

Dry run: 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, 2017-02-16 00:43:46]

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

[commit-bot: I haz the power, 2017-02-16 00:43:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ccameron, 2017-02-16 01:09:32]

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

[ccameron, 2017-02-16 01:09:32]

ptal -- I pulled this off from the other patch.

[hubbe, 2017-02-16 01:28:29]

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:103: ColorSpace result(ColorSpace::PrimaryID::CUSTOM,
It would be really cool if we could map custom transfer & primaries back to
their enum values.

It would help optimize things and also help code like color_space_win.cc figure
out what to do better.

Maybe add a TODO for it?

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:183: // will be constructed ignoring the range adjust and
transfer matrices.
Ouch, if we can't represent those, shouldn't we just fail?

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:549: if (!from.IsValid())
It would be really cool if we could just do DCHECK(from.NoUnspecifiedValues())
here :)

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform_...
File ui/gfx/color_transform_unittest.cc (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform_...
ui/gfx/color_transform_unittest.cc:197: TEST(SimpleColorSpace, UnknownToSRGB) {
The changes in this test confuse me.

1. They seem to indicate that the default behavior of ColorSpace is now sRGB
instead of BT709. I don't understand why that would be based on this change.

2. If the default was to change, I prefer to change the construction of
"unknown" to match what would happen to a video which has not set it's color
space.

[ccameron, 2017-02-16 01:58:48]

updated

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc
File ui/gfx/color_space.cc (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:103: ColorSpace result(ColorSpace::PrimaryID::CUSTOM,
On 2017/02/16 01:28:29, hubbe wrote:
> It would be really cool if we could map custom transfer & primaries back to
> their enum values.
> 
> It would help optimize things and also help code like color_space_win.cc
figure
> out what to do better.
> 
> Maybe add a TODO for it?

Added a TODO. Might get to it soon (w/ some pending patches for
SkTransferFunctions).

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:183: // will be constructed ignoring the range adjust and
transfer matrices.
On 2017/02/16 01:28:29, hubbe wrote:
> Ouch, if we can't represent those, shouldn't we just fail?

This is written with the IOSurface API in mind -- in that API, you specify the
ICC profile of the transfer function and primaries that you want applied
(AFAICT).

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:549: if (!from.IsValid())
On 2017/02/16 01:28:29, hubbe wrote:
> It would be really cool if we could just do DCHECK(from.NoUnspecifiedValues())
> here :)

Maybe one day :D

I rebased this patch on top of the one that changes the default values, which
made this change go away.

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform_...
File ui/gfx/color_transform_unittest.cc (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform_...
ui/gfx/color_transform_unittest.cc:197: TEST(SimpleColorSpace, UnknownToSRGB) {
On 2017/02/16 01:28:29, hubbe wrote:
> The changes in this test confuse me.
> 
> 1. They seem to indicate that the default behavior of ColorSpace is now sRGB
> instead of BT709. I don't understand why that would be based on this change.
> 
> 2. If the default was to change, I prefer to change the construction of
> "unknown" to match what would happen to a video which has not set it's color
> space.

I moved this to the previous color patch and rebased it. But to discuss here.

What to do when we get a default-constructor gfx::ColorSpace passed to
ColorTransform?

I'm voting for "do no transformation" at the moment. Assuming sRGB would
probably be "not too bad" for everything video. Assuming BT709 would be "not too
bad" for video, and a disaster for everything else.

When we get close to shipping "color correct everything", we should probably
DCHECK that the transform's spaces are valid.

[ccameron, 2017-02-16 02:02:57]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 02:04:40]

Dry run: 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, 2017-02-16 04:18:50]

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

[commit-bot: I haz the power, 2017-02-16 04:18:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hubbe, 2017-02-16 05:35:56]

On 2017/02/16 01:58:48, ccameron wrote:
> updated
> 
> https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc
> File ui/gfx/color_space.cc (right):
> 
>
https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.cc#n...
> ui/gfx/color_space.cc:103: ColorSpace result(ColorSpace::PrimaryID::CUSTOM,
> On 2017/02/16 01:28:29, hubbe wrote:
> > It would be really cool if we could map custom transfer & primaries back to
> > their enum values.
> > 
> > It would help optimize things and also help code like color_space_win.cc
> figure
> > out what to do better.
> > 
> > Maybe add a TODO for it?
> 
> Added a TODO. Might get to it soon (w/ some pending patches for
> SkTransferFunctions).
> 
> https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h
> File ui/gfx/color_space.h (right):
> 
>
https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h#ne...
> ui/gfx/color_space.h:183: // will be constructed ignoring the range adjust and
> transfer matrices.
> On 2017/02/16 01:28:29, hubbe wrote:
> > Ouch, if we can't represent those, shouldn't we just fail?
> 
> This is written with the IOSurface API in mind -- in that API, you specify the
> ICC profile of the transfer function and primaries that you want applied
> (AFAICT).
> 
>
https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform.cc
> File ui/gfx/color_transform.cc (right):
> 
>
https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform....
> ui/gfx/color_transform.cc:549: if (!from.IsValid())
> On 2017/02/16 01:28:29, hubbe wrote:
> > It would be really cool if we could just do
DCHECK(from.NoUnspecifiedValues())
> > here :)
> 
> Maybe one day :D
> 
> I rebased this patch on top of the one that changes the default values, which
> made this change go away.
> 
>
https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform_...
> File ui/gfx/color_transform_unittest.cc (right):
> 
>
https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_transform_...
> ui/gfx/color_transform_unittest.cc:197: TEST(SimpleColorSpace, UnknownToSRGB)
{
> On 2017/02/16 01:28:29, hubbe wrote:
> > The changes in this test confuse me.
> > 
> > 1. They seem to indicate that the default behavior of ColorSpace is now sRGB
> > instead of BT709. I don't understand why that would be based on this change.
> > 
> > 2. If the default was to change, I prefer to change the construction of
> > "unknown" to match what would happen to a video which has not set it's color
> > space.
> 
> I moved this to the previous color patch and rebased it. But to discuss here.
> 
> What to do when we get a default-constructor gfx::ColorSpace passed to
> ColorTransform?
> 
> I'm voting for "do no transformation" at the moment. Assuming sRGB would
> probably be "not too bad" for everything video. Assuming BT709 would be "not
too
> bad" for video, and a disaster for everything else.

By the same token, assuming SRGB or "no transform" would be a disaster for
video.[1]

> When we get close to shipping "color correct everything", we should probably
> DCHECK that the transform's spaces are valid.

Well, clearly defaulting to BT709 was not the right thing to do. However, I fail
to see how ANY default can be the right thing to do, which is why I think a
(D)CHECK()
would be best. "Any transform" is always going to be wrong, but take longer to
find
than a (D)CHECK().


[1] Vizzini: But it's so simple. All I have to do is divine from what I know of
you: are you the sort of man who would put the poison into his own goblet or his
enemy's? Now, a clever man would put the poison into his own goblet, because he
would know that only a great fool would reach for what he was given. I am not a
great fool, so I can clearly not choose the wine in front of you. But you must
have known I was not a great fool, you would have counted on it, so I can
clearly not choose the wine in front of me.   (So clearly the solution is to
poison both cups and build up a tolerance to it...)

[ccameron, 2017-02-16 05:48:48]

Description was changed from

==========
color: Don't use QCMS for transforms unless necessary

Don't use QCMS for color space transformations unless the ColorSpace is
not a faithful representation of the ICCProfile.

With this in place, we can make a few housekeeping changes.

Actually create ICCProfiles for ColorSpaces that weren't created
from an ICCProfile to begin with.

Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which
can fail for unrepresentable results.

Add an IsValid method to ICCProfile to indicate if SkICC was able
to parse the profile.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
color: Don't use QCMS for transforms unless necessary

Don't use QCMS for color space transformations unless the ColorSpace is
not a faithful representation of the ICCProfile.

With this in place, we can make a few housekeeping changes.

Actually create ICCProfiles for ColorSpaces that weren't created
from an ICCProfile to begin with.

Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which
can fail for unrepresentable results.

Add an IsValid method to ICCProfile to indicate if SkICC was able
to parse the profile.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ccameron, 2017-02-16 07:14:29]

On 2017/02/16 05:35:56, hubbe wrote:

> Well, clearly defaulting to BT709 was not the right thing to do. However, I
fail
> to see how ANY default can be the right thing to do, which is why I think a
> (D)CHECK()
> would be best. "Any transform" is always going to be wrong, but take longer to
> find
> than a (D)CHECK().

A DCHECK would be too explode-y at the moment.

Is the reset of the patch decent though?

[ccameron, 2017-02-16 07:15:31]

On 2017/02/16 07:14:29, ccameron wrote:
> On 2017/02/16 05:35:56, hubbe wrote:
> 
> > Well, clearly defaulting to BT709 was not the right thing to do. However, I
> fail
> > to see how ANY default can be the right thing to do, which is why I think a
> > (D)CHECK()
> > would be best. "Any transform" is always going to be wrong, but take longer
to
> > find
> > than a (D)CHECK().
> 
> A DCHECK would be too explode-y at the moment.

I may be able to add that in the next like-week-or-so ... once I patch up the
callers that would be explode-y.

> Is the reset of the patch decent though?

[hubbe, 2017-02-16 08:33:03]

lgtm

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:183: // will be constructed ignoring the range adjust and
transfer matrices.
On 2017/02/16 01:58:47, ccameron wrote:
> On 2017/02/16 01:28:29, hubbe wrote:
> > Ouch, if we can't represent those, shouldn't we just fail?
> 
> This is written with the IOSurface API in mind -- in that API, you specify the
> ICC profile of the transfer function and primaries that you want applied
> (AFAICT).

Can we add that to comment in the code, it doesn't make sense otherwise.

https://codereview.chromium.org/2691213007/diff/40002/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2691213007/diff/40002/ui/gfx/color_transform....
ui/gfx/color_transform.cc:549: if (!from.IsValid())
Can we just remove this?

[ccameron, 2017-02-16 22:33:10]

Thanks!

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2691213007/diff/20001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:183: // will be constructed ignoring the range adjust and
transfer matrices.
On 2017/02/16 08:33:02, hubbe wrote:
> On 2017/02/16 01:58:47, ccameron wrote:
> > On 2017/02/16 01:28:29, hubbe wrote:
> > > Ouch, if we can't represent those, shouldn't we just fail?
> > 
> > This is written with the IOSurface API in mind -- in that API, you specify
the
> > ICC profile of the transfer function and primaries that you want applied
> > (AFAICT).
> 
> Can we add that to comment in the code, it doesn't make sense otherwise.
> 

Good point -- done.

https://codereview.chromium.org/2691213007/diff/40002/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2691213007/diff/40002/ui/gfx/color_transform....
ui/gfx/color_transform.cc:549: if (!from.IsValid())
On 2017/02/16 08:33:02, hubbe wrote:
> Can we just remove this?

Oops, yes, that was dead code.

[ccameron, 2017-02-17 01:34:35]

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

[ccameron, 2017-02-17 01:34:36]

Adding avi@ for content/ stamp

[ccameron, 2017-02-17 01:34:52]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 01:36:16]

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

[Avi (use Gerrit), 2017-02-17 01:44:31]

content lgtm

[commit-bot: I haz the power, 2017-02-17 03:40:00]

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

[commit-bot: I haz the power, 2017-02-17 03:40:01]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_optional_gpu_tests_rel on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[ccameron, 2017-02-17 04:35:33]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-02-17 04:35:34]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org
Link to the patchset: https://codereview.chromium.org/2691213007/#ps90001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2017-02-17 04:36:17]

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, 2017-02-17 06:22:02]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1487306133892880,
"parent_rev": "3bdfccc8b18cf18642fc48056a7a58d07f01265a", "commit_rev":
"3b6fe6da979b8d9e084160ce6d8b7bd83f3aa878"}

[commit-bot: I haz the power, 2017-02-17 06:22:43]

Description was changed from

==========
color: Don't use QCMS for transforms unless necessary

Don't use QCMS for color space transformations unless the ColorSpace is
not a faithful representation of the ICCProfile.

With this in place, we can make a few housekeeping changes.

Actually create ICCProfiles for ColorSpaces that weren't created
from an ICCProfile to begin with.

Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which
can fail for unrepresentable results.

Add an IsValid method to ICCProfile to indicate if SkICC was able
to parse the profile.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
color: Don't use QCMS for transforms unless necessary

Don't use QCMS for color space transformations unless the ColorSpace is
not a faithful representation of the ICCProfile.

With this in place, we can make a few housekeeping changes.

Actually create ICCProfiles for ColorSpaces that weren't created
from an ICCProfile to begin with.

Move ICCProfile::FromColorSpace to ColorSpace::GetICCProfile, which
can fail for unrepresentable results.

Add an IsValid method to ICCProfile to indicate if SkICC was able
to parse the profile.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2691213007
Cr-Commit-Position: refs/heads/master@{#451244}
Committed:
https://chromium.googlesource.com/chromium/src/+/3b6fe6da979b8d9e084160ce6d8b...
==========

[commit-bot: I haz the power, 2017-02-17 06:22:44]

Committed patchset #6 (id:90001) as
https://chromium.googlesource.com/chromium/src/+/3b6fe6da979b8d9e084160ce6d8b...