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 <algorithm>
+
+#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 "media/audio/audio_device_description.h"
+#include "media/base/audio_renderer_sink.h"
+#include "url/origin.h"
+
+namespace content {
+
+constexpr int kDeleteTimeoutMs = 5000;
+
+// Cached sink data.
+struct AudioRendererSinkCacheImpl::CacheEntry {
+  int source_render_frame_id;
+  std::string device_id;
+  url::Origin security_origin;
+  scoped_refptr<media::AudioRendererSink> sink;  // Sink instance
+  bool used;                                     // True if in use by a client.
+};
+
+// Helper class for CacheEntry lookup.
+class CacheEntryFinder {
+ public:
+  CacheEntryFinder(int source_render_frame_id,
+                   const std::string& device_id,
+                   const url::Origin& security_origin,
+                   bool unused_only)
+      : source_render_frame_id_(source_render_frame_id),
+        device_id_(device_id),
+        security_origin_(security_origin),
+        unused_only_(unused_only) {}
+
+  bool operator()(const AudioRendererSinkCacheImpl::CacheEntry& s) const {
+    if (s.used && unused_only_)
+      return false;
+
+    if (s.source_render_frame_id != source_render_frame_id_)
+      return false;
+
+    if (media::AudioDeviceDescription::IsDefaultDevice(device_id_) &&
+        media::AudioDeviceDescription::IsDefaultDevice(s.device_id)) {
+      // Both device IDs represent the same default device => do not compare
+      // them; the default device is always authorized => ignoring security
+      // origin.
+      return true;
+    }
+
+    return (s.device_id == device_id_) &&

[DaleCurtis, 2016/05/24 20:41:42]

Avoid unnecessary parens.

[o1ka, 2016/05/25 12:20:52]

Done.
This is something interesting though - I haven't found any recommendations in
Google/Chromium guidelines for this, and in other style guides I've worked with
parentheses are usually recommended.
Should it probably be added to the style guide?

+           (s.security_origin == security_origin_);
+  }
+
+ private:
+  int source_render_frame_id_;

[DaleCurtis, 2016/05/24 20:41:42]

const all these

[o1ka, 2016/05/25 12:20:52]

Done. (And then I removed the code)

+  std::string device_id_;
+  url::Origin security_origin_;
+  bool unused_only_;
+};

[DaleCurtis, 2016/05/24 20:41:42]

DISALLOW_COPY_AND_ASSIGN

[o1ka, 2016/05/25 12:20:52]

It's copyable (and needs to be so for find_if), so I should have declared the
default copy constructor. But now when I think of all that copying happening
here I don't like it any more :) lambda it will be!

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

[DaleCurtis, 2016/05/24 20:41:42]

Pass base::TimeDelta by value (see time.h)

[o1ka, 2016/05/25 12:20:52]

Done.

+    : task_runner_(std::move(task_runner)),
+      create_sink_cb_(create_sink_cb),
+      delete_timeout_(delete_timeout),
+      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) {
+  CacheEntry cache_entry;

[DaleCurtis, 2016/05/24 20:41:42]

Worth adding a constructor to CacheEntry with some default values?

[miu, 2016/05/25 01:23:20]

IIRC, the uniform initializer syntax is now approved for Chromium code. Instead
of coding a constructor, you can initialize structs directly. For example:

  CacheEntry cache_entry{ source_render_frame_id, std::string(),
security_origin, nullptr, false };

[o1ka, 2016/05/25 12:20:52]

True, but assignment syntax is recommended for this case, and uniform
initializer is to be used only if neither this nor constructor works
(https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...).
So I went for assignment.

+  cache_entry.source_render_frame_id = source_render_frame_id;
+  cache_entry.security_origin = security_origin;
+  cache_entry.used = false;
+
+  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.
+    cache_entry.sink = create_sink_cb_.Run(source_render_frame_id, session_id,
+                                           device_id, security_origin);
+    cache_entry.device_id = cache_entry.sink->GetOutputDeviceInfo().device_id();
+
+    DVLOG(1) << "GetSinkInfo: address: " << cache_entry.sink.get()
+             << " - used session to create new sink.";
+
+    // Cache a newly-created sink.
+    base::AutoLock auto_lock(cache_lock_);
+    cache_.push_back(cache_entry);
+
+  } else {
+    // Ignore session id.
+    base::AutoLock auto_lock(cache_lock_);
+
+    auto cache_iter = std::find_if(
+        cache_.begin(), cache_.end(),
+        CacheEntryFinder(source_render_frame_id, device_id, security_origin,
+                         false /* unused_only */));
+
+    if (cache_iter != cache_.end()) {
+      // A matching cached sink is found.
+      DVLOG(1)
+          << "GetSinkInfo: address: " << cache_iter->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: ["
+          << cache_iter->sink->GetOutputDeviceInfo().AsHumanReadableString()
+          << "] ";
+
+      return cache_iter->sink->GetOutputDeviceInfo();
+    }  // if (cache_iter != cache_.end())

[DaleCurtis, 2016/05/24 20:41:42]

It's not chrome style to annotate trailing } in conditional blocks.

[o1ka, 2016/05/25 12:20:52]

Done.

+
+    // No matching sink found, create a new one.
+    cache_entry.device_id = device_id;
+    cache_entry.sink = create_sink_cb_.Run(
+        source_render_frame_id, 0 /* session_id */, device_id, security_origin);
+
+    DVLOG(1) << "GetSinkInfo: address: " << cache_entry.sink.get()
+             << " - no matching cached sink found, created a new one.";
+
+    // Cache a newly-created sink.
+    cache_.push_back(cache_entry);
+  }  // else
+
+  // Schedule it for deletion.
+  DeleteLaterIfUnused(cache_entry.sink.get());
+
+  DVLOG(1) << "GetSinkInfo: address: " << cache_entry.sink.get()
+           << " inserted. source_render_frame_id: " << source_render_frame_id
+           << " session_id: " << session_id << " device_id: " << device_id
+           << " security_origin: " << security_origin << " Device info: ["
+           << cache_entry.sink->GetOutputDeviceInfo().AsHumanReadableString()
+           << "] ";
+
+  return cache_entry.sink->GetOutputDeviceInfo();
+}
+
+scoped_refptr<media::AudioRendererSink> AudioRendererSinkCacheImpl::GetSink(
+    int source_render_frame_id,
+    const std::string& device_id,
+    const url::Origin& security_origin) {
+  base::AutoLock auto_lock(cache_lock_);
+
+  auto cache_iter =
+      std::find_if(cache_.begin(), cache_.end(),
+                   CacheEntryFinder(source_render_frame_id, device_id,
+                                    security_origin, true /* unused_only */));
+
+  if (cache_iter != cache_.end()) {
+    // Found unused sink; mark it as used and return.
+    DVLOG(1) << "GetSink: address: " << cache_iter->sink.get()
+             << " - found unused cached sink, reusing it."
+             << " source_render_frame_id: " << source_render_frame_id
+             << " device_id: " << device_id
+             << " security_origin: " << security_origin;
+
+    cache_iter->used = true;
+    return cache_iter->sink;
+  }  // if
+
+  // No unused sink is found, create one, mark it used, cache it and return.
+  CacheEntry cache_entry;

[miu, 2016/05/25 01:23:20]

ditto here: You could just use the uniform initializer syntax.

[o1ka, 2016/05/25 12:20:52]

Done, same as above.

+  cache_entry.source_render_frame_id = source_render_frame_id;
+  cache_entry.device_id = device_id;
+  cache_entry.security_origin = security_origin;
+  cache_entry.used = true;
+
+  cache_entry.sink = create_sink_cb_.Run(
+      source_render_frame_id, 0 /* session_id */, device_id, security_origin);
+
+  cache_.push_back(cache_entry);
+
+  DVLOG(1) << "GetSink: address: " << cache_entry.sink.get()

[DaleCurtis, 2016/05/24 20:41:42]

I'd drop some of these DVLOGs from the production version, but up to you.

[o1ka, 2016/05/25 12:20:52]

Yes, they hurt readability. Reduced a bit.

+           << " - 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 cache_entry.sink;
+}
+
+void AudioRendererSinkCacheImpl::ReleaseSink(
+    const media::AudioRendererSink* sink_ptr) {
+  // We don't know the sink state, so won't reuse it. Delete it immediately.

[DaleCurtis, 2016/05/24 20:41:42]

Aren't we always taking this path when tearing down a sink? Is it worth any
effort to try and reuse these?

[o1ka, 2016/05/25 12:20:52]

Do you mean reusing a sink after it was returned by a client? We can't do it as
of now, since AODs are not reinitializable until grunell@'s CL is landed. We can
look into it later (that will need some care to make sure we are not reusing
stale sinks, so we'll keep them for some short period of time). I'm not sure if
it will be very beneficial though, because chances that such a sink will be
requested for reuse seem to be low. I'm going to add some UMA stats to see what
sink reuse looks like.

So now we are reusing only sinks created to retrieve OutputDeviceInfo, which is
a typical scenario: the client requests output device parameters (sink created),
initializes rendering pipeline basing on those and then requests and initializes
a sink (sink reused).

+  DeleteSink(sink_ptr, true);
+}
+
+void AudioRendererSinkCacheImpl::DeleteLaterIfUnused(
+    const media::AudioRendererSink* sink_ptr) {
+  DVLOG(1) << "DeleteLaterIfUnused: address: " << sink_ptr;
+
+  task_runner_->PostDelayedTask(
+      FROM_HERE, base::Bind(&AudioRendererSinkCacheImpl::DeleteSink,
+                            weak_ptr_factory_.GetWeakPtr(), sink_ptr,
+                            false /*do not delete if used*/),
+      delete_timeout_);
+}
+
+void AudioRendererSinkCacheImpl::DeleteSink(
+    const media::AudioRendererSink* sink_ptr,
+    bool force_delete_used) {
+  DCHECK(sink_ptr);
+
+  scoped_refptr<media::AudioRendererSink> sink_to_stop;
+
+  {
+    base::AutoLock auto_lock(cache_lock_);
+
+    // Looking up the sink by its pointer.
+    auto cache_iter = std::find_if(cache_.begin(), cache_.end(),
+                                   [&sink_ptr](const CacheEntry& val) {
+                                     return val.sink.get() == sink_ptr;
+                                   });
+
+    if (cache_iter == cache_.end()) {
+      // 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_ptr
+          << " could not find a sink which is supposed to be in use";
+
+      DVLOG(1) << "DeleteSink: address: " << sink_ptr
+               << " force_delete_used = true - already deleted.";
+      return;
+    }
+
+    DVLOG(1) << "DeleteSink: address: " << sink_ptr
+             << " force_delete_used: " << force_delete_used
+             << " in use: " << cache_iter->used << " source_render_frame_id: "
+             << cache_iter->source_render_frame_id
+             << " device_id: " << cache_iter->device_id
+             << " security_origin: " << cache_iter->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 && cache_iter->used))
+        << "Attempt to delete a non-aquired sink.";
+
+    if (!force_delete_used && cache_iter->used) {
+      DVLOG(1) << "DeleteSink: address: " << sink_ptr
+               << " sink in use, skipping deletion.";
+      return;
+    }
+
+    // To stop the sink before deletion if it's not used, we need to hold
+    // a ref to it.
+    if (!cache_iter->used)
+      sink_to_stop = cache_iter->sink;
+
+    cache_.erase(cache_iter);
+    DVLOG(1) << "DeleteSink: address: " << sink_ptr;
+  }  // Lock scope;
+
+  // Stop the sink out of the lock scope.
+  if (sink_to_stop.get()) {
+    DCHECK_EQ(sink_ptr, sink_to_stop.get());
+    sink_to_stop->Stop();
+    DVLOG(1) << "DeleteSink: address: " << sink_ptr << " stopped.";
+  }
+}
+
+int AudioRendererSinkCacheImpl::GetCacheSizeForTesting() {
+  return cache_.size();
+}
+
+}  // namespace content