Break tight coupling of VideoCaptureDeviceClient to renderer_host

content/browser/renderer_host/media/video_capture_device_client.cc

 // Copyright 2015 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 "content/browser/renderer_host/media/video_capture_device_client.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "content/browser/renderer_host/media/video_capture_buffer_pool.h"
 #include "content/browser/renderer_host/media/video_capture_controller.h"
 #include "content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h"
-#include "content/public/browser/browser_thread.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_switches.h"
 #include "media/base/video_capture_types.h"
 #include "media/base/video_frame.h"
 #include "third_party/libyuv/include/libyuv.h"
 
 using media::VideoCaptureFormat;
 using media::VideoFrame;
 using media::VideoFrameMetadata;
 
 namespace content {
 
 // Class combining a Client::Buffer interface implementation and a pool buffer
 // implementation to guarantee proper cleanup on destruction on our side.
 class AutoReleaseBuffer : public media::VideoCaptureDevice::Client::Buffer {
  public:
-  AutoReleaseBuffer(const scoped_refptr<VideoCaptureBufferPool>& pool,
+  AutoReleaseBuffer(const scoped_refptr<VideoCaptureBufferPoolInterface>& pool,
                     int buffer_id)
       : id_(buffer_id),
         pool_(pool),
         buffer_handle_(pool_->GetBufferHandle(buffer_id)) {
     DCHECK(pool_.get());
   }
   int id() const override { return id_; }
   gfx::Size dimensions() const override { return buffer_handle_->dimensions(); }
   size_t mapped_size() const override { return buffer_handle_->mapped_size(); }
   void* data(int plane) override { return buffer_handle_->data(plane); }
   ClientBuffer AsClientBuffer(int plane) override {
     return buffer_handle_->AsClientBuffer(plane);
   }
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
   base::FileDescriptor AsPlatformFile() override {
     return buffer_handle_->AsPlatformFile();
   }
 #endif
 
  private:
   ~AutoReleaseBuffer() override { pool_->RelinquishProducerReservation(id_); }
 
   const int id_;
-  const scoped_refptr<VideoCaptureBufferPool> pool_;
-  const std::unique_ptr<VideoCaptureBufferPool::BufferHandle> buffer_handle_;
+  const scoped_refptr<VideoCaptureBufferPoolInterface> pool_;
+  const std::unique_ptr<VideoCaptureBufferPoolBufferHandle> buffer_handle_;
 };
 
 VideoCaptureDeviceClient::VideoCaptureDeviceClient(
-    const base::WeakPtr<VideoCaptureController>& controller,
-    const scoped_refptr<VideoCaptureBufferPool>& buffer_pool)
-    : controller_(controller),
+    std::unique_ptr<VideoFrameReceiver> receiver,
+    const scoped_refptr<VideoCaptureBufferPoolInterface>& buffer_pool,
+    std::unique_ptr<VideoCaptureJpegDecoderFactory> jpeg_decoder_factory,
+    const base::Closure& check_thread_closure)
+    : receiver_(std::move(receiver)),
+      jpeg_decoder_factory_(std::move(jpeg_decoder_factory)),
       external_jpeg_decoder_initialized_(false),
       buffer_pool_(buffer_pool),
       use_gpu_memory_buffers_(base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kUseGpuMemoryBuffersForCapture)),
       last_captured_pixel_format_(media::PIXEL_FORMAT_UNKNOWN) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  check_thread_closure.Run();

[mcasas, 2016/09/07 22:43:40]

I'd rather pass a const SingleThreadTaskRunner& where
this class is supposed to operate, cache it and then
DCHECK(task_runner_->BelongsToCurrentThread())
in all methods where it makes sense.

[chfremer, 2016/09/08 17:28:51]

Please explain why you would prefer doing that.
I don't see any advantage, but I see a disadvantage. The disadvantage is that it
would create a dependency on a rather complex concrete class
SingleThreadTaskRunner where none is needed. The VideoCaptureDeviceClient class
should not need to know or care about threads. All it needs to know is that
there is a method it is supposed to call at a certain time.

[mcasas, 2016/09/08 20:25:45]

But SingleThreadTaskRunners are pervasively passed 
around the codebase to check for thread
affinity of methods and/or PostTask()s ([1] gives 160); 
OTOH, a bound base::Callback is more opaque to me, 
since here I can't tell what's exactly being done.

