[Web Animations] Forward-filling animations should not schedule updates while filling
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Nov 2019 18:11:28 +0000 (18:11 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 29 Nov 2019 18:11:28 +0000 (18:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204697

Reviewed by Dean Jackson.

Source/WebCore:

Tests: webanimations/no-scheduling-while-filling-accelerated.html
       webanimations/no-scheduling-while-filling-non-accelerated.html

For two different reasons, we would continuously schedule updates for animations in their "after" phase, ie. when they would have "fill: forwards"
and be finished.

In the case of non-accelerated animations, that was because we would also schedule an immedate update in DocumentTimeline::scheduleNextTick() provided
we had relevant animations that weren't accelerated. But since animations in their "after" phase are still considered relevant, which means that they
would override the base style with an animated value, this caused the unnecessary scheduled updates.

To address this, we run WebAnimation::timeToNextTick() in DocumentTimeline::scheduleNextTick() for all relevant animations and we teach that function
to schedule an immediate update only during the "active" phase, and to schedule a timed update only in the "before" phase computing the delay until
the animation enters the "active" phase.

While performing this work, we found a couple of other issues that weren't apparent until we had this more efficient scheduling in place.

First, we would not allow any additional calls to DocumentTimeline::scheduleAnimationResolution() to actually schedule an update while we were in the
process of updating animations. While this wasn't a problem before because the mere presence of relevant animations would cause an upadte, we now had
to allow any animation changing timing properties as promises would resolve and events would be dispatched during the animation update to cause further
animation updates to be scheduled. So we moved the "m_animationResolutionScheduled" flag reset earlier in DocumentTimeline::updateAnimationsAndSendEvents()
before we actually run the animation update procedure.

Following that change, we also had to make sure that timing changes made through the evaluation of the pending play and pause tasks would _not_ cause
animations to be scheduled, which meant that an animation that became non-pending (ie. its first tick occured) would always schedule one immediate
animation update. So now we have an extra "silent" flag to WebAnimation::timingDidChange() which is only set to true when called from either
WebAnimation::runPendingPlayTask() or WebAnimation::runPendingPauseTask().

Finally, one existing test showed that calling KeyframeEffect::setKeyframes() while running animations didn't cause an animation update to be scheduled
since a call to WebAnimation::effectTimingDidChange() was lacking. This is now addressed.

As for accelerated animations, the extraneous animation scheduling occured because we would get in a state where we would record an "accelerated action"
to stop the accelerated animation but the accelerated animation had already completed and the renderer for the target element was no longer composited.
This meant that KeyframeEffect::applyPendingAcceleratedActions() would not perform any work and the pending accelerated action would remain in the
DocumentTimeline queue and cause an update to be scheduled to try again, endlessly.

To address that, we first check in KeyframeEffect::addPendingAcceleratedAction() if the recorded action differs from the last recorded action, avoiding
unnecessary work, and in KeyframeEffect::applyPendingAcceleratedActions(), if our last recorded action was "Stop" and the renderer was not composited,
we discard all pending accelerated actions.

* animation/DocumentTimeline.cpp:
(WebCore::DocumentTimeline::updateAnimationsAndSendEvents):
(WebCore::DocumentTimeline::scheduleNextTick):
* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::setKeyframes):
(WebCore::KeyframeEffect::addPendingAcceleratedAction):
(WebCore::KeyframeEffect::applyPendingAcceleratedActions):
* animation/WebAnimation.cpp:
(WebCore::WebAnimation::timingDidChange):
(WebCore::WebAnimation::runPendingPlayTask):
(WebCore::WebAnimation::runPendingPauseTask):
(WebCore::WebAnimation::timeToNextTick const):
* animation/WebAnimation.h:

LayoutTests:

Adding tests checking that we don't schedule animation updates while filling for accelerated and non-accelerated animations alike.

* webanimations/no-scheduling-while-filling-accelerated-expected.txt: Added.
* webanimations/no-scheduling-while-filling-accelerated.html: Added.
* webanimations/no-scheduling-while-filling-non-accelerated-expected.txt: Added.
* webanimations/no-scheduling-while-filling-non-accelerated.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/webanimations/no-scheduling-while-filling-accelerated-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/no-scheduling-while-filling-accelerated.html [new file with mode: 0644]
LayoutTests/webanimations/no-scheduling-while-filling-non-accelerated-expected.txt [new file with mode: 0644]
LayoutTests/webanimations/no-scheduling-while-filling-non-accelerated.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/animation/DocumentTimeline.cpp
Source/WebCore/animation/KeyframeEffect.cpp
Source/WebCore/animation/WebAnimation.cpp
Source/WebCore/animation/WebAnimation.h

index 84f922db54fc7013473174847c7053a4b56134c4..778f23d7b9f9f663ca128b62736d241ef4bba6e2 100644 (file)
@@ -1,3 +1,17 @@
+2019-11-29  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Forward-filling animations should not schedule updates while filling
+        https://bugs.webkit.org/show_bug.cgi?id=204697
+
+        Reviewed by Dean Jackson.
+
+        Adding tests checking that we don't schedule animation updates while filling for accelerated and non-accelerated animations alike.
+
+        * webanimations/no-scheduling-while-filling-accelerated-expected.txt: Added.
+        * webanimations/no-scheduling-while-filling-accelerated.html: Added.
+        * webanimations/no-scheduling-while-filling-non-accelerated-expected.txt: Added.
+        * webanimations/no-scheduling-while-filling-non-accelerated.html: Added.
+
 2019-11-28  Simon Fraser  <simon.fraser@apple.com>
 
         Element jumps to wrong position after perspective change on ancestor
diff --git a/LayoutTests/webanimations/no-scheduling-while-filling-accelerated-expected.txt b/LayoutTests/webanimations/no-scheduling-while-filling-accelerated-expected.txt
new file mode 100644 (file)
index 0000000..f395b60
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS There should not be more than one update made to the timeline after a forward-filling accelerated animation completes. 
+
diff --git a/LayoutTests/webanimations/no-scheduling-while-filling-accelerated.html b/LayoutTests/webanimations/no-scheduling-while-filling-accelerated.html
new file mode 100644 (file)
index 0000000..efee157
--- /dev/null
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+
+#target {
+    width: 100px;
+    height: 100px;
+    background-color: black;
+}
+
+</style>
+</head>
+<body>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<div id="target"></div>
+<script>
+
+function waitNFrames(numberOfFrames, continuation)
+{
+    let elapsedFrames = 0;
+    (function rAFCallback() {
+        if (elapsedFrames++ >= numberOfFrames)
+            continuation();
+        else
+            requestAnimationFrame(rAFCallback);
+    })();
+}
+
+async_test(t => {
+    const keyframes = { transform: ["translateX(50px)", "translateX(100px)"] };
+    const timing = { duration: 50, fill: "forwards" };
+    document.getElementById("target").animate(keyframes, timing).addEventListener("finish", event => {
+        const numberOfTimelineInvalidationsWhenFinished = internals.numberOfAnimationTimelineInvalidations();
+        waitNFrames(3, () => {
+            assert_equals(internals.numberOfAnimationTimelineInvalidations(), numberOfTimelineInvalidationsWhenFinished + 1);
+            t.done();
+        });
+    });
+}, "There should not be more than one update made to the timeline after a forward-filling accelerated animation completes.");
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/webanimations/no-scheduling-while-filling-non-accelerated-expected.txt b/LayoutTests/webanimations/no-scheduling-while-filling-non-accelerated-expected.txt
new file mode 100644 (file)
index 0000000..aa9e105
--- /dev/null
@@ -0,0 +1,3 @@
+
+PASS There should not be any updates made to the timeline after a forward-filling animation completes. 
+
diff --git a/LayoutTests/webanimations/no-scheduling-while-filling-non-accelerated.html b/LayoutTests/webanimations/no-scheduling-while-filling-non-accelerated.html
new file mode 100644 (file)
index 0000000..087b0ce
--- /dev/null
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+
+#target {
+    width: 100px;
+    height: 100px;
+    background-color: black;
+}
+
+</style>
+</head>
+<body>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+<div id="target"></div>
+<script>
+
+function waitNFrames(numberOfFrames, continuation)
+{
+    let elapsedFrames = 0;
+    (function rAFCallback() {
+        if (elapsedFrames++ >= numberOfFrames)
+            continuation();
+        else
+            requestAnimationFrame(rAFCallback);
+    })();
+}
+
+async_test(t => {
+    const keyframes = { marginLeft: ["50px", "100px"] };
+    const timing = { duration: 50, fill: "forwards" };
+    document.getElementById("target").animate(keyframes, timing).addEventListener("finish", event => {
+        const numberOfTimelineInvalidationsWhenFinished = internals.numberOfAnimationTimelineInvalidations();
+        waitNFrames(2, () => {
+            assert_equals(internals.numberOfAnimationTimelineInvalidations(), numberOfTimelineInvalidationsWhenFinished);
+            t.done();
+        });
+    });
+}, "There should not be any updates made to the timeline after a forward-filling animation completes.");
+
+</script>
+</body>
+</html>
index c3a184b999c96917eec219c02180dfe0fa5e4eaa..6ae3f87676af19036519ae621406d29ae4241fc2 100644 (file)
@@ -1,3 +1,63 @@
+2019-11-29  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Forward-filling animations should not schedule updates while filling
+        https://bugs.webkit.org/show_bug.cgi?id=204697
+
+        Reviewed by Dean Jackson.
+
+        Tests: webanimations/no-scheduling-while-filling-accelerated.html
+               webanimations/no-scheduling-while-filling-non-accelerated.html
+
+        For two different reasons, we would continuously schedule updates for animations in their "after" phase, ie. when they would have "fill: forwards"
+        and be finished.
+        
+        In the case of non-accelerated animations, that was because we would also schedule an immedate update in DocumentTimeline::scheduleNextTick() provided
+        we had relevant animations that weren't accelerated. But since animations in their "after" phase are still considered relevant, which means that they
+        would override the base style with an animated value, this caused the unnecessary scheduled updates.
+        
+        To address this, we run WebAnimation::timeToNextTick() in DocumentTimeline::scheduleNextTick() for all relevant animations and we teach that function
+        to schedule an immediate update only during the "active" phase, and to schedule a timed update only in the "before" phase computing the delay until
+        the animation enters the "active" phase.
+
+        While performing this work, we found a couple of other issues that weren't apparent until we had this more efficient scheduling in place.
+        
+        First, we would not allow any additional calls to DocumentTimeline::scheduleAnimationResolution() to actually schedule an update while we were in the
+        process of updating animations. While this wasn't a problem before because the mere presence of relevant animations would cause an upadte, we now had
+        to allow any animation changing timing properties as promises would resolve and events would be dispatched during the animation update to cause further
+        animation updates to be scheduled. So we moved the "m_animationResolutionScheduled" flag reset earlier in DocumentTimeline::updateAnimationsAndSendEvents()
+        before we actually run the animation update procedure.
+
+        Following that change, we also had to make sure that timing changes made through the evaluation of the pending play and pause tasks would _not_ cause
+        animations to be scheduled, which meant that an animation that became non-pending (ie. its first tick occured) would always schedule one immediate
+        animation update. So now we have an extra "silent" flag to WebAnimation::timingDidChange() which is only set to true when called from either
+        WebAnimation::runPendingPlayTask() or WebAnimation::runPendingPauseTask().
+
+        Finally, one existing test showed that calling KeyframeEffect::setKeyframes() while running animations didn't cause an animation update to be scheduled
+        since a call to WebAnimation::effectTimingDidChange() was lacking. This is now addressed.
+
+        As for accelerated animations, the extraneous animation scheduling occured because we would get in a state where we would record an "accelerated action"
+        to stop the accelerated animation but the accelerated animation had already completed and the renderer for the target element was no longer composited.
+        This meant that KeyframeEffect::applyPendingAcceleratedActions() would not perform any work and the pending accelerated action would remain in the
+        DocumentTimeline queue and cause an update to be scheduled to try again, endlessly.
+
+        To address that, we first check in KeyframeEffect::addPendingAcceleratedAction() if the recorded action differs from the last recorded action, avoiding
+        unnecessary work, and in KeyframeEffect::applyPendingAcceleratedActions(), if our last recorded action was "Stop" and the renderer was not composited,
+        we discard all pending accelerated actions.
+
+        * animation/DocumentTimeline.cpp:
+        (WebCore::DocumentTimeline::updateAnimationsAndSendEvents):
+        (WebCore::DocumentTimeline::scheduleNextTick):
+        * animation/KeyframeEffect.cpp:
+        (WebCore::KeyframeEffect::setKeyframes):
+        (WebCore::KeyframeEffect::addPendingAcceleratedAction):
+        (WebCore::KeyframeEffect::applyPendingAcceleratedActions):
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::timingDidChange):
+        (WebCore::WebAnimation::runPendingPlayTask):
+        (WebCore::WebAnimation::runPendingPauseTask):
+        (WebCore::WebAnimation::timeToNextTick const):
+        * animation/WebAnimation.h:
+
 2019-11-29  Antti Koivisto  <antti@apple.com>
 
         [LFC][IFC] Vector allocate LineBoxes and Display::Runs
