Changeset 207386 in webkit

Timestamp:

Oct 15, 2016 5:19:22 PM (8 years ago)

Author:

commit-queue@webkit.org

Message:

Delete the animated image catchup code
​https://bugs.webkit.org/show_bug.cgi?id=163410

Patch by Said Abou-Hallawa <​sabouhallawa@apple.com> on 2016-10-15
Reviewed by Simon Fraser.

Source/WebCore:

This patch fixes two issues in the animated image workflow:

1) Setting the animation timer should follow the following rules:

-- Initial case: Which happens before drawing the first frame. We
should set the timer to fire after the current_frame_duration.

-- Late case (Slow animation): This happens if the current_time is
past the next_frame_desired_time. In this case we should fire the
timer immediately.

-- Early case (Fast animation): This happens when there is still time
before the next_frame_desired_time. In this case we should set the
timer to fire after the difference between the next_frame_desired_time
and the current_time.

2) Deleting the code for catching up the current_frame:

This code used to run in the slow animation case. It was never used
on iOS. It was trying to adjust the current_frame according to the
current_time as if there were no delay. It turned out that this might
cause a bigger delay because most likely the decoder decodes the image
frames incrementally; i.e. to decode frame k, it has to have frame
(k - 1) decoded.

Test: fast/images/ordered-animated-image-frames.html

• platform/graphics/BitmapImage.cpp:

(WebCore::BitmapImage::draw): Remove the iOS specific code.
(WebCore::BitmapImage::startAnimation): Move the animation finishing code from
BitmapImage::internalAdvanceAnimation() to this function. Simplify the timer
duration code as it is described above.

(WebCore::BitmapImage::advanceAnimation): Merge BitmapImage::internalAdvanceAnimation()
into this function.

(WebCore::BitmapImage::resetAnimation):

(WebCore::BitmapImage::internalAdvanceAnimation): Deleted.

• platform/graphics/BitmapImage.h:

• platform/graphics/Image.cpp:

(WebCore::Image::drawTiled):

• platform/graphics/Image.h:

(WebCore::Image::startAnimation):

• svg/graphics/SVGImage.cpp:

(WebCore::SVGImage::startAnimation):

• svg/graphics/SVGImage.h:

Remove the catchup code form the Image and SVGImage classes.

LayoutTests:

This animated gif has one red frame, one green frame and two red frames.
The test page renders only two frames from this this image on a canvas. The
test passes if the second frame (the green one) is rendered on the canvas
even if drawImage() is called after the duration of the first frame.

