[Webkit-unassigned] [Bug 244128] Add experimental feature to disable Bitmap image animations
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 30 18:03:04 PDT 2022
https://bugs.webkit.org/show_bug.cgi?id=244128
--- Comment #37 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 462012
--> https://bugs.webkit.org/attachment.cgi?id=462012
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?
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220831/5e4bebc8/attachment-0001.htm>
More information about the webkit-unassigned
mailing list