[android] Make RWHVAndroid a BeginFrameObserver.

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 <utility>
 
@@ skipping 58 matching lines @@
 #include "content/public/common/content_switches.h"
 #include "gpu/command_buffer/client/gles2_implementation.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/config/gpu_driver_bug_workaround_type.h"
 #include "ipc/ipc_message_macros.h"
 #include "ipc/ipc_message_start.h"
 #include "skia/ext/image_operations.h"
 #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"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/events/blink/blink_event_util.h"
 #include "ui/events/blink/did_overscroll_params.h"
 #include "ui/events/blink/web_input_event_traits.h"
 #include "ui/events/gesture_detection/gesture_provider_config_helper.h"
@@ skipping 341 matching lines @@
       static_cast<RenderWidgetHostViewAndroid*>(widget->GetView())
           ->OnLostResources();
     }
   }
 }
 
 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),
       is_showing_overscroll_glow_(true),
       content_view_core_(nullptr),
       ime_adapter_android_(this),
       cached_background_color_(SK_ColorWHITE),
       last_compositor_frame_sink_id_(kUndefinedCompositorFrameSinkId),
       gesture_provider_(ui::GetGestureProviderConfig(
                             ui::GestureProviderConfigType::CURRENT_PLATFORM),
                         this),
       stylus_text_selector_(this),
       using_browser_compositor_(CompositorImpl::IsInitialized()),
       synchronous_compositor_client_(nullptr),
       frame_evictor_(new DelegatedFrameEvictor(this)),
       locks_on_frame_count_(0),
       observing_root_window_(false),
       weak_ptr_factory_(this) {
   // Set the layer which will hold the content layer for this view. The content
   // layer is managed by the DelegatedFrameHost.
   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);
   SetContentViewCore(content_view_core);
 
   if (GetTextInputManager())
     GetTextInputManager()->AddObserver(this);
 }
 
 RenderWidgetHostViewAndroid::~RenderWidgetHostViewAndroid() {
@@ skipping 317 matching lines @@
     delegated_frame_host_->UpdateBackgroundColor(color);
 
   if (content_view_core_)
     content_view_core_->OnBackgroundColorChanged(color);
 }
 
 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(
     const GURL& content_url, bool is_main_frame) {
   if (content_view_core_)
     content_view_core_->StartContentIntent(content_url, is_main_frame);
 }
 
 void RenderWidgetHostViewAndroid::OnSmartClipDataExtracted(
     const base::string16& text,
@@ skipping 29 matching lines @@
   if (!result.succeeded)
     return false;
 
   blink::WebTouchEvent web_event = ui::CreateWebTouchEventFromMotionEvent(
       event, result.moved_beyond_slop_region);
   ui::LatencyInfo latency_info(ui::SourceEventType::TOUCH);
   latency_info.AddLatencyNumber(ui::INPUT_EVENT_LATENCY_UI_COMPONENT, 0, 0);
   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;
 }
 
 bool RenderWidgetHostViewAndroid::OnTouchHandleEvent(
     const ui::MotionEvent& event) {
   return selection_controller_ &&
          selection_controller_->WillHandleTouchEvent(event);
 }
 
@@ skipping 505 matching lines @@
 
   view_.GetLayer()->SetHideLayerAndSubtree(false);
 
   frame_evictor_->SetVisible(true);
 
   if (overscroll_controller_)
     overscroll_controller_->Enable();
 
   host_->WasShown(ui::LatencyInfo());
 
