Bug 165039 – Enable async image decoding for large images

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

The large image will request its first frame to be decoded once it is drawn. This should improve the first time paint scenario. It can also improve the scrolling scenario if the image is drawn on a tile outside the viewport.

[Said Abou-Hallawa]

Created attachment 295353 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295354 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295355 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295356 [details]
Patch

[Build Bot]

Comment on attachment 295356 [details]
Patch

Attachment 295356 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2559609

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 295358 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295356 [details]
Patch

Attachment 295356 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2559615

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 295359 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295356 [details]
Patch

Attachment 295356 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2559636

New failing tests:
fast/images/image-subsampling.html
fast/flexbox/image-percent-max-height.html
canvas/philip/tests/2d.drawImage.9arg.sourcepos.html
platform/ios-simulator/ios/fast/canvas/image_subSampling_scale.html
imported/w3c/canvas/2d.drawImage.9arg.sourcesize.html
canvas/philip/tests/2d.drawImage.negativedest.html
compositing/backgrounds/fixed-background-on-descendant.html
fast/multicol/newmulticol/compare-with-old-impl/shrink-to-column-height-for-pagination.html
imported/w3c/canvas/2d.drawImage.negativedir.html
imported/w3c/canvas/2d.drawImage.negativedest.html
fast/canvas/canvas-drawImage-shadow.html
imported/w3c/canvas/2d.drawImage.9arg.sourcepos.html
canvas/philip/tests/2d.drawImage.negativesource.html
fast/css/object-fit/object-fit-canvas.html
fast/flexbox/aspect-ratio-intrinsic-adjust.html
compositing/backgrounds/fixed-backgrounds.html
imported/w3c/canvas/2d.drawImage.negativesource.html
fast/canvas/image-potential-subsample.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight.html
canvas/philip/tests/2d.drawImage.negativedir.html

[Build Bot]

Created attachment 295360 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 295356 [details]
Patch

Attachment 295356 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2559792

New failing tests:
fast/images/image-subsampling.html
canvas/philip/tests/2d.drawImage.9arg.sourcepos.html
canvas/philip/tests/2d.drawImage.negativedest.html
imported/w3c/canvas/2d.pattern.paint.repeat.coord2.html
fast/flexbox/image-percent-max-height.html
fast/flexbox/aspect-ratio-intrinsic-adjust.html
webgl/1.0.2/conformance/canvas/to-data-url-test.html
imported/w3c/canvas/2d.drawImage.negativedest.html
imported/w3c/canvas/2d.drawImage.negativedir.html
imported/w3c/canvas/2d.drawImage.9arg.sourcepos.html
fast/css/object-fit/object-fit-shrink.html
canvas/philip/tests/2d.pattern.paint.repeat.coord1.html
fast/borders/border-painting-correctness-dashed.html
canvas/philip/tests/2d.drawImage.negativesource.html
imported/w3c/canvas/2d.drawImage.negativesource.html
imported/w3c/canvas/2d.drawImage.9arg.sourcesize.html
fast/canvas/canvas-blending-image-over-color.html
fast/multicol/newmulticol/compare-with-old-impl/shrink-to-column-height-for-pagination.html
imported/w3c/canvas/2d.pattern.paint.repeat.coord1.html
fast/canvas/canvas-drawImage-shadow.html
fast/borders/border-painting-correctness-dotted.html
canvas/philip/tests/2d.pattern.paint.repeat.coord3.html
canvas/philip/tests/2d.pattern.paint.repeat.coord2.html
imported/w3c/canvas/2d.pattern.paint.repeat.coord3.html
canvas/philip/tests/2d.drawImage.9arg.sourcesize.html
canvas/philip/tests/2d.drawImage.negativeSourceHeight.html
canvas/philip/tests/2d.drawImage.negativedir.html

[Build Bot]

