Merge r222836 - [GTK][WPE] Fix playback of GIFs

https://bugs.webkit.org/show_bug.cgi?id=176089 Reviewed by Carlos Garcia Campos. Allow GIFImageReader to decode again already decoded frames. Thanks to this, we don't need to delete the GIFImageReader when GIFImageDecoder::clearFrameBufferCache() is called, and the we don't need the lock to avoid crashes in that situation. Covered by existent tests. * platform/image-decoders/gif/GIFImageDecoder.cpp: (WebCore::GIFImageDecoder::clearFrameBufferCache): (WebCore::GIFImageDecoder::decode): * platform/image-decoders/gif/GIFImageDecoder.h: * platform/image-decoders/gif/GIFImageReader.cpp: (GIFImageReader::decode):
WebKit · Oct 17, 2017 · b587b02 · b587b02
1 parent 765de56
commit b587b02
Show file tree

Hide file tree

Showing 4 changed files with 26 additions and 10 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,23 @@
+2017-10-04  Miguel Gomez  <magomez@igalia.com>
+
+        [GTK][WPE] Fix playback of GIFs
+        https://bugs.webkit.org/show_bug.cgi?id=176089
+
+        Reviewed by Carlos Garcia Campos.
+
+        Allow GIFImageReader to decode again already decoded frames. Thanks to this, we don't
+        need to delete the GIFImageReader when GIFImageDecoder::clearFrameBufferCache() is
+        called, and the we don't need the lock to avoid crashes in that situation.
+
+        Covered by existent tests.
+
+        * platform/image-decoders/gif/GIFImageDecoder.cpp:
+        (WebCore::GIFImageDecoder::clearFrameBufferCache):
+        (WebCore::GIFImageDecoder::decode):
+        * platform/image-decoders/gif/GIFImageDecoder.h:
+        * platform/image-decoders/gif/GIFImageReader.cpp:
+        (GIFImageReader::decode):
+
 2017-10-04  Joanmarie Diggs  <jdiggs@igalia.com>
 
         AX: [ATK] aria-pressed="mixed" should be exposed via ATK_STATE_INDETERMINATE

diff --git a/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp b/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp
@@ -125,10 +125,6 @@ void GIFImageDecoder::clearFrameBufferCache(size_t clearBeforeFrame)
     if (m_frameBufferCache.isEmpty())
         return; // Nothing to do.
 
-    // Lock the decodelock here, as we are going to destroy the GIFImageReader and doing so while
-    // there's an ongoing decode will cause a crash.
-    LockHolder locker(m_decodeLock);
-
     // The "-1" here is tricky.  It does not mean that |clearBeforeFrame| is the
     // last frame we wish to preserve, but rather that we never want to clear
     // the very last frame in the cache: it's empty (so clearing it is
@@ -170,10 +166,6 @@ void GIFImageDecoder::clearFrameBufferCache(size_t clearBeforeFrame)
         if (j->isInvalid())
             j->clear();
     }
-
-    // When some frames are cleared, the reader is out of sync, since the caller might ask for any frame not
-    // necessarily in the order expected by the reader. See https://bugs.webkit.org/show_bug.cgi?id=159089.
-    m_reader = nullptr;
 }
 
 bool GIFImageDecoder::haveDecodedRow(unsigned frameIndex, const Vector<unsigned char>& rowBuffer, size_t width, size_t rowNumber, unsigned repeatCount, bool writeTransparentPixels)
@@ -302,7 +294,6 @@ void GIFImageDecoder::decode(unsigned haltAtFrame, GIFQuery query, bool allDataR
     if (failed())
         return;
 
-    LockHolder locker(m_decodeLock);
     if (!m_reader) {
         m_reader = std::make_unique<GIFImageReader>(this);
         m_reader->setData(m_data.get());

diff --git a/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h b/Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h
@@ -80,7 +80,6 @@ namespace WebCore {
         bool m_currentBufferSawAlpha;
         mutable RepetitionCount m_repetitionCount { RepetitionCountOnce };
         std::unique_ptr<GIFImageReader> m_reader;
-        Lock m_decodeLock;
     };
 
 } // namespace WebCore
diff --git a/Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp b/Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp
@@ -363,6 +363,12 @@ bool GIFImageReader::decode(GIFImageDecoder::GIFQuery query, unsigned haltAtFram
     if (query != GIFImageDecoder::GIFFullQuery)
         return true;
 
+    // Already decoded frames can be deleted from the cache and then they require to be decoded again, so
+    // the haltAtFrame value we receive here may be lower than m_currentDecodingFrame. In this case
+    // we position m_currentDecodingFrame to haltAtFrame and decode from there.
+    // See bug https://bugs.webkit.org/show_bug.cgi?id=176089.
+    m_currentDecodingFrame = std::min(m_currentDecodingFrame, static_cast<size_t>(haltAtFrame));
+
     while (m_currentDecodingFrame < std::min(m_frames.size(), static_cast<size_t>(haltAtFrame))) {
         bool frameDecoded = false;
         GIFFrameContext* currentFrame = m_frames[m_currentDecodingFrame].get();