-  if (content_view_core_) {
+  if (content_view_core_ && view_.GetWindowAndroid()) {
     StartObservingRootWindow();
-    RequestVSyncUpdate(BEGIN_FRAME);
+    AddBeginFrameRequest(BEGIN_FRAME);
   }
 }
 
 void RenderWidgetHostViewAndroid::HideInternal() {
   DCHECK(!is_showing_ || !is_window_activity_started_ || !is_window_visible_)
       << "Hide called when the widget should be shown.";
 
   // Only preserve the frontbuffer if the activity was stopped while the
   // window is still visible. This avoids visual artificts when transitioning
   // between activities.
@@ skipping 22 matching lines @@
   if (overscroll_controller_)
     overscroll_controller_->Disable();
 
   RunAckCallbacks();
 
   // Inform the renderer that we are being hidden so it can reduce its resource
   // utilization.
   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;
 
-  // 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) {
+  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);
+}
+
+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_)
     return;
 
   observing_root_window_ = true;
   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();
   if (compositor) {
     delegated_frame_host_->RegisterFrameSinkHierarchy(
         compositor->GetFrameSinkId());
   }
 }
 
 void RenderWidgetHostViewAndroid::StopObservingRootWindow() {
   if (!(view_.GetWindowAndroid())) {
     DCHECK(!observing_root_window_);
     return;
   }
 
   if (!observing_root_window_)
     return;
 
   // Reset window state variables to their defaults.
   is_window_activity_started_ = true;
   is_window_visible_ = true;
   observing_root_window_ = false;
   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());
 }
 
 bool RenderWidgetHostViewAndroid::Animate(base::TimeTicks frame_time) {
   bool needs_animate = false;
   if (overscroll_controller_ && is_showing_overscroll_glow_) {
     needs_animate |= overscroll_controller_->Animate(
         frame_time, content_view_core_->GetViewAndroid()->GetLayer());
   }
@@ skipping 93 matching lines @@
     if (shim && gpu_data &&
         gpu_data->IsDriverBugWorkaroundActive(gpu::WAKE_UP_GPU_BEFORE_DRAWING))
       shim->Send(new GpuMsg_WakeUpGpu);
   }
 
   return INPUT_EVENT_ACK_STATE_NOT_CONSUMED;
 }
 
 void RenderWidgetHostViewAndroid::OnSetNeedsFlushInput() {
   TRACE_EVENT0("input", "RenderWidgetHostViewAndroid::OnSetNeedsFlushInput");
-  RequestVSyncUpdate(FLUSH_INPUT);
+  AddBeginFrameRequest(FLUSH_INPUT);
 }
 
 BrowserAccessibilityManager*
     RenderWidgetHostViewAndroid::CreateBrowserAccessibilityManager(
         BrowserAccessibilityDelegate* delegate, bool for_root_frame) {
   base::android::ScopedJavaLocalRef<jobject> content_view_core_obj;
   if (for_root_frame && host_ && content_view_core_)
     content_view_core_obj = content_view_core_->GetJavaObject();
   return new BrowserAccessibilityManagerAndroid(
       content_view_core_obj,
@@ skipping 162 matching lines @@
     if (content_view_core_)
       obj = content_view_core_->GetJavaObject();
     manager->ToBrowserAccessibilityManagerAndroid()->SetContentViewCore(obj);
   }
 
   if (!content_view_core_) {
     sync_compositor_.reset();
     return;
   }
 
-  if (is_showing_)
+  if (is_showing_ && view_.GetWindowAndroid())
     StartObservingRootWindow();
 
   if (resize)
     WasResized();
 
   if (!selection_controller_)
     selection_controller_ = CreateSelectionController(this, content_view_core_);
 
   if (!overscroll_controller_ &&
       view_.GetWindowAndroid()->GetCompositor()) {
@@ skipping 60 matching lines @@
 void RenderWidgetHostViewAndroid::OnDetachedFromWindow() {
   StopObservingRootWindow();
   OnDetachCompositor();
 }
 
 void RenderWidgetHostViewAndroid::OnAttachCompositor() {
   DCHECK(content_view_core_);
   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() {
   DCHECK(content_view_core_);
   DCHECK(using_browser_compositor_);
   RunAckCallbacks();
   overscroll_controller_.reset();
   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) {
   if (Animate(begin_frame_time))
     SetNeedsAnimate();
 }
 
 void RenderWidgetHostViewAndroid::OnActivityStopped() {
   TRACE_EVENT0("browser", "RenderWidgetHostViewAndroid::OnActivityStopped");
   DCHECK(observing_root_window_);
@@ skipping 58 matching lines @@
     case ui::MotionEvent::ACTION_UP:
     case ui::MotionEvent::ACTION_POINTER_UP:
       UMA_HISTOGRAM_CUSTOM_COUNTS("Event.Latency.OS.TOUCH_RELEASED",
                                   delta.InMicroseconds(), 1, 1000000, 50);
     default:
       return;
   }
 }
 
 }  // namespace content