Skip to content

REGRESSION(283753@main): Video as image source with decoding="async" is decoded always synchronously #47266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 27, 2025
Merged

REGRESSION(283753@main): Video as image source with decoding="async" is decoded always synchronously #47266

merged 1 commit into from
Jun 27, 2025

Conversation

shallawa
Copy link
Contributor

@shallawa shallawa commented Jun 26, 2025 •
edited by webkit-early-warning-system
Loading

f12f3a3

REGRESSION(283753@main): Video as image source with `decoding="async"` is decoded always synchronously
https://bugs.webkit.org/show_bug.cgi?id=295066
rdar://153915118

Reviewed by Simon Fraser.

The bug is in RenderBoxModelObject::decodingModeForImageDraw() when we choose the
DecodingMode for the image and the element has the attribute decoding="async”.
In this case we try to be smart and override the user choice if it is going to
cause flickering. We check if the element is in the viewport and if it is, we do
use DecodingMode::Synchronous.

But this should logic not be applied to animated images (or videos as images)
because for animated images we calculate the decoding mode for the next frame.

* Source/WebCore/platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::draw):
* Source/WebCore/rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::decodingModeForImageDraw const):

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

26afd37

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@shallawa shallawa self-assigned this Jun 26, 2025
@shallawa shallawa added the Images For bugs in image handling. label Jun 26, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 26, 2025 •
edited
Loading

EWS run on current version of this PR (hash 26afd37)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@shallawa shallawa requested review from smfr and heycam June 26, 2025 22:52
smfr
smfr approved these changes Jun 26, 2025
View reviewed changes
Source/WebCore/rendering/RenderBoxModelObject.cpp
@@ -344,7 +344,7 @@ DecodingMode RenderBoxModelObject::decodingModeForImageDraw(const Image& image,
// <img decoding="async"> forces asynchronous decoding but make sure this
// will not cause flickering.
if (imgElement->decodingMode() == DecodingMode::Asynchronous) {
if (bitmapImage->isAsyncDecodingEnabledForTesting())
if (bitmapImage->isAsyncDecodingEnabledForTesting() || bitmapImage->isAnimated())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK to change behavior for animated GIF/APNG etc?

Copy link
Contributor Author

@shallawa shallawa Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is restoring the code to what it was before 283753@main. If the page specifies decoding="async" for an animated image, I think we should respect this choice since it isn't going to cause any flickering. Please notice that for animated images, the returned DecodingMode here is for the next frame (not the current one).

For async animated image decoding, we always display the current frame which we ensure it is cached. The animation is advanced only when the frame finishes decoding. The image is repainted when the animation is advanced.

@shallawa shallawa added the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
@shallawa
REGRESSION(283753@main): Video as image source with `decoding="async"…
f12f3a3 
…` is decoded always synchronously

https://bugs.webkit.org/show_bug.cgi?id=295066
rdar://153915118

Reviewed by Simon Fraser.

The bug is in RenderBoxModelObject::decodingModeForImageDraw() when we choose the
DecodingMode for the image and the element has the attribute decoding="async”.
In this case we try to be smart and override the user choice if it is going to
cause flickering. We check if the element is in the viewport and if it is, we do
use DecodingMode::Synchronous.

But this should logic not be applied to animated images (or videos as images)
because for animated images we calculate the decoding mode for the next frame.

* Source/WebCore/platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::draw):
* Source/WebCore/rendering/RenderBoxModelObject.cpp:
(WebCore::RenderBoxModelObject::decodingModeForImageDraw const):

Canonical link: https://commits.webkit.org/296706@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/REGRESSION-283753-main-Video-as-image-source-with-decoding-async-is-decoded-always-synchronously branch from 26afd37 to f12f3a3 Compare June 27, 2025 04:05
@webkit-commit-queue
Copy link
Collaborator

Committed 296706@main (f12f3a3): https://commits.webkit.org/296706@main

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

@webkit-commit-queue webkit-commit-queue merged commit f12f3a3 into WebKit:main Jun 27, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smfr smfr smfr approved these changes

@heycam heycam Awaiting requested review from heycam

Assignees

@shallawa shallawa

Labels
Images For bugs in image handling.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shallawa @webkit-early-warning-system @webkit-commit-queue @smfr