Caching AudioOutputDevice instances in mixer manager

media/base/audio_renderer_mixer_input.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.
 //
 // THREAD SAFETY
 //
 // This class is generally not thread safe. Callers should ensure thread safety.
 // For instance, the |sink_lock_| in WebAudioSourceProvider synchronizes access
 // to this object across the main thread (for WebAudio APIs) and the
 // media thread (for HTMLMediaElement APIs).
 //
 // The one exception is protection for |volume_| via |volume_lock_|. This lock
 // prevents races between SetVolume() (called on any thread) and ProvideInput
 // (called on audio device thread). See http://crbug.com/588992.
 
 #ifndef MEDIA_BASE_AUDIO_RENDERER_MIXER_INPUT_H_
 #define MEDIA_BASE_AUDIO_RENDERER_MIXER_INPUT_H_
 
 #include <string>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/synchronization/lock.h"
 #include "media/base/audio_converter.h"
 #include "media/base/audio_renderer_sink.h"
 #include "url/origin.h"
 
 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);
 
   // SwitchableAudioRendererSink implementation.
   void Start() override;
   void Stop() override;
   void Play() override;
   void Pause() override;
   bool SetVolume(double volume) override;
   OutputDeviceInfo GetOutputDeviceInfo() override;
   void Initialize(const AudioParameters& params,
                   AudioRendererSink::RenderCallback* renderer) override;
   void SwitchOutputDevice(const std::string& device_id,
                           const url::Origin& security_origin,
                           const OutputDeviceStatusCB& callback) override;
 
   // Called by AudioRendererMixer when an error occurs.
   void OnRenderError();
 
  protected:
   ~AudioRendererMixerInput() override;
 
  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_;
 
   bool started_;
   bool playing_;
   double volume_;
 
   // AudioConverter::InputCallback implementation.
   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.
   AudioRendererSink::RenderCallback* callback_;
 
   // Error callback for handing to AudioRendererMixer.
   const base::Closure error_cb_;
 
   // Pending switch-device callback, in case SwitchOutputDevice() is invoked
   // before Start()
   OutputDeviceStatusCB pending_switch_callback_;
   std::string pending_switch_device_id_;
   url::Origin pending_switch_security_origin_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioRendererMixerInput);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_AUDIO_RENDERER_MIXER_INPUT_H_