Make VideoCaptureController single-threaded and not ref counted.

content/browser/renderer_host/media/video_capture_controller.h

Index: content/browser/renderer_host/media/video_capture_controller.h
diff --git a/content/browser/renderer_host/media/video_capture_controller.h b/content/browser/renderer_host/media/video_capture_controller.h
index 2c294505ca34f8d7bdbe2d148ca1f35819966e66..995a5e142eea2f87220c58ca1860cf3fce3493a4 100644
--- a/content/browser/renderer_host/media/video_capture_controller.h
+++ b/content/browser/renderer_host/media/video_capture_controller.h
@@ -13,21 +13,25 @@
 //     with I420 video frames for IPC communication between VideoCaptureHost (in
 //     the browser process) and VideoCaptureMessageFilter (in the renderer
 //     process).
-//   * Conveying events from the device thread (where capture devices live) to
-//     the IO thread (where the clients can be reached).
 //   * Broadcasting the events from a single VideoCaptureDevice, fanning them
 //     out to multiple clients.
 //   * Keeping track of the clients on behalf of the VideoCaptureManager, making
 //     it possible for the Manager to delete the Controller and its Device when
 //     there are no clients left.
+//
+// A helper class, VCC::VideoCaptureDeviceClient, is responsible for:
+//
+//   * Conveying events from the device thread (where VideoCaptureDevices live)
+//     the IO thread (where the VideoCaptureController lives).
 //   * Performing some image transformations on the output of the Device;
 //     specifically, colorspace conversion and rotation.
 //
 // Interactions between VideoCaptureController and other classes:
 //
-//   * VideoCaptureController receives events from its VideoCaptureDevice
-//     entirely through the VideoCaptureDevice::EventHandler interface, which
-//     VCC implements.
+//   * VideoCaptureController indirectly observes a VideoCaptureDevice
+//     by means of its proxy, VideoCaptureDeviceClient, which implements
+//     the VideoCaptureDevice::EventHandler interface. The proxy forwards
+//     observed events to the VideoCaptureController on the IO thread.
 //   * A VideoCaptureController interacts with its clients (VideoCaptureHosts)
 //     via the VideoCaptureControllerEventHandler interface.
 //   * Conversely, a VideoCaptureControllerEventHandler (typically,
@@ -38,12 +42,7 @@
 //     VideoCaptureController through its public methods, to add and remove
 //     clients.
 //
-// Thread safety:
-//
-//   The public methods implementing the VCD::EventHandler interface are safe to
-//   call from any thread -- in practice, this is the thread on which the Device
-//   is running. For this reason, it is RefCountedThreadSafe. ALL OTHER METHODS
-//   are only safe to run on the IO browser thread.
+// VideoCaptureController is not thread safe and operates on the IO thread only.
 
 #ifndef CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_CONTROLLER_H_
 #define CONTENT_BROWSER_RENDERER_HOST_MEDIA_VIDEO_CAPTURE_CONTROLLER_H_
@@ -53,6 +52,8 @@
 
 #include "base/compiler_specific.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/process/process.h"
 #include "base/synchronization/lock.h"
 #include "content/browser/renderer_host/media/video_capture_buffer_pool.h"
