[Video Capture Service] Operate jpeg decoder IO to gpu process on sep…

…arate thread

Adds a dedicated thread to the video capture service for operating the IO
between the capture device and the gpu process for acceleraged Mjpeg decoding.

This fixes a deadlock during shutdown of video capture in the video capture
service after a session that uses the accelerated jpeg decoder. See the crbug
for details.

The deadlock and the fix for it can currently only be provoked/verified manually
by running a ChromeOS device with a camera attached and command-line flag
--enable-features=MojoVideoCapture. Getting coverage in automated integration
testing is possible but requires modification of the fake video capture device
implementation, see crbug/852606.

Bug: 852606, 820608
Change-Id: Id9a57e386d2a741f15fd1de72539f4a9885624da
Reviewed-on: https://chromium-review.googlesource.com/1102878
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568966}

media/capture/video/video_capture_jpeg_decoder_impl.cc

@@ -11,7 +11,7 @@ namespace media {
 
 VideoCaptureJpegDecoderImpl::VideoCaptureJpegDecoderImpl(
     MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory,
-    scoped_refptr<base::SingleThreadTaskRunner> decoder_task_runner,
+    scoped_refptr<base::SequencedTaskRunner> decoder_task_runner,
     DecodeDoneCB decode_done_cb,
     base::RepeatingCallback<void(const std::string&)> send_log_message_cb)
     : jpeg_decoder_factory_(std::move(jpeg_decoder_factory)),

media/capture/video/video_capture_jpeg_decoder_impl.h

@@ -43,7 +43,7 @@ class CAPTURE_EXPORT VideoCaptureJpegDecoderImpl
   // VideoCaptureGpuJpegDecoder is destroyed.
   VideoCaptureJpegDecoderImpl(
       MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory,
-      scoped_refptr<base::SingleThreadTaskRunner> decoder_task_runner,
+      scoped_refptr<base::SequencedTaskRunner> decoder_task_runner,
       DecodeDoneCB decode_done_cb,
       base::RepeatingCallback<void(const std::string&)> send_log_message_cb);
   ~VideoCaptureJpegDecoderImpl() override;
@@ -78,7 +78,7 @@ class CAPTURE_EXPORT VideoCaptureJpegDecoderImpl
   void DestroyDecoderOnIOThread(base::WaitableEvent* event);
 
   MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory_;
-  scoped_refptr<base::SingleThreadTaskRunner> decoder_task_runner_;
+  scoped_refptr<base::SequencedTaskRunner> decoder_task_runner_;
 
   // The underlying JPEG decode accelerator.
   std::unique_ptr<media::JpegDecodeAccelerator> decoder_;

media/mojo/clients/mojo_jpeg_decode_accelerator.cc

@@ -17,13 +17,13 @@
 namespace media {
 
 MojoJpegDecodeAccelerator::MojoJpegDecodeAccelerator(
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+    scoped_refptr<base::SequencedTaskRunner> io_task_runner,
     mojom::JpegDecodeAcceleratorPtrInfo jpeg_decoder)
     : io_task_runner_(std::move(io_task_runner)),
       jpeg_decoder_info_(std::move(jpeg_decoder)) {}
 
 MojoJpegDecodeAccelerator::~MojoJpegDecodeAccelerator() {
-  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
 }
 
 bool MojoJpegDecodeAccelerator::Initialize(
@@ -34,7 +34,7 @@ bool MojoJpegDecodeAccelerator::Initialize(
 
 void MojoJpegDecodeAccelerator::InitializeAsync(Client* client,
                                                 InitCB init_cb) {
-  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
 
   jpeg_decoder_.Bind(std::move(jpeg_decoder_info_));
 
@@ -50,7 +50,7 @@ void MojoJpegDecodeAccelerator::InitializeAsync(Client* client,
 void MojoJpegDecodeAccelerator::Decode(
     const BitstreamBuffer& bitstream_buffer,
     const scoped_refptr<VideoFrame>& video_frame) {
-  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
   DCHECK(jpeg_decoder_.is_bound());
 
   DCHECK(
@@ -86,7 +86,7 @@ void MojoJpegDecodeAccelerator::OnInitializeDone(
     InitCB init_cb,
     JpegDecodeAccelerator::Client* client,
     bool success) {
-  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
 
   if (success)
     client_ = client;
@@ -97,7 +97,7 @@ void MojoJpegDecodeAccelerator::OnInitializeDone(
 void MojoJpegDecodeAccelerator::OnDecodeAck(
     int32_t bitstream_buffer_id,
     ::media::JpegDecodeAccelerator::Error error) {
-  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
 
   if (!client_)
     return;
@@ -116,7 +116,7 @@ void MojoJpegDecodeAccelerator::OnDecodeAck(
 }
 
 void MojoJpegDecodeAccelerator::OnLostConnectionToJpegDecoder() {
-  DCHECK(io_task_runner_->BelongsToCurrentThread());
+  DCHECK(io_task_runner_->RunsTasksInCurrentSequence());
   OnDecodeAck(kInvalidBitstreamBufferId,
               ::media::JpegDecodeAccelerator::Error::PLATFORM_FAILURE);
 }

media/mojo/clients/mojo_jpeg_decode_accelerator.h

@@ -14,18 +14,18 @@
 #include "media/video/jpeg_decode_accelerator.h"
 
 namespace base {
-class SingleThreadTaskRunner;
+class SequencedTaskRunner;
 }
 
 namespace media {
 
 // A JpegDecodeAccelerator, for use in the browser process, that proxies to a
 // mojom::JpegDecodeAccelerator. Created on the owner's thread, otherwise
-// operating and deleted on the IO thread.
+// operating and deleted on |io_task_runner|.
 class MojoJpegDecodeAccelerator : public JpegDecodeAccelerator {
  public:
   MojoJpegDecodeAccelerator(
-      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
+      scoped_refptr<base::SequencedTaskRunner> io_task_runner,
       mojom::JpegDecodeAcceleratorPtrInfo jpeg_decoder);
   ~MojoJpegDecodeAccelerator() override;
 
@@ -46,8 +46,7 @@ class MojoJpegDecodeAccelerator : public JpegDecodeAccelerator {
                    ::media::JpegDecodeAccelerator::Error error);
   void OnLostConnectionToJpegDecoder();
 
-  // Browser IO task runner.
-  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+  scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
 
   Client* client_ = nullptr;
 

services/video_capture/device_factory_media_to_mojo_adapter.cc

@@ -79,10 +79,12 @@ DeviceFactoryMediaToMojoAdapter::ActiveDeviceEntry::operator=(
 DeviceFactoryMediaToMojoAdapter::DeviceFactoryMediaToMojoAdapter(
     std::unique_ptr<service_manager::ServiceContextRef> service_ref,
     std::unique_ptr<media::VideoCaptureSystem> capture_system,
-    media::MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory_callback)
+    media::MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory_callback,
+    scoped_refptr<base::SequencedTaskRunner> jpeg_decoder_task_runner)
     : service_ref_(std::move(service_ref)),
       capture_system_(std::move(capture_system)),
       jpeg_decoder_factory_callback_(std::move(jpeg_decoder_factory_callback)),
+      jpeg_decoder_task_runner_(std::move(jpeg_decoder_task_runner)),
       has_called_get_device_infos_(false),
       weak_factory_(this) {}
 
@@ -158,7 +160,7 @@ void DeviceFactoryMediaToMojoAdapter::CreateAndAddNewDevice(
   ActiveDeviceEntry device_entry;
   device_entry.device = std::make_unique<DeviceMediaToMojoAdapter>(
       service_ref_->Clone(), std::move(media_device),
-      jpeg_decoder_factory_callback_);
+      jpeg_decoder_factory_callback_, jpeg_decoder_task_runner_);
   device_entry.binding = std::make_unique<mojo::Binding<mojom::Device>>(
       device_entry.device.get(), std::move(device_request));
   device_entry.binding->set_connection_error_handler(base::Bind(

services/video_capture/device_factory_media_to_mojo_adapter.h

@@ -26,7 +26,8 @@ class DeviceFactoryMediaToMojoAdapter : public mojom::DeviceFactory {
   DeviceFactoryMediaToMojoAdapter(
       std::unique_ptr<service_manager::ServiceContextRef> service_ref,
       std::unique_ptr<media::VideoCaptureSystem> capture_system,
-      media::MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory_callback);
+      media::MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory_callback,
+      scoped_refptr<base::SequencedTaskRunner> jpeg_decoder_task_runner);
   ~DeviceFactoryMediaToMojoAdapter() override;
 
   // mojom::DeviceFactory implementation.
@@ -65,6 +66,7 @@ class DeviceFactoryMediaToMojoAdapter : public mojom::DeviceFactory {
   const std::unique_ptr<media::VideoCaptureSystem> capture_system_;
   const media::MojoJpegDecodeAcceleratorFactoryCB
       jpeg_decoder_factory_callback_;
+  scoped_refptr<base::SequencedTaskRunner> jpeg_decoder_task_runner_;
   std::map<std::string, ActiveDeviceEntry> active_devices_by_id_;
   bool has_called_get_device_infos_;
 

services/video_capture/device_factory_provider_impl.cc

@@ -4,6 +4,8 @@
 
 #include "services/video_capture/device_factory_provider_impl.h"
 
+#include "base/task_scheduler/post_task.h"
+#include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
 #include "media/capture/video/fake_video_capture_device_factory.h"
 #include "media/capture/video/video_capture_buffer_pool.h"
 #include "media/capture/video/video_capture_buffer_tracker.h"
@@ -15,26 +17,102 @@
 
 namespace video_capture {
 
+// Intended usage of this class is to instantiate on any sequence, and then
+// operate and release the instance on the task runner exposed via
+// GetTaskRunner() via WeakPtrs provided via GetWeakPtr(). To this end,
+// GetTaskRunner() and GetWeakPtr() can be called from any sequence, typically
+// the same as the one calling the constructor.
+class DeviceFactoryProviderImpl::GpuDependenciesContext {
+ public:
+  GpuDependenciesContext() : weak_factory_for_gpu_io_thread_(this) {
+    gpu_io_task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
+        {base::TaskPriority::BACKGROUND, base::MayBlock()});
+  }
+
+  ~GpuDependenciesContext() {
+    DCHECK(gpu_io_task_runner_->RunsTasksInCurrentSequence());
+  }
+
+  base::WeakPtr<GpuDependenciesContext> GetWeakPtr() {
+    return weak_factory_for_gpu_io_thread_.GetWeakPtr();
+  }
+
+  scoped_refptr<base::SequencedTaskRunner> GetTaskRunner() {
+    return gpu_io_task_runner_;
+  }
+
+  gpu::GpuMemoryBufferManager* GetGpuMemoryBufferManager() {
+    return gpu_memory_buffer_manager_.get();
+  }
+
+  void InjectGpuDependencies(
+      ui::mojom::GpuMemoryBufferFactoryPtrInfo memory_buffer_factory_info,
+      mojom::AcceleratorFactoryPtrInfo accelerator_factory_info) {
+    DCHECK(gpu_io_task_runner_->RunsTasksInCurrentSequence());
+    accelerator_factory_.Bind(std::move(accelerator_factory_info));
+
+// Since the instance of ui::ClientGpuMemoryBufferManager seems kind of
+// expensive, we only create it on platforms where it gets actually used.
+#if defined(OS_CHROMEOS)
+    ui::mojom::GpuMemoryBufferFactoryPtr memory_buffer_factory;
+    memory_buffer_factory.Bind(std::move(memory_buffer_factory_info));
+    gpu_memory_buffer_manager_ =
+        std::make_unique<ui::ClientGpuMemoryBufferManager>(
+            std::move(memory_buffer_factory));
+#endif
+  }
+
+  void CreateJpegDecodeAccelerator(
+      media::mojom::JpegDecodeAcceleratorRequest request) {
+    DCHECK(gpu_io_task_runner_->RunsTasksInCurrentSequence());
+    if (!accelerator_factory_)
+      return;
+    accelerator_factory_->CreateJpegDecodeAccelerator(std::move(request));
+  }
+
+  void CreateJpegEncodeAccelerator(
+      media::mojom::JpegEncodeAcceleratorRequest request) {
+    DCHECK(gpu_io_task_runner_->RunsTasksInCurrentSequence());
+    if (!accelerator_factory_)
+      return;
+    accelerator_factory_->CreateJpegEncodeAccelerator(std::move(request));
+  }
+
+ private:
+  // Task runner for operating |accelerator_factory_| and
+  // |gpu_memory_buffer_manager_| on. This must be a different thread from the
+  // main service thread in order to avoid a deadlock during shutdown where
+  // the main service thread joins a video capture device thread that, in turn,
+  // will try to post the release of the jpeg decoder to the thread it is
+  // operated on.
+  scoped_refptr<base::SequencedTaskRunner> gpu_io_task_runner_;
+  mojom::AcceleratorFactoryPtr accelerator_factory_;
+  std::unique_ptr<gpu::GpuMemoryBufferManager> gpu_memory_buffer_manager_;
+  base::WeakPtrFactory<GpuDependenciesContext> weak_factory_for_gpu_io_thread_;
+};
+
 DeviceFactoryProviderImpl::DeviceFactoryProviderImpl(
     std::unique_ptr<service_manager::ServiceContextRef> service_ref,
     base::Callback<void(float)> set_shutdown_delay_cb)
     : service_ref_(std::move(service_ref)),
-      set_shutdown_delay_cb_(std::move(set_shutdown_delay_cb)),
-      weak_factory_(this) {}
+      set_shutdown_delay_cb_(std::move(set_shutdown_delay_cb)) {}
 
-DeviceFactoryProviderImpl::~DeviceFactoryProviderImpl() {}
+DeviceFactoryProviderImpl::~DeviceFactoryProviderImpl() {
+  factory_bindings_.CloseAllBindings();
+  device_factory_.reset();
+  gpu_dependencies_context_->GetTaskRunner()->DeleteSoon(
+      FROM_HERE, std::move(gpu_dependencies_context_));
+}
 
 void DeviceFactoryProviderImpl::InjectGpuDependencies(
     ui::mojom::GpuMemoryBufferFactoryPtr memory_buffer_factory,
     mojom::AcceleratorFactoryPtr accelerator_factory) {
-  accelerator_factory_ = std::move(accelerator_factory);
-// Since the instance of ui::ClientGpuMemoryBufferManager seems kind of
-// expensive, we only create it on platforms where it gets actually used.
-#if defined(OS_CHROMEOS)
-  gpu_memory_buffer_manager_ =
-      std::make_unique<ui::ClientGpuMemoryBufferManager>(
-          std::move(memory_buffer_factory));
-#endif
+  LazyInitializeGpuDependenciesContext();
+  gpu_dependencies_context_->GetTaskRunner()->PostTask(
+      FROM_HERE, base::BindOnce(&GpuDependenciesContext::InjectGpuDependencies,
+                                gpu_dependencies_context_->GetWeakPtr(),
+                                memory_buffer_factory.PassInterface(),
+                                accelerator_factory.PassInterface()));
 }
 
 void DeviceFactoryProviderImpl::ConnectToDeviceFactory(
@@ -47,23 +125,31 @@ void DeviceFactoryProviderImpl::SetShutdownDelayInSeconds(float seconds) {
   set_shutdown_delay_cb_.Run(seconds);
 }
 
+void DeviceFactoryProviderImpl::LazyInitializeGpuDependenciesContext() {
+  if (!gpu_dependencies_context_)
+    gpu_dependencies_context_ = std::make_unique<GpuDependenciesContext>();
+}
+
 void DeviceFactoryProviderImpl::LazyInitializeDeviceFactory() {
   if (device_factory_)
     return;
 
+  LazyInitializeGpuDependenciesContext();
+
   // Create the platform-specific device factory.
   // The task runner passed to CreateFactory is used for things that need to
   // happen on a "UI thread equivalent", e.g. obtaining screen rotation on
   // Chrome OS.
   std::unique_ptr<media::VideoCaptureDeviceFactory> media_device_factory =
       media::VideoCaptureDeviceFactory::CreateFactory(
-          base::ThreadTaskRunnerHandle::Get(), gpu_memory_buffer_manager_.get(),
+          base::ThreadTaskRunnerHandle::Get(),
+          gpu_dependencies_context_->GetGpuMemoryBufferManager(),
           base::BindRepeating(
-              &DeviceFactoryProviderImpl::CreateJpegDecodeAccelerator,
-              weak_factory_.GetWeakPtr()),
+              &GpuDependenciesContext::CreateJpegDecodeAccelerator,
+              gpu_dependencies_context_->GetWeakPtr()),
           base::BindRepeating(
-              &DeviceFactoryProviderImpl::CreateJpegEncodeAccelerator,
-              weak_factory_.GetWeakPtr()));
+              &GpuDependenciesContext::CreateJpegEncodeAccelerator,
+              gpu_dependencies_context_->GetWeakPtr()));
   auto video_capture_system = std::make_unique<media::VideoCaptureSystemImpl>(
       std::move(media_device_factory));
 
@@ -72,22 +158,9 @@ void DeviceFactoryProviderImpl::LazyInitializeDeviceFactory() {
       std::make_unique<DeviceFactoryMediaToMojoAdapter>(
           service_ref_->Clone(), std::move(video_capture_system),
           base::BindRepeating(
-              &DeviceFactoryProviderImpl::CreateJpegDecodeAccelerator,
-              weak_factory_.GetWeakPtr())));
-}
-
-void DeviceFactoryProviderImpl::CreateJpegDecodeAccelerator(
-    media::mojom::JpegDecodeAcceleratorRequest request) {
-  if (!accelerator_factory_)
-    return;
-  accelerator_factory_->CreateJpegDecodeAccelerator(std::move(request));
-}
-
-void DeviceFactoryProviderImpl::CreateJpegEncodeAccelerator(
-    media::mojom::JpegEncodeAcceleratorRequest request) {
-  if (!accelerator_factory_)
-    return;
-  accelerator_factory_->CreateJpegEncodeAccelerator(std::move(request));
+              &GpuDependenciesContext::CreateJpegDecodeAccelerator,
+              gpu_dependencies_context_->GetWeakPtr()),
+          gpu_dependencies_context_->GetTaskRunner()));
 }
 
 }  // namespace video_capture

services/video_capture/device_factory_provider_impl.h

@@ -7,7 +7,7 @@
 
 #include <memory>
 
-#include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
+#include "base/threading/thread.h"
 #include "media/capture/video/video_capture_jpeg_decoder.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -32,20 +32,16 @@ class DeviceFactoryProviderImpl : public mojom::DeviceFactoryProvider {
   void SetShutdownDelayInSeconds(float seconds) override;
 
  private:
+  class GpuDependenciesContext;
+
+  void LazyInitializeGpuDependenciesContext();
   void LazyInitializeDeviceFactory();
-  void CreateJpegDecodeAccelerator(
-      media::mojom::JpegDecodeAcceleratorRequest request);
-  void CreateJpegEncodeAccelerator(
-      media::mojom::JpegEncodeAcceleratorRequest request);
 
   mojo::BindingSet<mojom::DeviceFactory> factory_bindings_;
   std::unique_ptr<mojom::DeviceFactory> device_factory_;
-
   const std::unique_ptr<service_manager::ServiceContextRef> service_ref_;
-  mojom::AcceleratorFactoryPtr accelerator_factory_;
-  std::unique_ptr<gpu::GpuMemoryBufferManager> gpu_memory_buffer_manager_;
+  std::unique_ptr<GpuDependenciesContext> gpu_dependencies_context_;
   base::Callback<void(float)> set_shutdown_delay_cb_;
-  base::WeakPtrFactory<DeviceFactoryProviderImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceFactoryProviderImpl);
 };