[Fuchsia] Pass RasterContextProvider to FuchsiaVideoDecoder

Previously RasterContextProvider kept raw pointers to
SharedImageInterface and ContextSupport interfaces. These pointers were
also passed to OutputMailbox instances, which may outlive the decoder.
There as nothing to guarantee that OutputMailbox doesn't outlive the
SharedImageInterface implementation. Updated the decoder to use a
ref-counted pointer to RasterContextProvider interface. This allows to
ensure that SharedImageInterface is not destroyed before the
OutputMailbox instances that depend on it.

Bug: 1133325
Change-Id: I60b664d89207535d968cf998488457c789c800c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2441935Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816848}

media/filters/BUILD.gn

@@ -223,6 +223,7 @@ source_set("filters") {
       "fuchsia/fuchsia_video_decoder.h",
     ]
     deps += [
+      "//components/viz/common",
       "//gpu/command_buffer/client",
       "//gpu/command_buffer/common",
       "//gpu/ipc/common",
@@ -334,8 +335,10 @@ source_set("unit_tests") {
   if (is_fuchsia) {
     sources += [ "fuchsia/fuchsia_video_decoder_unittest.cc" ]
     deps += [
+      "//components/viz/common",
       "//components/viz/test:test_support",
       "//gpu/command_buffer/client",
+      "//gpu/config",
       "//third_party/fuchsia-sdk/sdk/fidl/fuchsia.sysmem",
       "//third_party/fuchsia-sdk/sdk/pkg/sys_cpp",
     ]

media/filters/fuchsia/DEPS

+include_rules = [
+  "+components/viz/common/gpu/raster_context_provider.h",
+]
\ No newline at end of file

media/filters/fuchsia/fuchsia_video_decoder.cc

@@ -24,6 +24,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/process/process_metrics.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "components/viz/common/gpu/raster_context_provider.h"
 #include "gpu/command_buffer/client/context_support.h"
 #include "gpu/command_buffer/client/shared_image_interface.h"
 #include "gpu/command_buffer/common/shared_image_usage.h"
@@ -60,22 +61,23 @@ const uint32_t kMaxUsedOutputFrames = 6;
 // outlive FuchsiaVideoDecoder if is referenced by a VideoFrame.
 class OutputMailbox {
  public:
-  OutputMailbox(gpu::SharedImageInterface* shared_image_interface,
-                gpu::ContextSupport* gpu_context_support,
-                std::unique_ptr<gfx::GpuMemoryBuffer> gmb)
-      : shared_image_interface_(shared_image_interface),
-        gpu_context_support_(gpu_context_support),
-        weak_factory_(this) {
+  OutputMailbox(
+      scoped_refptr<viz::RasterContextProvider> raster_context_provider,
+      std::unique_ptr<gfx::GpuMemoryBuffer> gmb)
+      : raster_context_provider_(raster_context_provider), weak_factory_(this) {
     uint32_t usage = gpu::SHARED_IMAGE_USAGE_RASTER |
                      gpu::SHARED_IMAGE_USAGE_OOP_RASTERIZATION |
                      gpu::SHARED_IMAGE_USAGE_DISPLAY |
                      gpu::SHARED_IMAGE_USAGE_SCANOUT;
-    mailbox_ = shared_image_interface_->CreateSharedImage(
-        gmb.get(), nullptr, gfx::ColorSpace(), kTopLeft_GrSurfaceOrigin,
-        kPremul_SkAlphaType, usage);
+    mailbox_ =
+        raster_context_provider_->SharedImageInterface()->CreateSharedImage(
+            gmb.get(), nullptr, gfx::ColorSpace(), kTopLeft_GrSurfaceOrigin,
+            kPremul_SkAlphaType, usage);
   }
+
   ~OutputMailbox() {
-    shared_image_interface_->DestroySharedImage(sync_token_, mailbox_);
+    raster_context_provider_->SharedImageInterface()->DestroySharedImage(
+        sync_token_, mailbox_);
   }
 
   const gpu::Mailbox& mailbox() { return mailbox_; }
@@ -94,7 +96,8 @@ class OutputMailbox {
 
     gpu::MailboxHolder mailboxes[VideoFrame::kMaxPlanes];
     mailboxes[0].mailbox = mailbox_;
-    mailboxes[0].sync_token = shared_image_interface_->GenUnverifiedSyncToken();
+    mailboxes[0].sync_token = raster_context_provider_->SharedImageInterface()
+                                  ->GenUnverifiedSyncToken();
 
     auto frame = VideoFrame::WrapNativeTextures(
         pixel_format, mailboxes,
@@ -132,7 +135,7 @@ class OutputMailbox {
       return;
     }
 
-    gpu_context_support_->SignalSyncToken(
+    raster_context_provider_->ContextSupport()->SignalSyncToken(
         sync_token_,
         BindToCurrentLoop(base::BindOnce(&OutputMailbox::OnSyncTokenSignaled,
                                          weak_factory_.GetWeakPtr())));
@@ -143,8 +146,7 @@ class OutputMailbox {
     std::move(reuse_callback_).Run();
   }
 
-  gpu::SharedImageInterface* const shared_image_interface_;
-  gpu::ContextSupport* const gpu_context_support_;
+  const scoped_refptr<viz::RasterContextProvider> raster_context_provider_;
 
   gpu::Mailbox mailbox_;
   gpu::SyncToken sync_token_;
@@ -169,9 +171,9 @@ struct InputDecoderPacket {
 class FuchsiaVideoDecoder : public VideoDecoder,
                             public FuchsiaSecureStreamDecryptor::Client {
  public:
-  FuchsiaVideoDecoder(gpu::SharedImageInterface* shared_image_interface,
-                      gpu::ContextSupport* gpu_context_support,
-                      bool enable_sw_decoding);
+  FuchsiaVideoDecoder(
+      scoped_refptr<viz::RasterContextProvider> raster_context_provider,
+      bool enable_sw_decoding);
   ~FuchsiaVideoDecoder() override;
 
   // Decoder implementation.
@@ -248,8 +250,7 @@ class FuchsiaVideoDecoder : public VideoDecoder,
   void ReleaseInputBuffers();
   void ReleaseOutputBuffers();
 
-  gpu::SharedImageInterface* const shared_image_interface_;
-  gpu::ContextSupport* const gpu_context_support_;
+  const scoped_refptr<viz::RasterContextProvider> raster_context_provider_;
   const bool enable_sw_decoding_;
   const bool use_overlays_for_video_;
 
@@ -311,17 +312,15 @@ class FuchsiaVideoDecoder : public VideoDecoder,
 };
 
 FuchsiaVideoDecoder::FuchsiaVideoDecoder(
-    gpu::SharedImageInterface* shared_image_interface,
-    gpu::ContextSupport* gpu_context_support,
+    scoped_refptr<viz::RasterContextProvider> raster_context_provider,
     bool enable_sw_decoding)
-    : shared_image_interface_(shared_image_interface),
-      gpu_context_support_(gpu_context_support),
+    : raster_context_provider_(raster_context_provider),
       enable_sw_decoding_(enable_sw_decoding),
       use_overlays_for_video_(base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kUseOverlaysForVideo)),
       client_native_pixmap_factory_(ui::CreateClientNativePixmapFactoryOzone()),
       weak_factory_(this) {
-  DCHECK(shared_image_interface_);
+  DCHECK(raster_context_provider_);
   weak_this_ = weak_factory_.GetWeakPtr();
 }
 
@@ -889,10 +888,10 @@ void FuchsiaVideoDecoder::OnOutputPacket(fuchsia::media::Packet output_packet,
         buffer_format, gfx::BufferUsage::GPU_READ,
         gpu::GpuMemoryBufferImpl::DestructionCallback());
 
-    output_mailboxes_[buffer_index] = new OutputMailbox(
-        shared_image_interface_, gpu_context_support_, std::move(gmb));
+    output_mailboxes_[buffer_index] =
+        new OutputMailbox(raster_context_provider_, std::move(gmb));
   } else {
-    shared_image_interface_->UpdateSharedImage(
+    raster_context_provider_->SharedImageInterface()->UpdateSharedImage(
         gpu::SyncToken(), output_mailboxes_[buffer_index]->mailbox());
   }
 
@@ -1028,11 +1027,12 @@ void FuchsiaVideoDecoder::InitializeOutputBufferCollection(
   // Register the new collection with the GPU process.
   DCHECK(!output_buffer_collection_id_);
   output_buffer_collection_id_ = gfx::SysmemBufferCollectionId::Create();
-  shared_image_interface_->RegisterSysmemBufferCollection(
-      output_buffer_collection_id_,
-      collection_token_for_gpu.Unbind().TakeChannel(),
-      gfx::BufferFormat::YUV_420_BIPLANAR, gfx::BufferUsage::GPU_READ,
-      true /*register_with_image_pipe*/);
+  raster_context_provider_->SharedImageInterface()
+      ->RegisterSysmemBufferCollection(
+          output_buffer_collection_id_,
+          collection_token_for_gpu.Unbind().TakeChannel(),
+          gfx::BufferFormat::YUV_420_BIPLANAR, gfx::BufferUsage::GPU_READ,
+          true /*register_with_image_pipe*/);
 
   // Pass new output buffer settings to the codec.
   fuchsia::media::StreamBufferPartialSettings settings;
@@ -1081,8 +1081,8 @@ void FuchsiaVideoDecoder::ReleaseOutputBuffers() {
 
   // Tell the GPU process to drop the buffer collection.
   if (output_buffer_collection_id_) {
-    shared_image_interface_->ReleaseSysmemBufferCollection(
-        output_buffer_collection_id_);
+    raster_context_provider_->SharedImageInterface()
+        ->ReleaseSysmemBufferCollection(output_buffer_collection_id_);
     output_buffer_collection_id_ = {};
   }
 }
@@ -1101,19 +1101,17 @@ void FuchsiaVideoDecoder::OnReuseMailbox(uint32_t buffer_index,
 }
 
 std::unique_ptr<VideoDecoder> CreateFuchsiaVideoDecoder(
-    gpu::SharedImageInterface* shared_image_interface,
-    gpu::ContextSupport* gpu_context_support) {
-  return std::make_unique<FuchsiaVideoDecoder>(shared_image_interface,
-                                               gpu_context_support,
-                                               /*enable_sw_decoding=*/false);
+    scoped_refptr<viz::RasterContextProvider> raster_context_provider) {
+  return std::make_unique<FuchsiaVideoDecoder>(
+      std::move(raster_context_provider),
+      /*enable_sw_decoding=*/false);
 }
 
 std::unique_ptr<VideoDecoder> CreateFuchsiaVideoDecoderForTests(
-    gpu::SharedImageInterface* shared_image_interface,
-    gpu::ContextSupport* gpu_context_support,
+    scoped_refptr<viz::RasterContextProvider> raster_context_provider,
     bool enable_sw_decoding) {
   return std::make_unique<FuchsiaVideoDecoder>(
-      shared_image_interface, gpu_context_support, enable_sw_decoding);
+      std::move(raster_context_provider), enable_sw_decoding);
 }
 
 }  // namespace media

media/filters/fuchsia/fuchsia_video_decoder.h

@@ -7,12 +7,12 @@
 
 #include <memory>
 
+#include "base/memory/scoped_refptr.h"
 #include "media/base/media_export.h"
 
-namespace gpu {
-class ContextSupport;
-class SharedImageInterface;
-}  // namespace gpu
+namespace viz {
+class RasterContextProvider;
+}  // namespace viz
 
 namespace media {
 
@@ -20,17 +20,14 @@ class VideoDecoder;
 
 // Creates VideoDecoder that uses fuchsia.mediacodec API. The returned
 // VideoDecoder instance will only try to use hardware video codecs.
-// |shared_image_interface| and |gpu_context_support| must outlive the decoder.
 MEDIA_EXPORT std::unique_ptr<VideoDecoder> CreateFuchsiaVideoDecoder(
-    gpu::SharedImageInterface* shared_image_interface,
-    gpu::ContextSupport* gpu_context_support);
+    scoped_refptr<viz::RasterContextProvider> raster_context_provider);
 
 // Same as above, but also allows to enable software codecs. This is useful for
 // FuchsiaVideoDecoder tests that run on systems that don't have hardware
 // decoder support.
 MEDIA_EXPORT std::unique_ptr<VideoDecoder> CreateFuchsiaVideoDecoderForTests(
-    gpu::SharedImageInterface* shared_image_interface,
-    gpu::ContextSupport* gpu_context_support,
+    scoped_refptr<viz::RasterContextProvider> raster_context_provider,
     bool enable_sw_decoding);
 
 }  // namespace media

media/filters/fuchsia/fuchsia_video_decoder_unittest.cc

@@ -13,9 +13,12 @@
 #include "base/containers/flat_set.h"
 #include "base/fuchsia/fuchsia_logging.h"
 #include "base/fuchsia/process_context.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
+#include "components/viz/common/gpu/raster_context_provider.h"
 #include "components/viz/test/test_context_support.h"
 #include "gpu/command_buffer/client/shared_image_interface.h"
+#include "gpu/config/gpu_feature_info.h"
 #include "media/base/test_data_util.h"
 #include "media/base/test_helpers.h"
 #include "media/base/video_decoder.h"
@@ -89,7 +92,7 @@ class TestSharedImageInterface : public gpu::SharedImageInterface {
                                  SkAlphaType alpha_type,
                                  uint32_t usage,
                                  gpu::SurfaceHandle surface_handle) override {
-    NOTREACHED();
+    ADD_FAILURE();
     return gpu::Mailbox();
   }
 
@@ -101,7 +104,7 @@ class TestSharedImageInterface : public gpu::SharedImageInterface {
       SkAlphaType alpha_type,
       uint32_t usage,
       base::span<const uint8_t> pixel_data) override {
-    NOTREACHED();
+    ADD_FAILURE();
     return gpu::Mailbox();
   }
 
@@ -128,12 +131,12 @@ class TestSharedImageInterface : public gpu::SharedImageInterface {
 
   void UpdateSharedImage(const gpu::SyncToken& sync_token,
                          const gpu::Mailbox& mailbox) override {
-    NOTREACHED();
+    ADD_FAILURE();
   }
   void UpdateSharedImage(const gpu::SyncToken& sync_token,
                          std::unique_ptr<gfx::GpuFence> acquire_fence,
                          const gpu::Mailbox& mailbox) override {
-    NOTREACHED();
+    ADD_FAILURE();
   }
 
   void DestroySharedImage(const gpu::SyncToken& sync_token,
@@ -147,12 +150,12 @@ class TestSharedImageInterface : public gpu::SharedImageInterface {
                                      GrSurfaceOrigin surface_origin,
                                      SkAlphaType alpha_type,
                                      uint32_t usage) override {
-    NOTREACHED();
+    ADD_FAILURE();
     return SwapChainMailboxes();
   }
   void PresentSwapChain(const gpu::SyncToken& sync_token,
                         const gpu::Mailbox& mailbox) override {
-    NOTREACHED();
+    ADD_FAILURE();
   }
 
   void RegisterSysmemBufferCollection(gfx::SysmemBufferCollectionId id,
@@ -173,7 +176,7 @@ class TestSharedImageInterface : public gpu::SharedImageInterface {
   }
 
   gpu::SyncToken GenVerifiedSyncToken() override {
-    NOTREACHED();
+    ADD_FAILURE();
     return gpu::SyncToken();
   }
   gpu::SyncToken GenUnverifiedSyncToken() override {
@@ -182,10 +185,10 @@ class TestSharedImageInterface : public gpu::SharedImageInterface {
   }
 
   void WaitSyncToken(const gpu::SyncToken& sync_token) override {
-    NOTREACHED();
+    ADD_FAILURE();
   }
 
-  void Flush() override { NOTREACHED(); }
+  void Flush() override { ADD_FAILURE(); }
 
   scoped_refptr<gfx::NativePixmap> GetNativePixmap(
       const gpu::Mailbox& mailbox) override {
@@ -200,16 +203,93 @@ class TestSharedImageInterface : public gpu::SharedImageInterface {
   base::flat_set<gpu::Mailbox> mailboxes_;
 };
 
+class TestRasterContextProvider
+    : public base::RefCountedThreadSafe<TestRasterContextProvider>,
+      public viz::RasterContextProvider {
+ public:
+  TestRasterContextProvider() {}
+
+  TestRasterContextProvider(TestRasterContextProvider&) = delete;
+  TestRasterContextProvider& operator=(TestRasterContextProvider&) = delete;
+
+  void SetOnDestroyedClosure(base::Closure on_destroyed) {
+    on_destroyed_ = on_destroyed;
+  }
+
+  // viz::RasterContextProvider implementation;
+  void AddRef() const override {
+    base::RefCountedThreadSafe<TestRasterContextProvider>::AddRef();
+  }
+  void Release() const override {
+    base::RefCountedThreadSafe<TestRasterContextProvider>::Release();
+  }
+  gpu::ContextResult BindToCurrentThread() override {
+    ADD_FAILURE();
+    return gpu::ContextResult::kFatalFailure;
+  }
+  void AddObserver(viz::ContextLostObserver* obs) override { ADD_FAILURE(); }
+  void RemoveObserver(viz::ContextLostObserver* obs) override { ADD_FAILURE(); }
+  base::Lock* GetLock() override {
+    ADD_FAILURE();
+    return nullptr;
+  }
+  viz::ContextCacheController* CacheController() override {
+    ADD_FAILURE();
+    return nullptr;
+  }
+  gpu::ContextSupport* ContextSupport() override {
+    return &gpu_context_support_;
+  }
+  class GrDirectContext* GrContext() override {
+    ADD_FAILURE();
+    return nullptr;
+  }
+  gpu::SharedImageInterface* SharedImageInterface() override {
+    return &shared_image_interface_;
+  }
+  const gpu::Capabilities& ContextCapabilities() const override {
+    ADD_FAILURE();
+    static gpu::Capabilities dummy_caps;
+    return dummy_caps;
+  }
+  const gpu::GpuFeatureInfo& GetGpuFeatureInfo() const override {
+    ADD_FAILURE();
+    static gpu::GpuFeatureInfo dummy_feature_info;
+    return dummy_feature_info;
+  }
+  gpu::gles2::GLES2Interface* ContextGL() override {
+    ADD_FAILURE();
+    return nullptr;
+  }
+  gpu::raster::RasterInterface* RasterInterface() override {
+    ADD_FAILURE();
+    return nullptr;
+  }
+
+ private:
+  friend class base::RefCountedThreadSafe<TestRasterContextProvider>;
+
+  ~TestRasterContextProvider() override {
+    if (on_destroyed_)
+      std::move(on_destroyed_).Run();
+  }
+
+  TestSharedImageInterface shared_image_interface_;
+  viz::TestContextSupport gpu_context_support_;
+
+  base::Closure on_destroyed_;
+};
+
 }  // namespace
 
 class FuchsiaVideoDecoderTest : public testing::Test {
  public:
-  FuchsiaVideoDecoderTest() {
-    decoder_ = CreateFuchsiaVideoDecoderForTests(&shared_image_interface_,
-                                                 &gpu_context_support_,
-
-                                                 /*enable_sw_decoding=*/true);
-  }
+  FuchsiaVideoDecoderTest()
+      : raster_context_provider_(
+            base::MakeRefCounted<TestRasterContextProvider>()),
+        decoder_(
+            CreateFuchsiaVideoDecoderForTests(raster_context_provider_.get(),
+                                              /*enable_sw_decoding=*/true)) {}
   ~FuchsiaVideoDecoderTest() override = default;
 
   bool InitializeDecoder(VideoDecoderConfig config) WARN_UNUSED_RESULT {
@@ -285,8 +365,7 @@ class FuchsiaVideoDecoderTest : public testing::Test {
   base::test::SingleThreadTaskEnvironment task_environment_{
       base::test::SingleThreadTaskEnvironment::MainThreadType::IO};
 
-  TestSharedImageInterface shared_image_interface_;
-  viz::TestContextSupport gpu_context_support_;
+  scoped_refptr<TestRasterContextProvider> raster_context_provider_;
 
   std::unique_ptr<VideoDecoder> decoder_;
 
@@ -396,4 +475,35 @@ TEST_F(FuchsiaVideoDecoderTest, ResetAndReinitializeH264) {
   EXPECT_EQ(num_output_frames_, 4U);
 }
 
+// Verifies that the decoder keeps reference to the RasterContextProvider.
+TEST_F(FuchsiaVideoDecoderTest, RasterContextLifetime) {
+  bool context_destroyed = false;
+  raster_context_provider_->SetOnDestroyedClosure(base::BindLambdaForTesting(
+      [&context_destroyed]() { context_destroyed = true; }));
+  ASSERT_TRUE(InitializeDecoder(TestVideoConfig::NormalH264()));
+  ASSERT_FALSE(context_destroyed);
+
+  // Decoder should keep reference to RasterContextProvider.
+  raster_context_provider_.reset();
+  ASSERT_FALSE(context_destroyed);
+
+  // Feed some frames to decoder to get decoded video frames.
+  for (int i = 0; i < 4; ++i) {
+    DecodeBuffer(GetH264Frame(i));
+  }
+  ASSERT_NO_FATAL_FAILURE(WaitDecodeDone());
+
+  // Destroy the decoder. RasterContextProvider will not be destroyed since
+  // it's still referenced by frames in |output_frames_|.
+  decoder_.reset();
+  task_environment_.RunUntilIdle();
+  ASSERT_FALSE(context_destroyed);
+
+  // RasterContextProvider reference should be dropped once all frames are
+  // dropped.
+  output_frames_.clear();
+  task_environment_.RunUntilIdle();
+  ASSERT_TRUE(context_destroyed);
+}
+
 }  // namespace media

media/renderers/default_decoder_factory.cc

@@ -133,9 +133,7 @@ void DefaultDecoderFactory::CreateVideoDecoders(
     //
     // TODO(crbug.com/580386): Handle context loss properly.
     if (context_provider) {
-      video_decoders->push_back(
-          CreateFuchsiaVideoDecoder(gpu_factories->SharedImageInterface(),
-                                    context_provider->ContextSupport()));
+      video_decoders->push_back(CreateFuchsiaVideoDecoder(context_provider));
     } else {
       DLOG(ERROR)
           << "Can't create FuchsiaVideoDecoder due to GPU context loss.";