Refactor video capturing code in the render process

content/renderer/media/video_capture_impl_manager.cc

Index: content/renderer/media/video_capture_impl_manager.cc
diff --git a/content/renderer/media/video_capture_impl_manager.cc b/content/renderer/media/video_capture_impl_manager.cc
index 07460fb6283337fd43f53c40cfff45dfd89f6c68..675fd365e44017377afc3ebadc49d8b7792b0d64 100644
--- a/content/renderer/media/video_capture_impl_manager.cc
+++ b/content/renderer/media/video_capture_impl_manager.cc
@@ -1,95 +1,145 @@
 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
+//
+// Implementation notes about interactions with VideoCaptureImpl.
+//
+// VideoCaptureImpl is an IO thread object while VideoCaptureImplManager
+// lives only on the render thread. It is only possible to access an
+// object of VideoCaptureImpl via a task on the IO thread.
+// It is accessed with Unretained() because it is deleted only after
+// there is no reference to it.

[Ami GONE FROM CHROMIUM, 2014/04/24 21:04:31]

This sentence makes me wonder what sort of "reference" you are thinking of here.

I think it would be clearer to say that VCI is deleted via a PostTask to the IO
thread and that immediately thereafter the pointer is dropped in the render
thread so no further posting can happen.
This is only safe if nothing else posts; e.g. VCI mustn't PostTask to itself
(seems like that should be doco'd in VCI).

[Alpha Left Google, 2014/04/24 22:50:34]

Done.

+// An object of VideoCaptureImpl is deleted when VideoCaptureImplManager
+// is destroyed or there is no client using it.
 
 #include "content/renderer/media/video_capture_impl_manager.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
-#include "content/public/renderer/render_thread.h"
+#include "content/child/child_process.h"
 #include "content/renderer/media/video_capture_impl.h"
 #include "content/renderer/media/video_capture_message_filter.h"
 #include "media/base/bind_to_current_loop.h"
 
 namespace content {
 
-VideoCaptureHandle::VideoCaptureHandle(
-    media::VideoCapture* impl, base::Closure destruction_cb)
-    : impl_(impl), destruction_cb_(destruction_cb) {
-}
-
-VideoCaptureHandle::~VideoCaptureHandle() {
-  destruction_cb_.Run();
-}
-
-void VideoCaptureHandle::StartCapture(
-    EventHandler* handler,
-    const media::VideoCaptureParams& params) {
-  impl_->StartCapture(handler, params);
-}
-
-void VideoCaptureHandle::StopCapture(EventHandler* handler) {
-  impl_->StopCapture(handler);
-}
-
-bool VideoCaptureHandle::CaptureStarted() {
-  return impl_->CaptureStarted();
-}
-
-int VideoCaptureHandle::CaptureFrameRate() {
-  return impl_->CaptureFrameRate();
-}
-
-void VideoCaptureHandle::GetDeviceSupportedFormats(
-    const DeviceFormatsCallback& callback) {
-  impl_->GetDeviceSupportedFormats(callback);
-}
-
-void VideoCaptureHandle::GetDeviceFormatsInUse(
-    const DeviceFormatsInUseCallback& callback) {
-  impl_->GetDeviceFormatsInUse(callback);
-}
-
 VideoCaptureImplManager::VideoCaptureImplManager()
-    : filter_(new VideoCaptureMessageFilter()),
+    : next_client_id_(0),
+      filter_(new VideoCaptureMessageFilter()),
       weak_factory_(this) {
 }
 
 VideoCaptureImplManager::~VideoCaptureImplManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (devices_.empty())
+    return;
+  // Forcibly release all video capture resources.
+  for (VideoCaptureDeviceMap::iterator it = devices_.begin();
+       it != devices_.end(); ++it) {
+    VideoCaptureImpl* impl = it->second.second;
+    ChildProcess::current()->io_message_loop_proxy()->PostTask(

[Ami GONE FROM CHROMIUM, 2014/04/24 21:04:31]

I don't follow.
I said that the construct you're using is equivalent to a shorter construct.
You said that the construct I proposed is the IO message loop.
I believe the construct you're using is also the IO message loop.
What are you saying, again?

[Alpha Left Google, 2014/04/24 22:50:34]

You said:
MessageLoopProxy::current() returns the render thread. VideoCaptureImplManager
is destroyed on the render thread.

The reason I use ChildProcess here is because of unit testing. It creates an
instance of ChildProcess. It is much simpler to instantiate than a RenderProcess
or a RenderThreadImpl object.

+        FROM_HERE,
+        base::Bind(&VideoCaptureImpl::DeInit,
+                   base::Unretained(impl)));
+    ChildProcess::current()->io_message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&base::DeletePointer<VideoCaptureImpl>,
+                   base::Unretained(impl)));
+  }
+  devices_.clear();
 }
 
-scoped_ptr<VideoCaptureHandle> VideoCaptureImplManager::UseDevice(
+base::Closure VideoCaptureImplManager::UseDevice(
     media::VideoCaptureSessionId id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  VideoCaptureImpl* video_capture_device = NULL;
+  VideoCaptureImpl* impl = NULL;
   VideoCaptureDeviceMap::iterator it = devices_.find(id);
   if (it == devices_.end()) {
-    video_capture_device = CreateVideoCaptureImpl(id, filter_.get());
-    devices_[id] =
-        std::make_pair(1, linked_ptr<VideoCaptureImpl>(video_capture_device));
-    video_capture_device->Init();
+    impl = CreateVideoCaptureImplForTesting(id, filter_.get());
+    if (!impl)
+      impl = new VideoCaptureImpl(id, filter_.get());
+    devices_[id] = std::make_pair(1, impl);
+    ChildProcess::current()->io_message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&VideoCaptureImpl::Init,
+                   base::Unretained(impl)));
   } else {
     ++it->second.first;
-    video_capture_device = it->second.second.get();
   }
