Android: Consolidate and simplify VSync logic

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index de2ac4ba292cea89e8bb1105942102dc8d99be45..1fd81b128e5044b5a585fe605ea022026605ced9 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -145,6 +145,7 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
     ContentViewCoreImpl* content_view_core)
     : host_(widget_host),
       needs_begin_frame_(false),
+      requested_vsync_for_touch_event_(false),
       is_showing_(!widget_host->is_hidden()),
       content_view_core_(NULL),
       ime_adapter_android_(this),
@@ -485,36 +486,14 @@ void RenderWidgetHostViewAndroid::OnDidChangeBodyBackgroundColor(
     content_view_core_->OnBackgroundColorChanged(color);
 }
 
-void RenderWidgetHostViewAndroid::SendBeginFrame(
-    const cc::BeginFrameArgs& args) {
-  TRACE_EVENT0("cc", "RenderWidgetHostViewAndroid::SendBeginFrame");
-  if (!host_)
-    return;
-
-  if (flush_input_requested_) {
-    flush_input_requested_ = false;
-    host_->FlushInput();
-    content_view_core_->RemoveBeginFrameSubscriber();
-  }
-
-  host_->Send(new ViewMsg_BeginFrame(host_->GetRoutingID(), args));
-}
-
 void RenderWidgetHostViewAndroid::OnSetNeedsBeginFrame(
     bool enabled) {
   TRACE_EVENT1("cc", "RenderWidgetHostViewAndroid::OnSetNeedsBeginFrame",
                "enabled", enabled);
-  // ContentViewCoreImpl handles multiple subscribers to the BeginFrame, so
-  // we have to make sure calls to ContentViewCoreImpl's
-  // {Add,Remove}BeginFrameSubscriber are balanced, even if
-  // RenderWidgetHostViewAndroid's may not be.
-  if (content_view_core_ && needs_begin_frame_ != enabled) {
-    if (enabled)
-      content_view_core_->AddBeginFrameSubscriber();
-    else
-      content_view_core_->RemoveBeginFrameSubscriber();
-    needs_begin_frame_ = enabled;
-  }
+  if (content_view_core_ && enabled)

[Sami, 2014/04/16 17:26:54]

Are we sure the calls here are balanced and never overlap badly? Maybe we can
adds some asserts to keep it that way. IIRC the problem was that the renderer
was doing something funky during navigation and the BeginFrame-disable from the
earlier page ended up disabling BeginFrame for the new page.

[no sievers, 2014/04/25 22:23:33]

Maybe that was just a problem with CVC<->RWHV badness? There is a whole bunch of
bugs because of that.
I don't think you can get IPC messages in RWHV from the wrong compositor
(RenderWidget and RWHV should be 1:1). Otherwise you'd also be able to receive
CompositorFrame messages from the wrong content :)

With some logging I do see a few transitions in terms of SetNeedsBeginFrame()
when I navigate from one domain to another, both for previous RWHV/OS and the
new pair. But doesn't look like there would be a problem.

