cc: Avoid unnecessary pending trees during image animation updates.

CURRENT CODE:
The current flow of animations initiated by the ImageAnimationController
is as follows:

1) Each time a pending tree is updated, which is when animated images
from display lists would have been registered with the controller, we
compute the earliest time the next animation update will be needed and
schedule a delayed task to trigger an invalidation.

2) The delayed task triggers an invalidation, which update the frame on
the sync tree created as a result. And schedules the next invalidation
if needed.

THE BUG:
In the steps above, first for progressing the animation on the sync tree
and for scheduling the delayed task, we use the frame_time from the
BeginFrameArgs of the current frame as the "now" time. This resulted in
2 bugs:

1) Since the delayed task could be scheduled at any time within the impl
frame, using the frame_time from BeginFrameArgs as now was incorrect and
the delayed task would run at an unexpected time.

2) When the task did run at the correct time, resulting in an invalidation,
the pending tree would not be updated if the frame times of the animation
did not align with the BeginFrame frame times. For instance, if the frame
time is 0ms and the animation wants to run at 8ms. Even if the
invalidation runs at 8ms, the "now" time would be the BeginFrame
frame_time, which is 0ms and earlier than when the animation wants to run.
This would create wasted pending trees.

FIX:
This patch fixes the above by changing some fundamental aspects of how
image animations are progressed.

1) Instead of respecting the frame durations associated with the animated
image completely, snap them to frame boundaries. This ensures that the
animation always starts at the beginning of the frame in which the desired
time lies. This is also the optimal time to start the work for the frame
in which the animation wants to update.

2) Instead of always requesting an invalidation at the time for the next
animation update, request an impl frame instead. At the beginning of the
impl frame, upgrade it to an invalidation request if the frame_time for
that impl frame ensures that the animation will be updated.
This avoids a race between triggering an invalidation and receiving the
target BeginFrame in the renderer.

R=enne@chromium.org,sunnyps@chromium.org

Change-Id: Iffcaafc3dd72f7649ada1eceea2835054c44e911
Reviewed-on: https://chromium-review.googlesource.com/c/1464972
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Auto-Submit: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634935}

cc/trees/image_animation_controller.h

@@ -17,6 +17,7 @@
 #include "cc/paint/paint_image.h"
 #include "cc/paint/paint_image_generator.h"
 #include "cc/tiles/tile_priority.h"
