Break tight coupling of VideoCaptureDeviceClient to renderer_host

content/browser/renderer_host/media/video_capture_device_client.h

Index: content/browser/renderer_host/media/video_capture_device_client.h
diff --git a/content/browser/renderer_host/media/video_capture_device_client.h b/content/browser/renderer_host/media/video_capture_device_client.h
index c5447ca18e74eed9969270fb657a72e2aefd87f6..6eed12a8f052e8e5a6b23c7435d40452b7ba6a0d 100644
--- a/content/browser/renderer_host/media/video_capture_device_client.h
+++ b/content/browser/renderer_host/media/video_capture_device_client.h
@@ -17,13 +17,14 @@
 #include "media/capture/video/video_capture_device.h"
 
 namespace content {
-class VideoCaptureBufferPool;
-class VideoCaptureController;
-class VideoCaptureGpuJpegDecoder;
+class VideoCaptureBufferPoolInterface;
+class VideoFrameReceiver;
+class VideoCaptureJpegDecoderFactory;
+class VideoCaptureJpegDecoder;
 
-// Receives events from the VideoCaptureDevice and posts them to a |controller_|
+// Receives events from the VideoCaptureDevice and posts them to a |receiver_|
 // on the IO thread. An instance of this class may safely outlive its target
-// VideoCaptureController. This is a shallow class meant to convert incoming
+// VideoFrameReceiver. This is a shallow class meant to convert incoming
 // frames and holds no significant state.
 //
 // Methods of this class may be called from any thread, and in practice will
@@ -40,8 +41,10 @@ class CONTENT_EXPORT VideoCaptureDeviceClient
       public base::SupportsWeakPtr<VideoCaptureDeviceClient> {
  public:
   VideoCaptureDeviceClient(
-      const base::WeakPtr<VideoCaptureController>& controller,
-      const scoped_refptr<VideoCaptureBufferPool>& buffer_pool);
+      std::unique_ptr<VideoFrameReceiver> receiver,
+      const scoped_refptr<VideoCaptureBufferPoolInterface>& buffer_pool,
+      std::unique_ptr<VideoCaptureJpegDecoderFactory> jpeg_decoder_factory,
+      const base::Closure& check_thread_closure);
   ~VideoCaptureDeviceClient() override;
 
   // VideoCaptureDevice::Client implementation.
@@ -91,17 +94,17 @@ class CONTENT_EXPORT VideoCaptureDeviceClient
       uint8_t** u_plane_data,
       uint8_t** v_plane_data);
 
-  // The controller to which we post events.
-  const base::WeakPtr<VideoCaptureController> controller_;
+  // The receiver to which we post events.
+  std::unique_ptr<VideoFrameReceiver> receiver_;

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

maybe const?

Also, there's a subtle change here in that |controller_| 
is only used on IO thread whereas |receiver_| is used 
on the thread where video frames are delievered.
The whole structure might be racy on destruction, 
right?

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

Not sure if my understanding of the threading issues is correct here.
But I believe that the refactoring does not change anything regarding the
threading.

Calls from the VideoCaptureDeviceClient instance to the instance formerly
referred to as |controller_| are still being issued on the Browser IO Thread,
just as before. The only difference is that after the refactoring, the class
VideoCaptureDeviceClient no longer has the responsibility of posting to that
thread. The responsibility of posting calls to the correct thread is now with
|receiver_|, which in our usage is an instance of the new class
PostOnBrowserIoThreadDecorator.

I'll ping you for an offline discussion to check if I am missing anything here.

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

Notwithstanding any offline discussions, I see this
sequence as potentially racy:

- |receiver_| is created on the thread where VCDClient is
created (which we know is IO thread, but that's irrelevant 
for the purpose of this conversation).
- |receiver_| is used from the thread where 
OnIncomingCapturedData(), which we cannot assume to 
be the same of where the class is constructed.
- VCDClient can still use |receiver_| in said
OnIncomingCapturedData() after being destructed
on IO thread.

This scenario wasn't possible before because of the 
PostTask(), that forced using |controller_| (weak) 
explicitly, which was invalidated synchronously in its
destructor.

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

Discussed offline. mcasas@ found out that after construction 
on the IO thread, ownership of the VCDClient is passed to a 
media::VideoCaptureDevice instance, where it is used and 
eventually destroyed on the Device Thread. We agreed to make it 
clear in the description of VCDClient that it is legal to construct 
on one thread and then pass to another thread for usage and ownership.

 
-  // Hardware JPEG decoder.
-  std::unique_ptr<VideoCaptureGpuJpegDecoder> external_jpeg_decoder_;
+  std::unique_ptr<VideoCaptureJpegDecoderFactory> jpeg_decoder_factory_;

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

const

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

Done.

+  std::unique_ptr<VideoCaptureJpegDecoder> external_jpeg_decoder_;
 
   // Whether |external_jpeg_decoder_| has been initialized.
   bool external_jpeg_decoder_initialized_;
 
   // The pool of shared-memory buffers used for capturing.
-  const scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
+  const scoped_refptr<VideoCaptureBufferPoolInterface> buffer_pool_;
 
 #if DCHECK_IS_ON()
   // Counter used to track the number of times consecutive capture buffers are