GIF Animations "clear from cache related glitch" fix

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

 /*
  * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 204 matching lines @@
         }
     }
 
     return true;
 }
 
 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)
 {
     if (m_reader && m_frameBufferCache[frameIndex].status() == ImageFrame::FramePartial) {
         // Reset the state of the partial frame in the reader so that the frame
         // can be decoded again when requested.
         m_reader->clearDecodeState(frameIndex);
     }
     ImageDecoder::clearFrameBuffer(frameIndex);
@@ skipping 93 matching lines @@
 
     // Update our status to be partially complete.
     buffer->setStatus(ImageFrame::FramePartial);
 
     // Reset the alpha pixel tracker for this frame.
     m_currentBufferSawAlpha = false;
     return true;
 }
 
 } // namespace blink