Queue commands to the Os to start a video device.

content/browser/renderer_host/media/video_capture_manager.cc

Index: content/browser/renderer_host/media/video_capture_manager.cc
diff --git a/content/browser/renderer_host/media/video_capture_manager.cc b/content/browser/renderer_host/media/video_capture_manager.cc
index 617a8696deeacb2c2f8d0ef5e8ceb1b24416b5f7..32a6b13a5c237af0459aea731fdd8bcb6b03070a 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -118,6 +118,7 @@ VideoCaptureManager::VideoCaptureManager(
 
 VideoCaptureManager::~VideoCaptureManager() {
   DCHECK(devices_.empty());
+  DCHECK(device_start_queue_.empty());
 }
 
 void VideoCaptureManager::Register(
@@ -216,22 +217,125 @@ void VideoCaptureManager::Close(int capture_session_id) {
   sessions_.erase(session_it);
 }
 
-void VideoCaptureManager::DoStartDeviceOnDeviceThread(
+VideoCaptureManager::
+CaptureDeviceStartRequest::CaptureDeviceStartRequest(

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

220-221 should fit on one line

[perkj_chrome, 2014/12/17 10:14:10]

Done.

+    DeviceEntry* entry,
+    media::VideoCaptureSessionId session_id,
+    const media::VideoCaptureParams& params)
+    : entry(entry),
+      session_id(session_id),
+      params(params) {
+}
+
+void VideoCaptureManager::QueueStartDevice(
     media::VideoCaptureSessionId session_id,
     DeviceEntry* entry,
+    const media::VideoCaptureParams& params) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  device_start_queue_.push_back(
+      CaptureDeviceStartRequest(entry, session_id, params));
+  if (device_start_queue_.size() == 1) {

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

nit: no {}

[perkj_chrome, 2014/12/17 10:14:10]

Done.

+    HandleQueuedStartRequest();
+  }
+}
+
+void VideoCaptureManager::DoStopDevice(DeviceEntry* entry) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // Check if the start request has not yet been sent to the device thread.
+  // If so, delete the request.
+  if (device_start_queue_.size() > 1) {

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

what's the reason for > 1 and not >0? (i.e. !empty())

Is there a chance that the entry might be in device_start_queue_ when size() ==
1?

[perkj_chrome, 2014/12/17 10:14:10]

Added more comments.

+    for (DeviceStartQueue::iterator request = ++device_start_queue_.begin();

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

it looks like you're incrementing begin() which can't be right.  If you want to
start at begin() + 1, then I'd prefer to use begin() + 1.

If I'm understanding this correctly though, you want to go from element [1] to
the end of the array and skip the first one.  That assumes some sort of an order
(i.e. there's something special about position [0]) which would be good to
explain if that's intentional.

[perkj_chrome, 2014/12/17 10:14:10]

I have to do ++ and not begin() + 1:
In file included from
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/list:67:
In file included from
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/list:34:
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/safe_iterator.h:361:13:
error: no viable overloaded '+='

[tommi (sloooow) - chröme, 2014/12/17 11:12:53]

That error says "error: no viable overloaded '+='"  are you sure you typed + 1?
:)

+         request != device_start_queue_.end(); ++request) {
+      if (request->entry == entry) {
+        device_start_queue_.erase(request);

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

this doesn't delete any object, right?

[perkj_chrome, 2014/12/17 10:14:10]

right.

+        DVLOG(3) << "DoStopDevice, erasing start request for device "
+                 << entry->id << ".";

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

should we assert that entry exists in the main vector at the top of this
function? (since if it doesn't, that could cause a crash).

[perkj_chrome, 2014/12/17 10:14:10]

Done.

+        return;
+      }
+    }
+  }
+
+  DVLOG(3) << "DoStopDevice. Send stop request for device " << entry->id << ".";
+  if (entry->video_capture_device.get()) {
+    // |entry->video_capture_device| can be null if creating the device fails.
+    device_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
+                   base::Passed(entry->video_capture_device.Pass())));

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

shouldn't this be base::Passed(&entry->video_capture_device)?

[perkj_chrome, 2014/12/17 10:14:10]

../../content/browser/renderer_host/media/video_capture_manager.cc:266:33:
error: calling a private constructor of class
'scoped_ptr<media::VideoCaptureDevice,
base::DefaultDeleter<media::VideoCaptureDevice> >'
                   base::Passed(entry->video_capture_device)));
                                ^
