Remove BitmapImage::updateFromSettings() #22844

shallawa · 2024-01-17T01:45:22Z

`0c6288b`

Remove BitmapImage::updateFromSettings()
https://bugs.webkit.org/show_bug.cgi?id=267607
rdar://121077174

Reviewed by Cameron McCormack.

Calculate all the image painting options before calling BitmapImage::draw().
Remove all the calls to BitmapImage::updateFromSettings() and pass the calculated
AllowImageSubsampling and ShowDebugBackground through ImagePaintingOptions.

The first frame of an animated image had to be decoded synchronously. In fact
this restriction is not needed. The rules to decode the static image and the
first frame of an animated image should be the same.

Late decoded frames of an animated images was showing a yellow rectangle if
showDebugBackground is enabled. This is not really needed since the animation
does not advance until the next frame is fully decoded.

* Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:
(WebCore::CanvasRenderingContext2DBase::drawImage):
* Source/WebCore/platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::draw):
(WebCore::BitmapImage::shouldUseAsyncDecodingForAnimatedImages const):
(WebCore::BitmapImage::subsamplingLevelForScaleFactor):
(WebCore::BitmapImage::internalStartAnimation):
(WebCore::BitmapImage::advanceAnimation):
(WebCore::BitmapImage::decode):
(WebCore::BitmapImage::updateFromSettings): Deleted.
* Source/WebCore/platform/graphics/BitmapImage.h:
* Source/WebCore/platform/graphics/ImagePaintingOptions.h:
(WebCore::ImagePaintingOptions::allowImageSubsampling const):
(WebCore::ImagePaintingOptions::showDebugBackground const):
(WebCore::ImagePaintingOptions::setOption):
* Source/WebCore/platform/graphics/ImageTypes.h:
* Source/WebCore/rendering/BackgroundPainter.cpp:
(WebCore::BackgroundPainter::paintFillLayer):
* Source/WebCore/rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::decodingModeForImageDraw const):
* Source/WebCore/rendering/RenderImage.cpp:
(WebCore::RenderImage::paintIntoRect):
* Source/WebCore/rendering/svg/RenderSVGImage.cpp:
(WebCore::RenderSVGImage::paintIntoRect):
* Source/WebCore/testing/Internals.cpp:
(WebCore::Internals::imageFrameIndex):

Canonical link: https://commits.webkit.org/273528@main

8f3509f

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	~~🛠 tv~~	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	~~🛠 watch~~
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-01-17T01:45:38Z

EWS run on previous version of this PR (hash f54003d)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	❌ 🛠 ios	❌ 🛠 mac	❌ 🛠 wpe	⏳ 🛠 wincairo
❌ 🧪 bindings	❌ 🛠 ios-sim	⏳ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
❌ 🧪 webkitperl	❌ 🧪 ios-wk2	❌ 🧪 api-mac	❌ 🧪 api-wpe
	❌ 🧪 ios-wk2-wpt	❌ 🧪 mac-wk1	❌ 🛠 gtk
⏳ 🛠 🧪 jsc	❌ 🧪 api-ios	❌ 🧪 mac-wk2	❌ 🧪 gtk-wk2
	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	⏳ 🛠 tv-sim	❌ 🧪 mac-wk2-stress
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2024-01-17T02:03:03Z

Starting EWS tests for dc1efb9. Live statuses available at the PR page, #22844

webkit-early-warning-system · 2024-01-17T02:03:03Z

EWS run on previous version of this PR (hash dc1efb9)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	❌ 🛠 wpe	❌ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	❌ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	❌ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	❌ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-01-17T07:03:00Z

Starting EWS tests for bb150b2. Live statuses available at the PR page, #22844

webkit-early-warning-system · 2024-01-17T07:03:00Z

EWS run on previous version of this PR (hash bb150b2)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

heycam · 2024-01-24T06:15:47Z

Source/WebCore/platform/graphics/BitmapImage.cpp

