cc: Compute target space transform dynamically

cc: Compute target space transform dynamically

This CL adds another code path of computing target space transforms. The
old code path is still in use as we have no idea what performance impact
would be.

The new logic is to compute the transforms on demand and cached all the
intermediate results. It is hidden behind the flag verify_transform_tree
_calculation. The flag is enabled in unit tests to verify the correct-
ness of the new logic.

BUG=624120
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/47c2a0e34fcc0c430dc8d3a75daf44061c4e19fd
Cr-Commit-Position: refs/heads/master@{#407480}

[sunxd, 2016-07-20 19:08:46]

Description was changed from

==========
cc: Compute target space transform dynamically

This CL adds another code path of computing target space transforms. The
old code path is still in use as we have no idea what performance impact
would be.

The new logic is to compute the transforms on demand and cached all the
intermediate results. It is hidden behind the flag verify_transform_tree
_calculation. The flag is enabled in unit tests to verify the correct-
ness of the new logic.

BUG=624120
==========

to

==========
cc: Compute target space transform dynamically

This CL adds another code path of computing target space transforms. The
old code path is still in use as we have no idea what performance impact
would be.

The new logic is to compute the transforms on demand and cached all the
intermediate results. It is hidden behind the flag verify_transform_tree
_calculation. The flag is enabled in unit tests to verify the correct-
ness of the new logic.

BUG=624120
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-07-20 19:46:22]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org, jaydasika@chromium.org, weiliangc@chromium.org

[sunxd, 2016-07-20 23:03:10]

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

[commit-bot: I haz the power, 2016-07-20 23:04:07]

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

[jaydasika, 2016-07-20 23:34:57]

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:269: // The stored target space transform has sublayer
scale baked in, but we
sublayer scale got renamed to surface contents scale. So, leave it as 'surface
contents scale' in the comment.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1905: const EffectNode* effect_node =
effect_tree.Node(effect_id);
Can you add a DCHECK(effect_node->has_render_surface) here ?

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1909: transform_node->surface_contents_scale.y());
Can you use the surface_contents_scale from effect node? This condition is true
when the transform node is created for the surface. So you could check
effect_node->transform_id == transform_id instead of
needs_surface_contents_scale and then use the scale in effect node. (You may
need to handle root separately)

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1912: DCHECK(transform_id >=
effect_node->transform_id);
nit : Use DCHECK_GE. Or actually, this could be DCHECK_GT

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1927: // sublayer scale
I don't see how this can be true (or maybe true only for root node). If
transform_id == effect_node->transform_id, then needs_surface_contents_scale
will be true and we won't enter the else (in line 1910)

[commit-bot: I haz the power, 2016-07-20 23:40:18]

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

[commit-bot: I haz the power, 2016-07-20 23:40:19]

Dry run: 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_...)

[sunxd, 2016-07-21 18:43:08]

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1905: const EffectNode* effect_node =
effect_tree.Node(effect_id);
On 2016/07/20 23:34:57, jaydasika wrote:
> Can you add a DCHECK(effect_node->has_render_surface) here ?

Done.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1909: transform_node->surface_contents_scale.y());
On 2016/07/20 23:34:57, jaydasika wrote:
> Can you use the surface_contents_scale from effect node? This condition is
true
> when the transform node is created for the surface. So you could check
> effect_node->transform_id == transform_id instead of
> needs_surface_contents_scale and then use the scale in effect node. (You may
> need to handle root separately)

I think in some situations we want to take surface contents scale of the source
node into consideration, even if src != dst. So I think we still need to judge
it using the needs_surface_contents_scale here and keep an if-branch for the
case where src == dst. This is the case in some unit tests like
LayerTreeHostCommonTest.ClipRectWhenCannotRenderToSeparateSurface, but I'm not
sure if needing contents scale implies src == dst in the real case.

Also I'm uncertain if we can use the dest node / effect node's surface contents
scale instead of the one of the src node. Unit test
PropertyTreeTestTransformsWithFlattening would fail if I change it.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1912: DCHECK(transform_id >=
effect_node->transform_id);
On 2016/07/20 23:34:57, jaydasika wrote:
> nit : Use DCHECK_GE. Or actually, this could be DCHECK_GT

