Android OOP-D: Tear down display when going invisible

When Android goes invisible in OOP-D, it wasn't tearing down the
display, which can lead to GL issues as we continue to use GL
after the window (used to create the GL surface) is destroyed.

In order to tear down the display for Viz, we need to invalidate
our root frame sink ID. This change refactors things so that we
always invalidate the root frame sink ID on going invisible, and
re-register it on becoming visible. This allows both viz/non-viz
to share the same logic.

As registering/unregistering isn't doing much in non-viz case,
this doesn't add significant overhead there.

Bug: 863049
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I1589e402185fd9e2cdb007d3d8cd739f303ad48a
Reviewed-on: https://chromium-review.googlesource.com/1184376Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585664}

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc

@@ -140,6 +140,10 @@ void RootCompositorFrameSinkImpl::SetDisplayVSyncParameters(
     synthetic_begin_frame_source_->OnUpdateVSyncParameters(timebase, interval);
 }
 
+void RootCompositorFrameSinkImpl::ForceImmediateDrawAndSwapIfPossible() {
+  display_->ForceImmediateDrawAndSwapIfPossible();
+}
+
 #if defined(OS_ANDROID)
 void RootCompositorFrameSinkImpl::SetVSyncPaused(bool paused) {
   if (external_begin_frame_source_)

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.h

@@ -50,6 +50,7 @@ class RootCompositorFrameSinkImpl : public mojom::CompositorFrameSink,
   void SetAuthoritativeVSyncInterval(base::TimeDelta interval) override;
   void SetDisplayVSyncParameters(base::TimeTicks timebase,
                                  base::TimeDelta interval) override;
+  void ForceImmediateDrawAndSwapIfPossible() override;
 #if defined(OS_ANDROID)
   void SetVSyncPaused(bool paused) override;
 #endif

content/browser/renderer_host/compositor_impl_android.cc

@@ -702,9 +702,6 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
       enable_viz_(
           base::FeatureList::IsEnabled(features::kVizDisplayCompositor)),
       weak_factory_(this) {
-  GetHostFrameSinkManager()->RegisterFrameSinkId(frame_sink_id_, this);
-  GetHostFrameSinkManager()->SetFrameSinkDebugLabel(frame_sink_id_,
-                                                    "CompositorImpl");
   DCHECK(client);
 
   SetRootWindow(root_window);
@@ -719,7 +716,6 @@ CompositorImpl::~CompositorImpl() {
   DetachRootWindow();
   // Clean-up any surface references.
   SetSurface(NULL);
-  GetHostFrameSinkManager()->InvalidateFrameSinkId(frame_sink_id_);
 }
 
 void CompositorImpl::DetachRootWindow() {
@@ -857,28 +853,24 @@ void CompositorImpl::CreateLayerTreeHost() {
 
 void CompositorImpl::SetVisible(bool visible) {
   TRACE_EVENT1("cc", "CompositorImpl::SetVisible", "visible", visible);
+
   if (!visible) {
     DCHECK(host_->IsVisible());
-
-    // Make a best effort to try to complete pending readbacks.
-    // TODO(crbug.com/637035): Consider doing this in a better way,
-    // ideally with the guarantee of readbacks completing.
-    if (display_.get() && HavePendingReadbacks())
-      display_->ForceImmediateDrawAndSwapIfPossible();
-
+    // Tear down the display first, synchronously completing any pending
+    // draws/readbacks if poosible.
+    TearDownDisplayAndUnregisterRootFrameSink();
+    // Hide the LayerTreeHost and release its frame sink.
     host_->SetVisible(false);
     host_->ReleaseLayerTreeFrameSink();
     has_layer_tree_frame_sink_ = false;
     pending_frames_ = 0;
-    if (display_) {
-      GetFrameSinkManager()->UnregisterBeginFrameSource(
-          root_window_->GetBeginFrameSource());
-    }
-    display_.reset();
+    // Handle GPU visibility signals.
     GpuDataManagerImpl::GetInstance()->SetApplicationVisible(false);
     SendOnBackgroundedToGpuService();
     EnqueueLowEndBackgroundCleanup();
   } else {
+    DCHECK(!host_->IsVisible());
+    RegisterRootFrameSink();
     host_->SetVisible(true);
     has_submitted_frame_since_became_visible_ = false;
     if (layer_tree_frame_sink_request_pending_)
@@ -887,8 +879,46 @@ void CompositorImpl::SetVisible(bool visible) {
     SendOnForegroundedToGpuService();
     low_end_background_cleanup_task_.Cancel();
   }
-  if (display_private_)
-    display_private_->SetDisplayVisible(visible);
+}
+
+void CompositorImpl::TearDownDisplayAndUnregisterRootFrameSink() {
+  if (enable_viz_) {
+    // Make a best effort to try to complete pending readbacks.
+    // TODO(crbug.com/637035): Consider doing this in a better way,
+    // ideally with the guarantee of readbacks completing.
+    if (display_private_ && HavePendingReadbacks()) {
+      // Note that while this is not a Sync IPC, the call to
+      // InvalidateFrameSinkId below will end up triggering a sync call to
+      // FrameSinkManager::DestroyCompositorFrameSink, as this is the root
+      // frame sink. Because |display_private_| is an associated interface to
+      // FrameSinkManager, this subsequent sync call will ensure ordered
+      // execution of this call.
+      display_private_->ForceImmediateDrawAndSwapIfPossible();
+    }
+
+    GetHostFrameSinkManager()->InvalidateFrameSinkId(frame_sink_id_);
+    display_private_.reset();
+  } else {
+    // Make a best effort to try to complete pending readbacks.
+    // TODO(crbug.com/637035): Consider doing this in a better way,
+    // ideally with the guarantee of readbacks completing.
+    if (display_ && HavePendingReadbacks())
+      display_->ForceImmediateDrawAndSwapIfPossible();
+
+    if (display_) {
+      GetFrameSinkManager()->UnregisterBeginFrameSource(
+          root_window_->GetBeginFrameSource());
+    }
+
+    GetHostFrameSinkManager()->InvalidateFrameSinkId(frame_sink_id_);
+    display_.reset();
+  }
+}
+
+void CompositorImpl::RegisterRootFrameSink() {
+  GetHostFrameSinkManager()->RegisterFrameSinkId(frame_sink_id_, this);
+  GetHostFrameSinkManager()->SetFrameSinkDebugLabel(frame_sink_id_,
+                                                    "CompositorImpl");
 }
 
 void CompositorImpl::SetWindowBounds(const gfx::Size& size) {

content/browser/renderer_host/compositor_impl_android.h

@@ -190,6 +190,13 @@ class CONTENT_EXPORT CompositorImpl
   // returns an empty surface.
   viz::LocalSurfaceId GenerateLocalSurfaceId() const;
 
+  // Tears down the display for both Viz and non-Viz, unregistering the root
+  // frame sink ID in the process.
+  void TearDownDisplayAndUnregisterRootFrameSink();
+
+  // Registers the root frame sink ID.
+  void RegisterRootFrameSink();
+
   // Viz specific functions:
   void InitializeVizLayerTreeFrameSink(
       scoped_refptr<ui::ContextProviderCommandBuffer> context_provider);

services/viz/privileged/interfaces/compositing/display_private.mojom

@@ -44,6 +44,14 @@ interface DisplayPrivate {
     mojo_base.mojom.TimeTicks timebase,
     mojo_base.mojom.TimeDelta interval);
 
+  // Attempts to immediately draw and swap a frame if possible. Note that this
+  // is not a sync IPC, as all current uses are during tear down of the root
+  // compositor frame sink and are immediately followed by a call to
+  // FrameSinkManager::DestroyCompositorFrameSink which is an associated sync
+  // IPC. If this function is needed in cases where ordering isn't guaranteed
+  // by other means, it should be made sync.
+  ForceImmediateDrawAndSwapIfPossible();
+
   // Notifies the display to pause VSync signals. Used for VR.
   [EnableIf=is_android]
   SetVSyncPaused(bool paused);