Force sRGB color profile on Mac for pixel tests

Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

Mark video-based tests on Mac as failing, since their output will
change with this patch (because CoreAnimation will be doing different
color conversion).

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2944733002
Cr-Commit-Position: refs/heads/master@{#481358}
Committed: https://chromium.googlesource.com/chromium/src/+/b80ffde89677d5d2b83ca25c221a768fa0029a9b

[ccameron, 2017-06-16 23:24:36]

Description was changed from

==========
color: Enable color correct rendering and force sRGB for pixel tests

BUG=734255
==========

to

==========
color: Enable color correct rendering and force sRGB for pixel tests

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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-06-16 23:26:41]

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

[commit-bot: I haz the power, 2017-06-16 23:27: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-06-17 00:18:30]

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

[commit-bot: I haz the power, 2017-06-17 00:18:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2017-06-20 10:02:24]

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

[commit-bot: I haz the power, 2017-06-20 10:02:39]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-06-20 10:04:34]

Description was changed from

==========
color: Enable color correct rendering and force sRGB for pixel tests

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
[For Discussion]: Force sRGB profile for pixel tests

Force Mac to change to a sRGB color profile when running pixel tests

Enable color correct rendering on GPU pixel tests
==========

[ccameron, 2017-06-20 10:04:35]

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

[commit-bot: I haz the power, 2017-06-20 10:11:02]

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

[commit-bot: I haz the power, 2017-06-20 10:11:02]

Dry run: 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, 2017-06-20 18:48:07]

kbr, does this general scheme of forcing color profile sgty? If so, I'll go
ahead and clean this up and send it our.

https://codereview.chromium.org/2944733002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/system_color_profile_manager.py (right):

https://codereview.chromium.org/2944733002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/system_color_profile_manager.py:5: import Foundation
I don't have "readability" for Python, so this is definitely not stylistically
correct.

That said, do you think it's reasonable to use these mechanisms (Quartz,
Foundation, and objc), or do you think it would be better to create an
additional target (written in C) for setting and re-setting system color
profiles?

[ccameron, 2017-06-20 21:40:50]

Description was changed from

==========
[For Discussion]: Force sRGB profile for pixel tests

Force Mac to change to a sRGB color profile when running pixel tests

Enable color correct rendering on GPU pixel tests
==========

to

==========
[For Discussion]: Force sRGB profile for pixel tests

Force Mac to change to a sRGB color profile when running pixel tests

Enable color correct rendering on GPU pixel tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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-06-20 21:40:52]

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

[commit-bot: I haz the power, 2017-06-20 21:41:22]

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-06-20 21:49:48]

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

[commit-bot: I haz the power, 2017-06-20 21:49:50]

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

[ccameron, 2017-06-20 22:01:26]

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

[commit-bot: I haz the power, 2017-06-20 22:01:57]

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-06-20 22:39:03]

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

[commit-bot: I haz the power, 2017-06-20 22:39:04]

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

[Ken Russell (switch to Gerrit), 2017-06-20 22:55:02]

Thanks for putting this together. The overall approach does seem OK to me. The
layout test harness used to perform a switch like this (I think again from
Python, not sure -- Dirk may remember, CC'd) and it was reliable enough to be
done on the bots.

A few comments. Could you link to a bug ID?

https://codereview.chromium.org/2944733002/diff/20001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/system_color_profile_manager.py (right):

https://codereview.chromium.org/2944733002/diff/20001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/system_color_profile_manager.py:5: import Foundation
On 2017/06/20 18:48:07, ccameron wrote:
> I don't have "readability" for Python, so this is definitely not stylistically
> correct.
> 
> That said, do you think it's reasonable to use these mechanisms (Quartz,
> Foundation, and objc), or do you think it would be better to create an
> additional target (written in C) for setting and re-setting system color
> profiles?

I think it's reasonable to use Python for this. Fantastic work tracking down
that this is possible at all from Python. Your later patch sets look a lot
cleaner, so more comments there.

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/color_profile_manager.py (right):

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/color_profile_manager.py:11: _display_profile_url_map
= {}
This is a class-scoped variable as written. To make it instance-scoped, set it
up in __init__ like:
  self._display_profile_url_map = {}
before the Mac-specific code.

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/color_profile_manager.py:18: def ForceSRGB(self):
Do you think it's worth registering an atexit handler which will call Restore on
itself if it hasn't already been called? Thinking of something like this:

https://pastebin.com/mGK5HMZb