+#include "components/viz/common/frame_sinks/begin_frame_args.h"
 
 namespace cc {
 class PaintImage;
@@ -34,7 +35,7 @@ class PaintImage;
 //    registered drivers interested in animating it.
 //
 //  3) An animation is only advanced on the sync tree, which is requested to be
-//     created using the |invalidation_callback|. This effectively means that
+//     created using the |client| callbacks. This effectively means that
 //     the frame of the image used remains consistent throughout the lifetime of
 //     a tree, guaranteeing that the image update is atomic.
 class CC_EXPORT ImageAnimationController {
@@ -52,16 +53,25 @@ class CC_EXPORT ImageAnimationController {
     virtual bool ShouldAnimate(PaintImage::Id paint_image_id) const = 0;
   };
 
-  // |invalidation_callback| is the callback to trigger an invalidation and
-  // create a sync tree for advancing an image animation. The controller is
-  // guaranteed to receive a call to AnimateForSyncTree when the sync tree is
-  // created.
-  // |task_runner| is the thread on which the controller is used. The
-  // invalidation_callback can only be run on this thread.
+  class CC_EXPORT Client {
+   public:
+    virtual ~Client() {}
+
+    // Notifies the client that an impl frame is needed to animate an image.
+    virtual void RequestBeginFrameForAnimatedImages() = 0;
+
+    // Notifies the client that a sync tree is needed to invalidate the animated
+    // images in this impl frame. This should only be called from within an impl
+    // frame.
+    virtual void RequestInvalidationForAnimatedImages() = 0;
+  };
+
+  // |task_runner| is the thread on which the controller is used. The |client|
+  // can only be called on this thread.
   // |enable_image_animation_resync| specifies whether the animation can be
   // reset to the beginning to avoid skipping many frames.
   ImageAnimationController(base::SingleThreadTaskRunner* task_runner,
-                           base::RepeatingClosure invalidation_callback,
+                           Client* client,
                            bool enable_image_animation_resync);
   ~ImageAnimationController();
 
@@ -83,13 +93,14 @@ class CC_EXPORT ImageAnimationController {
   // a call to DidActivate when this tree is activated.
   // Returns the set of images that were animated and should be invalidated on
   // this sync tree.
-  const PaintImageIdFlatSet& AnimateForSyncTree(base::TimeTicks now);
+  const PaintImageIdFlatSet& AnimateForSyncTree(
+      const viz::BeginFrameArgs& args);
 
   // Called whenever the ShouldAnimate response for a driver could have changed.
   // For instance on a change in the visibility of the image, we would pause
   // off-screen animations.
   // This is called after every DrawProperties update and commit.
-  void UpdateStateFromDrivers(base::TimeTicks now);
+  void UpdateStateFromDrivers();
 
   // Called when the sync tree was activated and the animations' associated
   // state should be pushed to the active tree.
@@ -99,6 +110,9 @@ class CC_EXPORT ImageAnimationController {
   size_t GetFrameIndexForImage(PaintImage::Id paint_image_id,
                                WhichTree tree) const;
 
+  // Notifies the beginning of an impl frame with the given |args|.
+  void WillBeginImplFrame(const viz::BeginFrameArgs& args);
+
   void set_did_navigate() { did_navigate_ = true; }
 
   const base::flat_set<AnimationDriver*>& GetDriversForTesting(
@@ -110,6 +124,11 @@ class CC_EXPORT ImageAnimationController {
     return animation_state_map_.size();
   }
 
+  using NowCallback = base::RepeatingCallback<base::TimeTicks()>;
+  void set_now_callback_for_testing(NowCallback cb) {
+    scheduler_.set_now_callback_for_testing(cb);
+  }
+
  private:
   class AnimationState {
    public:
@@ -119,7 +138,8 @@ class CC_EXPORT ImageAnimationController {
     ~AnimationState();
 
     bool ShouldAnimate() const;
-    bool AdvanceFrame(base::TimeTicks now, bool enable_image_animation_resync);
+    bool AdvanceFrame(const viz::BeginFrameArgs& args,
+                      bool enable_image_animation_resync);
     void UpdateMetadata(const DiscardableImageMap::AnimatedImageMetadata& data);
     void PushPendingToActive();
 
@@ -141,6 +161,8 @@ class CC_EXPORT ImageAnimationController {
     }
 
    private:
+    void AdvanceFrameInternal(const viz::BeginFrameArgs& args,
+                              bool enable_image_animation_resync);
     void ResetAnimation();
     size_t NextFrameIndex() const;
     bool is_complete() const {
@@ -199,30 +221,45 @@ class CC_EXPORT ImageAnimationController {
     DISALLOW_COPY_AND_ASSIGN(AnimationState);
   };
 
-  class DelayedNotifier {
+  class InvalidationScheduler {
    public:
-    DelayedNotifier(base::SingleThreadTaskRunner* task_runner,
-                    base::RepeatingClosure closure);
-    ~DelayedNotifier();
+    InvalidationScheduler(base::SingleThreadTaskRunner* task_runner,
+                          Client* client);
+    ~InvalidationScheduler();
 
-    void Schedule(base::TimeTicks now, base::TimeTicks notification_time);
+    void Schedule(base::TimeTicks animation_time);
     void Cancel();
     void WillAnimate();
+    void WillBeginImplFrame(const viz::BeginFrameArgs& args);
+    void set_now_callback_for_testing(NowCallback cb) {
+      now_callback_for_testing_ = cb;
+    }
 
    private:
-    void Notify();
+    enum InvalidationState {
+      // No notification pending.
+      kIdle,
+      // Task pending to request impl frame.
+      kPendingRequestBeginFrame,
+      // Impl frame request pending after request dispatched to client.
+      kPendingImplFrame,
+      // Sync tree for animation pending after request dispatched to client.
+      kPendingInvalidation,
+    };
+
+    void RequestBeginFrame();
+    void RequestInvalidation();
 
     base::SingleThreadTaskRunner* task_runner_;
-    base::RepeatingClosure closure_;
+    Client* const client_;
+    NowCallback now_callback_for_testing_;
 
-    // Set if a notification is currently pending.
-    base::Optional<base::TimeTicks> pending_notification_time_;
+    InvalidationState state_ = InvalidationState::kIdle;
 
-    // Set if the notification was dispatched and the resulting animation on the
-    // next sync tree is pending.
-    bool animation_pending_ = false;
+    // The time at which the next animation is expected to run.
+    base::TimeTicks next_animation_time_;
 
-    base::WeakPtrFactory<DelayedNotifier> weak_factory_;
+    base::WeakPtrFactory<InvalidationScheduler> weak_factory_;
   };
 
   // The AnimationState for images is persisted until they are cleared on
@@ -239,7 +276,7 @@ class CC_EXPORT ImageAnimationController {
   // The set of images that were animated and invalidated on the last sync tree.
   PaintImageIdFlatSet images_animated_on_sync_tree_;
 
-  DelayedNotifier notifier_;
+  InvalidationScheduler scheduler_;
 
   const bool enable_image_animation_resync_;
 

cc/trees/layer_tree_host_impl.cc

@@ -322,12 +322,9 @@ LayerTreeHostImpl::LayerTreeHostImpl(
       scroll_animating_latched_element_id_(kInvalidElementId),
       // It is safe to use base::Unretained here since we will outlive the
       // ImageAnimationController.
-      image_animation_controller_(
-          GetTaskRunner(),
-          base::BindRepeating(
-              &LayerTreeHostImpl::RequestInvalidationForAnimatedImages,
-              base::Unretained(this)),
-          settings_.enable_image_animation_resync),
+      image_animation_controller_(GetTaskRunner(),
+                                  this,
+                                  settings_.enable_image_animation_resync),
       frame_metrics_(LTHI_FrameMetricsSettings(settings_)),
       skipped_frame_tracker_(&frame_metrics_),
       is_animating_for_snap_(false),
@@ -485,8 +482,8 @@ void LayerTreeHostImpl::UpdateSyncTreeAfterCommitOrImplSideInvalidation() {
   if (ukm_manager_)
     ukm_manager_->AddCheckerboardedImages(images_to_invalidate.size());
 
-  const auto& animated_images = image_animation_controller_.AnimateForSyncTree(
-      CurrentBeginFrameArgs().frame_time);
+  const auto& animated_images =
+      image_animation_controller_.AnimateForSyncTree(CurrentBeginFrameArgs());
   images_to_invalidate.insert(animated_images.begin(), animated_images.end());
   sync_tree()->InvalidateRegionForImages(images_to_invalidate);
 
@@ -2426,6 +2423,8 @@ bool LayerTreeHostImpl::WillBeginImplFrame(const viz::BeginFrameArgs& args) {
 
   Animate();
 
+  image_animation_controller_.WillBeginImplFrame(args);
+
   for (auto* it : video_frame_controllers_)
     it->OnBeginFrame(args);
 
@@ -5683,13 +5682,6 @@ void LayerTreeHostImpl::ShowScrollbarsForImplScroll(ElementId element_id) {
     animation_controller->DidScrollUpdate();
 }
 
-void LayerTreeHostImpl::RequestInvalidationForAnimatedImages() {
-  // If we are animating an image, we want at least one draw of the active tree
-  // before a new tree is activated.
-  bool needs_first_draw_on_activation = true;
-  client_->NeedsImplSideInvalidation(needs_first_draw_on_activation);
-}
-
 void LayerTreeHostImpl::InitializeUkm(
     std::unique_ptr<ukm::UkmRecorder> recorder) {
   DCHECK(!ukm_manager_);
@@ -5744,4 +5736,17 @@ void LayerTreeHostImpl::AllocateLocalSurfaceId() {
       child_local_surface_id_allocator_.GetCurrentLocalSurfaceIdAllocation());
 }
 
+void LayerTreeHostImpl::RequestBeginFrameForAnimatedImages() {
+  SetNeedsOneBeginImplFrame();
+}
+
+void LayerTreeHostImpl::RequestInvalidationForAnimatedImages() {
+  DCHECK_EQ(impl_thread_phase_, ImplThreadPhase::INSIDE_IMPL_FRAME);
+
+  // If we are animating an image, we want at least one draw of the active tree
+  // before a new tree is activated.
+  bool needs_first_draw_on_activation = true;
+  client_->NeedsImplSideInvalidation(needs_first_draw_on_activation);
+}
+
 }  // namespace cc

cc/trees/layer_tree_host_impl.h

@@ -171,6 +171,7 @@ class CC_EXPORT LayerTreeHostImpl
       public ScrollbarAnimationControllerClient,
       public VideoFrameControllerClient,
       public MutatorHostClient,
+      public ImageAnimationController::Client,
       public base::SupportsWeakPtr<LayerTreeHostImpl> {
  public:
   // This structure is used to build all the state required for producing a
@@ -296,6 +297,10 @@ class CC_EXPORT LayerTreeHostImpl
   bool HaveRootScrollNode() const override;
   void SetNeedsCommit() override;
 
+  // ImageAnimationController::Client implementation.
+  void RequestBeginFrameForAnimatedImages() override;
+  void RequestInvalidationForAnimatedImages() override;
+
   void UpdateViewportContainerSizes();
 
   void set_resourceless_software_draw_for_testing() {
@@ -874,9 +879,6 @@ class CC_EXPORT LayerTreeHostImpl
   // tree, because the active tree value always takes precedence for scrollbars.
   void PushScrollbarOpacitiesFromActiveToPending();
 
-  // Request an impl-side invalidation to animate an image.
-  void RequestInvalidationForAnimatedImages();
-
   // Pushes state for image animations and checkerboarded images from the
   // pending to active tree. This is called during activation when a pending
   // tree exists, and during the commit if we are committing directly to the

cc/trees/layer_tree_impl.cc

@@ -1366,8 +1366,7 @@ bool LayerTreeImpl::UpdateDrawProperties(
   }
 
   if (update_image_animation_controller && image_animation_controller()) {
-    image_animation_controller()->UpdateStateFromDrivers(
-        CurrentBeginFrameArgs().frame_time);
+    image_animation_controller()->UpdateStateFromDrivers();
   }
 
   DCHECK(!needs_update_draw_properties_)