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

• Component: Images
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/31221145>

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 312368 [details]
Patch

[Said Abou-Hallawa]

Created attachment 312453 [details]
Patch

[Said Abou-Hallawa]

Created attachment 312461 [details]
Patch

[Build Bot]

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]

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 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]

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 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]

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]

Created attachment 312504 [details]
Patch

[Said Abou-Hallawa]

Created attachment 312525 [details]
Patch

[Said Abou-Hallawa]

Created attachment 312805 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 312805 [details]
Patch

You should be able to make a layout test for this.

[Said Abou-Hallawa]

Created attachment 313152 [details]
Patch

[Said Abou-Hallawa]

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

Created attachment 314238 [details]
Patch

[Build Bot]

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]

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 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]

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 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]

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 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]

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]

Created attachment 314280 [details]
Patch

[Build Bot]

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]

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]

Created attachment 314298 [details]
Patch

[Simon Fraser (smfr)]

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]

Created attachment 314332 [details]
Patch

[Said Abou-Hallawa]

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 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]

Created attachment 314342 [details]
Patch

[WebKit Commit Bot]

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]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 314342 [details]
Patch

Clearing flags on attachment: 314342

Committed r219045: <http://trac.webkit.org/changeset/219045>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.