Bug 234202 – [frame-rate] use the animation frame rate during animation resolution and scheduling

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Antoine Quint]

Use the animation frame rate during animation resolution and scheduling

[Antoine Quint]

Created attachment 446902 [details]
Patch

[Antoine Quint]

<rdar://problem/85983678>

[Antoine Quint]

Created attachment 446941 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 446941 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446941&action=review

> Source/WebCore/animation/DocumentTimeline.cpp:358
> +            auto timeToNextPossibleTickAccountingForFrameRate = timeUntilNextTickForAnimationsWithFrameRate(animationFrameRate);
> +            if (timeToNextPossibleTickAccountingForFrameRate && animationTimeToNextRequiredTick < *timeToNextPossibleTickAccountingForFrameRate)
> +                animationTimeToNextRequiredTick = *timeToNextPossibleTickAccountingForFrameRate - nextTickTimeEpsilon;

I feel like the long names make this hard to read.

> Source/WebCore/animation/DocumentTimelinesController.cpp:179
> +                // Now, we want to update all animations with a given frame rate as a group in the same
> +                // update. To do this, we keep track of when we've last performed a tick for animations
> +                // with a given frame rate. If we don't yet have an entry for this frame rate, we record
> +                // the current time in it since we'll be updating them in this update.
> +
> +                // FIXME: while updating all animations with a given frame rate in a group is good,
> +                // we currently do nothing to ensure, for instance, that animations running at 2fps
> +                // and animations running at 4fps, share at least one update. This would be a good
> +                // enhancement to guarantee the fewest amounts of updates.
> +                auto it = m_animationFrameRateToLastTickTimeMap.find(*animationFrameRate);
> +                if (it == m_animationFrameRateToLastTickTimeMap.end())
> +                    m_animationFrameRateToLastTickTimeMap.set(*animationFrameRate, timestamp);
> +                else {
> +                    // If we have updated animations with this frame rate at a previous time, we must
> +                    // ensure that that the necessary interval since that time has elapsed for us to
> +                    // update animations with this frame rate. If that interval has elapsed, we record
> +                    // the current time as the last time animations for this frame rate were updated.
> +                    // And to ensure that other animations with this frame rate that we haven't processed
> +                    // in this update are updated, we not only check that the interval has elapsed, but
> +                    // also that it's not the same as the recorded value since it may have been recorded
> +                    // in this very update for a previous animation with this frame rate.
> +
> +                    // FIXME: the 1ms tolerance to check that the interval has elapsed may not be sufficient,
> +                    // we may consider to adjust this tolerance based on the interval between updates to this
> +                    // timeline so that we don't end up further away from the target update cadence.
> +                    auto lastTickTime = it->value;
> +                    auto interval = Seconds(1.0 / *animationFrameRate);
> +                    auto nextTickTime = lastTickTime + interval - nextTickTimeEpsilon;
> +                    if (lastTickTime != timestamp) {
> +                        // We should only skip animations that are not matching the timeline frame rate,
> +                        // otherwise we risk not matching the display refresh cadence.
> +                        if (animationFrameRate != previousTimelineFrameRate && nextTickTime > timestamp)
> +                            continue;
> +                        it->value = timestamp;
> +                    }
> +                }
> +            }
> +

I feel like using DisplayUpdate might really simplify this code.

> Source/WebCore/animation/DocumentTimelinesController.cpp:266
> +    auto it = m_animationFrameRateToLastTickTimeMap.find(frameRate);
> +    if (it == m_animationFrameRateToLastTickTimeMap.end())

I think DisplayUpdate would avoid you having to store this?

[Antoine Quint]

Created attachment 452035 [details]
Patch

[Antoine Quint]

Created attachment 452037 [details]
Web Inspector timeline for an animation running at 10fps

[Antoine Quint]

Created attachment 452038 [details]
Web Inspector timeline for an animation running at 3fps

[Antoine Quint]

Created attachment 452046 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 452046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452046&action=review

Does this have the correct behavior when both rAF and animations are running? Does it cause rAF callback frequency to change based on active animations? Does rAF callback frequency leak across iframes?

> Source/WebCore/animation/DocumentTimeline.cpp:107
> +    auto frameRate = m_document->page()->preferredRenderingUpdateFramesPerSecond();
> +    if (frameRate && *frameRate)
> +        return Seconds(1.0 / *frameRate);

Is this the same as Page::preferredRenderingUpdateInterval()?

> Source/WebCore/animation/DocumentTimelinesController.cpp:90
> +        defaultTimelineFrameRate = page->preferredRenderingUpdateFramesPerSecond({ Page::PreferredRenderingUpdateOption::IncludeThrottlingReasons });
> +        previousTimelineFrameRate = page->preferredRenderingUpdateFramesPerSecond();