Done.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1912: DCHECK(transform_id >=
effect_node->transform_id);
On 2016/07/20 23:34:57, jaydasika wrote:
> nit : Use DCHECK_GE. Or actually, this could be DCHECK_GT

Done.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1927: // sublayer scale
On 2016/07/20 23:34:57, jaydasika wrote:
> I don't see how this can be true (or maybe true only for root node). If
> transform_id == effect_node->transform_id, then needs_surface_contents_scale
> will be true and we won't enter the else (in line 1910)

See the above comment.

[jaydasika, 2016-07-21 19:32:13]

lgtm % (ajuma or weiliangc)

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1909: transform_node->surface_contents_scale.y());
On 2016/07/21 18:43:08, sunxd wrote:
> On 2016/07/20 23:34:57, jaydasika wrote:
> > Can you use the surface_contents_scale from effect node? This condition is
> true
> > when the transform node is created for the surface. So you could check
> > effect_node->transform_id == transform_id instead of
> > needs_surface_contents_scale and then use the scale in effect node. (You may
> > need to handle root separately)
> 
> I think in some situations we want to take surface contents scale of the
source
> node into consideration, even if src != dst. So I think we still need to judge
> it using the needs_surface_contents_scale here and keep an if-branch for the
> case where src == dst. This is the case in some unit tests like
> LayerTreeHostCommonTest.ClipRectWhenCannotRenderToSeparateSurface, but I'm not
> sure if needing contents scale implies src == dst in the real case.
> 
> Also I'm uncertain if we can use the dest node / effect node's surface
contents
> scale instead of the one of the src node. Unit test
> PropertyTreeTestTransformsWithFlattening would fail if I change it.

Its confusing that we want to use src surface content scale for target space
transform when src != dst and dst never gets used.(We should probably call
target space transform using content target id instead of target id)  
Having said that, I think this CL is trying to do what we already do using an
effect node instead of transform node. So, as far as this CL goes, I am fine,
but that confusion needs to be sorted out.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1927: // sublayer scale
nit : surface contents scale

https://codereview.chromium.org/2166043002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:271: // need the unscaled transform.
nit : weird formatting in the comment

[sunxd, 2016-07-21 21:49:58]

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

[sunxd, 2016-07-21 21:50:08]

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:269: // The stored target space transform has sublayer
scale baked in, but we
On 2016/07/20 23:34:57, jaydasika wrote:
> sublayer scale got renamed to surface contents scale. So, leave it as 'surface
> contents scale' in the comment.

Done.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1909: transform_node->surface_contents_scale.y());
On 2016/07/21 19:32:12, jaydasika wrote:
> On 2016/07/21 18:43:08, sunxd wrote:
> > On 2016/07/20 23:34:57, jaydasika wrote:
> > > Can you use the surface_contents_scale from effect node? This condition is
> > true
> > > when the transform node is created for the surface. So you could check
> > > effect_node->transform_id == transform_id instead of
> > > needs_surface_contents_scale and then use the scale in effect node. (You
may
> > > need to handle root separately)
> > 
> > I think in some situations we want to take surface contents scale of the
> source
> > node into consideration, even if src != dst. So I think we still need to
judge
> > it using the needs_surface_contents_scale here and keep an if-branch for the
> > case where src == dst. This is the case in some unit tests like
> > LayerTreeHostCommonTest.ClipRectWhenCannotRenderToSeparateSurface, but I'm
not
> > sure if needing contents scale implies src == dst in the real case.
> > 
> > Also I'm uncertain if we can use the dest node / effect node's surface
> contents
> > scale instead of the one of the src node. Unit test
> > PropertyTreeTestTransformsWithFlattening would fail if I change it.
> 
> Its confusing that we want to use src surface content scale for target space
> transform when src != dst and dst never gets used.(We should probably call
> target space transform using content target id instead of target id)  
> Having said that, I think this CL is trying to do what we already do using an
> effect node instead of transform node. So, as far as this CL goes, I am fine,
> but that confusion needs to be sorted out.
>  

I sorted out that the problem is when we disable rendering to separate surfaces,
and now it's not influencing the check here.

https://codereview.chromium.org/2166043002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1927: // sublayer scale
On 2016/07/21 19:32:12, jaydasika wrote:
> nit : surface contents scale

Done.

