Decode GIF image frames on demand.

Source/core/platform/image-decoders/ImageDecoder.cpp

Index: Source/core/platform/image-decoders/ImageDecoder.cpp
diff --git a/Source/core/platform/image-decoders/ImageDecoder.cpp b/Source/core/platform/image-decoders/ImageDecoder.cpp
index 37cd9327dd39379d68debb4ab690c28d772a5dd3..bd987851b2727084aa78fddc572c4fb5a14acf97 100644
--- a/Source/core/platform/image-decoders/ImageDecoder.cpp
+++ b/Source/core/platform/image-decoders/ImageDecoder.cpp
@@ -227,4 +227,85 @@ int ImageDecoder::scaledY(int origY, int searchStart)
     return getScaledValue<Exact>(m_scaledRows, origY, searchStart);
 }
 
+void ImageDecoder::clearFrameBufferCache(size_t clearExceptFrame)
+{
+    // Don't clear if there is no or only one frame.
+    if (m_frameBufferCache.size() <= 1)
+        return;
+
+    // We need to preserve frames such that:
+    //  1. We don't clear |clearExceptFrame|;
+    //  2. We don't clear any frame from which a future initFrameBuffer() call
+    //     will copy bitmap data;
+    //  3. We don't clear empty frames.

[Peter Kasting, 2013/05/24 03:15:22]

Why not clear empty frames?  Shouldn't that just be a no-op?  It seems like we
don't need to special-case those.

[Xianzhu, 2013/05/28 22:54:21]

Done.

+    // All other frames can be cleared.
+    size_t frameToBeRequired;

[Peter Kasting, 2013/05/24 03:15:22]

Nit: Init this to notFound; this allows eliminating the "else" below and is
safer in case someone later changes this code and accidentally adds an
uninitialized-variable codepath.

[Xianzhu, 2013/05/28 22:54:21]

Done.

+    if (clearExceptFrame < m_frameBufferCache.size() && m_frameBufferCache[clearExceptFrame].status() == ImageFrame::FrameEmpty) {
+        frameToBeRequired = clearExceptFrame;
+        do {
+            frameToBeRequired = m_frameBufferCache[frameToBeRequired].requiredPreviousFrameIndex();
+        } while (frameToBeRequired != notFound && m_frameBufferCache[frameToBeRequired].status() == ImageFrame::FrameEmpty);

[Peter Kasting, 2013/05/24 03:15:22]

This loop tries to preserve the frame we'll need to start from if we later
decode frames >= clearExceptFrame.  But if we later ask for a frame <
clearExceptFrame, we may need to redecode the whole image to that point.

It's probably worth updating the comments on ImageSource::clear() to say that
the intent of the function is to throw away as much data as possible given an
expectation that you're next going to ask for a frame on/after the provided
frame index, and that if callers violate this by clearing and then requesting a
frame before the provided frame number, the decoder may need to redecode the
image from the beginning.

[Xianzhu, 2013/05/28 22:54:21]

Done.

+    } else {
+        // initFrameBuffer() has already been called for this frame.
+        frameToBeRequired = notFound;
+    }
+
+    for (size_t i = 0; i < m_frameBufferCache.size(); ++i) {
+        if (i != clearExceptFrame && i != frameToBeRequired && m_frameBufferCache[i].status() != ImageFrame::FrameEmpty)
+            clearFrameBuffer(i);
+    }
+}
+
+void ImageDecoder::clearFrameBuffer(size_t frameIndex)
+{
+    m_frameBufferCache[frameIndex].clearPixelData();
+}
+
+size_t ImageDecoder::findRequiredPreviousFrame(size_t frameIndex)

[Peter Kasting, 2013/05/24 03:15:22]

By the way, the algorithm in this function can be simpler and faster if we know
that all frames before frameIndex have already had m_requiredPreviousFrameIndex
set -- because we just need to look at the previous frame's disposal method and
required previous frame, we don't have to loop through other frames.

You may want to ensure that invariant holds and then change to the simpler
algorithm.

+{
+    if (!frameIndex) {
+        // The first frame doesn't rely on any previous data.
+        return notFound;
+    }
+
+    // The starting state for this frame depends on the previous frame's
+    // disposal method.
+    //
+    // Frames that use the DisposeOverwritePrevious method are effectively
+    // no-ops in terms of changing the starting state of a frame compared to
+    // the starting state of the previous frame, so skip over them.  (If the
+    // first frame specifies this method, it will get treated like
+    // DisposeOverwriteBgcolor below and reset to a completely empty image.)
+    size_t prevFrame = frameIndex - 1;
+    const ImageFrame* prevBuffer = &m_frameBufferCache[prevFrame];

[Peter Kasting, 2013/05/24 03:15:22]

If we're going to do this we should either check or ASSERT that frameIndex() is
<= m_frameBufferCache.size().

[Xianzhu, 2013/05/28 22:54:21]

Done.

+    ImageFrame::FrameDisposalMethod prevMethod = prevBuffer->disposalMethod();
+    while (prevFrame && (prevMethod == ImageFrame::DisposeOverwritePrevious)) {
+        prevBuffer = &m_frameBufferCache[--prevFrame];
+        prevMethod = prevBuffer->disposalMethod();
+    }
+
+    if (prevMethod == ImageFrame::DisposeNotSpecified || prevMethod == ImageFrame::DisposeKeep) {
+        // prevFrame will be used as the starting state for this frame.

[Peter Kasting, 2013/05/24 03:15:22]

Nit: Might want to add a note or FIXME about being even smarter by checking the
frame sizes and/or alpha-containing regions.

[Xianzhu, 2013/05/28 22:54:21]

Done.

+        return prevFrame;
+    }
+
+    if (!prevFrame) {
+        // The first frame whose disposal method is DisposeOverwriteBgColor or
+        // DisposeOverritePrevious will be reset to background despite its size.

[Peter Kasting, 2013/05/24 03:15:22]

Nit: Comment could be clearer; how about:

This frame's disposal method is DisposeOverwriteBgColor or
DisposeOverwritePrevious.  If this is the first frame, the starting state for
the next frame is guaranteed to be just an empty frame.

(I would also place this comment above the conditional instead of inside it.)

[Xianzhu, 2013/05/28 22:54:21]

Done.

+        return notFound;
+    }
+
+    ASSERT(prevMethod == ImageFrame::DisposeOverwriteBgcolor);
+    const IntRect& prevRect = prevBuffer->originalFrameRect();
+    const IntSize& bufferSize = scaledSize();

[Peter Kasting, 2013/05/24 03:15:22]

I think scaledSize() got nuked (or is getting nuked) in a separate CL.

Also, you don't actually use this temp.

[Xianzhu, 2013/05/28 22:54:21]

Done.

+    if (prevRect.contains(IntRect(IntPoint(), scaledSize()))) {
+        // prevFrame covering the whole image will be reset to background,

[Peter Kasting, 2013/05/24 03:15:22]

Nit: A slightly shorter way of doing this:

    // If prevRect fills the whole image, then the current frame can be decoded
    // alone.  Otherwise, prevFrame contributes to this frame.
    return prevRect.contains(IntRect(IntPoint(), size())) ? notFound :
prevFrame;

[Xianzhu, 2013/05/28 22:54:21]

Done.

+        return notFound;
+    }
+
+    // prevFrame only clears part of the image to background, so it contributes
+    // to the starting state of this frame.
+    return prevFrame;
+}
+
 } // namespace WebCore