color: Fix color not being reset between layout tests

color: Fix color not being reset between layout tests

Layout tests run WebTestDelegate::SetDeviceColorProfile("reset") after
every test, which resets the ICC profile for rasterization to an empty
profile.

This is not the right thing to do with color correct rendering, because
it will make LayerTreeHostImpl::GetRasterColorSpace return an invalid
color space, which is used as a signal that color correct rendering is
disabled.

Fix this in two ways.

First, make WebTestDelegate::SetDeviceColorProfile("reset") reset the
ICC profile to be whatever was specified at the command line.

Second, make LayerTreeHostImpl::GetRasterColorSpace never return
an invalid color space when color correct rendering is enabled.

Third, fix a bug in RenderWidget where ICCProfile::GetColorSpace is
used instead of ICCProfile::GetParametricColorSpace.

BUG=724714
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2898583002
Cr-Commit-Position: refs/heads/master@{#473829}
Committed: https://chromium.googlesource.com/chromium/src/+/01912990baad76adb75b28ccc11c74691c9b34d3

[ccameron, 2017-05-20 02:21:58]

Description was changed from

==========
color: Fix color not being reset between layout tests

Layout tests run WebTestDelegate::SetDeviceColorProfile("reset") after
every test, which resets the ICC profile for rasterization to an empty
profile.

This is not the right thing to do with color correct rendering, because
it will make LayerTreeHostImpl::GetRasterColorSpace return an invalid
color space, which is used as a signal that color correct rendering is
disabled.

Fix this in two ways.

First, make WebTestDelegate::SetDeviceColorProfile("reset") reset the
ICC profile to be whatever was specified at the command line.

Second, make LayerTreeHostImpl::GetRasterColorSpace never return
an invalid color space when color correct rendering is enabled.

Third, fix a bug in RenderWidget where ICCProfile::GetColorSpace is
used instead of ICCProfile::GetParametricColorSpace.

BUG=724714
==========

to

==========
color: Fix color not being reset between layout tests

Layout tests run WebTestDelegate::SetDeviceColorProfile("reset") after
every test, which resets the ICC profile for rasterization to an empty
profile.

This is not the right thing to do with color correct rendering, because
it will make LayerTreeHostImpl::GetRasterColorSpace return an invalid
color space, which is used as a signal that color correct rendering is
disabled.

Fix this in two ways.

First, make WebTestDelegate::SetDeviceColorProfile("reset") reset the
ICC profile to be whatever was specified at the command line.

Second, make LayerTreeHostImpl::GetRasterColorSpace never return
an invalid color space when color correct rendering is enabled.

Third, fix a bug in RenderWidget where ICCProfile::GetColorSpace is
used instead of ICCProfile::GetParametricColorSpace.

BUG=724714
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-05-20 02:23:52]

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

[ccameron, 2017-05-20 02:24:19]

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

[ccameron, 2017-05-20 02:24:20]

ptal -- enne@ for content, avi@ for OWNERship

[commit-bot: I haz the power, 2017-05-20 02:24:27]

Dry run: CQ is trying da patch.

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

[enne (OOO), 2017-05-20 03:32:21]

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1359: result =
sync_tree()->raster_color_space();
I realize you're not changing this here, but this is a little weird to me that
this only asks the pending tree and not the active tree? For threaded
compositors, sync_tree only exists during commit.  Would that be why you're
getting invalid color spaces in these cases?

[commit-bot: I haz the power, 2017-05-20 04:12:11]

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

[commit-bot: I haz the power, 2017-05-20 04:12:11]

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

[Avi (use Gerrit), 2017-05-20 23:35:36]

LGTM stamp for when enne is happy

[ccameron, 2017-05-22 23:27:08]

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1359: result =
sync_tree()->raster_color_space();
On 2017/05/20 03:32:21, enne wrote:
> I realize you're not changing this here, but this is a little weird to me that
> this only asks the pending tree and not the active tree? For threaded
> compositors, sync_tree only exists during commit.  Would that be why you're
> getting invalid color spaces in these cases?

Oh, LTHI::sync_tree() returns
  return pending_tree_ ? pending_tree_.get() : active_tree_.get();
That's not defined outside of commit?

In this particular case, we were getting an invalid color space because
sync_tree() existed, but had had its color space explicitly set to the
default-ctor color space (which is invalid).

