Stop using AudioRendererSinkCache for AudioRendererMixerManager.

This is part 3/4 CLs to move the <audio>/<video> elements off of
a synchronous API that can lead to renderer hangs and premature
audio renderer errors.

This moves the AudioRendererSinkCache from AudioRendererMixerManger
to a base::NoDestructor instance in AudioDeviceFactory and stops
ARMM from using the cache at all. The users remaining are WebAudio
and WebRTC, we can reevaluate metrics on keeping it after.

Due to base::(SingleThread|Sequenced)TaskRunner::Get() being
deprecated for content/ I've had to switch to use base::PostTask to
generate a cleanup task runner. Unfortunately this doesn't guarantee
that it runs on the render thread, but since we're now using a static
instance, we can just always use base::Unretained and drop the WeakPtr
factory that was handling the cancellation of these tasks.

There's no point in the cache for ARMM since we're going to require
a sink to get a mixer. That sink will always be used to get output
device info first too and then reused for the mixer.

BUG=905506
TEST=updated tests, compiles.
R=olka

Change-Id: I03408753f974e4c6fb9c89270508e26689162002
Reviewed-on: https://chromium-review.googlesource.com/c/1347730
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612460}

content/renderer/media/audio/audio_device_factory.cc

@@ -8,6 +8,8 @@
 
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/no_destructor.h"
+#include "base/task/post_task.h"
 #include "base/threading/platform_thread.h"
 #include "build/build_config.h"
 #include "content/common/content_constants_internal.h"
@@ -15,6 +17,7 @@
 #include "content/renderer/media/audio/audio_input_ipc_factory.h"
 #include "content/renderer/media/audio/audio_output_ipc_factory.h"
 #include "content/renderer/media/audio/audio_renderer_mixer_manager.h"
+#include "content/renderer/media/audio/audio_renderer_sink_cache_impl.h"
 #include "content/renderer/media/audio/mojo_audio_input_ipc.h"
 #include "content/renderer/render_frame_impl.h"
 #include "content/renderer/render_thread_impl.h"
