[Web Animations] Starting a transform animation with a 1ms delay doesn't run it accel...
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Feb 2022 13:57:38 +0000 (13:57 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 7 Feb 2022 13:57:38 +0000 (13:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=236080
<rdar://problem/88432373>

Reviewed by Dean Jackson.

Source/WebCore:

Accelerated animations can only be started if the keyframe effect's renderer is composited.
Keyframe effects enqueue a list of accelerated actions (play, pause, seek, stop, etc.) when
they can be accelerated, and these actions are performed in sync as the document timeline
is finishing up its "update animations and send events" procedure as we update the page
rendering.

When an animation is started _without_ a delay, the machinery to consider making a renderer
composited happens _before_ we update animations and send events, and since at this stage the
new animation's effect is already in its target's effect stack because it is immediately
"relevant" (per the Web Animations terminology) and targets a property that can be accelerated,
its renderer is indeed composited.

Thus by the time we consider starting this animation's effect with acceleration, it is in a
condition to do so since its renderer is already composited.

However, when an animation is started _with_ a delay, it is not immediately "relevant" and thus
won't appear in its target's effect stack. That will happen once we run the "update animations
and send events procedure" the next time we update the page rendering. But before that, the
effect's renderer will be considered to be made composited and _will not_ be because at this
point there is no effect animating a property that can be accelerated.

As we eventually run the "update animations and send events" procedure, we'll enqueue an
accelerated action to play the animation now that it became relevant, and when that procedure
completes and we try to apply that accelerated action, we'll fail to start an accelerated
animation since the renderer is not composited.

To address this, we only enqueue accelerated actions when the renderer is composited, thus
matching the condition to actually be able to apply this action.

This means we no longer need the animationDidPlay() on effects since we wouldn't be able to
honor enqueuing a "play" accelerated action since the renderer won't be composited yet at this
time in the vast majority of cases.

Test: webanimations/transform-animation-with-delay-yields-accelerated-animation.html

* animation/AnimationEffect.h:
(WebCore::AnimationEffect::animationDidTick):
(WebCore::AnimationEffect::animationDidPlay): Deleted.
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::updateAcceleratedActions):
(WebCore::KeyframeEffect::animationDidPlay): Deleted.
* animation/KeyframeEffect.h:
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::play):

* animation/AnimationEffect.h:
(WebCore::AnimationEffect::animationDidTick):
(WebCore::AnimationEffect::animationDidPlay): Deleted.
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::updateAcceleratedActions):
(WebCore::KeyframeEffect::animationDidPlay): Deleted.
* animation/KeyframeEffect.h:
(WebCore::KeyframeEffect::isAboutToRunAccelerated const):
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::play):

LayoutTests:

Add a test that runs a "transform" animation with a small delay and checks that it yielded
an accelerated animation. We also rewrite some existing tests to use internals.acceleratedAnimationsForElement()
since relying on a layer tree dump was not reliable, which also resolves some platform-specific
issues.

* platform/glib/TestExpectations:
* platform/gtk/webanimations/partly-accelerated-transition-by-removing-property-expected.txt: Removed.
* platform/mac-wk1/webanimations/partly-accelerated-transition-by-removing-property-expected.txt: Removed.
* platform/win/TestExpectations:
* webanimations/accelerated-css-transition-with-easing-y-axis-above-1.html: Make sure overflow doesn't cause an image failure.
* webanimations/partly-accelerated-transition-by-removing-property-expected.txt:
* webanimations/partly-accelerated-transition-by-removing-property.html: Rewrite test to check on the accelerated
animation count and ensure the element is already composited to make the test work on WK1 and GTK.
* webanimations/resources/request-frames-until-true.js: Added.
(const.requestFramesUntilTrue.async resolveCondition):
* webanimations/transform-animation-with-delay-yields-accelerated-animation-expected.txt: Added.
* webanimations/transform-animation-with-delay-yields-accelerated-animation.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@289211 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/glib/TestExpectations
LayoutTests/platform/gtk/webanimations/partly-accelerated-transition-by-removing-property-expected.txt [deleted file]
LayoutTests/platform/mac-wk1/webanimations/partly-accelerated-transition-by-removing-property-expected.txt [deleted file]
LayoutTests/platform/win/TestExpectations
LayoutTests/webanimations/accelerated-css-transition-with-easing-y-axis-above-1.html
LayoutTests/webanimations/partly-accelerated-transition-by-removing-property-expected.txt
LayoutTests/webanimations/partly-accelerated-transition-by-removing-property.html
LayoutTests/webanimations/resources/request-frames-until-true.js [new file with mode: 0644]
LayoutTests/webanimations/transform-animation-with-delay-yields-accelerated-animation-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/transform-animation-with-delay-yields-accelerated-animation.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/AnimationEffect.h
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/animation/KeyframeEffect.h
Source/WebCore/animation/WebAnimation.cpp

