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..2b8bdbbbebbce25babc375e281d922fc583f059f 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;

[ncarter (slow), 2013/11/07 22:43:01]

The VideoCaptureManager already knows whether a particular device is in use or
not, on the basis of whether an active DeviceEntry exists with that id and type,
so I wonder if it's redundant to track it in a bool here in a parallel struct.

[mcasas, 2013/11/08 02:45:21]

That's true. It implies an extra search, but that's not as bad as duplicating 
info. So, done away with |device_in_use|.

+      found->capability_in_use_ = params_as_capability;

[ncarter (slow), 2013/11/07 22:43:01]

Would it be better to just cache the Capability in the Controller?

If we track it in the DeviceInfo, then we wind up with three different structs
that are tracking the current active state of the device: the DeviceEntry, the
Controller, and now the DeviceInfo.

[mcasas, 2013/11/08 02:45:21]

Struct DeviceEntry and struct DeviceInfo actually cache different things, 
the first one being a dynamic id-controller*-device* used during the 
device's runtime, and DeviceInfo which is mostly static, containing names
and capabilities. Moreover they are created on different moments, the
DeviceInfo on early times and DeviceEntry during action (capture) time.
The only touching point is the case of the capability in use. Do you think 
is ok to keep |capability_in_use_| and just remove |device_in_use_|? (L.235)

+    }
     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(

[ncarter (slow), 2013/11/07 22:43:01]

How does the client of this API know when to call this function? It seems to
become valid only after the device enumeration event occurs. Is that the
intended usage -- you request enumeration, and when the enumeration completes,
you call back to get the extra info here?

[mcasas, 2013/11/08 02:45:21]

Yes exactly. There's a comment in the header file:
// Retrieve the available capture capabilities for a particular device. The
// capabilities are cached during device(s) enumeration and updated as clients
// open/close the device(s).

Next CL(s) will call wire the logic to call this method whenever
RTCVideoCapturer::GetSupportedFormats() is used during capabilities
negotiation in renderer side.

+    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,71 @@ 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_) {

[ncarter (slow), 2013/11/07 22:43:01]

Why do we check the listener_, if we're not invoking the listener?

[mcasas, 2013/11/08 02:45:21]

It's used in OnDeviceNameAndCapabilitiesEnumerated()
but if there's no listener there's no sense in continuing with the
logic at this point. Plus there are even unittests that hit this
transient: |listener_| gets removed between GetAvailableDevicesOnDeviceThread
and this method !

     // Listener has been removed.
     return;
   }
 
-  // Transform from VCD::Name to StreamDeviceInfo.
-  StreamDeviceInfoArray devices;
+  // Eliminate from |devices_info_cache_| all devices not in the snapshot.
+  ScopedVector<DeviceInfo>::iterator it_device_info =
+      devices_info_cache_.begin();
+  while(it_device_info != devices_info_cache_.end()) {

[ncarter (slow), 2013/11/07 22:43:01]

space after while

[mcasas, 2013/11/08 02:45:21]

Done.

+    media::VideoCaptureDevice::Names::const_iterator it;
+    for (it = names_snapshot.begin(); it != names_snapshot.end(); ++it) {
+      if ((*it_device_info)->name_.id() == it->id())
+        break;
+    }
+    if (it == names_snapshot.end())
+      devices_info_cache_.erase(it_device_info++);
+    else
+      ++it_device_info;
+  }
+
+  // Truly new devices are put in |new_device_names|, and we need to retrieve
+  // their capabilities.
+  media::VideoCaptureDevice::Names new_device_names;
   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()));
+           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));
+}
+
+void VideoCaptureManager::OnDeviceNameAndCapabilitiesEnumerated(
+    MediaStreamType stream_type,
+    ScopedVector<DeviceInfo> new_devices_info) {
+  DVLOG(1) << "OnDeviceNameAndCapabilitiesEnumerated, #new devices: "
+           << new_devices_info.size();
+
+  ScopedVector<DeviceInfo>::iterator it = new_devices_info.begin();
+  while (it != new_devices_info.end()) {
+    devices_info_cache_.push_back(*it);
+    it = new_devices_info.weak_erase(it);
   }
 
-  listener_->DevicesEnumerated(stream_type, devices);
+  // Walk the |devices_info_cache_|| and transform from VCD::Name to

[ncarter (slow), 2013/11/07 22:43:01]

|| -> |

[mcasas, 2013/11/08 02:45:21]

Done.

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

[perkj_chrome, 2013/11/07 10:49:11]

nit: indentation

[mcasas, 2013/11/07 17:14:30]

Ok, clang-format for Chromium style, and we leave it to it.

+    devices.push_back(StreamDeviceInfo(
+        stream_type, (*it)->name_.GetNameAndModel(), (*it)->name_.id()));
+  }
+  // |listener_| might have disappeared while we were scanning the devices.
+  if(listener_)

[ncarter (slow), 2013/11/07 22:43:01]

space after the if

[mcasas, 2013/11/08 02:45:21]

Done.

+    listener_->DevicesEnumerated(stream_type, devices);
 }
 
 bool VideoCaptureManager::IsOnDeviceThread() const {
@@ -312,23 +399,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 +417,31 @@ 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;
+  for (media::VideoCaptureDevice::Names::const_iterator it =
+          device_names.begin(); it != device_names.end(); ++it) {
+    media::VideoCaptureCapabilities capabilities;
+    DeviceInfo* device_info = new DeviceInfo((*it),
+        media::VideoCaptureCapabilities());
+    if (!use_fake_device_) {
+      media::VideoCaptureDevice::GetDeviceSupportedFormats(*it,
+          &(device_info->capabilities_));
+    } else {
+      media::FakeVideoCaptureDevice::GetDeviceSupportedFormats(*it,
+          &(device_info->capabilities_));
+    }
+    device_info_vector.push_back(device_info);
+  }
+  return device_info_vector.Pass();
 }
 
 VideoCaptureManager::DeviceEntry*
@@ -386,8 +488,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 +526,14 @@ VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
   return new_device;
 }
 
+VideoCaptureManager::DeviceInfo* VideoCaptureManager::FindDeviceInfoById(
+    const std::string& id) {
+  for (ScopedVector<DeviceInfo>::iterator it = devices_info_cache_.begin();
+       it != devices_info_cache_.end(); ++it) {
+    if ((*it)->name_.id() == id)
+      return *it;
+  }
+  return NULL;
+}
+
 }  // namespace content