169944 – If an image appears more than once on a page, decoding for painting one instance repaints them all

RESOLVED FIXED 169944

If an image appears more than once on a page, decoding for painting one instance repaints them all

https://bugs.webkit.org/show_bug.cgi?id=169944

Summary If an image appears more than once on a page, decoding for painting one insta...

Simon Fraser (smfr)

Reported 2017-03-21 20:16:22 PDT

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.

Attachments
Patch (42.21 KB, patch) 2017-06-08 18:39 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (46.07 KB, patch) 2017-06-09 10:33 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (47.17 KB, patch) 2017-06-09 11:16 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-elcapitan (999.41 KB, application/zip) 2017-06-09 12:16 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (912.58 KB, application/zip) 2017-06-09 12:33 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.74 MB, application/zip) 2017-06-09 12:51 PDT, Build Bot	no flags	Details
Patch (53.54 KB, patch) 2017-06-09 15:57 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (53.52 KB, patch) 2017-06-09 18:24 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (56.31 KB, patch) 2017-06-13 13:55 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (59.68 KB, patch) 2017-06-16 16:05 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (48.92 KB, patch) 2017-06-29 22:45 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.40 MB, application/zip) 2017-06-29 23:41 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews100 for mac-elcapitan (991.49 KB, application/zip) 2017-06-29 23:52 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.74 MB, application/zip) 2017-06-30 00:22 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (20.24 MB, application/zip) 2017-06-30 00:25 PDT, Build Bot	no flags	Details
Patch (50.76 KB, patch) 2017-06-30 11:52 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (948.16 KB, application/zip) 2017-06-30 13:26 PDT, Build Bot	no flags	Details
Patch (51.45 KB, patch) 2017-06-30 14:53 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (51.53 KB, patch) 2017-06-30 17:29 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Patch (51.53 KB, patch) 2017-06-30 18:14 PDT, Said Abou-Hallawa	no flags	Details Formatted Diff Diff
Show Obsolete (19) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-03-23 09:37:19 PDT

<rdar://problem/31221145>

Said Abou-Hallawa

Comment 2 2017-06-08 18:19:12 PDT

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

Said Abou-Hallawa

Comment 3 2017-06-08 18:39:28 PDT

Created attachment 312368 [details] Patch

Said Abou-Hallawa

Comment 4 2017-06-09 10:33:26 PDT

Created attachment 312453 [details] Patch

Said Abou-Hallawa

Comment 5 2017-06-09 11:16:31 PDT

Created attachment 312461 [details] Patch

Build Bot

Comment 6 2017-06-09 12:16:50 PDT

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

Build Bot

Comment 7 2017-06-09 12:16:51 PDT

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

Build Bot

Comment 8 2017-06-09 12:33:50 PDT

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

Build Bot

Comment 9 2017-06-09 12:33:51 PDT

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

Build Bot

Comment 10 2017-06-09 12:51:27 PDT

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

Build Bot

Comment 11 2017-06-09 12:51:28 PDT

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

Said Abou-Hallawa

Comment 12 2017-06-09 15:57:04 PDT

Created attachment 312504 [details] Patch

Said Abou-Hallawa

Comment 13 2017-06-09 18:24:05 PDT

Created attachment 312525 [details] Patch

Said Abou-Hallawa

Comment 14 2017-06-13 13:55:04 PDT

Created attachment 312805 [details] Patch

Simon Fraser (smfr)

Comment 15 2017-06-13 15:37:27 PDT

Comment on attachment 312805 [details] Patch You should be able to make a layout test for this.

Said Abou-Hallawa

Comment 16 2017-06-16 16:05:36 PDT

Created attachment 313152 [details] Patch

Said Abou-Hallawa

Comment 17 2017-06-16 16:13:47 PDT

(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) )

Simon Fraser (smfr)

Comment 18 2017-06-28 15:10:26 PDT

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.

Said Abou-Hallawa

Comment 19 2017-06-29 22:45:00 PDT

Created attachment 314238 [details] Patch

Build Bot

Comment 20 2017-06-29 23:41:10 PDT

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

Build Bot

Comment 21 2017-06-29 23:41:12 PDT

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

Build Bot

Comment 22 2017-06-29 23:52:32 PDT

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

Build Bot

Comment 23 2017-06-29 23:52:34 PDT

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

Build Bot

Comment 24 2017-06-30 00:22:07 PDT

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

Build Bot

Comment 25 2017-06-30 00:22:08 PDT

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

Build Bot

Comment 26 2017-06-30 00:25:40 PDT

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

Build Bot

Comment 27 2017-06-30 00:25:42 PDT

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

Said Abou-Hallawa

Comment 28 2017-06-30 11:52:27 PDT

Created attachment 314280 [details] Patch

Build Bot

Comment 29 2017-06-30 13:26:06 PDT

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

Build Bot

Comment 30 2017-06-30 13:26:07 PDT

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

Said Abou-Hallawa

Comment 31 2017-06-30 14:53:27 PDT

Created attachment 314298 [details] Patch

Simon Fraser (smfr)

Comment 32 2017-06-30 16:54:46 PDT

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

Said Abou-Hallawa

Comment 33 2017-06-30 17:29:11 PDT

Created attachment 314332 [details] Patch

Said Abou-Hallawa

Comment 34 2017-06-30 17:31:57 PDT

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.

WebKit Commit Bot

Comment 35 2017-06-30 17:44:16 PDT

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

Said Abou-Hallawa

Comment 36 2017-06-30 18:14:53 PDT

Created attachment 314342 [details] Patch

WebKit Commit Bot

Comment 37 2017-06-30 21:06:01 PDT

The commit-queue encountered the following flaky tests while processing attachment 314342 [details]: The commit-queue is continuing to process your patch.

WebKit Commit Bot

Comment 38 2017-06-30 21:06:11 PDT

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.

WebKit Commit Bot

Comment 39 2017-06-30 23:23:35 PDT

Comment on attachment 314342 [details] Patch Clearing flags on attachment: 314342 Committed r219045: <http://trac.webkit.org/changeset/219045>

WebKit Commit Bot

Comment 40 2017-06-30 23:23:36 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Local Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Images

Assignee

Said Abou-Hallawa

Reported

2017-03-21 20:16 PDT

Modified

2017-06-30 23:23 PDT History

CC List

7 users Show

URL

Keywords InRadar

Duplicates (1)

173124 View as bug list

Depends on

Blocks