Display linear-srgb color managed canvas

Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

Committed: https://crrev.com/1837b16f64c579bc60b2e3ae512de9ee75f36836
Cr-Commit-Position: refs/heads/master@{#433229}

[zakerinasab, 2016-11-11 18:51:15]

Description was changed from

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

BUG=657946
==========

to

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

[zakerinasab, 2016-11-11 18:52:45]

zakerinasab@chromium.org changed reviewers:
+ ccameron@chromium.org, junov@chromium.org

[zakerinasab, 2016-11-11 18:52:46]

New patch submitted. PTAL.

[Justin Novosad, 2016-11-11 19:10:30]

https://codereview.chromium.org/2496913003/diff/1/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/2496913003/diff/1/cc/resources/resource.h#new...
cc/resources/resource.h:25: if
(cmd->HasSwitch(switches::kEnableColorCorrectRendering))
Calling HasSwitch is an expensive check. You should have the state of the flag
cached in a boolean. I am not sure whether there is a reasonable way to access
cc::LayerTreeSettings from here. IF so, that would be great. Worst case, you can
make a static function that returns the value of a static bool that is
initialized with the result of the HasSwitch query.  Same comment applies to all
the other calls to HasSwitch in this CL.

[Justin Novosad, 2016-11-11 19:11:06]

lgtm for third_party/WebKit

[ccameron, 2016-11-11 20:00:44]

Hi, sorry for leaving so many traps here for other platforms (Mac is unusual in
that we have accurate gfx::ColorSpace<->gfx::ICCProfile conversion).

We have code that expects to be able to convert
gfx::ColorSpace->ICCProfile->gfx::ColorSpace->SkColorSpace, and have reasonable
results come out ... but this part of the plumbing is broken.

I think that we should get that full conversion to be fixed, instead of putting
the queries for the command line flag in more locations.

I'm OOO today, but I'll put more comments on later this evening.

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

https://codereview.chromium.org/2496913003/diff/20001/ui/gfx/color_space.cc#n...
ui/gfx/color_space.cc:147: // Check for srgb and linear-srgb
Thanks, sorry for leaving this part unimplemented.

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

https://codereview.chromium.org/2496913003/diff/20001/ui/gfx/icc_profile.cc#n...
ui/gfx/icc_profile.cc:104: // objects.
This part, doing gfx::ColorSpace->ICCProfile is broken for SRGB color spaces,
and should be fixed.

[ccameron, 2016-11-12 04:30:12]

Part of the issue here is that Aura (Linux+Windows) should specify a screen's
ICC profile at

https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...

It may be that that will suffice to get things working.

The next issue, though, is that we just don't have the correct conversions of
gfx::ICCProfile<->gfx::ColorSpace<->SkColorSpace working.

We only support creating ICC profiles for gfx::ColorSpace objects that were
originally created by ICC profiles (so, such things as SRGB aren't supported). I
tried putting together a patch just now to allow ICC profiles to "remember" the
gfx::ColorSpace that they came from, but then something about mojo didn't build,
and I haven't figured out how to fix it yet.

[ccameron, 2016-11-14 22:17:49]

On 2016/11/12 04:30:12, ccameron wrote:
> Part of the issue here is that Aura (Linux+Windows) should specify a screen's
> ICC profile at
> 
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> 
> It may be that that will suffice to get things working.
> 
> The next issue, though, is that we just don't have the correct conversions of
> gfx::ICCProfile<->gfx::ColorSpace<->SkColorSpace working.
> 
> We only support creating ICC profiles for gfx::ColorSpace objects that were
> originally created by ICC profiles (so, such things as SRGB aren't supported).
I
> tried putting together a patch just now to allow ICC profiles to "remember"
the
> gfx::ColorSpace that they came from, but then something about mojo didn't
build,
> and I haven't figured out how to fix it yet.

I've added that cleanup to: https://codereview.chromium.org/2502703002/

[zakerinasab, 2016-11-15 14:45:02]

On 2016/11/14 22:17:49, ccameron wrote:
> On 2016/11/12 04:30:12, ccameron wrote:
> > Part of the issue here is that Aura (Linux+Windows) should specify a
screen's
> > ICC profile at
> > 
> >
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> > 
> > It may be that that will suffice to get things working.
> > 
> > The next issue, though, is that we just don't have the correct conversions
of
> > gfx::ICCProfile<->gfx::ColorSpace<->SkColorSpace working.
> > 
> > We only support creating ICC profiles for gfx::ColorSpace objects that were
> > originally created by ICC profiles (so, such things as SRGB aren't
supported).
> I
> > tried putting together a patch just now to allow ICC profiles to "remember"
> the
> > gfx::ColorSpace that they came from, but then something about mojo didn't
> build,
> > and I haven't figured out how to fix it yet.
> 
> I've added that cleanup to: https://codereview.chromium.org/2502703002/

Thanks :) As I understand this preserves the color space in
gfx::ICCProfile<->gfx::ColorSpace<->SkColorSpace covnersion 
and correctly converts sRGB between gfx::ColorSpace and SkColorSpace.
I think we also need to support linear-rgb in ColorSpace::ToSkColorSpace
and ColorSpace::FromSkColorSpace.

[zakerinasab, 2016-11-16 16:15:48]

Patchset #4 (id:60001) has been deleted

[zakerinasab, 2016-11-16 20:05:57]

Patchset #4 (id:80001) has been deleted

[zakerinasab, 2016-11-16 20:13:59]

New patch submitted. PTAL.

https://codereview.chromium.org/2496913003/diff/1/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/2496913003/diff/1/cc/resources/resource.h#new...
cc/resources/resource.h:25: if
(cmd->HasSwitch(switches::kEnableColorCorrectRendering))
On 2016/11/11 19:10:30, Justin Novosad wrote:
> Calling HasSwitch is an expensive check. You should have the state of the flag
> cached in a boolean. I am not sure whether there is a reasonable way to access
> cc::LayerTreeSettings from here. IF so, that would be great. Worst case, you
can
> make a static function that returns the value of a static bool that is
> initialized with the result of the HasSwitch query.  Same comment applies to
all
> the other calls to HasSwitch in this CL.

Done.

[ccameron, 2016-11-16 20:26:22]

I think that the LayerTreeHostImpl part doesn't need to be changed. In
particular, the LayerTreeHostImpl is not having its color space set correctly on
non-Mac platforms.

On Mac, we set it here:
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...

On Aura, we don't set it. The place to set it would be here:
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...

The value you put in results->icc_profile should be transformed to a
gfx::ColorSpace that eventually makes it to LayerTreeHostImpl.

I'd suggest trying:
results->icc_profile =
gfx::ICCProfile::FromColorSpace(gfx::ColorSpace::CreateSRGB())

Or trying 
results->icc_profile = gfx::ICCProfile::FromBestMonitor()
if (results->icc_profile == gfx::ICCProfile())
  results->icc_profile =
gfx::ICCProfile::FromColorSpace(gfx::ColorSpace::CreateSRGB())

Eventually we should be doing something multi-monitor aware on all platforms
(but for now we can set it wrong).

[zakerinasab, 2016-11-17 16:13:19]

Description was changed from

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

to

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

[zakerinasab, 2016-11-17 16:13:19]

zakerinasab@chromium.org changed reviewers:
+ sadrul@chromium.org

[zakerinasab, 2016-11-17 16:15:53]

On 2016/11/16 20:26:22, ccameron wrote:
> I think that the LayerTreeHostImpl part doesn't need to be changed. In
> particular, the LayerTreeHostImpl is not having its color space set correctly
on
> non-Mac platforms.
> 
> On Mac, we set it here:
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> 
> On Aura, we don't set it. The place to set it would be here:
>
https://cs.chromium.org/chromium/src/content/browser/web_contents/web_content...
> 
> The value you put in results->icc_profile should be transformed to a
> gfx::ColorSpace that eventually makes it to LayerTreeHostImpl.
> 
> I'd suggest trying:
> results->icc_profile =
> gfx::ICCProfile::FromColorSpace(gfx::ColorSpace::CreateSRGB())
> 
> Or trying 
> results->icc_profile = gfx::ICCProfile::FromBestMonitor()
> if (results->icc_profile == gfx::ICCProfile())
>   results->icc_profile =
> gfx::ICCProfile::FromColorSpace(gfx::ColorSpace::CreateSRGB())
> 
> Eventually we should be doing something multi-monitor aware on all platforms
> (but for now we can set it wrong).

Unfortunately this does not solve the problem for Mac layout tests. However, you
we right about LayerTreeHostImpl. I removed that code.

[zakerinasab, 2016-11-17 16:17:11]

New patch submitted. PTAL.
We still have issue with layout tests on Mac.

[zakerinasab, 2016-11-17 18:14:23]

zakerinasab@chromium.org changed reviewers:
+ xidachen@chromium.org

[ccameron, 2016-11-17 21:26:58]

This LGTM despite the failures -- I suspect that they are related to the fact
that the Mac bots have a gamma=1.8 display color profile, which is being
(inappropriately) baked into images.

[ccameron, 2016-11-17 21:27:21]

On 2016/11/17 21:26:58, ccameron wrote:
> This LGTM despite the failures -- I suspect that they are related to the fact
> that the Mac bots have a gamma=1.8 display color profile, which is being
> (inappropriately) baked into images.

(the mac failures, that is)

[zakerinasab, 2016-11-17 21:32:45]

Patchset #6 (id:140001) has been deleted

[zakerinasab, 2016-11-17 21:32:54]

Patchset #6 (id:160001) has been deleted

[Justin Novosad, 2016-11-17 21:37:06]

https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(left):

https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:27: 
Please revert this whitespace change

[zakerinasab, 2016-11-17 21:40:01]

On 2016/11/17 21:37:06, Justin Novosad wrote:
>
https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
> (left):
> 
>
https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:27: 
> Please revert this whitespace change

Done.

[Justin Novosad, 2016-11-17 21:40:12]

https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:379: crbug.com/657946
fast/canvas/color-space/canvas-colorSpace-attribute.html [ Failure ]
How come this test regressed?

[zakerinasab, 2016-11-17 21:41:44]

Patchset #7 (id:200001) has been deleted

[sadrul, 2016-11-17 21:42:43]

stamp lgtm

https://codereview.chromium.org/2496913003/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2496913003/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:671:
gfx::ICCProfile::FromColorSpace(gfx::ColorSpace::CreateSRGB());
{}?

[Justin Novosad, 2016-11-17 21:44:40]

On 2016/11/17 21:40:12, Justin Novosad wrote:
>
https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:379: crbug.com/657946
> fast/canvas/color-space/canvas-colorSpace-attribute.html [ Failure ]
> How come this test regressed?

Ah I get it. only the new virtual test suite allows the test to pass. It is
wasteful to that this test run 4x, when it is only 1 of the four runs we are
interested in.  Also the current design does not give us gpu and display list
coverage with color correction enabled.

[Justin Novosad, 2016-11-17 21:44:43]

On 2016/11/17 21:40:12, Justin Novosad wrote:
>
https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/TestExpectations:379: crbug.com/657946
> fast/canvas/color-space/canvas-colorSpace-attribute.html [ Failure ]
> How come this test regressed?

Ah I get it. only the new virtual test suite allows the test to pass. It is
wasteful to that this test run 4x, when it is only 1 of the four runs we are
interested in.  Also the current design does not give us gpu and display list
coverage with color correction enabled.

[xidachen, 2016-11-18 13:56:58]

As discussed yesterday, please put this layout under the
virtual/color_space/fast/canvas/color_space folder, in this case, the
fast/canvas/, virtual/gpu/fast/canvas, virtual/display_list_2d_canvas/canvas
won't run.

Please use this CL as an example: https://codereview.chromium.org/2420203002/
In the above CL, the tests placed under virtual/threaded/fast/idleToBlob runs
only on the virtual suite.

[zakerinasab, 2016-11-18 14:38:27]

New patch submitted. Extra failures removed from TestExpectations.

https://codereview.chromium.org/2496913003/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2496913003/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_view_aura.cc:671:
gfx::ICCProfile::FromColorSpace(gfx::ColorSpace::CreateSRGB());
On 2016/11/17 21:42:43, sadrul wrote:
> {}?

Done.

https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(left):

https://codereview.chromium.org/2496913003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:27: 
On 2016/11/17 21:37:06, Justin Novosad wrote:
> Please revert this whitespace change

Done.

[Justin Novosad, 2016-11-18 14:58:55]

lgtm

[xidachen, 2016-11-18 15:00:17]

lgtm

[zakerinasab, 2016-11-18 15:37:22]

The CQ bit was checked by zakerinasab@chromium.org

[zakerinasab, 2016-11-18 15:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2496913003/#ps240001
(title: "Removing Failure lines from TestExpectations")

[commit-bot: I haz the power, 2016-11-18 15:37:50]

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-11-18 17:40:54]

Description was changed from

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

to

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

[commit-bot: I haz the power, 2016-11-18 17:40:56]

Committed patchset #8 (id:240001)

[commit-bot: I haz the power, 2016-11-18 17:45:26]

Description was changed from

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

to

==========
Display linear-srgb color managed canvas

When the canvas is tagged with linear-tgb color space, it is not
displayed. This CL fixes this problem for software path and accelerated
path and also the layout tests.

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

Committed: https://crrev.com/1837b16f64c579bc60b2e3ae512de9ee75f36836
Cr-Commit-Position: refs/heads/master@{#433229}
==========

[commit-bot: I haz the power, 2016-11-18 17:45:28]

Patchset 8 (id:??) landed as
https://crrev.com/1837b16f64c579bc60b2e3ae512de9ee75f36836
Cr-Commit-Position: refs/heads/master@{#433229}