cc: Add pixel tests for transfer function shaders

cc: Add pixel tests for transfer function shaders

This ensures that our shaders produce the same results as the CPU based
gfx::ColorTransform functions.

Refactor common parts to transfer function evaluation into a separate
color transform step base class.

Add shader code generation for almost all remaining steps.

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

Review-Url: https://codereview.chromium.org/2769863003
Cr-Commit-Position: refs/heads/master@{#459387}
Committed: https://chromium.googlesource.com/chromium/src/+/786fc6ecce57cdfc4cf14f467e82487adf751903

[ccameron, 2017-03-23 02:31:50]

Description was changed from

==========
cc: Add pixel tests for transfer function shaders

This ensures that our shaders produce the same results as the CPU based
gfx::ColorTransform functions.

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

to

==========
cc: Add pixel tests for transfer function shaders

This ensures that our shaders produce the same results as the CPU based
gfx::ColorTransform functions.

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

[ccameron, 2017-03-23 02:36:16]

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

[commit-bot: I haz the power, 2017-03-23 02:36:42]

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-23 03:06:02]

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

[commit-bot: I haz the power, 2017-03-23 03:06:14]

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-23 03:31:37]

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

[commit-bot: I haz the power, 2017-03-23 03:31:38]

Dry run: This issue passed the CQ dry run.

[ccameron, 2017-03-23 06:27:10]

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

[commit-bot: I haz the power, 2017-03-23 06:27:23]

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-23 06:33:19]

Description was changed from

==========
cc: Add pixel tests for transfer function shaders

This ensures that our shaders produce the same results as the CPU based
gfx::ColorTransform functions.

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

to

==========
cc: Add pixel tests for transfer function shaders

This ensures that our shaders produce the same results as the CPU based
gfx::ColorTransform functions.

Refactor common parts to transfer function evaluation into a separate
color transform step base class.

Add shader code generation for almost all remaining steps.

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

[ccameron, 2017-03-23 06:33:19]

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

[ccameron, 2017-03-23 06:41:41]

ptal

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:83: 
This was causing a single-frame flash of strange data (the LUT, interpreted as a
quad texture).

https://codereview.chromium.org/2769863003/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3010: } else {
Moving this allows me to test YUV shaders in the same test framework (no other
reason for the move).

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:3534: // gfx::ColorSpace(),
The errors in this case were very large (like ~16/255), so I suspect that there
may be an off-by-one somewhere. I'll investigate this in a follow-on.

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (left):

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:665: switch (from.transfer_) {
The "transfer functions approximately cancel" functionality generalizes this.

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:663: 
Moving this to a separate class since it's unique.

[hubbe, 2017-03-23 18:09:33]

lgtm

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:83: 
On 2017/03/23 06:41:40, ccameron wrote:
> This was causing a single-frame flash of strange data (the LUT, interpreted as
a
> quad texture).

Is this GetInteger slow?
(IE, does it have to fetch data from the GPU process?)
Or does it fetch data from locally cached state?

https://codereview.chromium.org/2769863003/diff/40001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:3010: } else {
On 2017/03/23 06:41:40, ccameron wrote:
> Moving this allows me to test YUV shaders in the same test framework (no other
> reason for the move).

I think this check is iffy anyways. :)

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:3534: // gfx::ColorSpace(),
On 2017/03/23 06:41:40, ccameron wrote:
> The errors in this case were very large (like ~16/255), so I suspect that
there
> may be an off-by-one somewhere. I'll investigate this in a follow-on.

