AX: Avoid work in BitmapImage::canAnimate if animation controls are d…

…isabled https://bugs.webkit.org/show_bug.cgi?id=270136 rdar://109646792 Reviewed by Tyler Wilcock. Before this patch, the expensive work that happens in BitmapImage::canAnimate would unnecessarily occur in two cases: (1) if the image has a single-frame and (2) if image animation controls were not enabled. This patch exits early from this code path if either of those two things are true. This will improve performance for users both with and without the controls enabled. Moving `systemAllowsAnimationControls` to a static global variable on `Image` also allows us to remove the method of the same name from `Page`, since this does not need to be a per-page setting. An API test had to be updated so that we invoke `Image::setSystemAllowsImageAnimation` via Internals::setImageAnimationEnabled. This is part one of related work. In a follow up, performance issues for computing canAnimate when animation controls are enabled will be addressed. * Source/WebCore/loader/cache/CachedImage.cpp: (WebCore::CachedImage::CachedImageObserver::allowsAnimation const): (WebCore::CachedImage::allowsAnimation const): * Source/WebCore/page/ContextMenuController.cpp: (WebCore::ContextMenuController::populate): * Source/WebCore/page/Page.cpp: (WebCore::Page::setImageAnimationEnabled): (WebCore::Page::setSystemAllowsAnimationControls): Deleted. * Source/WebCore/page/Page.h: (WebCore::Page::imageAnimationEnabled const): (WebCore::Page::systemAllowsAnimationControls const): Deleted. * Source/WebCore/platform/graphics/BitmapImage.cpp: (WebCore::BitmapImage::canAnimate const): * Source/WebCore/platform/graphics/Image.cpp: (WebCore::Image::setSystemAllowsAnimationControls): * Source/WebCore/platform/graphics/Image.h: (WebCore::Image::systemAllowsAnimationControls): * Source/WebCore/testing/Internals.cpp: (WebCore::Internals::setImageAnimationEnabled): * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::updateImageAnimationEnabled): * Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm: (WebKit::WebProcess::updatePageAccessibilitySettings): * Tools/TestWebKitAPI/Tests/WebKit/AnimationControl.mm: (TEST): Canonical link: https://commits.webkit.org/275563@main
WebKit · Mar 1, 2024 · 1d7be2a · 1d7be2a
1 parent 9ab0a13
commit 1d7be2a
Show file tree

Hide file tree

