[android] Make RWHVAndroid a BeginFrameObserver.

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 60078386bb068c281f82bf56a7bf23718125a432..8dabae252d89d87df355d4bd96b7de944a4fd3b8 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -76,7 +76,6 @@
 #include "third_party/khronos/GLES2/gl2.h"
 #include "third_party/khronos/GLES2/gl2ext.h"
 #include "third_party/skia/include/core/SkCanvas.h"
-#include "ui/android/delegated_frame_host_android.h"
 #include "ui/android/window_android.h"
 #include "ui/android/window_android_compositor.h"
 #include "ui/base/layout.h"
@@ -438,7 +437,8 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
     RenderWidgetHostImpl* widget_host,
     ContentViewCoreImpl* content_view_core)
     : host_(widget_host),
-      outstanding_vsync_requests_(0),
+      begin_frame_source_(nullptr),
+      outstanding_begin_frame_requests_(0),
       is_showing_(!widget_host->is_hidden()),
       is_window_visible_(true),
       is_window_activity_started_(true),
@@ -462,9 +462,7 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
   view_.SetLayer(cc::Layer::Create());
   if (using_browser_compositor_) {
     delegated_frame_host_.reset(new ui::DelegatedFrameHostAndroid(
-        &view_, cached_background_color_,
-        base::Bind(&RenderWidgetHostViewAndroid::ReturnResources,
-                   weak_ptr_factory_.GetWeakPtr())));
+        &view_, cached_background_color_, this));
   }
 
   host_->SetView(this);