+  return base::Bind(&VideoCaptureImplManager::UnrefDevice,
+                    weak_factory_.GetWeakPtr(), id);
+}
 
-  // This callback ensures UnrefDevice() happens on the render thread.
-  return scoped_ptr<VideoCaptureHandle>(
-      new VideoCaptureHandle(
-          video_capture_device,
-          media::BindToCurrentLoop(
-              base::Bind(
-                  &VideoCaptureImplManager::UnrefDevice,
-                  weak_factory_.GetWeakPtr(),
-                  id))));
+base::Closure VideoCaptureImplManager::StartCapture(
+    media::VideoCaptureSessionId id,
+    const media::VideoCaptureParams& params,
+    const VideoCaptureStateUpdateCB& state_update_cb,
+    const VideoCaptureDeliverFrameCB& deliver_frame_cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  VideoCaptureDeviceMap::iterator it = devices_.find(id);
+  DCHECK(it != devices_.end());
+  VideoCaptureImpl* impl = it->second.second;
+
+  // This ID is used to identify a client of VideoCaptureImpl.
+  const int client_id = ++next_client_id_;
+
+  ChildProcess::current()->io_message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&VideoCaptureImpl::StartCapture,
+                 base::Unretained(impl),
+                 client_id,
+                 params,
+                 state_update_cb,
+                 deliver_frame_cb));
+  return base::Bind(&VideoCaptureImplManager::StopCapture,
+                    weak_factory_.GetWeakPtr(),
+                    client_id, id);
+}
+
+void VideoCaptureImplManager::GetDeviceSupportedFormats(
+    media::VideoCaptureSessionId id,
+    const VideoCaptureDeviceFormatsCB& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  VideoCaptureDeviceMap::iterator it = devices_.find(id);
+  DCHECK(it != devices_.end());
+  VideoCaptureImpl* impl = it->second.second;
+  ChildProcess::current()->io_message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&VideoCaptureImpl::GetDeviceSupportedFormats,
+                 base::Unretained(impl), callback));
+}
+
+void VideoCaptureImplManager::GetDeviceFormatsInUse(
+    media::VideoCaptureSessionId id,
+    const VideoCaptureDeviceFormatsCB& callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  VideoCaptureDeviceMap::iterator it = devices_.find(id);
+  DCHECK(it != devices_.end());
+  VideoCaptureImpl* impl = it->second.second;
+  ChildProcess::current()->io_message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&VideoCaptureImpl::GetDeviceFormatsInUse,
+                 base::Unretained(impl), callback));
 }
 
-VideoCaptureImpl* VideoCaptureImplManager::CreateVideoCaptureImpl(
+VideoCaptureImpl*
+VideoCaptureImplManager::CreateVideoCaptureImplForTesting(
     media::VideoCaptureSessionId id,
     VideoCaptureMessageFilter* filter) const {
-  return new VideoCaptureImpl(id, filter);
+  return NULL;
+}
+
+void VideoCaptureImplManager::StopCapture(
+    int client_id, media::VideoCaptureSessionId id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  VideoCaptureDeviceMap::iterator it = devices_.find(id);
+  DCHECK(it != devices_.end());
+  VideoCaptureImpl* impl = it->second.second;
+  ChildProcess::current()->io_message_loop_proxy()->PostTask(
+      FROM_HERE,
+      base::Bind(&VideoCaptureImpl::StopCapture,
+                 base::Unretained(impl), client_id));
 }
 
 void VideoCaptureImplManager::UnrefDevice(
@@ -97,21 +147,34 @@ void VideoCaptureImplManager::UnrefDevice(
   DCHECK(thread_checker_.CalledOnValidThread());
   VideoCaptureDeviceMap::iterator it = devices_.find(id);
   DCHECK(it != devices_.end());
+  VideoCaptureImpl* impl = it->second.second;
 
+  // Unref and destroy on the IO thread if there's no more client.
   DCHECK(it->second.first);
   --it->second.first;
   if (!it->second.first) {
-    VideoCaptureImpl* impl = it->second.second.release();
     devices_.erase(id);
-    impl->DeInit(base::Bind(&base::DeletePointer<VideoCaptureImpl>, impl));
+    ChildProcess::current()->io_message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&VideoCaptureImpl::DeInit,
+                   base::Unretained(impl)));
+    ChildProcess::current()->io_message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&base::DeletePointer<VideoCaptureImpl>,
+                   base::Unretained(impl)));
   }
 }
 
 void VideoCaptureImplManager::SuspendDevices(bool suspend) {
   DCHECK(thread_checker_.CalledOnValidThread());
   for (VideoCaptureDeviceMap::iterator it = devices_.begin();
-       it != devices_.end(); ++it)
-    it->second.second->SuspendCapture(suspend);
+       it != devices_.end(); ++it) {
+    VideoCaptureImpl* impl = it->second.second;
+    ChildProcess::current()->io_message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&VideoCaptureImpl::SuspendCapture,
+                   base::Unretained(impl), suspend));
+  }
 }
 
 }  // namespace content