index 82f2d0923a839485355c97a78610fc909a41c6db..81f30b7dcb4d549c10b9a28f5b2050c16814a477 100644 (file)
@@ -1,3 +1,29 @@
+2022-02-07  Antoine Quint  <graouts@webkit.org>
+
+        [Web Animations] Starting a transform animation with a 1ms delay doesn't run it accelerated
+        https://bugs.webkit.org/show_bug.cgi?id=236080
+        <rdar://problem/88432373>
+
+        Reviewed by Dean Jackson.
+
+        Add a test that runs a "transform" animation with a small delay and checks that it yielded
+        an accelerated animation. We also rewrite some existing tests to use internals.acceleratedAnimationsForElement()
+        since relying on a layer tree dump was not reliable, which also resolves some platform-specific
+        issues.
+
+        * platform/glib/TestExpectations:
+        * platform/gtk/webanimations/partly-accelerated-transition-by-removing-property-expected.txt: Removed.
+        * platform/mac-wk1/webanimations/partly-accelerated-transition-by-removing-property-expected.txt: Removed.
+        * platform/win/TestExpectations:
+        * webanimations/accelerated-css-transition-with-easing-y-axis-above-1.html: Make sure overflow doesn't cause an image failure.
+        * webanimations/partly-accelerated-transition-by-removing-property-expected.txt:
+        * webanimations/partly-accelerated-transition-by-removing-property.html: Rewrite test to check on the accelerated
+        animation count and ensure the element is already composited to make the test work on WK1 and GTK.
+        * webanimations/resources/request-frames-until-true.js: Added.
+        (const.requestFramesUntilTrue.async resolveCondition):
+        * webanimations/transform-animation-with-delay-yields-accelerated-animation-expected.txt: Added.
+        * webanimations/transform-animation-with-delay-yields-accelerated-animation.html: Added.
+
 2022-02-07  Rob Buis  <rbuis@igalia.com>
 
         Bail out early in stopForUserCancel
index a03b1952bf9c9f0c6bc68c004694ff57e3ae1e4a..e1901f43cda72484bc6574b3e3d89467435c3f20 100644 (file)
@@ -1941,6 +1941,10 @@ http/tests/model/ [ Skip ]
 # The DocumentTimeline.maximumFrameRate property which this test is about is not available on GTK/WPE.
 webanimations/frame-rate/document-timeline-maximum-frame-rate.html [ Skip ]
 
+# This test requires an internal API which only works on Cocoa ports.
+webanimations/transform-animation-with-delay-yields-accelerated-animation.html [ Skip ]
+webanimations/partly-accelerated-transition-by-removing-property.html [ Skip ]
+
 # OT-SVG is not implemented on GTK/WPE
 fast/text/otsvg-canvas.html [ ImageOnlyFailure ]
 
diff --git a/LayoutTests/platform/gtk/webanimations/partly-accelerated-transition-by-removing-property-expected.txt b/LayoutTests/platform/gtk/webanimations/partly-accelerated-transition-by-removing-property-expected.txt
deleted file mode 100644 (file)
index 8b13789..0000000
+++ /dev/null
@@ -1 +0,0 @@
-
diff --git a/LayoutTests/platform/mac-wk1/webanimations/partly-accelerated-transition-by-removing-property-expected.txt b/LayoutTests/platform/mac-wk1/webanimations/partly-accelerated-transition-by-removing-property-expected.txt
deleted file mode 100644 (file)
index 8b13789..0000000
+++ /dev/null
@@ -1 +0,0 @@
-
index 0ab13b52b37618ccdf4a6680b9d1dde665fcfc3e..473607f9091e1b950c6196d137b94547b198d2a6 100644 (file)
@@ -4348,7 +4348,6 @@ webkit.org/b/189836 webanimations/accelerated-animation-suspension.html [ Skip ]
 [ Win10 ] http/tests/security/local-video-poster-from-remote.html [ Failure ]
 [ Win10 ] storage/indexeddb/modern/idbindex-getallkeys-1.html [ Failure ]
 [ Win10 ] transforms/2d/hindi-rotated.html [ Failure ]
