refactor EnumerateDevices to make it a persistent request.

content/browser/renderer_host/media/media_stream_manager.cc

Index: content/browser/renderer_host/media/media_stream_manager.cc
===================================================================
--- content/browser/renderer_host/media/media_stream_manager.cc	(revision 148913)
+++ content/browser/renderer_host/media/media_stream_manager.cc	(working copy)
@@ -11,6 +11,8 @@
 #include "base/logging.h"
 #include "base/rand_util.h"
 #include "base/win/scoped_com_initializer.h"
+#include "content/browser/browser_main_loop.h"
+#include "content/browser/devices_monitor/devices_monitor.h"
 #include "content/browser/renderer_host/media/audio_input_device_manager.h"
 #include "content/browser/renderer_host/media/media_stream_device_settings.h"
 #include "content/browser/renderer_host/media/media_stream_requester.h"
@@ -131,6 +133,10 @@
   StreamDeviceInfoArray video_devices;
 };
 
+MediaStreamManager::EnumerationCache::EnumerationCache()
+    : valid(false) {
+}
+
 MediaStreamManager::MediaStreamManager(
     AudioInputDeviceManager* audio_input_device_manager,
     VideoCaptureManager* video_capture_manager)
@@ -138,6 +144,7 @@
           device_settings_(new MediaStreamDeviceSettings(this))),
       audio_input_device_manager_(audio_input_device_manager),
       video_capture_manager_(video_capture_manager),
+      monitoring_started_(false),
       enumeration_in_progress_(content::NUM_MEDIA_STREAM_DEVICE_TYPES, false),

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

This array seems to be used in conjunction with the cache.  How about adding a
bool to the struct, enumeration_in_progress_ and reduce the bookkeeping?

[wjia(left Chromium), 2012/08/01 01:28:59]

This |enumeration_in_progress_| also include other type of requests, such as
GenerationStream, while cache is used for EnumerateDevices only.

       io_loop_(NULL) {
 }
@@ -176,6 +183,17 @@
                             render_view_id,
                             security_origin);
   StartEnumeration(&new_request, label);
+
+  // Get user confirmation to use capture devices.
+  // Need to make an asynchronous call to make sure the |requester| gets the
+  // |label| before it would receive any event.
+  BrowserThread::PostTask(BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&MediaStreamDeviceSettings::RequestCaptureDeviceUsage,
+                 base::Unretained(device_settings_.get()),
+                 *label, render_process_id,
+                 render_view_id, options,
+                 security_origin));
 }
 
 void MediaStreamManager::CancelRequests(MediaStreamRequester* requester) {
@@ -246,6 +264,10 @@
   // Find the request and close all open devices for the request.
   DeviceRequests::iterator it = requests_.find(label);
   if (it != requests_.end()) {
+    if (it->second.type == DeviceRequest::kEnumerateDevices) {

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

kEnumerateDevices -> ENUMERATE_DEVICES

[wjia(left Chromium), 2012/08/01 01:28:59]

Done.

+      StopEnumerateDevices(label);
+      return;
+    }
     for (StreamDeviceInfoArray::iterator audio_it =
          it->second.audio_devices.begin();
          audio_it != it->second.audio_devices.end(); ++audio_it) {
@@ -260,7 +282,6 @@
       NotifyObserverDevicesClosed(&(it->second));
     }
     requests_.erase(it);
-    return;
   }
 }
 
@@ -272,13 +293,19 @@
     const GURL& security_origin,
     std::string* label) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE ||
+         type == content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE);
 
   // Create a new request.
   StreamOptions options;
-  if (type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE)
+  EnumerationCache* cache = NULL;
+  if (type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE) {
     options.audio = true;
-  else
+    cache = &audio_enumeration_cache_;
+  } else {
     options.video = true;
+    cache = &video_enumeration_cache_;
+  }
 
   DeviceRequest new_request(requester, options,
                             render_process_id,
@@ -286,9 +313,35 @@
                             security_origin);
   new_request.type = DeviceRequest::kEnumerateDevices;
 
-  StartEnumeration(&new_request, label);
+  if (cache->valid) {
+    // Cached device list of this type exists. Just send it out.
+    new_request.state[type] = DeviceRequest::kRequested;

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

kRequested -> REQUESTED

[wjia(left Chromium), 2012/08/01 01:28:59]

Done.

+    AddRequest(&new_request, label);
+    // Need to post a task since the requester won't have label till
+    // this function returns.
+    BrowserThread::PostTask(BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&MediaStreamManager::SendCachedDeviceList,
+                   base::Unretained(this), cache, *label));
+  } else {
+    StartEnumeration(&new_request, label);
+    StartMonitoring();
+  }
 }
 
