Attachment 291529 Details for Bug 163410 – Patch

[patch] Patch

bug-163410-20161013153306.patch (text/plain), 16.73 KB, created by Said Abou-Hallawa on 2016-10-13 15:36:33 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Said Abou-Hallawa

Created: 2016-10-13 15:36:33 PDT

Size: 16.73 KB

patch

obsolete

>Subversion Revision: 207299
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0ce12de16ab41d0f39cb7e9ac4c28c49491ce124..e1b10148520846266bc32dfe5bdc1b0534b8f72c 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,57 @@
>+2016-10-13  Said Abou-Hallawa  <sabouhallawa@apple.com>
>+
>+        Delete the animated image catchup code
>+        https://bugs.webkit.org/show_bug.cgi?id=163410
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        To animate an image, BitmapImage::draw() sets a timer before it starts drawing
>+        When this timer fires, the current_frame is incremented and the image is
>+        invalidated. Ideally the timer should fire after the current_frame_duration.
>+        But because drawing the image happens on a different thread and because
>+        decoding the image frame might be slow, the elapsed time between the time
>+        when the timer fires and when BitmapImage::draw() gets called might be larger
>+        than the current_frame_duration. BitmapImage::startAnimation() has code to do
>+        catchup the current frame with the current time. But the code is unnecessary
>+        because decoding an out of order frame might be very costly. Besides the
>+        logic of this function does not handle the case of slow animation correctly.
>+        On iOS, the timer always fires after current_frame_duration regardless
>+        whether the current_time already passed the desired_next_frame_time or not.
>+
>+        Setting the animation timer should follow the following rules:
>+        
>+        1) Initial case: Which happens before drawing the first frame. We should
>+        set the timer to fire after the current_frame_duration.
>+        
>+        2) Late case (Slow animation): This happens if the current_time is already
>+        passed the desired_next_frame_time. In this case we should fire the timer
>+        immediately.
>+        
>+        3) Early case (Fast animation): This happens when there is still time before
>+        the desired_next_frame_time. In this case we should set the timer to fire
>+        after the difference between the desired_next_frame_time and the current_time.
>+
>+        * 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::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-13  Antoine Quint  <graouts@apple.com>
> 
>         [Modern Media Controls] MediaControls base class
>diff --git a/Source/WebCore/platform/graphics/BitmapImage.cpp b/Source/WebCore/platform/graphics/BitmapImage.cpp
>index 0c189c649c5472272163c688b7e60058a9b0123b..ad29ba72fcade37aefa4a3074263edd84711cd46 100644
>--- a/Source/WebCore/platform/graphics/BitmapImage.cpp
>+++ b/Source/WebCore/platform/graphics/BitmapImage.cpp
>@@ -145,11 +145,7 @@ void BitmapImage::draw(GraphicsContext& context, const FloatRect& destRect, cons
>     setRenderTarget(context);
> #endif
> 
>-#if PLATFORM(IOS)
>-    startAnimation(DoNotCatchUp);
>-#else
>     startAnimation();
>-#endif
> 
>     Color color = singlePixelSolidColor();
>     if (color.isValid()) {
>@@ -231,148 +227,61 @@ 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_desiredFrameStartTime = 0;
>+            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();
>+    float currentFrameDuration = frameDurationAtIndex(m_currentFrame);
> 
>-    // 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)
>+    if (!m_desiredFrameStartTime)
>+        m_desiredFrameStartTime = time + currentFrameDuration;
>+    else if (time > m_desiredFrameStartTime + currentFrameDuration)
>         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;
>-    }
>+    else
>+        m_desiredFrameStartTime += currentFrameDuration;
> 
>     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);
>+    ASSERT(m_desiredFrameStartTime - time >= 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;
>+    m_currentFrame = (m_currentFrame + 1) % frameCount();
>+    destroyDecodedDataIfNecessary(false);
> 
>-        // 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)
>+    if (imageObserver())
>         imageObserver()->animationAdvanced(this);
>-
>-    return advancedAnimation;
> }
> 
> void BitmapImage::stopAnimation()
>diff --git a/Source/WebCore/platform/graphics/BitmapImage.h b/Source/WebCore/platform/graphics/BitmapImage.h
>index 74c22a231b68bdc2a3b340474110c9367869b60d..f20b0c3f3783efd684f9ec7e64b6b1fac0ff690c 100644
>--- a/Source/WebCore/platform/graphics/BitmapImage.h
>+++ b/Source/WebCore/platform/graphics/BitmapImage.h
>@@ -155,17 +155,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.
>@@ -196,7 +188,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.
>diff --git a/Source/WebCore/platform/graphics/Image.cpp b/Source/WebCore/platform/graphics/Image.cpp
>index e1cd0b6950dd3a221ae5fc8606bca4269d0e3f6a..55079ade9d6ad074a09eecb9a3c81d9f17944bf3 100644
>--- a/Source/WebCore/platform/graphics/Image.cpp
>+++ b/Source/WebCore/platform/graphics/Image.cpp
>@@ -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)
>diff --git a/Source/WebCore/platform/graphics/Image.h b/Source/WebCore/platform/graphics/Image.h
>index a1d0952ba4fdbf19e477876e4277f939e7b727f8..e4903eb8ffaff5c3a1887b4021e74f0126d0041d 100644
>--- a/Source/WebCore/platform/graphics/Image.h
>+++ b/Source/WebCore/platform/graphics/Image.h
>@@ -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() {}
>     
>diff --git a/Source/WebCore/svg/graphics/SVGImage.cpp b/Source/WebCore/svg/graphics/SVGImage.cpp
>index a6a733f83cf589f90724dee599972b8ecd74cc13..e58dc7a73d039361eeb2891d8a3788de5a854092 100644
>--- a/Source/WebCore/svg/graphics/SVGImage.cpp
>+++ b/Source/WebCore/svg/graphics/SVGImage.cpp
>@@ -335,8 +335,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)
>diff --git a/Source/WebCore/svg/graphics/SVGImage.h b/Source/WebCore/svg/graphics/SVGImage.h
>index e32a4010b0e786a90c3aa3b5eaff53e50851b7f5..622edf80b37e663ab0bfb74e09fe45ab4d80c827 100644
>--- a/Source/WebCore/svg/graphics/SVGImage.h
>+++ b/Source/WebCore/svg/graphics/SVGImage.h
>@@ -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;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 163410: 291527 | 291529 | 291700 | 291724