-[ Win10 ] webanimations/partly-accelerated-transition-by-removing-property.html [ Failure ]
 
 [ Win10 ] fast/repaint/block-no-inflow-children.html [ ImageOnlyFailure ]
 [ Win10 ] imported/blink/fast/borders/border-radius-with-layer.html [ ImageOnlyFailure ]
@@ -4957,6 +4956,10 @@ fast/text/otsvg-canvas.html [ ImageOnlyFailure ]
 # The DocumentTimeline.maximumFrameRate property which this test is about is not available on Windows.
 webanimations/frame-rate/document-timeline-maximum-frame-rate.html [ Skip ]
 
+# This test requires an internal API which only works on Cocoa ports.
+webanimations/transform-animation-with-delay-yields-accelerated-animation.html [ Skip ]
+webanimations/partly-accelerated-transition-by-removing-property.html [ Skip ]
+
 pointerevents/mouse/pointer-button-and-buttons.html [ Failure ]
 pointerevents/mouse/pointer-capture.html [ Failure ]
 
index 8a5d2e36bc364c5f0e8b120e64e2114d7d4ec75d..816314a01925b331123833210dcf096be37b74c3 100644 (file)
@@ -3,6 +3,7 @@
     
     body {
         background-color: red;
+        overflow: hidden;
     }
     
     div {
index 0386ee02feb14c7c9a6d35b5868e4f2905c0a65b..d79c0555cc26e6fd462cd7f6d3eeaa7af39f0b11 100644 (file)
@@ -1,19 +1,3 @@
-(GraphicsLayer
-  (anchor 0.00 0.00)
-  (bounds 800.00 600.00)
-  (children 1
-    (GraphicsLayer
-      (bounds 800.00 600.00)
-      (contentsOpaque 1)
-      (children 1
-        (GraphicsLayer
-          (position 100.00 0.00)
-          (bounds 100.00 100.00)
-          (opacity 0.50)
-          (contentsOpaque 1)
-        )
-      )
-    )
-  )
-)
+
+PASS Transtioning two properties, one of which can be accelerated while the other cannot, yields only one accelerated animation.
 
index fa3c855dbcb26e8d997c1252428832c911187c8f..adeb341435c056ca242e897fac37a96d162d4800 100644 (file)
@@ -1,29 +1,48 @@
 <!DOCTYPE html>
 <body>
-<pre id="results"></pre>
-<div id="target" style="position: absolute; top: 0; left: 0; width: 100px; height: 100px; background-color: black;"></div>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<script src="resources/request-frames-until-true.js"></script>
+<style>
+
+    #target {
+        position: absolute;
+        left: 0;
+        top: 0;
+        width: 100px;
+        height: 100px;
+        background-color: black;
+        will-change: opacity;
+    }
+
+</style>
+<div id="target"></div>
 <script>
 
-if (window.testRunner) {
-    testRunner.waitUntilDone();
-    testRunner.dumpAsText();
-}
+promise_test(async t => {
+    const target = document.getElementById("target");
+
+    const acceleratedAnimationsStarted = async numberOfAnimations => {
+        return requestFramesUntilTrue(
+            () => internals.acceleratedAnimationsForElement(target).length == numberOfAnimations,
+            () => !"acceleratedAnimationsForElement" in window.internals);
+    };
 
-const target = document.getElementById("target");
-target.style.opacity = "0.5";
-target.style.marginLeft = "100px";
-setTimeout(() => {
+    // Set initial styles.
+    target.style.opacity = "0.5";
+    target.style.marginLeft = "100px";
+
+    await new Promise(setTimeout);
+
+    // Now update styles to trigger a transition for properties, only one of which should yield an accelerated animation.
     target.style.removeProperty("opacity");
     target.style.removeProperty("margin-left");
     target.style.transitionProperty = "margin-left opacity";
     target.style.transitionDuration = "1s";
-    requestAnimationFrame(() => {
-        if (window.internals)
-            document.getElementById("results").innerText = internals.layerTreeAsText(document);
-        if (window.testRunner)
-            testRunner.notifyDone();
-    });
-});
+
+    // Wait a few frames for the opacity animation to be commited.
+    await acceleratedAnimationsStarted(1);
+}, "Transtioning two properties, one of which can be accelerated while the other cannot, yields only one accelerated animation.");
 
 </script>
 </body>