+void MediaStreamManager::StopEnumerateDevices(const std::string& label) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  DeviceRequests::iterator it = requests_.find(label);
+  if (it != requests_.end()) {
+    DCHECK_EQ(it->second.type, DeviceRequest::kEnumerateDevices);
+    requests_.erase(it);
+    if (!HasEnumerationRequest()) {
+      StopMonitoring();
+    }
+  }
+}
+
 void MediaStreamManager::OpenDevice(
     MediaStreamRequester* requester,
     int render_process_id,
@@ -316,6 +369,42 @@
   StartEnumeration(&new_request, label);
 }
 
+void MediaStreamManager::SendCachedDeviceList(
+    EnumerationCache* cache,
+    const std::string& label) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (cache->valid && requests_.find(label) != requests_.end()) {
+    requests_[label].requester->DevicesEnumerated(label, cache->devices);

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

nit: you've already looked up the label so you could use an iterator instead of
looking it up again via the [] operator.

[wjia(left Chromium), 2012/08/01 01:28:59]

Done.

+  }
+}
+
+void MediaStreamManager::StartMonitoring() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!monitoring_started_) {
+    monitoring_started_ = true;
+    content::BrowserMainLoop::GetDevicesMonitor()->Start(this);
+    base::SystemMonitor::Get()->AddDevicesChangedObserver(this);

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

What do you think about this:

* Remove:
  - StartMonitoring
  - StopMonitoring
  - monitoring_started_ 
  - the call to BrowserMainLoop::GetDevicesMonitor()->Start(this).
  I don't think we need a dependency on BrowserLoop here since
  all we need is provided by the SystemMonitor.

* Move the creation of the MediaStreamManager from BML::MainMessageLoopStart to
BML::BrowserThreadsStarted.

* In the MediaStreamManager ctor invoke AddDevicesChangedObserver:

BrowserThread::PostTask(BrowserThread::IO,
    FROM_HERE,
    base::Bind(&base::SystemMonitor::AddDevicesChangedObserver,
        base::SystemMonitor::Get(), this));


I think this will simplify things quite a bit without reduced functionality.

[wjia(left Chromium), 2012/08/01 01:28:59]

The start/stop of DeviceMonitor has been removed because DeviceMonitor has been
started ever since browser is started.

It makes sense for MSM to listen to SystemMonitor only when there is such
request. 

[tommi (sloooow) - chröme, 2012/08/01 10:21:37]

Great.  Thanks.

+  }
+}
+
+void MediaStreamManager::StopMonitoring() {
+  DCHECK_EQ(MessageLoop::current(), io_loop_);
+  if (monitoring_started_ && !HasEnumerationRequest()) {
+    content::BrowserMainLoop::GetDevicesMonitor()->Stop(this);
+    base::SystemMonitor::Get()->RemoveDevicesChangedObserver(this);
+    monitoring_started_ = false;
+    ClearEnumerationCache(&audio_enumeration_cache_);
+    ClearEnumerationCache(&video_enumeration_cache_);
+  }
+}
+
+void MediaStreamManager::ClearEnumerationCache(EnumerationCache* cache) {

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

could an empty cache mean valid == false?
if so, we could just use the array directly instead of a struct and clear() it
here.

[wjia(left Chromium), 2012/08/01 01:28:59]

Unfortunately, empty cache doesn't mean valid or invalid since it's possible
enumeration returns an empty list of device when there is no such devices
attached to the computer.

[tommi (sloooow) - chröme, 2012/08/01 10:21:37]

sgtm.

+  DCHECK_EQ(MessageLoop::current(), io_loop_);
+  if (cache->valid) {

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

no need to check it first.  just set valid to false regardless.

[wjia(left Chromium), 2012/08/01 01:28:59]

Done.

+    cache->valid = false;
+  }
+}
+
 void MediaStreamManager::StartEnumeration(
     DeviceRequest* new_request,
     std::string* label) {
@@ -338,6 +427,14 @@
     }
   }
 
+  AddRequest(new_request, label);
+}
+
+void MediaStreamManager::AddRequest(
+    DeviceRequest* new_request,
+    std::string* label) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
   // Create a label for this request and verify it is unique.
   std::string request_label;
   do {
@@ -346,19 +443,6 @@
 
   requests_.insert(std::make_pair(request_label, *new_request));
 
-  // Get user confirmation to use capture devices.
-  // Need to make an asynchronous call to make sure the |requester| gets the
-  // |label| before it would receive any event.
-  if (new_request->type == DeviceRequest::kGenerateStream) {
-    BrowserThread::PostTask(BrowserThread::IO,
-        FROM_HERE,
-        base::Bind(&MediaStreamDeviceSettings::RequestCaptureDeviceUsage,
-                   base::Unretained(device_settings_.get()),
-                   request_label, new_request->render_process_id,
-                   new_request->render_view_id, new_request->options,
-                   new_request->security_origin));
-  }
-
   (*label) = request_label;
 }
 
