[webkit-changes] [222427] trunk/Source

Sat, 23 Sep 2017 14:06:08 -0700

Title: [222427] trunk/Source
Revision
222427
Author
[email protected]
Date
2017-09-23 14:05:52 -0700 (Sat, 23 Sep 2017)

Log Message

Images may render partial frames even after loading all the encoded data
https://bugs.webkit.org/show_bug.cgi?id=177406

Patch by Said Abou-Hallawa <[email protected]> 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):

Modified Paths

Diff

Modified: trunk/Source/WTF/ChangeLog (222426 => 222427)


--- trunk/Source/WTF/ChangeLog	2017-09-23 19:17:32 UTC (rev 222426)
+++ trunk/Source/WTF/ChangeLog	2017-09-23 21:05:52 UTC (rev 222427)
@@ -1,3 +1,17 @@
+2017-09-23  Said Abou-Hallawa  <[email protected]>
+
+        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-22  Zalan Bujtas  <[email protected]>
 
         WeakPtrFactory should populate m_ref lazily.

Modified: trunk/Source/WTF/wtf/SynchronizedFixedQueue.h (222426 => 222427)


--- trunk/Source/WTF/wtf/SynchronizedFixedQueue.h	2017-09-23 19:17:32 UTC (rev 222426)
+++ trunk/Source/WTF/wtf/SynchronizedFixedQueue.h	2017-09-23 21:05:52 UTC (rev 222427)
@@ -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 @@
         m_open = true;
         m_queue.clear();
     }
-    
+
     void close()
     {
         LockHolder lockHolder(m_mutex);
@@ -60,7 +66,7 @@
         m_open = false;
         m_condition.notifyAll();
     }
-    
+
     bool isOpen()
     {
         LockHolder lockHolder(m_mutex);
@@ -73,11 +79,11 @@
 
         // 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);
 
@@ -85,11 +91,11 @@
         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(); });

Modified: trunk/Source/WebCore/ChangeLog (222426 => 222427)


--- trunk/Source/WebCore/ChangeLog	2017-09-23 19:17:32 UTC (rev 222426)
+++ trunk/Source/WebCore/ChangeLog	2017-09-23 21:05:52 UTC (rev 222427)
@@ -1,3 +1,32 @@
+2017-09-23  Said Abou-Hallawa  <[email protected]>
+
+        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  Zalan Bujtas  <[email protected]>
 
         WeakPtrFactory should populate m_ref lazily.

Modified: trunk/Source/WebCore/platform/graphics/ImageFrameCache.cpp (222426 => 222427)


--- trunk/Source/WebCore/platform/graphics/ImageFrameCache.cpp	2017-09-23 19:17:32 UTC (rev 222426)
+++ trunk/Source/WebCore/platform/graphics/ImageFrameCache.cpp	2017-09-23 21:05:52 UTC (rev 222427)
@@ -270,18 +270,24 @@
     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 @@
             }
 
             // 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 @@
     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 @@
         }
     });
 
-    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());

Modified: trunk/Source/WebCore/platform/graphics/ImageFrameCache.h (222426 => 222427)


--- trunk/Source/WebCore/platform/graphics/ImageFrameCache.h	2017-09-23 19:17:32 UTC (rev 222426)
+++ trunk/Source/WebCore/platform/graphics/ImageFrameCache.h	2017-09-23 21:05:52 UTC (rev 222427)
@@ -137,7 +137,10 @@
     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>& = { });
 
@@ -159,10 +162,9 @@
             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;
 

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to