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..62495fa649e53bafd7ab527f47007b4c18de7756 100644
--- a/content/renderer/media/video_capture_impl_manager.cc
+++ b/content/renderer/media/video_capture_impl_manager.cc
@@ -1,89 +1,120 @@
 // 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 lifetime of VideoCaptureImpl.
+//
+// VideoCaptureImpl is an internal class visible only by this class.
+// Since it is an IO thread object we manage the lifetime of it manually.
+// It is first created on the render thread. We also post a task to
+// access it on the IO thread. It is accessed with Unretained() because
+// we delete it on the IO thread after all clients have stopped.
 
 #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/21 23:42:53]

ChildProcess::current()->io_message_loop_proxy()
is
MessageLoopProxy::current()

[Alpha Left Google, 2014/04/23 18:48:33]

MessageLoopProxy::current() is the IO message loop. This object is render thread
only.

+        FROM_HERE,
+        base::Bind(&VideoCaptureImpl::DeInit,

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

why not DeInit+delete directly in-line here?

[Alpha Left Google, 2014/04/23 18:48:33]

VCI is an IO thread only object. It is accessed via a task on IO thread only. I
have added comments at the top of this file to clarify.

+                   base::Unretained(impl)));

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

any "Unretained" use should be covered by an explanation for why it is safe.

[Alpha Left Google, 2014/04/23 18:48:33]

I documented it at the top of this file.

+    ChildProcess::current()->io_message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&base::DeletePointer<VideoCaptureImpl>, 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 = CreateVideoCaptureImpl(id, filter_.get());
+    devices_[id] = std::make_pair(1, impl);
+    ChildProcess::current()->io_message_loop_proxy()->PostTask(

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

ditto why post instead of running in-line?

[Alpha Left Google, 2014/04/23 18:48:33]

VCIM is render thread only. VideoCaptureImpl is IO thread only.

+        FROM_HERE,
+        base::Bind(&VideoCaptureImpl::Init,
+                   base::Unretained(impl)));

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

ditto Unretained

[Alpha Left Google, 2014/04/23 18:48:33]

Please see comments at the top of this file.

   } else {
     ++it->second.first;
-    video_capture_device = it->second.second.get();
+    impl = it->second.second;

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

unused?

[Alpha Left Google, 2014/04/23 18:48:33]

Removed.

   }
+  return base::Bind(&VideoCaptureImplManager::UnrefDevice, this, 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(

[Ami GONE FROM CHROMIUM, 2014/04/21 23:42:53]

not reviewing the rest of this file since I hope you can simplify it per above
commetns.

[Alpha Left Google, 2014/04/23 18:48:33]

I have changed the access pattern. Instead of VCI expecting calls from different
threads it is now explicitly accessed by VCIM through a task on IO thread. So
the PostTasks are necessary now.

+      FROM_HERE,
+      base::Bind(&VideoCaptureImpl::StartCapture,
+                 base::Unretained(impl),
+                 client_id,
+                 params,
+                 state_update_cb,
+                 deliver_frame_cb));
+  return base::Bind(&VideoCaptureImplManager::StopCapture, this,
+                    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(
@@ -92,26 +123,50 @@ VideoCaptureImpl* VideoCaptureImplManager::CreateVideoCaptureImpl(
   return new VideoCaptureImpl(id, filter);
 }
 
+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(
     media::VideoCaptureSessionId id) {
   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>, 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