The various frameworks (typ, I think, is responsible for this now) do call
TearDownProcess even if Ctrl-C is pressed, so perhaps this added complexity
isn't needed.

Please add documentation saying that if ForceSRGB is called, Restore must be
called.

We should give the Swarming team a heads-up that we are going to be changing
system-wide settings which may affect subsequent test runs.

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/maps_integration_test.py (right):

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/maps_integration_test.py:77:
cls._color_profile_manager.Restore()
See above comment about whether this class should auto-Restore during process
teardown.

[ccameron, 2017-06-21 00:05:07]

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

[commit-bot: I haz the power, 2017-06-21 00:05:33]

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-06-21 01:11:52]

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

[commit-bot: I haz the power, 2017-06-21 01:11:53]

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

[Dirk Pranke, 2017-06-21 01:59:38]

dpranke@chromium.org changed reviewers:
+ dnj@chromium.org, dpranke@chromium.org

[Dirk Pranke, 2017-06-21 01:59:38]

On 2017/06/20 22:55:02, Ken Russell wrote:
> Thanks for putting this together. The overall approach does seem OK to me. The
> layout test harness used to perform a switch like this (I think again from
> Python, not sure -- Dirk may remember, CC'd) and it was reliable enough to be
> done on the bots.

That was (and is, I think) done in content shell instead. Or, it might've been
done in 
layout_test_helper. It was not done via Python.

One concern with this is that I think it'll only work with the system-installed
Python, rather than a Python built from scratch.

We'd like to move to the latter in infra, so we'd have to figure out how to
bundle the apple packages into our own build,
if that's even possible, license-wise. Failing that, we'd probably have to move
this into a helper c++ binary, sadly :(.

dnj@, thoughts on the above problem?

[dnj, 2017-06-21 02:21:59]

On 2017/06/21 01:59:38, Dirk Pranke wrote:
> On 2017/06/20 22:55:02, Ken Russell wrote:
> > Thanks for putting this together. The overall approach does seem OK to me.
The
> > layout test harness used to perform a switch like this (I think again from
> > Python, not sure -- Dirk may remember, CC'd) and it was reliable enough to
be
> > done on the bots.
> 
> That was (and is, I think) done in content shell instead. Or, it might've been
> done in 
> layout_test_helper. It was not done via Python.
> 
> One concern with this is that I think it'll only work with the
system-installed
> Python, rather than a Python built from scratch.
> 
> We'd like to move to the latter in infra, so we'd have to figure out how to
> bundle the apple packages into our own build,
> if that's even possible, license-wise. Failing that, we'd probably have to
move
> this into a helper c++ binary, sadly :(.
> 
> dnj@, thoughts on the above problem?

The library in question seems to be an installation root of PyObjC
(https://pythonhosted.org/pyobjc/) and its wrapper set
(https://pythonhosted.org/pyobjc/notes/framework-wrappers.html). We could
*probably* package them all into wheels. It might be a good idea to actually
test that this is achievable in a VirtualEnv before committing to it, since we
want this sort of thing to be VirtualEnv-accessible.

[ccameron, 2017-06-21 05:55:57]

Description was changed from

==========
[For Discussion]: Force sRGB profile for pixel tests

Force Mac to change to a sRGB color profile when running pixel tests

Enable color correct rendering on GPU pixel tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

BUG=734255
==========

[ccameron, 2017-06-21 06:38:23]

Description was changed from

==========
Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

BUG=734255
==========

to

==========
Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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-06-21 06:40:01]

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

[commit-bot: I haz the power, 2017-06-21 06:40:12]

Dry run: CQ is trying da patch.

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

[ccameron, 2017-06-21 06:40:53]

Description was changed from

==========
Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

Mark video-based tests on Mac as failing, since their output will
change with this patch (because CoreAnimation will be doing different
color conversion).

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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-06-21 06:45:42]

Thanks for the comments -- I've updated the patch to be force-until-exit.

The LayoutTest behavior was removed in crrev.com/416170, and it was part of
layout_test_helper.

Is it okay to move forward with this, or does it pose immediate problems? It
would be easy to add a program to a BUILD.gn that would set these color profiles
-- the hard part would be to ensure that it is built appropriately and
integrated into the test harness. I'm happy to commit to doing that if we at
some point are unable to use the Python implementation.

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/color_profile_manager.py (right):

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/color_profile_manager.py:11: _display_profile_url_map
= {}
On 2017/06/20 22:55:02, Ken Russell wrote:
> This is a class-scoped variable as written. To make it instance-scoped, set it
> up in __init__ like:
>   self._display_profile_url_map = {}
> before the Mac-specific code.

Thanks. Ended up deleting this from the atexit part.

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/color_profile_manager.py:18: def ForceSRGB(self):
On 2017/06/20 22:55:02, Ken Russell wrote:
> Do you think it's worth registering an atexit handler which will call Restore
on
> itself if it hasn't already been called? Thinking of something like this:
> 
> https://pastebin.com/mGK5HMZb
> 
> The various frameworks (typ, I think, is responsible for this now) do call
> TearDownProcess even if Ctrl-C is pressed, so perhaps this added complexity
> isn't needed.
> 
> Please add documentation saying that if ForceSRGB is called, Restore must be
> called.
> 
> We should give the Swarming team a heads-up that we are going to be changing
> system-wide settings which may affect subsequent test runs.

Good idea -- I updated this to register an atexit handler, and just have a
method ForceUntilExitSRGB, which has these calls wrapped into one.

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
File content/test/gpu/gpu_tests/maps_integration_test.py (right):

https://codereview.chromium.org/2944733002/diff/80001/content/test/gpu/gpu_te...
content/test/gpu/gpu_tests/maps_integration_test.py:77:
cls._color_profile_manager.Restore()
On 2017/06/20 22:55:02, Ken Russell wrote:
> See above comment about whether this class should auto-Restore during process
> teardown.

Done -- this (and the whole method for pixel) are gone.

[commit-bot: I haz the power, 2017-06-21 08:21:19]

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

[commit-bot: I haz the power, 2017-06-21 08:21:20]

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

[Dirk Pranke, 2017-06-21 17:44:01]

On 2017/06/21 06:45:42, ccameron wrote:
> Thanks for the comments -- I've updated the patch to be force-until-exit.
> 
> The LayoutTest behavior was removed in crrev.com/416170, and it was part of
> layout_test_helper.
> 
> Is it okay to move forward with this, or does it pose immediate problems? It
> would be easy to add a program to a BUILD.gn that would set these color
profiles
> -- the hard part would be to ensure that it is built appropriately and
> integrated into the test harness. I'm happy to commit to doing that if we at
> some point are unable to use the Python implementation.

Based on Dan's comments, I think it's okay to move forward.

[ccameron, 2017-06-21 18:19:12]

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

[commit-bot: I haz the power, 2017-06-21 18:19:22]

Dry run: CQ is trying da patch.

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

[dnj, 2017-06-21 18:29:19]

I've confirmed that within a VirtualEnv:

$ pip install pyobjc

Is sufficient to install the set of wheels that enable these imports, so it
seems VirtualEnv safe!

[commit-bot: I haz the power, 2017-06-21 18:56:07]

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

[commit-bot: I haz the power, 2017-06-21 18:56:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ken Russell (switch to Gerrit), 2017-06-21 19:17:59]

Great. LGTM once this is passing the trybots.

CC'ing jbauman as FYI about the video tests.

[ccameron, 2017-06-21 19:31:13]

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

[commit-bot: I haz the power, 2017-06-21 19:31:25]

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-06-21 21:18:23]

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

[commit-bot: I haz the power, 2017-06-21 21:18:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ccameron, 2017-06-21 21:35:08]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-06-21 21:35:37]

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-06-21 23:43:14]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1498080908259950,
"parent_rev": "71e3138745bebe1b95e32ef86dcc928d4d4bf5c7", "commit_rev":
"b80ffde89677d5d2b83ca25c221a768fa0029a9b"}

[commit-bot: I haz the power, 2017-06-21 23:43:29]

Description was changed from

==========
Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

Mark video-based tests on Mac as failing, since their output will
change with this patch (because CoreAnimation will be doing different
color conversion).

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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

==========
Force sRGB color profile on Mac for pixel tests

This is following the behavior that was used for layout tests up
until its remove in crrev.com/416170

The pixel tests take screenshots using CGWindowListCreateImage,
which is always in the output device's color space. To get
deterministic results, force the output device's color space to be
sRGB for the duration of these tests.

Mark video-based tests on Mac as failing, since their output will
change with this patch (because CoreAnimation will be doing different
color conversion).

BUG=734255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2944733002
Cr-Commit-Position: refs/heads/master@{#481358}
Committed:
https://chromium.googlesource.com/chromium/src/+/b80ffde89677d5d2b83ca25c221a...
==========

[commit-bot: I haz the power, 2017-06-21 23:43:30]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/b80ffde89677d5d2b83ca25c221a...