Bug 169944

Summary:

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

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

Images

Assignee:

Said Abou-Hallawa <sabouhallawa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, rniwa, sabouhallawa, simon.fraser, thorton, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews102 for mac-elcapitan	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Archive of layout-test-results from ews113 for mac-elcapitan	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2	none
Archive of layout-test-results from ews100 for mac-elcapitan	none
Archive of layout-test-results from ews115 for mac-elcapitan	none
Archive of layout-test-results from ews122 for ios-simulator-wk2	none
Patch	none
Archive of layout-test-results from ews121 for ios-simulator-wk2	none
Patch	none
Patch	none
Patch	none

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.