Break tight coupling of VideoCaptureDeviceClient to renderer_host

content/browser/renderer_host/media/video_capture_controller.cc

Index: content/browser/renderer_host/media/video_capture_controller.cc
diff --git a/content/browser/renderer_host/media/video_capture_controller.cc b/content/browser/renderer_host/media/video_capture_controller.cc
index 3840a81a9b791da8c1b03af101f97f2bd80ae1af..a77585ff7598ba367ac3c5f17aa962766b8e9f11 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -21,6 +21,7 @@
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/media/video_capture_buffer_pool.h"
 #include "content/browser/renderer_host/media/video_capture_device_client.h"
+#include "content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.h"
 #include "content/browser/renderer_host/media/video_capture_manager.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
@@ -79,6 +80,60 @@ void ReturnVideoFrame(const scoped_refptr<VideoFrame>& video_frame,
 #endif
 }
 
+class VideoCaptureGpuJpegDecoderFactory
+    : public VideoCaptureJpegDecoderFactory {
+ public:
+  VideoCaptureGpuJpegDecoderFactory(
+      const VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb)
+      : decode_done_cb_(decode_done_cb) {}
+
+  std::unique_ptr<VideoCaptureJpegDecoder> CreateJpegDecoder() override {
+    return base::MakeUnique<VideoCaptureGpuJpegDecoder>(decode_done_cb_);
+  }
+
+ private:
+  VideoCaptureJpegDecoder::DecodeDoneCB decode_done_cb_;
+};

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

I don't think this deserves a full class, a callback signature should
be enough, e.g.

using VideoCaptureGpuJpegDecoderCreateCB =
    base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>(
        VideoCaptureJpegDecoder::DecodeDoneCB)>;

defined in video_capture_device_client.h, and base::Bind()ed
in l.209 to a static function.

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

I believe what you are proposing is to replace the explicit interface class
VideoCaptureJpegDecoderFactory with a callback interface

using VideoCaptureJpegDecoderFactoryCB = 
    base::Callback<std::unique_ptr<VideoCaptureJpegDecoder>()>;

and then using base::Bind() to create an instance by binding to a static
function

static std::unique_ptr<VideoCaptureJpegDecoder> CreateGpuJpegDecoder(
    const VideoCaptureJpegDecoder::DecodeDoneCB& decode_done_cb) {
  return base::MakeUnique<VideoCaptureGpuJpegDecoder>(decode_done_cb);
}

with the |decode_done_cb| provided at bind-time at l.209:

      base::Bind(&CreateGpuJpegDecoder,
                 base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame,
                            this->GetWeakPtrForIOThread())),

Both approaches are equivalent from an Object-Oriented design perspective.
But as a reader of the code, I find the explicit interface class plus explicit
implementation class much easier to understand than the ad-hoc binding.
Therefore I would like to keep using the existing approach unless there is any
other reason why the callback approach is superior that I missed.

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

emircan@ thinks the same in his comment in this PS
line 211 
:-)

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

Done.

+
+// Decorator for VideoFrameReceiver that forwards all incoming calls
+// to the Browser IO thread.
+class PostOnBrowserIoThreadDecorator : public VideoFrameReceiver {
+ public:
+  PostOnBrowserIoThreadDecorator(
+      const base::WeakPtr<VideoFrameReceiver>& receiver)
+      : receiver_(receiver) {}
+
+  void OnIncomingCapturedVideoFrame(
+      std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> buffer,
+      const scoped_refptr<media::VideoFrame>& frame) override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame, receiver_,
+                   base::Passed(&buffer), frame));
+  }
+
+  void OnError() override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&VideoFrameReceiver::OnError, receiver_));
+  }
+
+  void OnLog(const std::string& message) override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&VideoFrameReceiver::OnLog, receiver_, message));
+  }
+
+  void OnBufferDestroyed(int buffer_id_to_drop) override {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::Bind(&VideoFrameReceiver::OnBufferDestroyed,
+                                       receiver_, buffer_id_to_drop));
+  }
+
+ private:
+  base::WeakPtr<VideoFrameReceiver> receiver_;
+};
+
 }  // anonymous namespace
 
 struct VideoCaptureController::ControllerClient {
@@ -148,7 +203,13 @@ std::unique_ptr<media::VideoCaptureDevice::Client>
 VideoCaptureController::NewDeviceClient() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   return base::MakeUnique<VideoCaptureDeviceClient>(
-      this->GetWeakPtrForIOThread(), buffer_pool_);
+      base::MakeUnique<PostOnBrowserIoThreadDecorator>(
+          this->GetWeakPtrForIOThread()),
+      buffer_pool_,
+      base::MakeUnique<VideoCaptureGpuJpegDecoderFactory>(
+          base::Bind(&VideoFrameReceiver::OnIncomingCapturedVideoFrame,
+                     this->GetWeakPtrForIOThread())),
+      base::Bind([]() { DCHECK_CURRENTLY_ON(BrowserThread::IO); }));
 }
 
 void VideoCaptureController::AddClient(
@@ -354,7 +415,7 @@ VideoCaptureController::~VideoCaptureController() {
                                    controller_clients_.end());
 }
 
-void VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(
+void VideoCaptureController::OnIncomingCapturedVideoFrame(
     std::unique_ptr<media::VideoCaptureDevice::Client::Buffer> buffer,
     const scoped_refptr<VideoFrame>& frame) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -425,7 +486,7 @@ void VideoCaptureController::DoIncomingCapturedVideoFrameOnIOThread(
   buffer_pool_->HoldForConsumers(buffer_id, count);
 }
 
-void VideoCaptureController::DoErrorOnIOThread() {
+void VideoCaptureController::OnError() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   state_ = VIDEO_CAPTURE_STATE_ERROR;
 
@@ -436,13 +497,12 @@ void VideoCaptureController::DoErrorOnIOThread() {
   }
 }
 
-void VideoCaptureController::DoLogOnIOThread(const std::string& message) {
+void VideoCaptureController::OnLog(const std::string& message) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   MediaStreamManager::SendMessageToNativeLog("Video capture: " + message);
 }
 
-void VideoCaptureController::DoBufferDestroyedOnIOThread(
-    int buffer_id_to_drop) {
+void VideoCaptureController::OnBufferDestroyed(int buffer_id_to_drop) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   for (auto* client : controller_clients_) {