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_mixer_manager.h b/content/renderer/media/audio_renderer_sink_cache_impl.h
similarity index 22%
copy from content/renderer/media/audio_renderer_mixer_manager.h
copy to content/renderer/media/audio_renderer_sink_cache_impl.h
index 20a1aa59eaf3db3d8e65c0ef805dfe98c16e5e9e..1c6eabf139346925f86968d01b98c06de227a1eb 100644
--- a/content/renderer/media/audio_renderer_mixer_manager.h
+++ b/content/renderer/media/audio_renderer_sink_cache_impl.h
@@ -1,110 +1,73 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// 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.
 
-#ifndef CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_MIXER_MANAGER_H_
-#define CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_MIXER_MANAGER_H_
+#include "content/renderer/media/audio_renderer_sink_cache.h"

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

Need to surround the contents of this header with:

#ifndef CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_
#define CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_

...

#endif  // CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_SINK_CACHE_IMPL_H_

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

Ooops

 
 #include <map>
-#include <string>
-#include <utility>
 
-#include "base/macros.h"
+#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_parameters.h"
-#include "media/base/output_device_info.h"
-#include "url/origin.h"
-
-namespace media {
-class AudioHardwareConfig;
-class AudioRendererMixer;
-class AudioRendererMixerInput;
-class AudioRendererSink;
-}
+#include "media/base/audio_renderer_sink.h"
 
 namespace content {
 
-// Manages sharing of an AudioRendererMixer among AudioRendererMixerInputs based
-// on their AudioParameters configuration.  Inputs with the same AudioParameters
-// configuration will share a mixer while a new AudioRendererMixer will be
-// lazily created if one with the exact AudioParameters does not exist.
-//
-// There should only be one instance of AudioRendererMixerManager per render
-// thread.
-//
-// TODO(dalecurtis): Right now we require AudioParameters to be an exact match
-// when we should be able to ignore bits per channel since we're only dealing
-// with floats.  However, bits per channel is currently used to interleave the
-// audio data by AudioOutputDevice::AudioThreadCallback::Process for consumption
-// via the shared memory.  See http://crbug.com/114700.
-class CONTENT_EXPORT AudioRendererMixerManager {
+// AudioRendererSinkCache implementation.
+class CONTENT_EXPORT AudioRendererSinkCacheImpl
+    : public AudioRendererSinkCache {
  public:
-  AudioRendererMixerManager();
-  ~AudioRendererMixerManager();
-
-  // Creates an AudioRendererMixerInput with the proper callbacks necessary to
-  // retrieve an AudioRendererMixer instance from AudioRendererMixerManager.
-  // |source_render_frame_id| refers to the RenderFrame containing the entity
-  // rendering the audio.  Caller must ensure AudioRendererMixerManager outlives
-  // the returned input. |device_id|, |session_id| and |security_origin|
-  // identify the output device to use. If |device_id| is empty and |session_id|
-  // is nonzero, output device associated with the opened input device
-  // designated by |session_id| is used. Otherwise, |session_id| is ignored.
-  media::AudioRendererMixerInput* CreateInput(
+  // Callback to be used for AudioRendererSink creation
+  using CreateSinkCallback =
+      base::Callback<scoped_refptr<media::AudioRendererSink>(
+          int render_frame_id,
+          int session_id,
+          const std::string& device_id,
+          const url::Origin& security_origin)>;
+
+  AudioRendererSinkCacheImpl(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      const CreateSinkCallback& create_sink_callback,
+      int delete_timeout_ms);

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

Prefer using base::TimeDelta to pass time durations, rather than POD integer
values.

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

Could you suggest why it's more preferable?

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

Because it let's the compiler confirm, via static type-checking, that developers
are not mishandling time values or doing incorrect time math. Also, the Chromium
time types make it easy for developers to write self-documenting code (e.g.,
TimeDelta::FromMilliseconds(...) is unmistakable).

This may sound like a small thing, but I myself have fixed a number of weird,
very time-consuming bugs in Chromium code where people used POD ints instead of
a strong time type. For example, in one case someone renamed a value from
|foo_us| to |foo_ms| and forgot to change the interpretation of the value in one
spot; and impossible-to-solve crbugs piled in for several months before the root
cause was ascertained.

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

Aha, thanks for the explanation! Actually, I recently fixed just the same type
of bug when ms were renamed to frames. Now the profit becomes obvious :)

+
+  ~AudioRendererSinkCacheImpl() final;
+
+  media::OutputDeviceInfo GetSinkInfo(int source_render_frame_id,
+                                      int session_id,
+                                      const std::string& device_id,
+                                      const url::Origin& security_origin) final;
+
+  scoped_refptr<media::AudioRendererSink> GetSink(
       int source_render_frame_id,
-      int session_id,
       const std::string& device_id,
-      const url::Origin& security_origin);
+      const url::Origin& security_origin) final;
 
-  // Returns a mixer instance based on AudioParameters; an existing one if one
-  // with the provided AudioParameters exists or a new one if not.
-  media::AudioRendererMixer* GetMixer(int source_render_frame_id,
-                                      const media::AudioParameters& params,
-                                      const std::string& device_id,
-                                      const url::Origin& security_origin,
-                                      media::OutputDeviceStatus* device_status);
-
-  // Remove a mixer instance given a mixer if the only other reference is held
-  // by AudioRendererMixerManager.  Every AudioRendererMixer owner must call
-  // this method when it's done with a mixer.
-  void RemoveMixer(int source_render_frame_id,
-                   const media::AudioParameters& params,
+  void ReleaseSink(int source_render_frame_id,
                    const std::string& device_id,
-                   const url::Origin& security_origin);
+                   const url::Origin& security_origin,
+                   const media::AudioRendererSink* sink) final;
 
  private:
-  friend class AudioRendererMixerManagerTest;
-
-  // Define a key so that only those AudioRendererMixerInputs from the same
-  // RenderView, AudioParameters and output device can be mixed together.
-  struct MixerKey {
-    MixerKey(int source_render_frame_id,
-             const media::AudioParameters& params,
-             const std::string& device_id,
-             const url::Origin& security_origin);
-    MixerKey(const MixerKey& other);
+  friend class AudioRendererSinkCacheTest;
+
+  // 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;
-    media::AudioParameters params;
     std::string device_id;
     url::Origin security_origin;
   };
 
-  // Custom compare operator for the AudioRendererMixerMap.  Allows reuse of
-  // mixers where only irrelevant keys mismatch; e.g., effects, bits per sample.
-  struct MixerKeyCompare {
-    bool operator()(const MixerKey& a, const MixerKey& b) const {
+  // Strict weak ordering of the keys.
+  struct SinkKeyCompare {
+    bool operator()(const SinkKey& a, const SinkKey& b) const {

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

Along the lines of my comment from last round: You might get rid of this struct,
and instead declare an operator<() method in SinkKey, and then define the method
in the .cc file. This would put more of the implementation in the .cc file
instead of here in the header file.

[o1ka, 2016/05/20 11:59:03]

That would probably look neater, but according to Google style guide: "don't
define operator overloads just because other libraries expect them. For example,
if your type doesn't have a natural ordering, but you want to store it in a
std::set, use a custom comparator rather than overloading <".

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

Looks like this got resolved via the switch-over to std::vector.

       if (a.source_render_frame_id != b.source_render_frame_id)
         return a.source_render_frame_id < b.source_render_frame_id;
-      if (a.params.channels() != b.params.channels())
-        return a.params.channels() < b.params.channels();
-
-      // Ignore effects(), bits_per_sample(), format(), and frames_per_buffer(),
-      // these parameters do not affect mixer reuse.  All AudioRendererMixer
-      // units disable FIFO, so frames_per_buffer() can be safely ignored.
-      if (a.params.channel_layout() != b.params.channel_layout())
-        return a.params.channel_layout() < b.params.channel_layout();
 
       if (media::AudioDeviceDescription::IsDefaultDevice(a.device_id) &&
           media::AudioDeviceDescription::IsDefaultDevice(b.device_id)) {
@@ -121,23 +84,58 @@ class CONTENT_EXPORT AudioRendererMixerManager {
     }
   };
 
-  // Map of MixerKey to <AudioRendererMixer, Count>.  Count allows
-  // AudioRendererMixerManager to keep track explicitly (v.s. RefCounted which
-  // is implicit) of the number of outstanding AudioRendererMixers.
-  struct AudioRendererMixerReference {
-    media::AudioRendererMixer* mixer;
-    int ref_count;
+  // 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::map<MixerKey, AudioRendererMixerReference, MixerKeyCompare>
-      AudioRendererMixerMap;
 
-  // Active mixers.
-  AudioRendererMixerMap mixers_;
-  base::Lock mixers_lock_;
+  using AudioRendererSinkMap =
+      std::multimap<SinkKey, AudioRendererSinkReference, SinkKeyCompare>;
+
+  // Schedules a sink for deletion. Deletion will be performed on the same
+  // thread the cache is created on.
+  void DeleteLaterIfUnused(const SinkKey& key,
+                           const 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(const SinkKey& key,
+                  const media::AudioRendererSink* sink,
+                  bool force_delete_used);
 
-  DISALLOW_COPY_AND_ASSIGN(AudioRendererMixerManager);
+  // Creates a sink with a specified |key| and |used| state and inserts it into
+  // the cache.
+  scoped_refptr<media::AudioRendererSink> InsertNewSinkWhileLockHeld(
+      const SinkKey& key,
+      bool used);
+
+  // Cached sink deletion timeout.
+  // For example: (1) sink was create and cached in GetSinkInfo(), and then (2)

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

typo: s/create/created/

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

Done.

+  // 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.
+  const int delete_timeout_ms_;

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

ditto: Prefer using base::TimeDelta for time durations, rather than POD integer
values.

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

Done.

+
+  // Task runner for scheduled sink garbage collection.
+  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  // 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
-
-#endif  // CONTENT_RENDERER_MEDIA_AUDIO_RENDERER_MIXER_MANAGER_H_