Bug 168814 – Asynchronous image decoding should consider the drawing size if it is smaller than the size of the image

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

[Said Abou-Hallawa]

If the size of the destinationRect is smaller than the image source size (e.g. 3000x3000 pixels), 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.

[Said Abou-Hallawa]

Created attachment 302625 [details]
Patch

[Said Abou-Hallawa]

Created attachment 302672 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 302672 [details]
Patch

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

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

Yes. Before <http://trac.webkit.org/changeset/209914>, CachedImage was inheriting from ImageObserver and all the virtual functions of ImageObserver were implemented inside CachedImage. After this change, CachedImage holds a RefPtr<CachedImageObserver> named m_imageObserver. This pointer is initialized in two places. (1) CachedImage::createImage(): this function creates a new m_image and the m_imageObserver is also created. And this (the CachedImage) is added to m_imageObserver. (2) CachedImage::setBodyDataFrom(): in this function m_image is cloned from the other CachedImage to this CachedImage. The m_imageObserver also references the m_imageObserver of the other CachedImage. Then this (the CachedImage) is added to m_imageObserver.

[Simon Fraser (smfr)]

Comment on attachment 302672 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        -- if (sizeForDrawing && sizeForDrawing.value().isZero()): async image decoding
> +           is needed but do not set kCGImageSourceThumbnailMaxPixelSize. This is the
> +           case where destinationRect.size() is larger than the image sourceSize.
> +        -- if (sizeForDrawing && !sizeForDrawing.value().isZero()): async image decoding
> +           is needed and kCGImageSourceThumbnailMaxPixelSize has to be set. The is the
> +           case where destinationRect.size() is smaller than the image sourceSize.

Do you really need to differentiate between these two states? Why not just always pass down sizeForDrawing, and make the determination based on that vs. the native size of the image?

That would avoid this confusing 3-state system.

[Said Abou-Hallawa]

Created attachment 302709 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 302709 [details]
Patch

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

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

Can this be const?

It can but it will be very involving because BitmapImage::canAnimate() is not const. It is not const because it calls BitmapImage::frameCount() which is not const. BitmapImage::frameCount() is not const because ImageFrameCache::frameCount() has to cache the ImageFrameCache::m_frameCount if it is not set.

This a general problem with most of the query functions in BitmapImage, ImageSource and ImageFrameCache. Most of the properties are lazily calculated in these query functions. So either, we keep these query functions non-const. Or we make all the image properties mutable.

[Build Bot]

Comment on attachment 302709 [details]
Patch

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

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag.html
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag-is-prevented-over-button.html

[Build Bot]

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

[Radar WebKit Bug Importer]

<rdar://problem/30742918>

[Said Abou-Hallawa]

Created attachment 302908 [details]
Patch

[Said Abou-Hallawa]

Created attachment 302947 [details]
Patch

[Tim Horton]

Comment on attachment 302947 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.h:144
> +        IntSize sizeForDraw;

I would vote for "sizeForDrawing" or...  something else, this sounds weird.

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:88
> -    static NeverDestroyed<RetainPtr<CFDictionaryRef>> optionsOnDemand = createImageSourceOptions(SubsamplingLevel::First, DecodingMode::OnDemand);
> -    static NeverDestroyed<RetainPtr<CFDictionaryRef>> optionsImmediate = createImageSourceOptions(SubsamplingLevel::First, DecodingMode::Immediate);
> +static RetainPtr<CFMutableDictionaryRef> imageSourceAsyncOptions(SubsamplingLevel subsamplingLevel)

So now we're building CFDictionaries all over the place, where they used to be static? That seems not great.

> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:183
> -NativeImagePtr ImageDecoder::createFrameImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel, DecodingMode) const
> +NativeImagePtr ImageDecoder::createFrameImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel, , const std::optional<IntSize>&) const

This doesn't look right (, ,)

[Said Abou-Hallawa]