The LUTs are inherently less accurate, but 16/255 seems like a lot...
Thanks for looking into it (or I can if you prefer, since I wrote it.)

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (left):

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:665: switch (from.transfer_) {
On 2017/03/23 06:41:40, ccameron wrote:
> The "transfer functions approximately cancel" functionality generalizes this.

Acknowledged.

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:428: bool extended_ = false;
Isn't it better to let the individual transfer functions handle this?
Also, it might be interesting to drop this part if we know that the input is in
0-1 range.

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:663: 
On 2017/03/23 06:41:40, ccameron wrote:
> Moving this to a separate class since it's unique.

Acknowledged.

[ccameron, 2017-03-23 19:21:51]

thanks!

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:83: 
On 2017/03/23 18:09:33, hubbe wrote:
> On 2017/03/23 06:41:40, ccameron wrote:
> > This was causing a single-frame flash of strange data (the LUT, interpreted
as
> a
> > quad texture).
> 
> Is this GetInteger slow?
> (IE, does it have to fetch data from the GPU process?)
> Or does it fetch data from locally cached state?

I'm not 100% sure what the internal implementation is, but I suspect that we
shadow the data on the client side.

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:3534: // gfx::ColorSpace(),
On 2017/03/23 18:09:33, hubbe wrote:
> On 2017/03/23 06:41:40, ccameron wrote:
> > The errors in this case were very large (like ~16/255), so I suspect that
> there
> > may be an off-by-one somewhere. I'll investigate this in a follow-on.
> 
> The LUTs are inherently less accurate, but 16/255 seems like a lot...
> Thanks for looking into it (or I can if you prefer, since I wrote it.)

I found that if I change the test image to be 17x4, so that it exactly matches
the LUT, then the errors are pretty small (closer to 3-4), so I've done this in
the test. I'll take a closer look in a bit.

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/ui/gfx/color_transform....
ui/gfx/color_transform.cc:428: bool extended_ = false;
On 2017/03/23 18:09:33, hubbe wrote:
> Isn't it better to let the individual transfer functions handle this?
> Also, it might be interesting to drop this part if we know that the input is
in
> 0-1 range.

It seemed easier to put here because the symmetry can be accomplished by
changing the arguments to the per-channel transfer function.

Yes, I like the idea knowledge of "the input is coming from 0-1" to
ColorTransform. We have this a bit in our tone mapping for some HDR transfer
functions, but we may be able to generalize that. I'll keep it in mind.

For now, the only time that the point-symmetry kicks in when we're doing HDR
rendering (and we use the point-symmetry-extended space for blending).

[hubbe, 2017-03-23 19:30:40]

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/color_lut_cac...
cc/output/color_lut_cache.cc:83: 
On 2017/03/23 19:21:51, ccameron wrote:
> On 2017/03/23 18:09:33, hubbe wrote:
> > On 2017/03/23 06:41:40, ccameron wrote:
> > > This was causing a single-frame flash of strange data (the LUT,
interpreted
> as
> > a
> > > quad texture).
> > 
> > Is this GetInteger slow?
> > (IE, does it have to fetch data from the GPU process?)
> > Or does it fetch data from locally cached state?
> 
> I'm not 100% sure what the internal implementation is, but I suspect that we
> shadow the data on the client side.

If we do not, we should probably investigate if we can just move the GetLUT call
to somewhere where texture binding doesn't mess up the state...

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2769863003/diff/40001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:3534: // gfx::ColorSpace(),
On 2017/03/23 19:21:51, ccameron wrote:
> On 2017/03/23 18:09:33, hubbe wrote:
> > On 2017/03/23 06:41:40, ccameron wrote:
> > > The errors in this case were very large (like ~16/255), so I suspect that
> > there
> > > may be an off-by-one somewhere. I'll investigate this in a follow-on.
> > 
> > The LUTs are inherently less accurate, but 16/255 seems like a lot...
> > Thanks for looking into it (or I can if you prefer, since I wrote it.)
> 
> I found that if I change the test image to be 17x4, so that it exactly matches
> the LUT, then the errors are pretty small (closer to 3-4), so I've done this
in
> the test. I'll take a closer look in a bit.

One thing that we really should do is to adjust the LUT so that the average is
right instead of just point-sampling the function. I haven't figured out a cheap
way to do that yet though.

[ccameron, 2017-03-23 22:08:10]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-03-23 22:08:10]

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/2769863003/#ps60001
(title: "Make test sample LUTs at their input samples")

[commit-bot: I haz the power, 2017-03-23 22:09:18]

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-23 22:44:47]

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

[commit-bot: I haz the power, 2017-03-23 22:44:48]

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-24 03:40:35]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-03-24 03:40:35]

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/2769863003/#ps80001
(title: "Leave transfer substitutions unchanged")

[commit-bot: I haz the power, 2017-03-24 03:41: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-24 05:30:10]

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

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

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

[ccameron, 2017-03-24 08:00:30]

The CQ bit was checked by ccameron@chromium.org

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

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-24 11:16:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490342430629430,
"parent_rev": "c8ae0d577aa536d84ce37a570dd7c26709ea25c6", "commit_rev":
"786fc6ecce57cdfc4cf14f467e82487adf751903"}

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

Description was changed from

==========
cc: Add pixel tests for transfer function shaders

This ensures that our shaders produce the same results as the CPU based
gfx::ColorTransform functions.

Refactor common parts to transfer function evaluation into a separate
color transform step base class.

Add shader code generation for almost all remaining steps.

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

to

==========
cc: Add pixel tests for transfer function shaders

This ensures that our shaders produce the same results as the CPU based
gfx::ColorTransform functions.

Refactor common parts to transfer function evaluation into a separate
color transform step base class.

Add shader code generation for almost all remaining steps.

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

Review-Url: https://codereview.chromium.org/2769863003
Cr-Commit-Position: refs/heads/master@{#459387}
Committed:
https://chromium.googlesource.com/chromium/src/+/786fc6ecce57cdfc4cf14f467e82...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/786fc6ecce57cdfc4cf14f467e82...