[enne (OOO), 2017-05-23 00:13:44]

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1359: result =
sync_tree()->raster_color_space();
On 2017/05/22 at 23:27:08, ccameron wrote:
> On 2017/05/20 03:32:21, enne wrote:
> > I realize you're not changing this here, but this is a little weird to me
that
> > this only asks the pending tree and not the active tree? For threaded
> > compositors, sync_tree only exists during commit.  Would that be why you're
> > getting invalid color spaces in these cases?
> 
> Oh, LTHI::sync_tree() returns
>   return pending_tree_ ? pending_tree_.get() : active_tree_.get();
> That's not defined outside of commit?

Ok, you're correct that as written, sync tree will return you a tree outside of
commit.  However, it really just means "the tree you're committing to" and
probably it should DCHECK that you're in the middle of commit when you call it.

The reason this is problematic is you can get a weird state here where both of
the trees exist and the pending tree has a different color space, and suddenly
the active tree starts using that color space to do raster in.

This is outside of this patch at this point but I think there should be at the
very least a comment here explaining what the process is when the color space is
different on both trees and why this code is ok.  It'd also be better to not
call sync tree and just include the logic to say "use the pending tree if it
exists, otherwise use the active tree, and this is ok because X". 
 
> In this particular case, we were getting an invalid color space because
sync_tree() existed, but had had its color space explicitly set to the
default-ctor color space (which is invalid).

Ok, in that case this patch lgtm.

[ccameron, 2017-05-23 06:28:40]

Thanks!

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2898583002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1359: result =
sync_tree()->raster_color_space();
On 2017/05/23 00:13:43, enne wrote:
> The reason this is problematic is you can get a weird state here where both of
> the trees exist and the pending tree has a different color space, and suddenly
> the active tree starts using that color space to do raster in.

Yeah, this doesn't have guarantees about when it will be called (it doesn't need
to be during commit).

> This is outside of this patch at this point but I think there should be at the
> very least a comment here explaining what the process is when the color space
is
> different on both trees and why this code is ok.  It'd also be better to not
> call sync tree and just include the logic to say "use the pending tree if it
> exists, otherwise use the active tree, and this is ok because X". 

Added a comment (prefer pending tree because that color space is more recently
sent from the browser).

[ccameron, 2017-05-23 06:29:08]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-05-23 06:29:09]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2898583002/#ps40001
(title: "Review feedbakc")

[commit-bot: I haz the power, 2017-05-23 06:29:24]

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-05-23 07:34:06]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495520948559830,
"parent_rev": "54436b450d22d4ce66ea198ab9999c570c28f84e", "commit_rev":
"01912990baad76adb75b28ccc11c74691c9b34d3"}

[commit-bot: I haz the power, 2017-05-23 07:34:19]

Description was changed from

==========
color: Fix color not being reset between layout tests

Layout tests run WebTestDelegate::SetDeviceColorProfile("reset") after
every test, which resets the ICC profile for rasterization to an empty
profile.

This is not the right thing to do with color correct rendering, because
it will make LayerTreeHostImpl::GetRasterColorSpace return an invalid
color space, which is used as a signal that color correct rendering is
disabled.

Fix this in two ways.

First, make WebTestDelegate::SetDeviceColorProfile("reset") reset the
ICC profile to be whatever was specified at the command line.

Second, make LayerTreeHostImpl::GetRasterColorSpace never return
an invalid color space when color correct rendering is enabled.

Third, fix a bug in RenderWidget where ICCProfile::GetColorSpace is
used instead of ICCProfile::GetParametricColorSpace.

BUG=724714
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
color: Fix color not being reset between layout tests

Layout tests run WebTestDelegate::SetDeviceColorProfile("reset") after
every test, which resets the ICC profile for rasterization to an empty
profile.

This is not the right thing to do with color correct rendering, because
it will make LayerTreeHostImpl::GetRasterColorSpace return an invalid
color space, which is used as a signal that color correct rendering is
disabled.

Fix this in two ways.

First, make WebTestDelegate::SetDeviceColorProfile("reset") reset the
ICC profile to be whatever was specified at the command line.

Second, make LayerTreeHostImpl::GetRasterColorSpace never return
an invalid color space when color correct rendering is enabled.

Third, fix a bug in RenderWidget where ICCProfile::GetColorSpace is
used instead of ICCProfile::GetParametricColorSpace.

BUG=724714
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2898583002
Cr-Commit-Position: refs/heads/master@{#473829}
Committed:
https://chromium.googlesource.com/chromium/src/+/01912990baad76adb75b28ccc11c...
==========

[commit-bot: I haz the power, 2017-05-23 07:34:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/01912990baad76adb75b28ccc11c...