[Video Capture] Fix deadlock on shutdown when using accelerated MJPEG decoding

The destructor of class VideoCaptureJpegDecoderImpl was blocking the
current thread, which could lead to a deadlock described in
https://bugs.chromium.org/p/chromium/issues/detail?id=921694#c6.
This deadlock has shown up as test flakiness.

This CL removes the need for blocking the thread in the destructor
by requiring that instances are always destroyed on the thread we
would otherwise have to wait for.

This CL also reenables test cases that were disabled because of the
flakiness.

Test: content_browsertests --gtest_filter=*ReceiveFramesFromFakeCaptureDevice*
Bug: 921694
Change-Id: I06b960c8e9cf749b94ce7785a3726884fd47a37d
Reviewed-on: https://chromium-review.googlesource.com/c/1423638
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624957}

content/browser/renderer_host/media/in_process_video_capture_device_launcher.cc

@@ -22,6 +22,7 @@
 #include "media/base/media_switches.h"
 #include "media/capture/video/fake_video_capture_device.h"
 #include "media/capture/video/fake_video_capture_device_factory.h"
+#include "media/capture/video/scoped_video_capture_jpeg_decoder.h"
 #include "media/capture/video/video_capture_buffer_pool_impl.h"
 #include "media/capture/video/video_capture_buffer_tracker_factory_impl.h"
 #include "media/capture/video/video_capture_device_client.h"
