Bug 244128 – Add experimental feature to control image animation play/pause state

• Component: Accessibility
• Status: RESOLVED FIXED

[Joshua Hoffman]

Provide a setting to disable image animations, while also adding some new interfaces to control different animations at an individual and page-level.

[Radar WebKit Bug Importer]

<rdar://problem/98887677>

[Joshua Hoffman]

Created attachment 461738 [details]
Patch

[chris fleizach]

Comment on attachment 461738 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:787
> +    if (auto* cachedImage = this->cachedImage()) {

looks like a lot of duplicated code here - can you condense into helper methods

> Source/WebCore/svg/graphics/SVGImage.cpp:382
> +    if (!rootElement || !rootElement->animationsPaused() || m_page->settings().animatedImagesDisabled())

do we need to check m_page for not nil?

> Source/WebCore/testing/Internals.cpp:1061
> +    Document* document = contextDocument();

auto*

> Source/WebCore/testing/Internals.cpp:1065
> +    Page* page = document->page();

auto*

[Tyler Wilcock]

Comment on attachment 461738 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:509
> +        m_modelPlayer->setAnimationIsPlaying(false, [] (bool success) mutable {

Not 100% sure this will compile, but can you do this:

m_modelPlayer->setAnimationIsPlaying(false, [] (bool) mutable { }

(i.e. not assigning a name to the bool, and therefore not needing UNUSED_PARAM)?

> Source/WebCore/html/HTMLImageElement.cpp:68
> +#include "UserAgentStyleSheets.h"

You added a bunch of new includes in this file -- are they all necessary in this patch? (I know you've split your original patch up intentionally, so maybe artifacts of that?)

> Source/WebCore/html/HTMLImageElement.cpp:425
> +            if (is<BitmapImage>(*image)) {

This can be:

if (auto* bitmapImage = dynamicDowncast<BitmapImage>(image))
    bitmapImage->updateDocumentOnRender(document());

dynamicDowncast will return a nullptr if the object isn't the expected type.

> Source/WebCore/page/Page.cpp:4037
> +            if (is<HTMLImageElement>(element)) {

I think you can use dynamicDowncasts here too.

> Source/WebCore/page/Page.cpp:4057
> +                    if (auto cachedBackgroundImage = renderer->style().backgroundLayers().image()->cachedImage()) {

I believe:

renderer->style().backgroundLayers().image()->cachedImage()

Returns a CachedImage*, so you should use auto* instead of auto.

> Source/WebCore/page/Page.cpp:4058
> +                        if (auto backgroundImage = cachedBackgroundImage->image()) {

I think cachedBackgroundImage->image() also returns a pointer type, so this should also be auto*.

(If any of these return a RefPtr<T> rather than T*, then I believe just `auto` is fine)

> Source/WebCore/page/Page.cpp:4059
> +                            if (backgroundImage && is<BitmapImage>(backgroundImage)) {

I think is<T> returns false for nullptr objects, so the null check is unnecessary.

[Tyler Wilcock]

Comment on attachment 461738 [details]
Patch

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

> COMMIT_MESSAGE:3
> +

It has been requested that the Radar number be included underneath the bugzilla URL. If you rerun `git webkit setup`, this will be part of the commit message template.

> Source/WebCore/platform/graphics/Image.h:147
> +    WEBCORE_EXPORT virtual bool isAnimating() const;

Looks like you added an implementation to Image.cpp of this:

bool Image::isAnimating() const
{
    return false;
}

Is that necessary, or can we just keep what exists today but with the WEBCORE_EXPORT added? i.e.:

WEBCORE_EXPORT virtual bool isAnimating() const { return false };

[Chris Dumez]

Comment on attachment 461738 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:77
> +    m_disableImageAnimation = document.settings().animatedImagesDisabled();

Should be part of the static initializer list. Also, per coding style, since this is a boolean variable, it needs a prefix, for e.g. `m_shouldDisableImageAnimation`.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:509
> +        m_modelPlayer->setAnimationIsPlaying(false, [] (bool success) mutable {

Why is the lambda mutable?

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:535
> +    m_allowIndividualAnimation = true;

Missing prefix for boolean variables (per coding style).

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:541
> +    m_modelPlayer->setAnimationIsPlaying(false, [] (bool success) mutable {

Why mutable?

> Source/WebCore/html/HTMLImageElement.cpp:424
> +        if (auto* image = cachedImage->image()) {

if (auto* image = dynamicDowncast<BitmapImage>(cachedImage->image()))
    image->updateDocumentOnRender(document());

>> Source/WebCore/html/HTMLImageElement.cpp:787
>> +    if (auto* cachedImage = this->cachedImage()) {
> 
> looks like a lot of duplicated code here - can you condense into helper methods

I agree it would be nice to add something like a cachedBitmapImage() getter on HTMLImageElement.

> Source/WebCore/html/HTMLImageElement.cpp:789
> +            if (is<BitmapImage>(*image)) {

Should use dynamicDowncast like above.

> Source/WebCore/html/HTMLImageElement.cpp:802
> +            if (is<BitmapImage>(*image)) {

ditto.

> Source/WebCore/html/HTMLImageElement.cpp:815
> +        if (auto* image = cachedImage->image()) {

ditto.

> Source/WebCore/html/HTMLImageElement.cpp:1020
> +            if (is<BitmapImage>(*image)) {

ditto

> Source/WebCore/html/HTMLImageElement.h:160
> +    bool isAnimating();

Can this be const?

> Source/WebCore/html/HTMLImageElement.h:161
> +    bool isAnimatedImage();

ditto

> Source/WebCore/page/Page.cpp:4036
> +        for (auto& element : descendantsOfType<Element>(const_cast<ContainerNode&>(document.rootNode()))) {

I think that document.rootNode() is always the document itself.

> Source/WebCore/page/Page.cpp:4037
> +            if (is<HTMLImageElement>(element)) {

Could use dynamicDowncast.

> Source/WebCore/page/Page.cpp:4043
> +            if (is<SVGSMILElement>(element)) {

else if ?

Could use dynamicDowncast.

> Source/WebCore/page/Page.cpp:4049
> +            if (is<HTMLModelElement>(element)) {

else if ?

Could use dynamicDowncast.

> Source/WebCore/page/Page.cpp:4059
> +                            if (backgroundImage && is<BitmapImage>(backgroundImage)) {

is<> already does the null check for you. Also, can probably use dynamicDowncast()

> Source/WebCore/page/Page.cpp:4079
> +        for (auto& element : descendantsOfType<Element>(const_cast<ContainerNode&>(document.rootNode()))) {

document.rootNode() is the document AFAIK

> Source/WebCore/page/Page.cpp:4109
> +bool Page::isAllAnimationPaused()

Should probably be inlined in the header given how trivial it is.

Also isAllAnimationPaused -> areAllAnimationsPaused?

> Source/WebCore/page/Page.h:1349
> +    bool m_pageAnimationsUnpaused { false };

m_hasUnpausedAnimation? The name is a bit confusing at the moment.

> Source/WebCore/platform/graphics/BitmapImage.h:166
> +    bool isAnimationFinished() { return m_animationFinished; };

Should be const:
`bool isAnimationFinished() const { return m_animationFinished; };`

> Source/WebCore/platform/graphics/BitmapImage.h:256
> +    bool m_disableImageAnimation { false };

Missing prefix.

> Source/WebCore/platform/graphics/BitmapImage.h:257
> +    bool m_allowIndividualAnimation { false };

Missing prefix.

> Source/WebCore/svg/SVGElement.cpp:163
> +    , m_allowIndividualAnimation(false)

Please use inline initialization in the header.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:1245
> +    m_allowIndividualAnimation = !m_allowIndividualAnimation;

= true; would be clearer I think.

> Source/WebCore/svg/animation/SVGSMILElement.cpp:1253
> +    m_allowIndividualAnimation = !m_allowIndividualAnimation;

= false; would be clearer I think.

> LayoutTests/accessibility/mac/disable-animations-enabled.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/accessibility/mac/disable-animations-enabled.html:7
> +<body id="body">

id seems unnecessary.

> LayoutTests/accessibility/mac/disable-animations-enabled.html:9
> +    jsTestIsAsync = true;

Please add a `description("This tests that animations are disabled when the disable animated images setting is turned on.");` call before this.

> LayoutTests/accessibility/mac/disable-animations-enabled.html:13
> +        debug("This tests that animations are disabled when the disable animated images setting is turned on.");		

And drop this.

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:7
> +<body id="body">

id seems unnecessary?

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:14
> +        debug("This tests that when image animation is disabled, they can be toggled on and off individually.");		

Same comment about using description() instead of debug for this.

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:27
> +<script src="../../resources/js-test-post.js"></script>

Not needed.

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:7
> +<body id="body">

unnecessary id?

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:15
> +        debug("This tests that when image animation is disabled, all animations on a page can be toggled on and off.");		

Please use description()

> LayoutTests/accessibility/mac/disable-animations-toggle-page-animations.html:29
> +<script src="../../resources/js-test-post.js"></script>

Not needed

[Joshua Hoffman]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 461738 [details]
> Patch

Thanks for all of the comments!

> 
> > Source/WebCore/page/Page.cpp:4109
> > +bool Page::isAllAnimationPaused()
> 
> Should probably be inlined in the header given how trivial it is.
> 

The style checker gave me an error earlier when trying to WEBCORE_EXPORT an inline method. Is this expected?

[Chris Dumez]

(In reply to Joshua Hoffman from comment #7)
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 461738 [details]
> > Patch
> 
> Thanks for all of the comments!
> 
> > 
> > > Source/WebCore/page/Page.cpp:4109
> > > +bool Page::isAllAnimationPaused()
> > 
> > Should probably be inlined in the header given how trivial it is.
> > 
> 
> The style checker gave me an error earlier when trying to WEBCORE_EXPORT an
> inline method. Is this expected?

Just make it inline and DON'T add WEBCORE_EXPORT. There is no need to export an inline function.

[Joshua Hoffman]

Created attachment 461746 [details]
Patch

[Joshua Hoffman]

Created attachment 461798 [details]
Patch

[Tyler Wilcock]

Comment on attachment 461746 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:781
> +    if (auto* cachedImage = this->cachedImage()) {
> +        if (auto* image = cachedImage->image()) {
> +            if (auto* bitmapImage = dynamicDowncast<BitmapImage>(*image))
> +                return bitmapImage;
> +        }
> +    }
> +    return nullptr;

Can this be:

if (auto* cachedImage = this->cachedImage())
    return dynamicDowncast<BitmapImage>(cachedImage->image());
return nullptr;

> Source/WebCore/html/HTMLImageElement.cpp:1004
> +    if (auto* cachedImage = this->cachedImage()) {
> +        if (auto* bitmapImage = cachedBitmapImage())
> +            return cachedImage->image()->isAnimated() || !bitmapImage->isAnimationFinished();
> +    }
> +    return false;

Can cachedImage->image()->isAnimated() return true without a non-null bitmap image? If so, maybe that shouldn't be gated by the presence of a cachedBitmapImage(). Maybe something like this:

bool HTMLImageElement::isAnimatedImage() const
{
    if (auto* cachedImage = this->cachedImage()) {
        if (cachedImage->image()->isAnimated())
            return true;
    }

    if (auto* bitmapImage = cachedBitmapImage())
        return !bitmapImage->isAnimationFinished();

    return false;
}

> Source/WebCore/page/Page.cpp:4056
> +                        if (auto* backgroundImage = cachedBackgroundImage->image()) {
> +                            if (backgroundImage) {

Is this `if (backgroundImage)` unnecessary when you have `if (auto* backgroundImage = cachedBackgroundImage->image())` directly above?

> Source/WebCore/page/Page.cpp:4057
> +                                BitmapImage& bitmapImage = downcast<BitmapImage>(*backgroundImage);

Does auto& rather than BitmapImage& work here?

> Source/WebCore/page/Page.cpp:4078
> +                foundAnimatedImage = true;

Can we break out of this loop when we set `foundAnimatedImage = true;`?

> Source/WebCore/page/Page.cpp:4087
> +                        if (auto* backgroundImage = cachedBackgroundImage->image()) {
> +                            if (backgroundImage && backgroundImage->isAnimated())

Is the `backgroundImage` null check necessary here when we have` if (auto* backgroundImage = cachedBackgroundImage->image())` directly above?

> Source/WebCore/svg/graphics/SVGImage.cpp:382
> +    if (!rootElement || !rootElement->animationsPaused() || m_page->settings().animatedImagesDisabled() || !m_page)

Here we added:

|| m_page->settings().animatedImagesDisabled() || !m_page

Seems like we null check after dereferencing m_page via -> — should these be flipped?

> Source/WebCore/testing/Internals.cpp:1069
> +    auto* page = document->page();
> +    if (!page)
> +        return;
> +
> +    return page->toggleAllAnimations();

This is a void method, so we probably only need `page->toggleAllAnimations();` rather than `return page->toggleAllAnimations();`, right?

Also, I wonder if this could be:

if (auto* page = document->page())
   page->toggleAllAnimations();

[Joshua Hoffman]

Created attachment 461799 [details]
Patch

[Joshua Hoffman]

Created attachment 461834 [details]
Patch

[Tyler Wilcock]

Comment on attachment 461834 [details]
Patch

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

> Source/WebCore/page/Page.cpp:4059
> +                    if (auto* cachedBackgroundImage = renderer->style().backgroundLayers().image()->cachedImage()) {

Is it possible for:

renderer->style().backgroundLayers().image()

To be null when we deref it for ->cachedImage() here?

> Source/WebCore/page/Page.cpp:4096
> +                    if (auto* cachedBackgroundImage = renderer->style().backgroundLayers().image()->cachedImage()) {

Is it possible for:

renderer->style().backgroundLayers().image()

To be null when we deref it for ->cachedImage() here?

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #14)
> Is it possible for:
> 
> renderer->style().backgroundLayers().image()
> 
> To be null when we deref it for ->cachedImage() here?

Yeah, I think it can be—good call out. I can guard for that.

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #14)
> Is it possible for:
> 
> renderer->style().backgroundLayers().image()
> 
> To be null when we deref it for ->cachedImage() here?

Looking at this more, I think the condition on the line above: `if (renderer->style().hasBackgroundImage())` will do that null check.

[Joshua Hoffman]

Created attachment 461844 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 461844 [details]
Patch

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

> COMMIT_MESSAGE:7
> +This patch adds a new experimental AX feature, which when enabled, disables image animations. This includes regular bitmap images, background images, SVG animations, and support for the <model> element. Additionally, we provide new interfaces to control animations at the page and individual image level.

Can you split this patch into three patches: one for BitmapImage, another for the SVG image and the third for the <model> element?

> Source/WebCore/html/HTMLImageElement.cpp:777
> +void HTMLImageElement::playAnimation()

What does playAnimation() mean? Does it mean resume or restart?

> Source/WebCore/html/HTMLImageElement.cpp:779
> +    if (auto* image = cachedBitmapImage()) {

Why do we care about BitmapImage only here? The image can be SVGImage which may have animated elements.

> Source/WebCore/platform/graphics/BitmapImage.cpp:70
> +void BitmapImage::updateFromDocument(const Document& document)

This is layering violation. We should not be accessing Document in the platform directory.

[Joshua Hoffman]

(In reply to Said Abou-Hallawa from comment #18)

> Can you split this patch into three patches: one for BitmapImage, another
> for the SVG image and the third for the <model> element?
> 

Yes! I will update this one to just do BitmapImages, and have those other two follow. 

> > Source/WebCore/html/HTMLImageElement.cpp:777
> > +void HTMLImageElement::playAnimation()
> 
> What does playAnimation() mean? Does it mean resume or restart?

It means resume—I'll be more explicit and rename it. 

> > Source/WebCore/html/HTMLImageElement.cpp:779
> > +    if (auto* image = cachedBitmapImage()) {
> 
> Why do we care about BitmapImage only here? The image can be SVGImage which
> may have animated elements.

Understood. In the SVG patch this will handle SVGImages. 

> > Source/WebCore/platform/graphics/BitmapImage.cpp:70
> > +void BitmapImage::updateFromDocument(const Document& document)
> 
> This is layering violation. We should not be accessing Document in the
> platform directory.

Gotcha. I'll revert this back to updateFromSettings.

[Joshua Hoffman]

Created attachment 461861 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 461861 [details]
Patch

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

> Source/WebCore/page/Page.h:982
> +    bool areAllAnimationsPaused() { return !m_hasUnpausedAnimations; }
> +    WEBCORE_EXPORT void toggleAllAnimations();
> +    bool containsAnimatedImages();

If these are just for testing (and I think they are), please name them `toggleAllAnimationsForTesting()`, `containsAnimatedImagesForTesting()` etc, since they are expensive and we don't want to start calling them in production code.

Also containsAnimatedImages() should be `const`.

[Joshua Hoffman]

(In reply to Simon Fraser (smfr) from comment #21)
> 
> If these are just for testing (and I think they are), please name them
> `toggleAllAnimationsForTesting()`, `containsAnimatedImagesForTesting()` etc,
> since they are expensive and we don't want to start calling them in
> production code.

In a follow up patch, the intention is that both `toggleAllAnimations` and `containsAnimatedImages` will be used to control page animations. However, they are called very infrequently and only ever when the AnimatedImagesDisabled setting is enabled. Would renaming them to something like `toggleAllAnimationsWhenDisabled` make that more clear? I can also add a check in them to ensure the setting is enabled.

[Simon Fraser (smfr)]

(In reply to Joshua Hoffman from comment #22)
> (In reply to Simon Fraser (smfr) from comment #21)
> > 
> > If these are just for testing (and I think they are), please name them
> > `toggleAllAnimationsForTesting()`, `containsAnimatedImagesForTesting()` etc,
> > since they are expensive and we don't want to start calling them in
> > production code.
> 
> In a follow up patch, the intention is that both `toggleAllAnimations` and
> `containsAnimatedImages` will be used to control page animations. However,
> they are called very infrequently and only ever when the
> AnimatedImagesDisabled setting is enabled. Would renaming them to something
> like `toggleAllAnimationsWhenDisabled` make that more clear? I can also add
> a check in them to ensure the setting is enabled.

That's probably OK then.

Can you share code with FrameView::resumeVisibleImageAnimations()?

[Simon Fraser (smfr)]

Comment on attachment 461861 [details]
Patch

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

> Source/WebCore/page/Page.cpp:4032
> +    m_hasUnpausedAnimations = !m_hasUnpausedAnimations;

You're trying to maintain a flag here that reflects the state of all the images in all the subframes. That seems pretty hard: subframes can be created or destroyed, and in each subframe, images can be dynamically added or removed. How can you trust the state of this flag?

> Source/WebCore/page/Page.cpp:4042
> +                    if (auto* cachedBackgroundImage = renderer->style().backgroundLayers().image()->cachedImage()) {

This is only getting the first background layer; CSS backgrounds can have multiple images, so you need to walk the list here. But that list walking code shouldn't live here in Page (put it on a Renderer class).

[Joshua Hoffman]

(In reply to Simon Fraser (smfr) from comment #23)

> Can you share code with FrameView::resumeVisibleImageAnimations()?

I'll take a look!

(In reply to Simon Fraser (smfr) from comment #24)
 
> You're trying to maintain a flag here that reflects the state of all the
> images in all the subframes. That seems pretty hard: subframes can be
> created or destroyed, and in each subframe, images can be dynamically added
> or removed. How can you trust the state of this flag?

That's true. I'll remove this variable, and instead pass a boolean parameter into toggleAllAnimations to dictate the desired state of all animations so it's not tied to the state.

[Joshua Hoffman]

Created attachment 461871 [details]
Patch

[Joshua Hoffman]

Created attachment 461892 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 461892 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:71
> +  humanReadableName: "Control animated images"

It's a bit odd for the Experimental Feature to actually do the disabling. I'd expect the feature to be "allow image animation to be disabled", and then have some other affordance to actually control whether animation is on or off.

The title needs to reflect what the feature really does (and "control" is a bit vague). I'd also suggest choosing a name that starts with "image" or "animate" to make it easy to find in the UI.

> Source/WebCore/page/Page.cpp:4030
> +void Page::toggleAllAnimationsWhenDisabled(bool shouldAnimate)

The "when disabled" is hard to understand from a caller perspective. When what is disabled?

> Source/WebCore/platform/graphics/BitmapImage.cpp:73
> +    m_disableImageAnimation = settings.animatedImagesDisabled();

We're on Image here, so m_disableAnimation is fine. Or flip it and call this m_allowsAnimation

> Source/WebCore/platform/graphics/BitmapImage.h:254
> +    bool m_shouldAllowIndividualAnimation { false };

It's not clear what "individual" means here.

> Source/WebCore/platform/graphics/BitmapImage.h:255
> +    bool m_isPageAnimationEnabled { false };

This is unused.

> Source/WebCore/rendering/style/RenderStyle.cpp:456
> +void RenderStyle::setBackgroundImageAnimationState(bool shouldAnimate)

I'm not really a fan of poking state into Images inside styles. Can we instead add a way for the Image to ask whether it should animate via ImageObserver/CachedImageClient?

[Joshua Hoffman]

(In reply to Simon Fraser (smfr) from comment #28)
> Comment on attachment 461892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=461892&action=review
> 
> > Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:71
> > +  humanReadableName: "Control animated images"
> 
> It's a bit odd for the Experimental Feature to actually do the disabling.
> I'd expect the feature to be "allow image animation to be disabled", and
> then have some other affordance to actually control whether animation is on
> or off.

This will eventually use a currently unavailable system setting to control the animation state, so at present, we're using this setting to toggle for testing.

[Joshua Hoffman]

Created attachment 462012 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 462012 [details]
Patch

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

> Source/WebCore/html/ImageBitmap.cpp:639
> +    bool hasPageResumedAnimationWhenSettingEnabled() override { return false; }

This is a bit of a brain teaser. Why isn't it just called allowsAnimation() ?

> Source/WebCore/loader/cache/CachedImage.cpp:714
> +    return m_skippingRevalidationDocument && m_skippingRevalidationDocument->page() && m_skippingRevalidationDocument->page()->shouldPageAnimationPlayIfSettingEnabled();

m_skippingRevalidationDocument sounds like something special and I'm not sure it's OK to use to get to Page. I think you need to hop out again through CachedImageClient.

> Source/WebCore/loader/cache/CachedImage.h:161
> +        bool hasPageResumedAnimationWhenSettingEnabled() final;

allowsAnimation() ?

> Source/WebCore/page/Page.h:1349
> +    bool m_shouldPageAnimationPlayIfSettingEnabled { false };

This "if setting enabled" is cumbersome (partly because you can't tell which setting it's referring to). An enum would be better:

enum class ImageAnimationState : uint8_t {
  Allowed,
  Disabled,
  Reenabled
};

but I'm not sure why you need the "animations disabled by re-enabled by the user" state. Why not just have a `bool m_allowsImagesAnimations` that defaults to true, is set to false if the Setting is set, then goes back to true if the user re-enables animations?

BTW, what's the lifetime of the "re-enabled by user" state? Does it live across page loads?

> Source/WebCore/platform/graphics/BitmapImage.cpp:378
> +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );

Why doesn't the call out to imageObserver do the job of consulting the setting and whether animations have been re-enabled? It would be better if BitmapImage didn't have to store m_disableAnimation and m_allowAnimationWhenSettingEnabled

> Source/WebCore/platform/graphics/BitmapImage.cpp:680
> +void BitmapImage::resumeAnimation()

It's not good for a generically named function like "resumeAnimation" to store state which mirrors some settings-related state elsewhere (i.e. don't store m_allowAnimationWhenSettingEnabled here).

> Source/WebCore/platform/graphics/ImageObserver.h:56
> +    virtual bool hasPageResumedAnimationWhenSettingEnabled() = 0;

allowsAnimation() ?

> Source/WebCore/rendering/style/RenderStyle.cpp:471
> +bool RenderStyle::backgroundImagesCanAnimate() const

You could have animated images in masks, in border-image, in list-style-image and probably other places too.

[Joshua Hoffman]

(In reply to Simon Fraser (smfr) from comment #31)

thanks! I'll make these adjustments.

> BTW, what's the lifetime of the "re-enabled by user" state? Does it live across page loads?

It lives across page loads.

[Joshua Hoffman]

(In reply to Simon Fraser (smfr) from comment #31)

> > Source/WebCore/platform/graphics/BitmapImage.cpp:378
> > +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );
> 
> Why doesn't the call out to imageObserver do the job of consulting the
> setting and whether animations have been re-enabled? It would be better if
> BitmapImage didn't have to store m_disableAnimation and
> m_allowAnimationWhenSettingEnabled
> 

This will work for m_disableAnimation & for checking if page-wide animations are resumed (I'll move those there), but m_allowAnimationWhenSettingEnabled is used if just one image needs to animate, and not the whole page.

[Simon Fraser (smfr)]

(In reply to Joshua Hoffman from comment #33)
> (In reply to Simon Fraser (smfr) from comment #31)
> 
> > > Source/WebCore/platform/graphics/BitmapImage.cpp:378
> > > +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );
> > 
> > Why doesn't the call out to imageObserver do the job of consulting the
> > setting and whether animations have been re-enabled? It would be better if
> > BitmapImage didn't have to store m_disableAnimation and
> > m_allowAnimationWhenSettingEnabled
> > 
> 
> This will work for m_disableAnimation & for checking if page-wide animations
> are resumed (I'll move those there), but m_allowAnimationWhenSettingEnabled
> is used if just one image needs to animate, and not the whole page.

Ah, I didn't realize there was a per-image override.

One issue here is that multiple copies of the same image animate "in sync"; 10 image elements with the same GIF all render the same frame at the same time.

If the user is able to stop and start just some of these, it means that we'll be rendering different frames of the same GIF in different places which can get very slow (since decoding a frame may have to decode all the previous frames, if you're not showing the frame that was previously cached).

[Joshua Hoffman]

(In reply to Simon Fraser (smfr) from comment #34)
> (In reply to Joshua Hoffman from comment #33)
> > (In reply to Simon Fraser (smfr) from comment #31)
> > 
> > > > Source/WebCore/platform/graphics/BitmapImage.cpp:378
> > > > +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );
> > > 
> > > Why doesn't the call out to imageObserver do the job of consulting the
> > > setting and whether animations have been re-enabled? It would be better if
> > > BitmapImage didn't have to store m_disableAnimation and
> > > m_allowAnimationWhenSettingEnabled
> > > 
> > 
> > This will work for m_disableAnimation & for checking if page-wide animations
> > are resumed (I'll move those there), but m_allowAnimationWhenSettingEnabled
> > is used if just one image needs to animate, and not the whole page.
> 
> Ah, I didn't realize there was a per-image override.
> 
> One issue here is that multiple copies of the same image animate "in sync";
> 10 image elements with the same GIF all render the same frame at the same
> time.
> 
> If the user is able to stop and start just some of these, it means that
> we'll be rendering different frames of the same GIF in different places
> which can get very slow (since decoding a frame may have to decode all the
> previous frames, if you're not showing the frame that was previously cached).

The concession with this patch is that individually playing one image, if there are copies, will play all of the duplicates as well. This should prevent the issue you raised.

[Simon Fraser (smfr)]

Perhaps Document could store a HashSet of URLs for images whose animations have been re-enabled?

[Said Abou-Hallawa]

Comment on attachment 462012 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesExperimental.yaml:69
> +AnimatedImagesDisabled:

I think this naming is not right. AnimatedImages means the images which can be animated. So I think you meant "ImageAnimationDisabled" as the description below says.

Also should not we use Enabled instead of Disabled and have the default set to true; something like ImageAnimationEnabled() or ImagesAnimationEnabled()?

> Source/WebCore/html/HTMLImageElement.cpp:775
> +BitmapImage* HTMLImageElement::cachedBitmapImage() const
> +{
> +    if (auto* cachedImage = this->cachedImage())
> +        return dynamicDowncast<BitmapImage>(cachedImage->image());
> +    return nullptr;
> +}

I do not think this is a right approach. Please do not make explicit casting to BitmapImage inside HTMLImageElement unless it is really needed. Please reference the Image instead of referencing the BitmapImage. This function can look like this

Image* HTMLImageElement::image() const
{
    if (auto* cachedImage = this->cachedImage())
        return cachedImage->image();
    return nullptr;
}

> Source/WebCore/html/HTMLImageElement.cpp:781
> +void HTMLImageElement::resumeAnimation()
> +{
> +    if (auto* image = cachedBitmapImage())
> +        image->resumeAnimation();
> +}

Please use this->image() instead and add this virtual function to the Image class

virtual void resumeAnimation() { }

> Source/WebCore/html/HTMLImageElement.cpp:787
> +void HTMLImageElement::pauseAnimation()
> +{
> +    if (auto* image = cachedBitmapImage())
> +        image->pauseAnimation();
> +}

Please use this->image() instead and add this virtual function to the Image class

virtual void pauseAnimation() { }

> Source/WebCore/html/HTMLImageElement.cpp:794
> +bool HTMLImageElement::isAnimating() const
> +{
> +    if (auto* image = cachedBitmapImage())
> +        return image->isAnimating();
> +    return false;
> +}

Please use this->image() instead and add this virtual function to the Image class

virtual bool isAnimating() { return false; }

> Source/WebCore/html/HTMLImageElement.cpp:995
> +    if (auto* cachedImage = this->cachedImage()) {
> +        if (cachedImage->image()->isAnimated())
> +            return true;
> +    }
> +
> +    if (auto* bitmapImage = cachedBitmapImage())
> +        return !bitmapImage->isAnimationFinished();

Please notice that isAnimated() and isAnimationFinished() have different meanings. isAnimated() means it can animate. For example, for BitmapImage, it is true if frameCount() > 1. But isAnimationFinished() means it is not animating right now. And this can be true when isAnimated() is false or isAnimated () is true but the image has been already animated the specified repetitionCount(). So I am not sure what your function is answering: Is the image animatable? Or is it animating right now?

> Source/WebCore/loader/cache/CachedImage.cpp:457
> +bool CachedImage::CachedImageObserver::hasPageResumedAnimationWhenSettingEnabled()
> +{
> +    for (auto cachedImage : m_cachedImages) {
> +        if (cachedImage->hasPageResumedAnimationWhenSettingEnabled())
> +            return true;
> +    }
> +    return false;
> +}

What is this function for? Why does every CachedImage::Observer need to deal with page settings? Should not we have this function/setting the same for all images?

>> Source/WebCore/loader/cache/CachedImage.cpp:714
>> +    return m_skippingRevalidationDocument && m_skippingRevalidationDocument->page() && m_skippingRevalidationDocument->page()->shouldPageAnimationPlayIfSettingEnabled();
> 
> m_skippingRevalidationDocument sounds like something special and I'm not sure it's OK to use to get to Page. I think you need to hop out again through CachedImageClient.

Yes I think this is incorrect. CachedImage can be shared among multiple documents. When loading the image, the m_loader is set and it has a document() getter. But once the loading is finished, SubresourceLoader::didFinishLoading() calls CachedResource::clearLoader() via its releaseResources(). So we should not be assuming the CachedImage is attached to any document except while loading it.

> Source/WebCore/page/Page.cpp:4048
> +void Page::toggleAllAnimationsWhenAnimationSettingEnabled(bool shouldAnimate)
> +{
> +    if (!settings().animatedImagesDisabled())
> +        return;
> +    
> +    m_shouldPageAnimationPlayIfSettingEnabled = shouldAnimate;
> +    forEachDocument([] (Document& document) {
> +        for (auto& element : descendantsOfType<Element>(document.rootNode())) {
> +            if (auto* renderer = element.renderer()) {
> +                if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))
> +                    renderer->repaint();
> +                if (renderer->style().hasBackgroundImage()) {
> +                    if (renderer->style().backgroundImagesCanAnimate())
> +                        renderer->repaint();
> +                }
> +            }
> +        }
> +    });
> +}

Why do not we use Page::resumeAnimatingImages() instead of adding a new function? I think Page::resumeAnimatingImages() is called when animated images brought back in the current viewport like you scroll the window for example.

> Source/WebCore/page/Page.cpp:4070
> +bool Page::containsAnimatedImages() const
> +{
> +    bool foundAnimatedImage = false;
> +    forEachDocument([&foundAnimatedImage] (Document& document) {
> +        for (auto& element : descendantsOfType<Element>(document.rootNode())) {
> +            if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element)) {
> +                foundAnimatedImage = true;
> +                break;
> +            }
> +            if (auto* renderer = element.renderer()) {
> +                if (renderer->style().hasBackgroundImage()) {
> +                    if (renderer->style().backgroundImagesCanAnimate()) {
> +                        foundAnimatedImage = true;
> +                        break;
> +                    }
> +                }
> +            }
> +        }
> +    });
> +    return foundAnimatedImage;
> +}

I do not see this function is used in this patch. Please remove it. 

Also this function and toggleAllAnimationsWhenAnimationSettingEnabled() are expensive since they enumerate all sub-documents in the page and in each document they enumerate all the descendant elements. We usually care about the current view port when invalidating renderers.

> Source/WebCore/platform/graphics/BitmapImage.cpp:73
> +    m_disableAnimation = settings.animatedImagesDisabled();

I think renaming m_disableAnimation -> m_animationPaused will make reading the code easier since setting it true will pause the animation.

But This setting is different from the other ones above. It has to be toggled while the image is animating when the Settings changes. The other ones are set only once and will not be changed even if the Settings has changed until the page is reloaded. So I would suggest moving it to the ImageObserver and CachedImageClient. You can follow the pattern of canDestroyDecodedData() and add similar functions like isImageAnimationEnabled(). In RenderElement::isImageAnimationEnabled() you can access the Document, Page and the Settings and answer this question.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:378
>> +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );
> 
> Why doesn't the call out to imageObserver do the job of consulting the setting and whether animations have been re-enabled? It would be better if BitmapImage didn't have to store m_disableAnimation and m_allowAnimationWhenSettingEnabled

I agree. See my comment above. This new code can be replaced by "&& imageObserver()->isImageAnimationEnabled()"

>> Source/WebCore/platform/graphics/BitmapImage.cpp:680
>> +void BitmapImage::resumeAnimation()
> 
> It's not good for a generically named function like "resumeAnimation" to store state which mirrors some settings-related state elsewhere (i.e. don't store m_allowAnimationWhenSettingEnabled here).

I do not think you need this function. If the Settings changed and you invalidate the renderers of this BitmapImage, the BitmapImage will be redrawn. And in this time shouldAnimate() will be true because imageObserver()->isImageAnimationEnabled() will be true.

> Source/WebCore/rendering/style/RenderStyle.cpp:469
> +void RenderStyle::setBackgroundImageAnimationState(bool shouldAnimate)
> +{
> +    for (auto fillLayer = &backgroundLayers(); fillLayer; fillLayer = fillLayer->next()) {
> +        if (auto* cachedBackgroundImage = fillLayer->image()->cachedImage()) {
> +            if (auto* backgroundImage = cachedBackgroundImage->image()) {
> +                auto& backgroundBitmapImage = downcast<BitmapImage>(*backgroundImage);
> +                if (shouldAnimate)
> +                    backgroundBitmapImage.resumeAnimation();
> +                else
> +                    backgroundBitmapImage.pauseAnimation();
> +            }
> +        }
> +    }
> +}

Where is function is used? Please remove it if it is not used.

> Source/WebCore/testing/Internals.cpp:1067
> +void Internals::resumeImageAnimation(HTMLImageElement& element)
> +{
> +    element.resumeAnimation();
> +}
> +
> +void Internals::pauseImageAnimation(HTMLImageElement& element)
> +{
> +    element.pauseAnimation();
> +}
> +
> +void Internals::togglePageAnimation(bool shouldAnimate)
> +{
> +    auto* document = contextDocument();
> +    if (!document || !document->frame())
> +        return;
> +
> +    if (auto* page = document->page())
> +        page->toggleAllAnimationsWhenAnimationSettingEnabled(shouldAnimate);
> +}

I am confused here. Is this setting per image or per page? Do we need to pause/resume for a certain HTMLImageElement? Or should not we do that for all images? If we should do it for all images, then I think you can use something like this in your JavaScript:

internals.settings.setImageAnimationEnabled(false);

> LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:13
> +    if (window.accessibilityController) {

Why do we test this code only through window.accessibilityController? I think this feature should be tested for general use.

Also we need to test that the timing of toggling the flag will work as expected. For example if we disable the animation while painting the frame 5 of the image, and then resume the animation it should be resumed at frame 6.

Look at LayoutTests/fast/images/animated-heics-draw.html to see how we can control the timing of the animated image.

> LayoutTests/platform/mac-wk1/TestExpectations:436
> +# Image animation disabled (experimental feature)
> +accessibility/mac/disable-animations-toggle-page-animations.html [ Skip ]
> +accessibility/mac/disable-animations-enabled.html [ Skip ]
> +accessibility/mac/disable-animations-play-individual-animation.html [ Skip ]

Are these tests skipped for WK1 because window.accessibilityController is available for WK2?

[Joshua Hoffman]

Created attachment 462061 [details]
Patch

[Joshua Hoffman]

(In reply to Said Abou-Hallawa from comment #37)

Thanks for all of the comments! I addressed the feedback in most-recently uploaded patch.

> > Source/WebCore/page/Page.cpp:4048
> > +void Page::toggleAllAnimationsWhenAnimationSettingEnabled(bool shouldAnimate)
> > +{
> > +    if (!settings().animatedImagesDisabled())
> > +        return;
> > +    
> > +    m_shouldPageAnimationPlayIfSettingEnabled = shouldAnimate;
> > +    forEachDocument([] (Document& document) {
> > +        for (auto& element : descendantsOfType<Element>(document.rootNode())) {
> > +            if (auto* renderer = element.renderer()) {
> > +                if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))
> > +                    renderer->repaint();
> > +                if (renderer->style().hasBackgroundImage()) {
> > +                    if (renderer->style().backgroundImagesCanAnimate())
> > +                        renderer->repaint();
> > +                }
> > +            }
> > +        }
> > +    });
> > +}
> 
> Why do not we use Page::resumeAnimatingImages() instead of adding a new
> function? I think Page::resumeAnimatingImages() is called when animated
> images brought back in the current viewport like you scroll the window for
> example.
>
> ... 
>
> Also this function and toggleAllAnimationsWhenAnimationSettingEnabled() are
> expensive since they enumerate all sub-documents in the page and in each
> document they enumerate all the descendant elements. We usually care about
> the current view port when invalidating renderers.

In a subsequent patch, this method allows us to expand toggling all animations to <svg> & <model> elements as we walk the tree and resume/pause. `resumeAnimatingImages` was not repainting my bitmap images as expected. 

> 
> >> Source/WebCore/platform/graphics/BitmapImage.cpp:378
> >> +    return repetitionCount() && !m_animationFinished && imageObserver() && (imageObserver()->hasPageResumedAnimationWhenSettingEnabled() || !m_disableAnimation || m_allowAnimationWhenSettingEnabled );
> > 
> > Why doesn't the call out to imageObserver do the job of consulting the setting and whether animations have been re-enabled? It would be better if BitmapImage didn't have to store m_disableAnimation and m_allowAnimationWhenSettingEnabled
> 
> I agree. See my comment above. This new code can be replaced by "&&
> imageObserver()->isImageAnimationEnabled()"

I've addressed this and have moved all of the image's animation state to the CachedImageClient.

> 
> >> Source/WebCore/platform/graphics/BitmapImage.cpp:680
> >> +void BitmapImage::resumeAnimation()
> > 
> > It's not good for a generically named function like "resumeAnimation" to store state which mirrors some settings-related state elsewhere (i.e. don't store m_allowAnimationWhenSettingEnabled here).
> 
> I do not think you need this function. If the Settings changed and you
> invalidate the renderers of this BitmapImage, the BitmapImage will be
> redrawn. And in this time shouldAnimate() will be true because
> imageObserver()->isImageAnimationEnabled() will be true.

This patch allows image animation (when ImageAnimationEnabled is not turned on) to be resumed/paused at the page and at the individual image level. The resumeAnimation and pauseAnimation methods allow a single image to be controlled, while the setting is there to simply stop animations by default. But, in my updated patch, I've limited these methods to just HTMLImageElements, and removed these methods/state from BitmapImage. 

> 
> > LayoutTests/accessibility/mac/disable-animations-play-individual-animation.html:13
> > +    if (window.accessibilityController) {
> 
> Why do we test this code only through window.accessibilityController? I
> think this feature should be tested for general use.
> 
> Also we need to test that the timing of toggling the flag will work as
> expected. For example if we disable the animation while painting the frame 5
> of the image, and then resume the animation it should be resumed at frame 6.
> 
> Look at LayoutTests/fast/images/animated-heics-draw.html to see how we can
> control the timing of the animated image.
> 

I included a test for this in the new patch, which tests the frames are in order even if paused/resumed.

> > LayoutTests/platform/mac-wk1/TestExpectations:436
> > +# Image animation disabled (experimental feature)
> > +accessibility/mac/disable-animations-toggle-page-animations.html [ Skip ]
> > +accessibility/mac/disable-animations-enabled.html [ Skip ]
> > +accessibility/mac/disable-animations-play-individual-animation.html [ Skip ]
> 
> Are these tests skipped for WK1 because window.accessibilityController is
> available for WK2?

There is not a plan to support this feature on WK1 at the moment, so the test scope is limited.

[Said Abou-Hallawa]

Comment on attachment 462061 [details]
Patch

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

> COMMIT_MESSAGE:7
> +This patch adds a new experimental AX preference, which when disabled, prevents BitmapImage animations. Additionally, we provide new interfaces to control animations at the page and individual image level.

re: Additionally, we provide new interfaces to control animations at the page and individual image level.

1) What is the use case of controlling the animation on individual image?
2) What is the interface for using this feature? In this patch I see resumeAnimation()/pauseAnimation() are used by Internals only. Is the plan to use it for testing only?
3) What should happen if two HTMLImageElements are accessing the same image?

<img id="image1" src="animated_image.gif">
<img id="image2" src="animated_image.gif">
<script>
    internals.resumeImageAnimation(image1);
</script>

Do you expect the animation will be resumed in both elements (image1  and image2) or only the first one (image1)?

[Joshua Hoffman]

(In reply to Said Abou-Hallawa from comment #40)

> > +This patch adds a new experimental AX preference, which when disabled, prevents BitmapImage animations. Additionally, we provide new interfaces to control animations at the page and individual image level.
> 
> re: Additionally, we provide new interfaces to control animations at the
> page and individual image level.
> 
> 1) What is the use case of controlling the animation on individual image?
> 2) What is the interface for using this feature? In this patch I see
> resumeAnimation()/pauseAnimation() are used by Internals only. Is the plan
> to use it for testing only?
> 3) What should happen if two HTMLImageElements are accessing the same image
Users who have animations disabled globally may want to see a particular animation that contains important content, without having to resume all animations. Several sites & social media platforms implement this type of control in their own settings already.

A context menu item to resume/pause these animations would provide users an interface to control this.

For #3, see below. 
 
> <img id="image1" src="animated_image.gif">
> <img id="image2" src="animated_image.gif">
> <script>
>     internals.resumeImageAnimation(image1);
> </script>
> 
> Do you expect the animation will be resumed in both elements (image1  and
> image2) or only the first one (image1)?

If it's the same source, both images will resume animation. This assumes a user who wants to see a particular image animate will want to see that image animate in all occurrences.

[Said Abou-Hallawa]

(In reply to Joshua Hoffman from comment #41)
> (In reply to Said Abou-Hallawa from comment #40)
> 
> > > +This patch adds a new experimental AX preference, which when disabled, prevents BitmapImage animations. Additionally, we provide new interfaces to control animations at the page and individual image level.
> > 
> > re: Additionally, we provide new interfaces to control animations at the
> > page and individual image level.
> > 
> > 1) What is the use case of controlling the animation on individual image?
> > 2) What is the interface for using this feature? In this patch I see
> > resumeAnimation()/pauseAnimation() are used by Internals only. Is the plan
> > to use it for testing only?
> > 3) What should happen if two HTMLImageElements are accessing the same image
> Users who have animations disabled globally may want to see a particular
> animation that contains important content, without having to resume all
> animations. Several sites & social media platforms implement this type of
> control in their own settings already.
> 
> A context menu item to resume/pause these animations would provide users an
> interface to control this.
> 

I am not aware there's something like this in the web. Can you please guide me to the specs' link and where I can see this feature in Chrome and FireFox or sites & social media platforms?

If there is no specs' for this, then we need to file a GitHub issue for it. We have to provide an interface that all browsers agree on.

But if this is the plan, how can this be implemented by your patch given the only interface is through Internals?

> For #3, see below. 
>  
> > <img id="image1" src="animated_image.gif">
> > <img id="image2" src="animated_image.gif">
> > <script>
> >     internals.resumeImageAnimation(image1);
> > </script>
> > 
> > Do you expect the animation will be resumed in both elements (image1  and
> > image2) or only the first one (image1)?
> 
> If it's the same source, both images will resume animation. This assumes a
> user who wants to see a particular image animate will want to see that image
> animate in all occurrences.

I do not think this is correct. This flag should be set per HTMLImageElement as the Internal API says. I think having it per CachedImage (or per URL as you implemented it) is a technical details that the user should not deal with. 

Maybe I am wrong and this is why a specs' link or a GitHub issue can answer this question clearly.

[James Craig]

Said wrote:  

> If there is no spec for this, then we need to file a GitHub issue for it. We have to provide an interface that all browsers agree on.

prefers-reduced-motion is implemented by all major browsers. 
https://www.w3.org/TR/mediaqueries-5/#prefers-reduced-motion

I'm not aware whether any of them link it to GIF animations, but IMO that's within the current realm innovation/differentiation on behalf of the user.

[James Craig]

Chris also mentioned offline this may be more analogous to the auto-play media setting.

[Said Abou-Hallawa]

Comment on attachment 462061 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:995
> +    if (auto* cachedImage = this->cachedImage()) {
> +        if (cachedImage->image()->isAnimated())
> +            return true;
> +    }

if (auto* image = this->image())
    return image->isAnimated();
return false;

> Source/WebCore/loader/cache/CachedImage.cpp:453
> +        if (cachedImage->allowsAnimation(sourceUrl()))

Why do we pass sourceUrl() to the CachedImage? We get the sourceUrl() from the CachedImage. I think this should be 

if (cachedImage->allowsAnimation())

> Source/WebCore/loader/cache/CachedImage.cpp:712
> +bool CachedImage::allowsAnimation(const URL& sourceURL)

You do not have to pass a sourceURL. You can use url() which is defined in the base class CachedResource.

> Source/WebCore/loader/cache/CachedImage.cpp:716
> +        if (client->allowsAnimation(sourceURL))

I would suggest passing *this instead of the sourceURL. This will be consistent with other calls to CachedImageClient in this file.

> Source/WebCore/loader/cache/CachedImageClient.h:56
> +    virtual bool allowsAnimation(const URL&) { return false; }

Please make this function take a CachedImage& instead. So it looks similar to the other functions above.

> Source/WebCore/page/Page.cpp:4035
> +    if (settings().imageAnimationEnabled())
> +        return;
> +    
> +    m_allowsImageAnimations = shouldAnimate;

Why do we have to options to enable the animations? You even check the settings() member but you set the Page member. I think we can rely on the Settings only:

    if (settings().imageAnimationEnabled())
        return;

    settings().setImageAnimationEnabled(true);

Another approach is to add a webcoreOnChange to your setting. See CoreMathMLEnabled in WebPreferencesExperimental.yaml as an example.

> Source/WebCore/page/Page.cpp:4049
> +    forEachDocument([] (Document& document) {
> +        for (auto& element : descendantsOfType<Element>(document.rootNode())) {
> +            if (auto* renderer = element.renderer()) {
> +                if (auto* imageElement = dynamicDowncast<HTMLImageElement>(element))
> +                    renderer->repaint();
> +                if (renderer->style().hasBackgroundImage()
> +                    || renderer->style().hasBorderImage()
> +                    || renderer->style().hasMask()
> +                    || renderer->style().listStyleImage()) {
> +                    renderer->repaint();
> +                }
> +            }
> +        }
> +    });

This code is expensive. And you do not need to invalidate the renderers outside the viewport. Please consider refactoring or rewriting this code similar to Page::resumeAnimatingImages(). In other words, you need to work only within the visible rectangle.

> Source/WebCore/rendering/RenderElement.cpp:2499
> +    return settings().imageAnimationEnabled() || document().page()->allowsImageAnimation() || document().isImageURLAllowedToAnimate(sourceURL);

I think we should use document().page()->allowsImageAnimation() only to read the global setting.

> Source/WebCore/testing/Internals.cpp:1059
> +void Internals::togglePageAnimation(bool shouldAnimate)

I think this function should be removed. internals.settings.setImageAnimationEnabled() can be used instead.

[Said Abou-Hallawa]

Comment on attachment 462061 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:4035
>> +    m_allowsImageAnimations = shouldAnimate;
> 
> Why do we have to options to enable the animations? You even check the settings() member but you set the Page member. I think we can rely on the Settings only:
> 
>     if (settings().imageAnimationEnabled())
>         return;
> 
>     settings().setImageAnimationEnabled(true);
> 
> Another approach is to add a webcoreOnChange to your setting. See CoreMathMLEnabled in WebPreferencesExperimental.yaml as an example.

A better example to follow for webcoreOnChange may be ScrollingPerformanceTestingEnabled.

[Joshua Hoffman]

Created attachment 462100 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 462100 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:790
> +void HTMLImageElement::resumeAnimation()
> +{
> +    if (auto* image = this->image()) {
> +        document().addAllowedAnimatedImageURL(image->sourceURL());
> +        image->startAnimation();
> +    }
> +}
> +
> +void HTMLImageElement::pauseAnimation()
> +{
> +    if (auto* image = this->image()) {
> +        document().removeAllowedAnimatedImageURL(image->sourceURL());
> +        image->stopAnimation();
> +    }
> +}

This interface is a little bit troublesome because we have a global setting and a local overriding. And they both need to interact clearly. How about this suggestion?

Image.h:

class Image {
public:
    std::optional<bool> allowsAnimation() const { return m_allowsAnimation; }
    void setAllowsAnimation(std::optional<bool> allowsAnimation) { m_allowsAnimation = allowsAnimation; }

private:
    // This is a tri-state flag:
    // std::nullopt (means follow the global setting)
    // false (means do not allow animation regardless of the global setting)
    // true (means allow animation regardless of the global setting)
    std::optional<bool> m_allowsAnimation;
}

HTMLImageElement:

class HTMLImageElement {
public:
    bool allowsAnimation() const;
    void setAllowsAnimation(bool);
    void resetAllowsAnimation(); // This will set Image::m_allowsAnimation to std::nullopt.
}

bool HTMLImageElement::allowsAnimation() const
{
    if (auto* image = this->image())
        return image->allowsAnimation().value_or(document().settings().imageAnimationEnabled());
    return false; 
}

Internal.cpp

void Internals::resumeImageAnimation(HTMLImageElement& element)
{
    element.setAllowsAnimation(true);
}

RenderImage.cpp

bool RenderImage::allowsAnimation(CachedImage& image) const
{
    if (is<HTMLImageElement>(element()))
        return downcast<HTMLImageElement>(*element()).allowsAnimation();
    return RenderReplaced::allowsAnimation(image);
}

> Source/WebCore/testing/Internals.cpp:1060
> +void Internals::resumeImageAnimation(HTMLImageElement& element)
> +{
> +    element.resumeAnimation();
> +}
> +
> +void Internals::pauseImageAnimation(HTMLImageElement& element)
> +{
> +    element.pauseAnimation();
> +}

This interface works only for HTMLImageElement but it does not work for css images 

<style>
    .box-with-animation {
        background-image: url("animated.gif");
    }
</style>

But this patch will disable the css animation if the same src is used by an HTMLImageElement:

<img id="image1" src="animated.gif">
<script>
    internals.pauseImageAnimation(image1);
<script>

[Joshua Hoffman]

(In reply to Said Abou-Hallawa from comment #48)
> 
> This interface is a little bit troublesome because we have a global setting
> and a local overriding. And they both need to interact clearly. How about
> this suggestion?

I like that suggestion! I’ll swap out the URL methods for this.

> But this patch will disable the css animation if the same src is used by an
> HTMLImageElement:

Will having a flag now set on the `Image`, not the src, resolve this behavior?

[Joshua Hoffman]

Created attachment 462169 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 462169 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:9285
> +bool Document::isImageURLAllowedToAnimate(const URL& url) const
> +{
> +    return m_imageURLsAllowedToAnimate.contains(url);
> +}
> +
> +void Document::addAllowedAnimatedImageURL(const URL& url)
> +{
> +    m_imageURLsAllowedToAnimate.add(url);
> +}
> +
> +void Document::removeAllowedAnimatedImageURL(const URL& url)
> +{
> +    m_imageURLsAllowedToAnimate.remove(url);
> +}

This code is not needed. Please remove.

> Source/WebCore/dom/Document.h:1704
> +    bool isImageURLAllowedToAnimate(const URL&) const;
> +    void addAllowedAnimatedImageURL(const URL&);
> +    void removeAllowedAnimatedImageURL(const URL&);

Ditto.

> Source/WebCore/dom/Document.h:2312
> +    ListHashSet<URL> m_imageURLsAllowedToAnimate;

Ditto.

> Source/WebCore/html/HTMLImageElement.cpp:800
> +bool HTMLImageElement::isAnimating() const
> +{
> +    if (auto* image = this->image())
> +        return image->isAnimating();
> +    return false;
> +}

This function is not used. Please remove.

> Source/WebCore/html/HTMLImageElement.cpp:998
> +bool HTMLImageElement::isAnimatedImage() const
> +{
> +    if (auto* image = this->image())
> +        return image->isAnimated();
> +    return false;
> +}

This function is not used. Please remove.

> Source/WebCore/html/HTMLImageElement.h:163
> +    bool isAnimating() const;
> +    bool isAnimatedImage() const;

Please remove.

> Source/WebCore/html/ImageBitmap.cpp:639
> +    bool allowsAnimation() override { return true; }

This can be removed if ImageObserver::allowsAnimation() returns true.

> Source/WebCore/loader/cache/CachedImage.cpp:450
> +bool CachedImage::CachedImageObserver::allowsAnimation()

This function should take an Image as an input. See CachedImage::CachedImageObserver::canDestroyDecodedData() as an example.

> Source/WebCore/loader/cache/CachedImage.cpp:712
> +bool CachedImage::allowsAnimation()

This function should take an Image as an input. See CachedImage::canDestroyDecodedData() as an example.

> Source/WebCore/platform/graphics/BitmapImage.cpp:377
> +    return repetitionCount() && !m_animationFinished && imageObserver() && imageObserver()->allowsAnimation();

*this should be passed to allowsAnimation() here.

> Source/WebCore/platform/graphics/ImageObserver.h:56
> +    virtual bool allowsAnimation() = 0;

I think this should return true instead of having it a pure virtual function. I think all images should allow animation by default.

Also this function should take an Image as an input. See canDestroyDecodedData() above as an example.

> Source/WebCore/rendering/RenderImage.cpp:451
> +}

How about this?

    if (auto* image = cachedImage() ? cachedImage()->image() : nullptr)
        return image->isAnimated();
    return false;

Also I think isAnimatedImage() is not a correct name. I think it should be hasAnimatedImage() or something else. RenderImage is not an "image" per se. It just holds an "image".

> Source/WebCore/rendering/RenderView.cpp:879
> +    for (auto& element : descendantsOfType<RenderImage>(*this)) {

Getting the RenderImage only will not repaint elements with CSS background images.

> Tools/TestWebKitAPI/Tests/WebCore/SVGImageCasts.cpp:81
> +    bool allowsAnimation() final
> +    {
> +        return true;
> +    }

This can be removed if ImageObserver::allowsAnimation() returns true.

> LayoutTests/accessibility/mac/disable-animations-enabled-expected.txt:1
> +This tests that animations are disabled when the disable animated images setting is turned on.

I think these tests should be moved to fast/images directory.

> LayoutTests/platform/mac-wk1/TestExpectations:459
> +# Image animation disabled (experimental feature)
> +accessibility/mac/disable-animations-enabled.html [ Skip ]
> +accessibility/mac/disable-animations-play-individual-animation.html [ Skip ]
> +accessibility/mac/disable-animations-resuming-frame.html [ Skip ]

I think these tests should work for WK1 as well. Please remove these skip lines.

[Joshua Hoffman]

Created attachment 462205 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 462205 [details]
Patch

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

> Source/WebCore/html/HTMLImageElement.cpp:783
> +void HTMLImageElement::setAllowsAnimation(bool shouldAnimate)

shouldAnimate -> allowsAnimation.

> Source/WebCore/rendering/RenderView.cpp:881
> +            return;

Should not we "continue" instead of "return"?

> Source/WebCore/rendering/RenderView.cpp:884
> +            element.repaint();

Should we check whether the background image is animated or not? Also I think we should "continue" if we repaint the element.

> LayoutTests/fast/images/disable-animations-resuming-frame.html:22
> +                debug("Image frame: " + imageFrame + " was displayed.");
> +                image.removeEventListener("webkitImageFrameReady", listener, true);
> +                resolve(frame + 1);

Can something like this work?

debug("Image frame: " + imageFrame + " was displayed.");
image.removeEventListener("webkitImageFrameReady", listener, true);
internals.pauseImageAnimation(image);
setTimeout((image, imageFrame) => {
    shouldBeTrue("internals.imageFrameIndex(image) == imageFrame");
    internals.resumeImageAnimation(image);
    resolve(frame + 1);
}, 50, image, imageFrame);

> LayoutTests/fast/images/disable-animations-resuming-frame.html:37
> +            if (i == 2) {
> +                internals.pauseImageAnimation(image);
> +                internals.resumeImageAnimation(image);
> +            }

I think this does not test anything. Pausing and resuming the animation happens in the same frame. We should allow the page to render after pausing the animation before checking whether the animation is paused or not. See my suggestion above.

[Joshua Hoffman]

(In reply to Said Abou-Hallawa from comment #53)
 
> > Source/WebCore/rendering/RenderView.cpp:884
> > +            element.repaint();
> 
> Should we check whether the background image is animated or not? Also I
> think we should "continue" if we repaint the element.

I totally agree that we should continue after repainting. In terms of checking the background images, which would be less expensive: repainting anything with a background image or iterating through all of the fill layers and asking each image if it is animated? The later was why I held off on checking isAnimated.

 
> > LayoutTests/fast/images/disable-animations-resuming-frame.html:22
> > +                debug("Image frame: " + imageFrame + " was displayed.");
> > +                image.removeEventListener("webkitImageFrameReady", listener, true);
> > +                resolve(frame + 1);
> 
> Can something like this work?
> 
> debug("Image frame: " + imageFrame + " was displayed.");
> image.removeEventListener("webkitImageFrameReady", listener, true);
> internals.pauseImageAnimation(image);
> setTimeout((image, imageFrame) => {
>     shouldBeTrue("internals.imageFrameIndex(image) == imageFrame");
>     internals.resumeImageAnimation(image);
>     resolve(frame + 1);
> }, 50, image, imageFrame);

I tried this out (with both the ImageAnimationEnabled setting and without), and I haven't got this logic working. `WebkitImageFrameReady` is firing after the the frame advances, so the internals.imageFrameIndex call inside the listener is always returning a frame one more than imageFrame.

[Said Abou-Hallawa]

> I tried this out (with both the ImageAnimationEnabled setting and without), and I haven't got this logic working. `WebkitImageFrameReady` is firing after the the frame advances, so the internals.imageFrameIndex call inside the listener is always returning a frame one more than imageFrame

Okay let's check 

shouldBeTrue("internals.imageFrameIndex(image) == imageFrame + 1");

[Joshua Hoffman]

Created attachment 462242 [details]
Patch

[Tyler Wilcock]

Created attachment 462256 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 462256 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:387
> +    return !shouldAnimate() && m_source->canUseAsyncDecoding() && !(frameCount() > 1);

I do not think this change is needed for this patch. Also I think it is okay to treat "non-animating animated" images like static images. So I think we should return true for images which are (frameCount() == 1 || !shouldAnimate()). And this is equivalent to !canAnimate().

> Source/WebCore/platform/graphics/BitmapImage.cpp:640
> +    if (imageObserver() && !imageObserver()->allowsAnimation(*this))
> +        return;

Why do we have this early return? We do a few bookkeeping and callbacks after this new if-statement. I do not see why disallowing the animation make us skip all this code.

[Tyler Wilcock]

(In reply to Said Abou-Hallawa from comment #58)
> Comment on attachment 462256 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=462256&action=review
> 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:387
> > +    return !shouldAnimate() && m_source->canUseAsyncDecoding() && !(frameCount() > 1);
> 
> I do not think this change is needed for this patch. Also I think it is okay
> to treat "non-animating animated" images like static images. So I think we
> should return true for images which are (frameCount() == 1 ||
> !shouldAnimate()). And this is equivalent to !canAnimate().
That makes sense to me. Josh did this because the async decoding codepath has an assert that assumes decoding only happens for bitmap images that:

  1. Can't animate
  2. Have a m_currentFrame == 0 OR m_animationFinished

With this patch, animations can now be paused and played from any frame, breaking the assumption of (2). So maybe we need to change or remove this assert. What do you think?

https://github.com/WebKit/WebKit/blob/ac0b87b8f24429a12bd4999b5861e0829ca2a7bc/Source/WebCore/platform/graphics/BitmapImage.cpp#L251

Sample crashtrace I triggered locally, plus some logs:

ASSERTION FAILED: m_currentFrame 4, m_animationFinished 0
!m_currentFrame || m_animationFinished
platform/graphics/BitmapImage.cpp(251) : virtual WebCore::ImageDrawResult WebCore::BitmapImage::draw(WebCore::GraphicsContext &, const WebCore::FloatRect &, const WebCore::FloatRect &, const WebCore::ImagePaintingOptions &)
1   0x12cbf49d0 WTFCrash
2   0x14f2e90f4 WebCore::BitmapImage::draw(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&)
3   0x14f40e828 WebCore::Image::drawTiled(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatPoint const&, WebCore::FloatSize const&, WebCore::FloatSize const&, WebCore::ImagePaintingOptions const&)
4   0x14f3e58cc WebCore::GraphicsContext::drawTiledImage(WebCore::Image&, WebCore::FloatRect const&, WebCore::FloatPoint const&, WebCore::FloatSize const&, WebCore::FloatSize const&, WebCore::ImagePaintingOptions const&)
5   0x14f899fd4 WebCore::BackgroundPainter::paintFillLayer(WebCore::Color const&, WebCore::FillLayer const&, WebCore::LayoutRect const&, WebCore::BackgroundBleedAvoidance, WebCore::InlineIterator::InlineBoxIterator const&, WebCore::LayoutRect const&, WebCore::CompositeOperator, WebCore::RenderElement*, WebCore::BaseBackgroundColorUsage)
 
> > Source/WebCore/platform/graphics/BitmapImage.cpp:640
> > +    if (imageObserver() && !imageObserver()->allowsAnimation(*this))
> > +        return;
> 
> Why do we have this early return? We do a few bookkeeping and callbacks
> after this new if-statement. I do not see why disallowing the animation make
> us skip all this code.
This avoids a similar ASSERT to the one above:

ASSERT(index == m_currentFrame && !m_currentFrame);

https://github.com/WebKit/WebKit/blob/ac0b87b8f24429a12bd4999b5861e0829ca2a7bc/Source/WebCore/platform/graphics/BitmapImage.cpp#L639

ASSERTION FAILED: index 32, m_currentFrame 31
index == m_currentFrame && !m_currentFrame
platform/graphics/BitmapImage.cpp(643) : void WebCore::BitmapImage::imageFrameAvailableAtIndex(size_t)
1   0x12ff709d0 WTFCrash
2   0x2847afed4 WebCore::BitmapImage::imageFrameAvailableAtIndex(unsigned long)
3   0x2848e9e78 WebCore::ImageSource::cachePlatformImageAtIndexAsync(WTF::RetainPtr<CGImage*>&&, unsigned long, WebCore::SubsamplingLevel, WebCore::DecodingOptions const&, WebCore::DecodingStatus)
4   0x2848f6028 WebCore::ImageSource::startAsyncDecodingQueue()::$_3::operator()()::'lambda'()::operator()()
5   0x2848f5dbc WTF::Detail::CallableWrapper<WebCore::ImageSource::startAsyncDecodingQueue()::$_3::operator()()::'lambda'(), void>::call()
6   0x12ff99774 WTF::Function<void ()>::operator()() const
7   0x13002b158 WTF::RunLoop::performWork()

If we change both of these ASSERTs, I think we can remove the behavior changes you highlighted in your comment.

[Said Abou-Hallawa]

Comment on attachment 462256 [details]
Patch

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

>>> Source/WebCore/platform/graphics/BitmapImage.cpp:387
>>> +    return !shouldAnimate() && m_source->canUseAsyncDecoding() && !(frameCount() > 1);
>> 
>> I do not think this change is needed for this patch. Also I think it is okay to treat "non-animating animated" images like static images. So I think we should return true for images which are (frameCount() == 1 || !shouldAnimate()). And this is equivalent to !canAnimate().
> 
> That makes sense to me. Josh did this because the async decoding codepath has an assert that assumes decoding only happens for bitmap images that:
> 
>   1. Can't animate
>   2. Have a m_currentFrame == 0 OR m_animationFinished
> 
> With this patch, animations can now be paused and played from any frame, breaking the assumption of (2). So maybe we need to change or remove this assert. What do you think?
> 
> https://github.com/WebKit/WebKit/blob/ac0b87b8f24429a12bd4999b5861e0829ca2a7bc/Source/WebCore/platform/graphics/BitmapImage.cpp#L251
> 
> Sample crashtrace I triggered locally, plus some logs:
> 
> ASSERTION FAILED: m_currentFrame 4, m_animationFinished 0
> !m_currentFrame || m_animationFinished
> platform/graphics/BitmapImage.cpp(251) : virtual WebCore::ImageDrawResult WebCore::BitmapImage::draw(WebCore::GraphicsContext &, const WebCore::FloatRect &, const WebCore::FloatRect &, const WebCore::ImagePaintingOptions &)
> 1   0x12cbf49d0 WTFCrash
> 2   0x14f2e90f4 WebCore::BitmapImage::draw(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::ImagePaintingOptions const&)
> 3   0x14f40e828 WebCore::Image::drawTiled(WebCore::GraphicsContext&, WebCore::FloatRect const&, WebCore::FloatPoint const&, WebCore::FloatSize const&, WebCore::FloatSize const&, WebCore::ImagePaintingOptions const&)
> 4   0x14f3e58cc WebCore::GraphicsContext::drawTiledImage(WebCore::Image&, WebCore::FloatRect const&, WebCore::FloatPoint const&, WebCore::FloatSize const&, WebCore::FloatSize const&, WebCore::ImagePaintingOptions const&)
> 5   0x14f899fd4 WebCore::BackgroundPainter::paintFillLayer(WebCore::Color const&, WebCore::FillLayer const&, WebCore::LayoutRect const&, WebCore::BackgroundBleedAvoidance, WebCore::InlineIterator::InlineBoxIterator const&, WebCore::LayoutRect const&, WebCore::CompositeOperator, WebCore::RenderElement*, WebCore::BaseBackgroundColorUsage)

Yes I think we should change the assertion from 

    ASSERT(!canAnimate());
    ASSERT(!m_currentFrame || m_animationFinished);

to

    ASSERT(!m_currentFrame || !canAnimate());

>>> Source/WebCore/platform/graphics/BitmapImage.cpp:640
>>> +        return;
>> 
>> Why do we have this early return? We do a few bookkeeping and callbacks after this new if-statement. I do not see why disallowing the animation make us skip all this code.
> 
> This avoids a similar ASSERT to the one above:
> 
> ASSERT(index == m_currentFrame && !m_currentFrame);
> 
> https://github.com/WebKit/WebKit/blob/ac0b87b8f24429a12bd4999b5861e0829ca2a7bc/Source/WebCore/platform/graphics/BitmapImage.cpp#L639
> 
> ASSERTION FAILED: index 32, m_currentFrame 31
> index == m_currentFrame && !m_currentFrame
> platform/graphics/BitmapImage.cpp(643) : void WebCore::BitmapImage::imageFrameAvailableAtIndex(size_t)
> 1   0x12ff709d0 WTFCrash
> 2   0x2847afed4 WebCore::BitmapImage::imageFrameAvailableAtIndex(unsigned long)
> 3   0x2848e9e78 WebCore::ImageSource::cachePlatformImageAtIndexAsync(WTF::RetainPtr<CGImage*>&&, unsigned long, WebCore::SubsamplingLevel, WebCore::DecodingOptions const&, WebCore::DecodingStatus)
> 4   0x2848f6028 WebCore::ImageSource::startAsyncDecodingQueue()::$_3::operator()()::'lambda'()::operator()()
> 5   0x2848f5dbc WTF::Detail::CallableWrapper<WebCore::ImageSource::startAsyncDecodingQueue()::$_3::operator()()::'lambda'(), void>::call()
> 6   0x12ff99774 WTF::Function<void ()>::operator()() const
> 7   0x13002b158 WTF::RunLoop::performWork()
> 
> If we change both of these ASSERTs, I think we can remove the behavior changes you highlighted in your comment.

Yes I think we can change the assertion:

    ASSERT(index == m_currentFrame && !m_currentFrame);

to

    ASSERT((!index && !m_currentFrame) || !canAnimate());

[Tyler Wilcock]

Created attachment 462299 [details]
Patch

[EWS]

Committed 254469@main (55b3d145b200): <https://commits.webkit.org/254469@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 462299 [details].