Android: Enable vsync for synthetic gestures

Android: Enable vsync for synthetic gestures

A recent refactoring accidentally removed the code that turns on vsync
for synthetic gestures. This causes the gesture to never start if
nothing else happens to turn on vsync at the same time.

BUG=370967
TEST=tools/perf/run_benchmark rasterize_and_record_micro.key_silk_cases
--browser=android-chrome --page-filter=slide_drawer

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269561

[no sievers, 2014-05-08 18:34:27]

lgtm if this fixes the instrumentation

But can you explain a bit more why the gesture needs a vsync, so I don't break
it again? :)
Does it need the unsolicited BeginFrame resulting from that? If so, why is there
no SetNeedsBeginFrame?

[jdduke (slow), 2014-05-08 18:43:35]

On 2014/05/08 18:34:27, sievers wrote:
> lgtm if this fixes the instrumentation
> 
> But can you explain a bit more why the gesture needs a vsync, so I don't break
> it again? :)
> Does it need the unsolicited BeginFrame resulting from that? If so, why is
there
> no SetNeedsBeginFrame?

It *might* need a BeginFrame, but it definitely needs a regular impulse signal
to drive generation of touch/gesture events (e.g., on Android events are
dispatched before vsync, so the synthetic gesture framework mirrors that
approach). lgtm.

[no sievers, 2014-05-08 19:02:05]

On 2014/05/08 18:43:35, jdduke wrote:
> On 2014/05/08 18:34:27, sievers wrote:
> > lgtm if this fixes the instrumentation
> > 
> > But can you explain a bit more why the gesture needs a vsync, so I don't
break
> > it again? :)
> > Does it need the unsolicited BeginFrame resulting from that? If so, why is
> there
> > no SetNeedsBeginFrame?
> 
> It *might* need a BeginFrame, but it definitely needs a regular impulse signal
> to drive generation of touch/gesture events (e.g., on Android events are
> dispatched before vsync, so the synthetic gesture framework mirrors that
> approach). lgtm.

Ok so it mostly needs OnVSync->FlushInput().

I was mainly wondering about the extra logic that I had in my patch at one point
to only send the BeginFrame from OnVSync if needs_begin_frame_ is high and if it
would break this too.

[jdduke (slow), 2014-05-08 19:42:03]

On 2014/05/08 19:02:05, sievers wrote:
> I was mainly wondering about the extra logic that I had in my patch at one
point
> to only send the BeginFrame from OnVSync if needs_begin_frame_ is high and if
it
> would break this too.

Good question... it looks like we still have some logic in RWHVA::SendTouchEvent
to proactively subscribe to the vsync signal.  We'd probably have to tweak that
model if we were to suppress BeginFrame dispatch on vsync if
|needs_begin_frame_| is still false? Sami might know better?

[Sami, 2014-05-09 09:37:42]

On 2014/05/08 19:42:03, jdduke wrote:
> On 2014/05/08 19:02:05, sievers wrote:
> > I was mainly wondering about the extra logic that I had in my patch at one
> point
> > to only send the BeginFrame from OnVSync if needs_begin_frame_ is high and
if
> it
> > would break this too.
> 
> Good question... it looks like we still have some logic in
RWHVA::SendTouchEvent
> to proactively subscribe to the vsync signal.  We'd probably have to tweak
that
> model if we were to suppress BeginFrame dispatch on vsync if
> |needs_begin_frame_| is still false? Sami might know better?

Synthetic gestures aren't really directly tied to BeginFrames. We just need the
VSync to actually generate the gesture; whether or not that results in the
renderer asking for a BeginFrame is secondary. The proactive BeginFrame stuff is
also orthogonal.

On that note, I was also wondering whether that's still needed because the
VSyncMonitor can now react immediately to new subscriptions. The proactive route
is still likely faster thanks to fewer IPC round trips, so we'd have to check
the timings before we can remove it.

[Sami, 2014-05-09 09:37:47]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-09 09:42:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/269343013/1

[commit-bot: I haz the power, 2014-05-10 11:40:31]

Change committed as 269561