Bug 165131 – Some animated image do not animate after reseting their animations

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

1. Create an HTML page with a single <img> tag.
2. Set the src of the image tag to a local animated image.
3. Load the page in MiniBrowser.
4. Reload the same page in WebKit by pressing Enter in the address bar in MiniBrowser for example.

Result: The animated image stops animating after reloading the page.

[Said Abou-Hallawa]

Created attachment 295564 [details]
Patch

[Build Bot]

Comment on attachment 295564 [details]
Patch

Attachment 295564 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2586292

New failing tests:
intersection-observer/intersection-observer-entry-interface.html

[Build Bot]

Created attachment 295572 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 295607 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 295607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295607&action=review

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:249
> +    if (!frame.isBeingDecoded())
> +        return;

This is a bit confusing. Is the frame's "being decoded" state true because we know the caller is decoding the frame that it's passing to this function? What is the state in which this is not true? If the decoding thread is terminated, why not pass a null image in here?

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:300
> +        m_frameRequestQueue.open();
>          startAsyncDecodingQueue();

Confusing that the m_frameRequestQueue and the "async decoding queue" are two separate things.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:307
> +    if (frame.isBeingDecoded())
> +        return true;

Seems odd to check this here and not in the caller.

[Said Abou-Hallawa]

Comment on attachment 295607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295607&action=review

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:249
>> +        return;
> 
> This is a bit confusing. Is the frame's "being decoded" state true because we know the caller is decoding the frame that it's passing to this function? What is the state in which this is not true? If the decoding thread is terminated, why not pass a null image in here?

This can happen because of the following call in the decoding thread:

            // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
            callOnMainThread([this, nativeImage, frameRequest] () mutable {
                // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
                if (hasDecodingQueue())
                    cacheFrameNativeImageAtIndex(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel);
            });

This is another asynchronous call. It can be issued just before the decoding thread is terminating but it was not executed. Then ImageFrameCache::stopAsyncDecodingQueue() is called which will terminates the decoding thread and sets m_decodingQueue to null. Then the same image is reused from the cache and ImageFrameCache::startAsyncDecodingQueue() is called. Now m_decodingQueue is set to a new WorkQueue object. Then the following statement is executed on the main thread:

                if (hasDecodingQueue())
                    cacheFrameNativeImageAtIndex(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel);

This will check whether m_decodingQueue is not null. It finds it is not null but actually it is a new WorkQueue different from the one that issues it. So it calls cacheFrameNativeImageAtIndex(). The only thing I can check that this NativeImage a left-over code that we have to ignore is to check frame.isBeingDecoded(). So I have to call frame.clear() for all these frames which are being decoded in ImageFrameCache::stopAsyncDecodingQueue().

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:300
>>          startAsyncDecodingQueue();
> 
> Confusing that the m_frameRequestQueue and the "async decoding queue" are two separate things.

m_frameRequestQueue.open() is moved to startAsyncDecodingQueue() before starting the decoding thread.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:307
>> +        return true;
> 
> Seems odd to check this here and not in the caller.

This is an optimization and the plan is call requestFrameAsyncDecodingAtIndex() from two places: BitmapImage::draw() for large images and BitmapImage::internalStartAnimation() for animated images. So I think it is better to have this check in one place.

[Said Abou-Hallawa]

Created attachment 295653 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 295607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295607&action=review

>>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:249
>>> +        return;
>> 
>> This is a bit confusing. Is the frame's "being decoded" state true because we know the caller is decoding the frame that it's passing to this function? What is the state in which this is not true? If the decoding thread is terminated, why not pass a null image in here?
> 
> This can happen because of the following call in the decoding thread:
> 
>             // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
>             callOnMainThread([this, nativeImage, frameRequest] () mutable {
>                 // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
>                 if (hasDecodingQueue())
>                     cacheFrameNativeImageAtIndex(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel);
>             });
> 
> This is another asynchronous call. It can be issued just before the decoding thread is terminating but it was not executed. Then ImageFrameCache::stopAsyncDecodingQueue() is called which will terminates the decoding thread and sets m_decodingQueue to null. Then the same image is reused from the cache and ImageFrameCache::startAsyncDecodingQueue() is called. Now m_decodingQueue is set to a new WorkQueue object. Then the following statement is executed on the main thread:
> 
>                 if (hasDecodingQueue())
>                     cacheFrameNativeImageAtIndex(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel);
> 
> This will check whether m_decodingQueue is not null. It finds it is not null but actually it is a new WorkQueue different from the one that issues it. So it calls cacheFrameNativeImageAtIndex(). The only thing I can check that this NativeImage a left-over code that we have to ignore is to check frame.isBeingDecoded(). So I have to call frame.clear() for all these frames which are being decoded in ImageFrameCache::stopAsyncDecodingQueue().

It is normal for a function passed to callOnMainThread() to be called after the calling thread terminates. We need to handle this case especially when reloading the same page: (1) the document is destroyed, (2) the animation is stopped (3) the resources are set to be garbage collected, (4) a new document is created, (5) the resources are reused from the memory cache and (6) and the animation is reset. I hit a scenario where the function of callOnMainThread() was called after the animation was restarted.

[Said Abou-Hallawa]

Created attachment 295665 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 295665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295665&action=review

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:307
> +    if (frame.isBeingDecoded())
> +        return true;

I think you need a comment here explaining this check.

[Said Abou-Hallawa]

Created attachment 295697 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295699 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 295665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295665&action=review

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:250
> +    ASSERT(index < m_frames.size());
> +    ImageFrame& frame = m_frames[index];
> +
> +    if (!frame.isBeingDecoded())
> +        return;
> +

I removed this confusing code and I made another change before calling cacheFrameNativeImageAtIndex() through callOnMainThread() in the decoding thread. I replaced

<<    if (hasDecodingQueue())
          cacheFrameNativeImageAtIndex(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel);

with

>>    if (protectedQueue.ptr() == m_decodingQueue)
          cacheFrameNativeImageAtIndex(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel);

The condition if (protectedQueue.ptr() == m_decodingQueue) will be false if the decoding thread was terminated or restarted. In both cases, the NativeImage will not cached in the ImageFrame and this is what we want.  Note that the old if (hasDecodingQueue()) was false only if the decoding thread was terminated but not restarted.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:307
>> +        return true;
> 
> I think you need a comment here explaining this check.

Done.

[WebKit Commit Bot]

Comment on attachment 295699 [details]
Patch

Rejecting attachment 295699 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 295699, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2593560

[Said Abou-Hallawa]

Created attachment 295717 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 295717 [details]
Patch

Clearing flags on attachment: 295717

Committed r209131: <http://trac.webkit.org/changeset/209131>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.