Caching AudioOutputDevice instances in mixer manager

content/renderer/media/audio_renderer_sink_cache.cc

Index: content/renderer/media/audio_renderer_sink_cache.cc
diff --git a/content/renderer/media/audio_renderer_sink_cache.cc b/content/renderer/media/audio_renderer_sink_cache.cc
new file mode 100644
index 0000000000000000000000000000000000000000..87304f1df1d63e7a176395472426f06492f1e651
--- /dev/null
+++ b/content/renderer/media/audio_renderer_sink_cache.cc
@@ -0,0 +1,225 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

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

To be consistent with how this is done for other impl classes in Chromium code,
this file should be named audio_renderer_sink_cache_impl.cc. It contains the
entire Impl class implementation (and it's okay for it to also contain
AudioRendererSinkCache::CreateDefault()). This is similar to how RenderFrame vs.
RenderFrameImpl code is organized.

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

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/memory/ptr_util.h"
+#include "base/synchronization/lock.h"
+#include "base/thread_task_runner_handle.h"
+#include "content/renderer/media/audio_device_factory.h"
+#include "content/renderer/media/audio_renderer_sink_cache_impl.h"
+
+namespace content {
+
+// static
+std::unique_ptr<AudioRendererSinkCache>
+AudioRendererSinkCache::CreateDefault() {

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

You named MixerManager's method ::Create() (no Default) - should these names
match?

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

Done.

+  return base::WrapUnique(new AudioRendererSinkCacheImpl(
+      base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(AudioDeviceFactory::NewAudioRendererMixerSink)));
+}
+
+AudioRendererSinkCacheImpl::AudioRendererSinkCacheImpl(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    const CreateSinkCallback& create_sink_cb)
+    : task_runner_(task_runner),
+      create_sink_cb_(create_sink_cb),
+      weak_ptr_factory_(this) {}
+
+AudioRendererSinkCacheImpl::~AudioRendererSinkCacheImpl() {
+  for (auto& iter : sinks_)

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

Consider (for safety):

  sinks_lock_.AssertNotHeld();

Actually, is a lock necessary? Should this class be thread-safe? I see that
AudioRenderMixerManager does some locking too, but its public methods all seem
to be invoked on the main renderer thread as well.

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

AFAIK there is no AssertNotHeld() in chromium, only Try(), which behavior is
undefined if called on a thread holding the lock (
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...)

Re: thread-safety: Unfortunately not, as well as mixers. Mixer inputs (and
mixers and their sinks, as a result) are accessed from
WebAudioSourceProviderImpl, which methods run on both media thread (invoked by
audio renderer pipeline) and main thread (invoked by HTMLElement for output
client switch). This is something which needs fixing for sure, but is a bit out
of scope of our "mix all" effort.

+    iter.second.sink->Stop();
+}
+
+media::OutputDeviceInfo AudioRendererSinkCacheImpl::GetSinkInfo(
+    int source_render_frame_id,
+    int session_id,
+    const std::string& device_id,
+    const url::Origin& security_origin) {
+  if (media::AudioDeviceDescription::UseSessionIdToSelectDevice(session_id,
+                                                                device_id)) {
+    // We are provided with session id instead of device id. Session id is
+    // unique, so we can't find any matching sink. Creating a new one.
+    scoped_refptr<media::AudioRendererSink> sink = create_sink_cb_.Run(
+        source_render_frame_id, session_id, device_id, security_origin);
+
+    const media::OutputDeviceInfo& device_info = sink->GetOutputDeviceInfo();
+    DVLOG(1) << "GetSinkInfo: address: " << sink.get()
+             << " - used session to create new sink. source_render_frame_id: "
+             << source_render_frame_id << " session_id: " << session_id
+             << " device_id: " << device_id
+             << " security_origin: " << security_origin << " Device info: ["
+             << device_info.AsHumanReadableString() << "] ";
+
+    base::AutoLock auto_lock(sinks_lock_);
+    // Cache a newly-created sink, using device id obtained from the sink.
+    sinks_.insert(
+        std::make_pair(SinkKey(source_render_frame_id, device_info.device_id(),

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

Does session_id need to be in the SinkKey?

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

No. Session_id is a mapping between an open input device to an associated output
device and is unique for an input device session. The actual device id is
returned by browser side on AOD creation, and we use that as a part of the key.
This allows for better reuse of sinks.

+                               security_origin),
+                       AudioRendererSinkReference(sink, false /*not in use*/)));
+    // And schedule it for deletion.
+    DeleteLaterIfUnused(sink.get());
+    return device_info;
+  }
+
+  // We are provided with the actual device id.
+  const SinkKey key(source_render_frame_id, device_id, security_origin);
+  base::AutoLock auto_lock(sinks_lock_);
+
+  {
+    auto sink_iter = sinks_.find(key);
+    if (sink_iter != sinks_.end()) {
+      // A matching cached sink is found.
+      DVLOG(1) << "GetSinkInfo: address: " << sink_iter->second.sink.get()
+               << " - reusing a cached sink. source_render_frame_id: "
+               << source_render_frame_id << " session_id: " << session_id
+               << " device_id: " << device_id
+               << " security_origin: " << security_origin << " Device info: ["
+               << sink_iter->second.sink->GetOutputDeviceInfo()
+                      .AsHumanReadableString()
+               << "] ";
+      return sink_iter->second.sink->GetOutputDeviceInfo();
+    }
+  }
+
+  // No matching sink is found, create one and cache it - still under the lock,
+  // see the comment to InsertNewSink().
+  media::AudioRendererSink* sink = InsertNewSink(key, false /*not in use*/);
+  DVLOG(1) << "GetSinkInfo: address: " << sink
+           << " - no matching cached sink found, created a new one."
+           << " source_render_frame_id: " << source_render_frame_id
+           << " session_id: " << session_id << " device_id: " << device_id
+           << " security_origin: " << security_origin << " Device info: ["
+           << sink->GetOutputDeviceInfo().AsHumanReadableString() << "] ";
+  // Schedule it for deletion.
+  DeleteLaterIfUnused(sink);
+  return sink->GetOutputDeviceInfo();
+}
+
+media::AudioRendererSink* AudioRendererSinkCacheImpl::GetSink(
+    int source_render_frame_id,
+    const std::string& device_id,
+    const url::Origin& security_origin) {
+  const SinkKey key(source_render_frame_id, device_id, security_origin);
+  base::AutoLock auto_lock(sinks_lock_);
+
+  const auto range = sinks_.equal_range(key);
+  for (auto iter = range.first; iter != range.second; ++iter) {
+    if (!iter->second.used) {
+      // Found unused sink; mark it as used and return.
+      DVLOG(1) << "GetSink: address: " << iter->second.sink.get()
+               << " - found unused cached sink, reusing it."
+               << " source_render_frame_id: " << source_render_frame_id
+               << " device_id: " << device_id
+               << " security_origin: " << security_origin;
+
+      iter->second.used = true;
+      return iter->second.sink.get();
+    }
+  }
+
+  // No unused sink is found, create one, mark it used, cache it and return.
+  media::AudioRendererSink* sink = InsertNewSink(key, true);

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

Should there be an upper-bound, where we decide too many sinks have been created
without enough of them being released?

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

No, not for now at least. Temporary sinks created for GetOutputDeviceInfo() are
deleted if not reused after a timeout; and we can't prevent creating new sinks
(+AudioOutputDevices) requested by clients. There is no any limit for a number
of AOD now (before caching), and we don't want to introduce it at this point.
But agree that caching does enable a possibility to track and limit that number
and we may want to introduce a limit on some platforms at some point.

+  DVLOG(1) << "GetSink: address: " << sink
+           << " - no unused cached sink found, created a new one."
+           << " source_render_frame_id: " << source_render_frame_id
+           << " device_id: " << device_id
+           << " security_origin: " << security_origin;
+  return sink;
+}
+
+void AudioRendererSinkCacheImpl::ReleaseSink(media::AudioRendererSink* sink) {
+  // We don't know the sink state, so won't reused it. Delete it immediately.
+  DeleteSink(sink, true);
+}
+
+void AudioRendererSinkCacheImpl::DeleteLaterIfUnused(
+    media::AudioRendererSink* sink) {
+  DVLOG(1) << "DeleteLaterIfUnused: address: " << sink;
+
+  task_runner_->PostDelayedTask(
+      FROM_HERE, base::Bind(&AudioRendererSinkCacheImpl::DeleteSink,
+                            weak_ptr_factory_.GetWeakPtr(), sink,
+                            false /*do not delete if used*/),
+      base::TimeDelta::FromMilliseconds(kDeleteTimeoutMs));
+}
+
+void AudioRendererSinkCacheImpl::DeleteSink(media::AudioRendererSink* sink,
+                                            bool force_delete_used) {
+  DCHECK(sink);
+  base::AutoLock auto_lock(sinks_lock_);
+
+  // The sink key is not used for look up, to keep the interface simple:
+  // otherwise both the key and the sink pointer needs to be specified, since
+  // the key is not unique. Looking up by the key won't give any significant
+  // performance improvement, since the number of sinks is small.
+  for (auto iter = sinks_.begin(); iter != sinks_.end(); ++iter) {
+    if (iter->second.sink.get() == sink) {  // Sink is found.

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

nit: Invert the if-statement expression so the ~25 LOC don't need to be
indented.

Meaning:

    if (it->second.sink != sink)
      continue;
    ...rest of code as-is...

There are a few other places in this change where you might consider doing this
too...

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

Done. (I hope I did not miss anything)

+      AudioRendererSinkReference& sink_ref(iter->second);
+
+      DVLOG(1) << "DeleteSink: address: " << sink
+               << " force_delete_used: " << force_delete_used
+               << " in use: " << sink_ref.used << " source_render_frame_id: "
+               << iter->first.source_render_frame_id
+               << " device_id: " << iter->first.device_id
+               << " security_origin: " << iter->first.security_origin;
+
+      // When |force_delete_used| is set, it's expected that we are deleting a
+      // used sink.
+      DCHECK((!force_delete_used) || (force_delete_used && sink_ref.used))
+          << "Attempt to delete a non-aquired sink.";
+
+      if (force_delete_used || !sink_ref.used) {  // We can delete it.
+        sink_ref.sink->Stop();  // Sink must be stopped before deletion.
+        sinks_.erase(iter);
+        DVLOG(1) << "DeleteSink: address: " << sink << " deleted. ";
+      } else {
+        DVLOG(1) << "DeleteSink: address: " << sink
+                 << " sink in use, skipping deletion.";
+      }
+
+      return;
+    }
+  }
+
+  // If we get here and |force_delete_used| is not set it means the sink
+  // scheduled for deletion get aquired and released before scheduled deletion -

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

s/get/got/

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

Done.

+  // this is ok.
+  DCHECK(!force_delete_used) << "Attempt to delete non-existing sink.";
+}
+
+media::AudioRendererSink* AudioRendererSinkCacheImpl::InsertNewSink(

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

I'm used to seeing such methods named *_Locked - not sure if this is a chrome
thing or just a media thing. I like that style if you're ok with it. 

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

I've also seen people name methods like this: InsertNewSinkWhileLockHeld()

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

Done.

+    const SinkKey& key,
+    bool used) {
+  sinks_lock_.AssertAcquired();
+  const AudioRendererSinkMap::iterator& iter = sinks_.insert(std::make_pair(
+      key, AudioRendererSinkReference(
+               create_sink_cb_.Run(key.source_render_frame_id, 0, key.device_id,
+                                   key.security_origin),
+               used)));
+  return iter->second.sink.get();
+}
+
+AudioRendererSinkCacheImpl::SinkKey::SinkKey(int source_render_frame_id,
+                                             const std::string& device_id,
+                                             const url::Origin& security_origin)
+    : source_render_frame_id(source_render_frame_id),
+      device_id(device_id),
+      security_origin(security_origin) {}
+
+AudioRendererSinkCacheImpl::SinkKey::SinkKey(const SinkKey& other) = default;
+
+AudioRendererSinkCacheImpl::AudioRendererSinkReference::
+    AudioRendererSinkReference(scoped_refptr<media::AudioRendererSink> sink,
+                               bool used)
+    : sink(sink), used(used) {}
+
+AudioRendererSinkCacheImpl::AudioRendererSinkReference::
+    AudioRendererSinkReference(const AudioRendererSinkReference& other) =
+        default;
+
+AudioRendererSinkCacheImpl::AudioRendererSinkReference::
+    ~AudioRendererSinkReference() {}
+
+}  // namespace content