Caching AudioOutputDevice instances in mixer manager

content/renderer/media/audio_renderer_sink_cache_impl.h

Index: content/renderer/media/audio_renderer_sink_cache_impl.h
diff --git a/content/renderer/media/audio_renderer_sink_cache_impl.h b/content/renderer/media/audio_renderer_sink_cache_impl.h
new file mode 100644
index 0000000000000000000000000000000000000000..97eb75ef76f433f83848df8e6eb49bdaa9e9a313
--- /dev/null
+++ b/content/renderer/media/audio_renderer_sink_cache_impl.h
@@ -0,0 +1,133 @@
+// 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 "content/renderer/media/audio_renderer_sink_cache.h"
+
+#include <map>
+
+#include "base/single_thread_task_runner.h"
+#include "base/synchronization/lock.h"
+#include "content/common/content_export.h"
+#include "media/audio/audio_device_description.h"
+#include "media/base/audio_renderer_sink.h"
+
+namespace content {
+
+// AudioRendererSinkCache implementation.
+class CONTENT_EXPORT AudioRendererSinkCacheImpl
+    : public AudioRendererSinkCache {
+ public:
+  // Callback to be used for AudioRendererSink creation
+  typedef base::Callback<scoped_refptr<media::AudioRendererSink>(
+      int render_frame_id,
+      int session_id,
+      const std::string& device_id,
+      const url::Origin& security_origin)>
+      CreateSinkCallback;
+
+  AudioRendererSinkCacheImpl(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      const CreateSinkCallback& create_sink_callback);
+
+  ~AudioRendererSinkCacheImpl() final;
+
+  media::OutputDeviceInfo GetSinkInfo(int source_render_frame_id,
+                                      int session_id,
+                                      const std::string& device_id,
+                                      const url::Origin& security_origin) final;
+
+  media::AudioRendererSink* GetSink(int source_render_frame_id,
+                                    const std::string& device_id,
+                                    const url::Origin& security_origin) final;
+
+  void ReleaseSink(media::AudioRendererSink* sink) final;
+
+ private:
+  friend class AudioRendererSinkCacheTest;
+
+  // Cached sink deletion timeout.
+  // For example: (1) sink was create and cached in GetSinkInfo(), and then (2)
+  // the same sink is requested in GetSink(), if time interval between (1) and
+  // (2) is less than |kDeleteTimeoutMs|, then sink cached in (1) is reused in
+  // (2). On the other hand, if after (1) nobody is interested in the sink
+  // within |kDeleteTimeoutMs|, it is garbage-collected.
+  enum { kDeleteTimeoutMs = 5000 };
+
+  // The key to be used to access a cached sink (not unique,there can be
+  // multiple sinks per device.
+  struct SinkKey {
+    SinkKey(int source_render_frame_id,
+            const std::string& device_id,
+            const url::Origin& security_origin);
+    SinkKey(const SinkKey& other);
+    int source_render_frame_id;
+    std::string device_id;
+    url::Origin security_origin;
+  };
+
+  // Strict weak ordering of the keys.
+  struct SinkKeyCompare {
+    bool operator()(const SinkKey& a, const SinkKey& b) const {
+      if (a.source_render_frame_id != b.source_render_frame_id)
+        return a.source_render_frame_id < b.source_render_frame_id;
+
+      if (media::AudioDeviceDescription::IsDefaultDevice(a.device_id) &&
+          media::AudioDeviceDescription::IsDefaultDevice(b.device_id)) {
+        // Both device IDs represent the same default device => do not compare
+        // them; the default device is always authorized => ignoring security
+        // origin.
+        return false;
+      }
+
+      if (a.device_id != b.device_id)
+        return a.device_id < b.device_id;
+
+      return a.security_origin < b.security_origin;
+    }
+  };
+
+  // Cached sink data.
+  struct AudioRendererSinkReference {
+    AudioRendererSinkReference(scoped_refptr<media::AudioRendererSink> sink,
+                               bool used);
+    AudioRendererSinkReference(const AudioRendererSinkReference& other);
+    ~AudioRendererSinkReference();
+    scoped_refptr<media::AudioRendererSink> sink;  // Sink instance
+    bool used;  // True if in used by a client.
+  };
+
+  typedef std::multimap<SinkKey, AudioRendererSinkReference, SinkKeyCompare>

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

1. If the number of sinks is small, consider using std::vector<SinkEntry>
instead, where SinkEntry is just the members of three structs merged together:
SinkKey, SinkKeyCompare (a.k.a. operator<()), and AudioRendererSinkReference.
This would be more memory-efficient and reduce indirection. FWIW, the multimap
is only being used for the convenience of an equal_range() iterator in
GetSink(), but then DeleteSink() performs a full linear scan anyway.

2. These structs are only used as template parameters for the STL containers,
and in the .cc file. Therefore, they should just be forward-declared here, with
the full class declaration in the .cc file. In other words, try to put as much
of the implementation in the .cc file as possible. For example, in the .h file:

  class AudioRendererSinkCacheImpl {
    ...
   private:
    struct SinkEntry;
    ...
  };

...and in the .cc file:

  struct AudioRendererSinkCacheImpl::SinkEntry {
    int source_render_frame_id;
    std::string device_id;
    url::Origin security_origin;
    scoped_refptr<media::AudioRendererSink> sink;
    bool used;

    bool operator<(const SinkEntry& other) const {...}
  };

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

1. multimap is also used for find() in GetSinkInfo, which can happen multiple
times per sink (opposite to DeleteSink which happens only once). Overall, if the
number of elements is small, neither performance no memory matter much, and I do
not have good enough vision of how big it can be in some extreme cases. Also,
map interface looks clearer to me. I modified DeleteSink to accept the key as
well. WDYT?
2. My understanding is that incomplete types are not allowed in STL containers
by the standard (and the code does not compile), and I tried but haven't found a
proof of the opposite. Could you point me to the info you have on that?

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

Well, okay. I'll admit I'm "bike-shedding" a little here. If you prefer the
multimap, I'm fine with that.

As for requiring the key for DeleteSink() method: IMHO, it's just extra data
having to be stored and passed around; and it isn't really necessary to execute
a simple, efficient delete operation. So, I like the solution you had before
better. ;-)
IIRC, the type only needs to be complete where the template would be
instantiated. There's probably something in the header file that is causing the
template to be instantiated (i.e., when compiling other modules that include
this header file). It's not a huge deal, so don't go to any trouble here if it's
not obvious what's causing the compile error.

[o1ka, 2016/05/20 10:40:32]

Ok. But before reverting this to the first version I'd like to make sure Dale is
also happy with that.

+      AudioRendererSinkMap;
+
+  // Schedules a sink for deletion. Deletion will be performed on the same
+  // thread the cache is created on.
+  void DeleteLaterIfUnused(media::AudioRendererSink* sink);
+
+  // Deletes a sink from the cache. If |force_delete_used| is set, a sink being
+  // deleted can (and should) be in use at the moment of deletion; otherwise the
+  // sink is deleted only if unused.
+  void DeleteSink(media::AudioRendererSink* sink, bool force_delete_used);
+
+  // Creates a sink with a specified |key| and |used| state and inserts it into
+  // the cache. Note: |sinks_lock_| must be held while operating on the result
+  // of this function, otherwise the sink may be deleted and the pointer becomes
+  // invalid.
+  media::AudioRendererSink* InsertNewSink(const SinkKey& key, bool used);
+
+  // Task runner for scheduled sink garbage collection.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

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

Please make this const.

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

Done.

+
+  // Callback used for sink creation.
+  const CreateSinkCallback create_sink_cb_;
+
+  // Cached sinks, protected by lock.
+  base::Lock sinks_lock_;
+  AudioRendererSinkMap sinks_;
+
+  base::WeakPtrFactory<AudioRendererSinkCacheImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioRendererSinkCacheImpl);
+};
+
+}  // namespace content