Track transform animation content readiness on TransformNode.

Track transform animation content readiness on TransformNode.

Removes one LayerByElementId call site in LayerTreeHostImpl.

Logic was originally added to track when a layer with a
transform animation has no missing tiles in
http://crrev.com/1461243003 for http://crbug.com/554924.

This patch is a step toward removing animation system
dependency on layers. Animation subsystem logic should
ideally involve only element ids and their involved
property tree nodes.

BUG=702777, 709137
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[wkorman, 2017-05-20 01:19:01]

Description was changed from

==========
Move transform animation content readiness from layer to node.

BUG=702777,709137
==========

to

==========
Move transform animation content readiness from layer to node.

BUG=702777,709137
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wkorman, 2017-05-20 01:26:04]

Description was changed from

==========
Move transform animation content readiness from layer to node.

BUG=702777,709137
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Track transform animation content readiness on TransformNode.

Removes one LayerByElementId call site in LayerTreeHostImpl.

Logic was originally added to track when a layer with a
transform animation has no missing tiles in
http://crrev.com/1461243003 for http://crbug.com/554924.

This patch is a step toward removing animation system
dependency on layers. Animation subsystem logic should
ideally involve only element ids and their involved
property tree nodes.

BUG=702777,709137
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[wkorman, 2017-05-20 01:31:27]

wkorman@chromium.org changed reviewers:
+ chrishtr@chromium.org, jaydasika@chromium.org, pdr@chromium.org

[wkorman, 2017-05-20 01:31:28]

Hi Jayadev, I happened to hit this TODO again while working on something else
and figured I would take a stab at it. As I'm still new to this area of code I
have a few naive questions noted where applicable.

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:948: layer->transform_tree_index());
Are we assured that we will always have a transform node, or do we need to check
for TransformTree::kInvalidNodeId?

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (left):

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:619:
layer->set_was_ever_ready_since_last_transform_animation(false);
This is now enacted via TransformTree::OnTransformAnimated elsewhere in this
patch.

https://codereview.chromium.org/2895793002/diff/1/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2895793002/diff/1/cc/trees/property_tree.cc#n...
cc/trees/property_tree.cc:196: node->ready_since_animation = false;
Is it ok that we do this on both main and impl threads? I think it will be
ignored on main thread since only LayerTreeHostImpl actually reads the value.

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

https://codereview.chromium.org/2895793002/diff/1/cc/trees/transform_node.h#n...
cc/trees/transform_node.h:128: bool ready_since_animation : 1;
Is there a reason we can't/shouldn't do this on the transform node rather than
the layer?

[wkorman, 2017-05-20 01:31:56]

Description was changed from

==========
Track transform animation content readiness on TransformNode.

Removes one LayerByElementId call site in LayerTreeHostImpl.

Logic was originally added to track when a layer with a
transform animation has no missing tiles in
http://crrev.com/1461243003 for http://crbug.com/554924.

This patch is a step toward removing animation system
dependency on layers. Animation subsystem logic should
ideally involve only element ids and their involved
property tree nodes.

BUG=702777,709137
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Track transform animation content readiness on TransformNode.

Removes one LayerByElementId call site in LayerTreeHostImpl.

Logic was originally added to track when a layer with a
transform animation has no missing tiles in
http://crrev.com/1461243003 for http://crbug.com/554924.

This patch is a step toward removing animation system
dependency on layers. Animation subsystem logic should
ideally involve only element ids and their involved
property tree nodes.

BUG=702777,709137
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2017-05-20 01:32:26]

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

[commit-bot: I haz the power, 2017-05-20 01:33:11]

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-05-20 04:26:57]

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

[commit-bot: I haz the power, 2017-05-20 04:26:58]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-05-22 17:03:34]

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:948: layer->transform_tree_index());
On 2017/05/20 at 01:31:27, wkorman wrote:
> Are we assured that we will always have a transform node, or do we need to
check for TransformTree::kInvalidNodeId?

We've had a bunch of crashes in this area recently (crbug.com/712298,
crbug.com/708951, crbug.com/721772). That said, I think we should not guard
against this in new code, especially now that we better understand this area.
LGTM for leaving this as-is without checking for a transform node.

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