@@ -802,9 +800,9 @@ void RenderWidgetHostViewAndroid::SetNeedsBeginFrames(bool needs_begin_frames) {
   TRACE_EVENT1("cc", "RenderWidgetHostViewAndroid::SetNeedsBeginFrames",
                "needs_begin_frames", needs_begin_frames);
   if (needs_begin_frames)
-    RequestVSyncUpdate(PERSISTENT_BEGIN_FRAME);
+    AddBeginFrameRequest(PERSISTENT_BEGIN_FRAME);
   else
-    outstanding_vsync_requests_ &= ~PERSISTENT_BEGIN_FRAME;
+    ClearBeginFrameRequest(PERSISTENT_BEGIN_FRAME);
 }
 
 void RenderWidgetHostViewAndroid::OnStartContentIntent(
@@ -854,11 +852,11 @@ bool RenderWidgetHostViewAndroid::OnTouchEvent(
   host_->ForwardTouchEventWithLatencyInfo(web_event, latency_info);
 
   // Send a proactive BeginFrame for this vsync to reduce scroll latency for
-  // scroll-inducing touch events. Note that Android's Choreographer ensures
-  // that BeginFrame requests made during ACTION_MOVE dispatch will be honored
-  // in the same vsync phase.
+  // scroll-inducing touch events. Note that even if we weren't observing the
+  // BeginFrameSource prior to this, we will receive a (missed) BeginFrame for
+  // the current vsync phase.

[boliu, 2016/12/12 21:41:48]

I think previous comment is fine.

RWHVA could already be observing begin frames, in which case the missed frame
won't help. Also looking at https://codereview.chromium.org/1879833002 when the
MISSED frame behavior was added, it was more for the browser compositor rather
than the renderer one.

[Eric Seckler, 2016/12/13 10:03:56]

Ok, reverted. We probably don't want to get rid of missed frames entirely, see
below.

   if (observing_root_window_ && result.moved_beyond_slop_region)
-    RequestVSyncUpdate(BEGIN_FRAME);
+    AddBeginFrameRequest(BEGIN_FRAME);
 
   return true;
 }
@@ -1384,9 +1382,9 @@ void RenderWidgetHostViewAndroid::ShowInternal() {
 
   host_->WasShown(ui::LatencyInfo());
 
-  if (content_view_core_) {
+  if (content_view_core_ && view_.GetWindowAndroid()) {
     StartObservingRootWindow();
-    RequestVSyncUpdate(BEGIN_FRAME);
+    AddBeginFrameRequest(BEGIN_FRAME);
   }
 }
 
@@ -1429,29 +1427,53 @@ void RenderWidgetHostViewAndroid::HideInternal() {
   host_->WasHidden();
 }
 
-void RenderWidgetHostViewAndroid::RequestVSyncUpdate(uint32_t requests) {
-  bool should_request_vsync = !outstanding_vsync_requests_ && requests;
-  outstanding_vsync_requests_ |= requests;
+void RenderWidgetHostViewAndroid::SetBeginFrameSource(
+    cc::BeginFrameSource* begin_frame_source) {
+  if (begin_frame_source_ == begin_frame_source)
+    return;
+
+  if (begin_frame_source_ && outstanding_begin_frame_requests_)
+    begin_frame_source_->RemoveObserver(this);
+  begin_frame_source_ = begin_frame_source;
+  if (begin_frame_source_ && outstanding_begin_frame_requests_)
+    begin_frame_source_->AddObserver(this);
+}
 
-  // Note that if we're not currently observing the root window, outstanding
-  // vsync requests will be pushed if/when we resume observing in
-  // |StartObservingRootWindow()|.
-  if (observing_root_window_ && should_request_vsync) {
+void RenderWidgetHostViewAndroid::AddBeginFrameRequest(
+    BeginFrameRequestType request) {
+  DCHECK(this);

[boliu, 2016/12/12 21:41:48]

hmm?

[Eric Seckler, 2016/12/13 10:03:56]

thanks, was debugging here.. removed :)

+  uint32_t requests = outstanding_begin_frame_requests_ | request;
+
+  // Note that if we don't currently have a BeginFrameSource, outstanding begin
+  // frame requests will be pushed if/when we get one during
+  // |StartObservingRootWindow()| or when the DelegatedFrameHostAndroid sets it.
+  cc::BeginFrameSource* source = begin_frame_source_;
+  if (source && requests && !outstanding_begin_frame_requests_) {
     ui::WindowAndroid* windowAndroid = view_.GetWindowAndroid();
     DCHECK(windowAndroid);

[boliu, 2016/12/12 21:41:48]

So windowAndroid pointer can change throughout the lifetime of the RWHVA, and
the start/stopObservingWindowAndroid calls roughly corresponds to when the
window android is set/unset

For the webview case, I think this is still handled correctly.

What's not clear for me is for chrome after begin frame is unified, will the
begin frame source ever change, even when the windowAndroid changes? And does
the begin frame call still go through windowAndroid?

If not, then technically, this DCHECK isn't really valid for chrome anymore.

If yes, then will need to ensure that sequence still works correctly

[Eric Seckler, 2016/12/13 10:03:56]

In android-chrome, there's a direct relationship between a WindowAndroid and
cc::Display (one CompositorImpl instance, which owns cc::Display, per
WindowAndroid). The cc::Display receives the WindowAndroid's BFS, and registers
it at SurfaceManager, which then distributes it to SurfaceFactoryClients, one of
which is DelegatedFrameHostAndroid.

Thus, in chrome:
 - The BFS of RWHVAndroid changes when WindowAndroid changes.
 - The BFS provided by WindowAndroid feeds both the browser compositor
(cc::Display) and (possibly multiple) RWHVAndroid.

Irrespective of this, I'm not sure if the behavior you saw in crbug.com/639868
may still occur - and whether this check is still needed. In tests, the
DCHECK(windowAndroid) isn't hit - but it probably didn't earlier either.

[boliu, 2016/12/13 18:07:47]

And it looks like RegisterFrameSinkHierarchy->RegisterSurfaceFactoryClient is
where this whole BFS path is hooked up.

Ok, just remove getting the window android here then. It's definitely not going
to prevent any crash anymore afaict.

[Eric Seckler, 2016/12/14 15:36:57]

Ok, got rid of it.

-    // TODO(boliu): This check should be redundant with
-    // |observing_root_window_| check above. However we are receiving trickle
-    // of crash reports (crbug.com/639868) with no root cause. Should
-    // investigate more when time allows what corner case is missed.
+    // TODO(boliu): This check should be redundant with |source| check above.
+    // However, there seem to be cases where we are not notified of window
+    // destruction, which leaves a dangling |source| pointer. See also related
+    // crash reports in crbug.com/639868 (from before RWHVA became a
+    // BeginFrameObserver).
     if (windowAndroid)
-      windowAndroid->RequestVSyncUpdate();
+      source->AddObserver(this);
   }
+  outstanding_begin_frame_requests_ = requests;
+}
+
+void RenderWidgetHostViewAndroid::ClearBeginFrameRequest(
+    BeginFrameRequestType request) {
+  uint32_t requests = outstanding_begin_frame_requests_ & ~request;
+
+  cc::BeginFrameSource* source = begin_frame_source_;
+  if (source && !requests && outstanding_begin_frame_requests_)
+    source->RemoveObserver(this);
+  outstanding_begin_frame_requests_ = requests;
 }
 
 void RenderWidgetHostViewAndroid::StartObservingRootWindow() {
   DCHECK(content_view_core_);
-  // TODO(yusufo): This will need to have a better fallback for cases where
-  // setContentViewCore is called with a valid ContentViewCore without a window.
   DCHECK(view_.GetWindowAndroid());
   DCHECK(is_showing_);
   if (observing_root_window_)
@@ -1461,11 +1483,9 @@ void RenderWidgetHostViewAndroid::StartObservingRootWindow() {
   if (host_)
     host_->Send(new ViewMsg_SetBeginFramePaused(host_->GetRoutingID(), false));
   view_.GetWindowAndroid()->AddObserver(this);
-
-  // Clear existing vsync requests to allow a request to the new window.
-  uint32_t outstanding_vsync_requests = outstanding_vsync_requests_;
-  outstanding_vsync_requests_ = 0;
-  RequestVSyncUpdate(outstanding_vsync_requests);
+  // When using browser compositor, DelegatedFrameHostAndroid provides the BFS.
+  if (!using_browser_compositor_)
+    SetBeginFrameSource(view_.GetWindowAndroid()->GetBeginFrameSource());
 
   ui::WindowAndroidCompositor* compositor =
       view_.GetWindowAndroid()->GetCompositor();
@@ -1491,26 +1511,25 @@ void RenderWidgetHostViewAndroid::StopObservingRootWindow() {
   if (host_)
     host_->Send(new ViewMsg_SetBeginFramePaused(host_->GetRoutingID(), true));
   view_.GetWindowAndroid()->RemoveObserver(this);
+  if (!using_browser_compositor_)
+    SetBeginFrameSource(nullptr);
   // If the DFH has already been destroyed, it will have cleaned itself up.
   // This happens in some WebView cases.
   if (delegated_frame_host_)
     delegated_frame_host_->UnregisterFrameSinkHierarchy();
+  DCHECK_EQ(nullptr, begin_frame_source_);

[boliu, 2016/12/12 21:41:48]

nit: DCHECK(!begin_frame_source_)?

[Eric Seckler, 2016/12/13 10:03:55]

Done.

 }
 
-void RenderWidgetHostViewAndroid::SendBeginFrame(base::TimeTicks frame_time,
-                                                 base::TimeDelta vsync_period) {
+void RenderWidgetHostViewAndroid::SendBeginFrame(cc::BeginFrameArgs args) {
   TRACE_EVENT1("cc", "RenderWidgetHostViewAndroid::SendBeginFrame",
-               "frame_time_us", frame_time.ToInternalValue());
+               "frame_time_us", args.frame_time.ToInternalValue());
 
   // Synchronous compositor does not use deadline-based scheduling.
   // TODO(brianderson): Replace this hardcoded deadline after Android
-  // switches to Surfaces and the Browser's commit isn't in the critcal path.
-  base::TimeTicks deadline =
-      sync_compositor_ ? base::TimeTicks() : frame_time + (vsync_period * 0.6);
-  host_->Send(new ViewMsg_BeginFrame(
-      host_->GetRoutingID(),
-      cc::BeginFrameArgs::Create(BEGINFRAME_FROM_HERE, frame_time, deadline,
-                                 vsync_period, cc::BeginFrameArgs::NORMAL)));
+  // switches to Surfaces and the Browser's commit isn't in the critical path.
+  args.deadline = sync_compositor_ ? base::TimeTicks()
+  : args.frame_time + (args.interval * 0.6);
+  host_->Send(new ViewMsg_BeginFrame(host_->GetRoutingID(), args));
   if (sync_compositor_)
     sync_compositor_->DidSendBeginFrame(view_.GetWindowAndroid());
 }
@@ -1624,7 +1643,7 @@ InputEventAckState RenderWidgetHostViewAndroid::FilterInputEvent(
 
 void RenderWidgetHostViewAndroid::OnSetNeedsFlushInput() {
   TRACE_EVENT0("input", "RenderWidgetHostViewAndroid::OnSetNeedsFlushInput");
-  RequestVSyncUpdate(FLUSH_INPUT);
+  AddBeginFrameRequest(FLUSH_INPUT);
 }
 
 BrowserAccessibilityManager*
@@ -1807,7 +1826,7 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
     return;
   }
 
-  if (is_showing_)
+  if (is_showing_ && view_.GetWindowAndroid())
     StartObservingRootWindow();
 
   if (resize)
@@ -1888,10 +1907,12 @@ void RenderWidgetHostViewAndroid::OnAttachCompositor() {
   if (!overscroll_controller_)
     overscroll_controller_ = CreateOverscrollController(
         content_view_core_, ui::GetScaleFactorForNativeView(GetNativeView()));
-  ui::WindowAndroidCompositor* compositor =
-      view_.GetWindowAndroid()->GetCompositor();
-  delegated_frame_host_->RegisterFrameSinkHierarchy(
-      compositor->GetFrameSinkId());
+  if (observing_root_window_) {

[boliu, 2016/12/12 21:41:48]

this should not be needed, this still comes from the attached WindowAndroid, so
this should always be true here, hopefully

does this not hold anymore? that could indicate a bug (that I'm not seeing..)

[Eric Seckler, 2016/12/13 10:03:56]

I think this was a preexisting bug. What you say amounts to
|view_.GetWindowAndroid()| not being null. But we only observe the window if it
is actually showing. Thus, there are valid cases for |observing_root_window_| to
be false here. (And since Register/UnregisterFrameSinkHierarchy is coupled to
Start/StopObservingRootWindow, we should only RegisterFrameSinkHierarchy here if
|observing_root_window_| is true.)

Previously, this wouldn't have made much of a difference, I suppose. But now, we
receive the BFS from |delegated_frame_host_| if we RegisterFrameSinkHierarchy().
As a result, we wouldn't necessarily de-register it later because
StopObservingRootWindow() might early-out because |!observing_root_window_|,
which might leave our pointer dangling (and if I recall correctly, this happened
in some tests).

I can remove this again and see which (if any) tests fail if that's of interest
to you.

[boliu, 2016/12/13 18:07:47]

Keep it. Your analysis looks correct to me

+    ui::WindowAndroidCompositor* compositor =
+        view_.GetWindowAndroid()->GetCompositor();
+    delegated_frame_host_->RegisterFrameSinkHierarchy(
+        compositor->GetFrameSinkId());
+  }
 }
 
 void RenderWidgetHostViewAndroid::OnDetachCompositor() {
@@ -1902,28 +1923,41 @@ void RenderWidgetHostViewAndroid::OnDetachCompositor() {
   delegated_frame_host_->UnregisterFrameSinkHierarchy();
 }
 
-void RenderWidgetHostViewAndroid::OnVSync(base::TimeTicks frame_time,
-                                          base::TimeDelta vsync_period) {
-  TRACE_EVENT0("cc,benchmark", "RenderWidgetHostViewAndroid::OnVSync");
+void RenderWidgetHostViewAndroid::OnBeginFrame(const cc::BeginFrameArgs& args) {
+  TRACE_EVENT0("cc,benchmark", "RenderWidgetHostViewAndroid::OnBeginFrame");
   if (!host_)
     return;
 
-  if (outstanding_vsync_requests_ & FLUSH_INPUT) {
-    outstanding_vsync_requests_ &= ~FLUSH_INPUT;
+  // In sync mode, we disregard missed frame args to ensure that
+  // SynchronousCompositorBrowserFilter::SyncStateAfterVSync will be called
+  // during WindowAndroid::WindowBeginFrameSource::OnVSync() observer iteration.
+  if (sync_compositor_ && args.type == cc::BeginFrameArgs::MISSED)
+    return;
+
+  if (outstanding_begin_frame_requests_ & FLUSH_INPUT) {
+    ClearBeginFrameRequest(FLUSH_INPUT);
     host_->FlushInput();
   }
 
-  if (outstanding_vsync_requests_ & BEGIN_FRAME ||
-      outstanding_vsync_requests_ & PERSISTENT_BEGIN_FRAME) {
-    outstanding_vsync_requests_ &= ~BEGIN_FRAME;
-    SendBeginFrame(frame_time, vsync_period);
+  if ((outstanding_begin_frame_requests_ & BEGIN_FRAME) ||
+      (outstanding_begin_frame_requests_ & PERSISTENT_BEGIN_FRAME)) {
+    ClearBeginFrameRequest(BEGIN_FRAME);
+    SendBeginFrame(args);
   }
 
-  // This allows for SendBeginFrame and FlushInput to modify
-  // outstanding_vsync_requests.
-  uint32_t outstanding_vsync_requests = outstanding_vsync_requests_;
-  outstanding_vsync_requests_ = 0;
-  RequestVSyncUpdate(outstanding_vsync_requests);
+  last_begin_frame_args_ = args;
+}
+
+const cc::BeginFrameArgs& RenderWidgetHostViewAndroid::LastUsedBeginFrameArgs()
+    const {
+  return last_begin_frame_args_;
+}
+
+void RenderWidgetHostViewAndroid::OnBeginFrameSourcePausedChanged(bool paused) {
+  // The BeginFrameSources we listen to don't use this. For WebView, we signal
+  // the "paused" state to the RenderWidget when our window attaches/detaches,
+  // see |StartObservingRootWindow()| and |StopObservingRootWindow()|.
+  DCHECK(!paused);
 }
 
 void RenderWidgetHostViewAndroid::OnAnimate(base::TimeTicks begin_frame_time) {