@@ -393,7 +385,7 @@ bool BitmapImage::canUseAsyncDecodingForLargeImages() const

 bool BitmapImage::shouldUseAsyncDecodingForAnimatedImages() const
 {
-    return canAnimate() && m_allowAnimatedImageAsyncDecoding && (shouldUseAsyncDecodingForTesting() || m_source->canUseAsyncDecoding());


Can you explain why we no longer need m_allowAnimatedImageAsyncDecoding and no longer consult shouldUseAsyncDecodingForTesting()?

Because they are consulted in RenderBoxModelObject::decodingModeForImageDraw(). This makes the DecodingMode for the animated images and the large images are done the same way,

heycam · 2024-01-24T06:19:46Z

Source/WebCore/platform/graphics/BitmapImage.cpp

    LOG(Images, "BitmapImage::%s - %p - url: %s [subsamplingLevel = %d scaleFactorForDrawing = (%.4f, %.4f)]", __FUNCTION__, this, sourceURL().string().utf8().data(), static_cast<int>(m_currentSubsamplingLevel), scaleFactorForDrawing.width(), scaleFactorForDrawing.height());

    RefPtr<NativeImage> image;
-    if (options.decodingMode() == DecodingMode::Asynchronous) {
-        ASSERT(!m_currentFrame || !canAnimate());
+    if (!canAnimate() && options.decodingMode() == DecodingMode::Asynchronous) {


Can you explain this change, is it a change of behavior?

We used to pass DecodingMode::Synchronous for animated images always in RenderBoxModelObject::decodingModeForImageDraw()

auto* bitmapImage = dynamicDowncast<BitmapImage>(image); if (!bitmapImage || bitmapImage->canAnimate()) { // The DecodingMode for the current frame has to be Synchronous. The DecodingMode // for the next frame will be calculated in BitmapImage::internalStartAnimation(). return DecodingMode::Synchronous; }

This is to avoid going through the then part of this if-statement. The else part of this if- statement was handling the async for animated images through BitmapImage::internalStartAnimation()

if (shouldUseAsyncDecodingForAnimatedImages()) { ... m_source->requestFrameAsyncDecodingAtIndex }

This change fix this unclear behavior by making RenderBoxModelObject::decodingModeForImageDraw() passes the correct DecodingMode from the beginning. So this is not a behavior change.

heycam · 2024-01-24T06:22:09Z

Source/WebCore/platform/graphics/BitmapImage.cpp

-        // Force repaint if showDebugBackground() is on.
-        if (m_showDebugBackground)
-            imageObserver()->changedInRect(*this);


Can you explain why we no longer need this?

The animated image wants to advance to the next frame. But we do not advance to the next frame util it is fully decoded. In this case, the decoding takes too much time. The timer fire but the frame is not ready yet. So we have to wait till the decoding is complete and once we receive imageFrameAvailableAtIndex() from the work queue, we draw this frame.

The deleted code was showing a yellow rectangle in this case even though the current frame is rendered. So do we really need to show the yellow rectangle in this case? I do not think so. But I can put back.

OK, I agree we don't want the yellow rectangle in this case.

Since this is a behavior change, I think it is worth mentioning in the commit message.

heycam · 2024-01-24T06:25:30Z

Source/WebCore/rendering/RenderBoxModelObject.cpp

        return DecodingMode::Synchronous;
-    }

    // Some document types force synchronous decoding.


This comment should move up with the document().isImageDocument() check.

I do not quite understand your review here. I think we can even remove this comment since it does not add much.

heycam · 2024-01-24T06:29:02Z

Source/WebCore/rendering/RenderBoxModelObject.cpp

+        if (bitmapImage->currentFrameIndex())
+            return DecodingMode::Asynchronous;
+
+        return defaultDecodingMode();


To clarify, this is a behavior change for the first frame, is that right? Whereas before, we would always sync decode the first frame of an animated image, but now we will check e.g. if it's visible in the viewport to determine whether to sync decode?

If that's right, please mention this (and any other intended behavior changes) in the commit message.

Yes it is a behavior change. And I think it is the only behavior change in this PR. But I think it's the correct behavior. I think the old behavior was done because it was easier to do it this way. There should not be a reason for disallowing us from async decoding the first frame of an animated image.

I will add a comment about this in the commit message.

webkit-early-warning-system · 2024-01-25T18:06:27Z

EWS run on current version of this PR (hash 8f3509f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 wincairo~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	~~🛠 tv~~	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	~~🛠 watch~~
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=267607 rdar://121077174 Reviewed by Cameron McCormack. Calculate all the image painting options before calling BitmapImage::draw(). Remove all the calls to BitmapImage::updateFromSettings() and pass the calculated AllowImageSubsampling and ShowDebugBackground through ImagePaintingOptions. The first frame of an animated image had to be decoded synchronously. In fact this restriction is not needed. The rules to decode the static image and the first frame of an animated image should be the same. Late decoded frames of an animated images was showing a yellow rectangle if showDebugBackground is enabled. This is not really needed since the animation does not advance until the next frame is fully decoded. * Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp: (WebCore::CanvasRenderingContext2DBase::drawImage): * Source/WebCore/platform/graphics/BitmapImage.cpp: (WebCore::BitmapImage::draw): (WebCore::BitmapImage::shouldUseAsyncDecodingForAnimatedImages const): (WebCore::BitmapImage::subsamplingLevelForScaleFactor): (WebCore::BitmapImage::internalStartAnimation): (WebCore::BitmapImage::advanceAnimation): (WebCore::BitmapImage::decode): (WebCore::BitmapImage::updateFromSettings): Deleted. * Source/WebCore/platform/graphics/BitmapImage.h: * Source/WebCore/platform/graphics/ImagePaintingOptions.h: (WebCore::ImagePaintingOptions::allowImageSubsampling const): (WebCore::ImagePaintingOptions::showDebugBackground const): (WebCore::ImagePaintingOptions::setOption): * Source/WebCore/platform/graphics/ImageTypes.h: * Source/WebCore/rendering/BackgroundPainter.cpp: (WebCore::BackgroundPainter::paintFillLayer): * Source/WebCore/rendering/RenderBoxModelObject.cpp: (WebCore::RenderBoxModelObject::decodingModeForImageDraw const): * Source/WebCore/rendering/RenderImage.cpp: (WebCore::RenderImage::paintIntoRect): * Source/WebCore/rendering/svg/RenderSVGImage.cpp: (WebCore::RenderSVGImage::paintIntoRect): * Source/WebCore/testing/Internals.cpp: (WebCore::Internals::imageFrameIndex): Canonical link: https://commits.webkit.org/273528@main

webkit-commit-queue · 2024-01-25T20:16:18Z

Committed 273528@main (0c6288b): https://commits.webkit.org/273528@main

Reviewed commits have been landed. Closing PR #22844 and removing active labels.

shallawa requested review from cdumez and rniwa as code owners January 17, 2024 01:45

shallawa self-assigned this Jan 17, 2024

shallawa added the Images For bugs in image handling. label Jan 17, 2024

shallawa force-pushed the eng/Remove-BitmapImageupdateFromSettings branch from f54003d to dc1efb9 Compare January 17, 2024 02:02

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 17, 2024

shallawa removed the merging-blocked Applied to prevent a change from being merged label Jan 17, 2024

shallawa force-pushed the eng/Remove-BitmapImageupdateFromSettings branch from dc1efb9 to bb150b2 Compare January 17, 2024 07:02

shallawa requested review from smfr and heycam and removed request for rniwa and cdumez January 17, 2024 16:22

heycam reviewed Jan 24, 2024

View reviewed changes

heycam approved these changes Jan 25, 2024

View reviewed changes

shallawa force-pushed the eng/Remove-BitmapImageupdateFromSettings branch from bb150b2 to 8f3509f Compare January 25, 2024 18:01

shallawa added the merge-queue Applied to send a pull request to merge-queue label Jan 25, 2024

webkit-commit-queue force-pushed the eng/Remove-BitmapImageupdateFromSettings branch from 8f3509f to 4a88ce6 Compare January 25, 2024 20:12

webkit-commit-queue force-pushed the eng/Remove-BitmapImageupdateFromSettings branch from 4a88ce6 to 0c6288b Compare January 25, 2024 20:15

webkit-commit-queue merged commit 0c6288b into WebKit:main Jan 25, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 25, 2024

shallawa deleted the eng/Remove-BitmapImageupdateFromSettings branch February 1, 2024 05:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove BitmapImage::updateFromSettings() #22844

Remove BitmapImage::updateFromSettings() #22844

shallawa commented Jan 17, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 17, 2024 •

edited

webkit-early-warning-system commented Jan 17, 2024

webkit-early-warning-system commented Jan 17, 2024 •

edited

webkit-early-warning-system commented Jan 17, 2024

webkit-early-warning-system commented Jan 17, 2024 •

edited

heycam Jan 24, 2024

shallawa Jan 24, 2024

heycam Jan 24, 2024

shallawa Jan 24, 2024

heycam Jan 24, 2024

shallawa Jan 24, 2024

heycam Jan 25, 2024

heycam Jan 25, 2024

heycam Jan 24, 2024

shallawa Jan 24, 2024

heycam Jan 24, 2024

shallawa Jan 24, 2024

webkit-early-warning-system commented Jan 25, 2024 •

edited

webkit-commit-queue commented Jan 25, 2024

Remove BitmapImage::updateFromSettings() #22844

Remove BitmapImage::updateFromSettings() #22844

Conversation

shallawa commented Jan 17, 2024 • edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 17, 2024 • edited

webkit-early-warning-system commented Jan 17, 2024

webkit-early-warning-system commented Jan 17, 2024 • edited

webkit-early-warning-system commented Jan 17, 2024

webkit-early-warning-system commented Jan 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Jan 25, 2024 • edited

webkit-commit-queue commented Jan 25, 2024

shallawa commented Jan 17, 2024 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Jan 17, 2024 •

edited

webkit-early-warning-system commented Jan 17, 2024 •

edited

webkit-early-warning-system commented Jan 17, 2024 •

edited

webkit-early-warning-system commented Jan 25, 2024 •

edited