1117870 - Let the CompositorVsyncObserver unobserve vsync after a configurable number of vsyncs have passed

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

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.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Unobserve Vsync After N Number of Vsync Notifications in Compositor

Comment 2

[Benoit Girard (:BenWa)]

Do you intend others to change this value? Need a tiny bit of convincing that this needs a preference.

Comment 3

[Benoit Girard (:BenWa)]

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.

Comment 4

[Mason Chang [Inactive] [:mchang]]

(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.

Comment 5

[Benoit Girard (:BenWa)]

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.

Comment 6

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/378c4ef30784

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Unobserve Vsync After N Number of Vsync Notifications in Compositor

Carrying r+, updated to include comments in pref file.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/378c4ef30784