[1] grep -rn "const scoped_refptr<base::SingleThreadTaskRunner>&" content/ | wc
-l

[chfremer, 2016/09/09 00:55:37]

Being more opaque here is actually a good thing, since as a reader of the code
for this class you should not have to know or care what exactly this call is
doing. Using a name like |thread_checker| or |check_thread_closure| for the
variable should reveal enough about the intended purpose of the call.

Of course I am willing to conform with the rest of the code base for consistency
purposes, so I'll make the change. But I still think it is bad to depend on more
than needed.

[chfremer, 2016/09/09 16:48:06]

After looking at it one more time, I decided to simply remove the thread check
call in this constructor. This makes sense, because the VCDClient class does not
care on which thread it is created, since it is legal to use it on other threads
than the creating one.

And if we really would care that construction happens on the Browser IO Thread,
we still have a check in the method that calls the constructor in
VideoCaptureController.

 }
 
 VideoCaptureDeviceClient::~VideoCaptureDeviceClient() {
   // This should be on the platform auxiliary thread since
   // |external_jpeg_decoder_| need to be destructed on the same thread as
   // OnIncomingCapturedData.
 }
 
 void VideoCaptureDeviceClient::OnIncomingCapturedData(
     const uint8_t* data,
     int length,
     const VideoCaptureFormat& frame_format,
     int rotation,
     base::TimeTicks reference_time,
     base::TimeDelta timestamp) {
   TRACE_EVENT0("video", "VideoCaptureDeviceClient::OnIncomingCapturedData");
   DCHECK_EQ(media::PIXEL_STORAGE_CPU, frame_format.pixel_storage);
 
   if (last_captured_pixel_format_ != frame_format.pixel_format) {
     OnLog("Pixel format: " +
           media::VideoPixelFormatToString(frame_format.pixel_format));
     last_captured_pixel_format_ = frame_format.pixel_format;
 
     if (frame_format.pixel_format == media::PIXEL_FORMAT_MJPEG &&
         !external_jpeg_decoder_initialized_) {
       external_jpeg_decoder_initialized_ = true;

[mcasas, 2016/09/07 22:43:40]

I think we can remove this flag and test
|external_jpeg_decoder_| instead.

[chfremer, 2016/09/08 17:28:51]

Not sure we can do that without changing behavior.
Note that there is a call 

  external_jpeg_decoder_.reset();

at line 228, which means it is possible that external_jpeg_decoder_ is null
while external_jpeg_decoder_initialized_ == true.

[mcasas, 2016/09/08 20:25:45]

I see, forget it then.

[chfremer, 2016/09/09 00:55:37]

Acknowledged.

-      external_jpeg_decoder_.reset(new VideoCaptureGpuJpegDecoder(base::Bind(
-          &VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread,
-          controller_)));
+      external_jpeg_decoder_ = jpeg_decoder_factory_->CreateJpegDecoder();
       external_jpeg_decoder_->Initialize();
     }
   }
 
   if (!frame_format.IsValid())
     return;
 
   // |chopped_{width,height} and |new_unrotated_{width,height}| are the lowest
   // bit decomposition of {width, height}, grabbing the odd and even parts.
   const int chopped_width = frame_format.frame_size.width() & 1;
@@ skipping 155 matching lines @@
     const gfx::Size& frame_size,
     media::VideoPixelFormat pixel_format,
     media::VideoPixelStorage pixel_storage) {
   DCHECK_GT(frame_size.width(), 0);
   DCHECK_GT(frame_size.height(), 0);
   // Currently, only I420 pixel format is supported.
   DCHECK_EQ(media::PIXEL_FORMAT_I420, pixel_format);
 
   // TODO(mcasas): For PIXEL_STORAGE_GPUMEMORYBUFFER, find a way to indicate if
   // it's a ShMem GMB or a DmaBuf GMB.
-  int buffer_id_to_drop = VideoCaptureBufferPool::kInvalidId;
+  int buffer_id_to_drop = VideoCaptureBufferPoolInterface::kInvalidId;
   const int buffer_id = buffer_pool_->ReserveForProducer(
       frame_size, pixel_format, pixel_storage, &buffer_id_to_drop);
-  if (buffer_id_to_drop != VideoCaptureBufferPool::kInvalidId) {
-    BrowserThread::PostTask(BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&VideoCaptureController::DoBufferDestroyedOnIOThread,
-                   controller_, buffer_id_to_drop));
+  if (buffer_id_to_drop != VideoCaptureBufferPoolInterface::kInvalidId) {
+    receiver_->OnBufferDestroyed(buffer_id_to_drop);
   }

[mcasas, 2016/09/07 22:43:40]

nit: Remove {} 

[chfremer, 2016/09/08 17:28:51]

Done.

-  if (buffer_id == VideoCaptureBufferPool::kInvalidId)
+  if (buffer_id == VideoCaptureBufferPoolInterface::kInvalidId)
     return nullptr;
   return base::WrapUnique<Buffer>(
       new AutoReleaseBuffer(buffer_pool_, buffer_id));
 }
 
 void VideoCaptureDeviceClient::OnIncomingCapturedBuffer(
     std::unique_ptr<Buffer> buffer,
     const VideoCaptureFormat& frame_format,
     base::TimeTicks reference_time,
     base::TimeDelta timestamp) {
@@ skipping 29 matching lines @@
   frame->metadata()->SetDouble(media::VideoFrameMetadata::FRAME_RATE,
                                frame_format.frame_rate);
   frame->metadata()->SetTimeTicks(media::VideoFrameMetadata::REFERENCE_TIME,
                                   reference_time);
   OnIncomingCapturedVideoFrame(std::move(buffer), frame);
 }
 
 void VideoCaptureDeviceClient::OnIncomingCapturedVideoFrame(
     std::unique_ptr<Buffer> buffer,
     const scoped_refptr<VideoFrame>& frame) {
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(
-          &VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread,
-          controller_, base::Passed(&buffer), frame));
+  receiver_->OnIncomingCapturedVideoFrame(std::move(buffer), frame);
 }
 
 std::unique_ptr<media::VideoCaptureDevice::Client::Buffer>
 VideoCaptureDeviceClient::ResurrectLastOutputBuffer(
     const gfx::Size& dimensions,
     media::VideoPixelFormat format,
     media::VideoPixelStorage storage) {
   const int buffer_id =
       buffer_pool_->ResurrectLastForProducer(dimensions, format, storage);
-  if (buffer_id == VideoCaptureBufferPool::kInvalidId)
+  if (buffer_id == VideoCaptureBufferPoolInterface::kInvalidId)
     return nullptr;
   return base::WrapUnique<Buffer>(
       new AutoReleaseBuffer(buffer_pool_, buffer_id));
 }
 
 void VideoCaptureDeviceClient::OnError(
     const tracked_objects::Location& from_here,
     const std::string& reason) {
   const std::string log_message = base::StringPrintf(
       "error@ %s, %s, OS message: %s", from_here.ToString().c_str(),
       reason.c_str(),
       logging::SystemErrorCodeToString(logging::GetLastSystemErrorCode())
           .c_str());
   DLOG(ERROR) << log_message;
   OnLog(log_message);
-  BrowserThread::PostTask(BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&VideoCaptureController::DoErrorOnIOThread, controller_));
+  receiver_->OnError();
 }
 
 void VideoCaptureDeviceClient::OnLog(
     const std::string& message) {
-  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                          base::Bind(&VideoCaptureController::DoLogOnIOThread,
-                                     controller_, message));
+  receiver_->OnLog(message);
 }
 
 double VideoCaptureDeviceClient::GetBufferPoolUtilization() const {
-  // VideoCaptureBufferPool::GetBufferPoolUtilization() is thread-safe.
+  // VideoCaptureBufferPoolInterface::GetBufferPoolUtilization() is thread-safe.
   return buffer_pool_->GetBufferPoolUtilization();
 }
 
 std::unique_ptr<media::VideoCaptureDevice::Client::Buffer>
 VideoCaptureDeviceClient::ReserveI420OutputBuffer(
     const gfx::Size& dimensions,
     media::VideoPixelStorage storage,
     uint8_t** y_plane_data,
     uint8_t** u_plane_data,
     uint8_t** v_plane_data) {
@@ skipping 29 matching lines @@
           reinterpret_cast<uint8_t*>(buffer->data(VideoFrame::kUPlane));
       *v_plane_data =
           reinterpret_cast<uint8_t*>(buffer->data(VideoFrame::kVPlane));
       return buffer;
   }
   NOTREACHED();
   return std::unique_ptr<Buffer>();
 }
 
 }  // namespace content