• fast/images/ordered-animated-image-frames-expected.html: Added.
• fast/images/ordered-animated-image-frames.html: Added.
• fast/images/resources/animated-red-green-blue.gif: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/images/ordered-animated-image-frames-expected.html (added)
• LayoutTests/fast/images/ordered-animated-image-frames.html (added)
• LayoutTests/fast/images/resources/animated-red-green-blue.gif (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/BitmapImage.cpp (modified) (4 diffs)
• Source/WebCore/platform/graphics/BitmapImage.h (modified) (2 diffs)
• Source/WebCore/platform/graphics/Image.cpp (modified) (2 diffs)
• Source/WebCore/platform/graphics/Image.h (modified) (1 diff)
• Source/WebCore/svg/graphics/SVGImage.cpp (modified) (1 diff)
• Source/WebCore/svg/graphics/SVGImage.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-10-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Delete the animated image catchup code
+        https://bugs.webkit.org/show_bug.cgi?id=163410
+
+        Reviewed by Simon Fraser.
+
+        This animated gif has one red frame, one green frame and two red frames.
+        The test page renders only two frames from this this image on a canvas. The
+        test passes if the second frame (the green one) is rendered on the canvas
+        even if drawImage() is called after the duration of the first frame.
+
+        * fast/images/ordered-animated-image-frames-expected.html: Added.
+        * fast/images/ordered-animated-image-frames.html: Added.
+        * fast/images/resources/animated-red-green-blue.gif: Added.
+
 2016-10-15  Myles C. Maxfield  <mmaxfield@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-10-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Delete the animated image catchup code
+        https://bugs.webkit.org/show_bug.cgi?id=163410
+
+        Reviewed by Simon Fraser.
+
+        This patch fixes two issues in the animated image workflow:
+
+        1) Setting the animation timer should follow the following rules:
+        
+            -- Initial case: Which happens before drawing the first frame. We
+            should set the timer to fire after the current_frame_duration.
+
+            -- Late case (Slow animation): This happens if the current_time is
+            past the next_frame_desired_time. In this case we should fire the
+            timer immediately.
+
+            -- Early case (Fast animation): This happens when there is still time
+            before the next_frame_desired_time. In this case we should set the
+            timer to fire after the difference between the next_frame_desired_time
+            and the current_time.
+
+        2) Deleting the code for catching up the current_frame:
+        
+            This code used to run in the slow animation case. It was never used
+            on iOS. It was trying to adjust the current_frame according to the
+            current_time as if there were no delay. It turned out that this might
+            cause a bigger delay because most likely the decoder decodes the image
+            frames incrementally; i.e. to decode frame k, it has to have frame
+            (k - 1) decoded.
+
+        Test: fast/images/ordered-animated-image-frames.html
+
+        * platform/graphics/BitmapImage.cpp:
+        (WebCore::BitmapImage::draw): Remove the iOS specific code.
+        (WebCore::BitmapImage::startAnimation): Move the animation finishing code from 
+        BitmapImage::internalAdvanceAnimation() to this function. Simplify the timer
+        duration code as it is described above.
+
+        (WebCore::BitmapImage::advanceAnimation): Merge BitmapImage::internalAdvanceAnimation()
+        into this function.
+
+        (WebCore::BitmapImage::resetAnimation):
+        
+        (WebCore::BitmapImage::internalAdvanceAnimation): Deleted.
+        * platform/graphics/BitmapImage.h:
+
+        * platform/graphics/Image.cpp:
+        (WebCore::Image::drawTiled):
+        * platform/graphics/Image.h:
+        (WebCore::Image::startAnimation):
+        * svg/graphics/SVGImage.cpp:
+        (WebCore::SVGImage::startAnimation):
+        * svg/graphics/SVGImage.h:
+        Remove the catchup code form the Image and SVGImage classes.
+
 2016-10-15  Darin Adler  <darin@apple.com>
 

trunk/Source/WebCore/platform/graphics/BitmapImage.cpp

@@ -142 +142 @@
         return;
 
-#if PLATFORM(IOS)
-    startAnimation(DoNotCatchUp);
-#else
     startAnimation();
-#endif
 
     Color color = singlePixelSolidColor();
@@ -228 +224 @@
 }
 
