cc : Simplify layer skipping logic

cc : Simplify layer skipping logic

Since we no longer skip subtrees, we can remove the logic that supports
subtree skipping.

BUG=601088
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8185d3075363d17df8cd2348ffbc27af8740f3ca
Cr-Commit-Position: refs/heads/master@{#387320}

[jaydasika, 2016-04-12 19:33:53]

Description was changed from

==========
cc : Simplify layer skipping logic

Since we no longer skip subtrees, we can remove the logic that supports
subtree skipping.

BUG=601088
==========

to

==========
cc : Simplify layer skipping logic

Since we no longer skip subtrees, we can remove the logic that supports
subtree skipping.

BUG=601088
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-04-12 19:46:32]

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

[jaydasika, 2016-04-12 19:46:33]

PTAL

[ajuma, 2016-04-12 21:48:14]

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

https://codereview.chromium.org/1884613005/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1252: (!node->data.has_animated_opacity ||
property_trees()->is_active) &&
What if nothing triggers a property tree update on the active tree? (That is,
say we push property trees from the pending tree to the active tree, and there's
no animation to make needs_update true on the effect tree.)

[jaydasika, 2016-04-13 00:13:43]

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

https://codereview.chromium.org/1884613005/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1252: (!node->data.has_animated_opacity ||
property_trees()->is_active) &&
On 2016/04/12 21:48:14, ajuma wrote:
> What if nothing triggers a property tree update on the active tree? (That is,
> say we push property trees from the pending tree to the active tree, and
there's
> no animation to make needs_update true on the effect tree.)

Split this into 2 bools to handle the case.

[jaydasika, 2016-04-13 00:17:52]

https://codereview.chromium.org/1884613005/diff/40001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1884613005/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:5194:
EXPECT_FALSE(node->data.is_drawn_on_active);
This should be the main and pending bool.

[ajuma, 2016-04-13 14:33:15]

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

https://codereview.chromium.org/1884613005/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1252: (!node->data.has_animated_opacity ||
property_trees()->is_active) &&
On 2016/04/13 00:13:42, jaydasika wrote:
> On 2016/04/12 21:48:14, ajuma wrote:
> > What if nothing triggers a property tree update on the active tree? (That
is,
> > say we push property trees from the pending tree to the active tree, and
> there's
> > no animation to make needs_update true on the effect tree.)
> 
> Split this into 2 bools to handle the case.

Hmm. Given that the effect tree is likely to only have a small number of nodes,
how about just setting needs_update to true on it when setting it on the pending
tree? (This keeps the code simpler and avoids growing nodes for things we can
easily recompute.)

[jaydasika, 2016-04-13 18:48:42]

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

https://codereview.chromium.org/1884613005/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:1252: (!node->data.has_animated_opacity ||
property_trees()->is_active) &&
On 2016/04/13 14:33:15, ajuma wrote:
> On 2016/04/13 00:13:42, jaydasika wrote:
> > On 2016/04/12 21:48:14, ajuma wrote:
> > > What if nothing triggers a property tree update on the active tree? (That
> is,
> > > say we push property trees from the pending tree to the active tree, and
> > there's
> > > no animation to make needs_update true on the effect tree.)
> > 
> > Split this into 2 bools to handle the case.
> 
> Hmm. Given that the effect tree is likely to only have a small number of
nodes,
> how about just setting needs_update to true on it when setting it on the
pending
> tree? (This keeps the code simpler and avoids growing nodes for things we can
> easily recompute.)

Done.

[ajuma, 2016-04-13 19:53:07]

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:653: needs_update_again_after_activation);
This would also make us update every frame on the main thread, I think? How
about adding this logic to LayerTreeImpl::SetPropertyTrees (and unconditionally
setting needs_update to true for the EffectTree if the LayerTreeImpl is the
active tree)?

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.cc:1335: (node->data.has_animated_opacity &&
!property_trees()->is_active) ||
Is this needed? It changes behavior in the computation of the RSLL (and might be
what's causing crashes in browser tests).

[jaydasika, 2016-04-13 20:21:24]

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:653: needs_update_again_after_activation);
On 2016/04/13 19:53:06, ajuma wrote:
> This would also make us update every frame on the main thread, I think? How
> about adding this logic to LayerTreeImpl::SetPropertyTrees (and
unconditionally
> setting needs_update to true for the EffectTree if the LayerTreeImpl is the
> active tree)?

