Bug 161566 – Change the decoding for some animated images to be asynchronous

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

When there are many frames or the frames are large, the memory cache keeps asks for destroying the decoded frames. Having the decoding happens at the time of drawing makes the animation stuttered. To resolve this problem, the image decoding should be moved form the main thread to a operate thread. And the frame to be displayed should be requested in advance such such that when it the frame is drawn, its decoded data is ready.

[Said Abou-Hallawa]

Created attachment 287852 [details]
Patch

[Said Abou-Hallawa]

Created attachment 287855 [details]
Patch

[Build Bot]

Comment on attachment 287855 [details]
Patch

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

New failing tests:
fast/images/animated-gif-restored-from-bfcache.html

[Build Bot]

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

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

[Said Abou-Hallawa]

Created attachment 287920 [details]
Patch

[Build Bot]

Comment on attachment 287920 [details]
Patch

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

New failing tests:
fast/scrolling/ios/scroll-events-back-forward.html
fast/scrolling/ios/scrollTo-at-page-load.html

[Build Bot]

Created attachment 287921 [details]
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2

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

[Said Abou-Hallawa]

Created attachment 287991 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288078 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288144 [details]
Patch

[Said Abou-Hallawa]

Created attachment 288145 [details]
Patch for review

[Build Bot]

Comment on attachment 288144 [details]
Patch

Attachment 288144 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2026460

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 288158 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 291739 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291739 [details]
Patch

You have two changeling entries.

I'm not sure we want to do async decoding for all animated images; I think we should only do it for those over a certain size.

[Said Abou-Hallawa]

Created attachment 291794 [details]
Patch for review

[Said Abou-Hallawa]

(In reply to comment #15)
> Comment on attachment 291739 [details]
> Patch
> 
> You have two changeling entries.
>

Yes, this is true because I had to include the fix https://bugs.webkit.org/show_bug.cgi?id=155546 in the same patch. I split the fix of this bug in a separate patch for review. 

> I'm not sure we want to do async decoding for all animated images; I think
> we should only do it for those over a certain size.

In the attached patch: 

    if (m_source.isAsyncDecodingRequired())
        m_source.requestFrameAsyncDecodingAtIndex(nextFrame, m_currentSubsamplingLevel);

If the image frame size is more than a certain size, the image asynchronous decoding is enabled.

[Said Abou-Hallawa]

Created attachment 291828 [details]
Patch for review

[Said Abou-Hallawa]

Created attachment 293975 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 293975 [details]
Patch

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

Before this lands I would like to see:
* LOG(Images...) tracing for async image decoding (log when requesting a frame, and when the decode completes)
* Some way to test what happens on navigation etc. when an image decode is in-flight, and tests.

> Source/WebCore/loader/cache/CachedImage.h:142
> +    // The default value of m_allowSubsampling should be the same as defaultImageSubsamplingEnabled in Settings.cpp

Why?

> Source/WebCore/loader/cache/CachedImage.h:148
> +    bool m_allowAsyncImageDecoding { true };

Surprising. What about the other platforms?

> Source/WebCore/rendering/RenderImage.cpp:149
> +    auto* image = imageResource().image().get();
> +    if (image && !image->isNull())
> +        image->stopAnimation();
>      imageResource().shutdown();

Maybe shutdown() should call stopAnimation on the image?

[Said Abou-Hallawa]

Created attachment 294047 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 293975 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedImage.h:142
>> +    // The default value of m_allowSubsampling should be the same as defaultImageSubsamplingEnabled in Settings.cpp
> 
> Why?

I just added the comment. But the difference between iOS and the other platforms was added in https://trac.webkit.org/changeset/172348. defaultImageSubsamplingEnabled and m_allowSubsampling are initialized with true only for iOS.

>> Source/WebCore/rendering/RenderImage.cpp:149
>>      imageResource().shutdown();
> 
> Maybe shutdown() should call stopAnimation on the image?

Done although I had to call stopAnimation() from RenderImageResource::shutdown() and RenderImageResourceStyleImage::shutdown().

[Said Abou-Hallawa]

Comment on attachment 294047 [details]
Patch

The EWS are stuck but meanwhile I am trying to get this patch reviewed. I won't commit it until the issue is resolved and the results of my patch are all clean.

[Build Bot]

Comment on attachment 294047 [details]
Patch

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

New failing tests:
fast/images/stopped-animation-deleted-image.html

[Build Bot]

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

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

[Simon Fraser (smfr)]

Comment on attachment 294047 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.h:171
> +    Optional<float> m_frameDecodingDuration;
> +    Optional<float> m_frameAnimationDuration;

I think should have names that make it obvious that they are for testing. Like "ForTesting" in the name.

> LayoutTests/fast/images/slower-animation-than-decoding-image.html:27
> +             }, 100);

100ms is long.

> LayoutTests/fast/images/slower-decoding-than-animation-image.html:27
> +             }, 100);

