Skip to content

Animated GIF freezes when presentation attributes are changed on SVG image referenced by use element#64180

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
nullhook:svg-use-gif-animation
May 20, 2026
Merged

Animated GIF freezes when presentation attributes are changed on SVG image referenced by use element#64180
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
nullhook:svg-use-gif-animation

Conversation

@nullhook
Copy link
Copy Markdown
Contributor

@nullhook nullhook commented May 4, 2026

0eadba6

Animated GIF freezes when presentation attributes are changed on SVG image referenced by use element
https://bugs.webkit.org/show_bug.cgi?id=314093
rdar://96837306

Reviewed by Said Abou-Hallawa.

When an SVG <image> element displaying an animated GIF is referenced by a
<use> element, rapidly changing a presentation attribute like opacity causes
the animation to freeze.

Each setAttribute call triggers invalidateInstances(), which rebuilds the
<use> shadow tree. The clone's renderer destruction calls
RenderImageResource::willBeDestroyed(), which unconditionally calls
stopAnimation() on the shared BitmapImage. Since the animation timer is
killed faster than the frame duration, it never fires to advance frames.

* LayoutTests/svg/custom/resources/animated-gif-for-use-test.gif: Added.
* LayoutTests/svg/custom/use-animated-image-opacity-change-expected.txt: Added.
* LayoutTests/svg/custom/use-animated-image-opacity-change.html: Added.
* Source/WebCore/rendering/RenderImageResource.cpp:
(WebCore::RenderImageResource::willBeDestroyed):

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

6085746

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ✅ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ✅ 🛠 vision-apple
🧪 ios-wk2-wpt ✅ 🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
❌ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 ios-safer-cpp ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🛠 playstation
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim ✅ 🧪 mac-site-isolation
✅ 🛠 watch
✅ 🛠 watch-sim

@nullhook nullhook self-assigned this May 4, 2026
@nullhook nullhook marked this pull request as draft May 4, 2026 17:12
@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

iOS Safer C++ Build #18044 (0a6626a)

❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 4, 2026
@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

macOS Safer C++ Build #99707 (0a6626a)

❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 5, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 0a6626a to 2afe75f Compare May 5, 2026 15:37
@nullhook nullhook added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label May 5, 2026
@nullhook nullhook requested review from anttijk, shallawa and smfr May 5, 2026 15:42
@nullhook nullhook marked this pull request as ready for review May 5, 2026 15:42
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 2afe75f to 03c5643 Compare May 5, 2026 15:56
@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

iOS Safer C++ Build #18792 (2afe75f)

❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

macOS Safer C++ Build #100393 (2afe75f)

❌ Found 1 failing file with 1 issue. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 5, 2026
Comment thread LayoutTests/svg/custom/use-animated-image-opacity-change.html Outdated
Comment thread LayoutTests/svg/custom/use-animated-image-opacity-change.html Outdated
Comment thread LayoutTests/svg/custom/use-animated-image-opacity-change.html Outdated
Comment thread LayoutTests/svg/custom/use-animated-image-opacity-change.html Outdated
Comment thread LayoutTests/svg/custom/use-animated-image-opacity-change.html Outdated
Comment thread LayoutTests/svg/custom/use-animated-image-opacity-change.html Outdated
Comment thread Source/WebCore/rendering/RenderImageResource.cpp Outdated
Comment thread Source/WebCore/testing/Internals.cpp Outdated
Comment thread Source/WebCore/testing/Internals.cpp Outdated
Comment thread Source/WebCore/testing/Internals.cpp Outdated
{
CachedImage* cachedImage = nullptr;
if (auto* htmlImage = dynamicDowncast<HTMLImageElement>(element))
cachedImage = htmlImage->cachedImage();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is awkward. We still have the other function imageFromImageElement(HTMLImageElement& element). Since the code is repeated for HTMLImageElement and SVGImageElement, cannot we use templates?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think templates help here. However, we can replace imageFromImageElement and have everything go through the new imageFromElement

@nullhook
Copy link
Copy Markdown
Contributor Author

nullhook commented May 6, 2026

One thing I've observed is that a single <img> element’s ImageLoader and its Renderer both act as CachedImage clients. As a result, when reading numberOfClients, there’s no way to distinguish between them.

This matters because if you remove a single <img> that's animating, our guard cached->numberOfClients() <= 1 is false (ImageLoader + Renderer = 2) and we'd incorrectly skip stopAnimation()

edit: I've solved this by removing the renderer as client first and then checking numberOfClients() - It still seems like an approximation but the right fix would be to return number of renderers only

@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 6, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 03c5643 to a772916 Compare May 6, 2026 15:02
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 6, 2026
@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 7, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from a772916 to 40f4e66 Compare May 7, 2026 21:35
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 40f4e66 to 6df3aab Compare May 11, 2026 15:47
@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 13, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 6df3aab to 2de6cac Compare May 13, 2026 17:11
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 13, 2026
@nullhook nullhook requested a review from shallawa May 13, 2026 20:51
Comment thread Source/WebCore/testing/Internals.cpp Outdated
Comment thread Source/WebCore/rendering/RenderImageResource.cpp Outdated
@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 14, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 2de6cac to 76498b6 Compare May 14, 2026 13:33
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 14, 2026
@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 15, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 76498b6 to 03c0aad Compare May 15, 2026 18:45
@nullhook nullhook requested a review from cdumez as a code owner May 15, 2026 18:45
@nullhook nullhook requested a review from shallawa May 15, 2026 18:45
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 15, 2026
Comment thread Source/WebCore/rendering/RenderImageResource.cpp Outdated
@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 18, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 03c0aad to 875a2bd Compare May 18, 2026 20:22
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 18, 2026
@nullhook nullhook removed the merging-blocked Applied to prevent a change from being merged label May 19, 2026
@nullhook nullhook force-pushed the svg-use-gif-animation branch from 875a2bd to 6085746 Compare May 19, 2026 18:24
@nullhook nullhook added the merge-queue Applied to send a pull request to merge-queue label May 20, 2026
…image referenced by use element

https://bugs.webkit.org/show_bug.cgi?id=314093
rdar://96837306

Reviewed by Said Abou-Hallawa.

When an SVG <image> element displaying an animated GIF is referenced by a
<use> element, rapidly changing a presentation attribute like opacity causes
the animation to freeze.

Each setAttribute call triggers invalidateInstances(), which rebuilds the
<use> shadow tree. The clone's renderer destruction calls
RenderImageResource::willBeDestroyed(), which unconditionally calls
stopAnimation() on the shared BitmapImage. Since the animation timer is
killed faster than the frame duration, it never fires to advance frames.

* LayoutTests/svg/custom/resources/animated-gif-for-use-test.gif: Added.
* LayoutTests/svg/custom/use-animated-image-opacity-change-expected.txt: Added.
* LayoutTests/svg/custom/use-animated-image-opacity-change.html: Added.
* Source/WebCore/rendering/RenderImageResource.cpp:
(WebCore::RenderImageResource::willBeDestroyed):

Canonical link: https://commits.webkit.org/313531@main
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 313531@main (0eadba6): https://commits.webkit.org/313531@main

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

@webkit-commit-queue webkit-commit-queue merged commit 0eadba6 into WebKit:main May 20, 2026
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New Bugs Unclassified bugs are placed in this component until the correct component can be determined.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants