color: Ensure that VideoResourceUpdater give consistent colors

color: Ensure that VideoResourceUpdater give consistent colors

Update VideoResourceUpdater to use the right color spaces. In particular,
some of the resources from VideoResourceUpdater will have YUV to RGB
conversion done ahead of time, so ensure that we don't specify that these
resources are in YUV color space (or they will be converted twice, and look
wrong).

Add lots of DCHECKs to ensure that only YUV draw quads are using YUV
resources.

In order to use DCHECK_EQ with color spaces, add some helper functions
to print color spaces as vaguely-JSON-like strings.

While we're adding a function to split off the range-and-YUV part of a
color space, use that function to clean up some of the confusing
behavior of ColorSpace::GetICCProfile.

BUG=699243
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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/2738713003
Cr-Commit-Position: refs/heads/master@{#456673}
Committed: https://chromium.googlesource.com/chromium/src/+/5a5328db5be00d556034c92224f05904e863b410

[ccameron, 2017-03-07 23:28:50]

Description was changed from

==========
Split RGB

BUG=
==========

to

==========
Split RGB

BUG=
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-03-07 23:28:50]

Description was changed from

==========
Split RGB

BUG=
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

==========
Split RGB

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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-03-07 23:28:52]

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

[commit-bot: I haz the power, 2017-03-07 23:29:54]

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-08 00:01:39]

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

[commit-bot: I haz the power, 2017-03-08 00:03:19]

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-08 00:22:36]

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

[commit-bot: I haz the power, 2017-03-08 00:23:16]

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-08 00:27:29]

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

[commit-bot: I haz the power, 2017-03-08 00:27:30]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2017-03-08 01:19:13]

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

[commit-bot: I haz the power, 2017-03-08 01:20:04]

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-08 01:23:07]

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

[commit-bot: I haz the power, 2017-03-08 01:23:30]

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-08 02:40:21]

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

[commit-bot: I haz the power, 2017-03-08 02:40:22]

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-03-08 20:48:38]

Patchset #5 (id:80001) has been deleted

[ccameron, 2017-03-08 20:48:52]

Patchset #4 (id:60001) has been deleted

[ccameron, 2017-03-08 20:49:04]

Patchset #3 (id:40001) has been deleted

[ccameron, 2017-03-09 01:23:50]

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

[commit-bot: I haz the power, 2017-03-09 01:24:47]

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 01:27:47]

Description was changed from

==========
Split RGB

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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

==========
color: Ensure that VideoResourceUpdater give consistent colors

Update VideoResourceUpdater to use the right color spaces. In particular,
some of the resources from VideoResourceUpdater will have YUV to RGB
conversion done ahead of time, so ensure that we don't specify that these
resources are in YUV color space (or they will be converted twice, and look
wrong).

Add lots of DCHECKs to ensure that only YUV draw quads are using YUV
resources.

In order to use DCHECK_EQ with color spaces, add some helper functions
to print color spaces as vaguely-JSON-like strings.

While we're adding a function to split off the range-and-YUV part of a
color space, use that function to clean up some of the confusing
behavior of ColorSpace::GetICCProfile.

BUG=699243
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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-03-09 01:27:47]

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

[ccameron, 2017-03-09 01:29:58]

ptal -- this is the "better fix" for crbug.com/699123

[commit-bot: I haz the power, 2017-03-09 03:11:24]

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

[commit-bot: I haz the power, 2017-03-09 03:11:25]

Dry run: This issue passed the CQ dry run.

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

ping

[hubbe, 2017-03-13 17:53:45]

lgtm

https://codereview.chromium.org/2738713003/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2738713003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3000: // The source color space for non-YUV draw quads
should always be full-range
I can think of cases when we might not want this check in the future, but it
should be OK for now. (4:4:4 movies or JPEGs could use RGBA textures + color
management for presentation.)

https://codereview.chromium.org/2738713003/diff/100001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2738713003/diff/100001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:759: resource_color_space =
resource_color_space.GetAsFullRangeRGB();
If the YUV->RGB matrix applied by the hardware doesn't match the one requested
by the color space, we should correct for that by using custom primaries.