@@ -182,11 +185,23 @@ AudioDeviceFactory::NewAudioCapturerSource(
 media::OutputDeviceInfo AudioDeviceFactory::GetOutputDeviceInfo(
     int render_frame_id,
     const media::AudioSinkParameters& params) {
-  RenderThreadImpl* render_thread = RenderThreadImpl::current();
-  DCHECK(render_thread) << "RenderThreadImpl is not instantiated, or "
-                        << "GetOutputDeviceInfo() is called on a wrong thread ";
-  return render_thread->GetAudioRendererMixerManager()->GetOutputDeviceInfo(
-      render_frame_id, params.session_id, params.device_id);
+  DCHECK(RenderThreadImpl::current())
+      << "RenderThreadImpl is not instantiated, or "
+      << "GetOutputDeviceInfo() is called on a wrong thread ";
+
+  constexpr base::TimeDelta kDeleteTimeout =
+      base::TimeDelta::FromMilliseconds(5000);
+
+  // There's one process wide instance that lives on the render thread.
+  static base::NoDestructor<AudioRendererSinkCacheImpl> cache(
+      base::CreateSequencedTaskRunnerWithTraits(
+          {base::TaskPriority::BEST_EFFORT,
+           base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN}),
+      base::BindRepeating(&AudioDeviceFactory::NewAudioRendererSink,
+                          AudioDeviceFactory::kSourceNone),
+      kDeleteTimeout);
+  return cache->GetSinkInfo(render_frame_id, params.session_id,
+                            params.device_id);
 }
 
 AudioDeviceFactory::AudioDeviceFactory() {

content/renderer/media/audio/audio_renderer_mixer_manager.cc

@@ -14,7 +14,7 @@
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
-#include "content/renderer/media/audio/audio_renderer_sink_cache.h"
+#include "content/renderer/media/audio/audio_device_factory.h"
 #include "media/audio/audio_device_description.h"
 #include "media/base/audio_renderer_mixer.h"
 #include "media/base/audio_renderer_mixer_input.h"
@@ -130,9 +130,9 @@ void LogMixerUmaHistogram(media::AudioLatency::LatencyType latency, int value) {
 namespace content {
 
 AudioRendererMixerManager::AudioRendererMixerManager(
-    std::unique_ptr<AudioRendererSinkCache> sink_cache)
-    : sink_cache_(std::move(sink_cache)) {
-  DCHECK(sink_cache_);
+    CreateSinkCB create_sink_cb)
+    : create_sink_cb_(std::move(create_sink_cb)) {
+  DCHECK(create_sink_cb_);
 }
 
 AudioRendererMixerManager::~AudioRendererMixerManager() {
@@ -143,8 +143,8 @@ AudioRendererMixerManager::~AudioRendererMixerManager() {
 
 // static
 std::unique_ptr<AudioRendererMixerManager> AudioRendererMixerManager::Create() {
-  return base::WrapUnique(
-      new AudioRendererMixerManager(AudioRendererSinkCache::Create()));
+  return base::WrapUnique(new AudioRendererMixerManager(
+      base::BindRepeating(&AudioDeviceFactory::NewAudioRendererMixerSink)));
 }
 
 media::AudioRendererMixerInput* AudioRendererMixerManager::CreateInput(
@@ -197,14 +197,13 @@ media::AudioRendererMixer* AudioRendererMixerManager::GetMixer(
     return it->second.mixer;
   }
 
-  scoped_refptr<media::AudioRendererSink> sink =
-      sink_cache_->GetSink(source_render_frame_id, device_id);
+  scoped_refptr<media::AudioRendererSink> sink = create_sink_cb_.Run(
+      source_render_frame_id, media::AudioSinkParameters(0, device_id));
 
   const media::OutputDeviceInfo& device_info = sink->GetOutputDeviceInfo();
   if (device_status)
     *device_status = device_info.device_status();
   if (device_info.device_status() != media::OUTPUT_DEVICE_STATUS_OK) {
-    sink_cache_->ReleaseSink(sink.get());
     sink->Stop();
     return nullptr;
   }
@@ -212,9 +211,9 @@ media::AudioRendererMixer* AudioRendererMixerManager::GetMixer(
   const media::AudioParameters& mixer_output_params =
       GetMixerOutputParams(input_params, device_info.output_params(), latency);
   media::AudioRendererMixer* mixer = new media::AudioRendererMixer(
-      mixer_output_params, sink, base::Bind(LogMixerUmaHistogram, latency));
-  AudioRendererMixerReference mixer_reference = {mixer, 1, sink.get()};
-  mixers_[key] = mixer_reference;
+      mixer_output_params, std::move(sink),
+      base::BindRepeating(LogMixerUmaHistogram, latency));
+  mixers_[key] = {mixer, 1};
   DVLOG(1) << __func__ << " mixer: " << mixer << " latency: " << latency
            << "\n input: " << input_params.AsHumanReadableString()
            << "\noutput: " << mixer_output_params.AsHumanReadableString();
@@ -233,8 +232,6 @@ void AudioRendererMixerManager::ReturnMixer(media::AudioRendererMixer* mixer) {
   // Only remove the mixer if AudioRendererMixerManager is the last owner.
   it->second.ref_count--;
   if (it->second.ref_count == 0) {
-    // The mixer will be deleted now, so release the sink.
-    sink_cache_->ReleaseSink(it->second.sink_ptr);
     delete it->second.mixer;
     mixers_.erase(it);
   }
@@ -244,8 +241,12 @@ media::OutputDeviceInfo AudioRendererMixerManager::GetOutputDeviceInfo(
     int source_render_frame_id,
     int session_id,
     const std::string& device_id) {
-  return sink_cache_->GetSinkInfo(source_render_frame_id, session_id,
-                                  device_id);
+  auto sink =
+      create_sink_cb_.Run(source_render_frame_id,
+                          media::AudioSinkParameters(session_id, device_id));
+  auto info = sink->GetOutputDeviceInfo();
+  sink->Stop();
+  return info;
 }
 
 AudioRendererMixerManager::MixerKey::MixerKey(

content/renderer/media/audio/audio_renderer_mixer_manager.h

@@ -10,10 +10,12 @@
 #include <memory>
 #include <string>
 
+#include "base/containers/flat_map.h"
 #include "base/macros.h"
 #include "base/synchronization/lock.h"
 #include "content/common/content_export.h"
 #include "media/audio/audio_device_description.h"
+#include "media/audio/audio_sink_parameters.h"
 #include "media/base/audio_latency.h"
 #include "media/base/audio_parameters.h"
 #include "media/base/audio_renderer_mixer_pool.h"
@@ -26,7 +28,6 @@ class AudioRendererSink;
 }
 
 namespace content {
-class AudioRendererSinkCache;
 
 // Manages sharing of an AudioRendererMixer among AudioRendererMixerInputs based
 // on their AudioParameters configuration.  Inputs with the same AudioParameters
@@ -75,8 +76,14 @@ class CONTENT_EXPORT AudioRendererMixerManager
       const std::string& device_id) final;
 
  protected:
-  explicit AudioRendererMixerManager(
-      std::unique_ptr<AudioRendererSinkCache> sink_cache);
+  // Callback which will be used to create sinks. See AudioDeviceFactory for
+  // more details on the parameters.
+  using CreateSinkCB =
+      base::RepeatingCallback<scoped_refptr<media::AudioRendererSink>(
+          int source_render_frame_id,
+          const media::AudioSinkParameters& params)>;
+
+  explicit AudioRendererMixerManager(CreateSinkCB create_sink_cb);
 
  private:
   friend class AudioRendererMixerManagerTest;
@@ -130,26 +137,23 @@ class CONTENT_EXPORT AudioRendererMixerManager
     }
   };
 
+  const CreateSinkCB create_sink_cb_;
+
   // 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;
-    // Mixer sink pointer, to remove a sink from cache upon mixer destruction.
-    const media::AudioRendererSink* sink_ptr;
   };
 
   using AudioRendererMixerMap =
-      std::map<MixerKey, AudioRendererMixerReference, MixerKeyCompare>;
+      base::flat_map<MixerKey, AudioRendererMixerReference, MixerKeyCompare>;
 
   // Active mixers.
   AudioRendererMixerMap mixers_;
   base::Lock mixers_lock_;
 
-  // Mixer sink cache.
-  const std::unique_ptr<AudioRendererSinkCache> sink_cache_;
-
   // Map of the output latencies encountered throughout mixer manager lifetime.
   // Used for UMA histogram logging.
   std::bitset<media::AudioLatency::LATENCY_COUNT> latency_map_;

content/renderer/media/audio/audio_renderer_sink_cache.h

@@ -27,9 +27,6 @@ class CONTENT_EXPORT AudioRendererSinkCache {
  public:
   virtual ~AudioRendererSinkCache() {}
 
-  // Creates default cache, to be used by AudioRendererMixerManager.
-  static std::unique_ptr<AudioRendererSinkCache> Create();
-
   // If called, the cache will drop sinks belonging to the specified frame on
   // navigation.
   static void ObserveFrame(RenderFrame* frame);

content/renderer/media/audio/audio_renderer_sink_cache_impl.cc

@@ -14,7 +14,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/synchronization/lock.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/task/post_task.h"
 #include "base/trace_event/trace_event.h"
 #include "content/public/renderer/render_frame.h"
 #include "content/public/renderer/render_frame_observer.h"
@@ -25,14 +25,13 @@
 namespace content {
 
 AudioRendererSinkCacheImpl* AudioRendererSinkCacheImpl::instance_ = nullptr;
-constexpr int kDeleteTimeoutMs = 5000;
 constexpr int kDefaultSessionId = 0;
 
 class AudioRendererSinkCacheImpl::FrameObserver : public RenderFrameObserver {
  public:
   explicit FrameObserver(content::RenderFrame* render_frame)
       : RenderFrameObserver(render_frame) {}
-  ~FrameObserver() override{};
+  ~FrameObserver() override {}
 
  private:
   // content::RenderFrameObserver implementation:
@@ -87,36 +86,23 @@ struct AudioRendererSinkCacheImpl::CacheEntry {
   bool used;                                     // True if in use by a client.
 };
 
-// static
-std::unique_ptr<AudioRendererSinkCache> AudioRendererSinkCache::Create() {
-  return std::make_unique<AudioRendererSinkCacheImpl>(
-      base::ThreadTaskRunnerHandle::Get(),
-      base::Bind(&AudioDeviceFactory::NewAudioRendererMixerSink),
-      base::TimeDelta::FromMilliseconds(kDeleteTimeoutMs));
-}
-
 // static
 void AudioRendererSinkCache::ObserveFrame(RenderFrame* frame) {
   new AudioRendererSinkCacheImpl::FrameObserver(frame);
 }
 
 AudioRendererSinkCacheImpl::AudioRendererSinkCacheImpl(
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    scoped_refptr<base::SequencedTaskRunner> cleanup_task_runner,
     CreateSinkCallback create_sink_cb,
     base::TimeDelta delete_timeout)
-    : task_runner_(std::move(task_runner)),
+    : cleanup_task_runner_(std::move(cleanup_task_runner)),
       create_sink_cb_(std::move(create_sink_cb)),
-      delete_timeout_(delete_timeout),
-      weak_ptr_factory_(this) {
-  weak_this_ = weak_ptr_factory_.GetWeakPtr();
-  if (instance_)
-    LOG(ERROR) << "More that one AudioRendererSinkCache instance created. "
-                  "Allowed in tests only.";
+      delete_timeout_(delete_timeout) {
+  DCHECK(!instance_);
   instance_ = this;
 }
 
 AudioRendererSinkCacheImpl::~AudioRendererSinkCacheImpl() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
   // We just release all the cached sinks here. Stop them first.
   // We can stop all the sinks, no matter they are used or not, since everything
   // is being destroyed anyways.
@@ -238,10 +224,12 @@ void AudioRendererSinkCacheImpl::ReleaseSink(
 
 void AudioRendererSinkCacheImpl::DeleteLaterIfUnused(
     const media::AudioRendererSink* sink_ptr) {
-  task_runner_->PostDelayedTask(
+  cleanup_task_runner_->PostDelayedTask(
       FROM_HERE,
-      base::BindOnce(&AudioRendererSinkCacheImpl::DeleteSink, weak_this_,
-                     base::RetainedRef(sink_ptr),
+      base::BindOnce(&AudioRendererSinkCacheImpl::DeleteSink,
+                     // Unretained is safe here since this is a process-wide
+                     // singleton and tests will ensure lifetime.
+                     base::Unretained(this), base::RetainedRef(sink_ptr),
                      false /*do not delete if used*/),
       delete_timeout_);
 }

content/renderer/media/audio/audio_renderer_sink_cache_impl.h

@@ -11,8 +11,7 @@
 #include <vector>
 
 #include "base/callback.h"
-#include "base/memory/weak_ptr.h"
-#include "base/single_thread_task_runner.h"
+#include "base/sequenced_task_runner.h"
 #include "base/synchronization/lock.h"
 #include "base/time/time.h"
 #include "content/common/content_export.h"
@@ -32,8 +31,11 @@ class CONTENT_EXPORT AudioRendererSinkCacheImpl
           int render_frame_id,
           const media::AudioSinkParameters& params)>;
 
+  // |cleanup_task_runner| will be used to delete sinks when they are unused,
+  // AudioRendererSinkCacheImpl must outlive any tasks posted to it. Since
+  // the sink cache is normally a process-wide singleton, this isn't a problem.
   AudioRendererSinkCacheImpl(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      scoped_refptr<base::SequencedTaskRunner> cleanup_task_runner,
       CreateSinkCallback create_sink_callback,
       base::TimeDelta delete_timeout);
 
@@ -85,7 +87,7 @@ class CONTENT_EXPORT AudioRendererSinkCacheImpl
   static AudioRendererSinkCacheImpl* instance_;
 
   // Renderer main task runner.
-  const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  const scoped_refptr<base::SequencedTaskRunner> cleanup_task_runner_;
 
   // Callback used for sink creation.
   const CreateSinkCallback create_sink_cb_;
@@ -102,17 +104,6 @@ class CONTENT_EXPORT AudioRendererSinkCacheImpl
   base::Lock cache_lock_;
   CacheContainer cache_;
 
-  // Weak pointer to be used for delayed sink deletion on |task_runner_|.
-  // Pre-created in constructor and is used to post all the delayed tasks.
-  // A delayed task can be concurrently posted from any thread the cache is used
-  // on, so on-the-flight weak pointer creation with
-  // weak_ptr_factory_.GetWeakPtr() can't be used, because it will result in the
-  // racy access to the factory.
-  base::WeakPtr<AudioRendererSinkCacheImpl> weak_this_;
-
-  // Used to produce |weak_this_| on AudioRendererSinkCacheImpl construction.
-  base::WeakPtrFactory<AudioRendererSinkCacheImpl> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(AudioRendererSinkCacheImpl);
 };
 

content/renderer/media/audio/audio_renderer_sink_cache_unittest.cc

@@ -40,6 +40,9 @@ class AudioRendererSinkCacheTest : public testing::Test {
             base::BindRepeating(&AudioRendererSinkCacheTest::CreateSink,
                                 base::Unretained(this)),
             kDeleteTimeout)) {}
+  ~AudioRendererSinkCacheTest() override {
+    task_env_.FastForwardUntilNoTasksRemain();
+  }
 
   void GetSink(int render_frame_id,
                const std::string& device_id,
@@ -245,6 +248,7 @@ TEST_F(AudioRendererSinkCacheTest, UnhealthySinkIsStopped) {
       new media::MockAudioRendererSink(
           kUnhealthyDeviceId, media::OUTPUT_DEVICE_STATUS_ERROR_INTERNAL);
 
+  cache_.reset();  // Destruct first so there's only one cache at a time.
   cache_ = std::make_unique<AudioRendererSinkCacheImpl>(
       task_env_.GetMainThreadTaskRunner(),
       base::BindRepeating(
@@ -271,6 +275,7 @@ TEST_F(AudioRendererSinkCacheTest, UnhealthySinkUsingSessionIdIsStopped) {
       new media::MockAudioRendererSink(
           kUnhealthyDeviceId, media::OUTPUT_DEVICE_STATUS_ERROR_INTERNAL);
 
+  cache_.reset();  // Destruct first so there's only one cache at a time.
   cache_ = std::make_unique<AudioRendererSinkCacheImpl>(
       task_env_.GetMainThreadTaskRunner(),
       base::BindRepeating(