https://codereview.chromium.org/2895793002/diff/1/cc/trees/transform_node.h#n...
cc/trees/transform_node.h:128: bool ready_since_animation : 1;
On 2017/05/20 at 01:31:28, wkorman wrote:
> Is there a reason we can't/shouldn't do this on the transform node rather than
the layer?

Tiles sort of feel like a Layer concept more than a property tree concept. We
will still have both Layers and property nodes in SPV2; could this be put on
layer instead while still getting the benefits of this patch?

[wkorman, 2017-05-22 17:18:25]

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

https://codereview.chromium.org/2895793002/diff/1/cc/trees/transform_node.h#n...
cc/trees/transform_node.h:128: bool ready_since_animation : 1;
On 2017/05/22 17:03:34, pdr. wrote:
> On 2017/05/20 at 01:31:28, wkorman wrote:
> > Is there a reason we can't/shouldn't do this on the transform node rather
than
> the layer?
> 
> Tiles sort of feel like a Layer concept more than a property tree concept. We
> will still have both Layers and property nodes in SPV2; could this be put on
> layer instead while still getting the benefits of this patch?

Well, it's already on Layer (well, LayerImpl) today. Really this patch is just
moving it to TransformNode.

In order to remove references to element_id on Layer instances we have to either
move this to TransformNode or explore some alternate rework of the code path
that ends up calling LayerTreeHostImpl::ElementIsAnimatingChanged where we use
LayerByElementId today. Am definitely open to alternate approaches.

The particular thing being tracked is related to transform animations (and
specifically translates). In general I think, given that, it can make sense to
track on transform node directly. There are some animation specific methods on
LayerImpl, and this one field at ToT, but I would not be surprised if we end up
removing all of these as part of http://crbug.com/709137.

[wkorman, 2017-05-22 17:33:39]

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

[wkorman, 2017-05-22 17:33:40]

+enne with our other OWNERS reviewer OOO.

[pdr., 2017-05-22 17:33:53]

pdr@chromium.org changed reviewers:
- chrishtr@chromium.org

[pdr., 2017-05-22 17:34:27]

On 2017/05/22 at 17:33:40, wkorman wrote:
> +enne with our other OWNERS reviewer OOO.

LGTM here.

@enne, could you please do a full review too?

[jaydasika, 2017-05-22 18:11:29]

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2895793002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:954: transform_node->ready_since_animation =
true;
As layer->transform node is not 1:1, you can't do this here. The value of
ready_since_animation with this logic would depend on the num_missing_tiles
value of the last layer referencing that transform node (this function iterates
through all drawn layers). So, we need to have this on layers.

Since the aim here is to remove layer references from
LayerTreeImpl::SetTransformMutated, we could have a field for this on both
transform node and layer. So, you would update the field on transform node in
LayerTreeImpl::SetTransformMutated and the field on layer here and the actual
value of ready_since_animation would be inferred from both (this would be
similar to how we do damage tracking where we have both
LayerImpl::layer_property_changed and TransformNode::transform_changed).

[enne (OOO), 2017-05-22 18:52:00]

+vmpstr who originally added this tracking, in case we happen to not need it
anymore or there's some alternate way to implement it.

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

https://codereview.chromium.org/2895793002/diff/1/cc/trees/transform_node.h#n...
cc/trees/transform_node.h:128: bool ready_since_animation : 1;
On 2017/05/20 at 01:31:28, wkorman wrote:
> Is there a reason we can't/shouldn't do this on the transform node rather than
the layer?

Sorry, but I agree strongly with pdr here.  Tiles and quads are a Layer concept.
 Transform nodes should know nothing about them.  Many layers can share the same
transform node.  It looks like one layer being ready here would clobber the
value for all the other layers that share the same node, which seems incorrect.

[wkorman, 2017-05-22 18:55:52]

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

https://codereview.chromium.org/2895793002/diff/1/cc/trees/transform_node.h#n...
cc/trees/transform_node.h:128: bool ready_since_animation : 1;
On 2017/05/22 18:51:59, enne wrote:
> On 2017/05/20 at 01:31:28, wkorman wrote:
> > Is there a reason we can't/shouldn't do this on the transform node rather
than
> the layer?
> 
> Sorry, but I agree strongly with pdr here.  Tiles and quads are a Layer
concept.
>  Transform nodes should know nothing about them.  Many layers can share the
same
> transform node.  It looks like one layer being ready here would clobber the
> value for all the other layers that share the same node, which seems
incorrect.