../../base/memory/scoped_ptr.h:312:51: note: declared private here
  MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03(scoped_ptr)
                                                  ^
../../base/move.h:222:3: note: expanded from macro
'MOVE_ONLY_TYPE_WITH_MOVE_CONSTRUCTOR_FOR_CPP_03'
  type(type&); \


From scoped_ptr.h

//   {
//     scoped_ptr<Foo> ptr(new Foo("yay"));  // ptr manages Foo("yay").
//     TakesOwnership(ptr.Pass());           // ptr no longer owns Foo("yay").
//     scoped_ptr<Foo> ptr2 = CreateFoo();   // ptr2 owns the return Foo.
//     scoped_ptr<Foo> ptr3 =                // ptr3 now owns what was in ptr2.
//         PassThru(ptr2.Pass());            // ptr2 is correspondingly nullptr.
//   }
//
// Notice that if you do not call Pass() when returning from PassThru(), or
// when invoking TakesOwnership(), the code will not compile because scopers
// are not copyable; 

[tommi (sloooow) - chröme, 2014/12/17 11:12:53]

you missed an ampersand.

[perkj_chrome, 2014/12/17 11:47:21]

Seems like I have not fully grasped this magic... You are right of course.

+  }
+}
+
+void VideoCaptureManager::HandleQueuedStartRequest() {
+  auto request = device_start_queue_.begin();
+  if (request == device_start_queue_.end())

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

you should be able to replace this with

if (device_start_queue.empty())
  return;

Under what circumstances do we get there then?

[perkj_chrome, 2014/12/17 10:14:10]

Rewrote.

+    return;
+
+  DCHECK(std::find(devices_.begin(), devices_.end(), request->entry) !=
+      devices_.end());
+  DeviceEntry* entry = request->entry;
+
+  DVLOG(3) << "HandleQueuedStartRequest, Post start to device thread, device = "
+           << entry->id;
+
+  base::PostTaskAndReplyWithResult(
+      device_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(
+          &VideoCaptureManager::DoStartDeviceOnDeviceThread,
+          this,
+          request->session_id,
+          entry->id,
+          entry->stream_type,
+          request->params,
+          base::Passed(entry->video_capture_controller->NewDeviceClient())),
+      base::Bind(&VideoCaptureManager::OnDeviceStarted, this, entry));
+}
+
+void VideoCaptureManager::OnDeviceStarted(
+    DeviceEntry* device_entry,
+    scoped_ptr<media::VideoCaptureDevice> device) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DVLOG(3) << "OnDeviceStarted";
+  device_start_queue_.pop_front();

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

can we DCHECK that the device we're popping is the expected one?

[perkj_chrome, 2014/12/17 10:14:10]

Done.