+    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+
+  needs_begin_frame_ = enabled;
 }
 
 void RenderWidgetHostViewAndroid::OnStartContentIntent(
@@ -984,6 +963,10 @@ void RenderWidgetHostViewAndroid::RemoveLayers() {
   overscroll_effect_->Disable();
 }
 
+void RenderWidgetHostViewAndroid::SetNeedsAnimate() {
+  content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+}
+
 bool RenderWidgetHostViewAndroid::Animate(base::TimeTicks frame_time) {
   return overscroll_effect_->Animate(frame_time);
 }
@@ -1090,7 +1073,6 @@ void RenderWidgetHostViewAndroid::OnSetNeedsFlushInput() {
     return;
   TRACE_EVENT0("input", "RenderWidgetHostViewAndroid::OnSetNeedsFlushInput");
   flush_input_requested_ = true;
-  content_view_core_->AddBeginFrameSubscriber();
 }
 
 void RenderWidgetHostViewAndroid::CreateBrowserAccessibilityManagerIfNeeded() {
@@ -1181,6 +1163,14 @@ void RenderWidgetHostViewAndroid::SendTouchEvent(
     const blink::WebTouchEvent& event) {
   if (host_)
     host_->ForwardTouchEventWithLatencyInfo(event, CreateLatencyInfo(event));
+
+  // Send a proactive BeginFrame on the next vsync to reduce latency.

[Sami, 2014/04/16 17:26:54]

I don't think this is needed any more since VSyncMonitor triggers immediately
when activated from idle. Let's make sure with a trace though :) 

[no sievers, 2014/04/25 22:23:33]

Chatted with Brian. I'll leave it in this patch just to make sure it does not
regress because of the extra latency. The render compositor will only send
SetNeedsBeginFrame when needs_redraw becomes true, which might only be after we
send a scrollBy on the next vsync or so.

FWIU with this logic:

vsync1
BeginGesture
requestVsync()
SendBeginFrame() 1

vsync2
SendBeginFrame() 2
scroll

Now when the scroll happens in the renderer, the scheduler there falls into the
retroactive  BeginFrame logic and can draw immediately. I guess SendBeginFrame()
1 is not even needed.

Without this logic:

vsync1
BeginGesture

vsync2
scroll
renderer needs redraw
OnSetNeedsBeginFrame
requestVSync
OnVSync

Does that make sense?

[brianderson, 2014/04/25 23:26:02]

Does the Browser send a BeginFrame to the Renderer even before it's requested
it? I thought that was the case, so I think we should keep this logic and what
you say makes sense.

If the Browser doesn't send the BeginFrame early, I think we should fix that in
a separate patch and still keep this logic.

+  // This is good enough as long as the first touch event has Begin semantics
+  // and the actual scroll happens on the next vsync.
+  if (content_view_core_) {
+    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+    requested_vsync_for_touch_event_ = true;
+  }
 }
 
 void RenderWidgetHostViewAndroid::SendMouseEvent(
@@ -1226,7 +1216,7 @@ void RenderWidgetHostViewAndroid::OnOverscrolled(
           base::TimeTicks::Now(),
           gfx::ScaleVector2d(accumulated_overscroll, device_scale_factor),
           gfx::ScaleVector2d(current_fling_velocity, device_scale_factor))) {
-    content_view_core_->SetNeedsAnimate();
+    SetNeedsAnimate();
   }
 }
 
@@ -1264,6 +1254,8 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
   if (content_view_core_ && !root_window_destroyed_ &&
       !using_synchronous_compositor_) {
     content_view_core_->GetWindowAndroid()->AddObserver(this);
+    if (needs_begin_frame_)
+      content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
   }
 }
 
@@ -1288,6 +1280,39 @@ void RenderWidgetHostViewAndroid::OnWillDestroyWindow() {
   root_window_destroyed_ = true;
 }
 
+void RenderWidgetHostViewAndroid::OnVSync(base::TimeTicks frame_time,
+                                          base::TimeDelta vsync_period) {
+  TRACE_EVENT0("cc", "RenderWidgetHostViewAndroid::OnVSync");
+  if (!host_)
+    return;
+
+  if (flush_input_requested_) {
+    flush_input_requested_ = false;
+    host_->FlushInput();
+  }
+
+  if (needs_begin_frame_ || requested_vsync_for_touch_event_) {

[no sievers, 2014/04/25 22:23:33]

I removed this if-condition as it seems to be a functional change from before,
where we'd allow spurious extra BeginFrames (according to
MAX_AUTO_ONVSYNC_COUNT).

+    TRACE_EVENT0("cc", "RenderWidgetHostViewAndroid::SendBeginFrame");
+    base::TimeTicks display_time = frame_time + vsync_period;
+
+    // TODO(brianderson): Use adaptive draw-time estimation.
+    base::TimeDelta estimated_browser_composite_time =
+        base::TimeDelta::FromMilliseconds(1000.0 / (3 * 60));

[Sami, 2014/04/16 17:26:54]

This drops some precision -- better use FromMicroseconds.

[no sievers, 2014/04/25 22:23:33]

Done.

+
+    base::TimeTicks deadline = display_time - estimated_browser_composite_time;
+
+    host_->Send(new ViewMsg_BeginFrame(
+        host_->GetRoutingID(),
+        cc::BeginFrameArgs::Create(frame_time, deadline, vsync_period)));
+    requested_vsync_for_touch_event_ = false;
+  }
+
+  // TODO(sievers): This should use the LayerTreeHostClient callback
+  bool needs_animate = Animate(frame_time);
+  if (needs_begin_frame_ || needs_animate)
+    content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+}
+
 void RenderWidgetHostViewAndroid::OnLostResources() {
   ReleaseLocksOnSurface();
   if (layer_.get())