index 20a07ee7c81c57af63d3f8ae15295cf2f5259680..630c2fa4a3e2c58357e1349b0ff04cb0aec69657 100644 (file)
@@ -350,11 +350,15 @@ void DocumentTimeline::updateAnimationsAndSendEvents(DOMHighResTimeStamp timesta
     if (m_isSuspended || !m_animationResolutionScheduled || !shouldRunUpdateAnimationsAndSendEventsIgnoringSuspensionState())
         return;
 
+    // Updating animations and sending events may invalidate the timing of some animations, so we must set the m_animationResolutionScheduled
+    // flag to false prior to running that procedure to allow animation with timing model updates to schedule updates.
+    m_animationResolutionScheduled = false;
+
     internalUpdateAnimationsAndSendEvents();
     applyPendingAcceleratedAnimations();
 
-    m_animationResolutionScheduled = false;
-    scheduleNextTick();
+    if (!m_animationResolutionScheduled)
+        scheduleNextTick();
 }
 
 void DocumentTimeline::internalUpdateAnimationsAndSendEvents()
@@ -529,13 +533,6 @@ void DocumentTimeline::scheduleNextTick()
     if (m_animations.isEmpty())
         return;
 
-    for (const auto& animation : m_animations) {
-        if (!animation->isRunningAccelerated()) {
-            scheduleAnimationResolution();
-            return;
-        }
-    }
-
     Seconds scheduleDelay = Seconds::infinity();
 
     for (const auto& animation : m_animations) {
index af30ddc2c162aefbe6f047646afbee33bf92ad1f..6f5ed4ad9c68939f9ba2f02d7fda3b65ac8c9d32 100644 (file)
@@ -663,7 +663,10 @@ Vector<Strong<JSObject>> KeyframeEffect::getKeyframes(JSGlobalObject& lexicalGlo
 
 ExceptionOr<void> KeyframeEffect::setKeyframes(JSGlobalObject& lexicalGlobalObject, Strong<JSObject>&& keyframesInput)
 {
-    return processKeyframes(lexicalGlobalObject, WTFMove(keyframesInput));
+    auto processKeyframesResult = processKeyframes(lexicalGlobalObject, WTFMove(keyframesInput));
+    if (!processKeyframesResult.hasException() && animation())
+        animation()->effectTimingDidChange();
+    return processKeyframesResult;
 }
 
 ExceptionOr<void> KeyframeEffect::processKeyframes(JSGlobalObject& lexicalGlobalObject, Strong<JSObject>&& keyframesInput)
@@ -1342,6 +1345,9 @@ void KeyframeEffect::updateAcceleratedAnimationState()
 
 void KeyframeEffect::addPendingAcceleratedAction(AcceleratedAction action)
 {
+    if (action == m_lastRecordedAcceleratedAction)
+        return;
+
     if (action == AcceleratedAction::Stop)
         m_pendingAcceleratedActions.clear();
     m_pendingAcceleratedActions.append(action);
@@ -1381,8 +1387,13 @@ void KeyframeEffect::applyPendingAcceleratedActions()
         return;
 
     auto* renderer = this->renderer();
-    if (!renderer || !renderer->isComposited())
+    if (!renderer || !renderer->isComposited()) {
+        // The renderer may no longer be composited because the accelerated animation ended before we had a chance to update it,
+        // in which case if we asked for the animation to stop, we can discard the current set of accelerated actions.
+        if (m_lastRecordedAcceleratedAction == AcceleratedAction::Stop)
+            m_pendingAcceleratedActions.clear();
         return;
+    }
 
     auto pendingAcceleratedActions = m_pendingAcceleratedActions;
     m_pendingAcceleratedActions.clear();
index 737da755c6222ec5eb6394bdc0eef9ce8c2c8d32..732fed75941e83ea6e57a7702759aa00a2ba603b 100644 (file)
@@ -717,7 +717,7 @@ ExceptionOr<void> WebAnimation::finish()
     return { };
 }
 
-void WebAnimation::timingDidChange(DidSeek didSeek, SynchronouslyNotify synchronouslyNotify)
+void WebAnimation::timingDidChange(DidSeek didSeek, SynchronouslyNotify synchronouslyNotify, Silently silently)
 {
     m_shouldSkipUpdatingFinishedStateWhenResolving = false;
     updateFinishedState(didSeek, synchronouslyNotify);
@@ -727,7 +727,7 @@ void WebAnimation::timingDidChange(DidSeek didSeek, SynchronouslyNotify synchron
         downcast<KeyframeEffect>(*m_effect).animationTimingDidChange();
     }
 
-    if (m_timeline)
+    if (silently == Silently::No && m_timeline)
         m_timeline->animationTimingDidChange(*this);
 };
 
@@ -979,7 +979,7 @@ void WebAnimation::runPendingPlayTask()
         m_readyPromise->resolve(*this);
 
     // 5. Run the procedure to update an animation's finished state for animation with the did seek flag set to false, and the synchronously notify flag set to false.
-    timingDidChange(DidSeek::No, SynchronouslyNotify::No);
+    timingDidChange(DidSeek::No, SynchronouslyNotify::No, Silently::Yes);
 
     invalidateEffect();
 }
@@ -1105,7 +1105,7 @@ void WebAnimation::runPendingPauseTask()
 
     // 6. Run the procedure to update an animation's finished state for animation with the did seek flag set to false, and the
     //    synchronously notify flag set to false.
-    timingDidChange(DidSeek::No, SynchronouslyNotify::No);
+    timingDidChange(DidSeek::No, SynchronouslyNotify::No, Silently::Yes);
 
     invalidateEffect();
 }