+
+  DeviceEntries::iterator entry_it =
+      std::find(devices_.begin(), devices_.end(), device_entry);
+  if (entry_it == devices_.end()) {
+    // |device| can be null if creation failed in DoStartDeviceOnDeviceThread.
+    // The device is no longer wanted. Stop the device again.
+    if (device.get() && !device_task_runner_->PostTask(
+        FROM_HERE,

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

indentation looks a bit weird.  Would be good go move FROM_HERE to the end of
the previous line or .. hmm..
what about something like this:

if (device.get() &&
    !device_task_runner_->PostTask(FROM_HERE,
        base::Bind(...)))) {

[perkj_chrome, 2014/12/17 10:14:10]

Done.

+        base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
+                   base::Passed(&device)))) {
+      // PostTask failed. The device must be stopped anyway.
+      device->StopAndDeAllocate();

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

has base::Passed() not nulled |device| at this point?  If it does, then deleting
the closure should delete the device object and we'd need to call
StopAndDeAllocate() before that time.  You could do something like:

auto* device_ptr = device.get();
auto closure = base::Bind(...);

if (device_ptr &&
    !device_task_runner_->PostTask(FROM_HERE, closure)) {
  device_ptr->StopAndDeallocate();
}

[perkj_chrome, 2014/12/17 10:14:10]

Done.

+    }
+  } else {
+    (*entry_it)->video_capture_device.reset(device.release());

[tommi (sloooow) - chröme, 2014/12/16 22:31:25]

nit: (*entry_it)->video_capture_device.swap(device);

Should we DCHECK first that video_capture_device.get() == nullptr ?

[perkj_chrome, 2014/12/17 10:14:10]

feels like overkill to me.

[tommi (sloooow) - chröme, 2014/12/17 11:12:53]

which one is overkill:

(*entry_it)->video_capture_device.reset(device.release());
(*entry_it)->video_capture_device.swap(device);

[perkj_chrome, 2014/12/17 11:47:21]

The dcheck. But It can never hurt.

[tommi (sloooow) - chröme, 2014/12/17 14:46:12]

Oh the dcheck, right.  Yeah, I don't see how that could happen right now but to
make sure we never get here and already have a started device that we then
implicitly delete and never call StopAndDeallocate.

+  }
+
+  HandleQueuedStartRequest();
+}
+
+scoped_ptr<media::VideoCaptureDevice>
+VideoCaptureManager::DoStartDeviceOnDeviceThread(
+    media::VideoCaptureSessionId session_id,
+    const std::string& id,
+    MediaStreamType stream_type,
     const media::VideoCaptureParams& params,
     scoped_ptr<media::VideoCaptureDevice::Client> device_client) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StartDeviceTime");
   DCHECK(IsOnDeviceThread());
 
   scoped_ptr<media::VideoCaptureDevice> video_capture_device;
