Bug 223912

Summary: Allow non-60fps display updates to be driven by DisplayRefreshMonitor
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: WebCore Misc.Assignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, esprehn+autocc, ews-watchlist, graouts, kangil.han, sabouhallawa, sam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch sam: review+

Description Simon Fraser (smfr) 2021-03-29 22:08:33 PDT
Allow non-60fps display updates to be driven by DisplayRefreshMonitor
Comment 1 Simon Fraser (smfr) 2021-03-29 22:28:06 PDT
Created attachment 424616 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-03-30 08:35:25 PDT
<rdar://problem/76004517>
Comment 3 Sam Weinig 2021-03-30 09:38:16 PDT
Comment on attachment 424616 [details]
Patch

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

Love the tests!

> Source/WebCore/ChangeLog:21
> +        Also add preferredFramesPerSecond() to use here, and have both it and

I'm unclear what "here" is in this context.

> Source/WebCore/page/Page.cpp:1201
>      renderingUpdateScheduler().windowScreenDidChange(displayID);
> +    renderingUpdateScheduler().adjustRenderingUpdateFrequency();

I would either pull renderingUpdateScheduler into a local (since the getter does work) or just have windowScreenDidChange() call adjustRenderingUpdateFrequency(),

> Source/WebCore/page/Page.h:429
> +    // This can return nullopt if throttling reasons result in a frequency less than one, in which case
> +    // preferredRenderingUpdateInterval provides the frequency.
> +    Optional<FramesPerSecond> preferredRenderingUpdateFramesPerSecond() const;

This seems a little silly that two calls are needed. In the future, we should we consider making FramesPerSecond be a struct containing a ratio that can represent frequencies less than one, or make this return a Variant<FramesPerSecond, Seconds>.

> Source/WebCore/platform/Logging.cpp:95
> +    LogRequestAnimationFrame.state = WTFLogChannelState::On;
> +    LogDisplayLink.state = WTFLogChannelState::On;

Did you mean to leave these in?

> Source/WebCore/platform/graphics/AnimationFrameRate.cpp:33
> +static constexpr OptionSet<ThrottlingReason> halfSpeedThrottlingReasons = { ThrottlingReason::LowPowerMode, ThrottlingReason::NonInteractedCrossOriginFrame, ThrottlingReason::VisuallyIdle };

I don't think the `=` is needed here.
Comment 4 Simon Fraser (smfr) 2021-03-30 11:15:20 PDT
https://trac.webkit.org/changeset/275215/webkit