@@ -48,12 +49,15 @@ namespace {
 std::unique_ptr<media::VideoCaptureJpegDecoder> CreateGpuJpegDecoder(
     media::VideoCaptureJpegDecoder::DecodeDoneCB decode_done_cb,
     base::Callback<void(const std::string&)> send_log_message_cb) {
-  return std::make_unique<media::VideoCaptureJpegDecoderImpl>(
-      base::BindRepeating(
-          &content::VideoCaptureDependencies::CreateJpegDecodeAccelerator),
-      base::CreateSingleThreadTaskRunnerWithTraits(
-          {content::BrowserThread::IO}),
-      std::move(decode_done_cb), std::move(send_log_message_cb));
+  auto io_task_runner = base::CreateSingleThreadTaskRunnerWithTraits(
+      {content::BrowserThread::IO});
+  return std::make_unique<media::ScopedVideoCaptureJpegDecoder>(
+      std::make_unique<media::VideoCaptureJpegDecoderImpl>(
+          base::BindRepeating(
+              &content::VideoCaptureDependencies::CreateJpegDecodeAccelerator),
+          io_task_runner, std::move(decode_done_cb),
+          std::move(send_log_message_cb)),
+      io_task_runner);
 }
 
 // The maximum number of video frame buffers in-flight at any one time. This

content/browser/renderer_host/media/video_capture_browsertest.cc

@@ -244,8 +244,7 @@ IN_PROC_BROWSER_TEST_P(VideoCaptureBrowserTest, StartAndImmediatelyStop) {
 }
 
 // Flaky on MSAN. https://crbug.com/840294
-// Flaky on Linux Tests (dbg). https://crbug.com/921694
-#if defined(MEMORY_SANITIZER) || defined(OS_LINUX)
+#if defined(MEMORY_SANITIZER)
 #define MAYBE_ReceiveFramesFromFakeCaptureDevice \
   DISABLED_ReceiveFramesFromFakeCaptureDevice
 #else

media/capture/BUILD.gn

@@ -97,6 +97,8 @@ jumbo_component("capture_lib") {
     "video/create_video_capture_device_factory.cc",
     "video/create_video_capture_device_factory.h",
     "video/scoped_buffer_pool_reservation.h",
+    "video/scoped_video_capture_jpeg_decoder.cc",
+    "video/scoped_video_capture_jpeg_decoder.h",
     "video/shared_memory_buffer_tracker.cc",
     "video/shared_memory_buffer_tracker.h",
     "video/shared_memory_handle_provider.cc",

media/capture/video/scoped_video_capture_jpeg_decoder.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/capture/video/scoped_video_capture_jpeg_decoder.h"
+
+namespace media {
+
+ScopedVideoCaptureJpegDecoder::ScopedVideoCaptureJpegDecoder(
+    std::unique_ptr<VideoCaptureJpegDecoder> decoder,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : decoder_(std::move(decoder)), task_runner_(std::move(task_runner)) {}
+
+ScopedVideoCaptureJpegDecoder::~ScopedVideoCaptureJpegDecoder() {
+  task_runner_->DeleteSoon(FROM_HERE, std::move(decoder_));
+}
+
+// Implementation of VideoCaptureJpegDecoder:
+void ScopedVideoCaptureJpegDecoder::Initialize() {
+  decoder_->Initialize();
+}
+
+VideoCaptureJpegDecoder::STATUS ScopedVideoCaptureJpegDecoder::GetStatus()
+    const {
+  return decoder_->GetStatus();
+}
+
+void ScopedVideoCaptureJpegDecoder::DecodeCapturedData(
+    const uint8_t* data,
+    size_t in_buffer_size,
+    const media::VideoCaptureFormat& frame_format,
+    base::TimeTicks reference_time,
+    base::TimeDelta timestamp,
+    media::VideoCaptureDevice::Client::Buffer out_buffer) {
+  decoder_->DecodeCapturedData(data, in_buffer_size, frame_format,
+                               reference_time, timestamp,
+                               std::move(out_buffer));
+}
+
+}  // namespace media

media/capture/video/scoped_video_capture_jpeg_decoder.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef MEDIA_CAPTURE_VIDEO_SCOPED_VIDEO_CAPTURE_JPEG_DECODER_H_
+#define MEDIA_CAPTURE_VIDEO_SCOPED_VIDEO_CAPTURE_JPEG_DECODER_H_
+
+#include <memory>
+
+#include "base/sequenced_task_runner.h"
+#include "media/capture/capture_export.h"
+#include "media/capture/video/video_capture_jpeg_decoder.h"
+
+namespace media {
+
+// Decorator for media::VideoCaptureJpegDecoder that destroys the decorated
+// instance on a given task runner.
+class CAPTURE_EXPORT ScopedVideoCaptureJpegDecoder
+    : public VideoCaptureJpegDecoder {
+ public:
+  ScopedVideoCaptureJpegDecoder(
+      std::unique_ptr<VideoCaptureJpegDecoder> decoder,
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
+
+  ~ScopedVideoCaptureJpegDecoder() override;
+
+  // Implementation of VideoCaptureJpegDecoder:
+  void Initialize() override;
+  STATUS GetStatus() const override;
+  void DecodeCapturedData(
+      const uint8_t* data,
+      size_t in_buffer_size,
+      const media::VideoCaptureFormat& frame_format,
+      base::TimeTicks reference_time,
+      base::TimeDelta timestamp,
+      media::VideoCaptureDevice::Client::Buffer out_buffer) override;
+
+ private:
+  std::unique_ptr<VideoCaptureJpegDecoder> decoder_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+};
+
+}  // namespace media
+
+#endif  // MEDIA_CAPTURE_VIDEO_SCOPED_VIDEO_CAPTURE_JPEG_DECODER_H_

media/capture/video/video_capture_device_client.cc

@@ -113,10 +113,6 @@ VideoCaptureDeviceClient::VideoCaptureDeviceClient(
 }
 
 VideoCaptureDeviceClient::~VideoCaptureDeviceClient() {
-  // This should be on the platform auxiliary thread since
-  // |external_jpeg_decoder_| need to be destructed on the same thread as
-  // OnIncomingCapturedData.
-
   for (int buffer_id : buffer_ids_known_by_receiver_)
     receiver_->OnBufferRetired(buffer_id);
 }

media/capture/video/video_capture_jpeg_decoder_impl.cc

@@ -25,27 +25,7 @@ VideoCaptureJpegDecoderImpl::VideoCaptureJpegDecoderImpl(
       weak_ptr_factory_(this) {}
 
 VideoCaptureJpegDecoderImpl::~VideoCaptureJpegDecoderImpl() {
-  // |this| was set as |decoder_|'s client. |decoder_| has to be deleted on
-  // |decoder_task_runner_| before this destructor returns to ensure that it
-  // doesn't call back into its client.
-
-  if (!decoder_)
-    return;
-
-  if (decoder_task_runner_->RunsTasksInCurrentSequence()) {
-    decoder_.reset();
-    return;
-  }
-
-  base::WaitableEvent event(base::WaitableEvent::ResetPolicy::MANUAL,
-                            base::WaitableEvent::InitialState::NOT_SIGNALED);
-  // base::Unretained is safe because |this| will be valid until |event|
-  // is signaled.
-  decoder_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(&VideoCaptureJpegDecoderImpl::DestroyDecoderOnIOThread,
-                     base::Unretained(this), &event));
-  event.Wait();
+  DCHECK(decoder_task_runner_->RunsTasksInCurrentSequence());
 }
 
 void VideoCaptureJpegDecoderImpl::Initialize() {
@@ -61,8 +41,7 @@ void VideoCaptureJpegDecoderImpl::Initialize() {
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
-VideoCaptureJpegDecoderImpl::STATUS VideoCaptureJpegDecoderImpl::GetStatus()
-    const {
+VideoCaptureJpegDecoder::STATUS VideoCaptureJpegDecoderImpl::GetStatus() const {
   base::AutoLock lock(lock_);
   return decoder_status_;
 }

media/capture/video/video_capture_jpeg_decoder_impl.h

@@ -21,10 +21,6 @@
 #include "media/capture/video/video_capture_jpeg_decoder.h"
 #include "media/mojo/clients/mojo_jpeg_decode_accelerator.h"
 
-namespace base {
-class WaitableEvent;
-}
-
 namespace media {
 
 // Implementation of media::VideoCaptureJpegDecoder that delegates to a
@@ -34,13 +30,12 @@ namespace media {
 // is invoked. Until |decode_done_cb_| is invoked, subsequent calls to
 // DecodeCapturedData() are ignored.
 // The given |decoder_task_runner| must allow blocking on |lock_|.
+// Instances must be destroyed on |decoder_task_runner|, but the
+// media::VideoCaptureJpegDecoder methods may be called from any thread.
 class CAPTURE_EXPORT VideoCaptureJpegDecoderImpl
-    : public media::VideoCaptureJpegDecoder,
-      public media::JpegDecodeAccelerator::Client {
+    : public VideoCaptureJpegDecoder,
+      public JpegDecodeAccelerator::Client {
  public:
-  // |decode_done_cb| is called on the IO thread when decode succeeds. This can
-  // be on any thread. |decode_done_cb| is never called after
-  // VideoCaptureGpuJpegDecoder is destroyed.
   VideoCaptureJpegDecoderImpl(
       MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory,
       scoped_refptr<base::SequencedTaskRunner> decoder_task_runner,
@@ -60,7 +55,7 @@ class CAPTURE_EXPORT VideoCaptureJpegDecoderImpl
       media::VideoCaptureDevice::Client::Buffer out_buffer) override;
 
   // JpegDecodeAccelerator::Client implementation.
-  // These will be called on IO thread.
+  // These will be called on |decoder_task_runner|.
   void VideoFrameReady(int32_t buffer_id) override;
   void NotifyError(int32_t buffer_id,
                    media::JpegDecodeAccelerator::Error error) override;

services/video_capture/device_media_to_mojo_adapter.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "media/base/bind_to_current_loop.h"
+#include "media/capture/video/scoped_video_capture_jpeg_decoder.h"
 #include "media/capture/video/video_capture_buffer_pool_impl.h"
 #include "media/capture/video/video_capture_buffer_tracker_factory_impl.h"
 #include "media/capture/video/video_capture_jpeg_decoder_impl.h"
@@ -20,9 +21,11 @@ std::unique_ptr<media::VideoCaptureJpegDecoder> CreateGpuJpegDecoder(
     media::MojoJpegDecodeAcceleratorFactoryCB jpeg_decoder_factory_callback,
     media::VideoCaptureJpegDecoder::DecodeDoneCB decode_done_cb,
     base::RepeatingCallback<void(const std::string&)> send_log_message_cb) {
-  return std::make_unique<media::VideoCaptureJpegDecoderImpl>(
-      jpeg_decoder_factory_callback, std::move(decoder_task_runner),
-      std::move(decode_done_cb), std::move(send_log_message_cb));
+  return std::make_unique<media::ScopedVideoCaptureJpegDecoder>(
+      std::make_unique<media::VideoCaptureJpegDecoderImpl>(
+          jpeg_decoder_factory_callback, decoder_task_runner,
+          std::move(decode_done_cb), std::move(send_log_message_cb)),
+      decoder_task_runner);
 }
 
 }  // anonymous namespace