color: Always use parametric color spaces for raster

color: Always use parametric color spaces for raster

Rasterization must always be done to a parametric color space.

Display may be done to any color space.

Make ICCProfile::GetParametricColorSpace always return a parametric
color space (approximated if need be), and pass this to functions
specifying the space for rasterization.

Make ICCProfile::GetColorSpace return a space that references the
ICCProfile directly, and use the profile for color transforms
if the parametric version is not accurate.

Pass the ICCProfile to ui::Compositor, so that it can send
GetColorSpace to the GLRenderer and GetParametricColorSpace
to the LayerTreeHost.

R=hubbe
TBR=esprehn
BUG=697675
Review-Url: https://codereview.chromium.org/2742613002
Cr-Commit-Position: refs/heads/master@{#456849}
Committed: https://chromium.googlesource.com/chromium/src/+/24c87c3f418983673089663bb69045451d54f52d

[ccameron, 2017-03-09 02:55:56]

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

[commit-bot: I haz the power, 2017-03-09 02:56:28]

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

[ccameron, 2017-03-09 03:18:36]

Description was changed from

==========
color: Always use parametric color spaces for raster

Rasterization must always be done to a parametric color space.

Display may be done to any color space.

Make ICCProfile::GetParametricColorSpace always return a parametric
color space (approximated if need be), and pass this to functions
specifying the space for rasterization.

Make ICCProfile::GetColorSpace return a space that references the
ICCProfile directly, and use the profile for color transforms
if the parametric version is not accurate.

BUG=697675
==========

to

==========
color: Always use parametric color spaces for raster

Rasterization must always be done to a parametric color space.

Display may be done to any color space.

Make ICCProfile::GetParametricColorSpace always return a parametric
color space (approximated if need be), and pass this to functions
specifying the space for rasterization.

Make ICCProfile::GetColorSpace return a space that references the
ICCProfile directly, and use the profile for color transforms
if the parametric version is not accurate.

Pass the ICCProfile to ui::Compositor, so that it can send
GetColorSpace to the GLRenderer and GetParametricColorSpace
to the LayerTreeHost.

BUG=697675
==========

[ccameron, 2017-03-09 03:18:36]

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

[ccameron, 2017-03-09 07:13:40]

This keeps the "color space for display which can always use a LUT" separate
from the "color space for raster which has more restrictions", and clarifies a
bunch of behaviors (and adds tests).

[ccameron, 2017-03-13 12:50:21]

also ping

[hubbe, 2017-03-13 17:59:09]

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:45: ICC_PROFILE_REQUIRED,
Why?

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:78: ICC_PROFILE_REQUIRED,
Why?

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:799: // We needed the original ICC profile to
construct this transform, but it
This seems unacceptable to me.
In general, I don't understand what ICC_PROFILE_REQUIRED is for.

[ccameron, 2017-03-13 20:34:18]

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:45: ICC_PROFILE_REQUIRED,
On 2017/03/13 17:59:09, hubbe wrote:
> Why?

In the CL description, I wrote

> Make ICCProfile::GetColorSpace return a space that references the
> ICCProfile directly, and use the profile for color transforms
> if the parametric version is not accurate.

This enum denotes "the ICC profile is required to accurately represent this
color space".

Is there a more specific question here? It's hard for me to know what this
means.

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:799: // We needed the original ICC profile to
construct this transform, but it
On 2017/03/13 17:59:09, hubbe wrote:
> This seems unacceptable to me.
> In general, I don't understand what ICC_PROFILE_REQUIRED is for.

It denotes that the color space cannot be represented parametrically (see
previous comments).

We store ICC profiles that we will use for rendering. We should not store an
arbitrarily large number of ICC profiles (that's effectively a memory leak), so,
we do need to have some caching strategy that will at some point drop them. It
is theoretically possible for a frame to start rendering using a given ICC
profile, and then for the display to change enough times that the ICC profile
that we started with has been evicted. We need to draw *something* in that case,
and this is my suggestion.

I reiterate that this is highly unlikely -- it could probably only happen if one
were drawing complex content while running a script to cycle through display
profiles (cause you can't plug and unplug monitors or manually change settings
fast enough). But, it is an edge case that needs to be handled.

Another option would be to make ICCProfile be ref-ptr-ed. That may be something
to evaluate, but is a very large change (and needs consideration about how that
will integrate with cross-process stuff).

[hubbe, 2017-03-13 20:38:56]

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:45: ICC_PROFILE_REQUIRED,
On 2017/03/13 20:34:18, ccameron wrote:
> On 2017/03/13 17:59:09, hubbe wrote:
> > Why?
> 
> In the CL description, I wrote
> 
> > Make ICCProfile::GetColorSpace return a space that references the
> > ICCProfile directly, and use the profile for color transforms
> > if the parametric version is not accurate.
> 
> This enum denotes "the ICC profile is required to accurately represent this
> color space".
> 
> Is there a more specific question here? It's hard for me to know what this
> means.

I don't understand why we don't use CUSTOM with the best possible approximation
here?

If there is a good explanation for that, I think it should be in the code, not
the CL description.

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:799: // We needed the original ICC profile to
construct this transform, but it
On 2017/03/13 20:34:18, ccameron wrote:
> On 2017/03/13 17:59:09, hubbe wrote:
> > This seems unacceptable to me.
> > In general, I don't understand what ICC_PROFILE_REQUIRED is for.
> 
> It denotes that the color space cannot be represented parametrically (see
> previous comments).
> 
> We store ICC profiles that we will use for rendering. We should not store an
> arbitrarily large number of ICC profiles (that's effectively a memory leak),
so,
> we do need to have some caching strategy that will at some point drop them. It
> is theoretically possible for a frame to start rendering using a given ICC
> profile, and then for the display to change enough times that the ICC profile
> that we started with has been evicted. We need to draw *something* in that
case,
> and this is my suggestion.
> 
> I reiterate that this is highly unlikely -- it could probably only happen if
one
> were drawing complex content while running a script to cycle through display
> profiles (cause you can't plug and unplug monitors or manually change settings
> fast enough). But, it is an edge case that needs to be handled.
> 
> Another option would be to make ICCProfile be ref-ptr-ed. That may be
something
> to evaluate, but is a very large change (and needs consideration about how
that
> will integrate with cross-process stuff).

I think we should have some sort of approximation of the ICC profile in the
color space class so that we can have a fallback that's bad rather than broken.

[ccameron, 2017-03-14 10:19:54]

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_space.h#ne...
ui/gfx/color_space.h:45: ICC_PROFILE_REQUIRED,
On 2017/03/13 20:38:56, hubbe wrote:
> On 2017/03/13 20:34:18, ccameron wrote:
> > On 2017/03/13 17:59:09, hubbe wrote:
> > > Why?
> > 
> > In the CL description, I wrote
> > 
> > > Make ICCProfile::GetColorSpace return a space that references the
> > > ICCProfile directly, and use the profile for color transforms
> > > if the parametric version is not accurate.
> > 
> > This enum denotes "the ICC profile is required to accurately represent this
> > color space".
> > 
> > Is there a more specific question here? It's hard for me to know what this
> > means.
> 
> I don't understand why we don't use CUSTOM with the best possible
approximation
> here?

Because we need to distinguish between when CUSTOM is accurately representing
the ICCProfile from which it came, and when it isn't. In particular:

1. For rasterization, it is necessary to use the parametric color space. We need
to know if this matches or does not match the display's true color space. Hence
why we need one space to be the approximation (used by raster) and one to be
precise (used by display).

2. For creating ColorTransforms this creates a simpler way of knowing if we can
use parametric-transform-in-a-shader that we've proven to be more efficient, or
if we must fall back to a LUT.

> If there is a good explanation for that, I think it should be in the code, not
> the CL description.

I've added more comments to the enums. Please also see the comments in
ICCProfile::GetParametricColorSpace and ICCProfile::GetColorSpace.

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:799: // We needed the original ICC profile to
construct this transform, but it
On 2017/03/13 20:38:56, hubbe wrote:

> I think we should have some sort of approximation of the ICC profile in the
> color space class so that we can have a fallback that's bad rather than
broken.

This is to ensure well-defined behavior in a vanishingly-unlikely scenario.
"Broken" would mean giving something very wrong -- falling back to sRGB is
closer to "bad".

Also, I implemented this, and the complexity wasn't worth it (we'd have to have
a CUSTOM and CUSTOM_BUT_USE_ICC_IF_POSSIBLE), which share a bunch of state).

Also also, when I replace QCMS with SkColorSpaceXform here, we'll be able to use
|icc_profile_sk_color_space_| directly, so the complexity would be deleted soon
thereafter.

[hubbe, 2017-03-14 18:26:46]

lgtm

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:799: // We needed the original ICC profile to
construct this transform, but it
On 2017/03/14 10:19:54, ccameron wrote:
> On 2017/03/13 20:38:56, hubbe wrote:
> 
> > I think we should have some sort of approximation of the ICC profile in the
> > color space class so that we can have a fallback that's bad rather than
> broken.
> 
> This is to ensure well-defined behavior in a vanishingly-unlikely scenario.
> "Broken" would mean giving something very wrong -- falling back to sRGB is
> closer to "bad".
> 
> Also, I implemented this, and the complexity wasn't worth it (we'd have to
have
> a CUSTOM and CUSTOM_BUT_USE_ICC_IF_POSSIBLE), which share a bunch of state).
> 
> Also also, when I replace QCMS with SkColorSpaceXform here, we'll be able to
use
> |icc_profile_sk_color_space_| directly, so the complexity would be deleted
soon
> thereafter.

