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 {
+constexpr int kDeleteTimeoutMs = 5000;
+}  // namespace
+
+namespace content {
+
+// 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.

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

Why choose a helper class vs a lambda?

[o1ka, 2016/05/23 19:21:34]

It's used in two places and requires a quite lot of capture.

[DaleCurtis, 2016/05/23 20:20:26]

Hmm, looks like my original comment got eaten. I changed this to be: "Why not a
helper function like CacheEntry::iterator FindCacheEntry_Locked(...) { return
std::find_if(..., [] {...} }" I think it'll end up being a lot less boiler plate
as well as make the the calling functions clearer.

[o1ka, 2016/05/24 15:00:41]

Yes, this is another way to implement it. I don't sink that 17-line lambda in
std::find_if(..., [] {...}) is better than the current version, it's just
different. And I don't see a big difference in
std::find_if(cache_.begin(), cache_.end(),
          CacheEntryFinder(source_render_frame_id, device_id, security_origin,
                           false /* unused_only */));
vs
FindCacheEntry_Locked(source_render_frame_id, device_id, security_origin,
                           false /* unused_only */)
To me they are equally verbose.
I would not mind any of them when reading such a code (though I would feel like
lambdas are a bit overused :)).

+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_) &&
+           (s.security_origin == security_origin_);
+  }
+
+ private:
+  int source_render_frame_id_;
+  std::string device_id_;
+  url::Origin security_origin_;
+  bool unused_only_;
+};
+
+// 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)
+    : task_runner_(task_runner),

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

Isn't std::move necessary now whenever you pass-by-value?

[o1ka, 2016/05/24 15:00:41]

Done.

+      create_sink_cb_(create_sink_cb),
+      delete_timeout_ms_(delete_timeout_ms),

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

store as timedelta, FromMilliseconds(...) 

[o1ka, 2016/05/24 15:00:41]

Done.

+      weak_ptr_factory_(this) {}

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

If you take my suggestion below to use a RepeatingTimer instead for scrubbing,
you can drop the weak factory since it'll internally handle that.

[o1ka, 2016/05/24 15:00:41]

This is a nice suggestion. The advantage of the current code is that it's
straightforward => less opportunities to have bugs. I would prefer to leave it
as it is now, but keep in mind this approach as a possible optimization to be
done if required.

+
+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;
+  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.";
+  } else {
+    // Ignore session id.
+    {
+      base::AutoLock auto_lock(cache_lock_);

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

Instead of dropping in and out of lock state (something we know can be
expensive), why not grab it once at the top of the function and hold until
completion? None of these methods should be expensive and cycling the lock like
this makes me nervous about what other threads might be doing.

[o1ka, 2016/05/23 19:21:34]

Ok will do.

[o1ka, 2016/05/24 15:00:41]

Done.

+      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())
+    }    // Lock scope.
+
+    // 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.
+    base::AutoLock auto_lock(cache_lock_);
+    cache_.push_back(cache_entry);
+  }
+
+  // Schedule it for deletion.

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

Why schedule them individually for deletion vs having a repeating task that
scrubs them every x seconds based on some last_used_time_ inside the CacheEntry?

[o1ka, 2016/05/23 19:21:34]

Because it's quite a rare occasion when output parameters are requested and we
schedule the deletion. Why to have the timer always running if it may be needed
once on page load, for example? WDYT?

[o1ka, 2016/05/23 19:26:27]

(It was like that in the original draft version which you approved...)

[DaleCurtis, 2016/05/23 20:20:26]

You wouldn't run the timer the whole time. You'd only Start() it when sinks need
to be deleted. If you prefer the current approach you should at least use a
base::CancelableClosure() instead of the weak factory.

[o1ka, 2016/05/24 15:00:41]

I looked into CancelableCallback before implementing this. The problem is that
it can be created only on the same thread it's posted to/deleted on
(https://code.google.com/p/chromium/codesearch#chromium/src/base/cancelable_ca...),
wheres for weak pointer only dereference and invalidation are thread-bound
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...).
Mixer input(=>mixer) interfaces can be accessed from both media (pipeline) and
main (HTMLMediaElement) threads => DeleteLaterIfUnused can be called on one of
them. And we can't pre-create all those CancelableCallback instances for the
sinks.

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

This isn't true of WeakPtr either. You have a race right now where if there are
no WeakPtr refs outstanding and multiple threads try to create one they will
stomp on eachother. If you look at WeakPtr::GetRef() you can see that it is not
thread safe. The simplest fix is instead pre-create a weak_this_ on the right
thread that you hand out instead of using GetWeakptr(). Alternatively, make
RepeatingTimer or CancelableClosure work as suggested previously as the current
approach isn't as clear as you might think :)

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

Oh, I've had a feeling that I might be missing something here. Actually, I
should have asked you about it. Thanks for catching!

If I use RepeatingTimer of CancelableClosure => I'm not passing sink pointer to
the callback. So I mark each sink with timestamp when it is added, and in the
callback I'll go through all of them and check the expiration. When a new unused
sink is added to the cache, I set its time stamp, and then I need to reset the
timer only if it's not running. But timer API is not thread safe
(https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h...)
which is a problem here, since the code works on at least two threads.
If CancelableClosure is used, than I need to know if it's already been posted,
otherwise I'll post it for each sink (and it will be going through all sinks to
check their expiration) => I need a flag +  a lock.
So both options do not look very clean to me. Or do you have some specific idea
how to use them in a more elegant way?

[DaleCurtis, 2016/05/25 16:48:52]

The multiple threads thing isn't a problem if you use a timer. You would just
PostTask() to the right thread to start the timer (which would do nothing if
it's already running) and it would automatically stop when no unused sinks are
remaining.

For the closure you're only cancelling tasks during destruction, so you'd just
pre-bind the closure on the right thread and then post it when a deletion needs
to be scheduled.

As you've pointed out in both cases you'd skim the vector looking for unused
sinks, but that's cheap as you've benchmarked :)

[o1ka, 2016/05/25 17:00:56]

Would't I need to use a weak pointer to post that task? Am I missing something?
What is the advantage comparing to posting tasks with weak_ptr? (I think that's
in the end what CancelableClosure does).
(And disadvantage is that we need to deal with time stamps)
Yes, but it will cost a bit more since in both cases we need to do maths on time
stamps.

I still do not quite see obvious benefits this logic complication brings.

+  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_);

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

Ditto on lock once.

[o1ka, 2016/05/24 15:00:41]

Done.

+
+    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;
+    }  // for
+  }    // Lock scope.
+
+  // No unused sink is found, create one, mark it used, cache it and return.
+  CacheEntry cache_entry;
+  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);
+
+  base::AutoLock auto_lock(cache_lock_);
+  cache_.push_back(cache_entry);
+
+  DVLOG(1) << "GetSink: address: " << cache_entry.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 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.
+  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*/),
+      base::TimeDelta::FromMilliseconds(delete_timeout_ms_));
+}
+
+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