VideoCaptureDeviceFactory: Change device enumeration to callback + QTKit enumerates in UI thread.

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 6c90c91cae3343e18a7e1ec1bca1e433b953dd56..03b039d426c58d84dbdc44d9ede20995720e8a25 100644
--- a/content/browser/renderer_host/media/video_capture_manager.cc
+++ b/content/browser/renderer_host/media/video_capture_manager.cc
@@ -7,6 +7,7 @@
 #include <set>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
@@ -122,12 +123,29 @@ void VideoCaptureManager::EnumerateDevices(MediaStreamType stream_type) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DVLOG(1) << "VideoCaptureManager::EnumerateDevices, type " << stream_type;
   DCHECK(listener_);
-  base::PostTaskAndReplyWithResult(
-      device_task_runner_, FROM_HERE,
-      base::Bind(&VideoCaptureManager::GetAvailableDevicesInfoOnDeviceThread,
-                 this, stream_type, devices_info_cache_),
-      base::Bind(&VideoCaptureManager::OnDevicesInfoEnumerated, this,
-                 stream_type));
+  if (stream_type == MEDIA_DESKTOP_VIDEO_CAPTURE) {

[perkj_chrome, 2014/05/26 11:49:42]

THis looks weird. Why return  devices_info_cache_ ? Isn-t that only for 
MEDIA_DEVICE_VIDEO_CAPTURE ? And is this ever called with
MEDIA_DESKTOP_VIDEO_CAPTURE ? If not - just dcheck the type.

[mcasas, 2014/05/26 13:00:18]

This was a cleanup of previous code, where 
GetAvailableDevicesInfoOnDeviceThread() was called and
nothing was updated for this MEDIA_DESKTOP_VIDEO_CAPTURE...
(IOW I just made it obvious).

I'll leave only the l.130 DCHECK.

+    OnDevicesInfoEnumerated(stream_type, devices_info_cache_);
+    return;
+  }
+  DCHECK_EQ(stream_type, MEDIA_DEVICE_VIDEO_CAPTURE);
+
+  // TODO(mcasas): replace previous SCOPED_UMA_HISTOGRAM_TIMER(
+  // "Media.VideoCaptureManager.GetAvailableDevicesInfoOnDeviceThreadTime");

[perkj_chrome, 2014/05/26 11:49:42]

this does not seem to have anything to do with what you are doing.

[mcasas, 2014/05/26 13:00:18]

Yes, this is an open question.

Before this CL, there was an UMA collection in 
GetAvailableDevicesInfoOnDeviceThread(), which was measuring
the time spent in synchronous GetDeviceNames().

If we think is interesting to collect that UMA still,
this TODO is the way to do it. Otherwise, I'll remove
the TODO, WDYT?

[perkj_chrome, 2014/05/27 12:15:08]

Was that something you added because you wanted it or was it a request from
someone else? If someone else rely on it you must fix it in this cl. Your call. 

+  // with a base/timer/elapsed_timer.h ElapsedTime, constructed here and
+  // measured in OnDevicesInfoEnumerated()
+
+  base::Callback<void(media::VideoCaptureDevice::Names&)>
+      devices_enumerated_callback =
+          base::Bind(&VideoCaptureManager::ConsolidateDevicesInfoOnDeviceThread,

[perkj_chrome, 2014/05/26 11:49:42]

To avoid the raw message loop pointer - how about a nested callback that you
bind to the current loop?

       media::BindToCurrentLoop( base::bind(
&VideoCaptureManager::OnDevicesInfoEnumerated, this, stream_type));

ConsolidateDevicesInfoOnDeviceThread then dont need to Post a task back again -
it just have to run the callback. 

[mcasas, 2014/05/26 13:00:18]

Done.

+                     this,
+                     base::MessageLoop::current(),
+                     stream_type,
+                     devices_info_cache_);
+  // OK to use base::Unretained() since we own the VCDFactory.

[perkj_chrome, 2014/05/26 11:49:42]

What guarantee that the video capture manager is not destroyed? the bind to the
callback above?

[mcasas, 2014/05/26 13:00:18]

VCM outlives the VCDF, plus is reference counted via callback above.

+  device_task_runner_->PostTask(FROM_HERE,
+      base::Bind(&media::VideoCaptureDeviceFactory::EnumerateDeviceNames,
+                 base::Unretained(video_capture_device_factory_.get()),
+                 devices_enumerated_callback));
 }
 
 int VideoCaptureManager::Open(const StreamDeviceInfo& device_info) {
@@ -200,9 +218,7 @@ void VideoCaptureManager::DoStartDeviceOnDeviceThread(
       DeviceInfo* found = FindDeviceInfoById(entry->id, devices_info_cache_);
       if (found) {
         video_capture_device =
-            video_capture_device_factory_->Create(
-                BrowserThread::GetMessageLoopProxyForThread(BrowserThread::UI),
-                found->name);
+            video_capture_device_factory_->Create(found->name);
       }
       break;
     }
@@ -434,7 +450,6 @@ void VideoCaptureManager::OnDevicesInfoEnumerated(
     MediaStreamType stream_type,
     const DeviceInfos& new_devices_info_cache) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
   if (!listener_) {
     // Listener has been removed.
     return;
@@ -456,29 +471,12 @@ bool VideoCaptureManager::IsOnDeviceThread() const {
   return device_task_runner_->BelongsToCurrentThread();
 }
 
-VideoCaptureManager::DeviceInfos
-VideoCaptureManager::GetAvailableDevicesInfoOnDeviceThread(
+void VideoCaptureManager::ConsolidateDevicesInfoOnDeviceThread(
+    base::MessageLoop* io_loop,
     MediaStreamType stream_type,
-    const DeviceInfos& old_device_info_cache) {
-  SCOPED_UMA_HISTOGRAM_TIMER(
-      "Media.VideoCaptureManager.GetAvailableDevicesInfoOnDeviceThreadTime");
+    const DeviceInfos& old_device_info_cache,
+    media::VideoCaptureDevice::Names& names_snapshot) {
   DCHECK(IsOnDeviceThread());
-  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.
-      video_capture_device_factory_->GetDeviceNames(&names_snapshot);
-      break;
-    case MEDIA_DESKTOP_VIDEO_CAPTURE:
-      // Do nothing.
-      break;
-    default:
-      NOTREACHED();
-      break;
-  }
-
   // 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.
@@ -508,7 +506,10 @@ VideoCaptureManager::GetAvailableDevicesInfoOnDeviceThread(
     ConsolidateCaptureFormats(&device_info.supported_formats);
     new_devices_info_cache.push_back(device_info);
   }
-  return new_devices_info_cache;
+
+  io_loop->PostTask(FROM_HERE,

[perkj_chrome, 2014/05/26 11:49:42]

Use a callback as input parameter instead and just run it here?

[mcasas, 2014/05/26 13:00:18]

Done.

+        base::Bind(&VideoCaptureManager::OnDevicesInfoEnumerated,
+                   this, stream_type, new_devices_info_cache));
 }
 
 VideoCaptureManager::DeviceEntry*