Reland "viz: RGBA_TEXTURE copy requests in SkiaRenderer"

The original CL was reverted [1] because it broke a few tests
running on SkiaRenderer Vk (namely Pixel_Video_Context_Loss_MP4).
This was because we only ensure the GL context is current in the
ImplOnGpu::InitializeForGL path, but SharedImageFactory always
creates a GL backing factory which in turn invokes a few GL API
calls in its constructor.

This CL fixes the issue by always making the GL context current
regardless of Skia backend.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=1111312

Bug: 1111312, 1046788, 971257, 1098435
Change-Id: Ifa5d91dc6d96d2ecc4144be95221cd188095f0e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333991
Commit-Queue: Brian Ho <hob@chromium.org>
Reviewed-by: Jonathan Backer <backer@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795118}

cc/trees/layer_tree_host_pixeltest_readback.cc

@@ -420,12 +420,7 @@ ReadbackTestConfig const kTestConfigs[] = {
     ReadbackTestConfig{TestRendererType::kSoftware, TestReadBackType::kBitmap},
     ReadbackTestConfig{TestRendererType::kGL, TestReadBackType::kTexture},
     ReadbackTestConfig{TestRendererType::kGL, TestReadBackType::kBitmap},
-    // TODO(crbug.com/1046788): The skia readback path doesn't support
-    // RGBA_TEXTURE readback requests yet. Don't run these tests on platforms
-    // that have UseSkiaForGLReadback enabled by default.
-    //
-    // ReadbackTestConfig{TestRendererType::kSkiaGL,
-    //                    TestReadBackType::kTexture},
+    ReadbackTestConfig{TestRendererType::kSkiaGL, TestReadBackType::kTexture},
     ReadbackTestConfig{TestRendererType::kSkiaGL, TestReadBackType::kBitmap},
 #if defined(ENABLE_CC_VULKAN_TESTS)
     ReadbackTestConfig{TestRendererType::kSkiaVk, TestReadBackType::kBitmap},
@@ -445,12 +440,7 @@ ReadbackTestConfig const kMaybeVulkanTestConfigs[] = {
     ReadbackTestConfig{TestRendererType::kSoftware, TestReadBackType::kBitmap},
     ReadbackTestConfig{TestRendererType::kGL, TestReadBackType::kTexture},
     ReadbackTestConfig{TestRendererType::kGL, TestReadBackType::kBitmap},
-    // TODO(crbug.com/1046788): The skia readback path doesn't support
-    // RGBA_TEXTURE readback requests yet. Don't run these tests on platforms
-    // that have UseSkiaForGLReadback enabled by default.
-    //
-    // ReadbackTestConfig{TestRendererType::kSkiaGL,
-    //                    TestReadBackType::kTexture},
+    ReadbackTestConfig{TestRendererType::kSkiaGL, TestReadBackType::kTexture},
     ReadbackTestConfig{TestRendererType::kSkiaGL, TestReadBackType::kBitmap},
 #if defined(ENABLE_CC_VULKAN_TESTS) && !defined(THREAD_SANITIZER) && \
     !defined(MEMORY_SANITIZER)

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.h

@@ -26,6 +26,7 @@
 #include "gpu/command_buffer/common/mailbox.h"
 #include "gpu/command_buffer/common/sync_token.h"
 #include "gpu/command_buffer/service/shared_context_state.h"
+#include "gpu/command_buffer/service/shared_image_representation.h"
 #include "gpu/command_buffer/service/sync_point_manager.h"
 #include "gpu/ipc/service/context_url.h"
 #include "gpu/ipc/service/display_context.h"
@@ -45,6 +46,7 @@ class GLSurface;
 
 namespace gpu {
 class SharedImageRepresentationFactory;
+class SharedImageFactory;
 class SyncPointClientState;
 }
 
@@ -204,7 +206,7 @@ class SkiaOutputSurfaceImplOnGpu
     num_readbacks_pending_--;
   }
 
-  gpu::MemoryTracker* GetMemoryTracker() { return memory_tracker_.get(); }
+  gpu::MemoryTracker* GetMemoryTracker() { return memory_tracker_; }
 
  private:
   class OffscreenSurface;
@@ -226,6 +228,12 @@ class SkiaOutputSurfaceImplOnGpu
   bool MakeCurrent(bool need_fbo0);
   void MarkContextLost(ContextLostReason reason);
 
+  void DestroySharedImageOnImplThread(
+      std::unique_ptr<gpu::SharedImageRepresentationSkia> representation,
+      scoped_refptr<gpu::SharedContextState> context_state,
+      const gpu::SyncToken& sync_token,
+      bool is_lost);
+
   void PullTextureUpdates(std::vector<gpu::SyncToken> sync_token);
 
   void ReleaseFenceSyncAndPushTextureUpdates(uint64_t sync_fence_release);
@@ -277,9 +285,10 @@ class SkiaOutputSurfaceImplOnGpu
   SkiaOutputSurfaceDependency* const dependency_;
   scoped_refptr<gpu::gles2::FeatureInfo> feature_info_;
   scoped_refptr<gpu::SyncPointClientState> sync_point_client_state_;
-  std::unique_ptr<gpu::MemoryTracker> memory_tracker_;
+  gpu::MemoryTracker* memory_tracker_;
   std::unique_ptr<gpu::SharedImageRepresentationFactory>
       shared_image_representation_factory_;
+  std::unique_ptr<gpu::SharedImageFactory> shared_image_factory_;
   VulkanContextProvider* const vulkan_context_provider_;
   DawnContextProvider* const dawn_context_provider_;
   const RendererSettings renderer_settings_;

gpu/command_buffer/service/shared_context_state.cc

@@ -78,15 +78,15 @@ void SharedContextState::compileError(const char* shader, const char* errors) {
   }
 }
 
