Merge r222427 - Images may render partial frames even after loading a…

…ll the encoded data https://bugs.webkit.org/show_bug.cgi?id=177406 Patch by Said Abou-Hallawa <sabouhallawa@apple.com> on 2017-09-23 Reviewed by Simon Fraser. Source/WebCore: Because we do not want to block the main thread waiting for the image decoding thread to terminate, we let the decoding thread finish its work even it will be thrown away. If a new decoding thread is created and the SynchronizedFixedQueue is reopened, the terminating decoding thread might have the chance to process a new frame request. After it finishes decoding it, it realize that it is terminating so it will drop the decoded frame to the floor. So the new request was not processed by the new thread and because it was processed by the terminating thread, nothing will be reported to the BitmapImage object and the renderer will not be repainted. The fix is to create a new SynchronizedFixedQueue every time a decoding thread is created. This will guarantee that the terminating thread won't have access to the new frame request and will shut down after being notified by the old SynchronizedFixedQueue that it has been closed. * platform/graphics/ImageFrameCache.cpp: (WebCore::ImageFrameCache::frameRequestQueue): (WebCore::ImageFrameCache::startAsyncDecodingQueue): (WebCore::ImageFrameCache::requestFrameAsyncDecodingAtIndex): (WebCore::ImageFrameCache::stopAsyncDecodingQueue): * platform/graphics/ImageFrameCache.h: Source/WTF: Make it possible to create a RefPtr<SynchronizedFixedQueue>. * wtf/SynchronizedFixedQueue.h: (WTF::SynchronizedFixedQueue::create): (WTF::SynchronizedFixedQueue::enqueue): (WTF::SynchronizedFixedQueue::dequeue):
WebKit · Oct 16, 2017 · ec125d1 · ec125d1
1 parent a1ff577
commit ec125d1
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 17 deletions.
diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
@@ -1,3 +1,17 @@
+2017-09-23  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Images may render partial frames even after loading all the encoded data
+        https://bugs.webkit.org/show_bug.cgi?id=177406
+
+        Reviewed by Simon Fraser.
+
+        Make it possible to create a RefPtr<SynchronizedFixedQueue>.
+
+        * wtf/SynchronizedFixedQueue.h:
+        (WTF::SynchronizedFixedQueue::create):
+        (WTF::SynchronizedFixedQueue::enqueue):
+        (WTF::SynchronizedFixedQueue::dequeue):
+
 2017-09-20  Alberto Garcia  <berto@igalia.com>
 
         Fix HPPA and Alpha builds

diff --git a/Source/WTF/wtf/SynchronizedFixedQueue.h b/Source/WTF/wtf/SynchronizedFixedQueue.h
@@ -28,17 +28,23 @@
 #include <wtf/Condition.h>
 #include <wtf/Deque.h>
 #include <wtf/Lock.h>