I suppose we're never going to have non-RGB ICC profiles (because in those
cases, substituting with sRGB would definitely be broken.)
So I guess you're right that this isn't really Br0ken, just bad.
If it's going away anyways soon, it's not really a problem I suppose. (Maybe
drop a TODO here so we know it's going to change?)

https://codereview.chromium.org/2742613002/diff/60001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2742613002/diff/60001/ui/gfx/icc_profile.cc#n...
ui/gfx/icc_profile.cc:219: // Populate |color_space_|.
Unhelpful comment.
Either remove or say something more useful.

[ccameron, 2017-03-14 18:55:00]

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

[commit-bot: I haz the power, 2017-03-14 18:55:40]

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

[ccameron, 2017-03-14 20:03:51]

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

[commit-bot: I haz the power, 2017-03-14 20:04:28]

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-03-14 21:34:04]

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

[commit-bot: I haz the power, 2017-03-14 21:34:05]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-03-14 21:41:03]

Thanks!

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

https://codereview.chromium.org/2742613002/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:799: // We needed the original ICC profile to
construct this transform, but it
On 2017/03/14 18:26:46, hubbe wrote:
> On 2017/03/14 10:19:54, ccameron wrote:
> > On 2017/03/13 20:38:56, hubbe wrote:
> > 
> > > I think we should have some sort of approximation of the ICC profile in
the
> > > color space class so that we can have a fallback that's bad rather than
> > broken.
> > 
> > This is to ensure well-defined behavior in a vanishingly-unlikely scenario.
> > "Broken" would mean giving something very wrong -- falling back to sRGB is
> > closer to "bad".
> > 
> > Also, I implemented this, and the complexity wasn't worth it (we'd have to
> have
> > a CUSTOM and CUSTOM_BUT_USE_ICC_IF_POSSIBLE), which share a bunch of state).
> > 
> > Also also, when I replace QCMS with SkColorSpaceXform here, we'll be able to
> use
> > |icc_profile_sk_color_space_| directly, so the complexity would be deleted
> soon
> > thereafter.
> 
> I suppose we're never going to have non-RGB ICC profiles (because in those
> cases, substituting with sRGB would definitely be broken.)
> So I guess you're right that this isn't really Br0ken, just bad.
> If it's going away anyways soon, it's not really a problem I suppose. (Maybe
> drop a TODO here so we know it's going to change?)

Done.

https://codereview.chromium.org/2742613002/diff/60001/ui/gfx/icc_profile.cc
File ui/gfx/icc_profile.cc (right):

https://codereview.chromium.org/2742613002/diff/60001/ui/gfx/icc_profile.cc#n...
ui/gfx/icc_profile.cc:219: // Populate |color_space_|.
On 2017/03/14 18:26:46, hubbe wrote:
> Unhelpful comment.
> Either remove or say something more useful.

Good point. Done.

[ccameron, 2017-03-14 21:42:01]

Description was changed from

==========
color: Always use parametric color spaces for raster

Rasterization must always be done to a parametric color space.

Display may be done to any color space.

Make ICCProfile::GetParametricColorSpace always return a parametric
color space (approximated if need be), and pass this to functions
specifying the space for rasterization.

Make ICCProfile::GetColorSpace return a space that references the
ICCProfile directly, and use the profile for color transforms
if the parametric version is not accurate.

Pass the ICCProfile to ui::Compositor, so that it can send
GetColorSpace to the GLRenderer and GetParametricColorSpace
to the LayerTreeHost.

BUG=697675
==========

to

==========
color: Always use parametric color spaces for raster

Rasterization must always be done to a parametric color space.

Display may be done to any color space.

Make ICCProfile::GetParametricColorSpace always return a parametric
color space (approximated if need be), and pass this to functions
specifying the space for rasterization.

Make ICCProfile::GetColorSpace return a space that references the
ICCProfile directly, and use the profile for color transforms
if the parametric version is not accurate.

Pass the ICCProfile to ui::Compositor, so that it can send
GetColorSpace to the GLRenderer and GetParametricColorSpace
to the LayerTreeHost.

R=hubbe
TBR=esprehn
BUG=697675
==========

[ccameron, 2017-03-14 21:42:15]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-03-14 21:42:16]

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/2742613002/#ps80001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2017-03-14 21:43:12]

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-03-14 21:50:51]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489527735889020,
"parent_rev": "782364b2af9f1c3c7f0a39d5dd9fa1df34cdfb09", "commit_rev":
"24c87c3f418983673089663bb69045451d54f52d"}

