[cc] Avoid checking WorkletAnimation::NeedsUpdate() twice

Currently we check whether ticking mutator is needed and if so we
collect mutator input state. There are duplicated logic in the two
operations above. This patch is to skip the first check and collect
input state directly. Then mutate if the input state is not empty.

Bug: 791279
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ib4c7b92ca0b545b42d75b91bc3fd7f9296034d74
Reviewed-on: https://chromium-review.googlesource.com/1103013
Commit-Queue: Yi Gu <yigu@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567830}

cc/animation/animation_host.cc

@@ -275,22 +275,20 @@ bool AnimationHost::NeedsTickAnimations() const {
   return !ticking_animations_.empty();
 }
 
-bool AnimationHost::NeedsTickMutator(base::TimeTicks monotonic_time,
-                                     const ScrollTree& scroll_tree,
-                                     bool is_active_tree) const {
+bool AnimationHost::TickMutator(base::TimeTicks monotonic_time,
+                                const ScrollTree& scroll_tree,
+                                bool is_active_tree) {
   if (!mutator_ || !mutator_->HasAnimators())
     return false;
 
-  for (auto& animation : ticking_animations_) {
-    if (!animation->IsWorkletAnimation())
-      continue;
+  std::unique_ptr<MutatorInputState> state = CollectWorkletAnimationsState(
+      monotonic_time, scroll_tree, is_active_tree);
+  if (state->IsEmpty())
+    return false;
 
-    if (ToWorkletAnimation(animation.get())
-            ->NeedsUpdate(monotonic_time, scroll_tree, is_active_tree))
-      return true;
-  }
+  mutator_->Mutate(std::move(state));
 
-  return false;
+  return true;
 }
 
 bool AnimationHost::ActivateAnimations() {
@@ -319,15 +317,12 @@ bool AnimationHost::TickAnimations(base::TimeTicks monotonic_time,
 
     did_animate = true;
   }
-  if (NeedsTickMutator(monotonic_time, scroll_tree, is_active_tree)) {
-    // TODO(majidvp): At the moment we call this for both active and pending
-    // trees similar to other animations. However our final goal is to only call
-    // it once, ideally after activation, and only when the input
-    // to an active timeline has changed. http://crbug.com/767210
-    mutator_->Mutate(CollectWorkletAnimationsState(monotonic_time, scroll_tree,
-                                                   is_active_tree));
-    did_animate = true;
-  }
+
+  // TODO(majidvp): At the moment we call this for both active and pending
+  // trees similar to other animations. However our final goal is to only call
+  // it once, ideally after activation, and only when the input
+  // to an active timeline has changed. http://crbug.com/767210
+  did_animate |= TickMutator(monotonic_time, scroll_tree, is_active_tree);
 
   return did_animate;
 }
@@ -339,15 +334,10 @@ void AnimationHost::TickScrollAnimations(base::TimeTicks monotonic_time,
   // fine-grained approach based on invalidating individual ScrollTimelines and
   // then ticking the animations attached to those timelines. To make
   // this happen we probably need to move "ticking" animations to timeline.
-  const bool is_active_tree = true;
-  if (!NeedsTickMutator(monotonic_time, scroll_tree, is_active_tree))
-    return;
-  DCHECK(mutator_);
 
   // TODO(majidvp): We need to return a boolean here so that LTHI knows
   // whether it needs to schedule another frame.
-  mutator_->Mutate(CollectWorkletAnimationsState(monotonic_time, scroll_tree,
-                                                 is_active_tree));
+  TickMutator(monotonic_time, scroll_tree, true /* is_active_tree */);
 }
 
 std::unique_ptr<MutatorInputState> AnimationHost::CollectWorkletAnimationsState(

cc/animation/animation_host.h

@@ -200,9 +200,10 @@ class CC_ANIMATION_EXPORT AnimationHost : public MutatorHost,
 
   void EraseTimeline(scoped_refptr<AnimationTimeline> timeline);
 
-  bool NeedsTickMutator(base::TimeTicks monotonic_time,
-                        const ScrollTree& scroll_tree,
-                        bool is_active_tree) const;
+  // Return true if there are any animations that get mutated.
+  bool TickMutator(base::TimeTicks monotonic_time,
+                   const ScrollTree& scroll_tree,
+                   bool is_active_tree);
 
   // Return the state representing all ticking worklet animations.
   std::unique_ptr<MutatorInputState> CollectWorkletAnimationsState(

cc/animation/worklet_animation.h

@@ -53,12 +53,6 @@ class CC_ANIMATION_EXPORT WorkletAnimation final
 
   void PushPropertiesTo(Animation* animation_impl) override;
 
-  // Returns true if the worklet animation needs to be updated which happens iff
-  // its current time is going to be different from last time given these input.
-  bool NeedsUpdate(base::TimeTicks monotonic_time,
-                   const ScrollTree& scroll_tree,
-                   bool is_active_tree);
-
   // Should be called when the scroll source of the ScrollTimeline attached to
   // this animation has a change in ElementId. Such a change happens when the
   // scroll source changes compositing state.
@@ -79,6 +73,12 @@ class CC_ANIMATION_EXPORT WorkletAnimation final
                      const ScrollTree& scroll_tree,
                      bool is_active_tree);
 
+  // Returns true if the worklet animation needs to be updated which happens iff
+  // its current time is going to be different from last time given these input.
+  bool NeedsUpdate(base::TimeTicks monotonic_time,
+                   const ScrollTree& scroll_tree,
+                   bool is_active_tree);
+
   std::unique_ptr<AnimationOptions> CloneOptions() const {
     return options_ ? options_->Clone() : nullptr;
   }

cc/animation/worklet_animation_unittest.cc

@@ -192,30 +192,6 @@ TEST_F(WorkletAnimationTest,
   EXPECT_EQ(246.8, third_state->updated_animations[0].current_time);
 }
 
-TEST_F(WorkletAnimationTest, NeedsUpdateCorrectlyReflectsInputTimeChange) {
-  scoped_refptr<WorkletAnimation> worklet_animation = WorkletAnimation::Create(
-      worklet_animation_id_, "test_name", nullptr, nullptr);
-
-  base::TimeTicks first_ticks =
-      base::TimeTicks() + base::TimeDelta::FromMillisecondsD(111);
-  base::TimeTicks second_ticks =
-      base::TimeTicks() + base::TimeDelta::FromMillisecondsD(111 + 123.4);
-
-  ScrollTree scroll_tree;
-  std::unique_ptr<MutatorInputState> state =
-      std::make_unique<MutatorInputState>();
-  // First time should always be true.
-  EXPECT_TRUE(worklet_animation->NeedsUpdate(first_ticks, scroll_tree, true));
-  worklet_animation->UpdateInputState(state.get(), first_ticks, scroll_tree,
-                                      true);
-  // Should be false if time is not different from last GetState.
-  EXPECT_FALSE(worklet_animation->NeedsUpdate(first_ticks, scroll_tree, true));
-  // Should be true when input time is different.
-  EXPECT_TRUE(worklet_animation->NeedsUpdate(second_ticks, scroll_tree, true));
-  // Should be side-effect free.
-  EXPECT_TRUE(worklet_animation->NeedsUpdate(second_ticks, scroll_tree, true));
-}
-
 // This test verifies that worklet animation state is properly updated.
 TEST_F(WorkletAnimationTest, WorkletAnimationStateTestWithSingleKeyframeModel) {
   AttachWorkletAnimation();

cc/trees/layer_tree_mutator.h

@@ -41,9 +41,14 @@ struct CC_EXPORT MutatorInputState {
   MutatorInputState();
   ~MutatorInputState();
 
-  std::vector<int> removed_animations;
+  bool IsEmpty() {
+    return added_and_updated_animations.empty() && updated_animations.empty() &&
+           removed_animations.empty();
+  }
+
   std::vector<AddAndUpdateState> added_and_updated_animations;
   std::vector<UpdateState> updated_animations;
+  std::vector<int> removed_animations;
 
   DISALLOW_COPY_AND_ASSIGN(MutatorInputState);
 };