Showing 11 changed files with 38 additions and 13 deletions.
diff --git a/Source/WebCore/loader/cache/CachedImage.cpp b/Source/WebCore/loader/cache/CachedImage.cpp
@@ -471,6 +471,11 @@ void CachedImage::CachedImageObserver::scheduleRenderingUpdate(const Image& imag
 
 bool CachedImage::CachedImageObserver::allowsAnimation(const Image& image) const
 {
+    // *::allowsAnimation can only return false when systemAllowsAnimationControls == true,
+    // so this prevents unnecessary work by exiting early.
+    if (!Image::systemAllowsAnimationControls())
+        return true;
+
     for (CachedResourceHandle cachedImage : m_cachedImages) {
         if (cachedImage->allowsAnimation(image))
             return true;
@@ -736,6 +741,9 @@ bool CachedImage::allowsAnimation(const Image& image) const
     if (&image != m_image)
         return false;
 
+    if (!Image::systemAllowsAnimationControls())
+        return true;
+
     CachedResourceClientWalker<CachedImageClient> walker(*this);
     while (auto* client = walker.next()) {
         if (!client->allowsAnimation())

diff --git a/Source/WebCore/page/ContextMenuController.cpp b/Source/WebCore/page/ContextMenuController.cpp
@@ -1056,7 +1056,7 @@ void ContextMenuController::populate()
         if (!frame->page() || !frame->page()->settings().imageAnimationControlEnabled())
             return false;
 
-        return frame->page()->systemAllowsAnimationControls() || frame->page()->settings().allowAnimationControlsOverride();
+        return Image::systemAllowsAnimationControls() || frame->page()->settings().allowAnimationControlsOverride();
     };
 
     bool shouldAddPlayAllPauseAllAnimationsItem = canAddAnimationControls();

diff --git a/Source/WebCore/page/Page.cpp b/Source/WebCore/page/Page.cpp
@@ -2247,11 +2247,6 @@ void Page::setImageAnimationEnabled(bool enabled)
     updatePlayStateForAllAnimations();
     chrome().client().isAnyAnimationAllowedToPlayDidChange(enabled);
 }
-
-void Page::setSystemAllowsAnimationControls(bool isAllowed)
-{
-    m_systemAllowsAnimationControls = isAllowed;
-}
 #endif // ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)
 
 #if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR)

diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h
@@ -709,12 +709,10 @@ class Page : public RefCounted<Page>, public Supplementable<Page>, public CanMak
 #if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)
     void updatePlayStateForAllAnimations();
     WEBCORE_EXPORT void setImageAnimationEnabled(bool);
-    WEBCORE_EXPORT void setSystemAllowsAnimationControls(bool isAllowed);
     void addIndividuallyPlayingAnimationElement(HTMLImageElement&);
     void removeIndividuallyPlayingAnimationElement(HTMLImageElement&);
 #endif
     bool imageAnimationEnabled() const { return m_imageAnimationEnabled; }
-    bool systemAllowsAnimationControls() const { return m_systemAllowsAnimationControls; }
 
 #if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR)
     WEBCORE_EXPORT void setPrefersNonBlinkingCursor(bool);
@@ -1264,7 +1262,6 @@ class Page : public RefCounted<Page>, public Supplementable<Page>, public CanMak
 
     bool m_canStartMedia { true };
     bool m_imageAnimationEnabled { true };
-    bool m_systemAllowsAnimationControls { false };
     // Elements containing animations that are individually playing (potentially overriding the page-wide m_imageAnimationEnabled state).
     WeakHashSet<HTMLImageElement, WeakPtrImplWithEventTargetData> m_individuallyPlayingAnimationElements;
 #if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR)

diff --git a/Source/WebCore/platform/graphics/BitmapImage.cpp b/Source/WebCore/platform/graphics/BitmapImage.cpp
@@ -345,7 +345,7 @@ bool BitmapImage::shouldAnimate() const
 
 bool BitmapImage::canAnimate() const
 {
-    return shouldAnimate() && frameCount() > 1;
+    return frameCount() > 1 && shouldAnimate();
 }
 
 bool BitmapImage::canUseAsyncDecodingForLargeImages() const

diff --git a/Source/WebCore/platform/graphics/Image.cpp b/Source/WebCore/platform/graphics/Image.cpp
@@ -444,4 +444,11 @@ TextStream& operator<<(TextStream& ts, const Image& image)
     return ts;
 }
 
+bool Image::gSystemAllowsAnimationControls = false;
+
+void Image::setSystemAllowsAnimationControls(bool allowsControls)
+{
+    gSystemAllowsAnimationControls = allowsControls;
+}
+
 } // namespace WebCore
