Towards deleting YUV to RGB computation redundancy

Towards deleting YUV to RGB computation redundancy

Make the GetTransferMatrix and GetRangeAdjustMatrix functions part of
gfx::ColorSpace instead of gfx::ColorSpaceTransform, and call them from
GLRenderer to compute the YUV to RGB matrix and offset.

Do not actually use the new computation results yet, because they break
pixel tests. Rather, add a DCHECK that the new results are very close to
the previous values being used (within 1/255.f).

Fix the transfer matrix for SMPTE170M to have Kb=0.114 instead of 0.144.

Change the definition of gfx::ColorSpace::CreateJPEG to use the Rec601
transfer matrix, not Rec709. This is necessary for the resulting YUV to
RGB conversion to match the previous behavior.

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

Review-Url: https://codereview.chromium.org/2670773002
Cr-Commit-Position: refs/heads/master@{#447893}
Committed: https://chromium.googlesource.com/chromium/src/+/5f73187dc5b79607d4110d9d0519d1a474f01ee7

[ccameron, 2017-02-02 03:51:15]

Description was changed from

==========
Towards deleting YUV to RGB computation redundancy

Make the GetTransferMatrix and GetRangeAdjustMatrix functions part of
gfx::ColorSpace instead of gfx::ColorSpaceTransform, and call them from
GLRenderer to compute the YUV to RGB matrix and offset.

Do not actually use the new computation results yet, because they break
pixel tests. Rather, add a DCHECK that the new results are very close to
the previous values being used (within 1/255.f).

Fix the transfer matrix for SMPTE170M to have Kb=0.114 instead of 0.144.

Change the definition of gfx::ColorSpace::CreateJPEG to use the Rec601
transfer matrix, not Rec709. This is necessary for the resulting YUV to
RGB conversion to match the previous behavior.

BUG=667966
==========

to

==========
Towards deleting YUV to RGB computation redundancy

Make the GetTransferMatrix and GetRangeAdjustMatrix functions part of
gfx::ColorSpace instead of gfx::ColorSpaceTransform, and call them from
GLRenderer to compute the YUV to RGB matrix and offset.

Do not actually use the new computation results yet, because they break
pixel tests. Rather, add a DCHECK that the new results are very close to
the previous values being used (within 1/255.f).

Fix the transfer matrix for SMPTE170M to have Kb=0.114 instead of 0.144.

Change the definition of gfx::ColorSpace::CreateJPEG to use the Rec601
transfer matrix, not Rec709. This is necessary for the resulting YUV to
RGB conversion to match the previous behavior.

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

[ccameron, 2017-02-02 03:51:22]

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

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

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

[ccameron, 2017-02-02 03:54:25]

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

[ccameron, 2017-02-02 03:54:26]

PTAL.

After this, the only remaining instance of this nonsense will be at:

https://cs.chromium.org/chromium/src/ui/gl/yuv_to_rgb_converter.cc?rcl=5b2d88...

Which, FWICT, assumes Rec601...

[hubbe, 2017-02-02 04:51:37]

> Change the definition of gfx::ColorSpace::CreateJPEG to
> use the Rec601 transfer matrix, not Rec709. This is
> necessary for the resulting YUV to
> RGB conversion to match the previous behavior.

But is the previous behavior *right* ?

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2175: SkMatrix44 full_transform;
I still think GetAffineApproximation() is a better & simpler way to do this.
(https://codereview.chromium.org/2559273002/)

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2181: }
What is this {} for?
(Same for the ones below.)

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2194: gfx_color_space.GetTransferMatrix(&rgb_to_yuv);
In one case, this is is not a linear matrix...

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

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_space.cc#newco...
ui/gfx/color_space.cc:120: return ColorSpace(PrimaryID::SMPTE170M,
TransferID::IEC61966_2_1,
I thought JPEG used srgb primaries by default?

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

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:182: void GetTransferMatrix(SkMatrix44* matrix) const;
Maybe return a bool since this is not always possible?

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_transform_unit...
File ui/gfx/color_transform_unittest.cc (left):

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_transform_unit...
ui/gfx/color_transform_unittest.cc:177: TEST_P(MatrixTest, checkInvertible) {
Add color_space_unittest.cc and add this test there?

[commit-bot: I haz the power, 2017-02-02 05:06:55]

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

[commit-bot: I haz the power, 2017-02-02 05:06:56]

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_...)

[ccameron, 2017-02-02 17:21:47]

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

[commit-bot: I haz the power, 2017-02-02 17:22:17]

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

[ccameron, 2017-02-02 17:33:52]

updated -- ptal

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2175: SkMatrix44 full_transform;
On 2017/02/02 04:51:37, hubbe wrote:
> I still think GetAffineApproximation() is a better & simpler way to do this.
> (https://codereview.chromium.org/2559273002/)

This patch is solving a different problem than that would solve.

The goal of this patch is to

- ensure that the old YUV->RGB code matches the new one (considering that it
found 2 bugs, mission accomplished)
- delete redundant code (or, prepare to delete redundant code)

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2181: }
On 2017/02/02 04:51:36, hubbe wrote:
> What is this {} for?
> (Same for the ones below.)

In the instances below, it's to show to the reader that the temporary variables
are not used outside of the block. 

In this instance, it's was for consistency ... which doesn't really help, so I
got rid of it.

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2194: gfx_color_space.GetTransferMatrix(&rgb_to_yuv);
On 2017/02/02 04:51:37, hubbe wrote:
> In one case, this is is not a linear matrix...

How to handle this in the general case is a good question. For the 3 cases that
we're replacing here, there exists a RGB-to-YUV matrix.

For the BT2020_CL case, it's a RGB-to-RYB matrix (and for others its
RGB-to-YCgCo or RGB-to-YDxDy), which are similar in character, so it didn't
quite seem right to break them off.

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

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_space.cc#newco...
ui/gfx/color_space.cc:120: return ColorSpace(PrimaryID::SMPTE170M,
TransferID::IEC61966_2_1,
On 2017/02/02 04:51:37, hubbe wrote:
> I thought JPEG used srgb primaries by default?

It's not clear to me what exact spec "YUVVideoDrawQuad::JPEG" was meant to refer
to. I've changed to BT709 primaries and added a "let's find out what was
intended here" comment.

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

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:182: void GetTransferMatrix(SkMatrix44* matrix) const;
On 2017/02/02 04:51:37, hubbe wrote:
> Maybe return a bool since this is not always possible?

I was going back-and-forth on this, but in the end felt that it was more
appropriate to leave this together, since that's the structure that was already
present.

If we were to separate it out, I'd prefer that we put the full BT2020_CL
conversion in its own distinct function.

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_transform_unit...
File ui/gfx/color_transform_unittest.cc (left):

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_transform_unit...
ui/gfx/color_transform_unittest.cc:177: TEST_P(MatrixTest, checkInvertible) {
On 2017/02/02 04:51:37, hubbe wrote:
> Add color_space_unittest.cc and add this test there?

It's probably more important to ensure that the matrices are correct than that
they are invertible (esp considering how long it took to track down the
SMPTE170M error).

Once we have this plumbed through, we'll want to add some sample ->XYZD50-linear
data points.

[hubbe, 2017-02-02 18:35:26]

All the stuff blow is just expressing opinions on stuff that doesn't have to be
fixed right now.

Except for one thing: Please add some tests.

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2175: SkMatrix44 full_transform;
On 2017/02/02 17:33:52, ccameron wrote:
> On 2017/02/02 04:51:37, hubbe wrote:
> > I still think GetAffineApproximation() is a better & simpler way to do this.
> > (https://codereview.chromium.org/2559273002/)
> 
> This patch is solving a different problem than that would solve.
> 
> The goal of this patch is to
> 
> - ensure that the old YUV->RGB code matches the new one (considering that it
> found 2 bugs, mission accomplished)
> - delete redundant code (or, prepare to delete redundant code)

Matching is perhaps not the right goal since the code that tags the video frames
as JPEG/REC_601/REC_709 is not very accurate.
However, it is helpful to know if they match or not. (And *why* if they do not.)

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2181: }
On 2017/02/02 17:33:52, ccameron wrote:
> On 2017/02/02 04:51:36, hubbe wrote:
> > What is this {} for?
> > (Same for the ones below.)
> 
> In the instances below, it's to show to the reader that the temporary
variables
> are not used outside of the block. 
> 
> In this instance, it's was for consistency ... which doesn't really help, so I
> got rid of it.

For me the add cognitive load when I read the code as I keep looking to see if
there is a condition or loop before it.

https://codereview.chromium.org/2670773002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:2194: gfx_color_space.GetTransferMatrix(&rgb_to_yuv);
On 2017/02/02 17:33:52, ccameron wrote:
> On 2017/02/02 04:51:37, hubbe wrote:
> > In one case, this is is not a linear matrix...
> 
> How to handle this in the general case is a good question. For the 3 cases
that
> we're replacing here, there exists a RGB-to-YUV matrix.
> 
> For the BT2020_CL case, it's a RGB-to-RYB matrix (and for others its
> RGB-to-YCgCo or RGB-to-YDxDy), which are similar in character, so it didn't
> quite seem right to break them off.

The problem with the BT2020_CL case is that it depends on additional stuff
happening later. Also that "additional stuff" cannot be expressed as a matrix.

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

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_space.cc#newco...
ui/gfx/color_space.cc:120: return ColorSpace(PrimaryID::SMPTE170M,
TransferID::IEC61966_2_1,
On 2017/02/02 17:33:52, ccameron wrote:
> On 2017/02/02 04:51:37, hubbe wrote:
> > I thought JPEG used srgb primaries by default?
> 
> It's not clear to me what exact spec "YUVVideoDrawQuad::JPEG" was meant to
refer
> to. I've changed to BT709 primaries and added a "let's find out what was
> intended here" comment.

Yeah, YUVVideoDrawQuad::JPEG was normally just used to mean "video with values
from 0-255" it really didn't have an opinion on the primaries I think.  Either
way, that enum is too coarse-grained to accurately reflect the color space of
the video frame...

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

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_space.h#newcod...
ui/gfx/color_space.h:182: void GetTransferMatrix(SkMatrix44* matrix) const;
On 2017/02/02 17:33:52, ccameron wrote:
> On 2017/02/02 04:51:37, hubbe wrote:
> > Maybe return a bool since this is not always possible?
> 
> I was going back-and-forth on this, but in the end felt that it was more
> appropriate to leave this together, since that's the structure that was
already
> present.
> 
> If we were to separate it out, I'd prefer that we put the full BT2020_CL
> conversion in its own distinct function.

Doing so would make the implementation a lot bigger and harder to optimize.

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_transform_unit...
File ui/gfx/color_transform_unittest.cc (left):

https://codereview.chromium.org/2670773002/diff/1/ui/gfx/color_transform_unit...
ui/gfx/color_transform_unittest.cc:177: TEST_P(MatrixTest, checkInvertible) {
On 2017/02/02 17:33:52, ccameron wrote:
> On 2017/02/02 04:51:37, hubbe wrote:
> > Add color_space_unittest.cc and add this test there?
> 
> It's probably more important to ensure that the matrices are correct than that
> they are invertible (esp considering how long it took to track down the
> SMPTE170M error).
> 
> Once we have this plumbed through, we'll want to add some sample
->XYZD50-linear
> data points.

I'd prefer to add some tests now, especially since this CL corrects some bugs.
Those bugs should be covered by tests.

[commit-bot: I haz the power, 2017-02-02 18:51:11]

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

[commit-bot: I haz the power, 2017-02-02 18:51:11]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-02-02 20:37:19]

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

[ccameron, 2017-02-02 20:37:53]

Good point -- added unit tests YUV->RGB conversion for the 3 spaces that I've
verified.

[commit-bot: I haz the power, 2017-02-02 20:38:01]

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

[hubbe, 2017-02-02 20:53:57]

lgtm

[ccameron, 2017-02-02 21:01:12]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-02-02 21:01:23]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2017-02-02 21:02:29]

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-02-02 21:51:35]

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

