When frames are repainting for async decoding, we'll end up repainting every renderer that uses that image. That can cause repaints in tiles that would otherwise stay non-dirty, which is undesirable.
<rdar://problem/31221145>
*** Bug 173124 has been marked as a duplicate of this bug. ***
Created attachment 312368 [details] Patch
Created attachment 312453 [details] Patch
Created attachment 312461 [details] Patch
Comment on attachment 312461 [details] Patch Attachment 312461 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3901836 New failing tests: fast/images/async-image-background-image-repeated.html
Created attachment 312470 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 312461 [details] Patch Attachment 312461 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3901814 New failing tests: svg/animations/svglength-element-removed-crash.svg
Created attachment 312472 [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.12.5
Comment on attachment 312461 [details] Patch Attachment 312461 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3901944 New failing tests: fast/images/async-image-background-image-repeated.html
Created attachment 312476 [details] Archive of layout-test-results from ews113 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 312504 [details] Patch
Created attachment 312525 [details] Patch
Created attachment 312805 [details] Patch
Comment on attachment 312805 [details] Patch You should be able to make a layout test for this.
Created attachment 313152 [details] Patch
(In reply to Simon Fraser (smfr) from comment #15) > Comment on attachment 312805 [details] > Patch > > You should be able to make a layout test for this. A repaint test is added. Without the patch of this bug, the test gives the following results, which is incorrect: (repaint rects (rect 8 584 100 50) (rect 8 584 100 50) (rect 8 584 100 50) (rect 8 584 100 50) (rect 8 212 200 100) (rect 8 8 400 200) (rect 8 584 100 50) )
Comment on attachment 313152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313152&action=review > Source/WebCore/loader/cache/CachedImage.cpp:355 > +void CachedImage::CachedImageObserver::imageFrameAvailable(const Image& image, ImageAnimatingState animatingState, const std::optional<HashSet<CachedImageClient*>>& imageClients, const IntRect* changeRect) Make a typedef for std::optional<HashSet<CachedImageClient*>> to make the code easier to read. > Source/WebCore/loader/cache/CachedImage.cpp:542 > while (CachedImageClient* client = clientWalker.next()) { How is imageClients a different set from m_clients? Would be nice to name things to make this clear.
Created attachment 314238 [details] Patch
Comment on attachment 314238 [details] Patch Attachment 314238 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4024598 New failing tests: fast/images/async-image-background-image-repeated.html
Created attachment 314240 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 314238 [details] Patch Attachment 314238 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4024647 New failing tests: fast/images/async-image-background-image-repeated.html
Created attachment 314242 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 314238 [details] Patch Attachment 314238 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4024681 New failing tests: fast/images/async-image-background-image-repeated.html
Created attachment 314245 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 314238 [details] Patch Attachment 314238 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4024683 New failing tests: fast/images/async-image-multiple-clients-repaint.html fast/images/async-image-background-image-repeated.html
Created attachment 314246 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 314280 [details] Patch
Comment on attachment 314280 [details] Patch Attachment 314280 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4027949 New failing tests: fast/images/async-image-multiple-clients-repaint.html
Created attachment 314282 [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.12.5
Created attachment 314298 [details] Patch
Comment on attachment 314298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314298&action=review > Source/WebCore/loader/cache/CachedImage.cpp:553 > + if (animatingState == ImageAnimatingState::No && m_pendingImageDrawingClients.find(client) == m_pendingImageDrawingClients.end()) I think this can use m_pendingImageDrawingClients.find.contains() > Source/WebCore/loader/cache/CachedImage.h:157 > + HashSet<CachedImageClient*> m_pendingImageDrawingClients; Do we remove CachedImageClients from this set in all the code paths where those clients are destroyed? I.e. are we sure we never leave a dangling pointer in here? > Source/WebCore/platform/graphics/Image.cpp:186 > + while (result != ImageDrawResult::DidRequestDecoding && toY < destRect.maxY()) { > currentTileRect.shiftXEdgeTo(destRect.x()); > float toX = currentTileRect.x(); > - while (toX < destRect.maxX()) { > + while (result != ImageDrawResult::DidRequestDecoding && toX < destRect.maxX()) { > FloatRect toRect(toX, toY, currentTileRect.width(), currentTileRect.height()); > FloatRect fromRect(toFloatPoint(currentTileRect.location() - oneTileRect.location()), currentTileRect.size()); > fromRect.scale(1 / scale.width(), 1 / scale.height()); > > - draw(ctxt, toRect, fromRect, op, BlendModeNormal, decodingMode, ImageOrientationDescription()); > + result = draw(ctxt, toRect, fromRect, op, BlendModeNormal, decodingMode, ImageOrientationDescription()); I think it would be much clearer to just break from these loops if result == DidRequestDecoding
Created attachment 314332 [details] Patch
Comment on attachment 314298 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314298&action=review >> Source/WebCore/loader/cache/CachedImage.cpp:553 >> + if (animatingState == ImageAnimatingState::No && m_pendingImageDrawingClients.find(client) == m_pendingImageDrawingClients.end()) > > I think this can use m_pendingImageDrawingClients.find.contains() Done. >> Source/WebCore/loader/cache/CachedImage.h:157 >> + HashSet<CachedImageClient*> m_pendingImageDrawingClients; > > Do we remove CachedImageClients from this set in all the code paths where those clients are destroyed? I.e. are we sure we never leave a dangling pointer in here? Yes. I reviewed the code one more time and made sure we are following what we do for m_pendingContainerSizeRequests. >> Source/WebCore/platform/graphics/Image.cpp:186 >> + result = draw(ctxt, toRect, fromRect, op, BlendModeNormal, decodingMode, ImageOrientationDescription()); > > I think it would be much clearer to just break from these loops if result == DidRequestDecoding Done. I made this function returns result if result == DidRequestDecoding.
Comment on attachment 314332 [details] Patch Rejecting attachment 314332 [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-03', 'validate-changelog', '--check-oops', '--non-interactive', 314332, '--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/4030175
Created attachment 314342 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 314342 [details]: The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 314342 [details]: webgl/1.0.2/conformance/glsl/misc/shader-uniform-packing-restrictions.html bug 174064 (author: roger_fong@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 314342 [details] Patch Clearing flags on attachment: 314342 Committed r219045: <http://trac.webkit.org/changeset/219045>
All reviewed patches have been landed. Closing bug.