Changeset 222427 in webkit


Ignore:
Timestamp:
Sep 23, 2017 2:05:52 PM (7 years ago)
Author:
commit-queue@webkit.org
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 <​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):

Location:
trunk/Source
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r222422 r222427  
     12017-09-23  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        Images may render partial frames even after loading all the encoded data
     4        https://bugs.webkit.org/show_bug.cgi?id=177406
     5
     6        Reviewed by Simon Fraser.
     7
     8        Make it possible to create a RefPtr<SynchronizedFixedQueue>.
     9
     10        * wtf/SynchronizedFixedQueue.h:
     11        (WTF::SynchronizedFixedQueue::create):
     12        (WTF::SynchronizedFixedQueue::enqueue):
     13        (WTF::SynchronizedFixedQueue::dequeue):
     14
    1152017-09-22  Zalan Bujtas  <zalan@apple.com>
    216

  • trunk/Source/WTF/wtf/SynchronizedFixedQueue.h

    r218594 r222427  
    2929#include <wtf/Deque.h>
    3030#include <wtf/Lock.h>
     31#include <wtf/ThreadSafeRefCounted.h>
    3132
    3233namespace WTF {
    3334
    3435template<typename T, size_t BufferSize>
    35 class SynchronizedFixedQueue {
     36class SynchronizedFixedQueue : public ThreadSafeRefCounted<SynchronizedFixedQueue<T, BufferSize>> {
    3637public:
     38    static Ref<SynchronizedFixedQueue> create()
     39    {
     40        return adoptRef(*new SynchronizedFixedQueue());
     41    }
     42
    3743    SynchronizedFixedQueue()
    3844    {
    3945        static_assert(!((BufferSize - 1) & BufferSize), "BufferSize must be power of 2.");
    4046    }
    41    
     47
    4248    void open()
    4349    {
    … …  
    5056        m_queue.clear();
    5157    }
    52    
     58
    5359    void close()
    5460    {
    … …  
    6167        m_condition.notifyAll();
    6268    }
    63    
     69
    6470    bool isOpen()
    6571    {
    … …  
    7480        // Wait for an empty place to be available in the queue.
    7581        m_condition.wait(m_mutex, [this]() { return !m_open || m_queue.size() < BufferSize; });
    76        
     82
    7783        // The queue is closing, exit immediately.
    7884        if (!m_open)
    7985            return false;
    80        
     86
    8187        // Add the item in the queue.
    8288        m_queue.append(value);
    … …  
    8692        return true;
    8793    }
    88    
     94
    8995    bool dequeue(T& value)
    9096    {
    9197        LockHolder lockHolder(m_mutex);
    92        
     98
    9399        // Wait for an item to be added.
    94100        m_condition.wait(m_mutex, [this]() { return !m_open || m_queue.size(); });

  • trunk/Source/WebCore/ChangeLog

    r222422 r222427  
     12017-09-23  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        Images may render partial frames even after loading all the encoded data
     4        https://bugs.webkit.org/show_bug.cgi?id=177406
     5
     6        Reviewed by Simon Fraser.
     7
     8        Because we do not want to block the main thread waiting for the image decoding
     9        thread to terminate, we let the decoding thread finish its work even it will
     10        be thrown away. If a new decoding thread is created and the SynchronizedFixedQueue
     11        is reopened, the terminating decoding thread might have the chance to process
     12        a new frame request. After it finishes decoding it, it realize that it is
     13        terminating so it will drop the decoded frame to the floor. So the new request
     14        was not processed by the new thread and because it was processed by the
     15        terminating thread, nothing will be reported to the BitmapImage object and
     16        the renderer will not be repainted.
     17
     18        The fix is to create a new SynchronizedFixedQueue every time a decoding
     19        thread is created. This will guarantee that the terminating thread won't
     20        have access to the new frame request and will shut down after being notified
     21        by the old SynchronizedFixedQueue that it has been closed.
     22
     23        * platform/graphics/ImageFrameCache.cpp:
     24        (WebCore::ImageFrameCache::frameRequestQueue):
     25        (WebCore::ImageFrameCache::startAsyncDecodingQueue):
     26        (WebCore::ImageFrameCache::requestFrameAsyncDecodingAtIndex):
     27        (WebCore::ImageFrameCache::stopAsyncDecodingQueue):
     28        * platform/graphics/ImageFrameCache.h:
     29
    1302017-09-22  Zalan Bujtas  <zalan@apple.com>
    231

  • trunk/Source/WebCore/platform/graphics/ImageFrameCache.cpp

    r222225 r222427  
    271271}
    272272
     273Ref<ImageFrameCache::FrameRequestQueue> ImageFrameCache::frameRequestQueue()
     274{
     275    if (!m_frameRequestQueue)
     276        m_frameRequestQueue = FrameRequestQueue::create();
     277   
     278    return *m_frameRequestQueue;
     279}
     280
    273281void ImageFrameCache::startAsyncDecodingQueue()
    274282{
    … …  
    276284        return;
    277285
    278     m_frameRequestQueue.open();
    279 
    280286    // We need to protect this, m_decodingQueue and m_decoder from being deleted while we are in the decoding loop.
    281     decodingQueue()->dispatch([protectedThis = makeRef(*this), protectedQueue = decodingQueue(), protectedDecoder = makeRef(*m_decoder), sourceURL = sourceURL().string().isolatedCopy()] {
     287    decodingQueue()->dispatch([protectedThis = makeRef(*this), protectedDecodingQueue = decodingQueue(), protectedFrameRequestQueue = frameRequestQueue(), protectedDecoder = makeRef(*m_decoder), sourceURL = sourceURL().string().isolatedCopy()] {
    282288        ImageFrameRequest frameRequest;
    283289
    284         while (protectedThis->m_frameRequestQueue.dequeue(frameRequest)) {
     290        while (protectedFrameRequestQueue->dequeue(frameRequest)) {
    285291            TraceScope tracingScope(AsyncImageDecodeStart, AsyncImageDecodeEnd);
    286292
    … …  
    295301
    296302            // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
    297             callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), sourceURL = sourceURL.isolatedCopy(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
     303            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedDecodingQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), sourceURL = sourceURL.isolatedCopy(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
    298304                // The queue may have been closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called.
    299305                if (protectedQueue.ptr() == protectedThis->m_decodingQueue && protectedDecoder.ptr() == protectedThis->m_decoder) {
    … …  
    318324
    319325    LOG(Images, "ImageFrameCache::%s - %p - url: %s [enqueuing frame %ld for decoding]", __FUNCTION__, this, sourceURL().string().utf8().data(), index);
    320     m_frameRequestQueue.enqueue({ index, subsamplingLevel, sizeForDrawing, decodingStatus });
     326    m_frameRequestQueue->enqueue({ index, subsamplingLevel, sizeForDrawing, decodingStatus });
    321327    m_frameCommitQueue.append({ index, subsamplingLevel, sizeForDrawing, decodingStatus });
    322328}
    … …  
    340346    });
    341347
    342     m_frameRequestQueue.close();
     348    // Close m_frameRequestQueue then set it to nullptr. A new decoding thread might start and a
     349    // new m_frameRequestQueue will be created. So the terminating thread will not have access to it.
     350    m_frameRequestQueue->close();
     351    m_frameRequestQueue = nullptr;
    343352    m_frameCommitQueue.clear();
    344353    m_decodingQueue = nullptr;

  • trunk/Source/WebCore/platform/graphics/ImageFrameCache.h

    r222225 r222427  
    138138    void cacheNativeImageAtIndexAsync(NativeImagePtr&&, size_t, SubsamplingLevel, const DecodingOptions&, DecodingStatus);
    139139
     140    struct ImageFrameRequest;
     141    static const int BufferSize = 8;
    140142    Ref<WorkQueue> decodingQueue();
     143    Ref<SynchronizedFixedQueue<ImageFrameRequest, BufferSize>> frameRequestQueue();
    141144
    142145    const ImageFrame& frameAtIndexCacheIfNeeded(size_t, ImageFrame::Caching, const std::optional<SubsamplingLevel>& = { });
    … …  
    160163        }
    161164    };
    162     static const int BufferSize = 8;
    163165    using FrameRequestQueue = SynchronizedFixedQueue<ImageFrameRequest, BufferSize>;
    164166    using FrameCommitQueue = Deque<ImageFrameRequest, BufferSize>;
    165     FrameRequestQueue m_frameRequestQueue;
     167    RefPtr<FrameRequestQueue> m_frameRequestQueue;
    166168    FrameCommitQueue m_frameCommitQueue;
    167169    RefPtr<WorkQueue> m_decodingQueue;
Note: See TracChangeset for help on using the changeset viewer.