[commit-bot: I haz the power, 2017-02-02 21:51:36]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ccameron, 2017-02-02 23:01:30]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-02-02 23:01:30]

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/2670773002/#ps60001
(title: "Explicitly mark constants as float")

[commit-bot: I haz the power, 2017-02-02 23:02:35]

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-02-03 00:18:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486076490052040,
"parent_rev": "4a3cadb2d998fb8f477d0ebc7ff13619b123a805", "commit_rev":
"5f73187dc5b79607d4110d9d0519d1a474f01ee7"}

[commit-bot: I haz the power, 2017-02-03 00:18:53]

Description was changed from

==========
Towards deleting YUV to RGB computation redundancy

Make the GetTransferMatrix and GetRangeAdjustMatrix functions part of
gfx::ColorSpace instead of gfx::ColorSpaceTransform, and call them from
GLRenderer to compute the YUV to RGB matrix and offset.

Do not actually use the new computation results yet, because they break
pixel tests. Rather, add a DCHECK that the new results are very close to
the previous values being used (within 1/255.f).

Fix the transfer matrix for SMPTE170M to have Kb=0.114 instead of 0.144.

Change the definition of gfx::ColorSpace::CreateJPEG to use the Rec601
transfer matrix, not Rec709. This is necessary for the resulting YUV to
RGB conversion to match the previous behavior.

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

to

==========
Towards deleting YUV to RGB computation redundancy

Make the GetTransferMatrix and GetRangeAdjustMatrix functions part of
gfx::ColorSpace instead of gfx::ColorSpaceTransform, and call them from
GLRenderer to compute the YUV to RGB matrix and offset.

Do not actually use the new computation results yet, because they break
pixel tests. Rather, add a DCHECK that the new results are very close to
the previous values being used (within 1/255.f).

Fix the transfer matrix for SMPTE170M to have Kb=0.114 instead of 0.144.

Change the definition of gfx::ColorSpace::CreateJPEG to use the Rec601
transfer matrix, not Rec709. This is necessary for the resulting YUV to
RGB conversion to match the previous behavior.

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

Review-Url: https://codereview.chromium.org/2670773002
Cr-Commit-Position: refs/heads/master@{#447893}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f73187dc5b79607d4110d9d0519...
==========

[commit-bot: I haz the power, 2017-02-03 00:18:54]

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