Eliminate video capture thread in renderer

content/renderer/media/video_capture_impl_manager.h

Index: content/renderer/media/video_capture_impl_manager.h
diff --git a/content/renderer/media/video_capture_impl_manager.h b/content/renderer/media/video_capture_impl_manager.h
index e9422015b026bda6d3d41abc4398fed0aee1a34a..7367b9e9887d06bfd14a119a27efb795b0e6d816 100644
--- a/content/renderer/media/video_capture_impl_manager.h
+++ b/content/renderer/media/video_capture_impl_manager.h
@@ -2,77 +2,92 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// VideoCaptureImplManager manages video capture devices in renderer process.
-// The video capture clients use AddDevice() to get a pointer to
-// video capture device. VideoCaputreImplManager supports multiple clients
-// accessing same device.
+// VideoCaptureImplManager owns VideoCaptureImpl objects. Clients who
+// want access to a video capture device calls UseDevice() to get a handle

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

s/calls/call/

[Alpha Left Google, 2014/01/08 00:23:36]

Done.

+// to VideoCaptureImpl.
+//
+// THREADING
+//
+// This object lives only on the render thread. All methods must be called

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

"This object" is unclear.  I believe you refer to VCIM but it's not obvious
you're not referring to VCH or VCI.

[Alpha Left Google, 2014/01/08 00:23:36]

Done.

+// on this thread.
+//
+// The handle returned by UseDevice() is thread-safe. It ensures
+// destruction is handled on the render thread.
 
 #ifndef CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_MANAGER_H_
 #define CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_IMPL_MANAGER_H_
 
-#include <list>
 #include <map>
 
