From e7425277e33dd626227dc7aaf93ee6ddbed48f68 Mon Sep 17 00:00:00 2001 From: ajuma Date: Mon, 13 Jul 2015 11:37:34 -0700 Subject: [PATCH] cc: Fix post-commit property tree update for animations After a commit, property tree nodes corresponding to animated layers have their values updated during a layer tree walk, to handle the case where the most recent compositor thread animation tick happened at a different time than the most recent main thread tick. This CL fixes two problems with this update: 1) An animation that's running at the start of a BeginMainFrame may finish on the compositor thread before the commit. In this situation, the property tree still needs to updated after commit. The current logic used by LayerImpl to decide whether an update is needed involves calling TransformIsAnimating or OpacityIsAnimating, but these will be false since the animation has finished. As a result, the update doesn't happen and the property tree is out-of-sync. 2) If an in-progress animation is removed on the main thread, the previously-animated layer may no longer own a property tree node. However, right after commit, the animation is still in a "Running" state on the compositor thread (so that the active tree continues to be affected until tree activation). As a result, LayerImpl will update a tree node that it doesn't actually own, so this node will end up with the wrong value. To fix these problems, LayerImpl now checks for any animation rather than only those that aren't finished (to handle the possibility that the animation might have finished during the commit), and also now checks that it owns a property tree node before updating this node (to handle the possibility that it no longer owns a node). All layout tests now pass with property tree verification enabled. BUG=509679 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review URL: https://codereview.chromium.org/1231903005 Cr-Commit-Position: refs/heads/master@{#338537} --- cc/animation/animation_host.cc | 10 ++ cc/animation/animation_host.h | 4 + cc/layers/layer_impl.cc | 27 ++++- cc/layers/layer_impl.h | 5 + cc/trees/layer_tree_host_unittest_animation.cc | 119 +++++++++++++++++++ ...layer_tree_host_unittest_animation_timelines.cc | 129 +++++++++++++++++++++ cc/trees/layer_tree_host_unittest_picture.cc | 4 + cc/trees/layer_tree_impl.cc | 9 ++ cc/trees/layer_tree_impl.h | 4 + 9 files changed, 309 insertions(+), 2 deletions(-) diff --git a/cc/animation/animation_host.cc b/cc/animation/animation_host.cc index e7f289fad9a0..8bab8abad447 100644 --- a/cc/animation/animation_host.cc +++ b/cc/animation/animation_host.cc @@ -394,6 +394,16 @@ bool AnimationHost::HasPotentiallyRunningTransformAnimation( return animation && !animation->is_finished(); } +bool AnimationHost::HasAnyAnimationTargetingProperty( + int layer_id, + Animation::TargetProperty property) const { + LayerAnimationController* controller = GetControllerForLayerId(layer_id); + if (!controller) + return false; + + return !!controller->GetAnimation(property); +} + bool AnimationHost::FilterIsAnimatingOnImplOnly(int layer_id) const { LayerAnimationController* controller = GetControllerForLayerId(layer_id); if (!controller) diff --git a/cc/animation/animation_host.h b/cc/animation/animation_host.h index 3f69a0233791..0cb40a6eeee5 100644 --- a/cc/animation/animation_host.h +++ b/cc/animation/animation_host.h @@ -100,6 +100,10 @@ class CC_EXPORT AnimationHost { bool HasPotentiallyRunningOpacityAnimation(int layer_id) const; bool HasPotentiallyRunningTransformAnimation(int layer_id) const; + bool HasAnyAnimationTargetingProperty( + int layer_id, + Animation::TargetProperty property) const; + bool FilterIsAnimatingOnImplOnly(int layer_id) const; bool OpacityIsAnimatingOnImplOnly(int layer_id) const; bool TransformIsAnimatingOnImplOnly(int layer_id) const; diff --git a/cc/layers/layer_impl.cc b/cc/layers/layer_impl.cc index 5a943b07f42a..0a987334752c 100644 --- a/cc/layers/layer_impl.cc +++ b/cc/layers/layer_impl.cc @@ -780,6 +780,14 @@ void LayerImpl::UpdatePropertyTreeTransform() { TransformTree& transform_tree = layer_tree_impl()->property_trees()->transform_tree; TransformNode* node = transform_tree.Node(transform_tree_index_); + // A LayerImpl's own current state is insufficient for determining whether + // it owns a TransformNode, since this depends on the state of the + // corresponding Layer at the time of the last commit. For example, a + // transform animation might have been in progress at the time the last + // commit started, but might have finished since then on the compositor + // thread. + if (node->owner_id != id()) + return; if (node->data.local != transform_) { node->data.local = transform_; node->data.needs_local_transform_update = true; @@ -799,6 +807,13 @@ void LayerImpl::UpdatePropertyTreeOpacity() { OpacityTree& opacity_tree = layer_tree_impl()->property_trees()->opacity_tree; OpacityNode* node = opacity_tree.Node(opacity_tree_index_); + // A LayerImpl's own current state is insufficient for determining whether + // it owns an OpacityNode, since this depends on the state of the + // corresponding Layer at the time of the last commit. For example, an + // opacity animation might have been in progress at the time the last commit + // started, but might have finished since then on the compositor thread. + if (node->owner_id != id()) + return; node->data.opacity = opacity_; opacity_tree.set_needs_update(true); } @@ -808,10 +823,10 @@ void LayerImpl::UpdatePropertyTreeForScrollingAndAnimationIfNeeded() { if (scrollable()) UpdatePropertyTreeScrollOffset(); - if (OpacityIsAnimating()) + if (HasAnyAnimationTargetingProperty(Animation::OPACITY)) UpdatePropertyTreeOpacity(); - if (TransformIsAnimating()) + if (HasAnyAnimationTargetingProperty(Animation::TRANSFORM)) UpdatePropertyTreeTransform(); } @@ -1212,6 +1227,14 @@ bool LayerImpl::AnimationStartScale(float* start_scale) const { return layer_animation_controller_->AnimationStartScale(start_scale); } +bool LayerImpl::HasAnyAnimationTargetingProperty( + Animation::TargetProperty property) const { + if (!layer_animation_controller_) + return layer_tree_impl_->HasAnyAnimationTargetingProperty(this, property); + + return !!layer_animation_controller_->GetAnimation(property); +} + bool LayerImpl::HasFilterAnimationThatInflatesBounds() const { if (!layer_animation_controller_) return layer_tree_impl_->HasFilterAnimationThatInflatesBounds(this); diff --git a/cc/layers/layer_impl.h b/cc/layers/layer_impl.h index 2f424c3192a7..abad79f1b5a4 100644 --- a/cc/layers/layer_impl.h +++ b/cc/layers/layer_impl.h @@ -551,6 +551,11 @@ class CC_EXPORT LayerImpl : public LayerAnimationValueObserver, bool MaximumTargetScale(float* max_scale) const; bool AnimationStartScale(float* start_scale) const; + // This includes all animations, even those that are finished but haven't yet + // been deleted. + bool HasAnyAnimationTargetingProperty( + Animation::TargetProperty property) const; + bool HasFilterAnimationThatInflatesBounds() const; bool HasTransformAnimationThatInflatesBounds() const; bool HasAnimationThatInflatesBounds() const; diff --git a/cc/trees/layer_tree_host_unittest_animation.cc b/cc/trees/layer_tree_host_unittest_animation.cc index 9b16e39cb83f..72f10730a840 100644 --- a/cc/trees/layer_tree_host_unittest_animation.cc +++ b/cc/trees/layer_tree_host_unittest_animation.cc @@ -8,6 +8,7 @@ #include "cc/animation/layer_animation_controller.h" #include "cc/animation/scroll_offset_animation_curve.h" #include "cc/animation/timing_function.h" +#include "cc/base/completion_event.h" #include "cc/base/time_util.h" #include "cc/layers/layer.h" #include "cc/layers/layer_impl.h" @@ -1030,6 +1031,124 @@ class LayerTreeHostAnimationTestAddAnimationAfterAnimating SINGLE_AND_MULTI_THREAD_TEST_F( LayerTreeHostAnimationTestAddAnimationAfterAnimating); +class LayerTreeHostAnimationTestRemoveAnimation + : public LayerTreeHostAnimationTest { + public: + void SetupTree() override { + LayerTreeHostAnimationTest::SetupTree(); + layer_ = FakePictureLayer::Create(layer_settings(), &client_); + layer_->SetBounds(gfx::Size(4, 4)); + layer_tree_host()->root_layer()->AddChild(layer_); + } + + void BeginTest() override { PostSetNeedsCommitToMainThread(); } + + void DidCommit() override { + switch (layer_tree_host()->source_frame_number()) { + case 1: + AddAnimatedTransformToLayer(layer_.get(), 1.0, 5, 5); + break; + case 2: + LayerAnimationController* controller = + layer_->layer_animation_controller(); + Animation* animation = controller->GetAnimation(Animation::TRANSFORM); + controller->RemoveAnimation(animation->id()); + gfx::Transform transform; + transform.Translate(10.f, 10.f); + layer_->SetTransform(transform); + + // Do something that causes property trees to get rebuilt. + layer_->AddChild(Layer::Create(layer_settings())); + break; + } + } + + void DrawLayersOnThread(LayerTreeHostImpl* host_impl) override { + if (host_impl->active_tree()->source_frame_number() < 2) + return; + gfx::Transform expected_transform; + expected_transform.Translate(10.f, 10.f); + EXPECT_EQ(expected_transform, host_impl->active_tree() + ->root_layer() + ->children()[0] + ->draw_transform()); + EndTest(); + } + + void AfterTest() override {} + + private: + scoped_refptr layer_; + FakeContentLayerClient client_; +}; + +SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestRemoveAnimation); + +class LayerTreeHostAnimationTestAnimationFinishesDuringCommit + : public LayerTreeHostAnimationTest { + public: + void SetupTree() override { + LayerTreeHostAnimationTest::SetupTree(); + layer_ = FakePictureLayer::Create(layer_settings(), &client_); + layer_->SetBounds(gfx::Size(4, 4)); + layer_tree_host()->root_layer()->AddChild(layer_); + } + + void BeginTest() override { PostSetNeedsCommitToMainThread(); } + + void DidCommit() override { + if (layer_tree_host()->source_frame_number() == 1) + AddAnimatedTransformToLayer(layer_.get(), 0.04, 5, 5); + } + + void WillCommit() override { + if (layer_tree_host()->source_frame_number() == 2) { + // Block until the animation finishes on the compositor thread. Since + // animations have already been ticked on the main thread, when the commit + // happens the state on the main thread will be consistent with having a + // running animation but the state on the compositor thread will be + // consistent with having only a finished animation. + completion_.Wait(); + } + } + + void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) override { + switch (host_impl->sync_tree()->source_frame_number()) { + case 1: + PostSetNeedsCommitToMainThread(); + break; + case 2: + gfx::Transform expected_transform; + expected_transform.Translate(5.f, 5.f); + LayerImpl* layer_impl = + host_impl->sync_tree()->root_layer()->children()[0]; + EXPECT_EQ(expected_transform, layer_impl->draw_transform()); + EndTest(); + break; + } + } + + void UpdateAnimationState(LayerTreeHostImpl* host_impl, + bool has_unfinished_animation) override { + if (host_impl->active_tree()->source_frame_number() == 1 && + !has_unfinished_animation) { + // The animation has finished, so allow the main thread to commit. + completion_.Signal(); + } + } + + void AfterTest() override {} + + private: + scoped_refptr layer_; + FakeContentLayerClient client_; + CompletionEvent completion_; +}; + +// An animation finishing during commit can only happen when we have a separate +// compositor thread. +MULTI_THREAD_TEST_F(LayerTreeHostAnimationTestAnimationFinishesDuringCommit); + class LayerTreeHostAnimationTestNotifyAnimationFinished : public LayerTreeHostAnimationTest { public: diff --git a/cc/trees/layer_tree_host_unittest_animation_timelines.cc b/cc/trees/layer_tree_host_unittest_animation_timelines.cc index 4bf8e83d2212..472aa9823b23 100644 --- a/cc/trees/layer_tree_host_unittest_animation_timelines.cc +++ b/cc/trees/layer_tree_host_unittest_animation_timelines.cc @@ -13,6 +13,7 @@ #include "cc/animation/layer_animation_controller.h" #include "cc/animation/scroll_offset_animation_curve.h" #include "cc/animation/timing_function.h" +#include "cc/base/completion_event.h" #include "cc/base/time_util.h" #include "cc/layers/layer.h" #include "cc/layers/layer_impl.h" @@ -919,5 +920,133 @@ class LayerTreeHostTimelinesTestAddAnimationAfterAnimating SINGLE_AND_MULTI_THREAD_TEST_F( LayerTreeHostTimelinesTestAddAnimationAfterAnimating); +class LayerTreeHostTimelinesTestRemoveAnimation + : public LayerTreeHostTimelinesTest { + public: + void SetupTree() override { + LayerTreeHostTimelinesTest::SetupTree(); + layer_ = FakePictureLayer::Create(layer_settings(), &client_); + layer_->SetBounds(gfx::Size(4, 4)); + layer_tree_host()->root_layer()->AddChild(layer_); + + AttachPlayersToTimeline(); + + player_->AttachLayer(layer_tree_host()->root_layer()->id()); + player_child_->AttachLayer(layer_->id()); + } + + void BeginTest() override { PostSetNeedsCommitToMainThread(); } + + void DidCommit() override { + switch (layer_tree_host()->source_frame_number()) { + case 1: + AddAnimatedTransformToPlayer(player_child_.get(), 1.0, 5, 5); + break; + case 2: + LayerAnimationController* controller = + player_child_->element_animations()->layer_animation_controller(); + Animation* animation = controller->GetAnimation(Animation::TRANSFORM); + player_child_->RemoveAnimation(animation->id()); + gfx::Transform transform; + transform.Translate(10.f, 10.f); + layer_->SetTransform(transform); + + // Do something that causes property trees to get rebuilt. + layer_->AddChild(Layer::Create(layer_settings())); + break; + } + } + + void DrawLayersOnThread(LayerTreeHostImpl* host_impl) override { + if (host_impl->active_tree()->source_frame_number() < 2) + return; + gfx::Transform expected_transform; + expected_transform.Translate(10.f, 10.f); + EXPECT_EQ(expected_transform, host_impl->active_tree() + ->root_layer() + ->children()[0] + ->draw_transform()); + EndTest(); + } + + void AfterTest() override {} + + private: + scoped_refptr layer_; + FakeContentLayerClient client_; +}; + +SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTimelinesTestRemoveAnimation); + +class LayerTreeHostTimelinesTestAnimationFinishesDuringCommit + : public LayerTreeHostTimelinesTest { + public: + void SetupTree() override { + LayerTreeHostTimelinesTest::SetupTree(); + layer_ = FakePictureLayer::Create(layer_settings(), &client_); + layer_->SetBounds(gfx::Size(4, 4)); + layer_tree_host()->root_layer()->AddChild(layer_); + + AttachPlayersToTimeline(); + + player_->AttachLayer(layer_tree_host()->root_layer()->id()); + player_child_->AttachLayer(layer_->id()); + } + + void BeginTest() override { PostSetNeedsCommitToMainThread(); } + + void DidCommit() override { + if (layer_tree_host()->source_frame_number() == 1) + AddAnimatedTransformToPlayer(player_child_.get(), 0.04, 5, 5); + } + + void WillCommit() override { + if (layer_tree_host()->source_frame_number() == 2) { + // Block until the animation finishes on the compositor thread. Since + // animations have already been ticked on the main thread, when the commit + // happens the state on the main thread will be consistent with having a + // running animation but the state on the compositor thread will be + // consistent with having only a finished animation. + completion_.Wait(); + } + } + + void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) override { + switch (host_impl->sync_tree()->source_frame_number()) { + case 1: + PostSetNeedsCommitToMainThread(); + break; + case 2: + gfx::Transform expected_transform; + expected_transform.Translate(5.f, 5.f); + LayerImpl* layer_impl = + host_impl->sync_tree()->root_layer()->children()[0]; + EXPECT_EQ(expected_transform, layer_impl->draw_transform()); + EndTest(); + break; + } + } + + void UpdateAnimationState(LayerTreeHostImpl* host_impl, + bool has_unfinished_animation) override { + if (host_impl->active_tree()->source_frame_number() == 1 && + !has_unfinished_animation) { + // The animation has finished, so allow the main thread to commit. + completion_.Signal(); + } + } + + void AfterTest() override {} + + private: + scoped_refptr layer_; + FakeContentLayerClient client_; + CompletionEvent completion_; +}; + +// An animation finishing during commit can only happen when we have a separate +// compositor thread. +MULTI_THREAD_TEST_F(LayerTreeHostTimelinesTestAnimationFinishesDuringCommit); + } // namespace } // namespace cc diff --git a/cc/trees/layer_tree_host_unittest_picture.cc b/cc/trees/layer_tree_host_unittest_picture.cc index 7a0d7bfa5203..b12a170c7e0e 100644 --- a/cc/trees/layer_tree_host_unittest_picture.cc +++ b/cc/trees/layer_tree_host_unittest_picture.cc @@ -210,6 +210,10 @@ class LayerTreeHostPictureTestChangeLiveTilesRectWithRecycleTree picture_->SetBounds(gfx::Size(100, 100000)); root->AddChild(picture_); + // picture_'s transform is going to be changing on the compositor thread, so + // force it to have a transform node by making it scrollable. + picture_->SetScrollClipLayerId(root->id()); + layer_tree_host()->SetRootLayer(root); LayerTreeHostPictureTest::SetupTree(); } diff --git a/cc/trees/layer_tree_impl.cc b/cc/trees/layer_tree_impl.cc index 7044a65a3ed3..14e9dc5fa949 100644 --- a/cc/trees/layer_tree_impl.cc +++ b/cc/trees/layer_tree_impl.cc @@ -1664,6 +1664,15 @@ bool LayerTreeImpl::HasPotentiallyRunningTransformAnimation( : false; } +bool LayerTreeImpl::HasAnyAnimationTargetingProperty( + const LayerImpl* layer, + Animation::TargetProperty property) const { + return layer_tree_host_impl_->animation_host() + ? layer_tree_host_impl_->animation_host() + ->HasAnyAnimationTargetingProperty(layer->id(), property) + : false; +} + bool LayerTreeImpl::FilterIsAnimatingOnImplOnly(const LayerImpl* layer) const { return layer_tree_host_impl_->animation_host() ? layer_tree_host_impl_->animation_host() diff --git a/cc/trees/layer_tree_impl.h b/cc/trees/layer_tree_impl.h index 973df9777339..c203d5a62ae5 100644 --- a/cc/trees/layer_tree_impl.h +++ b/cc/trees/layer_tree_impl.h @@ -352,6 +352,10 @@ class CC_EXPORT LayerTreeImpl { bool HasPotentiallyRunningOpacityAnimation(const LayerImpl* layer) const; bool HasPotentiallyRunningTransformAnimation(const LayerImpl* layer) const; + bool HasAnyAnimationTargetingProperty( + const LayerImpl* layer, + Animation::TargetProperty property) const; + bool FilterIsAnimatingOnImplOnly(const LayerImpl* layer) const; bool OpacityIsAnimatingOnImplOnly(const LayerImpl* layer) const; bool TransformIsAnimatingOnImplOnly(const LayerImpl* layer) const; -- 2.11.4.GIT