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..b3942ee43ede9856ae022221b2cbdf839f851047 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -39,6 +39,15 @@ VideoCaptureManager::DeviceEntry::DeviceEntry(
 
 VideoCaptureManager::DeviceEntry::~DeviceEntry() {}
 
+VideoCaptureManager::DeviceInfo::DeviceInfo() {}
+
+VideoCaptureManager::DeviceInfo::DeviceInfo(
+    const media::VideoCaptureDevice::Name& name,
+    const media::VideoCaptureCapabilities& capabilities)
+    : name_(name), capabilities_(capabilities) {}
+
+VideoCaptureManager::DeviceInfo::~DeviceInfo() {}
+
 VideoCaptureManager::VideoCaptureManager()
     : listener_(NULL),
       new_capture_session_id_(1),
@@ -133,6 +142,7 @@ void VideoCaptureManager::UseFakeDevice() {
 
 void VideoCaptureManager::DoStartDeviceOnDeviceThread(
     DeviceEntry* entry,
+    DeviceInfo* device_info,

[perkj_chrome, 2013/10/31 09:46:00]

can you just pass the media::VideoCaptureDevice::Name instead of the device_info
here. It does not look like you need anything else.

[mcasas, 2013/10/31 14:25:23]

Done.

     const media::VideoCaptureCapability& capture_params,
     scoped_ptr<media::VideoCaptureDevice::Client> device_client) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StartDeviceTime");
@@ -144,12 +154,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) {
+      if (device_info) {
         video_capture_device.reset(use_fake_device_ ?
-            media::FakeVideoCaptureDevice::Create(*found) :
-            media::VideoCaptureDevice::Create(*found));
+            media::FakeVideoCaptureDevice::Create(device_info->name_) :
+            media::VideoCaptureDevice::Create(device_info->name_));
       }
       break;
     }
@@ -216,10 +224,18 @@ void VideoCaptureManager::StartCaptureForClient(
     params_as_capability.frame_size_type =
         params.requested_format.frame_size_type;
 
+    DeviceInfo* found = FindDeviceInfoById(entry->id);
     device_loop_->PostTask(FROM_HERE, base::Bind(
         &VideoCaptureManager::DoStartDeviceOnDeviceThread, this,
-        entry, params_as_capability,
+        entry, found, params_as_capability,

[perkj_chrome, 2013/10/31 09:46:00]

Just pass the name? 

[mcasas, 2013/10/31 14:25:23]

Done.

         base::Passed(entry->video_capture_controller->NewDeviceClient())));
+
+    if (found) {
+      // After opening a device, we currently assume that it cannot be opened
+      // in any other format than the actual, so we clear the capabilities.
+      found->capabilities_.clear();
+      found->capabilities_.push_back(params_as_capability);
+    }
   }
   // Run the callback first, as AddClient() may trigger OnFrameInfo().
   done_cb.Run(entry->video_capture_controller->GetWeakPtr());
@@ -252,6 +268,22 @@ void VideoCaptureManager::StopCaptureForClient(
   DestroyDeviceEntryIfNoClients(entry);
 }
 
+media::VideoCaptureCapabilities VideoCaptureManager::GetDeviceCapabilities(

[perkj_chrome, 2013/10/31 09:46:00]

would it make more sence to have this

void GetDeviceCapabilities(int session_id, media::VideoCaptureCapabilities*) ? 

that way you don't need to create a new array.

[mcasas, 2013/10/31 14:25:23]

Done.

+    int capture_session_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  std::map<int, MediaStreamDevice>::iterator it =
+      sessions_.find(capture_session_id);
+  if (it != sessions_.end()) {

[perkj_chrome, 2013/10/31 09:46:00]

suggest (D)CHECK(it != sessions_end()) - It should never happen that we ask for
capabilies of an unknown session_id.

[mcasas, 2013/10/31 14:25:23]

Done.

+    DVLOG(1) << "EnumerateDeviceCapabilites for: " << it->second.name;

[perkj_chrome, 2013/10/31 09:46:00]

nit: This function is not called EnumerateDeviceCapabilites

[mcasas, 2013/10/31 14:25:23]

Done.

+
+    DeviceInfo* device = FindDeviceInfoById(it->second.id);
+    if (device)
+      return device->capabilities_;
+  }
+  return media::VideoCaptureCapabilities();
+}
+
 void VideoCaptureManager::DoStopDeviceOnDeviceThread(DeviceEntry* entry) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StopDeviceTime");
   DCHECK(IsOnDeviceThread());