This function is called only on compositor thread.

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.cc:1335: (node->data.has_animated_opacity &&
!property_trees()->is_active) ||
On 2016/04/13 19:53:06, ajuma wrote:
> Is this needed? It changes behavior in the computation of the RSLL (and might
be
> what's causing crashes in browser tests).

This should be the cause for browser test crashes. But, I think we need this,
for the case A->B, A is hidden, B's opacity = 0, has_copy_request and
has_animating opacity, B's is drawn will be true but contributes to drawn
surface should be false on active tree (because its drawn only for copy
request). Having said that, I realized contributes to drawn_surface can have a
much simpler definition : node->is_drawn && parent->is_drawn and that will fix
the crashes also. I will still investigate the crashes as I want to know why its
happening, but that need not block this CL.

[ajuma, 2016-04-13 20:34:19]

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:653: needs_update_again_after_activation);
On 2016/04/13 20:21:24, jaydasika wrote:
> On 2016/04/13 19:53:06, ajuma wrote:
> > This would also make us update every frame on the main thread, I think? How
> > about adding this logic to LayerTreeImpl::SetPropertyTrees (and
> unconditionally
> > setting needs_update to true for the EffectTree if the LayerTreeImpl is the
> > active tree)?
> 
> This function is called only on compositor thread.

Ah, ok. But then what about when trees are pushed directly from the main thread
to the active tree? What triggers an update there?

[jaydasika, 2016-04-13 20:53:08]

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
File cc/trees/draw_property_utils.cc (right):

https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
cc/trees/draw_property_utils.cc:653: needs_update_again_after_activation);
On 2016/04/13 20:34:18, ajuma wrote:
> On 2016/04/13 20:21:24, jaydasika wrote:
> > On 2016/04/13 19:53:06, ajuma wrote:
> > > This would also make us update every frame on the main thread, I think?
How
> > > about adding this logic to LayerTreeImpl::SetPropertyTrees (and
> > unconditionally
> > > setting needs_update to true for the EffectTree if the LayerTreeImpl is
the
> > > active tree)?
> > 
> > This function is called only on compositor thread.
> 
> Ah, ok. But then what about when trees are pushed directly from the main
thread
> to the active tree? What triggers an update there?

Ah! I forgot that case. I think its better to do what you suggested(set needs
update in SetPropertyTrees) rather than replicating this logic in one more
place.

[jaydasika, 2016-04-13 20:55:14]

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

[commit-bot: I haz the power, 2016-04-13 20:55:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884613005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884613005/100001

[ajuma, 2016-04-13 21:01:50]

On 2016/04/13 20:53:08, jaydasika wrote:
>
https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
> File cc/trees/draw_property_utils.cc (right):
> 
>
https://codereview.chromium.org/1884613005/diff/60001/cc/trees/draw_property_...
> cc/trees/draw_property_utils.cc:653: needs_update_again_after_activation);
> On 2016/04/13 20:34:18, ajuma wrote:
> > On 2016/04/13 20:21:24, jaydasika wrote:
> > > On 2016/04/13 19:53:06, ajuma wrote:
> > > > This would also make us update every frame on the main thread, I think?
> How
> > > > about adding this logic to LayerTreeImpl::SetPropertyTrees (and
> > > unconditionally
> > > > setting needs_update to true for the EffectTree if the LayerTreeImpl is
> the
> > > > active tree)?
> > > 
> > > This function is called only on compositor thread.
> > 
> > Ah, ok. But then what about when trees are pushed directly from the main
> thread
> > to the active tree? What triggers an update there?
> 
> Ah! I forgot that case. I think its better to do what you suggested(set needs
> update in SetPropertyTrees) rather than replicating this logic in one more
> place.

Thanks! Please add a test that verifies that we compute is_drawn when the value
changes only because property trees got pushed.

[jaydasika, 2016-04-13 22:27:37]

Added test

[jaydasika, 2016-04-13 22:27:48]

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

[commit-bot: I haz the power, 2016-04-13 22:28:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884613005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884613005/140001

[commit-bot: I haz the power, 2016-04-13 23:39:06]

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

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

Dry run: This issue passed the CQ dry run.

[jaydasika, 2016-04-14 01:28:10]

Had an offline discussion with Ali and the summary is :
skipping arbitrary parts of the layer tree is fine, but in the (implicit) render
surface tree, we can only skip subtrees. 
This implies when we have copy requests, we cannot skip the copy request
surface's target. Otherwise, there will be discrepancies in terms of which
render surface's layer list the copy request will appear in and to which render
surface's content rect it will contribute to.

[ajuma, 2016-04-14 14:13:03]

Thanks, lgtm!

[jaydasika, 2016-04-14 14:46:11]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-04-14 14:46:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884613005/140002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884613005/140002

[commit-bot: I haz the power, 2016-04-14 15:20:21]

Committed patchset #9 (id:140002)

[commit-bot: I haz the power, 2016-04-14 15:22:07]

Description was changed from

==========
cc : Simplify layer skipping logic

Since we no longer skip subtrees, we can remove the logic that supports
subtree skipping.

BUG=601088
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Simplify layer skipping logic

Since we no longer skip subtrees, we can remove the logic that supports
subtree skipping.

BUG=601088
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8185d3075363d17df8cd2348ffbc27af8740f3ca
Cr-Commit-Position: refs/heads/master@{#387320}
==========

[commit-bot: I haz the power, 2016-04-14 15:22:08]

Patchset 9 (id:??) landed as
https://crrev.com/8185d3075363d17df8cd2348ffbc27af8740f3ca
Cr-Commit-Position: refs/heads/master@{#387320}