-void BitmapImage::startAnimation(CatchUpAnimation catchUpIfNecessary)
+void BitmapImage::startAnimation()
 {
     if (m_frameTimer || !shouldAnimate() || frameCount() <= 1)
         return;
 
-    // If we aren't already animating, set now as the animation start time.
-    const double time = monotonicallyIncreasingTime();
-    if (!m_desiredFrameStartTime)
-        m_desiredFrameStartTime = time;
+    if (m_currentFrame >= frameCount() - 1) {
+        // Don't advance past the last frame if we haven't decoded the whole image
+        // yet and our repetition count is potentially unset. The repetition count
+        // in a GIF can potentially come after all the rest of the image data, so
+        // wait on it.
+        if (!m_source.isAllDataReceived() && repetitionCount() == RepetitionCountOnce)
+            return;
+
+        ++m_repetitionsComplete;
+
+        // Check for the end of animation.
+        if (repetitionCount() != RepetitionCountInfinite && m_repetitionsComplete > repetitionCount()) {
+            m_animationFinished = true;
+            destroyDecodedDataIfNecessary(false);
+            return;
+        }
+
+        destroyDecodedDataIfNecessary(true);
+    }
 
     // Don't advance the animation to an incomplete frame.
@@ -243 +254 @@
         return;
 
-    // Don't advance past the last frame if we haven't decoded the whole image
-    // yet and our repetition count is potentially unset. The repetition count
-    // in a GIF can potentially come after all the rest of the image data, so
-    // wait on it.
-    if (!m_source.isAllDataReceived() && repetitionCount() == RepetitionCountOnce && m_currentFrame >= (frameCount() - 1))
-        return;
-
-    // Determine time for next frame to start. By ignoring paint and timer lag
-    // in this calculation, we make the animation appear to run at its desired
-    // rate regardless of how fast it's being repainted.
-    const double currentDuration = frameDurationAtIndex(m_currentFrame);
-    m_desiredFrameStartTime += currentDuration;
-
-#if !PLATFORM(IOS)
-    // When an animated image is more than five minutes out of date, the
-    // user probably doesn't care about resyncing and we could burn a lot of
-    // time looping through frames below. Just reset the timings.
-    const double cAnimationResyncCutoff = 5 * 60;
-    if ((time - m_desiredFrameStartTime) > cAnimationResyncCutoff)
-        m_desiredFrameStartTime = time + currentDuration;
-#else
-    // Maintaining frame-to-frame delays is more important than
-    // maintaining absolute animation timing, so reset the timings each frame.
-    m_desiredFrameStartTime = time + currentDuration;
-#endif
-
-    // The image may load more slowly than it's supposed to animate, so that by
-    // the time we reach the end of the first repetition, we're well behind.
-    // Clamp the desired frame start time in this case, so that we don't skip
-    // frames (or whole iterations) trying to "catch up". This is a tradeoff:
-    // It guarantees users see the whole animation the second time through and
-    // don't miss any repetitions, and is closer to what other browsers do; on
-    // the other hand, it makes animations "less accurate" for pages that try to
-    // sync an image and some other resource (e.g. audio), especially if users
-    // switch tabs (and thus stop drawing the animation, which will pause it)
-    // during that initial loop, then switch back later.
-    if (nextFrame == 0 && m_repetitionsComplete == 0 && m_desiredFrameStartTime < time)
+    double time = monotonicallyIncreasingTime();
+
+    // Handle initial state.
+    if (!m_desiredFrameStartTime)
         m_desiredFrameStartTime = time;
 
-    if (catchUpIfNecessary == DoNotCatchUp || time < m_desiredFrameStartTime) {
-        // Haven't yet reached time for next frame to start; delay until then.
-        startTimer(std::max<double>(m_desiredFrameStartTime - time, 0));
-        return;
-    }
+    // Setting 'm_desiredFrameStartTime' to 'time' means we are late; otherwise we are early.
+    m_desiredFrameStartTime = std::max(time, m_desiredFrameStartTime + frameDurationAtIndex(m_currentFrame));
 
     ASSERT(!m_frameTimer);
-
-    // We've already reached or passed the time for the next frame to start.
-    // 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.
-
-#if !LOG_DISABLED
-    size_t startCatchupFrameIndex = nextFrame;
-#endif
-
-    for (size_t frameAfterNext = (nextFrame + 1) % frameCount(); frameIsCompleteAtIndex(frameAfterNext); frameAfterNext = (nextFrame + 1) % frameCount()) {
-        // Should we skip the next frame?
-        double frameAfterNextStartTime = m_desiredFrameStartTime + frameDurationAtIndex(nextFrame);
-        if (time < frameAfterNextStartTime)
-            break;
-
-        // Yes; skip over it without notifying our observers. If we hit the end while catching up,
-        // tell the observer asynchronously.
-        if (!internalAdvanceAnimation(SkippingFramesToCatchUp)) {
-            m_animationFinishedWhenCatchingUp = true;
-            startTimer(0);
-            LOG(Images, "BitmapImage %p startAnimation catching up from frame %lu, ended", this, startCatchupFrameIndex);
-            return;
-        }
-        m_desiredFrameStartTime = frameAfterNextStartTime;
-        nextFrame = frameAfterNext;
-    }
-
-    LOG(Images, "BitmapImage %p startAnimation catching up jumped from from frame %lu to %d", this, startCatchupFrameIndex, (int)nextFrame - 1);
-
-    // Draw the next frame as soon as possible. Note that 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.
-    startTimer(0);
+    startTimer(m_desiredFrameStartTime - time);
 }
 
 void BitmapImage::advanceAnimation()
 {
-    internalAdvanceAnimation();
-    // At this point the image region has been marked dirty, and if it's
-    // onscreen, we'll soon make a call to draw(), which will call
-    // startAnimation() again to keep the animation moving.
-}
-
-bool BitmapImage::internalAdvanceAnimation(AnimationAdvancement advancement)
-{
     clearTimer();
 
-    if (m_animationFinishedWhenCatchingUp) {
+    m_currentFrame = (m_currentFrame + 1) % frameCount();
+    destroyDecodedDataIfNecessary(false);
+
+    if (imageObserver())
         imageObserver()->animationAdvanced(this);
-        m_animationFinishedWhenCatchingUp = false;
-        return false;
-    }
-
-    ++m_currentFrame;
-    bool advancedAnimation = true;
-    bool destroyAll = false;
-    if (m_currentFrame >= frameCount()) {
-        ++m_repetitionsComplete;
-
-        // Get the repetition count again. If we weren't able to get a
-        // repetition count before, we should have decoded the whole image by
-        // now, so it should now be available.
-        if (repetitionCount() != RepetitionCountInfinite && m_repetitionsComplete > repetitionCount()) {
-            m_animationFinished = true;
-            m_desiredFrameStartTime = 0;
-            --m_currentFrame;
-            advancedAnimation = false;
-        } else {
-            m_currentFrame = 0;
-            destroyAll = true;
-        }
-    }
-    destroyDecodedDataIfNecessary(destroyAll);
-
-    // We need to draw this frame if we advanced to it while not skipping, or if
-    // while trying to skip frames we hit the last frame and thus had to stop.
-    if (advancement == Normal && advancedAnimation)
-        imageObserver()->animationAdvanced(this);
-
-    return advancedAnimation;
 }
 