@@ -285,18 +317,29 @@ void VideoCaptureManager::OnDevicesEnumerated(
     MediaStreamType stream_type,
     const media::VideoCaptureDevice::Names& device_names) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-
   if (!listener_) {
     // Listener has been removed.
     return;
   }
 
-  // Transform from VCD::Name to StreamDeviceInfo.
+  // Walk the returned |device_names| do: a) transform from VCD::Name to
+  // StreamDeviceInfo, and b) at the same time retrieve device capture
+  // capabilities to create the |devices_info_cache_| - skipping known devices.
   StreamDeviceInfoArray devices;
+  media::VideoCaptureCapabilities capabilities;
   for (media::VideoCaptureDevice::Names::const_iterator it =
            device_names.begin(); it != device_names.end(); ++it) {
-    devices.push_back(StreamDeviceInfo(
-        stream_type, it->GetNameAndModel(), it->id()));
+    devices.push_back(
+        StreamDeviceInfo(stream_type, it->GetNameAndModel(), it->id()));
+    if (FindDeviceInfoById(it->id()))
+      continue;
+    if (!use_fake_device_) {
+      media::VideoCaptureDevice::GetDeviceSupportedFormats(*it, &capabilities);

[perkj_chrome, 2013/10/31 09:46:00]

We do not want to actually call this function on the IO thread, since it can
take a substantial amount of time depending on OS and drivers. That must still
happen on the device thread and it must have completed before we call
listener_->DevicesEnumerated(stream_type, devices) to avoid a potential race.

Suggestion: When enumeration is called: 
1. Get the available names on device thread -> return to io thread 
2. For each new name -> retrieve capabilities and return to IO thread. You can
post an array with all new names to the device thread.
3. For each removed name- > remove from DeviceInfo array
4. When the process above is done -> call listener->DevicesEnumerated.

[mcasas, 2013/10/31 14:25:23]

Done.

+    } else {
+      media::FakeVideoCaptureDevice::GetDeviceSupportedFormats(*it,
+                                                               &capabilities);
+    }
+    devices_info_cache_.push_back(new DeviceInfo(*it, capabilities));
   }
 
   listener_->DevicesEnumerated(stream_type, devices);
@@ -312,23 +355,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 +373,7 @@ VideoCaptureManager::GetAvailableDevicesOnDeviceThread(
       NOTREACHED();
       break;
   }
-  return result;
+  return names;
 }
 
 VideoCaptureManager::DeviceEntry*
@@ -386,8 +420,21 @@ 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)));
+    // When the device is closed, its capabilities are reread from the device

[perkj_chrome, 2013/10/31 09:46:00]

ok I see. May I suggest a different strategy? 
Instead of clearing and rereading capabilities- store the currently used
capability in 
VideoCaptureManager::DeviceInfo and clear it when the device is stopped. 
GetCaptureCapabilities can return only the currently used capability if it is
set.

 

[mcasas, 2013/10/31 14:25:23]

Done.

+    // to reflect that all are possible.
+    DeviceInfo* device_info = FindDeviceInfoById(entry->id);
+    if (device_info) {
+      if (!use_fake_device_) {
+        media::VideoCaptureDevice::GetDeviceSupportedFormats(
+            device_info->name_, &(device_info->capabilities_));
+      } else {
+        media::FakeVideoCaptureDevice::GetDeviceSupportedFormats(
+            device_info->name_, &(device_info->capabilities_));
+      }
+    }
   }
 }
 
@@ -420,4 +467,16 @@ VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
   return new_device;
 }
 
+VideoCaptureManager::DeviceInfo* VideoCaptureManager::FindDeviceInfoById(
+    const std::string& id) {
+  ScopedVector<DeviceInfo>::iterator it;
+  for (it = devices_info_cache_.begin();
+       it != devices_info_cache_.end();
+       ++it) {
+    if ((*it)->name_.id() == id)
+      return *it;
+  }
+  return NULL;
+}
+
 }  // namespace content