[Web Animations] Forward-filling animations should not schedule updates while filling
authorgraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Nov 2019 13:15:48 +0000 (13:15 +0000)
committergraouts@webkit.org <graouts@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Nov 2019 13:15:48 +0000 (13:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204697
<rdar://problem/57534005>

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@252957 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 ea7ca87..0c8dc80 100644 (file)
@@ -1,3 +1,18 @@
+2019-11-30  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
+        <rdar://problem/57534005>
+
+        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-29  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r252944.
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 78eecab..436c461 100644 (file)
@@ -1,3 +1,64 @@
+2019-11-30  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
+        <rdar://problem/57534005>
+
+        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-30  youenn fablet  <youenn@apple.com>
 
         MockAudioSharedUnit should reset its last render time on start/stop/reconfigure
index 20a07ee..630c2fa 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 af30ddc..6f5ed4a 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 737da75..702926c 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,37 +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;
+        // 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();
+    }
 
     ASSERT_NOT_REACHED();
     return Seconds::infinity();
index 15fad61..da4d8ec 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();