Caching AudioOutputDevice instances in mixer manager

content/renderer/media/audio_renderer_mixer_manager.cc

 // 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.
 
 #include "content/renderer/media/audio_renderer_mixer_manager.h"
 
 #include <string>
 
 #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"
 #include "media/base/audio_renderer_mixer_input.h"
 
 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
   // objects.  During process shutdown they may be leaked, so, transitively,
   // |mixers_| may leak (i.e., may be non-empty at this time) as well.
 }
 
 media::AudioRendererMixerInput* AudioRendererMixerManager::CreateInput(
     int source_render_frame_id,
     int session_id,
     const std::string& device_id,
     const url::Origin& security_origin) {
   // base::Unretained() is safe since AudioRendererMixerManager lives on the
   // renderer thread and is destroyed on renderer thread destruction.
   return new media::AudioRendererMixerInput(
       base::Bind(&AudioRendererMixerManager::GetMixer, base::Unretained(this),
                  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);
 }
 
 media::AudioRendererMixer* AudioRendererMixerManager::GetMixer(
     int source_render_frame_id,
     const media::AudioParameters& params,
     const std::string& device_id,
     const url::Origin& security_origin,
     media::OutputDeviceStatus* device_status) {
   // Effects are not passed through to output creation, so ensure none are set.
   DCHECK_EQ(params.effects(), media::AudioParameters::NO_EFFECTS);
 
   const MixerKey key(source_render_frame_id, params, device_id,
                      security_origin);
   base::AutoLock auto_lock(mixers_lock_);
 
   AudioRendererMixerMap::iterator it = mixers_.find(key);
   if (it != mixers_.end()) {
     if (device_status)
       *device_status = media::OUTPUT_DEVICE_STATUS_OK;
 
     it->second.ref_count++;
     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;
   }
 
   // On ChromeOS as well as when a fake device is used, we can rely on the
   // playback device to handle resampling, so don't waste cycles on it here.
   int sample_rate = params.sample_rate();
   int buffer_size =
       media::AudioHardwareConfig::GetHighLatencyBufferSize(sample_rate, 0);
 
 #if !defined(OS_CHROMEOS)
@@ skipping 10 matching lines @@
 #endif
 
   // Create output parameters based on the audio hardware configuration for
   // passing on to the output sink.  Force to 16-bit output for now since we
   // know that works everywhere; ChromeOS does not support other bit depths.
   media::AudioParameters output_params(
       media::AudioParameters::AUDIO_PCM_LOW_LATENCY, params.channel_layout(),
       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;
 }
 
 void AudioRendererMixerManager::RemoveMixer(
     int source_render_frame_id,
     const media::AudioParameters& params,
     const std::string& device_id,
     const url::Origin& security_origin) {
   const MixerKey key(source_render_frame_id, params, device_id,
                      security_origin);
   base::AutoLock auto_lock(mixers_lock_);
 
   AudioRendererMixerMap::iterator it = mixers_.find(key);
   DCHECK(it != mixers_.end());
 
   // Only remove the mixer if AudioRendererMixerManager is the last owner.
   it->second.ref_count--;
   if (it->second.ref_count == 0) {
     delete it->second.mixer;
     mixers_.erase(it);
   }
 }
 
+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,
     const std::string& device_id,
     const url::Origin& security_origin)
     : source_render_frame_id(source_render_frame_id),
       params(params),
       device_id(device_id),
       security_origin(security_origin) {}
 
 AudioRendererMixerManager::MixerKey::MixerKey(const MixerKey& other) = default;
 
 }  // namespace content