Created attachment 303040 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 302947 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageFrameCache.h:144
>> +        IntSize sizeForDraw;
> 
> I would vote for "sizeForDrawing" or...  something else, this sounds weird.

Done.

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:88
>> +static RetainPtr<CFMutableDictionaryRef> imageSourceAsyncOptions(SubsamplingLevel subsamplingLevel)
> 
> So now we're building CFDictionaries all over the place, where they used to be static? That seems not great.

I think I fixed this issue in the new patch.

>> Source/WebCore/platform/graphics/win/ImageDecoderDirect2D.cpp:183
>> +NativeImagePtr ImageDecoder::createFrameImageAtIndex(size_t index, SubsamplingLevel subsamplingLevel, , const std::optional<IntSize>&) const
> 
> This doesn't look right (, ,)

Fixed. There is not a port to build Windows D2D so EWS did not catch it.

[Said Abou-Hallawa]

Created attachment 303073 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 303073 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:389
> +    RetainPtr<CGImageRef> image = adoptCF(CGImageSourceCreateImageAtIndex(m_nativeDecoder.get(), index, options.get()));

This is wrong. I deleted the call to CGImageSourceCreateThumbnailAtIndex by mistake. This will disable the async image decoding completely. There should be a call to CGImageSourceCreateImageAtIndex() inside the if (!sizeForDrawing) above. And another call to CGImageSourceCreateThumbnailAtIndex() should be inside the else block.

[Simon Fraser (smfr)]