That's cool, I am happy to rework it. How do you feel about jaydasika@ comment
on this patch re: proposed rework which would still have a field on the node but
also have one on layer? That sounds workable but would still, for some
definition thereof, leak the concept of tile/quad content into the node. Because
otherwise why would we have that field on the node.

[enne (OOO), 2017-05-22 18:58:53]

Just so that I don't say no without giving other suggestions here.  I realize
that we don't really want a mapping of transform node -> layers affected by it. 
We also don't want to iterate through all the layers unnecessarily.

Brainstorming here, but I think if we need to keep this optimization, then one
option could have transform node have a "last compositor frame number updated
on" value, and then individual layers could check that during AppendQuads
(maybe?) and then set or unset their "ever ready since last update" flag
internally on a layer by layer basis.  That way transform node just knows about
transform updates only.

[wkorman, 2017-05-22 19:11:26]

On 2017/05/22 18:58:53, enne wrote:
> Just so that I don't say no without giving other suggestions here.  I realize
> that we don't really want a mapping of transform node -> layers affected by
it. 
> We also don't want to iterate through all the layers unnecessarily.
> 
> Brainstorming here, but I think if we need to keep this optimization, then one
> option could have transform node have a "last compositor frame number updated
> on" value, and then individual layers could check that during AppendQuads
> (maybe?) and then set or unset their "ever ready since last update" flag
> internally on a layer by layer basis.  That way transform node just knows
about
> transform updates only.

OK, thanks. We did consider just removing the optimization. It really seems like
what we have is sort of a workaround fix. We have a unit test, but no failing
layout test. Jayadev had said that he was unable to reproduce the issue on the
original site linked in the bug. But they could have changed their code, or
we've changed ours in some way that fixes that case, while underlying issue
could remain.

I actually did read vmpstr@ change and the discussion on the bug in depth. I
thought first step to just port this, but realized I may have over-simplified
(as you and Jayadev have confirmed) re: transform/layer interaction.

But given more work needed, perhaps just removing and seeing if issue persists.

I will see if we can create a web page test case that fails, with was-ever-ready
logic removed, based on knowledge from prior issue. That seems like it could
usefully inform next step.

[wkorman, 2017-05-23 18:20:26]

On 2017/05/22 19:11:26, wkorman wrote:
> On 2017/05/22 18:58:53, enne wrote:
> > Just so that I don't say no without giving other suggestions here.  I
realize
> > that we don't really want a mapping of transform node -> layers affected by
> it. 
> > We also don't want to iterate through all the layers unnecessarily.
> > 
> > Brainstorming here, but I think if we need to keep this optimization, then
one
> > option could have transform node have a "last compositor frame number
updated
> > on" value, and then individual layers could check that during AppendQuads
> > (maybe?) and then set or unset their "ever ready since last update" flag
> > internally on a layer by layer basis.  That way transform node just knows
> about
> > transform updates only.
> 
> OK, thanks. We did consider just removing the optimization. It really seems
like
> what we have is sort of a workaround fix. We have a unit test, but no failing
> layout test. Jayadev had said that he was unable to reproduce the issue on the
> original site linked in the bug. But they could have changed their code, or
> we've changed ours in some way that fixes that case, while underlying issue
> could remain.
> 
> I actually did read vmpstr@ change and the discussion on the bug in depth. I
> thought first step to just port this, but realized I may have over-simplified
> (as you and Jayadev have confirmed) re: transform/layer interaction.
> 
> But given more work needed, perhaps just removing and seeing if issue
persists.
> 
> I will see if we can create a web page test case that fails, with
was-ever-ready
> logic removed, based on knowledge from prior issue. That seems like it could
> usefully inform next step.

I can't repro locally or on their site. Many things have surely changed since
the original patch http://crrev.com/1461243003 landed. Two that come to mind
are (1) image decode changes that perhaps affect when/whether the image content
being animated is ready, and (2) synchronized paint and interest rect changes
(see for example http://crrev.com/1393083003.

Logically from the bug description in http://crbug.com/554924 it sounds like
the issue could still exist, but without a known failing real world test case
I am not sure maintaining the code complexity is worthwhile. From earlier
discussion in this bug it does not sound like it would be too hard to restore
in a modern agreed-upon manner if we find it's needed during next beta.

I will put up a patch to remove the logic and solicit feedback.