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..f97582c9f6142344140c0b4dd6c38d8bd30908cc 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
+#include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/browser/renderer_host/media/video_capture_controller.h"
 #include "content/browser/renderer_host/media/video_capture_controller_event_handler.h"
@@ -39,6 +40,17 @@ 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),
+      name_(name),
+      capabilities_(capabilities) {}
+
+VideoCaptureManager::DeviceInfo::~DeviceInfo() {}
+
 VideoCaptureManager::VideoCaptureManager()
     : listener_(NULL),
       new_capture_session_id_(1),
@@ -67,11 +79,18 @@ void VideoCaptureManager::EnumerateDevices(MediaStreamType stream_type) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DVLOG(1) << "VideoCaptureManager::EnumerateDevices, type " << stream_type;
   DCHECK(listener_);
+
   base::PostTaskAndReplyWithResult(
-      device_loop_, FROM_HERE,
-      base::Bind(&VideoCaptureManager::GetAvailableDevicesOnDeviceThread, this,
-                 stream_type),
-      base::Bind(&VideoCaptureManager::OnDevicesEnumerated, this, stream_type));
+      device_loop_,
+      FROM_HERE,
+      base::Bind(&VideoCaptureManager::
+                      GetAvailableDevicesAndCapabilitiesOnDeviceThread,
+                 this,
+                 stream_type,
+                 devices_info_cache_),
+      base::Bind(&VideoCaptureManager::OnDeviceNamesAndCapabilitiesEnumerated,
+                 this,
+                 stream_type));
 }
 
 int VideoCaptureManager::Open(const StreamDeviceInfo& device_info) {
@@ -144,12 +163,11 @@ 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);
+      DeviceInfo* found = FindDeviceInfoById(entry->id, devices_info_cache_);
       if (found) {
-        video_capture_device.reset(use_fake_device_ ?
-            media::FakeVideoCaptureDevice::Create(*found) :
-            media::VideoCaptureDevice::Create(*found));
+        video_capture_device.reset(use_fake_device_
+            ? media::FakeVideoCaptureDevice::Create(found->name_)

[no longer working on chromium, 2013/11/12 16:48:00]

nit, why ? changed here

[mcasas, 2013/11/12 18:10:16]

clang-format would make something entirely different:

if (found) {
    video_capture_device.reset(
        use_fake_device_
            ? media::FakeVideoCaptureDevice::Create(found->name)
            : media::VideoCaptureDevice::Create(found->name));

And I must have reverted this a couple of times, hence the 
shift. Changed back so is as similar as possible to before.

+            : media::VideoCaptureDevice::Create(found->name_));
       }
       break;
     }
@@ -216,9 +234,14 @@ void VideoCaptureManager::StartCaptureForClient(
     params_as_capability.frame_size_type =
         params.requested_format.frame_size_type;
 
+    DeviceInfo* found = FindDeviceInfoById(entry->id, devices_info_cache_);

[no longer working on chromium, 2013/11/12 16:48:00]

nit, remove the local variable, just use if (FindDeviceInfoById(entry->id,
devices_info_cache_))

[mcasas, 2013/11/12 18:10:16]

? I used the |found| twice, one to check != NULL and
again to set the |device_in_use|.

+    if (found)
+      found->device_in_use_ = true;
+
     device_loop_->PostTask(FROM_HERE, base::Bind(
         &VideoCaptureManager::DoStartDeviceOnDeviceThread, this,
-        entry, params_as_capability,
+        entry,
+        params_as_capability,
         base::Passed(entry->video_capture_controller->NewDeviceClient())));
   }
   // Run the callback first, as AddClient() may trigger OnFrameInfo().
