Break tight coupling of VideoCaptureDeviceClient to renderer_host

content/browser/renderer_host/media/video_capture_device_client.cc

Index: content/browser/renderer_host/media/video_capture_device_client.cc
diff --git a/content/browser/renderer_host/media/video_capture_device_client.cc b/content/browser/renderer_host/media/video_capture_device_client.cc
index cd39c5fa750dc48da1fc151b6c00d2f4b628e120..cf25ea38a351cd7ee0f50a674cc47e0ebe104602 100644
--- a/content/browser/renderer_host/media/video_capture_device_client.cc
+++ b/content/browser/renderer_host/media/video_capture_device_client.cc
@@ -17,7 +17,6 @@
 #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"
@@ -34,7 +33,7 @@ namespace content {
 // 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),
@@ -58,20 +57,23 @@ class AutoReleaseBuffer : public media::VideoCaptureDevice::Client::Buffer {
   ~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() {
@@ -98,9 +100,7 @@ void VideoCaptureDeviceClient::OnIncomingCapturedData(
     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();
     }
   }
@@ -276,16 +276,13 @@ VideoCaptureDeviceClient::ReserveOutputBuffer(
 
   // 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));
@@ -335,11 +332,7 @@ void VideoCaptureDeviceClient::OnIncomingCapturedBuffer(
 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>
@@ -349,7 +342,7 @@ VideoCaptureDeviceClient::ResurrectLastOutputBuffer(
     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));
@@ -365,20 +358,16 @@ void VideoCaptureDeviceClient::OnError(
           .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();
 }