Not sure how we find out what matrix the hardware will be using though.
Maybe add a TODO somewhere?

Same thing applies in other places where you use GetAsFullRangeRGB().

[ccameron, 2017-03-13 22:16:55]

Thanks!

https://codereview.chromium.org/2738713003/diff/100001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2738713003/diff/100001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:3000: // The source color space for non-YUV draw quads
should always be full-range
On 2017/03/13 17:53:45, hubbe wrote:
> I can think of cases when we might not want this check in the future, but it
> should be OK for now. (4:4:4 movies or JPEGs could use RGBA textures + color
> management for presentation.)

Yes, hopefully soon we'll be able to then up the DCHECKs here (e.g, to DCHECK
that src_color_space be valid). At that point we may want to tweak this a bit
more.

https://codereview.chromium.org/2738713003/diff/100001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2738713003/diff/100001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:759: resource_color_space =
resource_color_space.GetAsFullRangeRGB();
On 2017/03/13 17:53:45, hubbe wrote:
> If the YUV->RGB matrix applied by the hardware doesn't match the one requested
> by the color space, we should correct for that by using custom primaries.
> 
> Not sure how we find out what matrix the hardware will be using though.
> Maybe add a TODO somewhere?
> 
> Same thing applies in other places where you use GetAsFullRangeRGB().

Yes, hardware-performed YUV->RGB at sample time will be harder to ensure
correctness on. Fortunately we've stopped doing this on Mac, but we may do it on
other platforms.

The thing that I have in mind is stuff that does YUV->RGB inside the command
buffer, via this structure
https://cs.chromium.org/chromium/src/ui/gl/yuv_to_rgb_converter.h?rcl=7d5e412...

For that case we can ensure that things match, if we send to the command buffer
the parameters of the transformation (which can be computed from a
gfx::ColorTransform).

[ccameron, 2017-03-13 22:17:04]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-03-13 22:17:05]

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/2738713003/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-13 22:18: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-03-14 00:18:16]

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

[commit-bot: I haz the power, 2017-03-14 00:18:20]

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

[ccameron, 2017-03-14 10:18:39]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-03-14 10:19:00]

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 11:45:49]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489486719518490,
"parent_rev": "e0562233ef75682dfe43ccb703d255e4d2a4432e", "commit_rev":
"5a5328db5be00d556034c92224f05904e863b410"}

[commit-bot: I haz the power, 2017-03-14 11:46:32]

Description was changed from

==========
color: Ensure that VideoResourceUpdater give consistent colors

Update VideoResourceUpdater to use the right color spaces. In particular,
some of the resources from VideoResourceUpdater will have YUV to RGB
conversion done ahead of time, so ensure that we don't specify that these
resources are in YUV color space (or they will be converted twice, and look
wrong).

Add lots of DCHECKs to ensure that only YUV draw quads are using YUV
resources.

In order to use DCHECK_EQ with color spaces, add some helper functions
to print color spaces as vaguely-JSON-like strings.

While we're adding a function to split off the range-and-YUV part of a
color space, use that function to clean up some of the confusing
behavior of ColorSpace::GetICCProfile.

BUG=699243
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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

==========
color: Ensure that VideoResourceUpdater give consistent colors

Update VideoResourceUpdater to use the right color spaces. In particular,
some of the resources from VideoResourceUpdater will have YUV to RGB
conversion done ahead of time, so ensure that we don't specify that these
resources are in YUV color space (or they will be converted twice, and look
wrong).

Add lots of DCHECKs to ensure that only YUV draw quads are using YUV
resources.

In order to use DCHECK_EQ with color spaces, add some helper functions
to print color spaces as vaguely-JSON-like strings.

While we're adding a function to split off the range-and-YUV part of a
color space, use that function to clean up some of the confusing
behavior of ColorSpace::GetICCProfile.

BUG=699243
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;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/2738713003
Cr-Commit-Position: refs/heads/master@{#456673}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a5328db5be00d556034c92224f0...
==========

[commit-bot: I haz the power, 2017-03-14 11:46:34]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5a5328db5be00d556034c92224f0...