RESOLVED FIXED Bug 161566
Change the decoding for some animated images to be asynchronous
https://bugs.webkit.org/show_bug.cgi?id=161566
Summary Change the decoding for some animated images to be asynchronous
Said Abou-Hallawa
Reported 2016-09-02 20:40:18 PDT
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.
Attachments
Patch (448.65 KB, patch)
2016-09-02 20:52 PDT, Said Abou-Hallawa
no flags
Patch (449.88 KB, patch)
2016-09-02 23:30 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 (8.52 MB, application/zip)
2016-09-03 00:45 PDT, Build Bot
no flags
Patch (450.35 KB, patch)
2016-09-04 13:18 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews122 for ios-simulator-elcapitan-wk2 (6.20 MB, application/zip)
2016-09-04 15:02 PDT, Build Bot
no flags
Patch (449.91 KB, patch)
2016-09-05 20:28 PDT, Said Abou-Hallawa
no flags
Patch (453.47 KB, patch)
2016-09-06 18:05 PDT, Said Abou-Hallawa
no flags
Patch (453.37 KB, patch)
2016-09-07 09:29 PDT, Said Abou-Hallawa
no flags
Patch for review (27.97 KB, patch)
2016-09-07 09:32 PDT, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews117 for mac-yosemite (1.51 MB, application/zip)
2016-09-07 10:43 PDT, Build Bot
no flags
Patch (25.35 KB, patch)
2016-10-16 01:52 PDT, Said Abou-Hallawa
no flags
Patch for review (7.25 KB, patch)
2016-10-16 23:23 PDT, Said Abou-Hallawa
no flags
Patch for review (7.31 KB, patch)
2016-10-17 10:13 PDT, Said Abou-Hallawa
no flags
Patch (15.59 KB, patch)
2016-11-04 19:22 PDT, Said Abou-Hallawa
no flags
Patch (33.09 KB, patch)
2016-11-06 23:59 PST, Said Abou-Hallawa
no flags
Archive of layout-test-results from ews121 for ios-simulator-wk2 (17.89 MB, application/zip)
2016-11-07 14:38 PST, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.73 MB, application/zip)
2016-11-07 15:10 PST, Build Bot
no flags
Archive of layout-test-results from ews112 for mac-yosemite (1.99 MB, application/zip)
2016-11-07 15:12 PST, Build Bot
no flags
Archive of layout-test-results from ews100 for mac-yosemite (1.18 MB, application/zip)
2016-11-07 15:57 PST, Build Bot
no flags
Patch (29.77 KB, patch)
2016-11-07 19:14 PST, Said Abou-Hallawa
no flags
Patch (31.18 KB, patch)
2016-11-08 10:53 PST, Said Abou-Hallawa
no flags
Patch (34.78 KB, patch)
2016-11-08 15:03 PST, Said Abou-Hallawa
no flags
Patch (39.82 KB, patch)
2016-11-08 17:59 PST, Said Abou-Hallawa
no flags
Patch (39.75 KB, patch)
2016-11-09 13:03 PST, Said Abou-Hallawa
no flags
Said Abou-Hallawa
Comment 1 2016-09-02 20:52:41 PDT
Said Abou-Hallawa
Comment 2 2016-09-02 23:30:03 PDT
Build Bot
Comment 3 2016-09-03 00:45:07 PDT
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
Comment 4 2016-09-03 00:45:09 PDT
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
Comment 5 2016-09-04 13:18:04 PDT
Build Bot
Comment 6 2016-09-04 15:02:23 PDT
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
Comment 7 2016-09-04 15:02:26 PDT
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
Comment 8 2016-09-05 20:28:11 PDT
Said Abou-Hallawa
Comment 9 2016-09-06 18:05:13 PDT
Said Abou-Hallawa
Comment 10 2016-09-07 09:29:31 PDT
Said Abou-Hallawa
Comment 11 2016-09-07 09:32:43 PDT
Created attachment 288145 [details] Patch for review
Build Bot
Comment 12 2016-09-07 10:43:51 PDT
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
Comment 13 2016-09-07 10:43:54 PDT
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
Comment 14 2016-10-16 01:52:19 PDT
Simon Fraser (smfr)
Comment 15 2016-10-16 16:05:34 PDT
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
Comment 16 2016-10-16 23:23:16 PDT
Created attachment 291794 [details] Patch for review
Said Abou-Hallawa
Comment 17 2016-10-16 23:29:09 PDT
(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
Comment 18 2016-10-17 10:13:12 PDT
Created attachment 291828 [details] Patch for review
Said Abou-Hallawa
Comment 19 2016-11-04 19:22:12 PDT
Simon Fraser (smfr)
Comment 20 2016-11-04 20:43:37 PDT
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
Comment 21 2016-11-06 23:59:14 PST
Said Abou-Hallawa
Comment 22 2016-11-07 10:26:27 PST
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 23 2016-11-07 10:35:44 PST
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 24 2016-11-07 14:38:44 PST
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
Comment 25 2016-11-07 14:38:49 PST
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 26 2016-11-07 15:09:09 PST
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 27 2016-11-07 15:10:11 PST
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
Comment 28 2016-11-07 15:10:16 PST
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 29 2016-11-07 15:12:17 PST
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
Comment 30 2016-11-07 15:12:22 PST
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 31 2016-11-07 15:57:45 PST
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
Comment 32 2016-11-07 15:57:50 PST
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
Comment 33 2016-11-07 19:14:35 PST
Said Abou-Hallawa
Comment 34 2016-11-08 10:53:33 PST
Said Abou-Hallawa
Comment 35 2016-11-08 15:03:20 PST
Simon Fraser (smfr)
Comment 36 2016-11-08 15:48:37 PST
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 37 2016-11-08 17:56:07 PST
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
Comment 38 2016-11-08 17:59:26 PST
Said Abou-Hallawa
Comment 39 2016-11-09 13:03:06 PST
WebKit Commit Bot
Comment 40 2016-11-09 17:16:09 PST
Comment on attachment 294272 [details] Patch Clearing flags on attachment: 294272 Committed r208511: <http://trac.webkit.org/changeset/208511>
WebKit Commit Bot
Comment 41 2016-11-09 17:16:18 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 42 2017-02-27 14:49:44 PST
Said Abou-Hallawa
Comment 43 2017-08-24 16:21:59 PDT
*** Bug 109395 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.