Reland "Limit access to gpu thread in DisplayResourceProvider"

This is a reland of d60e79d1

The problem was that resources were never returned because
ScopedBatchReturnResources was in outer scope than ScopedAllowGpuAccess
so the state when we don't batch already, but still have access to gpu
thread didn't exist.

Solution to merge ScopedBatchReturnResources and ScopedAllowGpuAccess
to make sure everything happens in right order.

Original change's description:
> Limit access to gpu thread in DisplayResourceProvider
>
> This CL makes sure that DisplayResourceProvider return resources only
> when GPU Thread is available to make sure ReleaseImageContexts will
> complete in finite time.
>
> This doesn't change anything for Chrome where GPU Thread is always
> available, but on webview RenderThread that used for gpu access during
> composition is available only during specific times (init, dtor,
> DrawAndSwap)
>
> Bug: 1068716
>
> Change-Id: I8fa482ab7399d6ce7639c1d76187be86ce152059
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2138551
> Commit-Queue: Vasiliy Telezhnikov <vasilyt@chromium.org>
> Reviewed-by: Jonathan Backer <backer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#757967}

Bug: 1068716
Change-Id: Idf531b9ee9e10057b571725a256d2af09efbea69
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2149377Reviewed-by: Jonathan Backer <backer@chromium.org>
Commit-Queue: Vasiliy Telezhnikov <vasilyt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759672}

android_webview/browser/gfx/hardware_renderer_viz.cc

@@ -124,6 +124,7 @@ HardwareRendererViz::OnViz::OnViz(
                        output_surface_provider->enable_shared_image());
 
   display_->SetVisible(true);
+  display_->DisableGPUAccessByDefault();
 }
 
 HardwareRendererViz::OnViz::~OnViz() {

components/viz/service/display/display.cc

@@ -273,6 +273,9 @@ Display::~Display() {
   allow_schedule_gpu_task_during_destruction_.reset(
       new gpu::ScopedAllowScheduleGpuTask);
 #endif
+  if (resource_provider_) {
+    resource_provider_->SetAllowAccessToGPUThread(true);
+  }
 #if defined(OS_ANDROID)
   // In certain cases, drivers hang when tearing down the display. Finishing
   // before teardown appears to address this. As we're during display teardown,
@@ -584,7 +587,8 @@ bool Display::DrawAndSwap(base::TimeTicks expected_display_time) {
   // GL commands for deleting resources to after the draw, and prevents context
   // switching because the scheduler knows sync token dependencies at that time.
   DisplayResourceProvider::ScopedBatchReturnResources returner(
-      resource_provider_.get());
+      resource_provider_.get(), /*allow_access_to_gpu_thread=*/true);
+
   base::ElapsedTimer aggregate_timer;
   aggregate_timer.Begin();
   CompositorFrame frame;
@@ -1139,4 +1143,9 @@ base::ScopedClosureRunner Display::GetCacheBackBufferCb() {
   return output_surface_->GetCacheBackBufferCb();
 }
 
+void Display::DisableGPUAccessByDefault() {
+  DCHECK(resource_provider_);
+  resource_provider_->SetAllowAccessToGPUThread(false);
+}
+
 }  // namespace viz

components/viz/service/display/display.h

@@ -115,6 +115,10 @@ class VIZ_SERVICE_EXPORT Display : public DisplaySchedulerClient,
   void SetVisible(bool visible);
   void Resize(const gfx::Size& new_size);
 
+  // This disallows resource provider to access GPU thread to unlock resources
+  // outside of Initialize, DrawAndSwap and dtor.
+  void DisableGPUAccessByDefault();
+
   // Stop drawing until Resize() is called with a new size. If the display
   // hasn't drawn a frame at the current size *and* it's possible to immediately
   // draw then this will run DrawAndSwap() first.

components/viz/service/display/display_resource_provider.cc

@@ -22,6 +22,7 @@
 #include "gpu/command_buffer/client/context_support.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/command_buffer/common/shared_image_trace_utils.h"
+#include "gpu/ipc/scheduler_sequence.h"
 #include "third_party/skia/include/gpu/GrBackendSurface.h"
 #include "ui/gl/trace_util.h"
 
@@ -36,6 +37,19 @@ base::AtomicSequenceNumber g_next_display_resource_provider_tracing_id;
 
 }  // namespace
 
