WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(449.88 KB, patch)
2016-09-02 23:30 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(450.35 KB, patch)
2016-09-04 13:18 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(449.91 KB, patch)
2016-09-05 20:28 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(453.47 KB, patch)
2016-09-06 18:05 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(453.37 KB, patch)
2016-09-07 09:29 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch for review
(27.97 KB, patch)
2016-09-07 09:32 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
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
Details
Patch
(25.35 KB, patch)
2016-10-16 01:52 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch for review
(7.25 KB, patch)
2016-10-16 23:23 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch for review
(7.31 KB, patch)
2016-10-17 10:13 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(15.59 KB, patch)
2016-11-04 19:22 PDT
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(33.09 KB, patch)
2016-11-06 23:59 PST
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
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
Details
Patch
(29.77 KB, patch)
2016-11-07 19:14 PST
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(31.18 KB, patch)
2016-11-08 10:53 PST
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(34.78 KB, patch)
2016-11-08 15:03 PST
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(39.82 KB, patch)
2016-11-08 17:59 PST
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Patch
(39.75 KB, patch)
2016-11-09 13:03 PST
,
Said Abou-Hallawa
no flags
Details
Formatted Diff
Diff
Show Obsolete
(19)
View All
Add attachment
proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1
2016-09-02 20:52:41 PDT
Created
attachment 287852
[details]
Patch
Said Abou-Hallawa
Comment 2
2016-09-02 23:30:03 PDT
Created
attachment 287855
[details]
Patch
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
Created
attachment 287920
[details]
Patch
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
Created
attachment 287991
[details]
Patch
Said Abou-Hallawa
Comment 9
2016-09-06 18:05:13 PDT
Created
attachment 288078
[details]
Patch
Said Abou-Hallawa
Comment 10
2016-09-07 09:29:31 PDT
Created
attachment 288144
[details]
Patch
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
Created
attachment 291739
[details]
Patch
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
Created
attachment 293975
[details]
Patch
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
Created
attachment 294047
[details]
Patch
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
Created
attachment 294120
[details]
Patch
Said Abou-Hallawa
Comment 34
2016-11-08 10:53:33 PST
Created
attachment 294170
[details]
Patch
Said Abou-Hallawa
Comment 35
2016-11-08 15:03:20 PST
Created
attachment 294190
[details]
Patch
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
Created
attachment 294208
[details]
Patch
Said Abou-Hallawa
Comment 39
2016-11-09 13:03:06 PST
Created
attachment 294272
[details]
Patch
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
<
rdar://problem/30743188
>
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.
Top of Page
Format For Printing
XML
Clone This Bug