Created attachment 295361 [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

[Said Abou-Hallawa]

Created attachment 295381 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295382 [details]
Patch

[Build Bot]

Comment on attachment 295382 [details]
Patch

Attachment 295382 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2562528

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 295385 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 295386 [details]
Patch

[Build Bot]

Comment on attachment 295386 [details]
Patch

Attachment 295386 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2562748

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html
fast/css/object-fit/object-fit-embed.html
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 295388 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295386 [details]
Patch

Attachment 295386 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2562769

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html

[Build Bot]

Created attachment 295389 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295386 [details]
Patch

Attachment 295386 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2562768

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html
fast/css/object-fit/object-fit-embed.html
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 295390 [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

[Build Bot]

Comment on attachment 295386 [details]
Patch

Attachment 295386 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2562780

New failing tests:
imported/blink/fast/frames/navigation-in-pagehide.html

[Build Bot]

Created attachment 295391 [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

[Said Abou-Hallawa]

Created attachment 295398 [details]
Patch

[Build Bot]

Comment on attachment 295398 [details]
Patch

Attachment 295398 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2564490

New failing tests:
fast/css/object-fit/object-fit-embed.html

[Build Bot]

Created attachment 295401 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295398 [details]
Patch

Attachment 295398 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2564506

New failing tests:
fast/css/object-fit/object-fit-embed.html

[Build Bot]

Created attachment 295403 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 295460 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295464 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295465 [details]
Patch

[Build Bot]

Comment on attachment 295465 [details]
Patch

Attachment 295465 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2579935

New failing tests:
fast/css/object-fit/object-fit-embed.html

[Build Bot]

Created attachment 295468 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295465 [details]
Patch

Attachment 295465 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2579961

New failing tests:
fast/css/object-fit/object-fit-embed.html

[Build Bot]

Created attachment 295470 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Said Abou-Hallawa]

Created attachment 295478 [details]
Patch

[Build Bot]

Comment on attachment 295478 [details]
Patch

Attachment 295478 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2581106

New failing tests:
fast/css/object-fit/object-fit-embed.html
transitions/default-timing-function.html

[Build Bot]

Created attachment 295479 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295478 [details]
Patch

Attachment 295478 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2581110

New failing tests:
fast/css/object-fit/object-fit-embed.html

[Build Bot]

Created attachment 295480 [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]

Created attachment 295481 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295483 [details]
Patch

[Said Abou-Hallawa]

Created attachment 295503 [details]
Patch

[Said Abou-Hallawa]

Created attachment 302351 [details]
Patch

[Build Bot]

Comment on attachment 302351 [details]
Patch

Attachment 302351 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3169736

New failing tests:
compositing/backgrounds/fixed-background-on-descendant.html

[Build Bot]

Created attachment 302355 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 302435 [details]
Patch

[Build Bot]

Comment on attachment 302435 [details]
Patch

Attachment 302435 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3174868

New failing tests:
fast/dom/timer-throttling-hidden-page.html

[Build Bot]

Created attachment 302449 [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

[Chris Dumez]

(In reply to comment #50)
> Comment on attachment 302435 [details]
> Patch
> 
> Attachment 302435 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3174868
> 
> New failing tests:
> fast/dom/timer-throttling-hidden-page.html

Likely a flake, this is a test I have landed today.

[Said Abou-Hallawa]

Created attachment 302494 [details]
Patch

[Build Bot]

Comment on attachment 302494 [details]
Patch

Attachment 302494 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3177475

New failing tests:
fast/dom/timer-throttling-hidden-page.html

[Build Bot]

Created attachment 302501 [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

[Said Abou-Hallawa]

(In reply to comment #52)
> (In reply to comment #50)
> > Comment on attachment 302435 [details]
> > Patch
> > 
> > Attachment 302435 [details] did not pass mac-debug-ews (mac):
> > Output: http://webkit-queues.webkit.org/results/3174868
> > 
> > New failing tests:
> > fast/dom/timer-throttling-hidden-page.html
> 
> Likely a flake, this is a test I have landed today.

Shouldn't this test be marked as a flaky test?

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fdom%2Ftimer-throttling-hidden-page.html

[Simon Fraser (smfr)]

Comment on attachment 302494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302494&action=review

It's really unfortunate that we pass MaxPixelSize down through all these functions. Can we pass down something like a std::optional<IntSize>sizeForDraw or something instead?

> Source/WebCore/ChangeLog:20
> +        If image source size is large (e.g. 3000x3000 pixels) and the size of the
> +        destination rectangle is small, CGImageSourceCreateThumbnailAtIndex() is
> +        slower than CGImageSourceCreateImageAtIndex() in decoding this image. To
> +        overcome this problem, the entry (kCGImageSourceThumbnailMaxPixelSize,
> +        max(destinationRect.width,  destinationRect.height)) is added to the options
> +        dictionary when calling CGImageSourceCreateThumbnailAtIndex(). This will
> +        avoid copying a large block of memory for the unscaled bitmap image.

This seems like a different patch. I think you should split this into 2.

> Source/WebCore/loader/cache/CachedImage.h:129
> +        URL sourceUrl() const override { return m_cachedImages[0]->url(); }

Is m_cachedImages guaranteed to have at least one entry?

> Source/WebCore/page/FrameView.h:278
> +    void requestAsyncDecodingForImagesInRectIncludingSubframes(const IntRect&);

This needs to be more specific about what coordinate system 'rect' is in. Is it document coords, or view coords? Does it change with zooming?

> Source/WebCore/page/FrameView.h:653
> +    void requestAsyncDecodingForImagesInRect(const IntRect&);

Ditto.

> Source/WebCore/platform/graphics/BitmapImage.cpp:246
> +bool BitmapImage::isAsyncDecodingRequired()

Can this be const?

> Source/WebCore/platform/graphics/BitmapImage.h:136
> +    String sourceURL() const { return imageObserver() ? imageObserver()->sourceUrl().string() : ""; }

More efficient to use emptyString() than "".

> Source/WebCore/platform/graphics/BitmapImage.h:210
> +    MaxPixelSize m_currentMaxPixelSize { MaxPixelSizeNative };

It's not clear what this means. What is the context of "current"? What does "max pixel size" mean for a BitmapImage?

> Source/WebCore/platform/graphics/ImageFrame.h:76
> +typedef int MaxPixelSize;

You plumb MaxPixelSize through a lot of functions, and it's hard to know what it means.

> Source/WebCore/platform/graphics/ImageFrame.h:79
> +    MaxPixelSizeUndefined = -1,

We try to avoid magic -1 numbers. Can we use std::optional instead?

[Said Abou-Hallawa]

Comment on attachment 302494 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=302494&action=review

>> Source/WebCore/ChangeLog:20
>> +        avoid copying a large block of memory for the unscaled bitmap image.
> 
> This seems like a different patch. I think you should split this into 2.

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

[Jon Lee]

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

[Radar WebKit Bug Importer]

<rdar://problem/30743122>

[Jon Lee]

rdar://problem/24709033

[Said Abou-Hallawa]

Created attachment 303555 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 303555 [details]
Patch

This patch include the patch of https://bugs.webkit.org/show_bug.cgi?id=168814. I just want to make sure the test are passing with the combined patch.

[Build Bot]

Comment on attachment 303555 [details]
Patch

Attachment 303555 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3254591

New failing tests:
fast/images/reset-image-animation.html

[Build Bot]

Created attachment 303569 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303555 [details]
Patch

Attachment 303555 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3254594

New failing tests:
tables/mozilla/bugs/bug4527.html
fast/images/reset-image-animation.html

[Build Bot]

Created attachment 303572 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303555 [details]
Patch

Attachment 303555 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3254626

New failing tests:
fast/images/reset-image-animation.html

[Build Bot]

Created attachment 303573 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 303555 [details]
Patch

Attachment 303555 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3254615

New failing tests:
fast/images/reset-image-animation.html

[Build Bot]

Created attachment 303576 [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.11.6

[Said Abou-Hallawa]

Created attachment 303596 [details]
Patch

[Said Abou-Hallawa]

Created attachment 303597 [details]
Patch for 165039 separate from 168814

[Said Abou-Hallawa]

Created attachment 303687 [details]
Patch

[Said Abou-Hallawa]

Created attachment 303690 [details]
Patch for 165039 separate from 168814

This patch gets rid of using descendantsOfType<RenderImage>() in RenderView::requestAsyncDecodingForImagesInDocumentRect(). Instead it uses a HashSet of RenderElement which is built by the HTMLImageElement.

[Said Abou-Hallawa]

I am working on a test. But I have to implement the ondecode event instead of using a timer. If it turns out implementing the ondecode event is very involving, I will fallback to using the timer.

[Simon Fraser (smfr)]

Comment on attachment 303690 [details]
Patch for 165039 separate from 168814

View in context: https://bugs.webkit.org/attachment.cgi?id=303690&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:178
> +        if (!frameHasDecodedNativeImage(m_currentFrame))
> +            return;

Don't we want to paint the yellow debug color here too?

> Source/WebCore/platform/graphics/BitmapImage.cpp:409
> +            imageObserver()->changedInRect(this, nullptr);

When is newFrameNativeImageAvailableAtIndex() called? If it's during painting, the repaint triggered by changedInRect() will get dropped on the floor.

> Source/WebCore/platform/graphics/BitmapImage.cpp:422
> +    if (m_source.requestFrameAsyncDecodingAtIndex(0, m_currentSubsamplingLevel, sizeForDrawing))
> +        LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8());

I would prefer not doing work inside the if (), the LOGging here too.

> Source/WebCore/rendering/RenderElement.cpp:1440
> +    LayoutRect backgroundPaintingRect = backgroundIsPaintedByRoot ? view().backgroundRect() : absoluteClippedOverflowRect();

absoluteClippedOverflowRect is expensive. We should keep an eye out for this being a perf bottleneck.

> Source/WebCore/rendering/RenderElement.cpp:1506
> +void RenderElement::unregisterForAsyncImageDecodingCallback()

It makes me nervous that we rely on HTMLImageElement() to call this to get unregistered from the RenderView. If there's any code path where that call doesn't happen, then we have a security bug. I would prefer any RenderElement that was registered is able to unregister itself on teardown.

> Source/WebCore/rendering/RenderElement.h:140
> +    bool intersects(const IntRect&) const;

This should be called intersectsAbsoluteRect or something that clarifies the coordinate system of the rect.

> Source/WebCore/rendering/RenderView.cpp:1430
> +    if (!m_largeImageRenderers.contains(&renderer))
> +        m_largeImageRenderers.add(&renderer);

Don't call contains before add. That's two hash lookups instead of one.

> Source/WebCore/rendering/RenderView.cpp:1436
> +    if (m_largeImageRenderers.contains(&renderer))
> +        m_largeImageRenderers.remove(&renderer);

Don't call contains just to remove! that's two hash lookups instead of one.

> Source/WebCore/rendering/RenderView.h:394
> +    HashSet<RenderElement*> m_largeImageRenderers;

The same "m_largeImageRenderers" should match the function names "registerForAsyncImageDecodingCallback", so this should be m_asyncDecodingImageRenderers.

> Source/WebCore/testing/Internals.cpp:478
> +    if (InternalSettings* settings = this->settings())
> +        settings->setLargeImageAsyncDecodingEnabled(false);

That's not the correct way to disable a setting for testing. You should do it via code in DRT/WTR which changes the WK1 or WK2 pref.

[Said Abou-Hallawa]

Created attachment 303772 [details]
Patch

[Said Abou-Hallawa]

Created attachment 303814 [details]
Patch

[Said Abou-Hallawa]

Created attachment 303849 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 303690 [details]
Patch for 165039 separate from 168814

View in context: https://bugs.webkit.org/attachment.cgi?id=303690&action=review

>> Source/WebCore/platform/graphics/BitmapImage.cpp:178
>> +            return;
> 
> Don't we want to paint the yellow debug color here too?

Yes. Fixed.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:409
>> +            imageObserver()->changedInRect(this, nullptr);
> 
> When is newFrameNativeImageAvailableAtIndex() called? If it's during painting, the repaint triggered by changedInRect() will get dropped on the floor.

newFrameNativeImageAvailableAtIndex() is called from the decoding thread via callOnMainThread(). So missing a repaint should not happen.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:422
>> +        LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8());
> 
> I would prefer not doing work inside the if (), the LOGging here too.

Done.

>> Source/WebCore/rendering/RenderElement.cpp:1506
>> +void RenderElement::unregisterForAsyncImageDecodingCallback()
> 
> It makes me nervous that we rely on HTMLImageElement() to call this to get unregistered from the RenderView. If there's any code path where that call doesn't happen, then we have a security bug. I would prefer any RenderElement that was registered is able to unregister itself on teardown.

Done. I added a call to unregisterForAsyncImageDecodingCallback(*this) in the destructor of RenderElement.

>> Source/WebCore/rendering/RenderElement.h:140
>> +    bool intersects(const IntRect&) const;
> 
> This should be called intersectsAbsoluteRect or something that clarifies the coordinate system of the rect.

Done. Function is renamed.

>> Source/WebCore/rendering/RenderView.cpp:1430
>> +        m_largeImageRenderers.add(&renderer);
> 
> Don't call contains before add. That's two hash lookups instead of one.

Done. Instead I added isRegisteredForAsyncImageDecodingCallback() to RenderObject().

>> Source/WebCore/rendering/RenderView.cpp:1436
>> +        m_largeImageRenderers.remove(&renderer);
> 
> Don't call contains just to remove! that's two hash lookups instead of one.

Done.

>> Source/WebCore/rendering/RenderView.h:394
>> +    HashSet<RenderElement*> m_largeImageRenderers;
> 
> The same "m_largeImageRenderers" should match the function names "registerForAsyncImageDecodingCallback", so this should be m_asyncDecodingImageRenderers.

Done. Member was renamed.

>> Source/WebCore/testing/Internals.cpp:478
>> +        settings->setLargeImageAsyncDecodingEnabled(false);
> 
> That's not the correct way to disable a setting for testing. You should do it via code in DRT/WTR which changes the WK1 or WK2 pref.

Done. DRT and WTR was changed to disable the large image decoding.

[Simon Fraser (smfr)]

Comment on attachment 303849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303849&action=review

> Source/WebCore/page/FrameView.cpp:1063
> +        requestAsyncDecodingForImagesInAbsoluteRectIncludingSubframes(tiledBacking->tileCoverageRect());

Please fix the comment in TiledBacking that says:

    // Exposed for testing
    virtual IntRect tileCoverageRect() const = 0;

> Source/WebCore/platform/graphics/BitmapImage.cpp:420
> +            LOG(Images, "BitmapImage::%s - %p - url: %s [earlyFrameCount = %ld nextFrame = %ld]", __FUNCTION__, this, sourceURL().characters8(), ++m_earlyFrameCount, index);

Better to use string.utf8().data().

> Source/WebCore/platform/graphics/BitmapImage.cpp:444
> +        LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8());

Better to use string.utf8().data().

> Source/WebCore/rendering/RenderView.cpp:1451
> +        float scale = frame().frameScaleFactor() * frame().pageZoomFactor() * document().deviceScaleFactor();
> +        if (frame().settings().delegatesPageScaling())
> +            scale *= frame().page()->pageScaleFactor();

frameScaleFactor() IS pageScaleFactor() if !delegatesPageScaling, so this is super confusing. I think you just want .page()->pageScaleFactor() * frame().pageZoomFactor() * document().deviceScaleFactor();

> Source/WebCore/rendering/RenderView.cpp:1452
> +        image->image()->requestAsyncDecoding(expandedIntSize(renderer->objectBoundingBox().size() * scale));

objectBoundingBox isn't the right box, and may be slightly different from the rendered size, leading to slight resizing artifacts. You need to use the rect that RenderImage::paintReplaced() uses.

In addition, you need to file a bug to track taking ancestor transforms into account, including ancestor SVG transforms.

I really think we should just delay this request until paint time to avoid having to do all the math again. I don't think we'll get it right doing it here.

[Said Abou-Hallawa]

Created attachment 303870 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 303849 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303849&action=review

>> Source/WebCore/platform/graphics/BitmapImage.cpp:420
>> +            LOG(Images, "BitmapImage::%s - %p - url: %s [earlyFrameCount = %ld nextFrame = %ld]", __FUNCTION__, this, sourceURL().characters8(), ++m_earlyFrameCount, index);
> 
> Better to use string.utf8().data().

Done.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:444
>> +        LOG(Images, "BitmapImage::%s - %p - url: %s", __FUNCTION__, this, sourceURL().characters8());
> 
> Better to use string.utf8().data().

Done.

>> Source/WebCore/rendering/RenderView.cpp:1451
>> +            scale *= frame().page()->pageScaleFactor();
> 
> frameScaleFactor() IS pageScaleFactor() if !delegatesPageScaling, so this is super confusing. I think you just want .page()->pageScaleFactor() * frame().pageZoomFactor() * document().deviceScaleFactor();

Done.

>> Source/WebCore/rendering/RenderView.cpp:1452
>> +        image->image()->requestAsyncDecoding(expandedIntSize(renderer->objectBoundingBox().size() * scale));
> 
> objectBoundingBox isn't the right box, and may be slightly different from the rendered size, leading to slight resizing artifacts. You need to use the rect that RenderImage::paintReplaced() uses.
> 
> In addition, you need to file a bug to track taking ancestor transforms into account, including ancestor SVG transforms.
> 
> I really think we should just delay this request until paint time to avoid having to do all the math again. I don't think we'll get it right doing it here.

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

[WebKit Commit Bot]

Comment on attachment 303870 [details]
Patch

Clearing flags on attachment: 303870

Committed r213618: <http://trac.webkit.org/changeset/213618>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 169475

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 304133 [details]
Patch

[Build Bot]

Comment on attachment 304133 [details]
Patch

Attachment 304133 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3289079

New failing tests:
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 304139 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 304133 [details]
Patch

Attachment 304133 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3289099

New failing tests:
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 304140 [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 304133 [details]
Patch

Attachment 304133 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3289102

New failing tests:
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 304141 [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 304143 [details]
Patch

[Said Abou-Hallawa]

Created attachment 304164 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 304164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304164&action=review

We need to test that the right thing happens for a directly composited image, but otherwise this looks OK.

> Source/WebCore/platform/graphics/BitmapImage.cpp:166
> +    m_sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size();

I trust this works for retina, page zoom etc?

> Source/WebCore/platform/graphics/BitmapImage.cpp:179
> +    if (isLargeImageAsyncDecodingRequired()) {

Not new to this patch, but the "required" sounds too strong here. Would read better as "shouldUseAsyncDecoding" or something.

> Source/WebCore/platform/graphics/BitmapImage.cpp:435
> +        if (m_needsRepaint) {

I'm not convinced of the necessity of having the m_needsRepaint member. When would newFrameNativeImageAvailableAtIndex() get called when you do *not* want to call changedInRect?

> Source/WebCore/platform/graphics/BitmapImage.cpp:440
> +        // Keep the number of decoding threads under control. 

This seems a bit vague. Maybe just remove the comment.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:384
> +        // CGImageSourceCreateThumbnailAtIndex() creates a bitmap context with the image native size
> +        // regardless of the subsamplingLevel unless kCGImageSourceSubsampleFactor is passed. Getting
> +        // frameSizeAtIndex(index, subsamplingLevel) will return irrelevant size to the size of image
> +        // that is going to be created so get frameSizeAtIndex() for SubsamplingLevel::Default and
> +        // compare it with sizeForDrawing.

I'm having a hard time understanding this comment. First, CGImageSourceCreateThumbnailAtIndex() doesn't crate a bitmap context, it creates a CGImageRef, and it sounds like correct behavior for it to use the native image size unless you specify kCGImageSourceSubsampleFactor. Does passing SubsamplingLevel::Default here just ensure that *our* code behaves as we want?

[Said Abou-Hallawa]

Created attachment 304176 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 304164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304164&action=review

>> Source/WebCore/platform/graphics/BitmapImage.cpp:166
>> +    m_sizeForDrawing = enclosingIntRect(context.getCTM().mapRect(destRect)).size();
> 
> I trust this works for retina, page zoom etc?

Yes this works with page zoom and view zoom correctly.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:179
>> +    if (isLargeImageAsyncDecodingRequired()) {
> 
> Not new to this patch, but the "required" sounds too strong here. Would read better as "shouldUseAsyncDecoding" or something.

Done. Functions were renamed.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:435
>> +        if (m_needsRepaint) {
> 
> I'm not convinced of the necessity of having the m_needsRepaint member. When would newFrameNativeImageAvailableAtIndex() get called when you do *not* want to call changedInRect?

Done. You are right m_needsRepaint is not needed.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:440
>> +        // Keep the number of decoding threads under control. 
> 
> This seems a bit vague. Maybe just remove the comment.

The comment was removed.

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:384
>> +        // compare it with sizeForDrawing.
> 
> I'm having a hard time understanding this comment. First, CGImageSourceCreateThumbnailAtIndex() doesn't crate a bitmap context, it creates a CGImageRef, and it sounds like correct behavior for it to use the native image size unless you specify kCGImageSourceSubsampleFactor. Does passing SubsamplingLevel::Default here just ensure that *our* code behaves as we want?

I changed the comment a little. What I meant here is the following:

CGImageSourceCreateThumbnailAtIndex() calls CGBitmapContextCreate() to create a CGContext with the image native size regardless of the subsamplingLevel unless kCGImageSourceSubsampleFactor is passed. It then calls CGBitmapContextCreateImage() to get the frame CGImage. Here we are trying to see which size is smaller: the image native size or the sizeForDrawing(). Assuming that frameSizeAtIndex(index, subsamplingLevel) will return the size which CGBitmapContextCreate() will be called with was wrong. To fix this issue we need to get frameSizeAtIndex() for SubsamplingLevel::Default and compare it the sizeForDrawing.

[Said Abou-Hallawa]

(In reply to comment #98)
> Comment on attachment 304164 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304164&action=review
> 
> We need to test that the right thing happens for a directly composited
> image, but otherwise this looks OK.
> 

Did you mean something like this?

<style>
    .transformed {
        transform: scale(2);
        transform-origin: top left;
        width: 400px;
        height: 400px;
    }
</style>
<body>
    <div class="transformed">
        <img src="image.jpg">
    </div>
</body>

I tested this case and the quality of the image is correct.

[Simon Fraser (smfr)]

(In reply to comment #101)
> (In reply to comment #98)
> > Comment on attachment 304164 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=304164&action=review
> > 
> > We need to test that the right thing happens for a directly composited
> > image, but otherwise this looks OK.
> > 
> 
> Did you mean something like this?
> 
> <style>
>     .transformed {
>         transform: scale(2);
>         transform-origin: top left;
>         width: 400px;
>         height: 400px;
>     }
> </style>
> <body>
>     <div class="transformed">
>         <img src="image.jpg">
>     </div>
> </body>


No, more like this:

<img src="image.jpg" style="will-change: transform">

[Simon Fraser (smfr)]

Comment on attachment 304176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304176&action=review

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:385
> +        // CGImageSourceCreateThumbnailAtIndex() returns a CGImage with the image native size
> +        // regardless of the subsamplingLevel unless kCGImageSourceSubsampleFactor is passed.
> +        // Here we are trying to see which size is smaller: the image native size or the
> +        // sizeForDrawing. If we want a CGImage with the image native size, sizeForDrawing will
> +        // not passed. So we need to get the image native size with the default subsampling and
> +        // then compare it with sizeForDrawing.

Much clearer, thanks!

[Said Abou-Hallawa]

(In reply to comment #102)
> (In reply to comment #101)
> > (In reply to comment #98)
> > > Comment on attachment 304164 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=304164&action=review
> > > 
> > > We need to test that the right thing happens for a directly composited
> > > image, but otherwise this looks OK.
> > > 
> > 
> > Did you mean something like this?
> > 
> > <style>
> >     .transformed {
> >         transform: scale(2);
> >         transform-origin: top left;
> >         width: 400px;
> >         height: 400px;
> >     }
> > </style>
> > <body>
> >     <div class="transformed">
> >         <img src="image.jpg">
> >     </div>
> > </body>
> 
> 
> No, more like this:
> 
> <img src="image.jpg" style="will-change: transform">

This scenario runs correctly but through the synchronous decoding path. RenderLayerBacking::updateImageContents() calls GraphicsLayerCA::setContentsToImage() which calls BitmapImage::nativeImageForCurrentFrame(). This later function forces synchronous when it calls frameImageAtIndex with a null sizeForDrawing.

[WebKit Commit Bot]

Comment on attachment 304176 [details]
Patch

Clearing flags on attachment: 304176

Committed r213764: <http://trac.webkit.org/changeset/213764>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to WebKit Commit Bot from comment #105)
> Comment on attachment 304176 [details]
> Patch
> 
> Clearing flags on attachment: 304176
> 
> Committed r213764: <http://trac.webkit.org/changeset/213764>

This caused bug 170107.