viz: Do not use refptr for ContextProvider inside GLRendererCopier

In viz, the display compositor owns the context provider, and as the
display compositor is getting destroyed, the context provider should be
destroyed at the same time. Objects created by the GLRendererCopier
has been the only ones keeping the context provider alive longer than
the display compositor. This change will fail copy requests when the
display compositor is getting destroyed.


Bug: 1096487
Change-Id: I3cedd70dff42ed7ef9f946ec4b34017d3560df7f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2253159
Commit-Queue: weiliangc <weiliangc@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802424}

components/viz/common/gl_i420_converter.cc

@@ -10,16 +10,17 @@
 
 namespace viz {
 
-GLI420Converter::GLI420Converter(
-    scoped_refptr<ContextProvider> context_provider)
-    : GLI420Converter(std::move(context_provider), true) {}
-
-GLI420Converter::GLI420Converter(
-    scoped_refptr<ContextProvider> context_provider,
-    bool allow_mrt_path)
-    : context_provider_(std::move(context_provider)),
+GLI420Converter::GLI420Converter(ContextProvider* context_provider)
+    : GLI420Converter(context_provider, true) {
+  DCHECK(context_provider_);
+}
+
+GLI420Converter::GLI420Converter(ContextProvider* context_provider,
+                                 bool allow_mrt_path)
+    : context_provider_(context_provider),
       step1_(context_provider_),
       step2_(context_provider_) {
+  DCHECK(context_provider_);
   context_provider_->AddObserver(this);
   if (!allow_mrt_path || step1_.GetMaxDrawBuffersSupported() < 2) {
     step3_ = std::make_unique<GLScaler>(context_provider_);

components/viz/common/gl_i420_converter.h

@@ -76,7 +76,7 @@ class VIZ_COMMON_EXPORT GLI420Converter : public ContextLostObserver {
   // GLI420Converter uses the exact same parameters as GLScaler.
   using Parameters = GLScaler::Parameters;
 
-  explicit GLI420Converter(scoped_refptr<ContextProvider> context_provider);
+  explicit GLI420Converter(ContextProvider* context_provider);
   ~GLI420Converter() final;
 
   // Returns true if the GL context provides the necessary support for enabling
@@ -145,8 +145,7 @@ class VIZ_COMMON_EXPORT GLI420Converter : public ContextLostObserver {
  private:
   friend class GLI420ConverterPixelTest;
 
-  GLI420Converter(scoped_refptr<ContextProvider> context_provider,
-                  bool allow_mrt_path);
+  GLI420Converter(ContextProvider* context_provider, bool allow_mrt_path);
 
   bool is_using_mrt_path() const { return !step3_; }
 
@@ -159,7 +158,7 @@ class VIZ_COMMON_EXPORT GLI420Converter : public ContextLostObserver {
 
   // The provider of the GL context. This is non-null while the GL context is
   // valid and GLI420Converter is observing for context loss.
-  scoped_refptr<ContextProvider> context_provider_;
+  ContextProvider* context_provider_;
 
   // Scales the source content and produces either:
   //   * MRT path: NV61-format output in two textures.

components/viz/common/gl_scaler.cc

@@ -42,8 +42,8 @@ std::ostream& operator<<(std::ostream& out, const RelativeSize& size) {
 
 }  // namespace
 
-GLScaler::GLScaler(scoped_refptr<ContextProvider> context_provider)
-    : context_provider_(std::move(context_provider)) {
+GLScaler::GLScaler(ContextProvider* context_provider)
+    : context_provider_(context_provider) {
   if (context_provider_) {
     DCHECK(context_provider_->ContextGL());
     context_provider_->AddObserver(this);

components/viz/common/gl_scaler.h

@@ -191,7 +191,7 @@ class VIZ_COMMON_EXPORT GLScaler : public ContextLostObserver {
     ~Parameters();
   };
 
-  explicit GLScaler(scoped_refptr<ContextProvider> context_provider);
+  explicit GLScaler(ContextProvider* context_provider);
 
   ~GLScaler() final;
 
@@ -474,7 +474,7 @@ class VIZ_COMMON_EXPORT GLScaler : public ContextLostObserver {
 
   // The provider of the GL context. This is non-null while the GL context is
   // valid and GLScaler is observing for context loss.
-  scoped_refptr<ContextProvider> context_provider_;
+  ContextProvider* context_provider_;
 
   // Set by Configure() to the resolved set of Parameters.
   Parameters params_;

components/viz/common/gl_scaler_unittest.cc

@@ -99,7 +99,7 @@ TEST_F(GLScalerTest, AddAndRemovesSelfAsContextLossObserver) {
       .WillOnce(SaveArg<0>(&registered_observer));
   EXPECT_CALL(provider, RemoveObserver(Eq(ByRef(registered_observer))))
       .InSequence(s);
-  GLScaler scaler(base::WrapRefCounted(&provider));
+  GLScaler scaler(&provider);
 }
 
 TEST_F(GLScalerTest, RemovesObserverWhenContextIsLost) {
@@ -111,7 +111,7 @@ TEST_F(GLScalerTest, RemovesObserverWhenContextIsLost) {
       .WillOnce(SaveArg<0>(&registered_observer));
   EXPECT_CALL(provider, RemoveObserver(Eq(ByRef(registered_observer))))
       .InSequence(s);
-  GLScaler scaler(base::WrapRefCounted(&provider));
+  GLScaler scaler(&provider);
   static_cast<ContextLostObserver&>(scaler).OnContextLost();
   // Verify RemoveObserver() was called before |scaler| goes out-of-scope.
   Mock::VerifyAndClearExpectations(&provider);

components/viz/service/display/gl_renderer_copier.h

@@ -9,6 +9,7 @@
 
 #include <array>
 #include <memory>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
@@ -17,6 +18,8 @@
 #include "base/task_runner.h"
 #include "base/unguessable_token.h"
 #include "components/viz/service/viz_service_export.h"
+#include "ui/gfx/color_space.h"
+#include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 
 namespace gfx {
@@ -66,8 +69,8 @@ class VIZ_SERVICE_EXPORT GLRendererCopier {
   using GLuint = unsigned int;
   using GLenum = unsigned int;
 
-  // |texture_deleter| must outlive this instance.
-  GLRendererCopier(scoped_refptr<ContextProvider> context_provider,
+  // |context_provider| and |texture_deleter| must outlive this instance.
+  GLRendererCopier(ContextProvider* context_provider,
                    TextureDeleter* texture_deleter);
 
   ~GLRendererCopier();
@@ -141,6 +144,44 @@ class VIZ_SERVICE_EXPORT GLRendererCopier {
     DISALLOW_COPY_AND_ASSIGN(ReusableThings);
   };
 
+  // Manages the execution of one asynchronous framebuffer readback and contains
+  // all the relevant state needed to complete a copy request. The constructor
+  // initiates the operation, and the destructor cleans up all the GL objects
+  // created for this workflow. This class is owned by the GLRendererCopier, and
+  // GLRendererCopier is responsible for deleting this either after the workflow
+  // is finished, or when the GLRendererCopier is being destroyed.
+  struct ReadPixelsWorkflow {
+   public:
+    // Saves all revelant state and initiates the GL asynchronous read-pixels
+    // workflow.
+    ReadPixelsWorkflow(std::unique_ptr<CopyOutputRequest> copy_request,
+                       const gfx::Vector2d& readback_offset,
+                       bool flipped_source,
+                       bool swap_red_and_blue,
+                       const gfx::Rect& result_rect,
+                       const gfx::ColorSpace& color_space,
+                       ContextProvider* context_provider,
+                       GLenum readback_format);
+    ReadPixelsWorkflow(const ReadPixelsWorkflow&) = delete;
+
+    // The destructor is by the GLRendererCopier, either called after the
+    // workflow is finished or when GLRendererCopier is being destoryed.
+    ~ReadPixelsWorkflow();
+
+    GLuint query() const { return query_; }
+
+    const std::unique_ptr<CopyOutputRequest> copy_request;
+    const bool flipped_source;
+    const bool swap_red_and_blue;
+    const gfx::Rect result_rect;
+    const gfx::ColorSpace color_space;
+    GLuint transfer_buffer = 0;
+
+   private:
+    ContextProvider* const context_provider_;
+    GLuint query_ = 0;
+  };
+
   // Renders a scaled/transformed copy of a source texture according to the
   // |request| parameters and other source characteristics. |result_texture|
   // must be allocated/sized by the caller. For RGBA_BITMAP requests, the image
@@ -157,6 +198,43 @@ class VIZ_SERVICE_EXPORT GLRendererCopier {
                            GLuint result_texture,
                            ReusableThings* things);
 
+  // Like the ReadPixelsWorkflow, except for I420 planes readback. Because there
+  // are three separate glReadPixels operations that may complete in any order,
+  // a ReadI420PlanesWorkflow will receive notifications from three separate "GL
+  // query" callbacks. It is only after all three operations have completed that
+  // a fully-assembled CopyOutputResult can be sent.
+  //
+  // See class comments for GLI420Converter for an explanation of how
+  // planar data is packed into RGBA textures.
+  struct ReadI420PlanesWorkflow {
+   public:
+    ReadI420PlanesWorkflow(std::unique_ptr<CopyOutputRequest> copy_request,
+                           const gfx::Rect& aligned_rect,
+                           const gfx::Rect& result_rect,
+                           base::WeakPtr<GLRendererCopier> copier_weak_ptr,
+                           ContextProvider* context_provider);
+
+    void BindTransferBuffer();
+    void StartPlaneReadback(int plane, GLenum readback_format);
+    void UnbindTransferBuffer();
+
+    ~ReadI420PlanesWorkflow();
+
+    const std::unique_ptr<CopyOutputRequest> copy_request;
+    const gfx::Rect aligned_rect;
+    const gfx::Rect result_rect;
+    GLuint transfer_buffer;
+    std::array<GLuint, 3> queries;
+
+   private:
+    gfx::Size y_texture_size() const;
+    gfx::Size chroma_texture_size() const;
+
+    base::WeakPtr<GLRendererCopier> copier_weak_ptr_;
+    ContextProvider* const context_provider_;
+    std::array<int, 3> data_offsets_;
+  };
+
   // Similar to RenderResultTexture(), except also transform the image into I420
   // format (a popular video format). Three textures, representing each of the
   // Y/U/V planes (as described in GLI420Converter), are populated and their GL
@@ -251,8 +329,11 @@ class VIZ_SERVICE_EXPORT GLRendererCopier {
   // swap does not need to happen on the CPU (non-negligible cost).
   bool ShouldSwapRedAndBlueForBitmapReadback();
 
+  void FinishReadPixelsWorkflow(ReadPixelsWorkflow*);
+  void FinishReadI420PlanesWorkflow(ReadI420PlanesWorkflow*, int plane);
+
   // Injected dependencies.
-  const scoped_refptr<ContextProvider> context_provider_;
+  ContextProvider* const context_provider_;
   TextureDeleter* const texture_deleter_;
 
   // This increments by one for every call to FreeUnusedCachedResources(). It
@@ -279,6 +360,12 @@ class VIZ_SERVICE_EXPORT GLRendererCopier {
   // things to be auto-purged after approx. 1-2 seconds of not being used.
   static constexpr int kKeepalivePeriod = 60;
 
+  std::vector<std::unique_ptr<ReadPixelsWorkflow>> read_pixels_workflows_;
+  std::vector<std::unique_ptr<ReadI420PlanesWorkflow>> read_i420_workflows_;
+
+  // Weak ptr to this class.
+  base::WeakPtrFactory<GLRendererCopier> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(GLRendererCopier);
 };
 

components/viz/service/display/gl_renderer_copier_perftest.cc

@@ -61,15 +61,15 @@ base::FilePath GetTestFilePath(const base::FilePath::CharType* basename) {
 class GLRendererCopierPerfTest : public testing::Test {
  public:
   GLRendererCopierPerfTest() {
-    auto context_provider = base::MakeRefCounted<TestInProcessContextProvider>(
+    context_provider_ = base::MakeRefCounted<TestInProcessContextProvider>(
         /*enable_gpu_rasterization=*/false,
         /*enable_oop_rasterization=*/false, /*support_locking=*/false);
-    gpu::ContextResult result = context_provider->BindToCurrentThread();
+    gpu::ContextResult result = context_provider_->BindToCurrentThread();
     DCHECK_EQ(result, gpu::ContextResult::kSuccess);
-    gl_ = context_provider->ContextGL();
+    gl_ = context_provider_->ContextGL();
     texture_deleter_ =
         std::make_unique<TextureDeleter>(base::ThreadTaskRunnerHandle::Get());
-    copier_ = std::make_unique<GLRendererCopier>(std::move(context_provider),
+    copier_ = std::make_unique<GLRendererCopier>(context_provider_.get(),
                                                  texture_deleter_.get());
   }
 
@@ -259,6 +259,7 @@ class GLRendererCopierPerfTest : public testing::Test {
 
  private:
   gpu::gles2::GLES2Interface* gl_ = nullptr;
+  scoped_refptr<TestInProcessContextProvider> context_provider_;
   std::unique_ptr<TextureDeleter> texture_deleter_;
   std::unique_ptr<GLRendererCopier> copier_;
   GLuint source_texture_ = 0;

components/viz/service/display/gl_renderer_copier_unittest.cc

@@ -62,11 +62,11 @@ class GLRendererCopierTest : public testing::Test {
   using ReusableThings = GLRendererCopier::ReusableThings;
 
   void SetUp() override {
-    auto context_provider = TestContextProvider::Create(
+    context_provider_ = TestContextProvider::Create(
         std::make_unique<CopierTestGLES2Interface>());
-    context_provider->BindToCurrentThread();
-    copier_ = std::make_unique<GLRendererCopier>(std::move(context_provider),
-                                                 nullptr);
+    context_provider_->BindToCurrentThread();
+    copier_ =
+        std::make_unique<GLRendererCopier>(context_provider_.get(), nullptr);
   }
 
   void TearDown() override { copier_.reset(); }
@@ -102,6 +102,7 @@ class GLRendererCopierTest : public testing::Test {
   static constexpr int kKeepalivePeriod = GLRendererCopier::kKeepalivePeriod;
 
  private:
+  scoped_refptr<ContextProvider> context_provider_;
   std::unique_ptr<GLRendererCopier> copier_;
 };