Make VideoCaptureController single-threaded and not ref counted.

content/browser/renderer_host/media/video_capture_controller.cc

Index: content/browser/renderer_host/media/video_capture_controller.cc
diff --git a/content/browser/renderer_host/media/video_capture_controller.cc b/content/browser/renderer_host/media/video_capture_controller.cc
index dbafeba49223d38b2edec3f738977f6241e8898c..772b412c324d1dc14faab95ea9f77ca81c342ce6 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -94,13 +94,31 @@ struct VideoCaptureController::ControllerClient {
 };
 
 VideoCaptureController::VideoCaptureController()
-    : chopped_width_(0),
-      chopped_height_(0),
-      frame_info_available_(false),
-      state_(VIDEO_CAPTURE_STATE_STARTED) {
+    : frame_info_available_(false),
+      state_(VIDEO_CAPTURE_STATE_STARTED),
+      weak_ptr_factory_(this) {
   memset(&current_params_, 0, sizeof(current_params_));
 }
 
+VideoCaptureController::VideoCaptureDeviceClient::VideoCaptureDeviceClient(
+    const base::WeakPtr<VideoCaptureController>& controller)
+    : controller_(controller),
+      chopped_width_(0),
+      chopped_height_(0) {}
+
+VideoCaptureController::VideoCaptureDeviceClient::~VideoCaptureDeviceClient() {}
+
+base::WeakPtr<VideoCaptureController> VideoCaptureController::AsWeakPtr() {
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+scoped_ptr<media::VideoCaptureDevice::EventHandler>
+VideoCaptureController::NewDeviceClient() {
+  scoped_ptr<media::VideoCaptureDevice::EventHandler> result(
+    new VideoCaptureDeviceClient(this->AsWeakPtr()));
+  return result.Pass();

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

l.117-119 are equiv to
return make_scoped_ptr(new VideoCaptureDeviceClient(this->AsWeakPtr())));

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

Actually, clang throws a hissy fit on that. Because we're casting to the
interface type, I'd have to use
PassAs<media::VideoCaptureDevice::EventHandler>() or a
static_cast<media::VideoCaptureDevice::EventHandler> around the operator new. I
thought this was more readable that those other options.

+}
+
 void VideoCaptureController::AddClient(
     const VideoCaptureControllerID& id,
     VideoCaptureControllerEventHandler* event_handler,
@@ -165,8 +183,7 @@ int VideoCaptureController::RemoveClient(
   return session_id;
 }
 
-void VideoCaptureController::StopSession(
-    int session_id) {
+void VideoCaptureController::StopSession(int session_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DVLOG(1) << "VideoCaptureController::StopSession, id " << session_id;
 
@@ -198,20 +215,17 @@ void VideoCaptureController::ReturnBuffer(
   buffer_pool_->RelinquishConsumerHold(buffer_id, 1);
 }
 
-scoped_refptr<media::VideoFrame> VideoCaptureController::ReserveOutputBuffer() {
-  base::AutoLock lock(buffer_pool_lock_);
-  if (!buffer_pool_.get())
-    return NULL;
+scoped_refptr<media::VideoFrame>
+VideoCaptureController::VideoCaptureDeviceClient::ReserveOutputBuffer() {
   return buffer_pool_->ReserveI420VideoFrame(gfx::Size(frame_info_.width,
                                                        frame_info_.height),
                                              0);
 }
 
-// Implements VideoCaptureDevice::EventHandler.
 // OnIncomingCapturedFrame is called the thread running the capture device.
 // I.e.- DirectShow thread on windows and v4l2_thread on Linux.
 #if !defined(OS_IOS) && !defined(OS_ANDROID)
-void VideoCaptureController::OnIncomingCapturedFrame(
+void VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedFrame(
     const uint8* data,
     int length,
     base::Time timestamp,
@@ -220,14 +234,10 @@ void VideoCaptureController::OnIncomingCapturedFrame(
     bool flip_horiz) {
   TRACE_EVENT0("video", "VideoCaptureController::OnIncomingCapturedFrame");
 
-  scoped_refptr<media::VideoFrame> dst;
-  {
-    base::AutoLock lock(buffer_pool_lock_);
-    if (!buffer_pool_.get())
-      return;
-    dst = buffer_pool_->ReserveI420VideoFrame(
-        gfx::Size(frame_info_.width, frame_info_.height), rotation);
-  }
+  if (!buffer_pool_.get())
+    return;
+  scoped_refptr<media::VideoFrame> dst = buffer_pool_->ReserveI420VideoFrame(
+      gfx::Size(frame_info_.width, frame_info_.height), rotation);
 
   if (!dst.get())
     return;
@@ -336,12 +346,12 @@ void VideoCaptureController::OnIncomingCapturedFrame(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(&VideoCaptureController::DoIncomingCapturedFrameOnIOThread,
-                 this,
+                 controller_,
                  dst,
                  timestamp));
 }
 #else
-void VideoCaptureController::OnIncomingCapturedFrame(
+void VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedFrame(
     const uint8* data,
     int length,
     base::Time timestamp,
@@ -354,15 +364,12 @@ void VideoCaptureController::OnIncomingCapturedFrame(
 
   TRACE_EVENT0("video", "VideoCaptureController::OnIncomingCapturedFrame");
 
-  scoped_refptr<media::VideoFrame> dst;
-  {
-    base::AutoLock lock(buffer_pool_lock_);
-    if (!buffer_pool_.get())
-      return;
-    dst = buffer_pool_->ReserveI420VideoFrame(gfx::Size(frame_info_.width,
-                                                        frame_info_.height),
-                                              rotation);
-  }
+  if (!buffer_pool_)

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

There's a crbug to not need to defend against this, right?
(TODO-point at it here?)

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

Yes. I've added the TODO to a different place in the file, where we create and
share the buffer pool.

+    return;
+  scoped_refptr<media::VideoFrame> dst =
+      buffer_pool_->ReserveI420VideoFrame(gfx::Size(frame_info_.width,
+                                                    frame_info_.height),
+                                          rotation);
 
   if (!dst.get())
     return;
@@ -426,39 +433,37 @@ void VideoCaptureController::OnIncomingCapturedFrame(
   BrowserThread::PostTask(BrowserThread::IO,
       FROM_HERE,
       base::Bind(&VideoCaptureController::DoIncomingCapturedFrameOnIOThread,
-                 this, dst, timestamp));
+                 controller_, dst, timestamp));
 }
 #endif  // #if !defined(OS_IOS) && !defined(OS_ANDROID)
 
 // OnIncomingCapturedVideoFrame is called the thread running the capture device.

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

english

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

Deleted and elaborated the class comment.

-void VideoCaptureController::OnIncomingCapturedVideoFrame(
+void
+VideoCaptureController::VideoCaptureDeviceClient::OnIncomingCapturedVideoFrame(
     const scoped_refptr<media::VideoFrame>& frame,
     base::Time timestamp) {
 
-  scoped_refptr<media::VideoFrame> target;
-  {
-    base::AutoLock lock(buffer_pool_lock_);
-
-    if (!buffer_pool_.get())
-      return;
-
-    // If this is a frame that belongs to the buffer pool, we can forward it
-    // directly to the IO thread and be done.
-    if (buffer_pool_->RecognizeReservedBuffer(
-        frame->shared_memory_handle()) >= 0) {
-      BrowserThread::PostTask(BrowserThread::IO,
-          FROM_HERE,
-          base::Bind(&VideoCaptureController::DoIncomingCapturedFrameOnIOThread,
-                     this, frame, timestamp));
-      return;
-    }
-    // Otherwise, this is a frame that belongs to the caller, and we must copy
-    // it to a frame from the buffer pool.
-    target = buffer_pool_->ReserveI420VideoFrame(gfx::Size(frame_info_.width,
-                                                           frame_info_.height),
-                                                 0);
+  if (!buffer_pool_)
+    return;
+
+  // If this is a frame that belongs to the buffer pool, we can forward it
+  // directly to the IO thread and be done.
+  if (buffer_pool_->RecognizeReservedBuffer(
+      frame->shared_memory_handle()) >= 0) {
+    BrowserThread::PostTask(BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&VideoCaptureController::DoIncomingCapturedFrameOnIOThread,
+                   controller_, frame, timestamp));
+    return;
   }
 
+  // Otherwise, this is a frame that belongs to the caller, and we must copy
+  // it to a frame from the buffer pool.
+  scoped_refptr<media::VideoFrame> target =
+      buffer_pool_->ReserveI420VideoFrame(gfx::Size(frame_info_.width,
+                                                    frame_info_.height),
+                                          0);
+
   if (!target.get())
     return;
 
@@ -545,18 +550,18 @@ void VideoCaptureController::OnIncomingCapturedVideoFrame(
   BrowserThread::PostTask(BrowserThread::IO,
       FROM_HERE,
       base::Bind(&VideoCaptureController::DoIncomingCapturedFrameOnIOThread,
-                 this, target, timestamp));
+                 controller_, target, timestamp));
 }
 
-void VideoCaptureController::OnError() {
+void VideoCaptureController::VideoCaptureDeviceClient::OnError() {
   BrowserThread::PostTask(BrowserThread::IO,
       FROM_HERE,
-      base::Bind(&VideoCaptureController::DoErrorOnIOThread, this));
+      base::Bind(&VideoCaptureController::DoErrorOnIOThread, controller_));
 }
 
-void VideoCaptureController::OnFrameInfo(
+void VideoCaptureController::VideoCaptureDeviceClient::OnFrameInfo(
     const media::VideoCaptureCapability& info) {
-  frame_info_= info;
+  frame_info_ = info;
   // Handle cases when |info| has odd numbers for width/height.
   if (info.width & 1) {
     --frame_info_.width;
@@ -570,17 +575,35 @@ void VideoCaptureController::OnFrameInfo(
   } else {
     chopped_height_ = 0;
   }
+
+  DCHECK(!buffer_pool_.get());
+
+  buffer_pool_ = new VideoCaptureBufferPool(
+      media::VideoFrame::AllocationSize(
+            media::VideoFrame::I420,
+            gfx::Size(frame_info_.width, frame_info_.height)),
+      kNoOfBuffers);
+
+  // Check whether all buffers were created successfully.
+  if (!buffer_pool_->Allocate()) {
+    // Transition to the error state.
+    buffer_pool_ = NULL;
+    OnError();
+    return;
+  }
+
   BrowserThread::PostTask(BrowserThread::IO,
       FROM_HERE,
-      base::Bind(&VideoCaptureController::DoFrameInfoOnIOThread, this));
+      base::Bind(&VideoCaptureController::DoFrameInfoOnIOThread, controller_,
+                 frame_info_, buffer_pool_));
 }
 
-void VideoCaptureController::OnFrameInfoChanged(
+void VideoCaptureController::VideoCaptureDeviceClient::OnFrameInfoChanged(
     const media::VideoCaptureCapability& info) {
   BrowserThread::PostTask(BrowserThread::IO,
       FROM_HERE,
       base::Bind(&VideoCaptureController::DoFrameInfoChangedOnIOThread,
-                 this, info));
+                 controller_, info));
 }
 
 VideoCaptureController::~VideoCaptureController() {
@@ -621,7 +644,9 @@ void VideoCaptureController::DoIncomingCapturedFrameOnIOThread(
   buffer_pool_->HoldForConsumers(buffer_id, count);
 }
 
-void VideoCaptureController::DoFrameInfoOnIOThread() {
+void VideoCaptureController::DoFrameInfoOnIOThread(
+    const media::VideoCaptureCapability& frame_info,
+    const scoped_refptr<VideoCaptureBufferPool>& buffer_pool) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK(!buffer_pool_.get()) << "Frame info should happen only once.";
 
@@ -629,24 +654,9 @@ void VideoCaptureController::DoFrameInfoOnIOThread() {
   if (state_ != VIDEO_CAPTURE_STATE_STARTED)
     return;
 
-  scoped_refptr<VideoCaptureBufferPool> buffer_pool =
-      new VideoCaptureBufferPool(
-          media::VideoFrame::AllocationSize(
-              media::VideoFrame::I420,
-              gfx::Size(frame_info_.width, frame_info_.height)),
-          kNoOfBuffers);
-
-  // Check whether all buffers were created successfully.
-  if (!buffer_pool->Allocate()) {
-    DoErrorOnIOThread();
-    return;
-  }
-
-  {
-    base::AutoLock lock(buffer_pool_lock_);
-    buffer_pool_ = buffer_pool;
-  }
+  frame_info_ = frame_info;
   frame_info_available_ = true;
+  buffer_pool_ = buffer_pool;
 
   for (ControllerClients::iterator client_it = controller_clients_.begin();
        client_it != controller_clients_.end(); ++client_it) {