(In reply to comment #16)
 This will disable the async image decoding completely.

Why didn't tests fail then?

[Said Abou-Hallawa]

Created attachment 303090 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 303090 [details]
Patch

I made sure the animated images are decoded asynchronously and the frame rate is high as without this patch.

[Said Abou-Hallawa]

(In reply to comment #17)
> (In reply to comment #16)
>  This will disable the async image decoding completely.
> 
> Why didn't tests fail then?

Because the current tests for animated image can only verify the correctness of the async image decoding path. These tests force async image decoding since the images they use are small and simple ones. They also set the m_desiredFrameDecodeTimeForTesting to fake a lengthy decoding time. The tests expect to get the frame drawn correctly but they do not test the frame is drawn because it went through a specific code path.

They do not test if we used CGImageSourceCreateThumbnailAtIndex() instead of CGImageSourceCreateImageAtIndex() in the async image decoding path. What makes it difficult to do such test is the difference between decoding a simple small image using one of these two functions and then drawing it can hardly be noticeable. Using CGImageSourceCreateImageAtIndex() to decode an image asynchronously will still generate a valid CGImageRef. The only problem here is the real deducing will happen later at the drawing time.

[Said Abou-Hallawa]

Created attachment 303487 [details]
Patch

[Build Bot]

Comment on attachment 303487 [details]
Patch

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

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html

[Build Bot]

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

[Simon Fraser (smfr)]

Comment on attachment 303487 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:245
> +    if (!((canAnimate() && allowAnimatedImageAsyncDecoding()) || (!canAnimate() && allowLargeImageAsyncDecoding())))

I feel this could be simplified.

> Source/WebCore/platform/graphics/ImageFrame.h:151
> +    std::optional<IntSize> m_sizeForDrawing;

Thinking about it some more, I think this is a fundamentally wrong change. I think we need to preserve the existing behavior that BitmapImage is agnostic to the size at which it's going to get pained.

Tim points out that CachedImage has something relevant: CachedImage::setContainerSizeForRenderer().

[Said Abou-Hallawa]

Created attachment 303593 [details]
Patch

[Said Abou-Hallawa]

Created attachment 303686 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 303487 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.cpp:245
>> +    if (!((canAnimate() && allowAnimatedImageAsyncDecoding()) || (!canAnimate() && allowLargeImageAsyncDecoding())))
> 
> I feel this could be simplified.

I replaced this function by the two functions to removed the complication.

bool BitmapImage::isLargeImageAsyncDecodingRequired()
{
    return !canAnimate() && allowLargeImageAsyncDecoding() && (isAsyncDecodingForcedForTesting() || m_source.isAsyncDecodingRequired());
}

bool BitmapImage::isAnimatedImageAsyncDecodingRequired()
{
    return canAnimate() && allowAnimatedImageAsyncDecoding() && (isAsyncDecodingForcedForTesting() || m_source.isAsyncDecodingRequired());
}

>> Source/WebCore/platform/graphics/ImageFrame.h:151
>> +    std::optional<IntSize> m_sizeForDrawing;
> 
> Thinking about it some more, I think this is a fundamentally wrong change. I think we need to preserve the existing behavior that BitmapImage is agnostic to the size at which it's going to get pained.
> 
> Tim points out that CachedImage has something relevant: CachedImage::setContainerSizeForRenderer().

setContainerSizeForRenderer() is mainly used by the SVG to resize the FrameView.

For the BitmapImage, I think the sizeForDrawing is hidden by the ImageFrame. The BitmapImage still returns the native size of the image when it is requested. The ImageFrame is just transient thing. It can be deleted at any point if we go under memory stress for example. It knows when and how to rebuild itself. When drawing for a specific size, it tries to do some optimization but it gives the highest quality of the image. If BitmapImage tries to draw for a size whose MaxPixelSize is larger the MaxPixelSize of the ImageFrame sizeForDrawing, ImageFrame rebuilds a new NativeImage with the larger MaxPixelSize.

[Simon Fraser (smfr)]

Comment on attachment 303686 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        (e.g. 3000x3000 pixels), CGImageSourceCreateThumbnailAtIndex() is slower

Because the thumbnail is decoding at the native size of the image? Doesn't that also imply higher memory use?

> Source/WebCore/ChangeLog:17
> +        all the way from BitmapImage till ImageDecoder. If bool(sizeForDrawing) equals

till -> to

> Source/WebCore/ChangeLog:19
> +        asynchronously.

synchronously?

> Source/WebCore/platform/graphics/BitmapImage.cpp:319
> +        if (m_source.requestFrameAsyncDecodingAtIndex(nextFrame, m_currentSubsamplingLevel, *m_sizeForDrawing))
> +            LOG(Images, "BitmapImage::%s - %p - url: %s [requesting async decoding for nextFrame = %ld]", __FUNCTION__, this, sourceURL().characters8(), nextFrame);
> +        else
> +            LOG(Images, "BitmapImage::%s - %p - url: %s [cachedFrameCount = %ld nextFrame = %ld]", __FUNCTION__, this, sourceURL().characters8(), ++m_cachedFrameCount, nextFrame);

It's a bit odd to have the main decode happen in an if () statement whose result is only used for logging. This would be better as:

bool isAsyncDecode  = m_source.requestFrameAsyncDecodingAtIndex(nextFrame, m_currentSubsamplingLevel, *m_sizeForDrawing);
LOG(.... , isAsyncDecode, ...)

with the appropriate UNUSED_PARAM if LOG is not defined.

> Source/WebCore/platform/graphics/ImageFrame.cpp:128
> +static int maxPixelSize(const IntSize& size)

I would call this maxDimension or largerOfWidthAndHeight

> Source/WebCore/platform/graphics/ImageFrame.cpp:141
> +    return maxPixelSize(m_sizeForDecoding.last()) >= maxPixelSize(*sizeForDrawing);

Seems odd to compare a tall skinny image with a short, wide one. We may have to enhance this in future.

[Said Abou-Hallawa]

Created attachment 303754 [details]
Patch

[Build Bot]

Comment on attachment 303754 [details]
Patch

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

New failing tests:
media/track/track-in-band-style.html

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 303762 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 303762 [details]
Patch

Clearing flags on attachment: 303762

Committed r213563: <http://trac.webkit.org/changeset/213563>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.