+#include "base/callback.h"
+#include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
-#include "base/threading/thread.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
 #include "content/common/content_export.h"
 #include "media/video/capture/video_capture.h"
 
 namespace content {
 
 class VideoCaptureImpl;
+class VideoCaptureImplManager;
 class VideoCaptureMessageFilter;
 
-class CONTENT_EXPORT VideoCaptureImplManager
-    : public base::RefCountedThreadSafe<VideoCaptureImplManager> {
+// Thread-safe wrapper for a media::VideoCapture object. This class

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

Can you drop VCH entirely in favor of RefCountedDeleteOnMessageLoop?

[Alpha Left Google, 2014/01/08 00:23:36]

I don't think we can do away without an extra class here for the following
reasons:

1. We can't give scoped_refptr<VideoCaptureImpl> to user. De-initialization of
VideoCaptureImpl is asynchronous and requires more than just destruction.
2. We want to give VideoCaptureImpl to multiple users. The de-initialization of
VCI is done only once when there is no user. Hence VCIM needs to know the
refcount.

The only way to not have VCH is that user return the VCI to VCIM through a
method like VCIM::Release(VCI*). However this adds threading requirements about
where to make this call. And user of VCI can run on any threads which means user
of VCI needs a reference to VCIM and VCIM::Release() needs to handle calls from
any threads.

I would like to hide the threading requirements inside VCIM and the only way I
can think of is to have a separate VCH class.

+// ensures refcounting is done on the render thread.

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

This sentence (and the corresponding one in the CL description) is a bit
unclear.  I think you mean to say in both places that VCH's purpose in life is
to ensure its underlying VC is deleted on the render thread.

[Alpha Left Google, 2014/01/08 00:23:36]

Done.

+class CONTENT_EXPORT VideoCaptureHandle : media::VideoCapture {

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

Is it easy to see why this class belongs in this file?

[Alpha Left Google, 2014/01/08 00:23:36]

The reason I put this class in this file is because its only user is
VideoCaptureImplManager. I thought of putting it in
media/video/capture/video_capture_handle.h but it seems this mechanism is only
useful because of the threading requirements here. I'm open to suggestions.

  public:
-  VideoCaptureImplManager();
+  virtual ~VideoCaptureHandle();
+
+  // media::VideoCapture implementations.
+  virtual void StartCapture(
+      EventHandler* handler,
+      const media::VideoCaptureParams& params) OVERRIDE;
+  virtual void StopCapture(EventHandler* handler) OVERRIDE;
+  virtual bool CaptureStarted() OVERRIDE;
+  virtual int CaptureFrameRate() OVERRIDE;
+
+ private:
+  friend class VideoCaptureImplManager;
 
-  // Called by video capture client |handler| to add device referenced
-  // by |id| to VideoCaptureImplManager's list of opened device list.
-  // A pointer to VideoCapture is returned to client so that client can
-  // operate on that pointer, such as StartCaptrue, StopCapture.
-  virtual media::VideoCapture* AddDevice(
-      media::VideoCaptureSessionId id,
-      media::VideoCapture::EventHandler* handler);
+  VideoCaptureHandle(media::VideoCapture* impl,
+                     base::Closure destruction_cb);
 
-  // Called by video capture client |handler| to remove device referenced
-  // by |id| from VideoCaptureImplManager's list of opened device list.
-  virtual void RemoveDevice(media::VideoCaptureSessionId id,
-                            media::VideoCapture::EventHandler* handler);
+  media::VideoCapture* impl_;
+  base::Closure destruction_cb_;
+
+  DISALLOW_COPY_AND_ASSIGN(VideoCaptureHandle);
+};
+
+class CONTENT_EXPORT VideoCaptureImplManager {
+ public:
+  VideoCaptureImplManager();
+  virtual ~VideoCaptureImplManager();
+
+  // Returns a video capture device referenced by |id|.
+  scoped_ptr<VideoCaptureHandle> UseDevice(media::VideoCaptureSessionId id);
 
   // Make all existing VideoCaptureImpl instances stop/resume delivering
   // video frames to their clients, depends on flag |suspend|.
-  virtual void SuspendDevices(bool suspend);
+  void SuspendDevices(bool suspend);
 
   VideoCaptureMessageFilter* video_capture_message_filter() const {
     return filter_.get();
   }
 
- protected:
-  virtual ~VideoCaptureImplManager();
-
  private:
-  friend class base::RefCountedThreadSafe<VideoCaptureImplManager>;
-
-  struct Device {
-    Device(VideoCaptureImpl* device,
-           media::VideoCapture::EventHandler* handler);
-    ~Device();
-
-    VideoCaptureImpl* vc;
-    std::list<media::VideoCapture::EventHandler*> clients;
-  };
+  void UnrefDevice(media::VideoCaptureSessionId id);
 
-  void FreeDevice(VideoCaptureImpl* vc);
+  typedef std::map<media::VideoCaptureSessionId,
+                   linked_ptr<VideoCaptureImpl> > VideoCaptureDeviceMap;

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

FWIW if you made VideoCaptureDeviceMap be a 
std::map<
    media::VideoCaptureSessionId,
    std::pair<int,linked_ptr<VideoCaptureImpl> > >
then you could get away with a single map, which might make it easier to see why
you're guaranteed that lookups in the two maps always succeed or fail together.

But if you take my suggestion above to replace VCH with
RefCountedDeleteOnMessageLoop then I think this simplifies to what you have now
and drop use_count_ entirely.

[Alpha Left Google, 2014/01/08 00:23:36]

Took your advice and used std::pair<> and removed use_count_.

+  VideoCaptureDeviceMap devices_;
+  std::map<media::VideoCaptureSessionId, int> use_count_;
 
-  typedef std::map<media::VideoCaptureSessionId, Device*> Devices;
-  Devices devices_;
-  base::Lock lock_;
   scoped_refptr<VideoCaptureMessageFilter> filter_;
-  base::Thread thread_;
-  scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
+  base::WeakPtrFactory<VideoCaptureImplManager> weak_factory_;

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

IWBN to comment for both this and the next member that they're bound to the
render thread.

[Alpha Left Google, 2014/01/08 00:23:36]

Done.

+  base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoCaptureImplManager);
 };