Caching AudioOutputDevice instances in mixer manager

content/renderer/media/audio_renderer_sink_cache_impl.cc

+// Copyright 2016 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 "base/bind.h"
+#include "base/location.h"
+#include "base/memory/ptr_util.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "content/renderer/media/audio_device_factory.h"
+#include "content/renderer/media/audio_renderer_sink_cache_impl.h"
+#include "url/origin.h"
+
+namespace {
+enum { kDeleteTimeoutMs = 5000 };

[miu, 2016/05/19 22:27:15]

As of a few weeks ago, they've just started allowing the use of constexpr in
Chromium code. This can now become:

  constexpr int kDeleteTimeoutMs = 5000;

(and no need for anonymous namespace here since constexpr's are not visible
outside the module)

BTW--In the near future, we're going to try to make base::TimeDelta and friends
constexpr literals so eventually this LOC could be simply:

  constexpr base::TimeDelta kDeleteTimeout = ...;

[o1ka, 2016/05/23 16:16:54]

Thanks. Done.

+}  // namespace
+
+namespace content {
+
+// static
+std::unique_ptr<AudioRendererSinkCache> AudioRendererSinkCache::Create() {
+  return base::WrapUnique(new AudioRendererSinkCacheImpl(
+      base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(AudioDeviceFactory::NewAudioRendererMixerSink),
+      kDeleteTimeoutMs));
+}
+
+AudioRendererSinkCacheImpl::AudioRendererSinkCacheImpl(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    const CreateSinkCallback& create_sink_cb,
+    int delete_timeout_ms)
+    : delete_timeout_ms_(delete_timeout_ms),

[miu, 2016/05/19 22:27:15]

nit: For consistency/readability, try to maintain the same order of ctor args
with data members.

[o1ka, 2016/05/23 16:16:54]

Done.

+      task_runner_(task_runner),
+      create_sink_cb_(create_sink_cb),
+      weak_ptr_factory_(this) {}
+
+AudioRendererSinkCacheImpl::~AudioRendererSinkCacheImpl() {
+  // We just release all the cached sinks here.
+}
+
+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() << "] ";
+
+    const SinkKey key(source_render_frame_id, device_info.device_id(),
+                      security_origin);
+
+    base::AutoLock auto_lock(sinks_lock_);

[miu, 2016/05/19 22:27:15]

Try to minimize how long this lock is held (here and elsewhere in this file).
For example, it seems the code here should be:

  {
    base::AutoLock auto_lock(sinks_lock_);
    sinks_.insert(...);
  }
  DeleteLaterIfUnused(key, sink.get());
  return device_info;

Actually, the lines above are almost identical to the other code path for this
method. Suggested refactoring for this method:

{
  SinkKey key;
  scoped_refptr<media::AudioRendererSink> sink;
  if (media::AudioDeviceDescription::UseSessionIdToSelectDevice(...)) {
    const media::OutputDeviceInfo& device_info = sink->GetOutputDeviceInfo();
    key = SinkKey(source_render_frame_id, device_info.device_id(),
security_origin);
    sink = create_sink_cb_.Run(...);
  } else {
    key = SinkKey(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())
        return sink_iter->second.sink->GetOutputDeviceInfo();
    }

    // No matching sink is found, create one and cache it.
    sink = create_sink_cb_.Run(...);
  }

  {
    base::AutoLock auto_lock(sinks_lock_);
    sinks_.insert(std::make_pair(
        key, AudioRendererSinkReference(sink, false /*not in use*/)));
  }
  DeleteLaterIfUnused(key, sink.get());
  return sink->GetOutputDeviceInfo();
}

[o1ka, 2016/05/23 16:16:54]

Done.

[DaleCurtis, 2016/05/23 18:29:08]

I don't think the original comment is good advice. You shouldn't cycle the lock
within a single function unless you like unexpected behavior. :) I'm fine with
scoping the lock when you only have one thing to do, but you shouldn't take the
lock multiple times within a single function without a good reason.

+    // Cache a newly-created sink, using device id obtained from the sink.
+    sinks_.insert(std::make_pair(
+        key, AudioRendererSinkReference(sink, false /*not in use*/)));
+    // And schedule it for deletion.
+    DeleteLaterIfUnused(key, 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 InsertNewSinkWhileLockHeld().
+  scoped_refptr<media::AudioRendererSink> sink =
+      InsertNewSinkWhileLockHeld(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(key, sink.get());
+  return sink->GetOutputDeviceInfo();
+}
+
+scoped_refptr<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)
+      continue;
+
+    // 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;
+  }
+
+  // No unused sink is found, create one, mark it used, cache it and return.
+  scoped_refptr<media::AudioRendererSink> sink =
+      InsertNewSinkWhileLockHeld(key, true);
+  DVLOG(1) << "GetSink: address: " << sink.get()
+           << " - 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(
+    int source_render_frame_id,
+    const std::string& device_id,
+    const url::Origin& security_origin,
+    const media::AudioRendererSink* sink) {
+  // We don't know the sink state, so won't reused it. Delete it immediately.

[miu, 2016/05/19 22:27:15]

typo: s/reused/reuse/

[o1ka, 2016/05/23 16:16:54]

Done.

+  DeleteSink(SinkKey(source_render_frame_id, device_id, security_origin), sink,
+             true);
+}
+
+void AudioRendererSinkCacheImpl::DeleteLaterIfUnused(
+    const SinkKey& key,
+    const media::AudioRendererSink* sink) {
+  DVLOG(1) << "DeleteLaterIfUnused: address: " << sink;
+
+  task_runner_->PostDelayedTask(
+      FROM_HERE, base::Bind(&AudioRendererSinkCacheImpl::DeleteSink,
+                            weak_ptr_factory_.GetWeakPtr(), key, sink,
+                            false /*do not delete if used*/),
+      base::TimeDelta::FromMilliseconds(delete_timeout_ms_));
+}
+
+void AudioRendererSinkCacheImpl::DeleteSink(
+    const SinkKey& key,
+    const media::AudioRendererSink* sink,
+    bool force_delete_used) {
+  DCHECK(sink);
+  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.sink.get() != sink)
+      continue;
+
+    const 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: " << key.source_render_frame_id
+             << " device_id: " << key.device_id
+             << " security_origin: " << key.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.
+      if (!sink_ref.used)
+        sink_ref.sink->Stop();  // Stop the sink before deletion if not used.
+      sinks_.erase(iter);
+      DVLOG(1) << "DeleteSink: address: " << sink << " deleted. ";
+    } else {
+      DVLOG(1) << "DeleteSink: address: " << sink
+               << " sink in use, skipping deletion.";
+    }
+
+    return;
+  }  // for
+
+  // If we got here and |force_delete_used| is not set it means the sink
+  // scheduled for deletion get aquired and released before scheduled deletion -
+  // this is ok.
+  DCHECK(!force_delete_used) << "DeleteSink: address: " << sink
+                             << " could not find a sink which is supposed to be"
+                             << " in use. Wrong key? source_render_frame_id: "
+                             << key.source_render_frame_id
+                             << " device_id: " << key.device_id
+                             << " security_origin: " << key.security_origin;
+}
+
+scoped_refptr<media::AudioRendererSink>
+AudioRendererSinkCacheImpl::InsertNewSinkWhileLockHeld(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;
+}
+
+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