[Mojo Video Capture] Fix VideoCaptureManager exposing implementation details to clients

content/browser/renderer_host/media/media_stream_manager.cc

Index: content/browser/renderer_host/media/media_stream_manager.cc
diff --git a/content/browser/renderer_host/media/media_stream_manager.cc b/content/browser/renderer_host/media/media_stream_manager.cc
index 5d7884eb4f676b064722e49a85da749e1d684ceb..d5a7005697097964a4606bf6786037c9e5942bac 100644
--- a/content/browser/renderer_host/media/media_stream_manager.cc
+++ b/content/browser/renderer_host/media/media_stream_manager.cc
@@ -49,6 +49,7 @@
 #include "media/base/audio_parameters.h"
 #include "media/base/channel_layout.h"
 #include "media/base/media_switches.h"
+#include "media/capture/video/video_capture_device_factory.h"
 #include "media/capture/video/video_capture_system.h"
 #include "url/gurl.h"
 #include "url/origin.h"
@@ -401,17 +402,6 @@ MediaStreamManager::MediaStreamManager(media::AudioSystem* audio_system)
           switches::kUseFakeUIForMediaStream)) {
   DCHECK(audio_system_);
 
-  // Some unit tests create the MSM in the IO thread and assumes the
-  // initialization is done synchronously.
-  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-    InitializeDeviceManagersOnIOThread();
-  } else {
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::Bind(&MediaStreamManager::InitializeDeviceManagersOnIOThread,
-                   base::Unretained(this)));
-  }
-
   base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
   // BrowserMainLoop always creates the PowerMonitor instance before creating
   // MediaStreamManager, but power_monitor may be NULL in unit tests.
@@ -431,6 +421,40 @@ MediaStreamManager::~MediaStreamManager() {
     power_monitor->RemoveObserver(this);
 }
 
+// static
+std::unique_ptr<MediaStreamManager> MediaStreamManager::CreateWithDefaults(
+    media::AudioSystem* audio_system) {
+  std::unique_ptr<MediaStreamManager> result(
+      new MediaStreamManager(audio_system));
+  result->InitializeWithDefaults();
+  return result;
+}
+
+// static
+std::unique_ptr<MediaStreamManager>
+MediaStreamManager::CreateWithCustomVideoCaptureDeviceTaskRunner(
+    media::AudioSystem* audio_system,
+    scoped_refptr<base::SingleThreadTaskRunner> device_task_runner) {
+  std::unique_ptr<MediaStreamManager> result(
+      new MediaStreamManager(audio_system));
+  result->InitializeWithCustomVideoCaptureDeviceTaskRunner(
+      std::move(device_task_runner));
+  return result;
+}
+
+// static
+std::unique_ptr<MediaStreamManager>
+MediaStreamManager::CreateWithCustomVideoCaptureSystem(
+    media::AudioSystem* audio_system,
+    std::unique_ptr<media::VideoCaptureSystem> video_capture_system,
+    scoped_refptr<base::SingleThreadTaskRunner> device_task_runner) {
+  std::unique_ptr<MediaStreamManager> result(
+      new MediaStreamManager(audio_system));
+  result->InitializeWithCustomVideoCaptureSystem(
+      std::move(video_capture_system), std::move(device_task_runner));
+  return result;
+}
+
 VideoCaptureManager* MediaStreamManager::video_capture_manager() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(video_capture_manager_.get());
@@ -1214,8 +1238,74 @@ void MediaStreamManager::FinalizeMediaAccessRequest(
   DeleteRequest(label);
 }
 
