Bug 223912 - Allow non-60fps display updates to be driven by DisplayRefreshMonitor
Summary: Allow non-60fps display updates to be driven by DisplayRefreshMonitor
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
Keywords: InRadar
Depends on:
Reported: 2021-03-29 22:08 PDT by Simon Fraser (smfr)
Modified: 2021-03-30 11:15 PDT (History)
9 users (show)

See Also:

Patch (53.84 KB, patch)
2021-03-29 22:28 PDT, Simon Fraser (smfr)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Radar WebKit Bug Importer 2021-03-30 08:35:25 PDT
Comment 3 Sam Weinig 2021-03-30 09:38:16 PDT
Comment on attachment 424616 [details]

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