Caching AudioOutputDevice instances in mixer manager

content/renderer/media/audio_renderer_mixer_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 CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_MIXER_MANAGER_H_
 #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
 // configuration will share a mixer while a new AudioRendererMixer will be
 // lazily created if one with the exact AudioParameters does not exist.
 //
 // There should only be one instance of AudioRendererMixerManager per render
 // thread.
 //
 // TODO(dalecurtis): Right now we require AudioParameters to be an exact match
 // when we should be able to ignore bits per channel since we're only dealing
 // 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.
   // |source_render_frame_id| refers to the RenderFrame containing the entity
   // rendering the audio.  Caller must ensure AudioRendererMixerManager outlives
   // the returned input. |device_id|, |session_id| and |security_origin|
   // identify the output device to use. If |device_id| is empty and |session_id|
   // is nonzero, output device associated with the opened input device
   // designated by |session_id| is used. Otherwise, |session_id| is ignored.
   media::AudioRendererMixerInput* CreateInput(
       int source_render_frame_id,
       int session_id,
       const std::string& device_id,
       const url::Origin& security_origin);
 
   // 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;
 
   // Define a key so that only those AudioRendererMixerInputs from the same
   // RenderView, AudioParameters and output device can be mixed together.
   struct MixerKey {
     MixerKey(int source_render_frame_id,
              const media::AudioParameters& params,
              const std::string& device_id,
@@ skipping 34 matching lines @@
       return a.security_origin < b.security_origin;
     }
   };
 
   // Map of MixerKey to <AudioRendererMixer, Count>.  Count allows
   // AudioRendererMixerManager to keep track explicitly (v.s. RefCounted which
   // is implicit) of the number of outstanding AudioRendererMixers.
   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);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_MIXER_MANAGER_H_