-void MediaStreamManager::InitializeDeviceManagersOnIOThread() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+void MediaStreamManager::InitializeWithDefaults() {
+  scoped_refptr<base::SingleThreadTaskRunner> device_task_runner =
+      audio_system_->GetTaskRunner();
+#if defined(OS_WIN)
+  // Use an STA Video Capture Thread to try to avoid crashes on enumeration of
+  // buggy third party Direct Show modules, http://crbug.com/428958.
+  video_capture_thread_.init_com_with_mta(false);
+  CHECK(video_capture_thread_.Start());
+  device_task_runner = video_capture_thread_.task_runner();
+#endif
+  InitializeWithCustomVideoCaptureDeviceTaskRunner(
+      std::move(device_task_runner));
+}
+
+void MediaStreamManager::InitializeWithCustomVideoCaptureDeviceTaskRunner(
+    scoped_refptr<base::SingleThreadTaskRunner> device_task_runner) {
+  // Some unit tests initialize the MSM in the IO thread and assume the
+  // initialization is done synchronously. Other clients call this from a
+  // different thread and expect initialization to run asynchronously.
+  if (BrowserThread::CurrentlyOn(BrowserThread::IO) == false) {

[miu, 2017/04/03 21:31:23]

Usually, the style is:

  if (!BrowserThread::CurrentlyOn(...)) {

(I'm assuming CurrentlyOn() returns bool, and nothing fancy, right?)

[chfremer, 2017/04/04 21:59:32]

Done.

+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&MediaStreamManager::
+                       InitializeWithCustomVideoCaptureDeviceTaskRunner,
+                   base::Unretained(this), base::Passed(&device_task_runner)));
+    return;
+  }
+
+  PerformCommonInitializationRoutines();
+
+  auto video_capture_system = base::MakeUnique<media::VideoCaptureSystemImpl>(
+      media::VideoCaptureDeviceFactory::CreateFactory(
+          BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
+  video_capture_manager_ = new VideoCaptureManager(
+      std::move(video_capture_system), std::move(device_task_runner));
+  video_capture_manager_->RegisterListener(this);
+
+  media_devices_manager_.reset(
+      new MediaDevicesManager(audio_system_, video_capture_manager_, this));
+}
+
+void MediaStreamManager::InitializeWithCustomVideoCaptureSystem(
+    std::unique_ptr<media::VideoCaptureSystem> video_capture_system,
+    scoped_refptr<base::SingleThreadTaskRunner> device_task_runner) {
+  // Some unit tests initialize the MSM in the IO thread and assume the
+  // initialization is done synchronously. Other clients call this from a
+  // different thread and expect initialization to run asynchronously.
+  if (BrowserThread::CurrentlyOn(BrowserThread::IO) == false) {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&MediaStreamManager::InitializeWithCustomVideoCaptureSystem,
+                   base::Unretained(this), base::Passed(&video_capture_system),
+                   std::move(device_task_runner)));
+    return;
+  }
+
+  PerformCommonInitializationRoutines();
+
+  video_capture_manager_ = new VideoCaptureManager(
+      std::move(video_capture_system), std::move(device_task_runner));
+  video_capture_manager_->RegisterListener(this);
+
+  media_devices_manager_.reset(
+      new MediaDevicesManager(audio_system_, video_capture_manager_, this));
+}
+
+void MediaStreamManager::PerformCommonInitializationRoutines() {

[miu, 2017/04/03 21:31:23]

After reviewing all these CreateXXX() and InitializeXXX() methods, I'd like to
suggest something simpler: Just do it all in the ctor, like it was being done
before. However, use a 3-argument ctor where the VideoCaptureSystem and
TaskRunner arguments are optional. Something like:

  // |audio_system| is required, but defaults will be used if either
  // |video_capture_system| or |device_task_runner| are null.
  MediaStreamManager(media::AudioSystem* audio_system,
                     std::unique_ptr<media::VideoCaptureSystem>
video_capture_system,
                     scoped_refptr<base::SingleThreadTaskRunner>
device_task_runner);

Then, in the .cc file, there's just one, linear, easy-to-follow workflow where,
somewhere we have:

  if (!video_capture_system) {
    video_capture_system = base::MakeUnique<media::VideoCaptureSystemImpl>(
        media::VideoCaptureDeviceFactory::CreateFactory(
            BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
  }

  if (!device_task_runner) {
#if defined(OS_WIN)
    // Use an STA Video Capture Thread to try to avoid crashes on enumeration
of	
    // buggy third party Direct Show modules, http://crbug.com/428958.	
    video_capture_thread_.init_com_with_mta(false);	
    CHECK(video_capture_thread_.Start());
    device_task_runner = video_capture_thread_.task_runner();
#else
    device_task_runner = audio_system_->GetTaskRunner();
#endif
  }

[chfremer, 2017/04/04 21:59:32]

Thanks. That allows eliminating a lot of ugliness and duplication.
I want to note, though, that it changes some threading. Before, the constructor
of VideoCaptureSystemImpl and the static method
media::VideoCaptureDeviceFactory::CreateFactory() were executed on the
Browser::IO thread whereas after the simplification they are run on whichever
thread calls the constructor of MediaStreamManager (which is Browser Main thread
in production). But unless it causes any test failures, I assume that this is
not a problem.

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // Store a pointer to |this| on the IO thread to avoid having to jump to the
   // UI thread to fetch a pointer to the MSM. In particular on Android, it can
   // be problematic to post to a UI thread from arbitrary worker threads since
@@ -1228,7 +1318,6 @@ void MediaStreamManager::InitializeDeviceManagersOnIOThread() {
   tracked_objects::ScopedTracker tracking_profile1(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "457525 MediaStreamManager::InitializeDeviceManagersOnIOThread 1"));
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // TODO(dalecurtis): Remove ScopedTracker below once crbug.com/457525 is
   // fixed.
@@ -1253,25 +1342,6 @@ void MediaStreamManager::InitializeDeviceManagersOnIOThread() {
   tracked_objects::ScopedTracker tracking_profile4(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "457525 MediaStreamManager::InitializeDeviceManagersOnIOThread 4"));
-  auto video_capture_system = base::MakeUnique<media::VideoCaptureSystem>(
-      media::VideoCaptureDeviceFactory::CreateFactory(
-          BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)));
-#if defined(OS_WIN)
-  // Use an STA Video Capture Thread to try to avoid crashes on enumeration of
-  // buggy third party Direct Show modules, http://crbug.com/428958.
-  video_capture_thread_.init_com_with_mta(false);
-  CHECK(video_capture_thread_.Start());
-  video_capture_manager_ = new VideoCaptureManager(
-      std::move(video_capture_system), video_capture_thread_.task_runner());
-#else
-  video_capture_manager_ = new VideoCaptureManager(
-      std::move(video_capture_system), audio_system_->GetTaskRunner());
-#endif
-
-  video_capture_manager_->RegisterListener(this);
-
-  media_devices_manager_.reset(
-      new MediaDevicesManager(audio_system_, video_capture_manager_, this));
 }
 
 void MediaStreamManager::Opened(MediaStreamType stream_type,