+#include <wtf/ThreadSafeRefCounted.h>
 
 namespace WTF {
 
 template<typename T, size_t BufferSize>
-class SynchronizedFixedQueue {
+class SynchronizedFixedQueue : public ThreadSafeRefCounted<SynchronizedFixedQueue<T, BufferSize>> {
 public:
+    static Ref<SynchronizedFixedQueue> create()
+    {
+        return adoptRef(*new SynchronizedFixedQueue());
+    }
+
     SynchronizedFixedQueue()
     {
         static_assert(!((BufferSize - 1) & BufferSize), "BufferSize must be power of 2.");
     }
-    
+
     void open()
     {
         LockHolder lockHolder(m_mutex);
@@ -49,7 +55,7 @@ class SynchronizedFixedQueue {
         m_open = true;
         m_queue.clear();
     }
-    
+
     void close()
     {
         LockHolder lockHolder(m_mutex);
@@ -60,7 +66,7 @@ class SynchronizedFixedQueue {
         m_open = false;
         m_condition.notifyAll();
     }
-    
+
     bool isOpen()
     {
         LockHolder lockHolder(m_mutex);
@@ -73,23 +79,23 @@ class SynchronizedFixedQueue {
 
         // Wait for an empty place to be available in the queue.
         m_condition.wait(m_mutex, [this]() { return !m_open || m_queue.size() < BufferSize; });
-        
+
         // The queue is closing, exit immediately.
         if (!m_open)
             return false;
-        
+
         // Add the item in the queue.
         m_queue.append(value);
 
         // Notify the other threads that an item was added to the queue.
         m_condition.notifyAll();
         return true;
     }
-    
+
     bool dequeue(T& value)
     {
         LockHolder lockHolder(m_mutex);
-        
+
         // Wait for an item to be added.
         m_condition.wait(m_mutex, [this]() { return !m_open || m_queue.size(); });
 

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,32 @@
+2017-09-23  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Images may render partial frames even after loading all the encoded data
+        https://bugs.webkit.org/show_bug.cgi?id=177406
+
+        Reviewed by Simon Fraser.
+
+        Because we do not want to block the main thread waiting for the image decoding
+        thread to terminate, we let the decoding thread finish its work even it will
+        be thrown away. If a new decoding thread is created and the SynchronizedFixedQueue
+        is reopened, the terminating decoding thread might have the chance to process
+        a new frame request. After it finishes decoding it, it realize that it is
+        terminating so it will drop the decoded frame to the floor. So the new request
+        was not processed by the new thread and because it was processed by the
+        terminating thread, nothing will be reported to the BitmapImage object and
+        the renderer will not be repainted.
+
+        The fix is to create a new SynchronizedFixedQueue every time a decoding 
+        thread is created. This will guarantee that the terminating thread won't
+        have access to the new frame request and will shut down after being notified
+        by the old SynchronizedFixedQueue that it has been closed.
+
+        * platform/graphics/ImageFrameCache.cpp:
+        (WebCore::ImageFrameCache::frameRequestQueue):
+        (WebCore::ImageFrameCache::startAsyncDecodingQueue):
+        (WebCore::ImageFrameCache::requestFrameAsyncDecodingAtIndex):
+        (WebCore::ImageFrameCache::stopAsyncDecodingQueue):
+        * platform/graphics/ImageFrameCache.h:
+
 2017-09-22  Nael Ouedraogo  <nael.ouedraogo@crf.canon.fr>
 
         [GTK] HTMLMediaElement resize event not fired when video size changes

diff --git a/Source/WebCore/platform/graphics/ImageFrameCache.cpp b/Source/WebCore/platform/graphics/ImageFrameCache.cpp
@@ -270,18 +270,24 @@ Ref<WorkQueue> ImageFrameCache::decodingQueue()
     return *m_decodingQueue;
 }
 
+Ref<ImageFrameCache::FrameRequestQueue> ImageFrameCache::frameRequestQueue()
+{
+    if (!m_frameRequestQueue)
+        m_frameRequestQueue = FrameRequestQueue::create();
+
+    return *m_frameRequestQueue;
+}
+
 void ImageFrameCache::startAsyncDecodingQueue()
 {
     if (hasAsyncDecodingQueue() || !isDecoderAvailable())
         return;
 
-    m_frameRequestQueue.open();
-
     // We need to protect this, m_decodingQueue and m_decoder from being deleted while we are in the decoding loop.
-    decodingQueue()->dispatch([protectedThis = makeRef(*this), protectedQueue = decodingQueue(), protectedDecoder = makeRef(*m_decoder), sourceURL = sourceURL().string().isolatedCopy()] {
+    decodingQueue()->dispatch([protectedThis = makeRef(*this), protectedDecodingQueue = decodingQueue(), protectedFrameRequestQueue = frameRequestQueue(), protectedDecoder = makeRef(*m_decoder), sourceURL = sourceURL().string().isolatedCopy()] {
         ImageFrameRequest frameRequest;
 
-        while (protectedThis->m_frameRequestQueue.dequeue(frameRequest)) {
+        while (protectedFrameRequestQueue->dequeue(frameRequest)) {
             TraceScope tracingScope(AsyncImageDecodeStart, AsyncImageDecodeEnd);
 
             // Get the frame NativeImage on the decoding thread.
@@ -294,7 +300,7 @@ void ImageFrameCache::startAsyncDecodingQueue()
             }
 
             // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
-            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), sourceURL = sourceURL.isolatedCopy(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
+            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedDecodingQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), sourceURL = sourceURL.isolatedCopy(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
                 // The queue may have been closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called.
                 if (protectedQueue.ptr() == protectedThis->m_decodingQueue && protectedDecoder.ptr() == protectedThis->m_decoder) {
                     ASSERT(protectedThis->m_frameCommitQueue.first() == frameRequest);
@@ -317,7 +323,7 @@ void ImageFrameCache::requestFrameAsyncDecodingAtIndex(size_t index, Subsampling
     DecodingStatus decodingStatus = m_decoder->frameIsCompleteAtIndex(index) ? DecodingStatus::Complete : DecodingStatus::Partial;
 
     LOG(Images, "ImageFrameCache::%s - %p - url: %s [enqueuing frame %ld for decoding]", __FUNCTION__, this, sourceURL().string().utf8().data(), index);
-    m_frameRequestQueue.enqueue({ index, subsamplingLevel, sizeForDrawing, decodingStatus });
+    m_frameRequestQueue->enqueue({ index, subsamplingLevel, sizeForDrawing, decodingStatus });
     m_frameCommitQueue.append({ index, subsamplingLevel, sizeForDrawing, decodingStatus });
 }
 
@@ -339,7 +345,10 @@ void ImageFrameCache::stopAsyncDecodingQueue()
         }
     });
 
-    m_frameRequestQueue.close();
+    // Close m_frameRequestQueue then set it to nullptr. A new decoding thread might start and a
+    // new m_frameRequestQueue will be created. So the terminating thread will not have access to it.
+    m_frameRequestQueue->close();
+    m_frameRequestQueue = nullptr;
     m_frameCommitQueue.clear();
     m_decodingQueue = nullptr;
     LOG(Images, "ImageFrameCache::%s - %p - url: %s [decoding has been stopped]", __FUNCTION__, this, sourceURL().string().utf8().data());

diff --git a/Source/WebCore/platform/graphics/ImageFrameCache.h b/Source/WebCore/platform/graphics/ImageFrameCache.h
@@ -135,7 +135,10 @@ class ImageFrameCache : public ThreadSafeRefCounted<ImageFrameCache> {
     void cacheNativeImageAtIndex(NativeImagePtr&&, size_t, SubsamplingLevel, const DecodingOptions&, DecodingStatus = DecodingStatus::Invalid);
     void cacheNativeImageAtIndexAsync(NativeImagePtr&&, size_t, SubsamplingLevel, const DecodingOptions&, DecodingStatus);
 
+    struct ImageFrameRequest;
+    static const int BufferSize = 8;
     Ref<WorkQueue> decodingQueue();
+    Ref<SynchronizedFixedQueue<ImageFrameRequest, BufferSize>> frameRequestQueue();
 
     const ImageFrame& frameAtIndexCacheIfNeeded(size_t, ImageFrame::Caching, const std::optional<SubsamplingLevel>& = { });
 
@@ -157,10 +160,9 @@ class ImageFrameCache : public ThreadSafeRefCounted<ImageFrameCache> {
             return index == other.index && subsamplingLevel == other.subsamplingLevel && decodingOptions == other.decodingOptions && decodingStatus == other.decodingStatus;
         }
     };
-    static const int BufferSize = 8;
     using FrameRequestQueue = SynchronizedFixedQueue<ImageFrameRequest, BufferSize>;
     using FrameCommitQueue = Deque<ImageFrameRequest, BufferSize>;
-    FrameRequestQueue m_frameRequestQueue;
+    RefPtr<FrameRequestQueue> m_frameRequestQueue;
     FrameCommitQueue m_frameCommitQueue;
     RefPtr<WorkQueue> m_decodingQueue;