Changeset 252957 in webkit


Ignore:
Timestamp:
Nov 30, 2019 5:15:48 AM (4 years ago)
Author:
graouts@webkit.org
Message:

[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.

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.
Location:
trunk
Files:
4 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r252951 r252957  
     12019-11-30  Antoine Quint  <graouts@apple.com>
     2
     3        [Web Animations] Forward-filling animations should not schedule updates while filling
     4        https://bugs.webkit.org/show_bug.cgi?id=204697
     5        <rdar://problem/57534005>
     6
     7        Reviewed by Dean Jackson.
     8
     9        Adding tests checking that we don't schedule animation updates while filling for accelerated and non-accelerated animations alike.
     10
     11        * webanimations/no-scheduling-while-filling-accelerated-expected.txt: Added.
     12        * webanimations/no-scheduling-while-filling-accelerated.html: Added.
     13        * webanimations/no-scheduling-while-filling-non-accelerated-expected.txt: Added.
     14        * webanimations/no-scheduling-while-filling-non-accelerated.html: Added.
     15
    1162019-11-29  Commit Queue  <commit-queue@webkit.org>
    217

  • trunk/Source/WebCore/ChangeLog

    r252956 r252957  
     12019-11-30  Antoine Quint  <graouts@apple.com>
     2
     3        [Web Animations] Forward-filling animations should not schedule updates while filling
     4        https://bugs.webkit.org/show_bug.cgi?id=204697
     5        <rdar://problem/57534005>
     6
     7        Reviewed by Dean Jackson.
     8
     9        Tests: webanimations/no-scheduling-while-filling-accelerated.html
     10               webanimations/no-scheduling-while-filling-non-accelerated.html
     11
     12        For two different reasons, we would continuously schedule updates for animations in their "after" phase, ie. when they would have "fill: forwards"
     13        and be finished.
     14
     15        In the case of non-accelerated animations, that was because we would also schedule an immedate update in DocumentTimeline::scheduleNextTick() provided
     16        we had relevant animations that weren't accelerated. But since animations in their "after" phase are still considered relevant, which means that they
     17        would override the base style with an animated value, this caused the unnecessary scheduled updates.
     18
     19        To address this, we run WebAnimation::timeToNextTick() in DocumentTimeline::scheduleNextTick() for all relevant animations and we teach that function
     20        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
     21        the animation enters the "active" phase.
     22
     23        While performing this work, we found a couple of other issues that weren't apparent until we had this more efficient scheduling in place.
     24
     25        First, we would not allow any additional calls to DocumentTimeline::scheduleAnimationResolution() to actually schedule an update while we were in the
     26        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
     27        to allow any animation changing timing properties as promises would resolve and events would be dispatched during the animation update to cause further
     28        animation updates to be scheduled. So we moved the "m_animationResolutionScheduled" flag reset earlier in DocumentTimeline::updateAnimationsAndSendEvents()
     29        before we actually run the animation update procedure.
     30
     31        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
     32        animations to be scheduled, which meant that an animation that became non-pending (ie. its first tick occured) would always schedule one immediate
     33        animation update. So now we have an extra "silent" flag to WebAnimation::timingDidChange() which is only set to true when called from either
     34        WebAnimation::runPendingPlayTask() or WebAnimation::runPendingPauseTask().
     35
     36        Finally, one existing test showed that calling KeyframeEffect::setKeyframes() while running animations didn't cause an animation update to be scheduled
     37        since a call to WebAnimation::effectTimingDidChange() was lacking. This is now addressed.
     38
     39        As for accelerated animations, the extraneous animation scheduling occured because we would get in a state where we would record an "accelerated action"
     40        to stop the accelerated animation but the accelerated animation had already completed and the renderer for the target element was no longer composited.
     41        This meant that KeyframeEffect::applyPendingAcceleratedActions() would not perform any work and the pending accelerated action would remain in the
     42        DocumentTimeline queue and cause an update to be scheduled to try again, endlessly.
     43
     44        To address that, we first check in KeyframeEffect::addPendingAcceleratedAction() if the recorded action differs from the last recorded action, avoiding
     45        unnecessary work, and in KeyframeEffect::applyPendingAcceleratedActions(), if our last recorded action was "Stop" and the renderer was not composited,
     46        we discard all pending accelerated actions.
     47
     48        * animation/DocumentTimeline.cpp:
     49        (WebCore::DocumentTimeline::updateAnimationsAndSendEvents):
     50        (WebCore::DocumentTimeline::scheduleNextTick):
     51        * animation/KeyframeEffect.cpp:
     52        (WebCore::KeyframeEffect::setKeyframes):
     53        (WebCore::KeyframeEffect::addPendingAcceleratedAction):
     54        (WebCore::KeyframeEffect::applyPendingAcceleratedActions):
     55        * animation/WebAnimation.cpp:
     56        (WebCore::WebAnimation::timingDidChange):
     57        (WebCore::WebAnimation::runPendingPlayTask):
     58        (WebCore::WebAnimation::runPendingPauseTask):
     59        (WebCore::WebAnimation::timeToNextTick const):
     60        * animation/WebAnimation.h:
     61
    1622019-11-30  youenn fablet  <youenn@apple.com>
    263

  • trunk/Source/WebCore/animation/DocumentTimeline.cpp

    r252951 r252957  
    351351        return;
    352352
     353    // Updating animations and sending events may invalidate the timing of some animations, so we must set the m_animationResolutionScheduled
     354    // flag to false prior to running that procedure to allow animation with timing model updates to schedule updates.
     355    m_animationResolutionScheduled = false;
     356
    353357    internalUpdateAnimationsAndSendEvents();
    354358    applyPendingAcceleratedAnimations();
    355359
    356     m_animationResolutionScheduled = false;
    357     scheduleNextTick();
     360    if (!m_animationResolutionScheduled)
     361        scheduleNextTick();
    358362}
    359363
    … …  
    530534        return;
    531535
    532     for (const auto& animation : m_animations) {
    533         if (!animation->isRunningAccelerated()) {
    534             scheduleAnimationResolution();
    535             return;
    536         }
    537     }
    538 
    539536    Seconds scheduleDelay = Seconds::infinity();
    540537

  • trunk/Source/WebCore/animation/KeyframeEffect.cpp

    r252951 r252957  
    664664ExceptionOr<void> KeyframeEffect::setKeyframes(JSGlobalObject& lexicalGlobalObject, Strong<JSObject>&& keyframesInput)
    665665{
    666     return processKeyframes(lexicalGlobalObject, WTFMove(keyframesInput));
     666    auto processKeyframesResult = processKeyframes(lexicalGlobalObject, WTFMove(keyframesInput));
     667    if (!processKeyframesResult.hasException() && animation())
     668        animation()->effectTimingDidChange();
     669    return processKeyframesResult;
    667670}
    668671
    … …  
    13431346void KeyframeEffect::addPendingAcceleratedAction(AcceleratedAction action)
    13441347{
     1348    if (action == m_lastRecordedAcceleratedAction)
     1349        return;
     1350
    13451351    if (action == AcceleratedAction::Stop)
    13461352        m_pendingAcceleratedActions.clear();
    … …  
    13821388
    13831389    auto* renderer = this->renderer();
    1384     if (!renderer || !renderer->isComposited())
    1385         return;
     1390    if (!renderer || !renderer->isComposited()) {
     1391        // The renderer may no longer be composited because the accelerated animation ended before we had a chance to update it,
     1392        // in which case if we asked for the animation to stop, we can discard the current set of accelerated actions.
     1393        if (m_lastRecordedAcceleratedAction == AcceleratedAction::Stop)
     1394            m_pendingAcceleratedActions.clear();
     1395        return;
     1396    }
    13861397
    13871398    auto pendingAcceleratedActions = m_pendingAcceleratedActions;

  • trunk/Source/WebCore/animation/WebAnimation.cpp

    r252951 r252957  
    718718}
    719719
    720 void WebAnimation::timingDidChange(DidSeek didSeek, SynchronouslyNotify synchronouslyNotify)
     720void WebAnimation::timingDidChange(DidSeek didSeek, SynchronouslyNotify synchronouslyNotify, Silently silently)
    721721{
    722722    m_shouldSkipUpdatingFinishedStateWhenResolving = false;
    … …  
    728728    }
    729729
    730     if (m_timeline)
     730    if (silently == Silently::No && m_timeline)
    731731        m_timeline->animationTimingDidChange(*this);
    732732};
    … …  
    980980
    981981    // 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.
    982     timingDidChange(DidSeek::No, SynchronouslyNotify::No);
     982    timingDidChange(DidSeek::No, SynchronouslyNotify::No, Silently::Yes);
    983983
    984984    invalidateEffect();
    … …  
    11061106    // 6. Run the procedure to update an animation's finished state for animation with the did seek flag set to false, and the
    11071107    //    synchronously notify flag set to false.
    1108     timingDidChange(DidSeek::No, SynchronouslyNotify::No);
     1108    timingDidChange(DidSeek::No, SynchronouslyNotify::No, Silently::Yes);
    11091109
    11101110    invalidateEffect();
    … …  
    12901290Seconds WebAnimation::timeToNextTick() const
    12911291{
    1292     ASSERT(isRunningAccelerated());
    1293 
     1292    // Any animation that is pending needs immediate resolution.
    12941293    if (pending())
    12951294        return 0_s;
    12961295
    1297     // If we're not running, there's no telling when we'll end.
    1298     if (playState() != PlayState::Running)
     1296    // If we're not running, or time is not advancing for this animation, there's no telling when we'll end.
     1297    auto playbackRate = effectivePlaybackRate();
     1298    if (playState() != PlayState::Running || !playbackRate)
    12991299        return Seconds::infinity();
    13001300
    1301     // CSS Animations dispatch events for each iteration, so compute the time until
    1302     // the end of this iteration. Any other animation only cares about remaning total time.
    1303     if (isCSSAnimation()) {
    1304         auto* animationEffect = effect();
    1305         auto timing = animationEffect->getComputedTiming();
    1306         // If we're actively running, we need the time until the next iteration.
    1307         if (auto iterationProgress = timing.simpleIterationProgress)
    1308             return animationEffect->iterationDuration() * (1 - *iterationProgress);
    1309 
    1310         // Otherwise we're probably in the before phase waiting to reach our start time.
    1311         if (auto animationCurrentTime = currentTime()) {
    1312             // If our current time is negative, we need to be scheduled to be resolved at the inverse
    1313             // of our current time, unless we fill backwards, in which case we want to invalidate as
    1314             // soon as possible.
    1315             auto localTime = animationCurrentTime.value();
    1316             if (localTime < 0_s)
    1317                 return -localTime;
    1318             if (localTime < animationEffect->delay())
    1319                 return animationEffect->delay() - localTime;
     1301    auto& effect = *this->effect();
     1302    auto timing = effect.getBasicTiming();
     1303    switch (timing.phase) {
     1304    case AnimationEffectPhase::Before:
     1305        // The animation is in its "before" phase, in this case we can wait until it enters its "active" phase.
     1306        return (effect.delay() - timing.localTime.value()) / playbackRate;
     1307    case AnimationEffectPhase::Active:
     1308        // Non-accelerated animations in the "active" phase will need to update their animated value at the immediate next opportunity.
     1309        if (!isRunningAccelerated())
     1310            return 0_s;
     1311        // Accelerated CSS Animations need to trigger "animationiteration" events, in this case we can wait until the next iteration.
     1312        if (isCSSAnimation()) {
     1313            if (auto iterationProgress = effect.getComputedTiming().simpleIterationProgress)
     1314                return effect.iterationDuration() * (1 - *iterationProgress);
    13201315        }
    1321     } else if (auto animationCurrentTime = currentTime())
    1322         return effect()->endTime() - *animationCurrentTime;
     1316        // Accelerated animations in the "active" phase can wait until they ended.
     1317        return (effect.endTime() - timing.localTime.value()) / playbackRate;
     1318    case AnimationEffectPhase::After:
     1319        // The animation is in its after phase, which means it will no longer update its value, so it doens't need a tick.
     1320        return Seconds::infinity();
     1321    case AnimationEffectPhase::Idle:
     1322        ASSERT_NOT_REACHED();
     1323        return Seconds::infinity();
     1324    }
    13231325
    13241326    ASSERT_NOT_REACHED();

  • trunk/Source/WebCore/animation/WebAnimation.h

    r252951 r252957  
    155155    enum class TimeToRunPendingTask : uint8_t { NotScheduled, ASAP, WhenReady };
    156156
    157     void timingDidChange(DidSeek, SynchronouslyNotify);
     157    void timingDidChange(DidSeek, SynchronouslyNotify, Silently = Silently::No);
    158158    void updateFinishedState(DidSeek, SynchronouslyNotify);
    159159    Seconds effectEndTime() const;
Note: See TracChangeset for help on using the changeset viewer.