It would be clearer to pass `{ PreferredRenderingUpdateOption::IncludeThrottlingReasons, PreferredRenderingUpdateOption::IncludeAnimationsFrameRate }` for the second line.

> Source/WebCore/animation/FrameRateAligner.h:55
> +    HashMap<FramesPerSecond, FrameRateData> m_frameRates;

So you end up with an entry here for each animation with a unique frame rate, independent of start time?

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 452046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452046&action=review
> 
> Does this have the correct behavior when both rAF and animations are
> running? Does it cause rAF callback frequency to change based on active
> animations? Does rAF callback frequency leak across iframes?

The rAF callback frequency is completely different from this. This really only applies to scheduling calls to scheduleRenderingUpdate(RenderingUpdateStep::Animations). There is no change to rAF callback behavior.

> > Source/WebCore/animation/DocumentTimelinesController.cpp:90
> > +        defaultTimelineFrameRate = page->preferredRenderingUpdateFramesPerSecond({ Page::PreferredRenderingUpdateOption::IncludeThrottlingReasons });
> > +        previousTimelineFrameRate = page->preferredRenderingUpdateFramesPerSecond();
> 
> It would be clearer to pass `{
> PreferredRenderingUpdateOption::IncludeThrottlingReasons,
> PreferredRenderingUpdateOption::IncludeAnimationsFrameRate }` for the second
> line.

OK, will change in next patch.

> > Source/WebCore/animation/FrameRateAligner.h:55
> > +    HashMap<FramesPerSecond, FrameRateData> m_frameRates;
> 
> So you end up with an entry here for each animation with a unique frame
> rate, independent of start time?

Yes. This stores one entry per unique frame rate used by any animation. If you have 10 animations asking for 120fps, 5 for 60fps and 1 for 30fps, there will be three entries: 120, 60 and 30. The time at which the animations start does not matter since the whole point is for them to align to update in the same frames.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 452046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452046&action=review
> 
> > Source/WebCore/animation/DocumentTimeline.cpp:107
> > +    auto frameRate = m_document->page()->preferredRenderingUpdateFramesPerSecond();
> > +    if (frameRate && *frameRate)
> > +        return Seconds(1.0 / *frameRate);
> 
> Is this the same as Page::preferredRenderingUpdateInterval()?

I think so. Will remove in next patch.

[Antoine Quint]

Created attachment 452200 [details]
Patch

[Simon Fraser (smfr)]

(In reply to Antoine Quint from comment #10)
> (In reply to Simon Fraser (smfr) from comment #9)
> > Comment on attachment 452046 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=452046&action=review
> > 
> > Does this have the correct behavior when both rAF and animations are
> > running? Does it cause rAF callback frequency to change based on active
> > animations? Does rAF callback frequency leak across iframes?
> 
> The rAF callback frequency is completely different from this. This really
> only applies to scheduling calls to
> scheduleRenderingUpdate(RenderingUpdateStep::Animations). There is no change
> to rAF callback behavior.

Does this mean that this patch is just about scheduling animations when lower than the default frame rate (i.e. 60fps)?

[Simon Fraser (smfr)]

Comment on attachment 452200 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452200&action=review

> Source/WebCore/ChangeLog:25
> +        There is no test for this code currently. It is not possible to test for the actual
> +        rate at which animations are updated in a testing environment, since there are too

I think it should be possible to test things like the fact that all 15fps animations get serviced in the same updateRendering, even when started at different times.

[Antoine Quint]

Committed r290003 (247389@trunk): <https://commits.webkit.org/247389@trunk>

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #14)
> Comment on attachment 452200 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452200&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        There is no test for this code currently. It is not possible to test for the actual
> > +        rate at which animations are updated in a testing environment, since there are too
> 
> I think it should be possible to test things like the fact that all 15fps
> animations get serviced in the same updateRendering, even when started at
> different times.

I've added some tests in the next frameRate patch up for review in bug 236778.

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #13)
> (In reply to Antoine Quint from comment #10)
> > (In reply to Simon Fraser (smfr) from comment #9)
> > > Comment on attachment 452046 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=452046&action=review
> > > 
> > > Does this have the correct behavior when both rAF and animations are
> > > running? Does it cause rAF callback frequency to change based on active
> > > animations? Does rAF callback frequency leak across iframes?
> > 
> > The rAF callback frequency is completely different from this. This really
> > only applies to scheduling calls to
> > scheduleRenderingUpdate(RenderingUpdateStep::Animations). There is no change
> > to rAF callback behavior.
> 
> Does this mean that this patch is just about scheduling animations when
> lower than the default frame rate (i.e. 60fps)?

I had misunderstood the situation. This patch does have an impact on when requestAnimationFrame() callbacks are triggered! If you have an animation with frameRate set to 120, then requestAnimationFrame() callbacks will also be called as the animation is sampled at 120fps.

I've filed bug 236780 to discuss that and possibly address it.