Bug 234202

Summary: [frame-rate] use the animation frame rate during animation resolution and scheduling
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, dino, ews-watchlist, graouts, gyuyoung.kim, ryuan.choi, sergio, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=236774
https://bugs.webkit.org/show_bug.cgi?id=236780
Bug Depends on:    
Bug Blocks: 236778    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Web Inspector timeline for an animation running at 10fps
none
Web Inspector timeline for an animation running at 3fps
none
Patch
none
Patch simon.fraser: review+

Description Antoine Quint 2021-12-11 14:34:59 PST
Use the animation frame rate during animation resolution and scheduling
Comment 1 Antoine Quint 2021-12-11 14:49:44 PST
Created attachment 446902 [details]
Patch
Comment 2 Antoine Quint 2021-12-11 14:49:51 PST
<rdar://problem/85983678>
Comment 3 Antoine Quint 2021-12-12 12:49:03 PST
Created attachment 446941 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-12-12 13:05:57 PST
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?
Comment 5 Antoine Quint 2022-02-15 09:25:42 PST
Created attachment 452035 [details]
Patch
Comment 6 Antoine Quint 2022-02-15 09:32:00 PST
Created attachment 452037 [details]
Web Inspector timeline for an animation running at 10fps
Comment 7 Antoine Quint 2022-02-15 09:32:14 PST
Created attachment 452038 [details]
Web Inspector timeline for an animation running at 3fps
Comment 8 Antoine Quint 2022-02-15 10:24:11 PST
Created attachment 452046 [details]
Patch
Comment 9 Simon Fraser (smfr) 2022-02-15 21:08:54 PST
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?
Comment 10 Antoine Quint 2022-02-16 08:04:25 PST
(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.
Comment 11 Antoine Quint 2022-02-16 08:27:24 PST
(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.
Comment 12 Antoine Quint 2022-02-16 08:40:10 PST
Created attachment 452200 [details]
Patch
Comment 13 Simon Fraser (smfr) 2022-02-16 23:04:40 PST
(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)?
Comment 14 Simon Fraser (smfr) 2022-02-16 23:06:21 PST
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.
Comment 15 Antoine Quint 2022-02-17 00:14:27 PST
Committed r290003 (247389@trunk): <https://commits.webkit.org/247389@trunk>
Comment 16 Antoine Quint 2022-02-17 06:33:36 PST
(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.
Comment 17 Antoine Quint 2022-02-17 06:41:01 PST
(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.