blink: Cleanup image animation advancement in BitmapImage.

When catching up the animation, if the StartAnimation call for
advancing the next frame was delayed, if n is the desired current frame,
we advance uptill frame n-1 and a post a task to further advance to
frame n. This task also calls StartAnimation to schedule a task for
the next frame of the animation. In this call, kDoNotCatchUp is used to
avoid going through a catch up loop and schedule this task immediately
if necessary.

Clean up the logic to advance to frame n during catch up and schedule
the next task synchronously. And add a test.

R=chrishtr@chromium.org

Change-Id: I5fdb1d978001b0bf5f04cdaf4dada110db065801
Reviewed-on: https://chromium-review.googlesource.com/674056
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504249}

third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp

@@ -564,8 +564,7 @@ void SVGImage::FlushPendingTimelineRewind() {
   has_pending_timeline_rewind_ = false;
 }
 
-// FIXME: support CatchUpAnimation = CatchUp.
-void SVGImage::StartAnimation(CatchUpAnimation) {
+void SVGImage::StartAnimation() {
   SVGSVGElement* root_element = SvgRootElement(page_.Get());
   if (!root_element)
     return;

third_party/WebKit/Source/core/svg/graphics/SVGImage.h

@@ -72,7 +72,7 @@ class CORE_EXPORT SVGImage final : public Image {
   void CheckLoaded() const;
   bool CurrentFrameHasSingleSecurityOrigin() const override;
 
-  void StartAnimation(CatchUpAnimation = kCatchUp) override;
+  void StartAnimation() override;
   void ResetAnimation() override;
 
   PaintImage::CompletionState completion_state() const {

third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

@@ -84,7 +84,8 @@ BitmapImage::BitmapImage(ImageObserver* observer, bool is_multipart)
       task_runner_(Platform::Current()
                        ->CurrentThread()
                        ->Scheduler()
-                       ->CompositorTaskRunner()) {}
+                       ->CompositorTaskRunner()),
+      weak_factory_(this) {}
 
 BitmapImage::~BitmapImage() {
   StopAnimation();
@@ -484,12 +485,18 @@ bool BitmapImage::ShouldAnimate() {
   return animated;
 }
 
-void BitmapImage::StartAnimation(CatchUpAnimation catch_up_if_necessary) {
+void BitmapImage::StartAnimation() {
+  StartAnimationInternal(MonotonicallyIncreasingTime());
+}
+
+void BitmapImage::StartAnimationInternal(const double time) {
+  // If the |frame_timer_| is set, it indicates that a task is already pending
+  // to advance the current frame of the animation. We don't need to schedule
+  // a task to advance the animation in that case.
   if (frame_timer_ || !ShouldAnimate() || FrameCount() <= 1)
     return;
 
   // If we aren't already animating, set now as the animation start time.
-  const double time = MonotonicallyIncreasingTime();
   if (!desired_frame_start_time_)
     desired_frame_start_time_ = time;
 
@@ -538,8 +545,7 @@ void BitmapImage::StartAnimation(CatchUpAnimation catch_up_if_necessary) {
       desired_frame_start_time_ < time)
     desired_frame_start_time_ = time;
 
-  if (catch_up_if_necessary == kDoNotCatchUp ||
-      time < desired_frame_start_time_) {
+  if (time < desired_frame_start_time_) {
     // Haven't yet reached time for next frame to start; delay until then.
     frame_timer_ = WTF::WrapUnique(new TaskRunnerTimer<BitmapImage>(
         task_runner_, this, &BitmapImage::AdvanceAnimation));
@@ -550,6 +556,18 @@ void BitmapImage::StartAnimation(CatchUpAnimation catch_up_if_necessary) {
     // See if we've also passed the time for frames after that to start, in
     // case we need to skip some frames entirely.  Remember not to advance
     // to an incomplete frame.
+
+    // Skip the next frame by advancing the animation forward one frame.
+    if (!InternalAdvanceAnimation(kSkipFramesToCatchUp)) {
+      DCHECK(animation_finished_);
+      return;
+    }
+
+    // We have already realized that we need to skip the |next_frame| since
+    // |desired_frame_start_time_| is when |next_frame| should have been
+    // displayed, which is in the past. The rest of the loop determines if more
+    // frames need to be skipped to catch up.
+    last_num_frames_skipped_ = 1u;
     for (size_t frame_after_next = (next_frame + 1) % FrameCount();
          FrameIsReceivedAtIndex(frame_after_next);
          frame_after_next = (next_frame + 1) % FrameCount()) {
@@ -567,17 +585,27 @@ void BitmapImage::StartAnimation(CatchUpAnimation catch_up_if_necessary) {
         DCHECK(animation_finished_);
         return;
       }
+      last_num_frames_skipped_++;
       desired_frame_start_time_ = frame_after_next_start_time;
       next_frame = frame_after_next;
     }
 
-    // Post a task to advance the frame immediately. m_desiredFrameStartTime
-    // may be in the past, meaning the next time through this function we'll
-    // kick off the next advancement sooner than this frame's duration would
-    // suggest.
-    frame_timer_ = WTF::WrapUnique(new TaskRunnerTimer<BitmapImage>(
-        task_runner_, this, &BitmapImage::AdvanceAnimationWithoutCatchUp));
-    frame_timer_->StartOneShot(0, BLINK_FROM_HERE);
+    // Since we just advanced a bunch of frames during catch up, post a
+    // notification to the observers. Note this has to be async because
+    // animation can happen during painting and this invalidation is required
+    // after the current paint.
+    task_runner_->PostTask(
+        BLINK_FROM_HERE,
+        WTF::Bind(&BitmapImage::NotifyObserversOfAnimationAdvance,
+                  weak_factory_.CreateWeakPtr(), nullptr));
+
+    // Set up the timer for the next frame if required. Note that we have
+    // already advanced to the current_frame_index_ after catching up. And in
+    // the loop above, we either could not advance the animation further or we
+    // advanced it up till the desired time for the current frame. This ensures
+    // that calling StartAnimationInternal here with the same |time| will not
+    // need to perform any catch up skipping.
+    StartAnimationInternal(time);
   }
 }
 
@@ -620,11 +648,6 @@ void BitmapImage::AdvanceAnimation(TimerBase*) {
   // startAnimation() again to keep the animation moving.
 }
 
-void BitmapImage::AdvanceAnimationWithoutCatchUp(TimerBase*) {
-  if (InternalAdvanceAnimation())
-    StartAnimation(kDoNotCatchUp);
-}
-
 bool BitmapImage::InternalAdvanceAnimation(AnimationAdvancement advancement) {
   // Stop the animation.
   StopAnimation();

third_party/WebKit/Source/platform/graphics/BitmapImage.h

@@ -99,6 +99,13 @@ class PLATFORM_EXPORT BitmapImage final : public Image {
   void SetDecoderForTesting(std::unique_ptr<DeferredImageDecoder> decoder) {
     decoder_ = std::move(decoder);
   }
+  void SetTaskRunnerForTesting(RefPtr<WebTaskRunner> task_runner) {
+    task_runner_ = task_runner;
+  }
+
+  size_t last_num_frames_skipped_for_testing() const {
+    return last_num_frames_skipped_;
+  }
 
  private:
   enum RepetitionCountStatus : uint8_t {
@@ -154,17 +161,11 @@ class PLATFORM_EXPORT BitmapImage final : public Image {
   int RepetitionCount();
 
   bool ShouldAnimate();
-  void StartAnimation(CatchUpAnimation = kCatchUp) override;
+  void StartAnimation() override;
+  void StartAnimationInternal(const double time);
   void StopAnimation();
   void AdvanceAnimation(TimerBase*);
 
-  // Advance the animation and let the next frame get scheduled without
-  // catch-up logic. For large images with slow or heavily-loaded systems,
-  // throwing away data as we go (see destroyDecodedData()) means we can spend
-  // so much time re-decoding data that we are always behind. To prevent this,
-  // we force the next animation to skip the catch up logic.
-  void AdvanceAnimationWithoutCatchUp(TimerBase*);
-
   // This function does the real work of advancing the animation. When
   // skipping frames to catch up, we're in the middle of a loop trying to skip
   // over a bunch of animation frames, so we should not do things like decode
@@ -213,9 +214,13 @@ class PLATFORM_EXPORT BitmapImage final : public Image {
   double desired_frame_start_time_;  // The system time at which we hope to see
                                      // the next call to startAnimation().
 
+  size_t last_num_frames_skipped_ = 0u;
+
   size_t frame_count_;
 
   RefPtr<WebTaskRunner> task_runner_;
+
+  WTF::WeakPtrFactory<BitmapImage> weak_factory_;
 };
 
 DEFINE_IMAGE_TYPE_CASTS(BitmapImage);

third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp

@@ -36,6 +36,7 @@
 #include "platform/graphics/ImageObserver.h"
 #include "platform/graphics/test/MockImageDecoder.h"
 #include "platform/runtime_enabled_features.h"
+#include "platform/scheduler/test/fake_web_task_runner.h"
 #include "platform/testing/HistogramTester.h"
 #include "platform/testing/TestingPlatformSupport.h"
 #include "platform/testing/UnitTestHelpers.h"
@@ -85,6 +86,7 @@ class BitmapImageTest : public ::testing::Test {
     return image_->frames_[frame].frame_bytes_;
   }
   size_t DecodedFramesCount() const { return image_->frames_.size(); }
+  void StartAnimation() { image_->StartAnimation(); }
 
   void SetFirstFrameNotComplete() { image_->frames_[0].is_complete_ = false; }
 
@@ -347,6 +349,8 @@ class BitmapImageTestWithMockDecoder : public BitmapImageTest,
                                        public MockImageDecoderClient {
  public:
   void SetUp() override {
+    original_time_function_ = SetTimeFunctionsForTesting([] { return now_; });
+
     BitmapImageTest::SetUp();
     auto decoder = MockImageDecoder::Create(this);
     decoder->SetSize(10, 10);
@@ -354,6 +358,10 @@ class BitmapImageTestWithMockDecoder : public BitmapImageTest,
         DeferredImageDecoder::CreateForTesting(std::move(decoder)));
   }
 
+  void TearDown() override {
+    SetTimeFunctionsForTesting(original_time_function_);
+  }
+
   void DecoderBeingDestroyed() override {}
   void DecodeRequested() override {}
   ImageFrame::Status GetStatus(size_t index) override {
@@ -371,8 +379,13 @@ class BitmapImageTestWithMockDecoder : public BitmapImageTest,
   size_t frame_count_;
   ImageFrame::Status status_;
   bool last_frame_complete_;
+
+  static double now_;
+  TimeFunction original_time_function_;
 };
 
+double BitmapImageTestWithMockDecoder::now_ = 0;
+
 TEST_F(BitmapImageTestWithMockDecoder, ImageMetadataTracking) {
   // For a zero duration, we should make it non-zero when creating a PaintImage.
   repetition_count_ = kAnimationLoopOnce;
@@ -442,6 +455,58 @@ TEST_F(BitmapImageTestWithMockDecoder, AnimationPolicyOverride) {
   EXPECT_EQ(image.repetition_count(), repetition_count_);
 }
 
+TEST_F(BitmapImageTestWithMockDecoder, FrameSkipTracking) {
+  RuntimeEnabledFeatures::SetCompositorImageAnimationsEnabled(false);
+
+  repetition_count_ = kAnimationLoopInfinite;
+  frame_count_ = 5u;
+  last_frame_complete_ = true;
+  duration_ = TimeDelta::FromSeconds(10);
+  now_ = 10;
+  image_->SetData(SharedBuffer::Create("data", sizeof("data")), true);
+
+  RefPtr<scheduler::FakeWebTaskRunner> task_runner =
+      AdoptRef(new scheduler::FakeWebTaskRunner);
+  image_->SetTaskRunnerForTesting(task_runner);
+  task_runner->SetTime(10);
+
+  // Start the animation at 10s. This should schedule a timer for the next frame
+  // after 10 seconds.
+  StartAnimation();
+  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 0u);
+
+  // No frames skipped since we just started the animation.
+  EXPECT_EQ(image_->last_num_frames_skipped_for_testing(), 0u);
+
+  // Advance the time to 15s. The frame is still at 0u because the posted task
+  // should run at 20s.
+  task_runner->AdvanceTimeAndRun(5);
+  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 0u);
+
+  // Advance the time to 20s. The task runs and advances the animation forward
+  // to 1u.
+  task_runner->AdvanceTimeAndRun(5);
+  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 1u);
+
+  // Set now_ to 41 seconds. Since the animation started at 10s, and each frame
+  // has a duration of 10s, we should see the fourth frame at 41 seconds.
+  now_ = 41;
+  task_runner->SetTime(41);
+  StartAnimation();
+  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 3u);
+  EXPECT_EQ(image_->last_num_frames_skipped_for_testing(), 2u);
+
+  // We should have scheduled a task to move to the last frame in
+  // StartAnimation above, at 50s.
+  // Advance by 5s, not time for the next frame yet.
+  task_runner->AdvanceTimeAndRun(5);
+  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 3u);
+
+  task_runner->SetTime(50);
+  task_runner->AdvanceTimeAndRun(0);
+  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 4u);
+}
+
 template <typename HistogramEnumType>
 struct HistogramTestParams {
   const char* filename;

third_party/WebKit/Source/platform/graphics/Image.h

@@ -140,8 +140,7 @@ class PLATFORM_EXPORT Image : public ThreadSafeRefCounted<Image> {
   // Animation begins whenever someone draws the image, so startAnimation() is
   // not normally called. It will automatically pause once all observers no
   // longer want to render the image anywhere.
-  enum CatchUpAnimation { kDoNotCatchUp, kCatchUp };
-  virtual void StartAnimation(CatchUpAnimation = kCatchUp) {}
+  virtual void StartAnimation() {}
   virtual void ResetAnimation() {}
 
   // True if this image can potentially animate.