The default bug view has changed. See this FAQ.

Allow fast iterations of the refresh driver on OS X

RESOLVED FIXED in mozilla25

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: avih, Assigned: avih)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla25
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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).
(Assignee)

Comment 1

4 years ago
Created attachment 769663 [details] [diff] [review]
Use blocking swap only for default frame_rate
Attachment #769663 - Flags: review?(jmuizelaar)
(Assignee)

Updated

4 years ago
Blocks: 845943
(Assignee)

Updated

4 years ago
Blocks: 848358
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.
Attachment #769663 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 3

4 years ago
Created attachment 772396 [details] [diff] [review]
V2 - Access prefs from the main thread only
Attachment #769663 - Attachment is obsolete: true
Attachment #772396 - Flags: review?(jmuizelaar)
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.
Attachment #772396 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 5

4 years ago
Created attachment 773433 [details] [diff] [review]
V3 - More verbose comment

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c495f93e185
Attachment #772396 - Attachment is obsolete: true
Attachment #773433 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0c495f93e185
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 7

4 years ago
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?
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 8

4 years ago
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(jmuizelaar)
Resolution: FIXED → ---
(Assignee)

Updated

4 years ago
Blocks: 897054
(Assignee)

Comment 9

4 years ago
ni? on the prefs naming/semantics/approach from comment 8, while taking into account future compatibility/platform issues, if possible.
Flags: needinfo?(tnikkel)
Flags: needinfo?(bgirard)
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?
Flags: needinfo?(tnikkel)
(Assignee)

Comment 11

4 years ago
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.
In production
(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.
Flags: needinfo?(bgirard)
(Assignee)

Comment 14

4 years ago
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.
(Assignee)

Comment 15

4 years ago
Created attachment 785070 [details] [diff] [review]
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.
Attachment #773433 - Attachment is obsolete: true
Attachment #785070 - Flags: review?(jmuizelaar)
(Assignee)

Updated

4 years ago
Attachment #773433 - Attachment is obsolete: false
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.
Attachment #785070 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 785192 [details] [diff] [review]
Part2 v2: address comment 16

https://hg.mozilla.org/integration/mozilla-inbound/rev/da07105f7c70
Attachment #785070 - Attachment is obsolete: true
Attachment #785192 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/da07105f7c70
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Blocks: 908741
You need to log in before you can comment on or make changes to this bug.