@@ -252,6 +275,37 @@ 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, devices_info_cache_);
+  if (device) {

[no longer working on chromium, 2013/11/12 16:48:00]

early return
if (!device) {
  // Add comment.
  *capabilities = device->capabilities_;
  return;
}

DeviceEntry* const existing_device = ...
  

[mcasas, 2013/11/12 18:10:16]

Done.

+    DeviceEntry* const existing_device =
+        GetDeviceEntryForMediaStreamDevice(it->second);
+    if (existing_device && device->device_in_use_) {
+      media::VideoCaptureParams params =
+          existing_device->video_capture_controller->GetVideoCaptureParams();
+      media::VideoCaptureCapability current_format(
+          params.requested_format.width,
+          params.requested_format.height,
+          params.requested_format.frame_rate,
+          media::PIXEL_FORMAT_UNKNOWN,
+          params.requested_format.frame_size_type);
+      capabilities->push_back(current_format);
+    } else {
+      *capabilities = device->capabilities_;
+    }
+  }
+}
+
 void VideoCaptureManager::DoStopDeviceOnDeviceThread(DeviceEntry* entry) {
   SCOPED_UMA_HISTOGRAM_TIMER("Media.VideoCaptureManager.StopDeviceTime");
   DCHECK(IsOnDeviceThread());
@@ -281,65 +335,92 @@ void VideoCaptureManager::OnClosed(MediaStreamType stream_type,
   listener_->Closed(stream_type, capture_session_id);
 }
 
-void VideoCaptureManager::OnDevicesEnumerated(
+void VideoCaptureManager::OnDeviceNamesAndCapabilitiesEnumerated(
     MediaStreamType stream_type,
-    const media::VideoCaptureDevice::Names& device_names) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    const DevicesInfo& new_devices_info_cache) {
+  DVLOG(1) << "OnDeviceNameAndCapabilitiesEnumerated, #new devices: "
+           << new_devices_info_cache.size();
 
-  if (!listener_) {
-    // Listener has been removed.
-    return;

[no longer working on chromium, 2013/11/12 16:48:00]

why do you change these, please added back the thread check and early return if
the listen has gone.

[mcasas, 2013/11/12 18:10:16]

Done.

-  }
+  devices_info_cache_ = new_devices_info_cache;
 
-  // Transform from VCD::Name to StreamDeviceInfo.
+  // Walk the |devices_info_cache_| and transform from VCD::Name to
+  // StreamDeviceInfo for return purposes.
   StreamDeviceInfoArray devices;
-  for (media::VideoCaptureDevice::Names::const_iterator it =
-           device_names.begin(); it != device_names.end(); ++it) {
+  for (DevicesInfo::const_iterator 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.

[no longer working on chromium, 2013/11/12 16:48:00]

I think you should do nothing if the listener_ has gone away.

[mcasas, 2013/11/12 18:10:16]

Done.

+  if (listener_)
+    listener_->DevicesEnumerated(stream_type, devices);
 }
 
 bool VideoCaptureManager::IsOnDeviceThread() const {
   return device_loop_->BelongsToCurrentThread();
 }
 
-media::VideoCaptureDevice::Names
-VideoCaptureManager::GetAvailableDevicesOnDeviceThread(
-    MediaStreamType stream_type) {
+VideoCaptureManager::DevicesInfo
+VideoCaptureManager::GetAvailableDevicesAndCapabilitiesOnDeviceThread(
+    MediaStreamType stream_type,
+    const DevicesInfo& old_device_info_cache) {
   SCOPED_UMA_HISTOGRAM_TIMER(
-      "Media.VideoCaptureManager.GetAvailableDevicesTime");
+      "Media.VideoCaptureManager."
+      "GetAvailableDevicesAndCapabilitiesOnDeviceThreadTime");
   DCHECK(IsOnDeviceThread());
-  media::VideoCaptureDevice::Names result;
-
+  media::VideoCaptureDevice::Names names_snapshot;
   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_snapshot);
+      else
+        media::FakeVideoCaptureDevice::GetDeviceNames(&names_snapshot);
       break;
-
     case MEDIA_DESKTOP_VIDEO_CAPTURE:
       // Do nothing.
       break;
-
     default:
       NOTREACHED();
       break;
   }
-  return result;
+
+  // Construct |new_devices_info_cache| with the cached devices that are still
+  // present in the system, and remove their names from |names_snapshot|, so we
+  // keep there the truly new devices.
+  DevicesInfo new_devices_info_cache;
+  for (DevicesInfo::const_iterator it_device_info =
+           old_device_info_cache.begin();
+       it_device_info != old_device_info_cache.end();
+       ++it_device_info) {
+    media::VideoCaptureDevice::Names::iterator it;
+    for (it = names_snapshot.begin(); it != names_snapshot.end(); ++it) {
+      if (it_device_info->name_.id() == it->id())
+        break;

[no longer working on chromium, 2013/11/12 16:48:00]

I think you can remove the if two lines below if you do
if (it_device_info->name_.id() == it->id()) {
  new_devices_info_cache.push_back(*it_device_info);
  names_snapshot.erase(it);
  break;
}

[mcasas, 2013/11/12 18:10:16]

Good catch! I think so too, so Done.

+    }
+    if (it != names_snapshot.end()) {
+      new_devices_info_cache.push_back(*it_device_info);
+      names_snapshot.erase(it);
+    }
+  }
+
+  // Need to get the capabilities for the truly new devices in |names_snapshot|.
+  for (media::VideoCaptureDevice::Names::const_iterator it =
+           names_snapshot.begin();
+       it != names_snapshot.end();
+       ++it) {
+    media::VideoCaptureCapabilities capabilities;
+    DeviceInfo device_info(*it, media::VideoCaptureCapabilities());
+    if (!use_fake_device_) {
+      media::VideoCaptureDevice::GetDeviceSupportedFormats(
+          *it, &(device_info.capabilities_));
+    } else {
+      media::FakeVideoCaptureDevice::GetDeviceSupportedFormats(
+          *it, &(device_info.capabilities_));
+    }
+    new_devices_info_cache.push_back(device_info);
+  }
+  return new_devices_info_cache;
 }
 
 VideoCaptureManager::DeviceEntry*
@@ -386,8 +467,13 @@ 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, devices_info_cache_);
+    if (device_info)
+      device_info->device_in_use_ = false;
   }
 }
 
@@ -420,4 +506,15 @@ VideoCaptureManager::DeviceEntry* VideoCaptureManager::GetOrCreateDeviceEntry(
   return new_device;
 }
 
+VideoCaptureManager::DeviceInfo* VideoCaptureManager::FindDeviceInfoById(
+    const std::string& id,
+    DevicesInfo& device_vector) {
+  for (DevicesInfo::iterator it = device_vector.begin();
+       it != device_vector.end(); ++it) {
+    if (it->name_.id() == id)
+      return it.base();

[no longer working on chromium, 2013/11/12 16:48:00]

FYI, chrome rarely uses .base(), most of the case it just uses &(*it)
I will let you decide which one to use.

[mcasas, 2013/11/12 18:10:16]

Chromiumisms are my thing!

+  }
+  return NULL;
+}
+
 }  // namespace content