@@ -455,6 +539,19 @@
     MediaStreamType stream_type, const StreamDeviceInfoArray& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
+  // Only cache device list when there is EnumerateDevices request, since
+  // other requests don't turn on device monitoring.
+  bool need_update_clients = false;
+  EnumerationCache* cache =
+      (stream_type == content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE ?
+       &audio_enumeration_cache_ : &video_enumeration_cache_);
+  if (HasEnumerationRequest(stream_type)) {
+    if (!cache->valid || cache->devices != devices) {

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

I think this is a case for not needing the |valid| variable.  Just clear the
array if we want to refresh the cache.

[wjia(left Chromium), 2012/08/01 01:28:59]

Without |valid| flag, we have to keep cache always up-to-date. That means we
have to enumerate devices whenever their is a DevicesChanged event. That's what
we don't want to do since enumeration is known to be expensive and we try to
avoid it.

[tommi (sloooow) - chröme, 2012/08/01 10:21:37]

sgtm

+      cache->devices = devices;
+      need_update_clients = true;
+    }
+  }
+
   // Publish the result for all requests waiting for device list(s).
   // Find the requests waiting for this device list, store their labels and
   // release the iterator before calling device settings. We might get a call
@@ -464,7 +561,8 @@
        ++it) {
     if (it->second.state[stream_type] == DeviceRequest::kRequested &&
         Requested(it->second.options, stream_type)) {
-      it->second.state[stream_type] = DeviceRequest::kPendingApproval;
+      if (it->second.type != DeviceRequest::kEnumerateDevices)
+        it->second.state[stream_type] = DeviceRequest::kPendingApproval;
       label_list.push_back(it->first);
     }
   }
@@ -473,8 +571,8 @@
     DeviceRequest& request = requests_[*it];
     switch (request.type) {
       case DeviceRequest::kEnumerateDevices:
-        request.requester->DevicesEnumerated(*it, devices);
-        requests_.erase(*it);
+        if (need_update_clients)
+          request.requester->DevicesEnumerated(*it, devices);
         break;
       case DeviceRequest::kOpenDevice:
         for (StreamDeviceInfoArray::const_iterator device_it = devices.begin();
@@ -628,7 +726,10 @@
 
 void MediaStreamManager::WillDestroyCurrentMessageLoop() {
   DCHECK_EQ(MessageLoop::current(), io_loop_);
+  DCHECK(requests_.empty());
   if (device_thread_.get()) {
+    StopMonitoring();
+
     video_capture_manager_->Unregister();
     audio_input_device_manager_->Unregister();
     device_thread_.reset();
@@ -721,4 +822,55 @@
   return NULL;
 }
 
+void MediaStreamManager::OnDevicesChanged(
+    base::SystemMonitor::DeviceType device_type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  MediaStreamType stream_type;
+  EnumerationCache* cache;
+  if (device_type == base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE) {
+    stream_type = content::MEDIA_STREAM_DEVICE_TYPE_AUDIO_CAPTURE;
+    cache = &audio_enumeration_cache_;
+  } else if (device_type == base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE) {
+    stream_type = content::MEDIA_STREAM_DEVICE_TYPE_VIDEO_CAPTURE;
+    cache = &video_enumeration_cache_;
+  } else {
+    return;  // Uninterested device change.

[tommi (sloooow) - chröme, 2012/07/30 12:10:21]

s/Uninterested/Uninteresting.

[wjia(left Chromium), 2012/08/01 01:28:59]

Done.

+  }
+
+  if (!HasEnumerationRequest(stream_type)) {
+    // There is no request for that type, No need to enumerate devices.
+    // Therefore, invalidate the cache of that type.
+    ClearEnumerationCache(cache);
+  } else if (!enumeration_in_progress_[stream_type]) {
+    enumeration_in_progress_[stream_type] = true;
+    GetDeviceManager(stream_type)->EnumerateDevices();
+  }
+}
+
+bool MediaStreamManager::HasEnumerationRequest() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  for (DeviceRequests::iterator it = requests_.begin();
+       it != requests_.end(); ++it) {
+    DeviceRequest& request = it->second;
+    if (request.type == DeviceRequest::kEnumerateDevices) {
+      return true;
+    }
+  }
+  return false;
+}
+
+bool MediaStreamManager::HasEnumerationRequest(
+    MediaStreamType stream_type) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  for (DeviceRequests::iterator it = requests_.begin();
+       it != requests_.end(); ++it) {
+    DeviceRequest& request = it->second;
+    if (request.type == DeviceRequest::kEnumerateDevices &&
+        Requested(request.options, stream_type)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 }  // namespace media_stream