-  switch (entry->stream_type) {
+  switch (stream_type) {
     case MEDIA_DEVICE_VIDEO_CAPTURE: {
       // We look up the device id from the renderer in our local enumeration
       // since the renderer does not have all the information that might be
       // held in the browser-side VideoCaptureDevice::Name structure.
       media::VideoCaptureDeviceInfo* found =
-          FindDeviceInfoById(entry->id, devices_info_cache_);
+          FindDeviceInfoById(id, devices_info_cache_);
       if (found) {
         video_capture_device =
             video_capture_device_factory_->Create(found->name);
@@ -240,20 +344,21 @@ void VideoCaptureManager::DoStartDeviceOnDeviceThread(
     }
     case MEDIA_TAB_VIDEO_CAPTURE: {
       video_capture_device.reset(
-          WebContentsVideoCaptureDevice::Create(entry->id));
+          WebContentsVideoCaptureDevice::Create(id));
       break;
     }
     case MEDIA_DESKTOP_VIDEO_CAPTURE: {
 #if defined(ENABLE_SCREEN_CAPTURE)
-      DesktopMediaID id = DesktopMediaID::Parse(entry->id);
+      DesktopMediaID desktop_id = DesktopMediaID::Parse(id);
 #if defined(USE_AURA)
-      if (id.type == DesktopMediaID::TYPE_AURA_WINDOW) {
-        video_capture_device.reset(DesktopCaptureDeviceAura::Create(id));
+      if (desktop_id.type == DesktopMediaID::TYPE_AURA_WINDOW) {
+        video_capture_device.reset(
+            DesktopCaptureDeviceAura::Create(desktop_id));
       } else
 #endif
-      if (id.type != DesktopMediaID::TYPE_NONE &&
-          id.type != DesktopMediaID::TYPE_AURA_WINDOW) {
-        video_capture_device = DesktopCaptureDevice::Create(id);
+      if (desktop_id.type != DesktopMediaID::TYPE_NONE &&
+          desktop_id.type != DesktopMediaID::TYPE_AURA_WINDOW) {
+        video_capture_device = DesktopCaptureDevice::Create(desktop_id);
         if (notification_window_ids_.find(session_id) !=
             notification_window_ids_.end()) {
           static_cast<DesktopCaptureDevice*>(video_capture_device.get())
@@ -273,11 +378,11 @@ void VideoCaptureManager::DoStartDeviceOnDeviceThread(
 
   if (!video_capture_device) {
     device_client->OnError("Could not create capture device");
-    return;
+    return nullptr;
   }
 
   video_capture_device->AllocateAndStart(params, device_client.Pass());
-  entry->video_capture_device = video_capture_device.Pass();
+  return video_capture_device.Pass();
 }
 
 void VideoCaptureManager::StartCaptureForClient(
@@ -306,16 +411,7 @@ void VideoCaptureManager::StartCaptureForClient(
   if (entry->video_capture_controller->GetActiveClientCount() == 0) {
     DVLOG(1) << "VideoCaptureManager starting device (type = "
              << entry->stream_type << ", id = " << entry->id << ")";
-
-    device_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(
-            &VideoCaptureManager::DoStartDeviceOnDeviceThread,
-            this,
-            session_id,
-            entry,
-            params,
-            base::Passed(entry->video_capture_controller->NewDeviceClient())));
+    QueueStartDevice(session_id, entry, params);
   }
   // Run the callback first, as AddClient() may trigger OnFrameInfo().
   done_cb.Run(entry->video_capture_controller->GetWeakPtr());
@@ -392,10 +488,7 @@ void VideoCaptureManager::PauseCaptureForClient(
     return;
 
   // There is no more client, release the camera.
-  device_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
-                 base::Unretained(entry)));
+  DoStopDevice(entry);
 }
 
 void VideoCaptureManager::ResumeCaptureForClient(
@@ -423,15 +516,7 @@ void VideoCaptureManager::ResumeCaptureForClient(
     return;
 
   // This is first active client, allocate the camera.
-  device_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(
-          &VideoCaptureManager::DoStartDeviceOnDeviceThread,
-          this,
-          session_id,
-          entry,
-          params,
-          base::Passed(entry->video_capture_controller->NewDeviceClient())));
+  QueueStartDevice(session_id, entry, params);
 }
 
 bool VideoCaptureManager::GetDeviceSupportedFormats(
@@ -517,13 +602,12 @@ void VideoCaptureManager::SetDesktopCaptureWindowId(
                  window_id));
 }
 
-void VideoCaptureManager::DoStopDeviceOnDeviceThread(DeviceEntry* entry) {
+void VideoCaptureManager::DoStopDeviceOnDeviceThread(
+    scoped_ptr<media::VideoCaptureDevice> device) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StopDeviceTime");
   DCHECK(IsOnDeviceThread());
-  if (entry->video_capture_device) {
-    entry->video_capture_device->StopAndDeAllocate();
-  }
-  entry->video_capture_device.reset();
+  device->StopAndDeAllocate();
+  DVLOG(3) << "DoStopDeviceOnDeviceThread";
 }
 
 void VideoCaptureManager::OnOpened(
@@ -653,12 +737,11 @@ void VideoCaptureManager::DestroyDeviceEntryIfNoClients(DeviceEntry* entry) {
     // deleted immediately, and the device is freed asynchronously. After this
     // point, subsequent requests to open this same device ID will create a new
     // DeviceEntry, VideoCaptureController, and VideoCaptureDevice.
-    devices_.erase(entry);
-    entry->video_capture_controller.reset();
-    device_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
-                   base::Owned(entry)));
+    DoStopDevice(entry);
+    DeviceEntries::iterator device_it = std::find(devices_.begin(),
+                                                  devices_.end(),
+                                                  entry);
+    devices_.erase(device_it);
   }
 }
 
@@ -688,7 +771,7 @@ VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
   DeviceEntry* new_device = new DeviceEntry(device_info.type,
                                             device_info.id,
                                             video_capture_controller.Pass());
-  devices_.insert(new_device);
+  devices_.push_back(new_device);
   return new_device;
 }