@@ -1289,40 +1289,39 @@ ExceptionOr<void> WebAnimation::commitStyles()
 
 Seconds WebAnimation::timeToNextTick() const
 {
-    ASSERT(isRunningAccelerated());
-
+    // Any animation that is pending needs immediate resolution.
     if (pending())
         return 0_s;
 
-    // If we're not running, there's no telling when we'll end.
-    if (playState() != PlayState::Running)
+    // If we're not running, or time is not advancing for this animation, there's no telling when we'll end.
+    auto playbackRate = effectivePlaybackRate();
+    if (playState() != PlayState::Running || !playbackRate)
         return Seconds::infinity();
 
-    // CSS Animations dispatch events for each iteration, so compute the time until
-    // the end of this iteration. Any other animation only cares about remaning total time.
-    if (isCSSAnimation()) {
-        auto* animationEffect = effect();
-        auto timing = animationEffect->getComputedTiming();
-        // If we're actively running, we need the time until the next iteration.
-        if (auto iterationProgress = timing.simpleIterationProgress)
-            return animationEffect->iterationDuration() * (1 - *iterationProgress);
-
-        // Otherwise we're probably in the before phase waiting to reach our start time.
-        if (auto animationCurrentTime = currentTime()) {
-            // If our current time is negative, we need to be scheduled to be resolved at the inverse
-            // of our current time, unless we fill backwards, in which case we want to invalidate as
-            // soon as possible.
-            auto localTime = animationCurrentTime.value();
-            if (localTime < 0_s)
-                return -localTime;
-            if (localTime < animationEffect->delay())
-                return animationEffect->delay() - localTime;
+    auto& effect = *this->effect();
+    auto timing = effect.getBasicTiming();
+    switch (timing.phase) {
+    case AnimationEffectPhase::Before:
+        // The animation is in its "before" phase, in this case we can wait until it enters its "active" phase.
+        return (effect.delay() - timing.localTime.value()) / playbackRate;
+    case AnimationEffectPhase::Active:
+        // Non-accelerated animations in the "active" phase will need to update their animated value at the immediate next opportunity.
+        if (!isRunningAccelerated())
+            return 0_s;
+        // Accelerated CSS Animations need to trigger "animationiteration" events, in this case we can wait until the next iteration.
+        if (isCSSAnimation()) {
+            if (auto iterationProgress = effect.getComputedTiming().simpleIterationProgress)
+                return effect.iterationDuration() * (1 - *iterationProgress);
         }
-    } else if (auto animationCurrentTime = currentTime())
-        return effect()->endTime() - *animationCurrentTime;
-
-    ASSERT_NOT_REACHED();
-    return Seconds::infinity();
+        // Accelerated animations in the "active" phase can wait until they ended.
+        return (effect.endTime() - timing.localTime.value()) / playbackRate;
+    case AnimationEffectPhase::After:
+        // The animation is in its after phase, which means it will no longer update its value, so it doens't need a tick.
+        return Seconds::infinity();
+    case AnimationEffectPhase::Idle:
+        ASSERT_NOT_REACHED();
+        return Seconds::infinity();
+    }
 }
 
 } // namespace WebCore
index 15fad6108834a1f389cbdec30b533ec2075e87dd..da4d8ec3bc2e570b48a51dbc14a344e6c49ba070 100644 (file)
@@ -154,7 +154,7 @@ private:
     enum class AutoRewind : uint8_t { Yes, No };
     enum class TimeToRunPendingTask : uint8_t { NotScheduled, ASAP, WhenReady };
 
-    void timingDidChange(DidSeek, SynchronouslyNotify);
+    void timingDidChange(DidSeek, SynchronouslyNotify, Silently = Silently::No);
     void updateFinishedState(DidSeek, SynchronouslyNotify);
     Seconds effectEndTime() const;
     WebAnimation& readyPromiseResolve();