100ms is very long for a test.

> LayoutTests/fast/images/stopped-animation-deleted-image.html:31
> +                }, 100);
> +             }, 100);

Can we do this without a 200ms test? This would be one of the longest-running tests if you added this.

[Build Bot]

Comment on attachment 294047 [details]
Patch

Attachment 294047 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2469555

New failing tests:
fast/images/stopped-animation-deleted-image.html

[Build Bot]

Created attachment 294091 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 294047 [details]
Patch

Attachment 294047 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2469492

New failing tests:
fast/images/slower-animation-than-decoding-image.html
fast/images/stopped-animation-deleted-image.html
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 294092 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 294047 [details]
Patch

Attachment 294047 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2469789

New failing tests:
fast/images/slower-animation-than-decoding-image.html
fast/images/stopped-animation-deleted-image.html
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 294096 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 294120 [details]
Patch

[Said Abou-Hallawa]

Created attachment 294170 [details]
Patch

[Said Abou-Hallawa]

Created attachment 294190 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 294190 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:278
> +    // it will be decoded on a separate work queue. When decoding nextFrame finishes, this class will be notified

"this class" -> "this object" or just "we".

> Source/WebCore/platform/graphics/BitmapImage.cpp:281
> +    if (isAsyncDecodingForcedForTesting() || (allowAsyncImageDecoding() && m_source.isAsyncDecodingRequired())) {

I would put the isAsyncDecodingForcedForTesting() last.

> Source/WebCore/platform/graphics/BitmapImage.cpp:311
> +    else if (showDebugBackground())
> +        imageObserver()->animationAdvanced(this);

I don't get this. Why advanced the animation if you're showing the debug background?

So we could paint the debug background in two situations:
1. non-animated image which is currently decoding (it would previously have blocked the main thread). This is the one I care about most.
2. animated image where the frame is late because of async decoding. In this case, you need to trigger a repaint when you expected the frame (to paint the background). I don' think the current code does that. I feel less strongly about needing the debug background here.

> Source/WebCore/platform/graphics/Color.h:259
>      static const RGBA32 cyan = 0xFF00FFFF;
> +    static const RGBA32 yellow = 0xffffff00;

Please be consistent with the capitalization.

> LayoutTests/fast/images/stopped-animation-deleted-image.html:24
> +        var image = document.getElementsByTagName("img")[0];
> +        var loopCount = 0;
> +        var frameIndex;
> +
> +        function imageLoaded()
> +        {
> +            if (!window.internals)
> +                return;
> +            internals.setImageFrameDecodingDuration(image, 0.040);
> +            imageRemove();
> +        }

I think this is racy. It would be better to call internals.setImageFrameDecodingDuration(image, 0.040); before setting the image source.

[Said Abou-Hallawa]

Comment on attachment 294190 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:278
>> +    // it will be decoded on a separate work queue. When decoding nextFrame finishes, this class will be notified
> 
> "this class" -> "this object" or just "we".

Done.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:281
>> +    if (isAsyncDecodingForcedForTesting() || (allowAsyncImageDecoding() && m_source.isAsyncDecodingRequired())) {
> 
> I would put the isAsyncDecodingForcedForTesting() last.

Done.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:311
>> +        imageObserver()->animationAdvanced(this);
> 
> I don't get this. Why advanced the animation if you're showing the debug background?
> 
> So we could paint the debug background in two situations:
> 1. non-animated image which is currently decoding (it would previously have blocked the main thread). This is the one I care about most.
> 2. animated image where the frame is late because of async decoding. In this case, you need to trigger a repaint when you expected the frame (to paint the background). I don' think the current code does that. I feel less strongly about needing the debug background here.

I was using animationAdvanced() as a hack to force repaint. But I found out ImageObserver::changedInRect() is what I need. It forces a repaint only.

>> Source/WebCore/platform/graphics/Color.h:259
>> +    static const RGBA32 yellow = 0xffffff00;
> 
> Please be consistent with the capitalization.

Done.

>> LayoutTests/fast/images/stopped-animation-deleted-image.html:24
>> +        }
> 
> I think this is racy. It would be better to call internals.setImageFrameDecodingDuration(image, 0.040); before setting the image source.

I think I can't do that because setImageFrameDecodingDuration() gets the BitmapImage of the CachedImage of the HTMLImageElement and stores the decodingDuration there. The BitmapImage is set only when data is received. If the HTMLImageElement does not have a BitmapImage, nothing will be set. Another way to fix this issue is to have the decoding Duration a global setting and not tied to a specific image.

[Said Abou-Hallawa]

Created attachment 294208 [details]
Patch

[Said Abou-Hallawa]

Created attachment 294272 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 294272 [details]
Patch

Clearing flags on attachment: 294272

Committed r208511: <http://trac.webkit.org/changeset/208511>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/30743188>

[Said Abou-Hallawa]

*** Bug 109395 has been marked as a duplicate of this bug. ***