Pass task runners to AudioManager constructor.

media/audio/audio_manager.h

Index: media/audio/audio_manager.h
diff --git a/media/audio/audio_manager.h b/media/audio/audio_manager.h
index d3eea4a6b9cdcf5b944e076dc12e983a82109b05..7d23cecd7b9793978c68f5c48c08ba78f658741d 100644
--- a/media/audio/audio_manager.h
+++ b/media/audio/audio_manager.h
@@ -46,20 +46,32 @@ class MEDIA_EXPORT AudioManager {
   // which must not be NULL.
   static void SetFactory(AudioManagerFactory* factory);
 
-  // Construct the audio manager; only one instance is allowed.  The manager
-  // will forward CreateAudioLog() calls to the provided AudioLogFactory; as
-  // such |audio_log_factory| must outlive the AudioManager.
-  static AudioManager* Create(AudioLogFactory* audio_log_factory);
-
-  // Similar to Create() except also schedules a monitor on the given task
-  // runner to ensure the audio thread is not stuck for more than 60 seconds; if
-  // a hang is detected, the process will be crashed.  See EnableHangMonitor().
-  static AudioManager* CreateWithHangTimer(
-      AudioLogFactory* audio_log_factory,
-      const scoped_refptr<base::SingleThreadTaskRunner>& monitor_task_runner);
-
-  // Similar to Create() except uses a FakeAudioLogFactory for testing.
-  static AudioManager* CreateForTesting();

[DaleCurtis, 2016/03/18 17:48:18]

As mentioned elsewhere this might be worth keeping, but perhaps with the
addition of a task_runner.

[alokp, 2016/03/18 18:40:07]

Just one task_runner should be sufficient - no worker_task_runner?

[DaleCurtis, 2016/03/18 20:11:49]

Correct.

+  // Construct the audio manager; only one instance is allowed.
+  // The returned instance must be deleted on AudioManager::GetTaskRunnner().
+  //
+  // The manager will forward CreateAudioLog() calls to the provided
+  // AudioLogFactory; as such |audio_log_factory| must outlive the AudioManager.
+  //
+  // The manager will use |task_runner| for audio IO. If the given |task_runner|

[alokp, 2016/03/18 16:27:25]

We could make the task_runner arguments mandatory by creating the audio thread
in BrowserMainLoop. But then Cast needs a way to override it. One way to do that
would be to add ContentBrowserClient::GetAudioTaskRunner. The default
implementation will return NULL, in which case BML will create the audio thread.

ContentBrowserClient::GetAudioTaskRunner will work better for us and we will
also avoid the singleton AudioThread class but I am not sure if this function
belongs in ContentBrowserClient. We do have a
ContentBrowserClient::CreateCdmFactory, so maybe?

[DaleCurtis, 2016/03/18 20:11:49]

Hmm, if you take the scoped_ptr w/ custom deleter approach I suggested below, we
actually don't need to have BrowserMainLoop pass in the threads anymore; I think
we still want that to happen under the hood. The deleter can just put it on the
right thread naturally.

So something like:

public ScopedAudioManager Create(factory, monitor_task_runner);
public ScopedAudioManager CreateForTesting(task_runner);

protected static CreateDefaultAudioTaskRunner();
private static CreateAudioManager(factory, task_runner);
private AudioManager(factory, task_runner),

Within most of the platform specific CreateAudioManager() methods, you would
just do something like:

return <Platform>AudioManager(factory, task_runner ? task_runner :
CreateDefaultAudioTaskRunner());

For Cast and OSX you would instead do something like:

return <Cast|OSX>AudioManager>(factory, task_runner ? task_runner :
<custom_task_runner>);

Internally the OSX and Cast ones can either use CreateDefaultAudioTaskRunner()
or spin up their own thread for the worker task runner. I don't think any other
platform needs to worry about specifying this.

During destruction ScopedAudioManager will do pretty much what your destroy
method does now, but without needing the explicit addition in BML.

I think this minimizes the changes in BML and elsewhere while getting all the
gains we're looking for.

+  // is NULL, the implementation will choose a default task runner, which can
+  // be obtained by calling GetTaskRunner().
+  //
+  // The manager will use |worker_task_runner| for heavyweight tasks.
+  // The |worker_task_runner| may be the same as |task_runner| or NULL, in
+  // which case the implementation will choose a default one.
+  //
+  // If |monitor_task_runner| is not NULL, a monitor will be scheduled on

[DaleCurtis, 2016/03/18 17:48:18]

This should just say that a UMA will be logged now. We don't crash anymore since
it's a third party driver / OS bug that isn't being addressed readily by
Microsoft :( Really you can just say that |monitor_task_runner| will be used to
monitor |task_runner|.

[alokp, 2016/03/18 18:40:06]

Done.

+  // to ensure that |task_runner| is not stuck for more than 60 seconds; if
+  // a hang is detected, the process will be crashed. See EnableHangMonitor().
+  static AudioManager* Create(

[DaleCurtis, 2016/03/18 17:48:18]

Could we just have this return a scoped_ptr w/ custom deleter that calls
Destroy? I'm fine with doing that in a follow up patch if it proves to be churn
in this CL. 

[alokp, 2016/03/18 18:40:07]

Acknowledged.

+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,

[DaleCurtis, 2016/03/18 17:48:18]

Per recent chromium-dev discussion, now that we have movable types we should be
passing scoped_refptr by value.

[alokp, 2016/03/18 18:40:07]

Done.

+      const scoped_refptr<base::SingleThreadTaskRunner>& worker_task_runner,
+      const scoped_refptr<base::SingleThreadTaskRunner>& monitor_task_runner,
+      AudioLogFactory* audio_log_factory);
+
+  // Helper function to ensure that AudioManager is properly shutdown
+  // and destroyed on the audio thread.
+  static void Destroy(AudioManager* instance);
 
   // Enables non-crash dumps when audio thread hangs are detected.
   // TODO(dalecurtis): There are no callers to this function at present. A list
@@ -178,12 +190,18 @@ class MEDIA_EXPORT AudioManager {
       const std::string& device_id) = 0;
 
   // Returns the task runner used for audio IO.
-  virtual scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() = 0;
+  // TODO(alokp): Rename to task_runner().
+  scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() const {
+    return task_runner_;
+  }
 
   // Heavyweight tasks should use GetWorkerTaskRunner() instead of
   // GetTaskRunner(). On most platforms they are the same, but some share the
   // UI loop with the audio IO loop.
-  virtual scoped_refptr<base::SingleThreadTaskRunner> GetWorkerTaskRunner() = 0;
+  // TODO(alokp): Rename to worker_task_runner().
+  scoped_refptr<base::SingleThreadTaskRunner> GetWorkerTaskRunner() const {
+    return worker_task_runner_;
+  }
 
   // Allows clients to listen for device state changes; e.g. preferred sample
   // rate or channel layout changes.  The typical response to receiving this
@@ -230,9 +248,13 @@ class MEDIA_EXPORT AudioManager {
       AudioLogFactory::AudioComponent component) = 0;
 
  protected:
-  AudioManager();
+  AudioManager(
+      const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+      const scoped_refptr<base::SingleThreadTaskRunner>& worker_task_runner);
 
  private:
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner_;
   DISALLOW_COPY_AND_ASSIGN(AudioManager);
 };