Delete the animated image catchup code
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 16 Oct 2016 00:19:22 +0000 (00:19 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 16 Oct 2016 00:19:22 +0000 (00:19 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@207386 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/images/ordered-animated-image-frames-expected.html [new file with mode: 0644]
LayoutTests/fast/images/ordered-animated-image-frames.html [new file with mode: 0644]
LayoutTests/fast/images/resources/animated-red-green-blue.gif [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/BitmapImage.cpp
Source/WebCore/platform/graphics/BitmapImage.h
Source/WebCore/platform/graphics/Image.cpp
Source/WebCore/platform/graphics/Image.h
Source/WebCore/svg/graphics/SVGImage.cpp
Source/WebCore/svg/graphics/SVGImage.h

index 813b4b7..cab7c74 100644 (file)
@@ -1,3 +1,19 @@
+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>
 
         Test that duplicate axis values in font-variation-settings are resolved correctly
diff --git a/LayoutTests/fast/images/ordered-animated-image-frames-expected.html b/LayoutTests/fast/images/ordered-animated-image-frames-expected.html
new file mode 100644 (file)
index 0000000..062530b
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html>
+<style>
+    div {
+        width: 100px;
+        height: 100px;
+        background-color: green;
+    } 
+</style>    
+<body>
+    <div></div>
+</body>
+</html>
diff --git a/LayoutTests/fast/images/ordered-animated-image-frames.html b/LayoutTests/fast/images/ordered-animated-image-frames.html
new file mode 100644 (file)
index 0000000..d685417
--- /dev/null
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<html>
+<style>
+    canvas {
+        width: 100px;
+        height: 100px;
+    }
+</style>    
+<body>
+    <canvas id="canvas"></canvas>
+    <script>
+        if (window.testRunner)
+            testRunner.waitUntilDone();
+
+        var image = new Image;
+        image.onload = draw;
+        image.src = "resources/animated-red-green-blue.gif";
+        var drawCount = 0;
+
+        function draw()
+        {   
+            var canvas = document.getElementById("canvas");
+            var ctx = canvas.getContext("2d");
+
+            setTimeout(function() {
+                ctx.drawImage(image, 0, 0, canvas.width, canvas.height);
+                if (++drawCount == 2)
+                    testRunner.notifyDone();
+                else
+                    draw();
+            }, 100);
+        }
+    </script>
+</body>
+</html>
diff --git a/LayoutTests/fast/images/resources/animated-red-green-blue.gif b/LayoutTests/fast/images/resources/animated-red-green-blue.gif
new file mode 100644 (file)
index 0000000..aa31fc3
Binary files /dev/null and b/LayoutTests/fast/images/resources/animated-red-green-blue.gif differ
index 6f73ba7..e949fa2 100644 (file)
@@ -1,3 +1,60 @@
+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>
 
         Move Web SQL database and WebSockets off legacy exceptions
index c65f7cb..1d0b492 100644 (file)
@@ -141,11 +141,7 @@ void BitmapImage::draw(GraphicsContext& context, const FloatRect& destRect, cons
     if (destRect.isEmpty() || srcRect.isEmpty())
         return;
 
-#if PLATFORM(IOS)
-    startAnimation(DoNotCatchUp);
-#else
     startAnimation();
-#endif
 
     Color color = singlePixelSolidColor();
     if (color.isValid()) {
@@ -227,148 +223,58 @@ void BitmapImage::startTimer(double delay)
     m_frameTimer->startOneShot(delay);
 }
 
-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.
     size_t nextFrame = (m_currentFrame + 1) % frameCount();
     if (!m_source.isAllDataReceived() && !frameIsCompleteAtIndex(nextFrame))
         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
+    double time = monotonicallyIncreasingTime();
 
-    // 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)
+    // 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) {
-        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);
+    m_currentFrame = (m_currentFrame + 1) % frameCount();
+    destroyDecodedDataIfNecessary(false);
 
-    // 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)
+    if (imageObserver())
         imageObserver()->animationAdvanced(this);
-
-    return advancedAnimation;
 }
 
 void BitmapImage::stopAnimation()
@@ -382,7 +288,7 @@ void BitmapImage::resetAnimation()
 {
     stopAnimation();
     m_currentFrame = 0;
-    m_repetitionsComplete = 0;
+    m_repetitionsComplete = RepetitionCountNone;
     m_desiredFrameStartTime = 0;
     m_animationFinished = false;
 
index 092ae63..d0182a7 100644 (file)
@@ -152,17 +152,9 @@ protected:
     bool isAnimated() const override { return m_source.frameCount() > 1; }
     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
     // we start and stop animating lazily. Animation begins whenever someone draws the image. It will
     // automatically pause once all observers no longer want to render the image anywhere.
@@ -193,7 +185,6 @@ private:
     RepetitionCount m_repetitionsComplete { RepetitionCountNone }; // How many repetitions we've finished.
     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)
     mutable RetainPtr<NSImage> m_nsImage; // A cached NSImage of all the frames. Only built lazily if someone actually queries for one.
index e1cd0b6..55079ad 100644 (file)
@@ -194,12 +194,7 @@ void Image::drawTiled(GraphicsContext& ctxt, const FloatRect& destRect, const Fl
     AffineTransform patternTransform = AffineTransform().scaleNonUniform(scale.width(), scale.height());
     FloatRect tileRect(FloatPoint(), intrinsicTileSize);
     drawPattern(ctxt, destRect, tileRect, patternTransform, oneTileRect.location(), spacing, op, blendMode);
-
-#if PLATFORM(IOS)
-    startAnimation(DoNotCatchUp);
-#else
     startAnimation();
-#endif
 }
 
 // FIXME: Merge with the other drawTiled eventually, since we need a combination of both for some things.
@@ -279,12 +274,7 @@ void Image::drawTiled(GraphicsContext& ctxt, const FloatRect& dstRect, const Flo
 
     FloatPoint patternPhase(dstRect.x() - hPhase, dstRect.y() - vPhase);
     drawPattern(ctxt, dstRect, srcRect, patternTransform, patternPhase, spacing, op);
-
-#if PLATFORM(IOS)
-    startAnimation(DoNotCatchUp);
-#else
     startAnimation();
-#endif
 }
 
 #if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
index 915018f..cc182cd 100644 (file)
@@ -126,8 +126,7 @@ public:
 
     // 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() {}
     
index c5a4ba0..ea951bd 100644 (file)
@@ -372,8 +372,7 @@ void SVGImage::computeIntrinsicDimensions(Length& intrinsicWidth, Length& intrin
         intrinsicRatio = FloatSize(floatValueForLength(intrinsicWidth, 0), floatValueForLength(intrinsicHeight, 0));
 }
 
-// FIXME: support catchUpIfNecessary.
-void SVGImage::startAnimation(CatchUpAnimation)
+void SVGImage::startAnimation()
 {
     SVGSVGElement* rootElement = this->rootElement();
     if (!rootElement)
index 80bb9df..68bd232 100644 (file)
@@ -61,7 +61,7 @@ public:
     bool hasRelativeWidth() const final;
     bool hasRelativeHeight() const final;
 
-    void startAnimation(CatchUpAnimation = CatchUp) final;
+    void startAnimation() final;
     void stopAnimation() final;
     void resetAnimation() final;