cc: Move output color space from DrawingFrame to RenderPass

cc: Move output color space from DrawingFrame to RenderPass

This assigns a color space to each RenderPass during surface
aggregation. For the moment, this just means using the output
color space, but in the future it can be used to create a final
color conversion render pass for exotic color spaces.

The OutputSurface is assigned the color space of the root
RenderPass.

Add tests to ensure that the color spaces changes are propagated
through to the RenderPass and the OutputSurface.

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

Review-Url: https://codereview.chromium.org/2693723002
Cr-Commit-Position: refs/heads/master@{#450151}
Committed: https://chromium.googlesource.com/chromium/src/+/625dae1ec4772b0842e701d5771b4b08154d0df6

[ccameron, 2017-02-12 03:49:39]

Description was changed from

==========
cc: Move output color space from DrawingFrame to RenderPass


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

to

==========
cc: Move output color space from DrawingFrame to RenderPass


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

[ccameron, 2017-02-12 03:49:47]

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

[commit-bot: I haz the power, 2017-02-12 03:49:52]

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-02-12 04:02:27]

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

[commit-bot: I haz the power, 2017-02-12 04:02:28]

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-13 00:29:04]

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

[commit-bot: I haz the power, 2017-02-13 00:29:11]

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-13 00:31:36]

Description was changed from

==========
cc: Move output color space from DrawingFrame to RenderPass


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

to

==========
cc: Move output color space from DrawingFrame to RenderPass

This assigns a color space to each RenderPass during surface
aggregation. For the moment, this just means using the output
color space, but in the future it can be used to create a final
color conversion render pass for exotic color spaces.

The OutputSurface is assigned the color space of the root
RenderPass.

Add tests to ensure that the color spaces changes are propagated
through to the RenderPass and the OutputSurface.

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

[ccameron, 2017-02-13 00:31:36]

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

[ccameron, 2017-02-13 00:32:15]

ptal -- no functional changes, but it moves things around so that the functional
change will be small.

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

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

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

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-02-13 05:07:06]

lgtm

https://codereview.chromium.org/2693723002/diff/20001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2693723002/diff/20001/cc/quads/render_pass.h#...
cc/quads/render_pass.h:125: // The color space into which content will be render
for this render pass.
rendered?

[ccameron, 2017-02-13 18:15:54]

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

[ccameron, 2017-02-13 18:15:55]

Thanks!

Adding dcheng@ for ipc

https://codereview.chromium.org/2693723002/diff/20001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/2693723002/diff/20001/cc/quads/render_pass.h#...
cc/quads/render_pass.h:125: // The color space into which content will be render
for this render pass.
On 2017/02/13 05:07:06, enne wrote:
> rendered?

Done.

[ccameron, 2017-02-13 18:16:05]

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

[commit-bot: I haz the power, 2017-02-13 18:17:03]

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-02-13 20:12:21]

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

[commit-bot: I haz the power, 2017-02-13 20:12:22]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-13 22:47:45]

We're now plumbing ColorSpace from the renderer to the gpu right? This seems
new:

looking at |custom_primary_matrix_| and |custom_transfer_params_|, I'm wondering
if there's any additional validation we should be doing.

For example, the comment for SkColorSpaceTransferFn states this:
/**
 *  Contains the coefficients for a common transfer function equation, specified
as
 *  a transformation from a curved space to linear.
 *
 *  LinearVal = C*InputVal + F        , for 0.0f <= InputVal <  D
 *  LinearVal = (A*InputVal + B)^G + E, for D    <= InputVal <= 1.0f
 *
 *  Function is undefined if InputVal is not in [ 0.0f, 1.0f ].
 *  Resulting LinearVals must be in [ 0.0f, 1.0f ].
 *  Function must be positive and increasing.
 */

but it seems trivial to send values which would make this untrue.

https://codereview.chromium.org/2693723002/diff/40001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2693723002/diff/40001/cc/ipc/cc_param_traits....
cc/ipc/cc_param_traits.cc:456: background_filters, color_space,
has_transparent_background);
Just curious, but how come ParamTraits uses SetAll() which StructTraits sets the
member directly?

[ccameron, 2017-02-13 23:28:54]

On 2017/02/13 22:47:45, dcheng wrote:
> We're now plumbing ColorSpace from the renderer to the gpu right?

Yes, I should have pointed that out -- this is indeed something new.

