Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one.

media/audio/audio_system.h

Index: media/audio/audio_system.h
diff --git a/media/audio/audio_system.h b/media/audio/audio_system.h
index 66dc622db267575d189e74adc5fb4729c6fbc6c7..ecbd7b60ec183cdb7975229fea95cc545a26f436 100644
--- a/media/audio/audio_system.h
+++ b/media/audio/audio_system.h
@@ -6,6 +6,7 @@
 #define MEDIA_AUDIO_AUDIO_SYSTEM_H_
 
 #include "base/callback.h"
+#include "base/memory/weak_ptr.h"
 #include "media/audio/audio_device_description.h"
 #include "media/base/audio_parameters.h"
 #include "media/base/media_export.h"
@@ -23,13 +24,18 @@ class AudioManager;
 class MEDIA_EXPORT AudioSystem {
  public:
   // Replies are asynchronously sent from audio system thread to the thread the
-  // call is issued on. Attention! Since audio system thread may outlive all the
-  // others, callbacks must always be bound to weak pointers!
+  // call is issued on. Attention! Audio system thread may outlive the client
+  // objects; bind callbacks with care.
   using OnAudioParamsCallback = base::Callback<void(const AudioParameters&)>;
   using OnBoolCallback = base::Callback<void(bool)>;
   using OnDeviceDescriptionsCallback =
       base::Callback<void(AudioDeviceDescriptions)>;
+  using OnDeviceIdCallback = base::Callback<void(const std::string&)>;
+  using OnInputDeviceInfoCallback = base::Callback<
+      void(const AudioParameters&, const AudioParameters&, const std::string&)>;
 
+  // Must not be called on audio system thread if it differs from the one
+  // AudioSystem is destroyed on.

[o1ka, 2017/03/28 16:40:16]

See also this comment
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?type....

Now AudioSystem is deleted on UI thread before Audiomanager/AudioThread, which
means there potentially can be AudioSystem operation scheduled on the audio
thread after AudioSystem is deleted.

Audio thread needs to be stopped before AudioSystem is deleted.
One option is to make AudioSystem to own AudioManager - which I don't really
like, because we want to expose AM stream creation functionality through a
separate interface.

Another option is to have an "audio owner" which would own AudioThread,
AudioManager and AudioSystem and would do a correct shutdown (something like
shuts down AM, deleted AS, and then deletes AM).

It depends on how the new shut down logic for AudioManager looks like.
alokp@ have you figured it out?

In any case, there are no AudioSystem operations posted to the audio thread as
of now, so there is no use after free (which does not make it a good design
though).

[DaleCurtis, 2017/03/28 18:22:11]

This comment will need resolution and removal before submission, such a
restriction is too onerous for callers to actually verify exhaustively. We don't
want internal details of the AM implementation to leak here.

[o1ka, 2017/03/30 15:11:53]

Agree. This is a problem shared between AM and AS.
https://codereview.chromium.org/2784433002/ needs to address it.
Should we wait for it to land?

   static AudioSystem* Get();
 
   virtual ~AudioSystem();
@@ -59,9 +65,21 @@ class MEDIA_EXPORT AudioSystem {
   // Replies with device descriptions of input audio devices if |for_input| is
   // true, and of output audio devices otherwise.
   virtual void GetDeviceDescriptions(
-      OnDeviceDescriptionsCallback on_descriptions_cp,
+      OnDeviceDescriptionsCallback on_descriptions_cb,
       bool for_input) = 0;
 
+  // Replies with an empty string if there is no associated output device found.
+  virtual void GetAssociatedOutputDeviceID(
+      const std::string& input_device_id,
+      OnDeviceIdCallback on_device_id_cb) = 0;

[o1ka, 2017/03/28 16:40:16]

This method will be used by WebRTC private API as well.

+
+  // Replies with audio parameters for the specified input device and audio
+  // parameters and device ID of the associated output device, if any (otherwise
+  // it's AudioParameters() and an empty string).
+  virtual void GetInputDeviceInfo(
+      const std::string& input_device_id,
+      OnInputDeviceInfoCallback on_input_device_info_cb) = 0;

[o1ka, 2017/03/28 16:40:16]

I do not really like it because it's basically a composite method consisting of
GetInputStreamParameters(), GetAssociatedOutputDeviceID() and
GetOutputStreamParameters().
On the other hand, with Mojo we'll end up with 3 IPC calls to get this info,
which is not nice as well.

[DaleCurtis, 2017/03/28 18:22:11]

I guess it depends on how callers end up using this normally. Do they end up
issuing all 3 callbacks to get the information they need in regular course? Are
any clients only using one of these?

[o1ka, 2017/03/30 15:11:53]

Each of the operations has multiple users.
AudioInputDeviceManager is the only one who runs a sequence of calls. It always
gets associated output device id; input parameters - if there is no "use fake
device" flag, and output parameters if there is an associated output device and
no "use fake" flag. All this is done for each device in the enumeration.

In the future there may be other situations when we want to execute a sequence
of operations in the audio process, but for now it's only this case. So to me
having this one combined call is an acceptable compromise. (Actually, I would
prefer to refactor the whole audio device enumeration at some point)

+
   virtual base::SingleThreadTaskRunner* GetTaskRunner() const = 0;
 
   // Must not be used for anything but stream creation.