diff --git a/LayoutTests/webanimations/resources/request-frames-until-true.js b/LayoutTests/webanimations/resources/request-frames-until-true.js
new file mode 100644 (file)
index 0000000..cfe21c9
--- /dev/null
@@ -0,0 +1,16 @@
+
+const requestFramesUntilTrue = async (resolveCondition, rejectCondition) => {
+    return new Promise((resolve, reject) => {
+        if (rejectCondition && rejectCondition()) {
+            reject();
+            return;
+        }
+
+        (function tryFrame () {
+            if (resolveCondition())
+                resolve();
+            else
+                requestAnimationFrame(tryFrame);
+        })();
+    });
+};
diff --git a/LayoutTests/webanimations/transform-animation-with-delay-yields-accelerated-animation-expected.txt b/LayoutTests/webanimations/transform-animation-with-delay-yields-accelerated-animation-expected.txt
new file mode 100644 (file)
index 0000000..2083be8
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS A transform animation with a delay should run accelerated.
+
diff --git a/LayoutTests/webanimations/transform-animation-with-delay-yields-accelerated-animation.html b/LayoutTests/webanimations/transform-animation-with-delay-yields-accelerated-animation.html
new file mode 100644 (file)
index 0000000..2b6b2de
--- /dev/null
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<body>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<style>
+
+    #target {
+        position: absolute;
+        left: 0;
+        top: 0;
+        width: 100px;
+        height: 100px;
+        background-color: black;
+    }
+
+</style>
+<div id="target"></div>
+<script>
+
+promise_test(async t => {
+    // Start an accelerated "transform" animation with a tiny delay.
+    const animation = document.getElementById("target").animate({ transform: "translateX(600px)" }, { duration: 1000 * 1000, delay: 1 });
+
+    // Wait two frames for the accelerated animation to be committed.
+    await animation.ready;
+    await new Promise(requestAnimationFrame);
+    await new Promise(requestAnimationFrame);
+    // And another one since the delay means it will take another frame until the animation enters its active phase.
+    await new Promise(requestAnimationFrame);
+
+    assert_equals(internals.acceleratedAnimationsForElement(target).length, 1, "There should be an accelerated animation.");
+}, "A transform animation with a delay should run accelerated.");
+
+</script>
+</body>
index 8d363a3aa7194eb0452fa3e037cdb5a93272424d..e769eeeaef0e4df9fb570c0133248bd007f9614c 100644 (file)
@@ -1,3 +1,67 @@
+2022-02-07  Antoine Quint  <graouts@webkit.org>
+
+        [Web Animations] Starting a transform animation with a 1ms delay doesn't run it accelerated
+        https://bugs.webkit.org/show_bug.cgi?id=236080
+        <rdar://problem/88432373>
+
+        Reviewed by Dean Jackson.
+
+        Accelerated animations can only be started if the keyframe effect's renderer is composited.
+        Keyframe effects enqueue a list of accelerated actions (play, pause, seek, stop, etc.) when
+        they can be accelerated, and these actions are performed in sync as the document timeline
+        is finishing up its "update animations and send events" procedure as we update the page
+        rendering.
+
+        When an animation is started _without_ a delay, the machinery to consider making a renderer
+        composited happens _before_ we update animations and send events, and since at this stage the
+        new animation's effect is already in its target's effect stack because it is immediately
+        "relevant" (per the Web Animations terminology) and targets a property that can be accelerated,
+        its renderer is indeed composited.
+
+        Thus by the time we consider starting this animation's effect with acceleration, it is in a
+        condition to do so since its renderer is already composited.
+
+        However, when an animation is started _with_ a delay, it is not immediately "relevant" and thus
+        won't appear in its target's effect stack. That will happen once we run the "update animations
+        and send events procedure" the next time we update the page rendering. But before that, the
+        effect's renderer will be considered to be made composited and _will not_ be because at this
+        point there is no effect animating a property that can be accelerated.
+
+        As we eventually run the "update animations and send events" procedure, we'll enqueue an
+        accelerated action to play the animation now that it became relevant, and when that procedure
+        completes and we try to apply that accelerated action, we'll fail to start an accelerated
+        animation since the renderer is not composited.
+
+        To address this, we only enqueue accelerated actions when the renderer is composited, thus
+        matching the condition to actually be able to apply this action.
+
+        This means we no longer need the animationDidPlay() on effects since we wouldn't be able to
+        honor enqueuing a "play" accelerated action since the renderer won't be composited yet at this
+        time in the vast majority of cases.
+
+        Test: webanimations/transform-animation-with-delay-yields-accelerated-animation.html
+
+        * animation/AnimationEffect.h:
+        (WebCore::AnimationEffect::animationDidTick):
+        (WebCore::AnimationEffect::animationDidPlay): Deleted.
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::updateAcceleratedActions):
+        (WebCore::KeyframeEffect::animationDidPlay): Deleted.
+        * animation/KeyframeEffect.h:
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::play):
+
+        * animation/AnimationEffect.h:
+        (WebCore::AnimationEffect::animationDidTick):
+        (WebCore::AnimationEffect::animationDidPlay): Deleted.
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::updateAcceleratedActions):
+        (WebCore::KeyframeEffect::animationDidPlay): Deleted.
+        * animation/KeyframeEffect.h:
+        (WebCore::KeyframeEffect::isAboutToRunAccelerated const):
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::play):
+
 2022-02-07  Nikolas Zimmermann  <nzimmermann@igalia.com>
 
         [LBSE] Generalize RenderLayer::renderBoxLocation(), adding support for SVG layers
