Caching AudioOutputDevice instances in mixer manager

content/renderer/media/audio_renderer_mixer_manager.cc

Index: content/renderer/media/audio_renderer_mixer_manager.cc
diff --git a/content/renderer/media/audio_renderer_mixer_manager.cc b/content/renderer/media/audio_renderer_mixer_manager.cc
index 75b7ed60f306a71abaf34b8144bf639f0791882f..b1ce5502ddc965426178e4029817f3098c05c222 100644
--- a/content/renderer/media/audio_renderer_mixer_manager.cc
+++ b/content/renderer/media/audio_renderer_mixer_manager.cc
@@ -8,8 +8,9 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/memory/ptr_util.h"
 #include "build/build_config.h"
-#include "content/renderer/media/audio_device_factory.h"
+#include "content/renderer/media/audio_renderer_sink_cache.h"
 #include "media/audio/audio_device_description.h"
 #include "media/base/audio_hardware_config.h"
 #include "media/base/audio_renderer_mixer.h"
@@ -17,7 +18,15 @@
 
 namespace content {
 
-AudioRendererMixerManager::AudioRendererMixerManager() {}
+// static
+std::unique_ptr<AudioRendererMixerManager> AudioRendererMixerManager::Create() {
+  return base::WrapUnique(
+      new AudioRendererMixerManager(AudioRendererSinkCache::CreateDefault()));
+}
+
+AudioRendererMixerManager::AudioRendererMixerManager(
+    std::unique_ptr<AudioRendererSinkCache> sink_cache)
+    : sink_cache_(std::move(sink_cache)) {}

[miu, 2016/05/12 21:53:06]

nit: Looks like all the code assumes |sink_cache_| is never nullptr. Please add
a DCHECK(sink_cache_) in the ctor body (self-documenting code FTW!).

[o1ka, 2016/05/17 17:17:23]

Done.

 
 AudioRendererMixerManager::~AudioRendererMixerManager() {
   // References to AudioRendererMixers may be owned by garbage collected
@@ -37,10 +46,13 @@ media::AudioRendererMixerInput* AudioRendererMixerManager::CreateInput(
                  source_render_frame_id),
       base::Bind(&AudioRendererMixerManager::RemoveMixer,
                  base::Unretained(this), source_render_frame_id),
+      base::Bind(&AudioRendererMixerManager::GetOutputDeviceInfo,
+                 base::Unretained(this), source_render_frame_id, 0 /*sink id*/),
       media::AudioDeviceDescription::UseSessionIdToSelectDevice(session_id,
                                                                 device_id)
-          ? AudioDeviceFactory::GetOutputDeviceInfo(
-                source_render_frame_id, session_id, device_id, security_origin)
+          ? sink_cache_

[chcunningham, 2016/05/12 19:56:31]

Can you make this call AudioRendererMixerManager::GetOutputDeviceInfo?

[o1ka, 2016/05/17 17:17:23]

Done.

+                ->GetSinkInfo(source_render_frame_id, session_id, device_id,
+                              security_origin)
                 .device_id()
           : device_id,
       security_origin);
@@ -68,15 +80,14 @@ media::AudioRendererMixer* AudioRendererMixerManager::GetMixer(
     return it->second.mixer;
   }
 
-  scoped_refptr<media::AudioRendererSink> sink =
-      AudioDeviceFactory::NewAudioRendererMixerSink(source_render_frame_id, 0,
-                                                    device_id, security_origin);
+  media::AudioRendererSink* sink =
+      sink_cache_->GetSink(source_render_frame_id, device_id, security_origin);
 
   const media::OutputDeviceInfo& device_info = sink->GetOutputDeviceInfo();
   if (device_status)
     *device_status = device_info.device_status();
   if (device_info.device_status() != media::OUTPUT_DEVICE_STATUS_OK) {
-    sink->Stop();
+    sink_cache_->ReleaseSink(sink);
     return nullptr;
   }
 
@@ -107,8 +118,10 @@ media::AudioRendererMixer* AudioRendererMixerManager::GetMixer(
       sample_rate, 16, buffer_size);
   DCHECK(output_params.IsValid());
 
-  media::AudioRendererMixer* mixer =
-      new media::AudioRendererMixer(output_params, sink);
+  // |sink_cache_| will outlive any mixers, so it's fine to use Unretained().

[chcunningham, 2016/05/12 19:56:31]

I'm not sure sink_cache_ really will outlive mixers_. sink_cache_ gets deleted
with this class, but the destructor calls out that mixers_ may leak (be non
empty) as not all reference holders will have released their reference. 

[o1ka, 2016/05/17 17:17:23]

I made some changes around that, mostly because if cache returns scoped_refptr
instead of a raw pointer, we can use it to get sinks for all the rendering
clients, not only mixers. I also made mixer manager to notify the cache to
release mixer's sink, because mixer manager is the only one who destroys mixers.
Now object lifetime requirements are the same as in the original code.

+  media::AudioRendererMixer* mixer = new media::AudioRendererMixer(
+      output_params, sink, base::Bind(&AudioRendererSinkCache::ReleaseSink,
+                                      base::Unretained(sink_cache_.get())));
   AudioRendererMixerReference mixer_reference = { mixer, 1 };
   mixers_[key] = mixer_reference;
   return mixer;
@@ -134,6 +147,15 @@ void AudioRendererMixerManager::RemoveMixer(
   }
 }
 
+media::OutputDeviceInfo AudioRendererMixerManager::GetOutputDeviceInfo(
+    int source_render_frame_id,
+    int session_id,
+    const std::string& device_id,
+    const url::Origin& security_origin) {
+  return sink_cache_->GetSinkInfo(source_render_frame_id, session_id, device_id,
+                                  security_origin);
+}
+
 AudioRendererMixerManager::MixerKey::MixerKey(
     int source_render_frame_id,
     const media::AudioParameters& params,