That plumbing is not used yet (it's part of the mus display re-architecture),
but I wanted to make sure that the values came across correctly for when it is
used.

We currently send ColorSpace from renderer->browser, so renderer->gpu is
threatening. Ultimately when the ColorSpace is used in mus, it will be used by
the same code that currently uses it in the browser process (just, in the GPU
process).

> looking at |custom_primary_matrix_| and |custom_transfer_params_|, I'm
wondering
> if there's any additional validation we should be doing.
> 
> For example, the comment for SkColorSpaceTransferFn states this:
> /**
>  *  Contains the coefficients for a common transfer function equation,
specified
> as
>  *  a transformation from a curved space to linear.
>  *
>  *  LinearVal = C*InputVal + F        , for 0.0f <= InputVal <  D
>  *  LinearVal = (A*InputVal + B)^G + E, for D    <= InputVal <= 1.0f
>  *
>  *  Function is undefined if InputVal is not in [ 0.0f, 1.0f ].
>  *  Resulting LinearVals must be in [ 0.0f, 1.0f ].
>  *  Function must be positive and increasing.
>  */
> but it seems trivial to send values which would make this untrue.

We actually loosen those requirements for when we use ColorSpace in cc:: -- it's
valid to have negative values (e.g, scRGB color space) and >1 values (e.g, HDR,
and also scRGB).

It's also possible to send coefficients that will give you NaNs (e.g, A=0, B<0).

The result of this would be that the pixels drawn on the screen will be
undefined (usually black for invalid values), but that would only affect the
content that specified the invalid color space.


>
https://codereview.chromium.org/2693723002/diff/40001/cc/ipc/cc_param_traits.cc
> File cc/ipc/cc_param_traits.cc (right):
> 
>
https://codereview.chromium.org/2693723002/diff/40001/cc/ipc/cc_param_traits....
> cc/ipc/cc_param_traits.cc:456: background_filters, color_space,
> has_transparent_background);
> Just curious, but how come ParamTraits uses SetAll() which StructTraits sets
the
> member directly?

Good question ... I didn't notice that that was being done.

[dcheng, 2017-02-13 23:32:30]

ipc lgtm

[ccameron, 2017-02-13 23:32:44]

On 2017/02/13 23:28:54, ccameron wrote:
> On 2017/02/13 22:47:45, dcheng wrote:
> > We're now plumbing ColorSpace from the renderer to the gpu right?
> 
> Yes, I should have pointed that out -- this is indeed something new.
> 
> That plumbing is not used yet (it's part of the mus display re-architecture),
> but I wanted to make sure that the values came across correctly for when it is
> used.
> 
> We currently send ColorSpace from renderer->browser, so renderer->gpu is
> threatening. Ultimately when the ColorSpace is used in mus, it will be used by
> the same code that currently uses it in the browser process (just, in the GPU
> process).

Errrum, "less threatening"

[ccameron, 2017-02-13 23:36:59]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-02-13 23:37:00]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2693723002/#ps40001
(title: "Fix typo")

[commit-bot: I haz the power, 2017-02-13 23:37:40]

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-13 23:44:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487029019861010,
"parent_rev": "17c63c837dfebdfd0fce1e4fe7a6981cfa4fc939", "commit_rev":
"625dae1ec4772b0842e701d5771b4b08154d0df6"}

[commit-bot: I haz the power, 2017-02-13 23:45:26]

Description was changed from

==========
cc: Move output color space from DrawingFrame to RenderPass

This assigns a color space to each RenderPass during surface
aggregation. For the moment, this just means using the output
color space, but in the future it can be used to create a final
color conversion render pass for exotic color spaces.

The OutputSurface is assigned the color space of the root
RenderPass.

Add tests to ensure that the color spaces changes are propagated
through to the RenderPass and the OutputSurface.

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

to

==========
cc: Move output color space from DrawingFrame to RenderPass

This assigns a color space to each RenderPass during surface
aggregation. For the moment, this just means using the output
color space, but in the future it can be used to create a final
color conversion render pass for exotic color spaces.

The OutputSurface is assigned the color space of the root
RenderPass.

Add tests to ensure that the color spaces changes are propagated
through to the RenderPass and the OutputSurface.

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

Review-Url: https://codereview.chromium.org/2693723002
Cr-Commit-Position: refs/heads/master@{#450151}
Committed:
https://chromium.googlesource.com/chromium/src/+/625dae1ec4772b0842e701d5771b...
==========

[commit-bot: I haz the power, 2017-02-13 23:45:27]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/625dae1ec4772b0842e701d5771b...