@@ -383 +289 @@
     stopAnimation();
     m_currentFrame = 0;
-    m_repetitionsComplete = 0;
+    m_repetitionsComplete = RepetitionCountNone;
     m_desiredFrameStartTime = 0;
     m_animationFinished = false;

trunk/Source/WebCore/platform/graphics/BitmapImage.h

@@ -153 +153 @@
     bool shouldAnimate();
     bool canAnimate();
-    void startAnimation(CatchUpAnimation = CatchUp) override;
+    void startAnimation() override;
     void advanceAnimation();
-
-    // Function that does the real work of advancing the animation. When
-    // skippingFrames is true, 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 each
-    // one or notify our observers.
-    // Returns whether the animation was advanced.
-    enum AnimationAdvancement { Normal, SkippingFramesToCatchUp };
-    bool internalAdvanceAnimation(AnimationAdvancement = Normal);
 
     // It may look unusual that there is no start animation call as public API. This is because
@@ -194 +186 @@
     double m_desiredFrameStartTime { 0 }; // The system time at which we hope to see the next call to startAnimation().
     bool m_animationFinished { false };
-    bool m_animationFinishedWhenCatchingUp { false };
 
 #if USE(APPKIT)

trunk/Source/WebCore/platform/graphics/Image.cpp

@@ -195 +195 @@
     FloatRect tileRect(FloatPoint(), intrinsicTileSize);
     drawPattern(ctxt, destRect, tileRect, patternTransform, oneTileRect.location(), spacing, op, blendMode);
-
-#if PLATFORM(IOS)
-    startAnimation(DoNotCatchUp);
-#else
     startAnimation();
-#endif
 }
 
@@ -280 +275 @@
     FloatPoint patternPhase(dstRect.x() - hPhase, dstRect.y() - vPhase);
     drawPattern(ctxt, dstRect, srcRect, patternTransform, patternPhase, spacing, op);
-
-#if PLATFORM(IOS)
-    startAnimation(DoNotCatchUp);
-#else
     startAnimation();
-#endif
 }
 

trunk/Source/WebCore/platform/graphics/Image.h

@@ -127 +127 @@
     // 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 { DoNotCatchUp, CatchUp };
-    virtual void startAnimation(CatchUpAnimation = CatchUp) { }
+    virtual void startAnimation() { }
     virtual void stopAnimation() {}
     virtual void resetAnimation() {}

trunk/Source/WebCore/svg/graphics/SVGImage.cpp

@@ -373 +373 @@
 }
 
-// FIXME: support catchUpIfNecessary.
-void SVGImage::startAnimation(CatchUpAnimation)
+void SVGImage::startAnimation()
 {
     SVGSVGElement* rootElement = this->rootElement();

trunk/Source/WebCore/svg/graphics/SVGImage.h

@@ -62 +62 @@
     bool hasRelativeHeight() const final;
 
-    void startAnimation(CatchUpAnimation = CatchUp) final;
+    void startAnimation() final;
     void stopAnimation() final;
     void resetAnimation() final;