888899 - Allow fast iterations of the refresh driver on OS X

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Avi Halachmi (:avih)]

When setting layout.frame_rate to any specific value (other than the default -1) e.g. for some talos tests which stress the browser by iterating ASAP, on OS X it still won't iterate faster than 60hz since presentation is blocked on glSwapBuffers (and apparently currently the refresh driver won't iterate before the layout was presented).

Solution: Use non-blocking swap when layout.frame_rate has a non-default value.

Note that present could be starved when iterating quickly (bug 880036) and until it's fixed, we could reduce starvation duration by setting docshell.event_starvation_delay_hint=10 (bug 884955).

Comment 1

[Avi Halachmi (:avih)]

Attachment: Use blocking swap only for default frame_rate

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 769663 [details] [diff] [review]
Use blocking swap only for default frame_rate

Review of attachment 769663 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextProviderCGL.mm
@@ +137,5 @@
>          if (mContext) {
>              [mContext makeCurrentContext];
> +            // Only use blocking swap for default frame-rate
> +            // (allow faster iterations with higher specified frame rates)
> +            GLint swapInt = Preferences::GetInt("layout.frame_rate", -1) == -1 ? 1 : 0;

I think this can be used from a thread other than the main thread. I'm not sure what our story for getting preferences to non-main threads is. BenWa may know.

Comment 3

[Avi Halachmi (:avih)]

Attachment: V2 - Access prefs from the main thread only

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 772396 [details] [diff] [review]
V2 - Access prefs from the main thread only

Review of attachment 772396 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContextProviderCGL.mm
@@ +136,5 @@
>  
>          if (mContext) {
>              [mContext makeCurrentContext];
> +            // Use blocking swap only with default frame rate
> +            GLint swapInt = gfxPlatform::GetPrefLayoutFrameRate() == -1 ? 1 : 0;

It's probably worth expanding this comment to explain the motivation behind this.

Comment 5

[Avi Halachmi (:avih)]

Attachment: V3 - More verbose comment

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c495f93e185

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0c495f93e185

Comment 7

[Avi Halachmi (:avih)]

It came to my mind that if a non-60hz monitor is used, and the user wishes to adjust the refresh rate to e.g. 75 to sync with her monitor, then following this bug we'll loose blocking on such case, which could result in tearing.

I think the better solution is to adjust to the monitor refresh rate whenever layout.frame_rate==-1. We can do that either as a fixed rate (i.e. read the monitor's rate and set timer frequency to it) or sync to the actual vblank signal per iteration (as we do on windows following bug 856427).

Another solution is to back out this bug and modify it such that we use another pref which indicates yes/not-blocking, rather than use non-blocking whenever layout.framerate!=-1 .

The bottom line, however, is that currently users with non-60hz monitors can't manually set the rate and expect nice smooth animation like they would with 60hz monitors and default rate, and they can get tearing.

@jeff: how easy would it be to read the monitor's refresh rate?

Comment 8

[Avi Halachmi (:avih)]

Due to the issue mentioned in comment 7, after discussing with jeff on IRC, we agreed on backing out patch v3, change the pref name to layout.frame_rate.vsync.enabled (defaults to true) and re-land.

This new pref isn't used elsewhere right now, but we might use it elsewhere in the future.

Note: this creates somewhat inconsistent/confusing semantics together with layout.frame_rate.

Before applying the above, layout.frame_rate is used to define the frame-rate (effectively max frame rate), where -1 indicates a "default" frame rate, which resolves to the monitor's refresh rate if available (vblank intervals on windows with DWM) or 60 otherwise (linux, osx).

However, on OSX -1 also meant, following patch V3, that we don't block on swap/present (we do block by default). On window we currently never block on present.

After applying the above:
- layout.frame_rate will define ONLY the maximum frame rate.
- layout.frame_rate.vsync.enabled will choose between blocking/non-blocking present, if available.

This will allow the somewhat confusing combinations of frame_rate=-1 with vsync.enabled=false (monitor's refresh rate but non-blocking present), and frame_rate=NN with vsync.enabled=true.

It does make sense semantically, but it's more refined than before and possibly slightly confusing.

Comment 9

[Avi Halachmi (:avih)]

ni? on the prefs naming/semantics/approach from comment 8, while taking into account future compatibility/platform issues, if possible.

Comment 10

[Timothy Nikkel (:tnikkel)]

Do we only want this for testing/benchmarking purposes? How about a pref for "run the refresh driver as fast as possible" instead of hacking it by using a really high frame rate?

Or is separating vsync from the frame rate actually a desirable change we want outside of benchmarking?

Comment 11

[Avi Halachmi (:avih)]

Iterating frames/animations as fast as possible is useful for regression testing, which was the main trigger for this bug.

However, for firefox-gaming, I foresee the need for vsync on/off (as many games offer - since vsync can introduce some input-display lag which hardcore gamers will do anything to gminimize) - where vsync-off typically means as-fast-as-possible, and I also see the usefulness of frame-rate limit (some games offer this as well).

So a pref for "iterate as fast as possible" to rule them all doesn't sound too bad, especially for testing, but at some stage we will probably also want to control vsync on/off and frame-rate limit.

Comment 12

[Rail Aliiev [:rail]]

In production

Comment 13

[Benoit Girard (:BenWa)]

(In reply to Avi Halachmi (:avih) from comment #8)
> This will allow the somewhat confusing combinations of frame_rate=-1 with
> vsync.enabled=false (monitor's refresh rate but non-blocking present), and
> frame_rate=NN with vsync.enabled=true.
> 
> It does make sense semantically, but it's more refined than before and
> possibly slightly confusing.

A few things are still not very clear and let me clarify them:
layout.frame_rate: For this I think we want 3 states: As fast as possible, refresh interval (or 60 if we can't read it), fixed/user defined interval. Perhaps the values can be (-1, 0, >0).
layout.frame_rate.vsync.enabled: We do our best to vsync as available in the platform. Note that I am being vague here because it either single buffer or double buffers. So while you may run at 5FPS, 2 of those frames may be within the vsync interval.

With this definition I don't think the combo is confusing if you think about it. frame_rate=0 and no vsync means you want to produce a correct amount of frames but don't want to block like on non-OMTC platforms.

Comment 14

[Avi Halachmi (:avih)]

Yeah, I like the suggestion for 3 modes of layout.frame_rate .

For semantic backwards compatibility, however, I'll keep -1 for "automatic limiter" (vsync or 60), 0 for ASAP, and >0 for explicit.

I'll post a patch soon which will also take this into account at the refresh driver. Thanks.

Comment 15

[Avi Halachmi (:avih)]

Attachment: Part 2 - Uses layout.frame_rate=0 for asap mode (instead of 10k)

After talking to benwa, we've decided that right now there's no need for special pref for vsync on/off.

The suggested 3 modes of layout.frame_rate (default, asap, custom) also define vsync blocking implicitly well enough: non-blocking for asap, as required/possible otherwise.

If/when we need the extra resolution, we can add other prefs according to those needs, but right now we're unaware of such potential needs.

Also, I think this should get into fx25, since otherwise 25 will use 10k for asap mode, while fx26 will change it to 0.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 785070 [details] [diff] [review]
Part 2 - Uses layout.frame_rate=0 for asap mode (instead of 10k)

Review of attachment 785070 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +655,5 @@
>  // outIsDefault indicates that rate was not explicitly set by the user
>  // so we might choose other, more appropriate rates (e.g. vsync, etc)
> +// layout.frame_rate=0 indicates "ASAP mode".
> +// In ASAP mode rendering is iterated as fast as possible (typically for stress testing).
> +// A target rate of 10k is used internally instead of special-handling 0.

It may be worth noting that backends should not wait for vsync if possible.

Comment 17

[Avi Halachmi (:avih)]

Attachment: Part2 v2: address comment 16

https://hg.mozilla.org/integration/mozilla-inbound/rev/da07105f7c70

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da07105f7c70