index 93eb219a68b662108710a85e68f4e8f8ebc2e7c9..eea1c0be2f022662d5537a49f169603a6fd3f2fa 100644 (file)
@@ -63,7 +63,6 @@ public:
     ExceptionOr<void> updateTiming(std::optional<OptionalEffectTiming>);
 
     virtual void animationDidTick() { };
-    virtual void animationDidPlay() { };
     virtual void animationDidChangeTimingProperties() { };
     virtual void animationWasCanceled() { };
     virtual void animationSuspensionStateDidChange(bool) { };
index af3d69ebc2b4c15358d7e0f1ebcd0a1e85642e36..7cc77f84df13924645a11aa20f459cbac7c66835 100644 (file)
@@ -1616,6 +1616,10 @@ bool KeyframeEffect::canBeAccelerated() const
 
 void KeyframeEffect::updateAcceleratedActions()
 {
+    auto* renderer = this->renderer();
+    if (!renderer || !renderer->isComposited())
+        return;
+
     if (!canBeAccelerated()) {
         // In the case where this animation is actively targeting a transform-related property and yet
         // cannot be accelerated, we must notify the effect stack such that any running accelerated
@@ -1680,12 +1684,6 @@ void KeyframeEffect::animationDidTick()
     updateAcceleratedActions();
 }
 
-void KeyframeEffect::animationDidPlay()
-{
-    if (m_acceleratedPropertiesState != AcceleratedProperties::None)
-        addPendingAcceleratedAction(AcceleratedAction::Play);
-}
-
 void KeyframeEffect::animationDidChangeTimingProperties()
 {
     computeSomeKeyframesUseStepsTimingFunction();
index acb29ae1c4f9df41b20691e1f434eb24279d398b..7f9e6e6882a1057fd0664f2c8f7cf8734b7ce72f 100644 (file)
@@ -135,7 +135,8 @@ public:
     bool triggersStackingContext() const { return m_triggersStackingContext; }
     bool isRunningAccelerated() const { return m_runningAccelerated == RunningAccelerated::Yes; }
 
-    bool isAboutToRunAccelerated() const { return canBeAccelerated() && m_lastRecordedAcceleratedAction != AcceleratedAction::Stop; }
+    // FIXME: These ignore the fact that some timing functions can prevent acceleration.
+    bool isAboutToRunAccelerated() const { return m_acceleratedPropertiesState != AcceleratedProperties::None && m_lastRecordedAcceleratedAction != AcceleratedAction::Stop; }
 
     bool filterFunctionListsMatch() const override { return m_filterFunctionListsMatch; }
     bool transformFunctionListsMatch() const override { return m_transformFunctionListsMatch; }
@@ -210,7 +211,6 @@ private:
     // AnimationEffect
     bool isKeyframeEffect() const final { return true; }
     void animationDidTick() final;
-    void animationDidPlay() final;
     void animationDidChangeTimingProperties() final;
     void animationWasCanceled() final;
     void animationSuspensionStateDidChange(bool) final;
index f20bbcfdcd44650f150ceb7e0f23ff803037cff9..7ca95e891b058e8f44bc76739d71bf8133fcdac6 100644 (file)
@@ -1047,9 +1047,6 @@ ExceptionOr<void> WebAnimation::play(AutoRewind autoRewind)
 
     invalidateEffect();
 
-    if (m_effect)
-        m_effect->animationDidPlay();
-
     return { };
 }