Currently if you plug a 144Hz display into a Mac Mini, WebKit will update the page and fire rAF callbacks at 144Hz. However, we should ensure that rAF, non-accelerated animations and page rendering in general refreshes at closest interval to the one used for 60Hz, so that we would run at 72Hz on a 144Hz display.
<rdar://problem/72398605>
Created attachment 419844 [details] Patch
Comment on attachment 419844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419844&action=review > Source/WebCore/platform/graphics/AnimationFrameRate.h:59 > + if (nominalFramesPerSecond < FullSpeedFramesPerSecond) I think we should make an early return if the nominalFramesPerSecond is 60fps, i.e if (nominalFramesPerSecond <= FullSpeedFramesPerSecond) > Source/WebCore/platform/graphics/AnimationFrameRate.h:71 > + auto fullSpeedInterval = 1.0 / FullSpeedFramesPerSecond; > + auto fullSpeedRatio = nominalFramesPerSecond / FullSpeedFramesPerSecond; > + > + auto floorInterval = 1.0 / (nominalFramesPerSecond / std::floor(fullSpeedRatio)); // => 1 / 72 > + auto ceilInterval = 1.0 / (nominalFramesPerSecond / std::ceil(fullSpeedRatio)); // => 1 / 48 > + > + auto intervalToFloor = fullSpeedInterval - floorInterval; > + auto intervalToCeil = ceilInterval - fullSpeedInterval; > + > + return framesPerSecondForInterval((intervalToFloor < intervalToCeil) ? floorInterval : ceilInterval); I think this calculation can be simplified if we use integer calculation. I think something like this might work: float fullSpeedRatio = nominalFramesPerSecond / FullSpeedFramesPerSecond; // 2.4 FramesPerSecond floorSpeed = nominalFramesPerSecond / std::floor(fullSpeedRatio); // 144 / 2 = 72 FramesPerSecond ceilSpeed = nominalFramesPerSecond / std::ceil(fullSpeedRatio); // 144 / 3 = 48 return fullSpeedRatio - std::floor(fullSpeedRatio) <= 0.5 ? floorSpeed : ceilSpeed; // 72
(In reply to Said Abou-Hallawa from comment #3) > Comment on attachment 419844 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419844&action=review > > > Source/WebCore/platform/graphics/AnimationFrameRate.h:59 > > + if (nominalFramesPerSecond < FullSpeedFramesPerSecond) > > I think we should make an early return if the nominalFramesPerSecond is > 60fps, i.e > > if (nominalFramesPerSecond <= FullSpeedFramesPerSecond) Absolutely, no need to run the maths if we already know the answer. > > Source/WebCore/platform/graphics/AnimationFrameRate.h:71 > > + auto fullSpeedInterval = 1.0 / FullSpeedFramesPerSecond; > > + auto fullSpeedRatio = nominalFramesPerSecond / FullSpeedFramesPerSecond; > > + > > + auto floorInterval = 1.0 / (nominalFramesPerSecond / std::floor(fullSpeedRatio)); // => 1 / 72 > > + auto ceilInterval = 1.0 / (nominalFramesPerSecond / std::ceil(fullSpeedRatio)); // => 1 / 48 Whoops, those comments shouldn't have remained there. > > + > > + auto intervalToFloor = fullSpeedInterval - floorInterval; > > + auto intervalToCeil = ceilInterval - fullSpeedInterval; > > + > > + return framesPerSecondForInterval((intervalToFloor < intervalToCeil) ? floorInterval : ceilInterval); > > I think this calculation can be simplified if we use integer calculation. I > think something like this might work: > > float fullSpeedRatio = nominalFramesPerSecond / > FullSpeedFramesPerSecond; // 2.4 > FramesPerSecond floorSpeed = nominalFramesPerSecond / > std::floor(fullSpeedRatio); // 144 / 2 = 72 > FramesPerSecond ceilSpeed = nominalFramesPerSecond / > std::ceil(fullSpeedRatio); // 144 / 3 = 48 > > return fullSpeedRatio - std::floor(fullSpeedRatio) <= 0.5 ? floorSpeed : > ceilSpeed; // 72 I'll check that works correctly and use that.
Comment on attachment 419844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419844&action=review My reading of the specification says that the browser is free to pick a refresh rate based on the hardware refresh, so this patch is ok. However, I think we need to propose changes to the specification to be clear why we are unilaterally picking a value closer to 60 and not even attempting to run at the native frame rate. > Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:781 > +VariablePageUpdateRefreshRateEnabled: > + type: bool > + humanReadableName: "Variable Page Update Refresh Rate" > + humanReadableDescription: "Enable a refresh rate that varies depending on the display refresh rate for software Web Animations, requestAnimationFrame() and anything triggering the page rendering steps" It's not really a variable rate - it's fixed to the monitor refresh. And, from reading the spec, it's what we should have been doing already. So maybe the flag needs to be the other way around: ForcePageUpdatesTo60Hz? > Source/WebCore/platform/graphics/AnimationFrameRate.h:66 > + auto floorInterval = 1.0 / (nominalFramesPerSecond / std::floor(fullSpeedRatio)); // => 1 / 72 > + auto ceilInterval = 1.0 / (nominalFramesPerSecond / std::ceil(fullSpeedRatio)); // => 1 / 48 where does the 48 come from? A 90Hz display would want 45Hz, not 48.
(In reply to Dean Jackson from comment #5) > Comment on attachment 419844 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419844&action=review > > My reading of the specification says that the browser is free to pick a > refresh rate based on the hardware refresh, so this patch is ok. However, I > think we need to propose changes to the specification to be clear why we are > unilaterally picking a value closer to 60 and not even attempting to run at > the native frame rate. I'll file an issue on this, watch this space. > > Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:781 > > +VariablePageUpdateRefreshRateEnabled: > > + type: bool > > + humanReadableName: "Variable Page Update Refresh Rate" > > + humanReadableDescription: "Enable a refresh rate that varies depending on the display refresh rate for software Web Animations, requestAnimationFrame() and anything triggering the page rendering steps" > > It's not really a variable rate - it's fixed to the monitor refresh. I'll fix the wording. > And, from reading the spec, it's what we should have been doing already. So > maybe the flag needs to be the other way around: ForcePageUpdatesTo60Hz? Good idea! I'll flip it around and make it off by default. > > Source/WebCore/platform/graphics/AnimationFrameRate.h:66 > > + auto floorInterval = 1.0 / (nominalFramesPerSecond / std::floor(fullSpeedRatio)); // => 1 / 72 > > + auto ceilInterval = 1.0 / (nominalFramesPerSecond / std::ceil(fullSpeedRatio)); // => 1 / 48 > > where does the 48 come from? This was stray comments from playing around with 144Hz. Should not have been in the patch. > A 90Hz display would want 45Hz, not 48. Actually, a 90Hz display could get either 90fps or 45fps since 60fps falls right in the middle. In fact, 90fps would be the fastest we would ever run.
Created attachment 419873 [details] Patch
Comment on attachment 419873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419873&action=review > Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:274 > + humanReadableDescription: "Force the rate for updates to software Web Animations, requestAnimationFrame() and anything triggering the page rendering steps to 60 frames per second rather than use the display's refresh rate" Keep in mind this goes in a tooltip, so this is a lot of words
Created attachment 420120 [details] Patch
(In reply to Tim Horton from comment #8) > Comment on attachment 419873 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419873&action=review > > > Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:274 > > + humanReadableDescription: "Force the rate for updates to software Web Animations, requestAnimationFrame() and anything triggering the page rendering steps to 60 frames per second rather than use the display's refresh rate" > > Keep in mind this goes in a tooltip, so this is a lot of words I've made it a bit snappier in the latest patch.
Comment on attachment 420120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420120&action=review > Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:273 > +ForcePageRenderingUpdatesAt60FPSEnabled: > + type: bool > + humanReadableName: "Force Page Rendering Updates at 60fps" I think the "at60" is deceptive. Maybe call it "WebCompatibleRenderingUpdateFrequencyEnabled" or soothing a bit more vague like that. > Source/WebCore/page/RenderingUpdateScheduler.cpp:143 > + manager.windowScreenDidChange(displayID, *this); > + manager.setPreferredFramesPerSecond(*this, m_preferredFramesPerSecond); It would be nicer if windowScreenDidChange() took care of updating preferred FPS. Did you test this? Do we have the new FPS at the right time here? > Source/WebCore/platform/graphics/AnimationFrameRate.h:61 > + return fullSpeedRatio - std::floor(fullSpeedRatio) <= 0.5 ? floorSpeed : ceilSpeed; If you're half way between, does this prefer the lower or higher? Can we API-test this code? > Source/WebCore/platform/graphics/AnimationFrameRate.h:70 > + // FIXME: handle ThrottlingReason::VisuallyIdle Do we have to address this FIXME now? If we don't, won't that cause massive power regressions? > Source/WebCore/platform/graphics/AnimationFrameRate.h:81 > + interval *= 2; The '2' should be in a named constant. > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:822 > +void WebProcessPool::setDisplayLinkPreferredFramesPerSecond(PlatformDisplayID displayID, WebCore::FramesPerSecond preferredFramesPerSecond) > +{ > + for (auto& displayLink : m_displayLinks) { > + if (displayLink->displayID() == displayID) { > + displayLink->setPreferredFramesPerSecond(preferredFramesPerSecond); > + return; > + } > + } > +} If you have two windows on the same screen, and one is obscured (or throttled for some reason), will they both call this with different preferredFramesPerSecond and therefore clobber eachother's value?
Comment on attachment 420120 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420120&action=review > Source/WebKit/UIProcess/mac/DisplayLink.cpp:79 > +void DisplayLink::setPreferredFramesPerSecond(WebCore::FramesPerSecond preferredFramesPerSecond) > +{ > + m_preferredFramesPerSecond = preferredFramesPerSecond; > +} There is one DisplayLink per display, so you can't have clients push preferredFramesPerSecond here.
(In reply to Simon Fraser (smfr) from comment #11) > Comment on attachment 420120 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=420120&action=review > > > Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:273 > > +ForcePageRenderingUpdatesAt60FPSEnabled: > > + type: bool > > + humanReadableName: "Force Page Rendering Updates at 60fps" > > I think the "at60" is deceptive. Maybe call it > "WebCompatibleRenderingUpdateFrequencyEnabled" or soothing a bit more vague > like that. Note that this flag is about forcing 60fps updates which is the old path. The code added here is enabled with the flag turned off, which is the default. This was to address feedback from Dean in comment #5. > > Source/WebCore/page/RenderingUpdateScheduler.cpp:143 > > + manager.windowScreenDidChange(displayID, *this); > > + manager.setPreferredFramesPerSecond(*this, m_preferredFramesPerSecond); > > It would be nicer if windowScreenDidChange() took care of updating preferred > FPS. > > Did you test this? Do we have the new FPS at the right time here? This is now out of scope for this patch which won't make changes to the display link on macOS. > > Source/WebCore/platform/graphics/AnimationFrameRate.h:61 > > + return fullSpeedRatio - std::floor(fullSpeedRatio) <= 0.5 ? floorSpeed : ceilSpeed; > > If you're half way between, does this prefer the lower or higher? I'd say the higher fps (lower ratio), looking at the code. > Can we API-test this code? Looking into it. > > Source/WebCore/platform/graphics/AnimationFrameRate.h:70 > > + // FIXME: handle ThrottlingReason::VisuallyIdle > > Do we have to address this FIXME now? If we don't, won't that cause massive > power regressions? What do you suggest we do in this case? Halve the refresh rate? > > Source/WebCore/platform/graphics/AnimationFrameRate.h:81 > > + interval *= 2; > > The '2' should be in a named constant. Will fix. > > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:822 > > +void WebProcessPool::setDisplayLinkPreferredFramesPerSecond(PlatformDisplayID displayID, WebCore::FramesPerSecond preferredFramesPerSecond) > > +{ > > + for (auto& displayLink : m_displayLinks) { > > + if (displayLink->displayID() == displayID) { > > + displayLink->setPreferredFramesPerSecond(preferredFramesPerSecond); > > + return; > > + } > > + } > > +} > > If you have two windows on the same screen, and one is obscured (or > throttled for some reason), will they both call this with different > preferredFramesPerSecond and therefore clobber eachother's value? This is now outside the scope of this patch, but I'll consider this for a future patch.
(In reply to Antoine Quint from comment #13) > > If you have two windows on the same screen, and one is obscured (or > > throttled for some reason), will they both call this with different > > preferredFramesPerSecond and therefore clobber eachother's value? > > This is now outside the scope of this patch, but I'll consider this for a > future patch. I don't think it's OK to regress that with this patch. It reveals an architectural fault that you need to address in this change.
(In reply to Simon Fraser (smfr) from comment #14) > (In reply to Antoine Quint from comment #13) > > > > If you have two windows on the same screen, and one is obscured (or > > > throttled for some reason), will they both call this with different > > > preferredFramesPerSecond and therefore clobber eachother's value? > > > > This is now outside the scope of this patch, but I'll consider this for a > > future patch. > > I don't think it's OK to regress that with this patch. It reveals an > architectural fault that you need to address in this change. What I'm saying is that this DisplayLink-related change is no longer within the scope of this patch, ie. this code is no longer there.
Created attachment 420720 [details] Patch
Committed r273067: <https://commits.webkit.org/r273067> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420720 [details].