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.
Created attachment 302625 [details] Patch
Created attachment 302672 [details] Patch
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.
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.
Created attachment 302709 [details] Patch
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.
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
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
<rdar://problem/30742918>
Created attachment 302908 [details] Patch
Created attachment 302947 [details] Patch
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 (, ,)
Created attachment 303040 [details] Patch
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.
Created attachment 303073 [details] Patch
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.
(In reply to comment #16) This will disable the async image decoding completely. Why didn't tests fail then?
Created attachment 303090 [details] Patch
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.
(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.
Created attachment 303487 [details] Patch
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
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
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().
Created attachment 303593 [details] Patch
Created attachment 303686 [details] Patch
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.
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.
Created attachment 303754 [details] Patch
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
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
Created attachment 303762 [details] Patch
Comment on attachment 303762 [details] Patch Clearing flags on attachment: 303762 Committed r213563: <http://trac.webkit.org/changeset/213563>
All reviewed patches have been landed. Closing bug.