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

content/browser/renderer_host/media/video_capture_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/media/video_capture_manager.h"
 
 #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"
 #include "base/task_runner_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "content/browser/media/capture/web_contents_video_capture_device.h"
 #include "content/browser/renderer_host/media/video_capture_controller.h"
 #include "content/browser/renderer_host/media/video_capture_controller_event_handler.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/desktop_media_id.h"
@@ skipping 95 matching lines @@
 
 void VideoCaptureManager::Unregister() {
   DCHECK(listener_);
   listener_ = NULL;
 }
 
 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) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(listener_);
 
   // Generate a new id for the session being opened.
   const media::VideoCaptureSessionId capture_session_id =
       new_capture_session_id_++;
 
@@ skipping 52 matching lines @@
 
   scoped_ptr<media::VideoCaptureDevice> video_capture_device;
   switch (entry->stream_type) {
     case MEDIA_DEVICE_VIDEO_CAPTURE: {
       // 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.
       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;
     }
     case MEDIA_TAB_VIDEO_CAPTURE: {
       video_capture_device.reset(
           WebContentsVideoCaptureDevice::Create(entry->id));
       break;
     }
     case MEDIA_DESKTOP_VIDEO_CAPTURE: {
 #if defined(ENABLE_SCREEN_CAPTURE)
@@ skipping 211 matching lines @@
     // Listener has been removed.
     return;
   }
   listener_->Closed(stream_type, capture_session_id);
 }
 
 void VideoCaptureManager::OnDevicesInfoEnumerated(
     MediaStreamType stream_type,
     const DeviceInfos& new_devices_info_cache) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
   if (!listener_) {
     // Listener has been removed.
     return;
   }
   devices_info_cache_ = new_devices_info_cache;
 
   // Walk the |devices_info_cache_| and transform from VCD::Name to
   // StreamDeviceInfo for return purposes.
   StreamDeviceInfoArray devices;
   for (DeviceInfos::const_iterator it = devices_info_cache_.begin();
        it != devices_info_cache_.end(); ++it) {
     devices.push_back(StreamDeviceInfo(
         stream_type, it->name.GetNameAndModel(), it->name.id()));
   }
   listener_->DevicesEnumerated(stream_type, devices);
 }
 
 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.
   DeviceInfos new_devices_info_cache;
   for (DeviceInfos::const_iterator it_device_info =
            old_device_info_cache.begin();
        it_device_info != old_device_info_cache.end(); ++it_device_info) {
      for (media::VideoCaptureDevice::Names::iterator it =
               names_snapshot.begin();
           it != names_snapshot.end(); ++it) {
       if (it_device_info->name.id() == it->id()) {
         new_devices_info_cache.push_back(*it_device_info);
         names_snapshot.erase(it);
         break;
       }
     }
   }
 
   // Get the supported capture formats for the new devices in |names_snapshot|.
   for (media::VideoCaptureDevice::Names::const_iterator it =
            names_snapshot.begin();
        it != names_snapshot.end(); ++it) {
     media::VideoCaptureFormats supported_formats;
     DeviceInfo device_info(*it, media::VideoCaptureFormats());
     video_capture_device_factory_->GetDeviceSupportedFormats(
         *it, &(device_info.supported_formats));
     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*
 VideoCaptureManager::GetDeviceEntryForMediaStreamDevice(
     const MediaStreamDevice& device_info) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   for (DeviceEntries::iterator it = devices_.begin();
        it != devices_.end(); ++it) {
     DeviceEntry* device = *it;
@@ skipping 92 matching lines @@
 void VideoCaptureManager::SaveDesktopCaptureWindowIdOnDeviceThread(
     media::VideoCaptureSessionId session_id,
     gfx::NativeViewId window_id) {
   DCHECK(IsOnDeviceThread());
   DCHECK(notification_window_ids_.find(session_id) ==
          notification_window_ids_.end());
   notification_window_ids_[session_id] = window_id;
 }
 
 }  // namespace content