@@ -66,11 +67,16 @@
 namespace content {
 class VideoCaptureBufferPool;
 
-class CONTENT_EXPORT VideoCaptureController
-    : public base::RefCountedThreadSafe<VideoCaptureController>,
-      public media::VideoCaptureDevice::EventHandler {
+class CONTENT_EXPORT VideoCaptureController {
  public:
   VideoCaptureController();
+  virtual ~VideoCaptureController();
+
+  base::WeakPtr<VideoCaptureController> AsWeakPtr();

[Ami GONE FROM CHROMIUM, 2013/09/13 21:17:59]

"As" connotes SupportsWeakPtr, which is generally meh.  Being part of the public
API makes it meh-er.
I mildly prefer GetWeakPtr().  Use your judgement.

[ncarter (slow), 2013/09/14 00:07:24]

Done. Having thought about it, I agree with you.

+
+  // Return a new VideoCaptureDeviceClient to forward capture events to this
+  // instance.
+  scoped_ptr<media::VideoCaptureDevice::EventHandler> NewDeviceClient();
 
   // Start video capturing and try to use the resolution specified in
   // |params|.
@@ -100,36 +106,61 @@ class CONTENT_EXPORT VideoCaptureController
                     VideoCaptureControllerEventHandler* event_handler,
                     int buffer_id);
 
-  // Implement media::VideoCaptureDevice::EventHandler.
-  virtual scoped_refptr<media::VideoFrame> ReserveOutputBuffer() OVERRIDE;
-  virtual void OnIncomingCapturedFrame(const uint8* data,
-                                       int length,
-                                       base::Time timestamp,
-                                       int rotation,
-                                       bool flip_vert,
-                                       bool flip_horiz) OVERRIDE;
-  virtual void OnIncomingCapturedVideoFrame(
-      const scoped_refptr<media::VideoFrame>& frame,
-      base::Time timestamp) OVERRIDE;
-  virtual void OnError() OVERRIDE;
-  virtual void OnFrameInfo(const media::VideoCaptureCapability& info) OVERRIDE;
-  virtual void OnFrameInfoChanged(
-      const media::VideoCaptureCapability& info) OVERRIDE;
-
- protected:
-  virtual ~VideoCaptureController();
-
  private:
-  friend class base::RefCountedThreadSafe<VideoCaptureController>;
+  // Receives events from the VideoCaptureDevice and posts them to a
+  // VideoCaptureController on the IO thread. An instance of this class may
+  // safely outlive its target VideoCaptureController. Methods of this class may
+  // be called from any thread.
+  class VideoCaptureDeviceClient

[Ami GONE FROM CHROMIUM, 2013/09/13 21:17:59]

Could this move into the .cc file with merely a fwd decl here?

[ncarter (slow), 2013/09/14 00:07:24]

Sure. Done. FWIW, I put it here in the header for "theoretical unit test"
reasons as well as for hard-to-universalize "I like to grep for
member_variable_declarations_ in .h files" reasons. But once we move the
color-conversion responsibility to the Rigs, this class will just be a fairly
dumb proxy, probably not worth unit testing alone or splitting into its own
file.

+      : public media::VideoCaptureDevice::EventHandler {
+   public:
+    explicit VideoCaptureDeviceClient(
+        const base::WeakPtr<VideoCaptureController>& controller);
+    virtual ~VideoCaptureDeviceClient();
+
+    // VideoCaptureDevice::EventHandler implementation.
+    virtual scoped_refptr<media::VideoFrame> ReserveOutputBuffer() OVERRIDE;
+    virtual void OnIncomingCapturedFrame(const uint8* data,
+                                         int length,
+                                         base::Time timestamp,
+                                         int rotation,
+                                         bool flip_vert,
+                                         bool flip_horiz) OVERRIDE;
+    virtual void OnIncomingCapturedVideoFrame(
+        const scoped_refptr<media::VideoFrame>& frame,
+        base::Time timestamp) OVERRIDE;
+    virtual void OnError() OVERRIDE;
+    virtual void OnFrameInfo(
+        const media::VideoCaptureCapability& info) OVERRIDE;
+    virtual void OnFrameInfoChanged(
+        const media::VideoCaptureCapability& info) OVERRIDE;
+
+   private:
+    // The controller to which we post events.
+    const base::WeakPtr<VideoCaptureController> controller_;
+
+    // The pool of shared-memory buffers used for capturing.
+    scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
+
+    // Chopped pixels in width/height in case video capture device has odd
+    // numbers for width/height.
+    int chopped_width_;
+    int chopped_height_;
+
+    // Tracks the current frame format.
+    media::VideoCaptureCapability frame_info_;
+  };
 
   struct ControllerClient;
   typedef std::list<ControllerClient*> ControllerClients;
 
-  // Worker functions on IO thread.
+  // Worker functions on IO thread. Called by the VideoCaptureDeviceClient.
   void DoIncomingCapturedFrameOnIOThread(
       const scoped_refptr<media::VideoFrame>& captured_frame,
       base::Time timestamp);
-  void DoFrameInfoOnIOThread();
+  void DoFrameInfoOnIOThread(
+      const media::VideoCaptureCapability& frame_info,
+      const scoped_refptr<VideoCaptureBufferPool>& buffer_pool);
   void DoFrameInfoChangedOnIOThread(const media::VideoCaptureCapability& info);
   void DoErrorOnIOThread();
   void DoDeviceStoppedOnIOThread();
@@ -148,11 +179,6 @@ class CONTENT_EXPORT VideoCaptureController
       int session_id,
       const ControllerClients& clients);
 
-  // Protects access to the |buffer_pool_| pointer on non-IO threads.  IO thread
-  // must hold this lock when modifying the |buffer_pool_| pointer itself.
-  // TODO(nick): Make it so that this lock isn't required.

[Ami GONE FROM CHROMIUM, 2013/09/13 21:17:59]

You made it so!

[ncarter (slow), 2013/09/14 00:07:24]

Done.

-  base::Lock buffer_pool_lock_;
-
   // The pool of shared-memory buffers used for capturing.
   scoped_refptr<VideoCaptureBufferPool> buffer_pool_;
 
@@ -162,22 +188,19 @@ class CONTENT_EXPORT VideoCaptureController
   // The parameter that currently used for the capturing.
   media::VideoCaptureParams current_params_;
 
-  // It's modified on caller thread, assuming there is only one OnFrameInfo()
-  // call per StartCapture().
-  media::VideoCaptureCapability frame_info_;
-
-  // Chopped pixels in width/height in case video capture device has odd numbers
-  // for width/height. Accessed only on the device thread.
-  int chopped_width_;
-  int chopped_height_;
-
-  // It's accessed only on IO thread.
+  // Tracks whether OnFrameInfo() has occurred, and whether |frame_info_| is
+  // valid.
   bool frame_info_available_;

[Ami GONE FROM CHROMIUM, 2013/09/13 21:17:59]

VideoCaptureCapability's default ctor zero-initializes fields that guarantee its
IsValid() method will return false until stuff is set explicitly, so I think you
can probably drop this member in favor of using IsValid().

[ncarter (slow), 2013/09/14 00:07:24]

Neato. Done.

 
+  // Tracks the current frame format.
+  media::VideoCaptureCapability frame_info_;
+
   // Takes on only the states 'STARTED' and 'ERROR'. 'ERROR' is an absorbing
-  // state which stops the flow of data to clients. Accessed only on IO thread.
+  // state which stops the flow of data to clients.
   VideoCaptureState state_;
 
+  base::WeakPtrFactory<VideoCaptureController> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(VideoCaptureController);
 };