Refactor client visibility handling

cc/trees/layer_tree_host_impl.cc

 // Copyright 2011 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 "cc/trees/layer_tree_host_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 139 matching lines @@
   bool scroll_on_main_thread = (scroll_thread == MAIN_THREAD);
   if (IsWheelBasedScroll(type)) {
     UMA_HISTOGRAM_BOOLEAN("Renderer4.CompositorWheelScrollUpdateThread",
                           scroll_on_main_thread);
   } else {
     UMA_HISTOGRAM_BOOLEAN("Renderer4.CompositorTouchScrollUpdateThread",
                           scroll_on_main_thread);
   }
 }
 
-// Calls SetClientVisible on the provided |context_provider| and handles
-// additional cache cleanup.
-void UpdateVisibilityForContextProvider(int client_id,
-                                        ContextProvider* context_provider,
-                                        bool is_visible) {
-  if (!context_provider)
-    return;
-  gpu::ContextSupport* context_support = context_provider->ContextSupport();
-
-  context_support->SetClientVisible(client_id, is_visible);
-  bool aggressively_free_resources = !context_support->AnyClientsVisible();
-  if (aggressively_free_resources) {
-    context_provider->DeleteCachedResources();
-  }
-  context_support->SetAggressivelyFreeResources(aggressively_free_resources);
-}
-
-// Same as UpdateVisibilityForContextProvider, except that the
-// |context_provider| is locked before being used.
-void LockAndUpdateVisibilityForContextProvider(
-    int client_id,
-    ContextProvider* context_provider,
-    bool is_visible) {
-  if (!context_provider)
-    return;
-  ContextProvider::ScopedContextLock hold(context_provider);
-  UpdateVisibilityForContextProvider(client_id, context_provider, is_visible);
-}
-
 }  // namespace
 
 DEFINE_SCOPED_UMA_HISTOGRAM_TIMER(PendingTreeDurationHistogramTimer,
                                   "Scheduling.%s.PendingTreeDuration");
 
 LayerTreeHostImpl::FrameData::FrameData()
     : render_surface_layer_list(nullptr),
       has_no_damage(false),
       may_contain_video(false) {}
 
@@ skipping 1090 matching lines @@
       ManagedMemoryPolicy::PriorityCutoffToTileMemoryLimitPolicy(
           visible_ ? policy.priority_cutoff_when_visible
                    : gpu::MemoryAllocation::CUTOFF_ALLOW_NOTHING);
   global_tile_state_.num_resources_limit = policy.num_resources_limit;
 
   if (global_tile_state_.hard_memory_limit_in_bytes > 0) {
     // If |global_tile_state_.hard_memory_limit_in_bytes| is greater than 0, we
     // are visible. Notify the worker context here. We handle becoming
     // invisible in NotifyAllTileTasksComplete to avoid interrupting running
     // work.
-    if (output_surface_) {
-      LockAndUpdateVisibilityForContextProvider(
-          id_, output_surface_->worker_context_provider(),
-          true /* is_visible */);
-    }
+    SetWorkerContextVisibility(true /* is_visible */);

[danakj, 2016/08/26 23:49:09]

i think the name of this bool is in the function name already

[ericrk, 2016/08/29 22:43:22]

Done.

 
     // If |global_tile_state_.hard_memory_limit_in_bytes| is greater than 0, we
     // allow the image decode controller to retain resources. We handle the
     // equal to 0 case in NotifyAllTileTasksComplete to avoid interrupting
     // running work.
     if (image_decode_controller_) {
       image_decode_controller_->SetShouldAggressivelyFreeResources(
           false /* aggressively_free_resources */);
     }
   }
