Pass task runners to AudioManager constructor.

media/audio/audio_manager.h

 // 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.
 
 #ifndef MEDIA_AUDIO_AUDIO_MANAGER_H_
 #define MEDIA_AUDIO_AUDIO_MANAGER_H_
 
 #include <string>
 
 #include "base/macros.h"
@@ skipping 28 matching lines @@
   // This provides an alternative to the statically linked factory method used
   // to create AudioManager. This is useful for dynamically-linked third
   // party clients seeking to provide a platform-specific implementation of
   // AudioManager. After this is called, all static AudioManager::Create*
   // methods will return an instance of AudioManager provided by |factory|. This
   // call may be made at most once per process, and before any calls to static
   // AudioManager::Create* methods. This method takes ownership of |factory|,
   // 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);
+  // 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);
 
-  // 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.

+  // 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
   // of bad drivers has been given to Microsoft. This should be re-enabled in
   // the future if Microsoft is able to triage third party drivers.
   // See http://crbug.com/422522
   static void EnableCrashKeyLoggingForAudioThreadHangs();
 
 #if defined(OS_LINUX)
   // Sets the name of the audio source as seen by external apps. Only actually
@@ skipping 98 matching lines @@
   // Returns NULL if the combination of the parameters is not supported, or if
   // we have reached some other platform specific limit.
   //
   // Do not free the returned AudioInputStream. It is owned by AudioManager.
   // When you are done with it, call |Stop()| and |Close()| to release it.
   virtual AudioInputStream* MakeAudioInputStream(
       const AudioParameters& params,
       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
   // callback is to recreate the stream.
   class AudioDeviceListener {
    public:
     virtual void OnDeviceChange() = 0;
   };
 
   virtual void AddOutputDeviceChangeListener(AudioDeviceListener* listener) = 0;
@@ skipping 26 matching lines @@
   // GetWorkerTaskRunner()).
   virtual std::string GetAssociatedOutputDeviceID(
       const std::string& input_device_id) = 0;
 
   // Create a new AudioLog object for tracking the behavior for one or more
   // instances of the given component.  See AudioLogFactory for more details.
   virtual scoped_ptr<AudioLog> CreateAudioLog(
       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);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_AUDIO_AUDIO_MANAGER_H_