Caching AudioOutputDevice instances in mixer manager

media/base/audio_renderer_mixer_input.h

Index: media/base/audio_renderer_mixer_input.h
diff --git a/media/base/audio_renderer_mixer_input.h b/media/base/audio_renderer_mixer_input.h
index a75320e78c681c73303c8d65bca0f9fb8a7cfd30..4cac30eef34180cee520d41884a7a0218372b1bf 100644
--- a/media/base/audio_renderer_mixer_input.h
+++ b/media/base/audio_renderer_mixer_input.h
@@ -27,24 +27,15 @@
 
 namespace media {
 
+class AudioRendererMixerPool;
 class AudioRendererMixer;
 
 class MEDIA_EXPORT AudioRendererMixerInput
     : NON_EXPORTED_BASE(public SwitchableAudioRendererSink),
       public AudioConverter::InputCallback {
  public:
-  typedef base::Callback<AudioRendererMixer*(const AudioParameters& params,
-                                             const std::string& device_id,
-                                             const url::Origin& security_origin,
-                                             OutputDeviceStatus* device_status)>
-      GetMixerCB;
-  typedef base::Callback<void(const AudioParameters& params,
-                              const std::string& device_id,
-                              const url::Origin& security_origin)>
-      RemoveMixerCB;
-
-  AudioRendererMixerInput(const GetMixerCB& get_mixer_cb,
-                          const RemoveMixerCB& remove_mixer_cb,
+  AudioRendererMixerInput(AudioRendererMixerPool* mixer_pool,
+                          int source_render_frame_id,
                           const std::string& device_id,
                           const url::Origin& security_origin);
 
@@ -70,6 +61,9 @@ class MEDIA_EXPORT AudioRendererMixerInput
  private:
   friend class AudioRendererMixerInputTest;
 
+  // Pool to obtain mixers from / return them to.
+  AudioRendererMixerPool* const mixer_pool_;
+
   // Protect |volume_|, accessed by separate threads in ProvideInput() and
   // SetVolume().
   base::Lock volume_lock_;
@@ -82,20 +76,15 @@ class MEDIA_EXPORT AudioRendererMixerInput
   double ProvideInput(AudioBus* audio_bus,
                       base::TimeDelta buffer_delay) override;
 
-  // Callbacks provided during construction which allow AudioRendererMixerInput
-  // to retrieve a mixer during Initialize() and notify when it's done with it.
-  const GetMixerCB get_mixer_cb_;
-  const RemoveMixerCB remove_mixer_cb_;
-
   // AudioParameters received during Initialize().
   AudioParameters params_;
 
-  // ID of hardware device to use
-  std::string device_id_;
+  const int source_render_frame_id_;

[miu, 2016/05/19 22:27:16]

Dale may have already mentioned this, but "render frame ID" is a concept that
should not be known to src/media code (since src/media code cannot depend on
src/content code). I'm afraid you'll have to find another way to plumb this
through. Suggestion: Can the object that implements AudioRendererMixerPool store
the |source_render_frame_id_| instead?

[o1ka, 2016/05/20 10:40:32]

...Well, "render frame ID" hadn't been known to mixer inputs util a colleague of
mine proposed passing it in the constructor. :)

"Hmm...This is now the third callback being injected, which is starting to abuse
the main purpose of Callbacks (to invoke unknown external implementation after
an asynchronous operation completes). Consider replacing these with an interface
like:

  class MixerPool {
    virtual AudioRendererMixer* AcquireMixer(...) = 0;
    virtual void ReleaseMixer(...) = 0;
    virtual OutputDeviceInfo GetDeviceInfo(...) = 0;
  };

...and the constructor becomes:

  AudioRendererMixerInput(MixerPool* pool,
                          const std::string& device_id,
                          const url::Origin& security_origin);"


1) Why an innocent int named "render frame ID" means introducing a whole new
dependency of media to content? Maybe it's just a naming problem? We can call it
"owner id" or whatever (open to suggestions, I'm bad at naming).

2) No, MixerPool as it is now cannot hold render frame id, because it serves all
the mixer inputs for the renderer process and it does not keep track of them.

3) If you insist that we need to remove that int, I can, for example:
a) Store it in the mixer input as an "unnamed blob".
b) Instead of passing frame id into constructor, pass a callback returning frame
id or an "unamed blob". 
(both of the above still expose frame id in pool interface)
c) : b) + pass that callback as a parameter to MixerPool interface instead of
frame id.
d) Stand on my head (well, actually no, I cannot).
e) Return back to binding (which looks like a more natural solution to me).
g) Have a wrapper class that implements MixerPool interface, holds frame id and
redirects all calls to mixer manager (this is an extra layer of indirection
which does not add much value).
h) ?

I hope that just renaming will do though.
What would you suggest? 

[o1ka, 2016/05/23 16:16:55]

Renamed.

[miu, 2016/05/25 01:23:20]

It's not that it is introducing a dependency. It is that "render frame" is a
concept unknown to src/media code. It's an abstraction layer violation, since
src/media code is completely independent of src/content concepts.

I'm fine with just calling it "owner_id" to mitigate this.

+  std::string device_id_;  // ID of hardware device to use
   url::Origin security_origin_;
 
-  // AudioRendererMixer provided through |get_mixer_cb_| during Initialize(),
-  // guaranteed to live (at least) until |remove_mixer_cb_| is called.
+  // AudioRendererMixer obtained from mixer pool during Initialize(),
+  // guaranteed to live (at least) until it is returned to the pool.
   AudioRendererMixer* mixer_;
 
   // Source of audio data which is provided to the mixer.