[commit-bot: I haz the power, 2017-03-14 21:51:39]

Description was changed from

==========
color: Always use parametric color spaces for raster

Rasterization must always be done to a parametric color space.

Display may be done to any color space.

Make ICCProfile::GetParametricColorSpace always return a parametric
color space (approximated if need be), and pass this to functions
specifying the space for rasterization.

Make ICCProfile::GetColorSpace return a space that references the
ICCProfile directly, and use the profile for color transforms
if the parametric version is not accurate.

Pass the ICCProfile to ui::Compositor, so that it can send
GetColorSpace to the GLRenderer and GetParametricColorSpace
to the LayerTreeHost.

R=hubbe
TBR=esprehn
BUG=697675
==========

to

==========
color: Always use parametric color spaces for raster

Rasterization must always be done to a parametric color space.

Display may be done to any color space.

Make ICCProfile::GetParametricColorSpace always return a parametric
color space (approximated if need be), and pass this to functions
specifying the space for rasterization.

Make ICCProfile::GetColorSpace return a space that references the
ICCProfile directly, and use the profile for color transforms
if the parametric version is not accurate.

Pass the ICCProfile to ui::Compositor, so that it can send
GetColorSpace to the GLRenderer and GetParametricColorSpace
to the LayerTreeHost.

R=hubbe
TBR=esprehn
BUG=697675

Review-Url: https://codereview.chromium.org/2742613002
Cr-Commit-Position: refs/heads/master@{#456849}
Committed:
https://chromium.googlesource.com/chromium/src/+/24c87c3f418983673089663bb690...
==========

[commit-bot: I haz the power, 2017-03-14 21:51:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/24c87c3f418983673089663bb690...