Closed
Bug 1117870
Opened 10 years ago
Closed 10 years ago
Let the CompositorVsyncObserver unobserve vsync after a configurable number of vsyncs have passed
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: mchang, Assigned: mchang)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
3.93 KB,
patch
|
mchang
:
review+
|
Details | Diff | Splinter Review |
Before, the CompositorVsyncObserver would unobserve vsync after 1 vsync if a composite was not needed. This unobserves from vsync too often, especially now that we always dispatch touch events only from the compositor. For example, on b2g, there is a delay between a touch start / move and the compositor compositing, at which point a vsync can occur and the compositor would unobserve from vsync. This would force us to miss the touch event, which creates a very janky experience.
Instead, let's introduce a heuristic where after N number of vsync events, we unobserve vsync.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8544095 -
Flags: review?(bgirard)
Comment 2•10 years ago
|
||
Do you intend others to change this value? Need a tiny bit of convincing that this needs a preference.
Comment 3•10 years ago
|
||
Comment on attachment 8544095 [details] [diff] [review]
Unobserve Vsync After N Number of Vsync Notifications in Compositor
Review of attachment 8544095 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/thebes/gfxPrefs.h
@@ +218,5 @@
>
> // Use vsync events generated by hardware
> DECL_GFX_PREF(Once, "gfx.vsync.hw-vsync.enabled", HardwareVsyncEnabled, bool, false);
> DECL_GFX_PREF(Once, "gfx.vsync.compositor", VsyncAlignedCompositor, bool, false);
> + DECL_GFX_PREF(Once, "gfx.vsync.compositor.unobserve-count", CompositorUnobserveCount, int32_t, 10);
Let's document this value. What it does and why 10 is chosen. If we decide to remove the preference then document it where this variable is moved.
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #2)
> Do you intend others to change this value? Need a tiny bit of convincing
> that this needs a preference.
I don't intend to have others change this value, but I think it will make QA's life easier if we hit situations during more extensive testing where 10 might not be enough, or might actually be too much. It's mostly so that when we flip the switch, it's easier for QA / anyone else to test to see if it fixes their issue. If during testing, we find that 10 is enough, I'd gladly move it out of preferences and into a static constant variable. Is that convincing enough?
(In reply to Benoit Girard (:BenWa) from comment #3)
> Comment on attachment 8544095 [details] [diff] [review]
> Unobserve Vsync After N Number of Vsync Notifications in Compositor
>
> Review of attachment 8544095 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/thebes/gfxPrefs.h
> @@ +218,5 @@
> >
> > // Use vsync events generated by hardware
> > DECL_GFX_PREF(Once, "gfx.vsync.hw-vsync.enabled", HardwareVsyncEnabled, bool, false);
> > DECL_GFX_PREF(Once, "gfx.vsync.compositor", VsyncAlignedCompositor, bool, false);
> > + DECL_GFX_PREF(Once, "gfx.vsync.compositor.unobserve-count", CompositorUnobserveCount, int32_t, 10);
>
> Let's document this value. What it does and why 10 is chosen. If we decide
> to remove the preference then document it where this variable is moved.
On b2g, in really bad cases, I've seen up to 80 ms delays between touch events and the main thread processing them. So 80 ms / 16 = 5 vsync events. Double it up just to be on the safe side. I can document this reasoning in the pref file.
Flags: needinfo?(bgirard)
Comment 5•10 years ago
|
||
Comment on attachment 8544095 [details] [diff] [review]
Unobserve Vsync After N Number of Vsync Notifications in Compositor
Review of attachment 8544095 [details] [diff] [review]:
-----------------------------------------------------------------
Ok. r+ with the comment.
Attachment #8544095 -
Flags: review?(bgirard) → review+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bgirard)
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Carrying r+, updated to include comments in pref file.
Attachment #8544095 -
Attachment is obsolete: true
Attachment #8544213 -
Flags: review+
Comment 8•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•