Android: Consolidate and simplify VSync logic

content/browser/renderer_host/render_widget_host_view_android.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/render_widget_host_view_android.h"
 
 #include <android/bitmap.h>
 
 #include "base/android/sys_utils.h"
 #include "base/basictypes.h"
@@ skipping 127 matching lines @@
     scoped_ptr<cc::CompositorFrame> output_frame)
     : output_surface_id(output_id), frame(output_frame.Pass()) {}
 
 RenderWidgetHostViewAndroid::LastFrameInfo::~LastFrameInfo() {}
 
 RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
     RenderWidgetHostImpl* widget_host,
     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),
       cached_background_color_(SK_ColorWHITE),
       last_output_surface_id_(kUndefinedOutputSurfaceId),
       weak_ptr_factory_(this),
       overscroll_effect_enabled_(!CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableOverscrollEdgeEffect)),
       overscroll_effect_(OverscrollGlow::Create(overscroll_effect_enabled_)),
       flush_input_requested_(false),
@@ skipping 320 matching lines @@
 void RenderWidgetHostViewAndroid::OnDidChangeBodyBackgroundColor(
     SkColor color) {
   if (cached_background_color_ == color)
     return;
 
   cached_background_color_ = color;
   if (content_view_core_)
     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(
     const GURL& content_url) {
   if (content_view_core_)
     content_view_core_->StartContentIntent(content_url);
 }
 
 void RenderWidgetHostViewAndroid::OnSmartClipDataExtracted(
     const base::string16& result) {
@@ skipping 449 matching lines @@
 void RenderWidgetHostViewAndroid::RemoveLayers() {
   if (!content_view_core_)
     return;
   if (!layer_.get())
     return;
 
   content_view_core_->RemoveLayer(layer_);
   overscroll_effect_->Disable();
 }
 
+void RenderWidgetHostViewAndroid::SetNeedsAnimate() {
+  content_view_core_->GetWindowAndroid()->RequestVSyncUpdate();
+}
+
 bool RenderWidgetHostViewAndroid::Animate(base::TimeTicks frame_time) {
   return overscroll_effect_->Animate(frame_time);
 }
 
 void RenderWidgetHostViewAndroid::AcceleratedSurfacePostSubBuffer(
     const GpuHostMsg_AcceleratedSurfacePostSubBuffer_Params& params,
     int gpu_host_id) {
   NOTREACHED();
 }
 
@@ skipping 86 matching lines @@
   if (compositor)
     return compositor->HandleInputEvent(input_event);
   return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
 }
 
 void RenderWidgetHostViewAndroid::OnSetNeedsFlushInput() {
   if (flush_input_requested_ || !content_view_core_)
     return;
   TRACE_EVENT0("input", "RenderWidgetHostViewAndroid::OnSetNeedsFlushInput");
   flush_input_requested_ = true;
-  content_view_core_->AddBeginFrameSubscriber();
 }
 
 void RenderWidgetHostViewAndroid::CreateBrowserAccessibilityManagerIfNeeded() {
   if (!host_ || host_->accessibility_mode() != AccessibilityModeComplete)
     return;
 
   if (!GetBrowserAccessibilityManager()) {
     base::android::ScopedJavaLocalRef<jobject> obj;
     if (content_view_core_)
       obj = content_view_core_->GetJavaObject();
@@ skipping 70 matching lines @@
 void RenderWidgetHostViewAndroid::SendKeyEvent(
     const NativeWebKeyboardEvent& event) {
   if (host_)
     host_->ForwardKeyboardEvent(event);
 }
 
 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(
     const blink::WebMouseEvent& event) {
   if (host_)
     host_->ForwardMouseEvent(event);
 }
 
 void RenderWidgetHostViewAndroid::SendMouseWheelEvent(
     const blink::WebMouseWheelEvent& event) {
@@ skipping 25 matching lines @@
     gfx::Vector2dF current_fling_velocity) {
   if (!content_view_core_ || !layer_ || !is_showing_)
     return;
 
   const float device_scale_factor = content_view_core_->GetDpiScale();
   if (overscroll_effect_->OnOverscrolled(
           content_view_core_->GetLayer(),
           base::TimeTicks::Now(),
           gfx::ScaleVector2d(accumulated_overscroll, device_scale_factor),
           gfx::ScaleVector2d(current_fling_velocity, device_scale_factor))) {
-    content_view_core_->SetNeedsAnimate();
+    SetNeedsAnimate();
   }
 }
 
 void RenderWidgetHostViewAndroid::DidStopFlinging() {
   if (content_view_core_)
     content_view_core_->DidStopFlinging();
 }
 
 void RenderWidgetHostViewAndroid::SetContentViewCore(
     ContentViewCoreImpl* content_view_core) {
@@ skipping 17 matching lines @@
     if (content_view_core_)
       obj = content_view_core_->GetJavaObject();
     GetBrowserAccessibilityManager()->ToBrowserAccessibilityManagerAndroid()->
         SetContentViewCore(obj);
   }
 
   AttachLayers();
   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();
   }
 }
 
 void RenderWidgetHostViewAndroid::RunAckCallbacks() {
   while (!ack_callbacks_.empty()) {
     ack_callbacks_.front().Run();
     ack_callbacks_.pop();
   }
 }
 
 void RenderWidgetHostViewAndroid::OnCompositingDidCommit() {
   RunAckCallbacks();
 }
 
 void RenderWidgetHostViewAndroid::OnDetachCompositor() {
   DCHECK(content_view_core_);
   DCHECK(!using_synchronous_compositor_);
   RunAckCallbacks();
 }
 
 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())
     DestroyDelegatedContent();
   DCHECK(ack_callbacks_.empty());
 }
 
 // static
 void
 RenderWidgetHostViewAndroid::PrepareTextureCopyOutputResultForDelegatedReadback(
@@ skipping 105 matching lines @@
 // RenderWidgetHostView, public:
 
 // static
 RenderWidgetHostView*
 RenderWidgetHostView::CreateViewForWidget(RenderWidgetHost* widget) {
   RenderWidgetHostImpl* rwhi = RenderWidgetHostImpl::From(widget);
   return new RenderWidgetHostViewAndroid(rwhi, NULL);
 }
 
 } // namespace content