Added video capture capabilities retrieval and caching to VideoCaptureManager

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 a1fcad7b6a6071a1b8b23349ffee9ce728dcde02..bb95da5a05ac41da987f0db66fd4431d724520c6 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -39,6 +39,18 @@ VideoCaptureManager::DeviceEntry::DeviceEntry(
 
 VideoCaptureManager::DeviceEntry::~DeviceEntry() {}
 
+VideoCaptureManager::DeviceInfo::DeviceInfo() {}
+
+VideoCaptureManager::DeviceInfo::DeviceInfo(
+    const media::VideoCaptureDevice::Name& name,
+    const media::VideoCaptureCapabilities& capabilities)
+    : device_in_use_(false),
+      capability_in_use_(),
+      name_(name),
+      capabilities_(capabilities) {}
+
+VideoCaptureManager::DeviceInfo::~DeviceInfo() {}
+
 VideoCaptureManager::VideoCaptureManager()
     : listener_(NULL),
       new_capture_session_id_(1),
@@ -68,10 +80,13 @@ void VideoCaptureManager::EnumerateDevices(MediaStreamType stream_type) {
   DVLOG(1) << "VideoCaptureManager::EnumerateDevices, type " << stream_type;
   DCHECK(listener_);
   base::PostTaskAndReplyWithResult(
-      device_loop_, FROM_HERE,
-      base::Bind(&VideoCaptureManager::GetAvailableDevicesOnDeviceThread, this,
+      device_loop_,
+      FROM_HERE,
+      base::Bind(&VideoCaptureManager::GetAvailableDevicesOnDeviceThread,
+                 this,
                  stream_type),
-      base::Bind(&VideoCaptureManager::OnDevicesEnumerated, this, stream_type));
+      base::Bind(
+          &VideoCaptureManager::OnDeviceNamesEnumerated, this, stream_type));
 }
 
 int VideoCaptureManager::Open(const StreamDeviceInfo& device_info) {
@@ -133,6 +148,7 @@ void VideoCaptureManager::UseFakeDevice() {
 
 void VideoCaptureManager::DoStartDeviceOnDeviceThread(
     DeviceEntry* entry,
+    const media::VideoCaptureDevice::Name* device_name,
     const media::VideoCaptureCapability& capture_params,
     scoped_ptr<media::VideoCaptureDevice::Client> device_client) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StartDeviceTime");
@@ -144,12 +160,10 @@ void VideoCaptureManager::DoStartDeviceOnDeviceThread(
       // 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::VideoCaptureDevice::Name* found =
-          video_capture_devices_.FindById(entry->id);
-      if (found) {
-        video_capture_device.reset(use_fake_device_ ?
-            media::FakeVideoCaptureDevice::Create(*found) :
-            media::VideoCaptureDevice::Create(*found));
+      if (device_name) {
+        video_capture_device.reset(use_fake_device_
+            ? media::FakeVideoCaptureDevice::Create(*device_name)
+            : media::VideoCaptureDevice::Create(*device_name));
       }
       break;
     }
@@ -216,9 +230,16 @@ void VideoCaptureManager::StartCaptureForClient(
     params_as_capability.frame_size_type =
         params.requested_format.frame_size_type;
 
+    DeviceInfo* found = FindDeviceInfoById(entry->id);
+    if (found) {
+      found->device_in_use_ = true;
+      found->capability_in_use_ = params_as_capability;
+    }
     device_loop_->PostTask(FROM_HERE, base::Bind(
         &VideoCaptureManager::DoStartDeviceOnDeviceThread, this,
-        entry, params_as_capability,
+        entry,
+        (found ? & found->name_ : NULL),
+        params_as_capability,
         base::Passed(entry->video_capture_controller->NewDeviceClient())));
   }
   // Run the callback first, as AddClient() may trigger OnFrameInfo().
@@ -252,6 +273,26 @@ void VideoCaptureManager::StopCaptureForClient(
   DestroyDeviceEntryIfNoClients(entry);
 }
 
+void VideoCaptureManager::GetDeviceCapabilities(
+    int capture_session_id,
+    media::VideoCaptureCapabilities* capabilities) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  capabilities->clear();
+
+  std::map<int, MediaStreamDevice>::iterator it =
+      sessions_.find(capture_session_id);
+  DCHECK(it != sessions_.end());
+  DVLOG(1) << "GetDeviceCapabilities for device: " << it->second.name;
+
+  DeviceInfo* device = FindDeviceInfoById(it->second.id);
+  if (device) {
+    if (device->device_in_use_)
+      capabilities->push_back(device->capability_in_use_);
+    else
+      *capabilities = device->capabilities_;
+  }
+}
+
 void VideoCaptureManager::DoStopDeviceOnDeviceThread(DeviceEntry* entry) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StopDeviceTime");
   DCHECK(IsOnDeviceThread());
@@ -281,25 +322,70 @@ void VideoCaptureManager::OnClosed(MediaStreamType stream_type,
   listener_->Closed(stream_type, capture_session_id);
 }
 
-void VideoCaptureManager::OnDevicesEnumerated(
+void VideoCaptureManager::OnDeviceNamesEnumerated(
     MediaStreamType stream_type,
-    const media::VideoCaptureDevice::Names& device_names) {
+    const media::VideoCaptureDevice::Names& names_snapshot) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   if (!listener_) {
     // Listener has been removed.
     return;
   }
 
-  // Transform from VCD::Name to StreamDeviceInfo.
+  // We have the names of the current devices in |device_names|, select the new
+  // devices for capabilities retrieval, put them in |new_device_names|.
+  media::VideoCaptureDevice::Names new_device_names;

[perkj_chrome, 2013/11/05 09:34:34]

suggest something like

|names_snapshot| contain all currently available devices.
We need to retrieve the capabilities of all new devices.

[mcasas, 2013/11/05 22:46:06]

Done.

+  media::VideoCaptureDevice::Names::const_iterator it;

[perkj_chrome, 2013/11/05 09:34:34]

please try to be consistent with existing for loops and move the iterator to the
scope of the for loop.
Here and else where.

for (media::VideoCaptureDevice::Names::const_iterator it = 
         names_snapshot.begin(); ...

[mcasas, 2013/11/05 22:46:06]

I think it clutters the lines a lot, but hey you're the reviewer. Done.

+  for (it = names_snapshot.begin(); it != names_snapshot.end(); ++it) {
+    DeviceInfo* device_info = FindDeviceInfoById(it->id());
+    if (!device_info)
+      new_device_names.push_back(*it);
+  }
+  base::PostTaskAndReplyWithResult(
+      device_loop_, FROM_HERE,
+      base::Bind(&VideoCaptureManager::GetDevicesCapabilitiesOnDeviceThread,
+                 this, new_device_names),
+      base::Bind(&VideoCaptureManager::OnDeviceNameAndCapabilitiesEnumerated,
+                 this, stream_type, names_snapshot));
+}
+
+void VideoCaptureManager::OnDeviceNameAndCapabilitiesEnumerated(
+    MediaStreamType stream_type,
+    const media::VideoCaptureDevice::Names& names_snapshot,
+    ScopedVector<DeviceInfo> new_devices_info) {

[perkj_chrome, 2013/11/05 09:34:34]

I think ScopedVector<DeviceInfo> new_devices_info should be const ref as well
instead of creating an implicit copy here.

If that is done - would it be easier to remove unexisting devices from 
|devices_info_cache_| in OnDeviceNamesEnumerated ? 
In that case you don't need |names_snapshot| here at all.

Move to OnDeviceNamesEnumerated
and do something like

ScopedVector<DeviceInfo>::iterator it_device_info =        
devices_info_cache_.begin();
while(it_device_info != devices_info_cache_.end()) {
 if !FindDeviceNameWithId(names_snapshot, it_device_info->id)
    devices_info_cache_.erase(it_device_info++)
 else
   ++it_device_info;
}

[mcasas, 2013/11/05 22:46:06]

Moved to OnDeviceNamesEnumerated: First search for devices that are in the cache
but not on the current system snapshot, and remove them from the cache. Then
compose the full new devices list and retrieve their capabilities.

+  DVLOG(1) << "OnDeviceNameAndCapabilitiesEnumerated, #new devices: "
+           << new_devices_info.size();
+
+  // Walk the system snapshot names and add from |devices_info_cache_| to the
+  // |new_device_info| all those elements still present. Then just swap
+  // |new_device_info| and |devices_info_cache|.
+  for (media::VideoCaptureDevice::Names::const_iterator it_name =
+      names_snapshot.begin();
+      it_name != names_snapshot.end();
+      ++it_name) {
+    for (ScopedVector<DeviceInfo>::iterator it_device_info =
+        devices_info_cache_.begin();
+        it_device_info != devices_info_cache_.end();
+        ++it_device_info) {
+      if ((*it_device_info)->name_.id() == it_name->id()) {
+        new_devices_info.push_back(new DeviceInfo(
+            (*it_device_info)->name_, (*it_device_info)->capabilities_));
+      }
+    }
+  }
+  devices_info_cache_.swap(new_devices_info);
+
+  // Walk the returned |device_names| and transform from VCD::Name to

[perkj_chrome, 2013/11/05 09:34:34]

Please add something like 
|names_snapshot| contain all available devices.
|new_devices_info| contain capabilities of new devices.

[mcasas, 2013/11/05 22:46:06]

This comment does not make sense.

+  // StreamDeviceInfo for return purposes.
   StreamDeviceInfoArray devices;
-  for (media::VideoCaptureDevice::Names::const_iterator it =
-           device_names.begin(); it != device_names.end(); ++it) {
+  ScopedVector<DeviceInfo>::const_iterator it;

[perkj_chrome, 2013/11/05 09:34:34]

dito

[mcasas, 2013/11/05 22:46:06]

Done.

+  for (it = devices_info_cache_.begin(); it != devices_info_cache_.end();
+       ++it) {
     devices.push_back(StreamDeviceInfo(
-        stream_type, it->GetNameAndModel(), it->id()));
+        stream_type, (*it)->name_.GetNameAndModel(), (*it)->name_.id()));
   }
-
-  listener_->DevicesEnumerated(stream_type, devices);
+  // |listener_| might have disappeared while we were scanning the devices.
+  if(listener_)
+    listener_->DevicesEnumerated(stream_type, devices);
 }
 
 bool VideoCaptureManager::IsOnDeviceThread() const {
@@ -312,23 +398,14 @@ VideoCaptureManager::GetAvailableDevicesOnDeviceThread(
   SCOPED_UMA_HISTOGRAM_TIMER(
       "Media.VideoCaptureManager.GetAvailableDevicesTime");
   DCHECK(IsOnDeviceThread());
-  media::VideoCaptureDevice::Names result;
+  media::VideoCaptureDevice::Names names;
 
   switch (stream_type) {
     case MEDIA_DEVICE_VIDEO_CAPTURE:
-      // Cache the latest enumeration of video capture devices.
-      // We'll refer to this list again in OnOpen to avoid having to
-      // enumerate the devices again.
-      if (!use_fake_device_) {
-        media::VideoCaptureDevice::GetDeviceNames(&result);
-      } else {
-        media::FakeVideoCaptureDevice::GetDeviceNames(&result);
-      }
-
-      // TODO(nick): The correctness of device start depends on this cache being
-      // maintained, but it seems a little odd to keep a cache here. Can we
-      // eliminate it?
-      video_capture_devices_ = result;
+      if (!use_fake_device_)
+        media::VideoCaptureDevice::GetDeviceNames(&names);
+      else
+        media::FakeVideoCaptureDevice::GetDeviceNames(&names);
       break;
 
     case MEDIA_DESKTOP_VIDEO_CAPTURE:
@@ -339,7 +416,28 @@ VideoCaptureManager::GetAvailableDevicesOnDeviceThread(
       NOTREACHED();
       break;
   }
-  return result;
+  return names;
+}
+
+ScopedVector<VideoCaptureManager::DeviceInfo>
+VideoCaptureManager::GetDevicesCapabilitiesOnDeviceThread(
+    const media::VideoCaptureDevice::Names& device_names) {
+  DCHECK(IsOnDeviceThread());
+  DVLOG(1) << "GetDevicesCapabilitiesOnDeviceThread";
+
+  ScopedVector<DeviceInfo> device_info_vector;
+  media::VideoCaptureDevice::Names::const_iterator it;

[perkj_chrome, 2013/11/05 09:34:34]

dito

[mcasas, 2013/11/05 22:46:06]

Done.

+  for (it = device_names.begin(); it != device_names.end(); ++it) {
+    media::VideoCaptureCapabilities capabilities;
+    if (!use_fake_device_) {
+      media::VideoCaptureDevice::GetDeviceSupportedFormats(*it, &capabilities);
+    } else {
+      media::FakeVideoCaptureDevice::GetDeviceSupportedFormats(*it,
+                                                               &capabilities);
+    }
+    device_info_vector.push_back(new DeviceInfo(*it, capabilities));

[perkj_chrome, 2013/11/05 09:34:34]

This creates a copy of capabilities.
How about creating the DeviceInfo before calling GetDeviceSupportedFormats
and pass device_info->capabilities to GetDeviceSupportedFormats to avoid a copy.

[mcasas, 2013/11/05 22:46:06]

Done.

+  }
+  return device_info_vector.Pass();
 }
 
 VideoCaptureManager::DeviceEntry*
@@ -386,8 +484,12 @@ void VideoCaptureManager::DestroyDeviceEntryIfNoClients(DeviceEntry* entry) {
     entry->video_capture_controller.reset();
     device_loop_->PostTask(
         FROM_HERE,
-        base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread, this,
+        base::Bind(&VideoCaptureManager::DoStopDeviceOnDeviceThread,
+                   this,
                    base::Owned(entry)));
+    DeviceInfo* device_info = FindDeviceInfoById(entry->id);
+    if (device_info)
+      device_info->device_in_use_ = false;
   }
 }
 
@@ -420,4 +522,16 @@ VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
   return new_device;
 }
 
+VideoCaptureManager::DeviceInfo* VideoCaptureManager::FindDeviceInfoById(
+    const std::string& id) {
+  ScopedVector<DeviceInfo>::iterator it;

[perkj_chrome, 2013/11/05 09:34:34]

dito

[mcasas, 2013/11/05 22:46:06]

Done.

+  for (it = devices_info_cache_.begin();
+       it != devices_info_cache_.end();
+       ++it) {
+    if ((*it)->name_.id() == id)
+      return *it;
+  }
+  return NULL;
+}
+
 }  // namespace content