Refactor video capturing code in the render process

content/renderer/media/media_stream_video_capturer_source.cc

Index: content/renderer/media/media_stream_video_capturer_source.cc
diff --git a/content/renderer/media/media_stream_video_capturer_source.cc b/content/renderer/media/media_stream_video_capturer_source.cc
index 6359c4595dcafd2d4868abde5a8897af159858e0..1309e033ddc28a5ac230c93d412185b3ae8852c9 100644
--- a/content/renderer/media/media_stream_video_capturer_source.cc
+++ b/content/renderer/media/media_stream_video_capturer_source.cc
@@ -8,6 +8,7 @@
 #include "base/location.h"
 #include "content/renderer/media/video_capture_impl_manager.h"
 #include "content/renderer/render_thread_impl.h"
+#include "media/base/bind_to_current_loop.h"
 #include "media/base/video_frame.h"
 
 namespace {
@@ -38,18 +39,21 @@ VideoCapturerDelegate::VideoCapturerDelegate(
                       device_info.device.type == MEDIA_DESKTOP_VIDEO_CAPTURE),
       got_first_frame_(false) {
   DVLOG(3) << "VideoCapturerDelegate::ctor";
-  // RenderThreadImpl::current() may be NULL in testing.
+  message_loop_proxy_ = base::MessageLoopProxy::current();
+
+  // NULL in unit test.
   if (RenderThreadImpl::current()) {
-    capture_engine_ = RenderThreadImpl::current()->video_capture_impl_manager()
-              ->UseDevice(device_info.session_id);
-    DCHECK(capture_engine_);
+    VideoCaptureImplManager* manager =
+        RenderThreadImpl::current()->video_capture_impl_manager();
+    release_device_cb_ = manager->UseDevice(session_id_);
   }
-  message_loop_proxy_ = base::MessageLoopProxy::current();
 }
 
 VideoCapturerDelegate::~VideoCapturerDelegate() {
   DVLOG(3) << "VideoCapturerDelegate::dtor";
   DCHECK(new_frame_callback_.is_null());
+  if (!release_device_cb_.is_null())
+    release_device_cb_.Run();
 }
 
 void VideoCapturerDelegate::GetCurrentSupportedFormats(
@@ -77,8 +81,13 @@ void VideoCapturerDelegate::GetCurrentSupportedFormats(
 
   DCHECK(source_formats_callback_.is_null());
   source_formats_callback_ = callback;
-  capture_engine_->GetDeviceFormatsInUse(base::Bind(
-      &VideoCapturerDelegate::OnDeviceFormatsInUseReceived, this));
+  VideoCaptureImplManager* manager =
+      RenderThreadImpl::current()->video_capture_impl_manager();

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

don't need to worry about RTI::current() being NULL in tests like l.44?  Maybe
l.44 is outdated now?

(ditto elsewhere below)

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

Done.

+  manager->GetDeviceFormatsInUse(
+      session_id_,
+      media::BindToCurrentLoop(
+          base::Bind(
+              &VideoCapturerDelegate::OnDeviceFormatsInUseReceived, this)));
 }
 
 void VideoCapturerDelegate::StartDeliver(
@@ -91,69 +100,35 @@ void VideoCapturerDelegate::StartDeliver(
   started_callback_ = started_callback;
   got_first_frame_ = false;
 
-  // Increase the reference count to ensure the object is not deleted until
-  // it is unregistered in VideoCapturerDelegate::OnRemoved.
-  AddRef();
-  capture_engine_->StartCapture(this, params);
+  VideoCaptureImplManager* manager =
+      RenderThreadImpl::current()->video_capture_impl_manager();
+  stop_capture_cb_ =
+      manager->StartCapture(
+          session_id_,
+          params,
+          media::BindToCurrentLoop(base::Bind(
+              &VideoCapturerDelegate::OnStateUpdateOnRenderThread, this)),
+          media::BindToCurrentLoop(base::Bind(
+              &VideoCapturerDelegate::OnFrameReadyOnRenderThread, this)));
 }
 
 void VideoCapturerDelegate::StopDeliver() {
   // Immediately make sure we don't provide more frames.
   DVLOG(3) << "VideoCapturerDelegate::StopDeliver()";
   DCHECK(message_loop_proxy_->BelongsToCurrentThread());
-  capture_engine_->StopCapture(this);
+  if (!stop_capture_cb_.is_null()) {
+    stop_capture_cb_.Run();
+    stop_capture_cb_.Reset();

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

base::ResetAndReturn(&stop_capture_cb_);
makes it easier to see that the Run() is safe even if stop_capture_cb_ would be
accessed during the Run().

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

Done.

+  }
   new_frame_callback_.Reset();
   started_callback_.Reset();
   source_formats_callback_.Reset();
 }
 
-void VideoCapturerDelegate::OnStarted(media::VideoCapture* capture) {
-  DVLOG(3) << "VideoCapturerDelegate::OnStarted";
-  DCHECK(!message_loop_proxy_->BelongsToCurrentThread());
-}
-
-void VideoCapturerDelegate::OnStopped(media::VideoCapture* capture) {
-  DCHECK(!message_loop_proxy_->BelongsToCurrentThread());
-}
-
-void VideoCapturerDelegate::OnPaused(media::VideoCapture* capture) {
-  DCHECK(!message_loop_proxy_->BelongsToCurrentThread());
-}
-
-void VideoCapturerDelegate::OnError(media::VideoCapture* capture,
-                                    int error_code) {
-  DVLOG(3) << "VideoCapturerDelegate::OnError";
-  DCHECK(!message_loop_proxy_->BelongsToCurrentThread());
-  message_loop_proxy_->PostTask(
-      FROM_HERE,
-      base::Bind(&VideoCapturerDelegate::OnErrorOnRenderThread, this, capture));
-}
-
-void VideoCapturerDelegate::OnRemoved(media::VideoCapture* capture) {
-  DVLOG(3) << " MediaStreamVideoCapturerSource::OnRemoved";
-  DCHECK(!message_loop_proxy_->BelongsToCurrentThread());
-
-  // Balance the AddRef in StartDeliver.
-  // This means we are no longer registered as an event handler and can safely
-  // be deleted.
-  Release();
-}
-
-void VideoCapturerDelegate::OnFrameReady(
-    media::VideoCapture* capture,
-    const scoped_refptr<media::VideoFrame>& frame) {
-  DCHECK(!message_loop_proxy_->BelongsToCurrentThread());
-  message_loop_proxy_->PostTask(
-      FROM_HERE,
-      base::Bind(&VideoCapturerDelegate::OnFrameReadyOnRenderThread,
-                 this,
-                 capture,
-                 frame));
-}
-
 void VideoCapturerDelegate::OnFrameReadyOnRenderThread(
-    media::VideoCapture* capture,
-    const scoped_refptr<media::VideoFrame>& frame) {
+    const scoped_refptr<media::VideoFrame>& frame,
+    const media::VideoCaptureFormat& format,
+    const base::TimeTicks& timestamp) {
   if (!got_first_frame_) {
     got_first_frame_ = true;
     if (!started_callback_.is_null())
@@ -165,10 +140,11 @@ void VideoCapturerDelegate::OnFrameReadyOnRenderThread(
   }
 }
 
-void VideoCapturerDelegate::OnErrorOnRenderThread(
-    media::VideoCapture* capture) {
-  if (!started_callback_.is_null())
+void VideoCapturerDelegate::OnStateUpdateOnRenderThread(
+    VideoCaptureState state) {
+  if (state == VIDEO_CAPTURE_STATE_ERROR && !started_callback_.is_null()) {

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

what happens if error is delivered before StartCapture()?
(seems like a DCHECK is in order here instead of the if?)

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

Because stopping of VCIM is not synchronous and we reset started_callback_ in
StopDeliver(). In rare case it is possible that right after calling
StopDeliver() this object is notified of an error in capturing. 

     started_callback_.Run(false);
+  }
 }
 
 void VideoCapturerDelegate::OnDeviceFormatsInUseReceived(
@@ -182,12 +158,19 @@ void VideoCapturerDelegate::OnDeviceFormatsInUseReceived(
   if (!formats_in_use.empty()) {
     source_formats_callback_.Run(formats_in_use);
     source_formats_callback_.Reset();
-  } else {
-    // If there are no formats in use, try to retrieve the whole list of
-    // supported formats.
-    capture_engine_->GetDeviceSupportedFormats(base::Bind(
-        &VideoCapturerDelegate::OnDeviceSupportedFormatsEnumerated, this));
+    return;
   }
+
+  // If there are no formats in use, try to retrieve the whole list of
+  // supported formats.
+  VideoCaptureImplManager* manager =
+      RenderThreadImpl::current()->video_capture_impl_manager();
+  manager->GetDeviceSupportedFormats(
+      session_id_,
+      media::BindToCurrentLoop(
+          base::Bind(
+              &VideoCapturerDelegate::OnDeviceSupportedFormatsEnumerated,
+              this)));
 }
 
 void VideoCapturerDelegate::OnDeviceSupportedFormatsEnumerated(