Android: Don't destroy LayerTreeHost when Surface goes away

content/browser/renderer_host/compositor_impl_android.cc

Index: content/browser/renderer_host/compositor_impl_android.cc
diff --git a/content/browser/renderer_host/compositor_impl_android.cc b/content/browser/renderer_host/compositor_impl_android.cc
index f4ae15040b2285108164a1c5076ed356efc13906..0b557092ad422db9c9f399396cebce3644f2c975 100644
--- a/content/browser/renderer_host/compositor_impl_android.cc
+++ b/content/browser/renderer_host/compositor_impl_android.cc
@@ -230,7 +230,6 @@ scoped_ptr<cc::SurfaceIdAllocator> CompositorImpl::CreateSurfaceIdAllocator() {
 CompositorImpl::CompositorImpl(CompositorClient* client,

[danakj, 2015/09/28 22:45:43]

Style-guidey comment: This class should probably have an Init method. This is a
very heavy constructor.

[no sievers, 2015/09/30 02:32:40]

ui/compositor/compositor.cc is even heavier though :)
(not that that's a good argument)

                                gfx::NativeWindow root_window)
     : root_layer_(cc::Layer::Create(Compositor::LayerSettings())),
-      resource_manager_(&ui_resource_provider_),
       surface_id_allocator_(GetSurfaceManager() ? CreateSurfaceIdAllocator()
                                                 : nullptr),
       has_transparent_background_(false),
@@ -252,6 +251,10 @@ CompositorImpl::CompositorImpl(CompositorClient* client,
   DCHECK(client);
   DCHECK(root_window);
   root_window->AttachCompositor(this);
+  CreateLayerTreeHost();
+  ui_resource_provider_.reset(new ui::UIResourceProvider(host_.get()));

[danakj, 2015/09/28 22:45:43]

I'm wondering if this class had an Init, and maybe the ui_resource_provider_ had
an init, if you wouldn't need to make these 2 things into scoped_ptrs and they
could be inline members still.

[no sievers, 2015/09/30 02:32:40]

Done.

+  resource_manager_.reset(
+      new ui::ResourceManagerImpl(ui_resource_provider_.get()));
 }
 
 CompositorImpl::~CompositorImpl() {
@@ -369,11 +372,11 @@ void CompositorImpl::Composite(CompositingTrigger trigger) {
 }
 
 ui::UIResourceProvider& CompositorImpl::GetUIResourceProvider() {
-  return ui_resource_provider_;
+  return *ui_resource_provider_;
 }
 
 ui::ResourceManager& CompositorImpl::GetResourceManager() {
-  return resource_manager_;
+  return *resource_manager_;
 }
 
 void CompositorImpl::SetRootLayer(scoped_refptr<cc::Layer> root_layer) {
@@ -440,8 +443,6 @@ void CompositorImpl::SetSurface(jobject surface) {
 void CompositorImpl::CreateLayerTreeHost() {
   DCHECK(!host_);
   DCHECK(!WillCompositeThisFrame());
-  needs_composite_ = false;
-  pending_swapbuffers_ = 0;
   cc::LayerTreeSettings settings;
   settings.renderer_settings.refresh_rate = 60.0;
   settings.renderer_settings.allow_antialiasing = false;
@@ -469,8 +470,7 @@ void CompositorImpl::CreateLayerTreeHost() {
   params.settings = &settings;
   host_ = cc::LayerTreeHost::CreateSingleThreaded(this, &params);
   host_->SetRootLayer(root_layer_);
-
-  host_->SetVisible(true);
+  host_->SetVisible(false);
   host_->SetLayerTreeHostClientReady();
   host_->SetViewportSize(size_);
   host_->set_has_transparent_background(has_transparent_background_);
@@ -500,8 +500,11 @@ void CompositorImpl::SetVisible(bool visible) {
     }
     if (WillComposite())
       CancelComposite();
-    ui_resource_provider_.SetLayerTreeHost(NULL);
-    host_.reset();
+    host_->SetVisible(false);
+    host_->ReleaseOutputSurface();
+    pending_swapbuffers_ = 0;

[danakj, 2015/09/28 22:45:43]

I don't really know what these are about, but these were in CreateLTH which set
visible true. Now they are in a set visible false path. Why is that right?

[no sievers, 2015/09/30 02:32:40]

We just have to make sure the accounting doesn't break. If the output surface
gets deleted we don't get anymore pending acks. Now one corner case is that
|pending_swapbuffers_ = kMaxSwapBuffers| and we destroy the OS (or previously
LTH). When we become visible again, we then might bail out of
CompositorImpl::Composite() forever below (line 339) before ever getting a
RequestNewOutputSurface() if we don't reset the pending acks (before
Composite()) somewhere.

I don't think it matters whether we reset it on becoming invisible or visible
again, since it shouldn't be able to change if there is no OutputSurface,
because DidPostSwapBuffers() wouldn't get called even if we tried to force a
composite. Thinking more about it, we probably also want to avoid any failed
attempt to create an OutputSurface while we are invisible and don't have an
underlying surface. (Also there is a DCHECK() for the surface id in
CreateOutputSurface()). Since we cancel |current_composite_task_| here, I can
actually put a DCHECK(!host_->visible()) inside Composite().

Does that add up? (Thanks for forcing me to think it through thoroughly.)

+    needs_composite_ = false;
+    ui_resource_provider_->OnUIResourcesEvicted();
     establish_gpu_channel_timeout_.Stop();
     output_surface_request_pending_ = false;
     display_client_.reset();
@@ -509,9 +512,9 @@ void CompositorImpl::SetVisible(bool visible) {
       current_composite_task_->Cancel();
       current_composite_task_.reset();
     }
-  } else if (!host_) {
-    CreateLayerTreeHost();
-    ui_resource_provider_.SetLayerTreeHost(host_.get());
+  } else {
+    host_->SetVisible(true);
+    SetNeedsComposite();
   }
 }
 
@@ -540,7 +543,7 @@ void CompositorImpl::SetHasTransparentBackground(bool flag) {
 }
 
 void CompositorImpl::SetNeedsComposite() {
-  if (!host_.get())
+  if (!host_->visible())
     return;
   DCHECK(!needs_composite_ || WillComposite());
 
@@ -686,7 +689,7 @@ void CompositorImpl::CreateOutputSurface() {
 
 void CompositorImpl::PopulateGpuCapabilities(
     gpu::Capabilities gpu_capabilities) {
-  ui_resource_provider_.SetSupportsETC1NonPowerOfTwo(
+  ui_resource_provider_->SetSupportsETC1NonPowerOfTwo(
       gpu_capabilities.texture_format_etc1_npot);
 }
 
@@ -699,10 +702,10 @@ void CompositorImpl::RemoveObserver(VSyncObserver* observer) {
 }
 
 void CompositorImpl::ScheduleComposite() {
-  DCHECK(!needs_composite_ || WillComposite());
-  if (ignore_schedule_composite_)
+  if (ignore_schedule_composite_ || !host_->visible())
     return;
 
+  DCHECK_IMPLIES(needs_composite_, WillComposite());
   needs_composite_ = true;
   // We currently expect layer tree invalidations at most once per frame
   // during normal operation and therefore try to composite immediately
@@ -711,7 +714,7 @@ void CompositorImpl::ScheduleComposite() {
 }
 
 void CompositorImpl::ScheduleAnimation() {
-  DCHECK(!needs_composite_ || WillComposite());
+  DCHECK_IMPLIES(needs_composite_, WillComposite());
   needs_animate_ = true;
 
   if (needs_composite_)
@@ -785,7 +788,7 @@ void CompositorImpl::OnVSync(base::TimeTicks frame_time,
 
 void CompositorImpl::SetNeedsAnimate() {
   needs_animate_ = true;
-  if (!host_)
+  if (!host_->visible())
     return;
 
   host_->SetNeedsAnimate();