GIF Animations "clear from cache related glitch" fix

third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
index cc5954edf9de97a6391a4c904dc4826cc341741d..e77a70c851e906cdeb37eda10611e915fb670830 100644
--- a/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -222,13 +222,24 @@ size_t GIFImageDecoder::clearCacheExceptFrame(size_t clearExceptFrame)
 {
     // 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
+    //  2. We don't clear any frame from which a two* future initFrameBuffer() calls

[tomhudson, 2016/01/12 16:15:59]

Nit: 'which a two' -> 'which two', although this could use a broader rewriting

[aleksandar.stojiljkovic, 2016/01/12 17:34:59]

Done.

     //     will copy bitmap data.
     // All other frames can be cleared.
-    while ((clearExceptFrame < m_frameBufferCache.size()) && (m_frameBufferCache[clearExceptFrame].status() == ImageFrame::FrameEmpty))
-        clearExceptFrame = m_frameBufferCache[clearExceptFrame].requiredPreviousFrameIndex();
-
-    return ImageDecoder::clearCacheExceptFrame(clearExceptFrame);
+    // *) Two frames range is chosen as dependent animated GIF frames usually depend
+    // on the previous or the frame before previous.

[urvang, 2016/01/12 18:02:01]

As an extreme example, what if all frames in a GIF use RESTORE_TO_PREVIOUS
disposal method? In that case, any frame in GIF would depend on the 1st frame
due to the dependency chain (and not on the previous two frames).

What I'm trying to say is that, the number 'two' seems kind of arbitrary. In
future, if you see another GIF example that depends on 3rd frame before that,
would you increase this number to 3 then?

[aleksandar.stojiljkovic, 2016/01/12 18:19:14]

Theoretically, you're right.
While exploring GIF related issues reported and checking various GIFs details, I
didn't see this case (except in our unit tests).
Depends on frequency and severity.
In performance issues related to GIF animations and those I checked while doing
GIF Index8 decoding, dependency to frame-1 or frame-2 is much more common.
Case of dependency to frame-3 this patch doesn't change - performance after this
patch is the same as it was.

+    if (clearExceptFrame == kNotFound)
+        return ImageDecoder::clearCacheExceptFrame(clearExceptFrame);
+    SizeTHashSet clearExceptFrames;
+    for (int i = 0; i < 2; ++i, ++clearExceptFrame) {
+        size_t leave = clearExceptFrame;
+        while ((leave < m_frameBufferCache.size()) && (m_frameBufferCache[leave].status() == ImageFrame::FrameEmpty))
+            leave = m_frameBufferCache[leave].requiredPreviousFrameIndex();
+        if (leave != kNotFound)
+            clearExceptFrames.add(leave);
+    }
+    if (clearExceptFrames.size() > 1)
+        return clearCacheExceptFrames(clearExceptFrames);
+    return ImageDecoder::clearCacheExceptFrame(clearExceptFrames.size() ? *(clearExceptFrames.begin()) : kNotFound);

[tomhudson, 2016/01/12 16:15:59]

These three lines are significantly more complicated to understand than just
calling
  return clearCacheExceptFrame(clearExceptFrames).

Is there really that huge a performance difference? Or does this point to the
"if (m_frameBufferCache.size() <= 1) return 0;" clause possibly being a poor
design?

[aleksandar.stojiljkovic, 2016/01/12 17:34:59]

Acknowledged.

[aleksandar.stojiljkovic, 2016/01/12 18:54:02]

Fixed this in new patch.
Thanks for pointing - measurements on K3 Note show that clearCacheExceptFrame
(existing code and clearCacheExceptFrames for one item in hashset) takes ~0.3ms
in average for 100 frames animation (screen size 512*384). Need to check it
further and submit a bug.

 }
 
 void GIFImageDecoder::clearFrameBuffer(size_t frameIndex)