-SharedContextState::MemoryTracker::MemoryTracker(
+SharedContextState::MemoryTrackerObserver::MemoryTrackerObserver(
     base::WeakPtr<gpu::MemoryTracker::Observer> peak_memory_monitor)
     : peak_memory_monitor_(peak_memory_monitor) {}
 
-SharedContextState::MemoryTracker::~MemoryTracker() {
+SharedContextState::MemoryTrackerObserver::~MemoryTrackerObserver() {
   DCHECK(!size_);
 }
 
-void SharedContextState::MemoryTracker::OnMemoryAllocatedChange(
+void SharedContextState::MemoryTrackerObserver::OnMemoryAllocatedChange(
     CommandBufferId id,
     uint64_t old_size,
     uint64_t new_size,
@@ -100,6 +100,45 @@ void SharedContextState::MemoryTracker::OnMemoryAllocatedChange(
   }
 }
 
+base::AtomicSequenceNumber g_next_command_buffer_id;
+
+SharedContextState::MemoryTracker::MemoryTracker(Observer* observer)
+    : command_buffer_id_(gpu::CommandBufferId::FromUnsafeValue(
+          g_next_command_buffer_id.GetNext() + 1)),
+      client_tracing_id_(base::trace_event::MemoryDumpManager::GetInstance()
+                             ->GetTracingProcessId()),
+      observer_(observer) {}
+
+SharedContextState::MemoryTracker::~MemoryTracker() {
+  DCHECK(!size_);
+}
+
+void SharedContextState::MemoryTracker::TrackMemoryAllocatedChange(
+    int64_t delta) {
+  DCHECK(delta >= 0 || size_ >= static_cast<uint64_t>(-delta));
+  uint64_t old_size = size_;
+  size_ += delta;
+  DCHECK(observer_);
+  observer_->OnMemoryAllocatedChange(command_buffer_id_, old_size, size_,
+                                     gpu::GpuPeakMemoryAllocationSource::SKIA);
+}
+
+uint64_t SharedContextState::MemoryTracker::GetSize() const {
+  return size_;
+}
+
+uint64_t SharedContextState::MemoryTracker::ClientTracingId() const {
+  return client_tracing_id_;
+}
+
+int SharedContextState::MemoryTracker::ClientId() const {
+  return gpu::ChannelIdFromCommandBufferId(command_buffer_id_);
+}
+
+uint64_t SharedContextState::MemoryTracker::ContextGroupTracingId() const {
+  return command_buffer_id_.GetUnsafeValue();
+}
+
 SharedContextState::SharedContextState(
     scoped_refptr<gl::GLShareGroup> share_group,
     scoped_refptr<gl::GLSurface> surface,
@@ -114,7 +153,9 @@ SharedContextState::SharedContextState(
     : use_virtualized_gl_contexts_(use_virtualized_gl_contexts),
       context_lost_callback_(std::move(context_lost_callback)),
       gr_context_type_(gr_context_type),
-      memory_tracker_(peak_memory_monitor),
+      memory_tracker_observer_(peak_memory_monitor),
+      memory_tracker_(&memory_tracker_observer_),
+      memory_type_tracker_(&memory_tracker_),
       vk_context_provider_(vulkan_context_provider),
       metal_context_provider_(metal_context_provider),
       dawn_context_provider_(dawn_context_provider),
@@ -172,7 +213,6 @@ SharedContextState::SharedContextState(
   // Initialize the scratch buffer to some small initial size.
   scratch_deserialization_buffer_.resize(
       kInitialScratchDeserializationBufferSize);
-
 }
 
 SharedContextState::~SharedContextState() {
@@ -195,8 +235,8 @@ SharedContextState::~SharedContextState() {
   DCHECK(!owned_gr_context_ || owned_gr_context_->unique());
 
   // GPU memory allocations except skia_gr_cache_size_ tracked by this
-  // memory_tracker_ should have been released.
-  DCHECK_EQ(skia_gr_cache_size_, memory_tracker_.GetMemoryUsage());
+  // memory_tracker_observer_ should have been released.
+  DCHECK_EQ(skia_gr_cache_size_, memory_tracker_observer_.GetMemoryUsage());
   // gr_context_ and all resources owned by it will be released soon, so set it
   // to null, and UpdateSkiaOwnedMemorySize() will update skia memory usage to
   // 0, to ensure that PeakGpuMemoryMonitor sees 0 allocated memory.
@@ -511,6 +551,9 @@ bool SharedContextState::IsCurrent(gl::GLSurface* surface) {
   return context_->IsCurrent(surface);
 }
 
+// TODO(https://crbug.com/1110357): Account for memory tracked by
+// memory_tracker_ and memory_type_tracker_ (e.g. SharedImages allocated in
+// SkiaOutputSurfaceImplOnGpu::CopyOutput).
 bool SharedContextState::OnMemoryDump(
     const base::trace_event::MemoryDumpArgs& args,
     base::trace_event::ProcessMemoryDump* pmd) {
@@ -574,13 +617,13 @@ void SharedContextState::PurgeMemory(
 
 uint64_t SharedContextState::GetMemoryUsage() {
   UpdateSkiaOwnedMemorySize();
-  return memory_tracker_.GetMemoryUsage();
+  return memory_tracker_observer_.GetMemoryUsage();
 }
 
 void SharedContextState::UpdateSkiaOwnedMemorySize() {
   if (!gr_context_) {
-    memory_tracker_.OnMemoryAllocatedChange(CommandBufferId(),
-                                            skia_gr_cache_size_, 0u);
+    memory_tracker_observer_.OnMemoryAllocatedChange(CommandBufferId(),
+                                                     skia_gr_cache_size_, 0u);
     skia_gr_cache_size_ = 0u;
     return;
   }
@@ -589,7 +632,7 @@ void SharedContextState::UpdateSkiaOwnedMemorySize() {
   // Skia does not have a CommandBufferId. PeakMemoryMonitor currently does not
   // use CommandBufferId to identify source, so use zero here to separate
   // prevent confusion.
-  memory_tracker_.OnMemoryAllocatedChange(
+  memory_tracker_observer_.OnMemoryAllocatedChange(
       CommandBufferId(), skia_gr_cache_size_, static_cast<uint64_t>(new_size));
   skia_gr_cache_size_ = static_cast<uint64_t>(new_size);
 }

gpu/command_buffer/service/shared_context_state.h

@@ -24,6 +24,7 @@
 #include "gpu/command_buffer/service/memory_tracking.h"
 #include "gpu/config/gpu_preferences.h"
 #include "gpu/gpu_gles2_export.h"
+#include "gpu/ipc/common/command_buffer_id.h"
 #include "gpu/ipc/common/gpu_peak_memory.h"
 #include "gpu/vulkan/buildflags.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -153,7 +154,13 @@ class GPU_GLES2_EXPORT SharedContextState
   bool support_vulkan_external_object() const {
     return support_vulkan_external_object_;
   }
-  gpu::MemoryTracker::Observer* memory_tracker() { return &memory_tracker_; }
+  gpu::MemoryTracker::Observer* memory_tracker_observer() {
+    return &memory_tracker_observer_;
+  }
+  gpu::MemoryTracker* memory_tracker() { return &memory_tracker_; }
+  gpu::MemoryTypeTracker* memory_type_tracker() {
+    return &memory_type_tracker_;
+  }
   ExternalSemaphorePool* external_semaphore_pool() {
 #if BUILDFLAG(ENABLE_VULKAN)
     return external_semaphore_pool_.get();
@@ -213,13 +220,14 @@ class GPU_GLES2_EXPORT SharedContextState
 
   // Observer which is notified when SkiaOutputSurfaceImpl takes ownership of a
   // shared image, and forward information to both histograms and task manager.
-  class GPU_GLES2_EXPORT MemoryTracker : public gpu::MemoryTracker::Observer {
+  class GPU_GLES2_EXPORT MemoryTrackerObserver
+      : public gpu::MemoryTracker::Observer {
    public:
-    explicit MemoryTracker(
+    explicit MemoryTrackerObserver(
         base::WeakPtr<gpu::MemoryTracker::Observer> peak_memory_monitor);
-    MemoryTracker(MemoryTracker&) = delete;
-    MemoryTracker& operator=(MemoryTracker&) = delete;
-    ~MemoryTracker() override;
+    MemoryTrackerObserver(MemoryTrackerObserver&) = delete;
+    MemoryTrackerObserver& operator=(MemoryTrackerObserver&) = delete;
+    ~MemoryTrackerObserver() override;
 
     // gpu::MemoryTracker::Observer implementation:
     void OnMemoryAllocatedChange(
@@ -237,6 +245,29 @@ class GPU_GLES2_EXPORT SharedContextState
     base::WeakPtr<gpu::MemoryTracker::Observer> const peak_memory_monitor_;
   };
 
+  // MemoryTracker implementation used to track SharedImages owned by
+  // SkiaOutputSurfaceImpl.
+  class MemoryTracker : public gpu::MemoryTracker {
+   public:
+    explicit MemoryTracker(gpu::MemoryTracker::Observer* observer);
+    MemoryTracker(const MemoryTracker&) = delete;
+    MemoryTracker& operator=(const MemoryTracker&) = delete;
+    ~MemoryTracker() override;
+
+    // MemoryTracker implementation:
+    void TrackMemoryAllocatedChange(int64_t delta) override;
+    uint64_t GetSize() const override;
+    uint64_t ClientTracingId() const override;
+    int ClientId() const override;
+    uint64_t ContextGroupTracingId() const override;
+
+   private:
+    gpu::CommandBufferId command_buffer_id_;
+    const uint64_t client_tracing_id_;
+    gpu::MemoryTracker::Observer* const observer_;
+    uint64_t size_ = 0;
+  };
+
   ~SharedContextState() override;
 
   // gpu::GLContextVirtualDelegate implementation.
@@ -263,7 +294,9 @@ class GPU_GLES2_EXPORT SharedContextState
   bool support_vulkan_external_object_ = false;
   ContextLostCallback context_lost_callback_;
   GrContextType gr_context_type_ = GrContextType::kGL;
+  MemoryTrackerObserver memory_tracker_observer_;
   MemoryTracker memory_tracker_;
+  gpu::MemoryTypeTracker memory_type_tracker_;
   viz::VulkanContextProvider* const vk_context_provider_;
   viz::MetalContextProvider* const metal_context_provider_;
   viz::DawnContextProvider* const dawn_context_provider_;