Caching AudioOutputDevice instances in mixer manager

content/renderer/media/audio_renderer_mixer_manager.h

Index: content/renderer/media/audio_renderer_mixer_manager.h
diff --git a/content/renderer/media/audio_renderer_mixer_manager.h b/content/renderer/media/audio_renderer_mixer_manager.h
index 20a1aa59eaf3db3d8e65c0ef805dfe98c16e5e9e..6dec29eaa4686ad524ec74ff4729529abfd3d610 100644
--- a/content/renderer/media/audio_renderer_mixer_manager.h
+++ b/content/renderer/media/audio_renderer_mixer_manager.h
@@ -6,25 +6,26 @@
 #define CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_MIXER_MANAGER_H_
 
 #include <map>
+#include <memory>
 #include <string>
-#include <utility>
 
 #include "base/macros.h"
 #include "base/synchronization/lock.h"
 #include "content/common/content_export.h"
 #include "media/audio/audio_device_description.h"
 #include "media/base/audio_parameters.h"
+#include "media/base/audio_renderer_mixer_pool.h"
 #include "media/base/output_device_info.h"
 #include "url/origin.h"
 
 namespace media {
-class AudioHardwareConfig;
 class AudioRendererMixer;
 class AudioRendererMixerInput;
 class AudioRendererSink;
 }
 
 namespace content {
+class AudioRendererSinkCache;
 
 // Manages sharing of an AudioRendererMixer among AudioRendererMixerInputs based
 // on their AudioParameters configuration.  Inputs with the same AudioParameters
@@ -39,10 +40,12 @@ namespace content {
 // with floats.  However, bits per channel is currently used to interleave the
 // audio data by AudioOutputDevice::AudioThreadCallback::Process for consumption
 // via the shared memory.  See http://crbug.com/114700.
-class CONTENT_EXPORT AudioRendererMixerManager {
+class CONTENT_EXPORT AudioRendererMixerManager
+    : public media::AudioRendererMixerPool {
  public:
-  AudioRendererMixerManager();
-  ~AudioRendererMixerManager();
+  ~AudioRendererMixerManager() final;
+
+  static std::unique_ptr<AudioRendererMixerManager> Create();
 
   // Creates an AudioRendererMixerInput with the proper callbacks necessary to
   // retrieve an AudioRendererMixer instance from AudioRendererMixerManager.
@@ -60,19 +63,31 @@ class CONTENT_EXPORT AudioRendererMixerManager {
 
   // Returns a mixer instance based on AudioParameters; an existing one if one

[Guido Urdaneta, 2016/05/25 13:33:13]

Since GetMixer and ReturnMixer are now implementations of an interface, you
should just have a comment saying "AudioRendererMixerPool implementation." and
rely on the documentation of the interface.

[o1ka, 2016/05/25 14:52:11]

This comment contains implementation details not exposed in the interface. Is it
ok to have it here?

[Guido Urdaneta, 2016/05/25 15:44:09]

I think the comments at the top of the class together with the interface
comments already contain what these comments say. Perhaps you can complement the
class comment saying that mixers are removed when they are no longer referenced.

[o1ka, 2016/05/26 09:00:27]

Done.

   // with the provided AudioParameters exists or a new one if not.
-  media::AudioRendererMixer* GetMixer(int source_render_frame_id,
-                                      const media::AudioParameters& params,
-                                      const std::string& device_id,
-                                      const url::Origin& security_origin,
-                                      media::OutputDeviceStatus* device_status);
+  media::AudioRendererMixer* GetMixer(
+      int source_render_frame_id,
+      const media::AudioParameters& params,
+      const std::string& device_id,
+      const url::Origin& security_origin,
+      media::OutputDeviceStatus* device_status) final;
 
   // Remove a mixer instance given a mixer if the only other reference is held
   // by AudioRendererMixerManager.  Every AudioRendererMixer owner must call
   // this method when it's done with a mixer.
-  void RemoveMixer(int source_render_frame_id,
+  void ReturnMixer(int source_render_frame_id,
                    const media::AudioParameters& params,
                    const std::string& device_id,
-                   const url::Origin& security_origin);
+                   const url::Origin& security_origin) final;
+
+  // Returns output device information. This call goes to the sink cache.

[Guido Urdaneta, 2016/05/25 13:33:13]

Nit: So far, the sink cache has not been mentioned in the docs for this class,
but you are referring to it.
If you choose to refer to it, perhaps you should explain somewhere the existence
of the cache. Alternatively, you may just not mention the cache in this comment
and consider it an implementation detail.

[o1ka, 2016/05/25 14:52:11]

changed to |sink_cache_| which is mentioned here.

+  media::OutputDeviceInfo GetOutputDeviceInfo(
+      int source_render_frame_id,
+      int session_id,
+      const std::string& device_id,
+      const url::Origin& security_origin) final;
+
+ protected:
+  explicit AudioRendererMixerManager(
+      std::unique_ptr<AudioRendererSinkCache> sink_cache);
 
  private:
   friend class AudioRendererMixerManagerTest;
@@ -127,14 +142,20 @@ class CONTENT_EXPORT AudioRendererMixerManager {
   struct AudioRendererMixerReference {
     media::AudioRendererMixer* mixer;
     int ref_count;
+    // Mixer sink pointer, to remove a sink from cache upon mixer destruction.
+    const media::AudioRendererSink* sink_ptr;

[Guido Urdaneta, 2016/05/25 13:33:13]

What about adding a sink() getter to ARM, so that you get the sink pointer from
|mixer| instead of storing it here?
Not a blocker.

[o1ka, 2016/05/25 14:52:11]

We've already been there at PS7 :) changed according to Dale's recommendation.

[Guido Urdaneta, 2016/05/25 15:44:09]

Acknowledged.

   };
-  typedef std::map<MixerKey, AudioRendererMixerReference, MixerKeyCompare>
-      AudioRendererMixerMap;
+
+  using AudioRendererMixerMap =
+      std::map<MixerKey, AudioRendererMixerReference, MixerKeyCompare>;
 
   // Active mixers.
   AudioRendererMixerMap mixers_;
   base::Lock mixers_lock_;
 
+  // Mixer sink cache.
+  const std::unique_ptr<AudioRendererSinkCache> sink_cache_;
+
   DISALLOW_COPY_AND_ASSIGN(AudioRendererMixerManager);
 };