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

content/browser/renderer_host/media/media_stream_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/media_stream_manager.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 31 matching lines @@
 #include "content/public/browser/web_contents_media_capture_id.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/media_stream_request.h"
 #include "crypto/hmac.h"
 #include "media/audio/audio_device_description.h"
 #include "media/audio/audio_system.h"
 #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"
 
 #if defined(OS_WIN)
 #include "base/win/scoped_com_initializer.h"
 #endif
 
 #if defined(OS_CHROMEOS)
 #include "chromeos/audio/cras_audio_handler.h"
@@ skipping 332 matching lines @@
 
 MediaStreamManager::MediaStreamManager(media::AudioSystem* audio_system)
     : audio_system_(audio_system),
 #if defined(OS_WIN)
       video_capture_thread_("VideoCaptureThread"),
 #endif
       use_fake_ui_(base::CommandLine::ForCurrentProcess()->HasSwitch(
           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.
   if (power_monitor)
     power_monitor->AddObserver(this);
 }
 
 MediaStreamManager::~MediaStreamManager() {
   DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::IO));
   DVLOG(1) << "~MediaStreamManager";
   DCHECK(requests_.empty());
 
   base::PowerMonitor* power_monitor = base::PowerMonitor::Get();
   // The PowerMonitor instance owned by BrowserMainLoops always outlives the
   // MediaStreamManager, but it may be NULL in unit tests.
   if (power_monitor)
     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());
   return video_capture_manager_.get();
 }
 
 AudioInputDeviceManager* MediaStreamManager::audio_input_device_manager() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(audio_input_device_manager_.get());
   return audio_input_device_manager_.get();
@@ skipping 763 matching lines @@
     const std::string& label,
     DeviceRequest* request,
     const MediaStreamDevices& devices) {
   if (!request->callback.is_null())
     request->callback.Run(devices, std::move(request->ui_proxy));
 
   // Delete the request since it is done.
   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
   // attaching to the VM is required and we may have to access the MSM from
   // callback threads that we don't own and don't want to attach.
   g_media_stream_manager_tls_ptr.Pointer()->Set(this);
 
   // TODO(dalecurtis): Remove ScopedTracker below once crbug.com/457525 is
   // fixed.
   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.
   tracked_objects::ScopedTracker tracking_profile2(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "457525 MediaStreamManager::InitializeDeviceManagersOnIOThread 2"));
   audio_input_device_manager_ =
       new AudioInputDeviceManager(audio_system_->GetAudioManager());
   audio_input_device_manager_->RegisterListener(this);
 
   // TODO(dalecurtis): Remove ScopedTracker below once crbug.com/457525 is
   // fixed.
   tracked_objects::ScopedTracker tracking_profile3(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "457525 MediaStreamManager::InitializeDeviceManagersOnIOThread 3"));
   // We want to be notified of IO message loop destruction to delete the thread
   // and the device managers.
   base::MessageLoop::current()->AddDestructionObserver(this);
 
   // TODO(dalecurtis): Remove ScopedTracker below once crbug.com/457525 is
   // fixed.
   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,
                                 int capture_session_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DVLOG(1) << "Opened({stream_type = " << stream_type <<  "} "
            << "{capture_session_id = " << capture_session_id << "})";
   // Find the request(s) containing this device and mark it as used.
   // It can be used in several requests since the same device can be
   // requested from the same web page.
@@ skipping 482 matching lines @@
     request->ui_proxy->OnStarted(
         base::Bind(&MediaStreamManager::StopMediaStreamFromBrowser,
                    base::Unretained(this), label),
         base::Bind(&MediaStreamManager::OnMediaStreamUIWindowId,
                    base::Unretained(this), request->video_type(),
                    request->devices));
   }
 }
 
 }  // namespace content