https://codereview.chromium.org/2166043002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:271: // need the unscaled transform.
On 2016/07/21 19:32:12, jaydasika wrote:
> nit : weird formatting in the comment

Done.

[commit-bot: I haz the power, 2016-07-21 21:50:47]

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

[jaydasika, 2016-07-21 22:03:34]

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1170: return
effect_tree.Node(effect_node->target_id)->render_surface;
Is this change required ? If target_effect_tree_index() checks render_surface
instead of has_render_surface like in the earlier patch, this won't be required,
right ?

[commit-bot: I haz the power, 2016-07-21 23:00:39]

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

[commit-bot: I haz the power, 2016-07-21 23:00:40]

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

[sunxd, 2016-07-22 14:41:17]

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1170: return
effect_tree.Node(effect_node->target_id)->render_surface;
On 2016/07/21 22:03:34, jaydasika wrote:
> Is this change required ? If target_effect_tree_index() checks render_surface
> instead of has_render_surface like in the earlier patch, this won't be
required,
> right ?

The render_target function is sometimes called before we ever create a render
target, in another word, at this time, effect_node->has_render_surface == true
but effect_node->render_surface == nullptr as we didn't create it.

[ajuma, 2016-07-22 15:05:48]

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1170: return
effect_tree.Node(effect_node->target_id)->render_surface;
On 2016/07/22 14:41:17, sunxd wrote:
> On 2016/07/21 22:03:34, jaydasika wrote:
> > Is this change required ? If target_effect_tree_index() checks
render_surface
> > instead of has_render_surface like in the earlier patch, this won't be
> required,
> > right ?
> 
> The render_target function is sometimes called before we ever create a render
> target, in another word, at this time, effect_node->has_render_surface == true
> but effect_node->render_surface == nullptr as we didn't create it.

What's an example of such a callsite? One thing to worry about the way
render_target_effect_tree_index is currently defined is that for resourceless
software mode, non-root effect nodes might have has_render_surface = true even
though they can't have render surfaces (that is, will always have render_surface
== null), so defining render_target_effect_tree_index this way won't return the
effect_tree_index of the actual render target.

[sunxd, 2016-07-22 15:32:14]

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2166043002/diff/60001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1170: return
effect_tree.Node(effect_node->target_id)->render_surface;
On 2016/07/22 15:05:48, ajuma wrote:
> On 2016/07/22 14:41:17, sunxd wrote:
> > On 2016/07/21 22:03:34, jaydasika wrote:
> > > Is this change required ? If target_effect_tree_index() checks
> render_surface
> > > instead of has_render_surface like in the earlier patch, this won't be
> > required,
> > > right ?
> > 
> > The render_target function is sometimes called before we ever create a
render
> > target, in another word, at this time, effect_node->has_render_surface ==
true
> > but effect_node->render_surface == nullptr as we didn't create it.
> 
> What's an example of such a callsite? One thing to worry about the way
> render_target_effect_tree_index is currently defined is that for resourceless
> software mode, non-root effect nodes might have has_render_surface = true even
> though they can't have render surfaces (that is, will always have
render_surface
> == null), so defining render_target_effect_tree_index this way won't return
the
> effect_tree_index of the actual render target.

I think that's when we cannot render to separate surfaces. Should I define it so
that it returns the root id in this case?

[sunxd, 2016-07-22 15:52:53]

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

[commit-bot: I haz the power, 2016-07-22 15:53:08]

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, 2016-07-22 17:02:10]

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

[commit-bot: I haz the power, 2016-07-22 17:02:11]

Dry run: This issue passed the CQ dry run.

