CC Animations: Fix draw properties invalidation on LAC destruction

CC Animations: Fix draw properties invalidation on LAC destruction

We missed a call to
layer_tree_impl()->set_needs_update_draw_properties();
in
LayerImpl::UpdatePropertyTreeTransformIsAnimated
so
the active tree draw properties (in particular, screen_space_transform_is_animating) were not recalculated in LayerTreeHostImpl::PrepareToDraw.

BUG=547186
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/62daf92eb731d83e64696e6fcff21281d5d3ba59
Cr-Commit-Position: refs/heads/master@{#360748}

[loyso (OOO), 2015-10-27 05:23:51]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org

[loyso (OOO), 2015-10-27 06:28:00]

Description was changed from

==========
CC Animations: Fix flaky unittest on Android

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

to

==========
CC Animations: Fix flaky unittest on Android

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

[loyso (OOO), 2015-10-27 06:28:00]

loyso@chromium.org changed reviewers:
+ enne@chromium.org
- ajuma@chromium.org

[Ian Vollick, 2015-10-27 14:08:18]

https://codereview.chromium.org/1409393005/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (left):

https://codereview.chromium.org/1409393005/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:1024: if
(host_impl->active_tree()->source_frame_number() == 2) {
I'm sorry, it should probably be obvious to me, but I don't see how, if the
current code consistently passes, the old code was flaky and not just wrong all
the time.

Wouldn't the active tree have source_frame_number == 2 after activation, and
wouldn't that also imply that that the active tree's root layer would not be
animating? Could you please add an explanation either in the description of the
patch, or here?

[loyso (OOO), 2015-10-27 22:43:38]

Description was changed from

==========
CC Animations: Fix flaky unittest on Android

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

to

==========
CC Animations: Fix flaky unittest on Android

It worked only by accident - no were changes between two CommitCompleteOnThread
calls.

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

[loyso (OOO), 2015-10-27 22:43:46]

https://codereview.chromium.org/1409393005/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (left):

https://codereview.chromium.org/1409393005/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:1024: if
(host_impl->active_tree()->source_frame_number() == 2) {
On 2015/10/27 14:08:18, vollick wrote:
> I'm sorry, it should probably be obvious to me, but I don't see how, if the
> current code consistently passes, the old code was flaky and not just wrong
all
> the time.
Yes, your observation is correct. It worked only by accident - no were changes
between two CommitCompleteOnThread calls.
 
> Wouldn't the active tree have source_frame_number == 2 after activation, and
> wouldn't that also imply that that the active tree's root layer would not be
> animating? Could you please add an explanation either in the description of
the
> patch, or here?
AFAIU, at CommitCompleteOnThread time LTH tree copied into LTHI pending tree
with draw properties calculated, but pending tree isn't activated and drawn.

[Ian Vollick, 2015-10-28 22:54:42]

https://codereview.chromium.org/1409393005/diff/1/cc/trees/layer_tree_host_un...
File cc/trees/layer_tree_host_unittest_animation_timelines.cc (left):

https://codereview.chromium.org/1409393005/diff/1/cc/trees/layer_tree_host_un...
cc/trees/layer_tree_host_unittest_animation_timelines.cc:1024: if
(host_impl->active_tree()->source_frame_number() == 2) {
On 2015/10/27 22:43:46, loyso wrote:
> On 2015/10/27 14:08:18, vollick wrote:
> > I'm sorry, it should probably be obvious to me, but I don't see how, if the
> > current code consistently passes, the old code was flaky and not just wrong
> all
> > the time.
> Yes, your observation is correct. It worked only by accident - no were changes
> between two CommitCompleteOnThread calls.
>  
> > Wouldn't the active tree have source_frame_number == 2 after activation, and
> > wouldn't that also imply that that the active tree's root layer would not be
> > animating? Could you please add an explanation either in the description of
> the
> > patch, or here?
> AFAIU, at CommitCompleteOnThread time LTH tree copied into LTHI pending tree
> with draw properties calculated, but pending tree isn't activated and drawn.

Still a little confused (sorry). Could you add a link to a failure?

[loyso (OOO), 2015-10-28 22:56:17]

On 2015/10/28 22:54:42, vollick wrote:
> Still a little confused (sorry). Could you add a link to a failure?

https://code.google.com/p/chromium/issues/detail?id=547186
https://code.google.com/p/chromium/issues/detail?id=546743

[danakj, 2015-10-29 20:34:25]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-10-29 20:34:25]

My guess is that the scheduler's animate step is racing with the commit
complete, so sometimes it animates and stops the animation in frame 2 on the
active tree, and sometimes it doesn't before the test checks run.

It should probably not be in CommitCompleteOnThread, maybe in WillActivate().
But the fix here also looks right then.

Maybe you can confirm if that's what is going on before the change.

[loyso (OOO), 2015-10-29 22:57:51]

On 2015/10/29 20:34:25, danakj wrote:
> My guess is that the scheduler's animate step is racing with the commit
> complete, so sometimes it animates and stops the animation in frame 2 on the
> active tree, and sometimes it doesn't before the test checks run.
> 
> It should probably not be in CommitCompleteOnThread, maybe in WillActivate().
> But the fix here also looks right then.
> 
> Maybe you can confirm if that's what is going on before the change.

AFAIR, draw properties are not yet calculated on impl thread in WillActivate.
The version before fix was stupid, anyway. I had a nice repro on my Nexus 7.

[ajuma, 2015-11-02 20:39:18]

ajuma@chromium.org changed reviewers:
+ ajuma@chromium.org

[ajuma, 2015-11-02 20:49:22]

On 2015/10/29 22:57:51, loyso wrote:
> On 2015/10/29 20:34:25, danakj wrote:
> > My guess is that the scheduler's animate step is racing with the commit
> > complete, so sometimes it animates and stops the animation in frame 2 on the
> > active tree, and sometimes it doesn't before the test checks run.
> > 
> > It should probably not be in CommitCompleteOnThread, maybe in
WillActivate().
> > But the fix here also looks right then.
> > 
> > Maybe you can confirm if that's what is going on before the change.

It looks like before the change, we were mostly not reaching the checks inside
CommitCompleteOnThread (which happened when committing frame 3) since EndTest
had already been called when we drew frame 2? loyso, can you verify if that's
the case (just check if that code was getting reached when the test was passing
before).

In any case, the new version seems correct, so lgtm. But please clarify the
description (in particular, the "no were changes between two
CommitCompleteOnThread calls" part seems to be missing words or something).

[loyso (OOO), 2015-11-03 06:53:24]

On 2015/11/02 20:49:22, ajuma wrote:
> It looks like before the change, we were mostly not reaching the checks inside
> CommitCompleteOnThread (which happened when committing frame 3) since EndTest
> had already been called when we drew frame 2? loyso, can you verify if that's
> the case (just check if that code was getting reached when the test was
passing
> before).
AFAIR, we were always reaching those checks. I'll take a look tomorrow to
understand, why it worked.

[loyso (OOO), 2015-11-19 06:14:37]

In LayerTreeHostImpl::CommitComplete we do:
sync_tree()->UpdateDrawProperties(update_lcd_text);

  // Returns the tree LTH synchronizes with.
  LayerTreeImpl* sync_tree() {
    // TODO(enne): This is bogus.  It should return based on the value of
    // CommitToActiveTree() and not whether the pending tree exists.
    return pending_tree_ ? pending_tree_.get() : active_tree_.get();
  }


I.e. we calculate screen_space_transform_is_animating() always for the pending
tree.
We don't recalculate it for active tree.

So, the invariant
      EXPECT_TRUE(host_impl->active_tree()
                      ->root_layer()
                      ->screen_space_transform_is_animating());
      EXPECT_FALSE(host_impl->pending_tree()
                       ->root_layer()
                       ->screen_space_transform_is_animating());
works for both the frames 1 and 2 (in most cases).

Let's land this CL! PTAL!

[loyso (OOO), 2015-11-19 06:28:44]

p.s. 

1) We call active_tree_->UpdateDrawProperties from
LayerTreeHostImpl::PrepareToDraw.
2) The LayerTreeHostImpl::PrepareToDraw call happens between
CommitCompleteOnThread for the frame 1 and CommitCompleteOnThread for the frame
2.
3) But after the frame 1 LayerTreeHostImpl::PrepareToDraw doesn't call
CalculateDrawPropertiesInternal, needs_update_draw_properties_ == false.
  if (!needs_update_draw_properties_)
    return true;

[loyso (OOO), 2015-11-19 06:35:50]

Currently:

void LayerTreeHost::SetMutatorsNeedCommit() {
  SetNeedsCommit();
}

void LayerTreeHost::SetMutatorsNeedRebuildPropertyTrees() {
  property_trees_.needs_rebuild = true;
}

and

void LayerTreeHostImpl::SetMutatorsNeedCommit() {
  SetNeedsCommit();
}

void LayerTreeHostImpl::SetMutatorsNeedRebuildPropertyTrees() {}

I guess, I need to invalidate active tree ^^^ here somehow. Any suggestions?

[ajuma, 2015-11-19 14:17:59]

On 2015/11/19 06:14:37, loyso wrote:
> In LayerTreeHostImpl::CommitComplete we do:
> sync_tree()->UpdateDrawProperties(update_lcd_text);
> 
>   // Returns the tree LTH synchronizes with.
>   LayerTreeImpl* sync_tree() {
>     // TODO(enne): This is bogus.  It should return based on the value of
>     // CommitToActiveTree() and not whether the pending tree exists.
>     return pending_tree_ ? pending_tree_.get() : active_tree_.get();
>   }
> 
> 
> I.e. we calculate screen_space_transform_is_animating() always for the pending
> tree.
> We don't recalculate it for active tree.

At activation time, the pending tree's property trees will be sent to the active
tree. At draw time, we'll compute draw properties for the active tree's layers.
So at that point, the layers on the active tree should have an up-to-date
version of screen_space_transform_is_animating.

> 
> So, the invariant
>       EXPECT_TRUE(host_impl->active_tree()
>                       ->root_layer()
>                       ->screen_space_transform_is_animating());
>       EXPECT_FALSE(host_impl->pending_tree()
>                        ->root_layer()
>                        ->screen_space_transform_is_animating());
> works for both the frames 1 and 2 (in most cases).

If that's true, there's a bug somewhere. If the pending tree isn't animating on
frame 2 (which is the commit that happens when the active tree is drawing frame
1), the active tree shouldn't be animating when it draws frame 2.

[ajuma, 2015-11-19 14:32:38]

On 2015/11/19 06:35:50, loyso wrote:
> Currently:
> 
> void LayerTreeHost::SetMutatorsNeedCommit() {
>   SetNeedsCommit();
> }
> 
> void LayerTreeHost::SetMutatorsNeedRebuildPropertyTrees() {
>   property_trees_.needs_rebuild = true;
> }
> 
> and
> 
> void LayerTreeHostImpl::SetMutatorsNeedCommit() {
>   SetNeedsCommit();
> }
> 
> void LayerTreeHostImpl::SetMutatorsNeedRebuildPropertyTrees() {}
> 
> I guess, I need to invalidate active tree ^^^ here somehow. Any suggestions?

The way this is meant to work is that removing/adding a transform animation
triggers a call to
LayerAnimationController::UpdatePotentiallyAnimatingTransform, which eventually
leads to a call to LayerImpl::OnTransformIsPotentiallyAnimatingChanged, which
calls LayerImpl::UpdatePropertyTreeTransformIsAnimated, which updates the
transform tree. Hmm, but I don't see a call to set_needs_update_draw_properties.
I'd suggest adding a call to
layer_tree_impl()->set_needs_update_draw_properties() right after the call to
transform_tree.set_needs_update(true) in
LayerImpl::UpdatePropertyTreeTransformIsAnimated.

[loyso (OOO), 2015-11-19 23:18:15]

On 2015/11/19 14:32:38, ajuma wrote:
> I'd suggest adding a call to
> layer_tree_impl()->set_needs_update_draw_properties() right after the call to
> transform_tree.set_needs_update(true) in
> LayerImpl::UpdatePropertyTreeTransformIsAnimated.
Yup, that's the solution. Now the test fails on frame 2 (the old version with
typo) and works only for the frame 1.
Cool!