diff --git a/Source/WebCore/platform/graphics/Image.h b/Source/WebCore/platform/graphics/Image.h
@@ -133,6 +133,8 @@ class Image : public RefCounted<Image>, public CanMakeWeakPtr<Image> {
     bool animationPending() const { return m_animationStartTimer && m_animationStartTimer->isActive(); }
     std::optional<bool> allowsAnimation() const { return m_allowsAnimation; }
     void setAllowsAnimation(std::optional<bool> allowsAnimation) { m_allowsAnimation = allowsAnimation; }
+    static bool systemAllowsAnimationControls() { return gSystemAllowsAnimationControls; }
+    WEBCORE_EXPORT static void setSystemAllowsAnimationControls(bool allowsControls);
 
     // Typically the CachedImage that owns us.
     RefPtr<ImageObserver> imageObserver() const;
@@ -185,6 +187,7 @@ class Image : public RefCounted<Image>, public CanMakeWeakPtr<Image> {
     // A value of true or false will override the default Page::imageAnimationEnabled state.
     std::optional<bool> m_allowsAnimation { std::nullopt };
     std::unique_ptr<Timer> m_animationStartTimer;
+    static bool gSystemAllowsAnimationControls;
 };
 
 WTF::TextStream& operator<<(WTF::TextStream&, const Image&);

diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp
@@ -1124,8 +1124,14 @@ bool Internals::isImageAnimating(HTMLImageElement& element)
 #if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)
 void Internals::setImageAnimationEnabled(bool enabled)
 {
-    if (auto* page = contextDocument() ? contextDocument()->page() : nullptr)
+    if (auto* page = contextDocument() ? contextDocument()->page() : nullptr) {
+        if (!page->settings().imageAnimationControlEnabled())
+            return;
+
+        // We need to set this here to mimic the behavior of the AX preference changing
+        Image::setSystemAllowsAnimationControls(!enabled);
         page->setImageAnimationEnabled(enabled);
+    }
 }
 
 void Internals::resumeImageAnimation(HTMLImageElement& element)

diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
@@ -9121,7 +9121,6 @@ void WebPage::generateTestReport(String&& message, String&& group)
 void WebPage::updateImageAnimationEnabled()
 {
     corePage()->setImageAnimationEnabled(WebProcess::singleton().imageAnimationEnabled());
-    corePage()->setSystemAllowsAnimationControls(!WebProcess::singleton().imageAnimationEnabled());
 }
 
 void WebPage::pauseAllAnimations(CompletionHandler<void()>&& completionHandler)

diff --git a/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm b/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm
@@ -76,6 +76,7 @@
 #import <WebCore/HistoryController.h>
 #import <WebCore/HistoryItem.h>
 #import <WebCore/IOSurface.h>
+#import <WebCore/Image.h>
 #import <WebCore/ImageDecoderCG.h>
 #import <WebCore/LocalFrameView.h>
 #import <WebCore/LocalizedDeviceModel.h>
@@ -1278,6 +1279,10 @@ static float currentBacklightLevel()
 
 void WebProcess::updatePageAccessibilitySettings()
 {
+#if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)
+    Image::setSystemAllowsAnimationControls(!imageAnimationEnabled());
+#endif
+
 #if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL) || ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR)
     for (auto& page : m_pageMap.values()) {
 #if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)

diff --git a/Tools/TestWebKitAPI/Tests/WebKit/AnimationControl.mm b/Tools/TestWebKitAPI/Tests/WebKit/AnimationControl.mm
@@ -77,8 +77,8 @@ static bool isAnimating(NSString *domID, RetainPtr<TestWKWebView> webView)
 
     [webView synchronouslyLoadHTMLString:@"<img id='imgOne' src='test-mse.mp4'><img id='imgTwo' src='test-without-audio-track.mp4'>"];
     [webView stringByEvaluatingJavaScript:@"window.internals.settings.setImageAnimationControlEnabled(true)"];
+    [webView stringByEvaluatingJavaScript:@"window.internals.setImageAnimationEnabled(false)"];
 
-    [webView _pauseAllAnimationsWithCompletionHandler:nil];
     while (isAnimating(@"imgOne", webView) || isAnimating(@"imgTwo", webView))
         TestWebKitAPI::Util::runFor(0.05_s);
     ASSERT_FALSE([webView _allowsAnyAnimationToPlay]);
@@ -100,5 +100,10 @@ static bool isAnimating(NSString *domID, RetainPtr<TestWKWebView> webView)
     while (![webView _allowsAnyAnimationToPlay])
         TestWebKitAPI::Util::runFor(0.05_s);
     ASSERT_TRUE([webView _allowsAnyAnimationToPlay]);
+
+    [webView _pauseAllAnimationsWithCompletionHandler:nil];
+    while (isAnimating(@"imgTwo", webView))
+        TestWebKitAPI::Util::runFor(0.05_s);
+    ASSERT_FALSE([webView _allowsAnyAnimationToPlay]);
 }
 #endif // ENABLE(ACCESSIBILITY_ANIMATION_CONTROL)