[ajuma, 2016-07-22 18:29:41]

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:987: if (effect_node->render_surface)
I think this is always null on the main thread side. So I think this function is
currently always returning the root effect node's id. Did you mean to use
has_render_surface here (on the main thread side it makes sense to do that,
since there's no resourceless software mode to worry about). It also suggests
that the remaining tests that depend on this logic aren't doing interesting
things with surfaces.

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.h#newco...
cc/layers/layer.h:434: // main thread in tests
Nit (here and elsewhere): Comments should be sentences (so start with a capital
letter, end with a period).

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.cc:1890: target_space_transform.MakeIdentity();
I think target_space_transform is already the identity here.

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.cc:1918: transform_node->ancestors_are_invertible =
false;
With the way this is currently being tested, this will change the behavior of
the existing code path (since it changes the contents of the transform node).
Maybe this bit (whether the draw transform is invertible) could be cached
separately for the new code path?

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/transform_node.h
File cc/trees/transform_node.h (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/transform_node...
cc/trees/transform_node.h:73: mutable bool ancestors_are_invertible : 1;
See earlier comment about this. Instead of allowing the new code path to mutate
this value, perhaps it can separately cache whether the draw transform is
invertible.

[sunxd, 2016-07-22 20:25:04]

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:987: if (effect_node->render_surface)
On 2016/07/22 18:29:40, ajuma wrote:
> I think this is always null on the main thread side. So I think this function
is
> currently always returning the root effect node's id. Did you mean to use
> has_render_surface here (on the main thread side it makes sense to do that,
> since there's no resourceless software mode to worry about). It also suggests
> that the remaining tests that depend on this logic aren't doing interesting
> things with surfaces.

Done.

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/layers/layer.h#newco...
cc/layers/layer.h:434: // main thread in tests
On 2016/07/22 18:29:40, ajuma wrote:
> Nit (here and elsewhere): Comments should be sentences (so start with a
capital
> letter, end with a period).

Done.

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.cc:1890: target_space_transform.MakeIdentity();
On 2016/07/22 18:29:40, ajuma wrote:
> I think target_space_transform is already the identity here.

Done.

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/property_tree....
cc/trees/property_tree.cc:1918: transform_node->ancestors_are_invertible =
false;
On 2016/07/22 18:29:40, ajuma wrote:
> With the way this is currently being tested, this will change the behavior of
> the existing code path (since it changes the contents of the transform node).
> Maybe this bit (whether the draw transform is invertible) could be cached
> separately for the new code path?

Done.

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/transform_node.h
File cc/trees/transform_node.h (right):

https://codereview.chromium.org/2166043002/diff/80001/cc/trees/transform_node...
cc/trees/transform_node.h:73: mutable bool ancestors_are_invertible : 1;
On 2016/07/22 18:29:40, ajuma wrote:
> See earlier comment about this. Instead of allowing the new code path to
mutate
> this value, perhaps it can separately cache whether the draw transform is
> invertible.

Done.

[sunxd, 2016-07-22 21:32:39]

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

[commit-bot: I haz the power, 2016-07-22 21:33:26]

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

[ajuma, 2016-07-22 22:32:12]

Thanks, lgtm

[commit-bot: I haz the power, 2016-07-22 22:46:36]

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

[commit-bot: I haz the power, 2016-07-22 22:46:37]

Dry run: This issue passed the CQ dry run.

[sunxd, 2016-07-25 14:37:17]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-07-25 14:37:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
jaydasika@chromium.org
Link to the patchset: https://codereview.chromium.org/2166043002/#ps140001
(title: "Revert ancestors_are_invertible change")

[commit-bot: I haz the power, 2016-07-25 14:37:32]

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, 2016-07-25 15:34:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-25 15:36:17]

Description was changed from

==========
cc: Compute target space transform dynamically

This CL adds another code path of computing target space transforms. The
old code path is still in use as we have no idea what performance impact
would be.

The new logic is to compute the transforms on demand and cached all the
intermediate results. It is hidden behind the flag verify_transform_tree
_calculation. The flag is enabled in unit tests to verify the correct-
ness of the new logic.

BUG=624120
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
cc: Compute target space transform dynamically

This CL adds another code path of computing target space transforms. The
old code path is still in use as we have no idea what performance impact
would be.

The new logic is to compute the transforms on demand and cached all the
intermediate results. It is hidden behind the flag verify_transform_tree
_calculation. The flag is enabled in unit tests to verify the correct-
ness of the new logic.

BUG=624120
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel

Committed: https://crrev.com/47c2a0e34fcc0c430dc8d3a75daf44061c4e19fd
Cr-Commit-Position: refs/heads/master@{#407480}
==========

[commit-bot: I haz the power, 2016-07-25 15:36:18]

Patchset 8 (id:??) landed as
https://crrev.com/47c2a0e34fcc0c430dc8d3a75daf44061c4e19fd
Cr-Commit-Position: refs/heads/master@{#407480}