[loyso (OOO), 2015-11-19 23:27:14]

Description was changed from

==========
CC Animations: Fix flaky unittest on Android

It worked only by accident - no were changes between two CommitCompleteOnThread
calls.

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

to

==========
CC Animations: Fix draw properties invalidation on LAC destruction

We missed a call to 
layer_tree_impl()->set_needs_update_draw_properties();
in
LayerImpl::UpdatePropertyTreeTransformIsAnimated
so
an active tree draw properties were not recalculated in PrepareToDraw.


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

[loyso (OOO), 2015-11-19 23:28:56]

Description was changed from

==========
CC Animations: Fix draw properties invalidation on LAC destruction

We missed a call to 
layer_tree_impl()->set_needs_update_draw_properties();
in
LayerImpl::UpdatePropertyTreeTransformIsAnimated
so
an active tree draw properties were not recalculated in PrepareToDraw.


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

to

==========
CC Animations: Fix draw properties invalidation on LAC destruction

We missed a call to 
layer_tree_impl()->set_needs_update_draw_properties();
in
LayerImpl::UpdatePropertyTreeTransformIsAnimated
so
the active tree draw properties (in particular,
screen_space_transform_is_animating) were not recalculated in
LayerTreeHostImpl::PrepareToDraw.

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

[ajuma, 2015-11-20 00:08:54]

On 2015/11/19 23:18:15, loyso wrote:
> On 2015/11/19 14:32:38, ajuma wrote:
> > I'd suggest adding a call to
> > layer_tree_impl()->set_needs_update_draw_properties() right after the call
to
> > transform_tree.set_needs_update(true) in
> > LayerImpl::UpdatePropertyTreeTransformIsAnimated.
> Yup, that's the solution. Now the test fails on frame 2 (the old version with
> typo) and works only for the frame 1.
> Cool!

lgtm

[loyso (OOO), 2015-11-20 04:22:23]

The CQ bit was checked by loyso@chromium.org

[loyso (OOO), 2015-11-20 04:22:23]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1409393005/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-20 04:23:14]

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

[commit-bot: I haz the power, 2015-11-20 04:47:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-20 04:48:34]

Patchset 4 (id:??) landed as
https://crrev.com/62daf92eb731d83e64696e6fcff21281d5d3ba59
Cr-Commit-Position: refs/heads/master@{#360748}