+class ScopedAllowGpuAccessForDisplayResourceProvider {
+ public:
+  ~ScopedAllowGpuAccessForDisplayResourceProvider() = default;
+
+  explicit ScopedAllowGpuAccessForDisplayResourceProvider(
+      DisplayResourceProvider* provider) {
+    DCHECK(provider->can_access_gpu_thread_);
+  }
+
+ private:
+  gpu::ScopedAllowScheduleGpuTask allow_gpu_;
+};
+
 class ScopedSetActiveTexture {
  public:
   ScopedSetActiveTexture(GLES2Interface* gl, GLenum unit)
@@ -622,8 +636,11 @@ void DisplayResourceProvider::DeleteAndReturnUnusedResourcesToChild(
   if (unused.empty() && !child_info->marked_for_deletion)
     return;
 
-  // Store unused resources while batching is enabled.
-  if (batch_return_resources_lock_count_ > 0) {
+  // Store unused resources while batching is enabled or we can't access gpu
+  // thread right now.
+  // TODO(vasilyt): Technically we need to delay only resources with
+  // |image_context|.
+  if (batch_return_resources_lock_count_ > 0 || !can_access_gpu_thread_) {
     int child_id = child_it->first;
     // Ensure that we have an entry in |batched_returning_resources_| for child
     // even if |unused| is empty, in case child is marked for deletion.
@@ -729,6 +746,7 @@ void DisplayResourceProvider::DeleteAndReturnUnusedResourcesToChild(
 
   if (external_use_client_) {
     if (!image_contexts_to_return.empty()) {
+      ScopedAllowGpuAccessForDisplayResourceProvider allow_gpu(this);
       gpu::SyncToken sync_token = external_use_client_->ReleaseImageContexts(
           std::move(image_contexts_to_return));
       for (auto* resource : external_used_resources) {
@@ -782,6 +800,27 @@ void DisplayResourceProvider::DestroyChildInternal(ChildMap::iterator it,
   DeleteAndReturnUnusedResourcesToChild(it, style, resources_for_child);
 }
 
+void DisplayResourceProvider::TryFlushBatchedResources() {
+  if (batch_return_resources_lock_count_ == 0 && can_access_gpu_thread_) {
+    for (auto& child_resources_kv : batched_returning_resources_) {
+      auto child_it = children_.find(child_resources_kv.first);
+
+      // Remove duplicates from child's unused resources.  Duplicates are
+      // possible when batching is enabled because resources are saved in
+      // |batched_returning_resources_| for removal, and not removed from the
+      // child's |child_to_parent_map|, so the same set of resources can be
+      // saved again using DeclareUsedResourcesForChild() or DestroyChild().
+      auto& unused_resources = child_resources_kv.second;
+      std::sort(unused_resources.begin(), unused_resources.end());
+      auto last = std::unique(unused_resources.begin(), unused_resources.end());
+      unused_resources.erase(last, unused_resources.end());
+
+      DeleteAndReturnUnusedResourcesToChild(child_it, NORMAL, unused_resources);
+    }
+    batched_returning_resources_.clear();
+  }
+}
+
 void DisplayResourceProvider::SetBatchReturnResources(bool batch) {
   if (batch) {
     DCHECK_GE(batch_return_resources_lock_count_, 0);
@@ -796,28 +835,18 @@ void DisplayResourceProvider::SetBatchReturnResources(bool batch) {
     if (batch_return_resources_lock_count_ == 0) {
       DCHECK(scoped_batch_read_access_);
       scoped_batch_read_access_.reset();
-      for (auto& child_resources_kv : batched_returning_resources_) {
-        auto child_it = children_.find(child_resources_kv.first);
-
-        // Remove duplicates from child's unused resources.  Duplicates are
-        // possible when batching is enabled because resources are saved in
-        // |batched_returning_resources_| for removal, and not removed from the
-        // child's |child_to_parent_map|, so the same set of resources can be
-        // saved again using DeclareUsedResourcesForChild() or DestroyChild().
-        auto& unused_resources = child_resources_kv.second;
-        std::sort(unused_resources.begin(), unused_resources.end());
-        auto last =
-            std::unique(unused_resources.begin(), unused_resources.end());
-        unused_resources.erase(last, unused_resources.end());
-
-        DeleteAndReturnUnusedResourcesToChild(child_it, NORMAL,
-                                              unused_resources);
-      }
-      batched_returning_resources_.clear();
+      TryFlushBatchedResources();
     }
   }
 }
 
+void DisplayResourceProvider::SetAllowAccessToGPUThread(bool allow) {
+  can_access_gpu_thread_ = allow;
+  if (allow) {
+    TryFlushBatchedResources();
+  }
+}
+
 DisplayResourceProvider::ScopedReadLockGL::ScopedReadLockGL(
     DisplayResourceProvider* resource_provider,
     ResourceId resource_id)
@@ -1057,14 +1086,21 @@ void DisplayResourceProvider::SynchronousFence::Synchronize() {
 }
 
 DisplayResourceProvider::ScopedBatchReturnResources::ScopedBatchReturnResources(
-    DisplayResourceProvider* resource_provider)
-    : resource_provider_(resource_provider) {
+    DisplayResourceProvider* resource_provider,
+    bool allow_access_to_gpu_thread)
+    : resource_provider_(resource_provider),
+      was_access_to_gpu_thread_allowed_(
+          resource_provider_->can_access_gpu_thread_) {
   resource_provider_->SetBatchReturnResources(true);
+  if (allow_access_to_gpu_thread)
+    resource_provider_->SetAllowAccessToGPUThread(true);
 }
 
 DisplayResourceProvider::ScopedBatchReturnResources::
     ~ScopedBatchReturnResources() {
   resource_provider_->SetBatchReturnResources(false);
+  resource_provider_->SetAllowAccessToGPUThread(
+      was_access_to_gpu_thread_allowed_);
 }
 
 DisplayResourceProvider::Child::Child() = default;

components/viz/service/display/display_resource_provider.h

@@ -45,6 +45,7 @@ class GLES2Interface;
 namespace viz {
 
 class ContextProvider;
+class ScopedAllowGpuAccessForDisplayResourceProvider;
 class SharedBitmapManager;
 
 // This class provides abstractions for receiving and using resources from other
@@ -272,11 +273,13 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
   class VIZ_SERVICE_EXPORT ScopedBatchReturnResources {
    public:
     explicit ScopedBatchReturnResources(
-        DisplayResourceProvider* resource_provider);
+        DisplayResourceProvider* resource_provider,
+        bool allow_access_to_gpu_thread = false);
     ~ScopedBatchReturnResources();
 
    private:
     DisplayResourceProvider* const resource_provider_;
+    const bool was_access_to_gpu_thread_allowed_;
   };
 
   class VIZ_SERVICE_EXPORT SynchronousFence : public ResourceFence {
@@ -341,7 +344,12 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
   // Returns the mailbox corresponding to a resource id.
   gpu::Mailbox GetMailbox(int resource_id);
 
+  // Sets if the GPU thread is available (it always is for Chrome, but for
+  // WebView it happens only when Android calls us on RenderThread.
+  void SetAllowAccessToGPUThread(bool allow);
+
  private:
+  friend class ScopedAllowGpuAccessForDisplayResourceProvider;
   enum DeleteStyle {
     NORMAL,
     FOR_SHUTDOWN,
@@ -533,6 +541,7 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
   void DestroyChildInternal(ChildMap::iterator it, DeleteStyle style);
 
   void SetBatchReturnResources(bool aggregate);
+  void TryFlushBatchedResources();
 
   THREAD_CHECKER(thread_checker_);
   const Mode mode_;
@@ -569,6 +578,11 @@ class VIZ_SERVICE_EXPORT DisplayResourceProvider
 
   bool enable_shared_images_;
   std::unique_ptr<ScopedBatchReadAccess> scoped_batch_read_access_;
+
+  // Indicates that gpu thread is available and calls like
+  // ReleaseImageContexts() are expected to finish in finite time. It's always
+  // true for Chrome, but on WebView we need to have access to RenderThread.
+  bool can_access_gpu_thread_ = true;
 };
 
 }  // namespace viz

components/viz/service/display/display_resource_provider_unittest.cc

@@ -326,6 +326,104 @@ TEST_P(DisplayResourceProviderTest, LockForExternalUse) {
   child_resource_provider_->RemoveImportedResource(id1);
 }
 
+TEST_P(DisplayResourceProviderTest, LockForExternalUseWebView) {
+  // TODO(penghuang): consider supporting SW mode.
+  if (!use_gpu())
+    return;
+
+  gpu::SyncToken sync_token1(gpu::CommandBufferNamespace::GPU_IO,
+                             gpu::CommandBufferId::FromUnsafeValue(0x123),
+                             0x42);
+  auto mailbox = gpu::Mailbox::Generate();
+  constexpr gfx::Size size(64, 64);
+  TransferableResource gl_resource = TransferableResource::MakeGL(
+      mailbox, GL_LINEAR, GL_TEXTURE_2D, sync_token1, size,
+      false /* is_overlay_candidate */);
+  ResourceId id1 = child_resource_provider_->ImportResource(
+      gl_resource, SingleReleaseCallback::Create(base::DoNothing()));
+  std::vector<ReturnedResource> returned_to_child;
+  int child_id =
+      resource_provider_->CreateChild(GetReturnCallback(&returned_to_child));
+
+  // Transfer some resources to the parent.
+  std::vector<TransferableResource> list;
+  child_resource_provider_->PrepareSendToParent(
+      {id1}, &list,
+      static_cast<RasterContextProvider*>(child_context_provider_.get()));
+  ASSERT_EQ(1u, list.size());
+  EXPECT_TRUE(child_resource_provider_->InUseByConsumer(id1));
+
+  resource_provider_->ReceiveFromChild(child_id, list);
+
+  // In DisplayResourceProvider's namespace, use the mapped resource id.
+  std::unordered_map<ResourceId, ResourceId> resource_map =
+      resource_provider_->GetChildToParentMap(child_id);
+
+  unsigned parent_id = resource_map[list.front().id];
+
+  auto owned_image_context = std::make_unique<ExternalUseClient::ImageContext>(
+      gpu::MailboxHolder(mailbox, sync_token1, GL_TEXTURE_2D), size, RGBA_8888,
+      /*ycbcr_info=*/base::nullopt, /*color_space=*/nullptr);
+  auto* image_context = owned_image_context.get();
+
+  testing::StrictMock<MockExternalUseClient> client;
+  DisplayResourceProvider::LockSetForExternalUse lock_set(
+      resource_provider_.get(), &client);
+  gpu::MailboxHolder holder;
+  EXPECT_CALL(client, CreateImageContext(_, _, _, _, _))
+      .WillOnce(DoAll(SaveArg<0>(&holder),
+                      Return(ByMove(std::move(owned_image_context)))));
+
+  ExternalUseClient::ImageContext* locked_image_context =
+      lock_set.LockResource(parent_id, /*is_video_plane=*/false);
+  EXPECT_EQ(image_context, locked_image_context);
+  ASSERT_EQ(holder.mailbox, mailbox);
+  ASSERT_TRUE(holder.sync_token.HasData());
+
+  // Don't release while locked.
+  EXPECT_CALL(client, ReleaseImageContexts(_)).Times(0);
+  // Return the resources back to the child. Nothing should happen because
+  // of the resource lock.
+  resource_provider_->DeclareUsedResourcesFromChild(child_id, ResourceIdSet());
+  // The resource should not be returned due to the external use lock.
+  EXPECT_EQ(0u, returned_to_child.size());
+
+  // Disable access to gpu thread.
+  resource_provider_->SetAllowAccessToGPUThread(false);
+
+  gpu::SyncToken sync_token2(gpu::CommandBufferNamespace::GPU_IO,
+                             gpu::CommandBufferId::FromUnsafeValue(0x234),
+                             0x456);
+  sync_token2.SetVerifyFlush();
+
+  gpu::SyncToken sync_token3(gpu::CommandBufferNamespace::GPU_IO,
+                             gpu::CommandBufferId::FromUnsafeValue(0x234),
+                             0x567);
+  sync_token3.SetVerifyFlush();
+
+  // Without GPU thread access no ReleaseImageContexts() should happen
+  EXPECT_CALL(client, ReleaseImageContexts(_)).Times(0);
+  // Unlock resources
+  lock_set.UnlockResources(sync_token2);
+  // Resources should not be returned because we can't unlock them on GPU
+  // thread.
+  EXPECT_EQ(0u, returned_to_child.size());
+
+  // We will get a second release of |parent_id| now that we've released our
+  // external lock and have access to GPU thread.
+  EXPECT_CALL(client, ReleaseImageContexts(
+                          testing::ElementsAre(SamePtr(locked_image_context))))
+      .WillOnce(Return(sync_token3));
+  // Enable access to GPU Thread
+  resource_provider_->SetAllowAccessToGPUThread(true);
+
+  // The resource should be returned after the lock is released.
+  EXPECT_EQ(1u, returned_to_child.size());
+  EXPECT_EQ(sync_token3, returned_to_child[0].sync_token);
+  child_resource_provider_->ReceiveReturnsFromParent(returned_to_child);
+  child_resource_provider_->RemoveImportedResource(id1);
+}
+
 TEST_P(DisplayResourceProviderTest, ReadLockCountStopsReturnToChildOrDelete) {
   if (!use_gpu())
     return;

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -372,7 +372,7 @@ gpu::SyncToken SkiaOutputSurfaceImpl::ReleaseImageContexts(
       base::BindOnce(&SkiaOutputSurfaceImplOnGpu::ReleaseImageContexts,
                      base::Unretained(impl_on_gpu_.get()),
                      std::move(image_contexts), sync_fence_release_);
-  gpu_task_scheduler_->ScheduleOrRetainGpuTask(std::move(callback), {});
+  gpu_task_scheduler_->ScheduleGpuTask(std::move(callback), {});
   return sync_token;
 }
 

gpu/ipc/scheduler_sequence.h

@@ -18,6 +18,7 @@
 
 namespace viz {
 class Display;
+class ScopedAllowGpuAccessForDisplayResourceProvider;
 class OutputSurfaceProviderImpl;
 class OverlayProcessorAndroid;
 }  // namespace viz
@@ -35,6 +36,7 @@ class GL_IN_PROCESS_CONTEXT_EXPORT ScopedAllowScheduleGpuTask {
   // Only add more friend declarations for classes that Android WebView is
   // guaranteed to be able to support. Talk to boliu@ if in doubt.
   friend class viz::Display;
+  friend class viz::ScopedAllowGpuAccessForDisplayResourceProvider;
   friend class viz::OutputSurfaceProviderImpl;
   // Overlay is not supported for WebView. However the initialization and
   // destruction of OverlayProcessor requires posting task to gpu thread, which