@@ skipping 69 matching lines @@
   // completed. Now is a good time to free resources if necessary.
   if (global_tile_state_.hard_memory_limit_in_bytes == 0) {
     // Free image decode controller resources before notifying the worker
     // context of visibility change. This ensures that the imaged decode
     // controller has released all Skia refs at the time Skia's cleanup
     // executes (within worker context's cleanup).
     if (image_decode_controller_) {
       image_decode_controller_->SetShouldAggressivelyFreeResources(
           true /* aggressively_free_resources */);
     }
-    if (output_surface_) {
-      LockAndUpdateVisibilityForContextProvider(
-          id_, output_surface_->worker_context_provider(),
-          false /* is_visible */);
-    }
+    SetWorkerContextVisibility(false /* is_visible */);

[danakj, 2016/08/26 23:49:09]

dittos

[ericrk, 2016/08/29 22:43:23]

Done.

   }
 }
 
 void LayerTreeHostImpl::NotifyTileStateChanged(const Tile* tile) {
   TRACE_EVENT0("cc", "LayerTreeHostImpl::NotifyTileStateChanged");
 
   if (active_tree_) {
     LayerImpl* layer_impl =
         active_tree_->FindActiveTreeLayerById(tile->layer_id());
     if (layer_impl)
@@ skipping 717 matching lines @@
   if (visible_) {
     // TODO(crbug.com/469175): Replace with RequiresHighResToDraw.
     SetRequiresHighResToDraw();
   } else {
     EvictAllUIResources();
     // Call PrepareTiles to evict tiles when we become invisible.
     PrepareTiles();
   }
 
   // Update visibility for the compositor context provider.
-  if (output_surface_) {
-    UpdateVisibilityForContextProvider(id_, output_surface_->context_provider(),
-                                       visible);
-  }
+  SetMainContextVisibility(visible);
 }
 
 void LayerTreeHostImpl::SetNeedsOneBeginImplFrame() {
   // TODO(miletus): This is just the compositor-thread-side call to the
   // SwapPromiseMonitor to say something happened that may cause a swap in the
   // future. The name should not refer to SetNeedsRedraw but it does for now.
   NotifySwapPromiseMonitorsOfSetNeedsRedraw();
   client_->SetNeedsOneBeginImplFrameOnImplThread();
 }
 
@@ skipping 194 matching lines @@
   // Since we will create a new resource provider, we cannot continue to use
   // the old resources (i.e. render_surfaces and texture IDs). Clear them
   // before we destroy the old resource provider.
   ReleaseTreeResources();
 
   // Note: order is important here.
   renderer_ = nullptr;
   CleanUpTileManagerAndUIResources();
   resource_provider_ = nullptr;
 
+  // Release any context visibility before we destroy the OutputSurface.
+  SetMainContextVisibility(false /* is_visible */);

[danakj, 2016/08/26 23:49:09]

dittos

[ericrk, 2016/08/29 22:43:23]

Done.

+  SetWorkerContextVisibility(false /* is_visible */);
+
   // Detach from the old output surface and reset |output_surface_| pointer
   // as this surface is going to be destroyed independent of if binding the
   // new output surface succeeds or not.
   if (output_surface_) {
     output_surface_->DetachFromClient();
     output_surface_ = nullptr;
   }
 
   // We don't know if the next OutputSurface will support GPU rasterization.
   // Make sure to clear the flag so that we force a re-computation.
   use_gpu_rasterization_ = false;
 }
 
 bool LayerTreeHostImpl::InitializeRenderer(OutputSurface* output_surface) {
   TRACE_EVENT0("cc", "LayerTreeHostImpl::InitializeRenderer");
 
   ReleaseOutputSurface();
   if (!output_surface->BindToClient(this)) {
     // Avoid recreating tree resources because we might not have enough
     // information to do this yet (eg. we don't have a TileManager at this
     // point).
     return false;
   }
 
   output_surface_ = output_surface;

[danakj, 2016/08/26 23:49:08]

Should we set visibility on the compositor context here?

[ericrk, 2016/08/29 22:43:23]

Yeah, if we are visible_, and only for the main context - worker context is set
via memory policy change below, added a comment.

Added a unit test.

   resource_provider_ = base::MakeUnique<ResourceProvider>(
       output_surface_->context_provider(), shared_bitmap_manager_,
       gpu_memory_buffer_manager_,
       task_runner_provider_->blocking_main_thread_task_runner(),
       settings_.renderer_settings.highp_threshold_min,
       settings_.renderer_settings.texture_id_allocation_chunk_size,
       output_surface_->capabilities().delegated_sync_points_required,
       settings_.renderer_settings.use_gpu_memory_buffer_resources,
       settings_.renderer_settings.buffer_to_texture_target_map);
 
@@ skipping 1734 matching lines @@
   // Supported in threaded mode.
   return task_runner_provider_->HasImplThread();
 }
 
 bool LayerTreeHostImpl::CommitToActiveTree() const {
   // In single threaded mode we skip the pending tree and commit directly to the
   // active tree.
   return !task_runner_provider_->HasImplThread();
 }
 
+void LayerTreeHostImpl::SetMainContextVisibility(bool is_visible) {
+  if (is_visible == !!main_context_visibility_)

[danakj, 2016/08/26 23:49:09]

Is it actually possible to have this early out happen and its not a bug?

[ericrk, 2016/08/29 22:43:22]

Sure, only change I may need to make is not call it with false/true directly, so
I need to check at the call site... but this would help us catch any bugs, so
probably best to remove this and handle exceptions at the call site.

+    return;
+
+  if (!output_surface_)
+    return;
+
+  auto* main_context = output_surface_->context_provider();
+

[danakj, 2016/08/26 23:49:08]

nit: remove whitespace

[ericrk, 2016/08/29 22:43:23]

Done.

+  if (!main_context)
+    return;
+
+  if (is_visible) {
+    main_context_visibility_ =
+        main_context->CacheController()->ClientBecameVisible();
+  } else {
+    main_context->CacheController()->ClientBecameNotVisible(
+        std::move(main_context_visibility_));
+  }
+}
+
+void LayerTreeHostImpl::SetWorkerContextVisibility(bool is_visible) {
+  if (is_visible == !!worker_context_visibility_)

[danakj, 2016/08/26 23:49:08]

Same Q

[ericrk, 2016/08/29 22:43:23]

Worker context visibility is handled through both direct visibility changes and
changes to the managed memory policy (if memory limit is zeroed we set to
invisible)... I'm not too confident that the memory policy changes are as
controlled as the visibility changes...

We should look into this more - it seems pretty messy to conflate these two...
and I'm not sure when we'd need to use memory policy rather than direct
visibility.  I think Android webview may do this. Let's look at this as a
follow-up, and leave this check for now (in the worker context case).

+    return;
+
+  if (!output_surface_)
+    return;
+
+  auto* worker_context = output_surface_->worker_context_provider();
+

[danakj, 2016/08/26 23:49:09]

nit: remove whitespace

fwiw I try to think of empty lines separating paragraphs of code, not
sentences.. aka for topic changes

[ericrk, 2016/08/29 22:43:23]

Done.

+  if (!worker_context)
+    return;
+
+  ContextProvider::ScopedContextLock hold(worker_context);
+

[danakj, 2016/08/26 23:49:09]

nit: remove whitespace

[ericrk, 2016/08/29 22:43:22]

Done.

+  if (is_visible) {
+    worker_context_visibility_ =
+        worker_context->CacheController()->ClientBecameVisible();
+  } else {
+    worker_context->CacheController()->ClientBecameNotVisible(
+        std::move(worker_context_visibility_));
+  }
+}
+
 }  // namespace cc