Convert <audio> pipeline to use async device info requests.

This is part 4/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 changes the AudioRendererMixerPool API to require an
AudioRendererSink and OutputDeviceInfo when providing a mixer.
AudioRendererMixerInputs are subsequently changed to use the new
API.

Likewise AudioRendererImpl also now uses the asynchronous API. To
simplify the async process, AudioRendererMixerInputs will only setup
correctly when OutputDeviceInfo has been requested ahead of time,
since that's the pattern that AudioRendererImpl will use.

This also moves the NullAudioSink setup from WebAudioSourceProvider
over to the AudioRendererImpl. This causes WebAudio to be disconnected
from the element, but if audio isn't work anyways, it shouldn't matter.

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

Change-Id: I4edf89bb1e20cc91191a6eb97a0e38b6aeba68f8
Reviewed-on: https://chromium-review.googlesource.com/c/1347795
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612526}

content/renderer/media/audio/audio_device_factory.cc

@@ -78,10 +78,9 @@ scoped_refptr<media::SwitchableAudioRendererSink> NewMixableSink(
   DCHECK(render_thread) << "RenderThreadImpl is not instantiated, or "
                         << "GetOutputDeviceInfo() is called on a wrong thread ";
   DCHECK(!params.processing_id.has_value());
-  return scoped_refptr<media::AudioRendererMixerInput>(
-      render_thread->GetAudioRendererMixerManager()->CreateInput(
-          render_frame_id, params.session_id, params.device_id,
-          AudioDeviceFactory::GetSourceLatencyType(source_type)));
+  return render_thread->GetAudioRendererMixerManager()->CreateInput(
+      render_frame_id, params.session_id, params.device_id,
+      AudioDeviceFactory::GetSourceLatencyType(source_type));
 }
 
 }  // namespace
@@ -110,8 +109,9 @@ scoped_refptr<media::AudioRendererSink>
 AudioDeviceFactory::NewAudioRendererMixerSink(
     int render_frame_id,
     const media::AudioSinkParameters& params) {
-  return NewFinalAudioRendererSink(render_frame_id, params,
-                                   GetDefaultAuthTimeout());
+  // AudioRendererMixer sinks are always used asynchronously and thus can
+  // operate without a timeout value.
+  return NewFinalAudioRendererSink(render_frame_id, params, base::TimeDelta());
 }
 
 // static

content/renderer/media/audio/audio_device_factory.h

@@ -51,9 +51,11 @@ class CONTENT_EXPORT AudioDeviceFactory {
   static media::AudioLatency::LatencyType GetSourceLatencyType(
       SourceType source);
 
-  // Creates a sink for AudioRendererMixer.
-  // |render_frame_id| refers to the RenderFrame containing the entity
-  // producing the audio.
+  // Creates a sink for AudioRendererMixer. |render_frame_id| refers to the
+  // RenderFrame containing the entity producing the audio. Note: These sinks do
+  // not support the blocking GetOutputDeviceInfo() API and instead clients are
+  // required to use the GetOutputDeviceInfoAsync() API. As such they are
+  // configured with no authorization timeout value.
   static scoped_refptr<media::AudioRendererSink> NewAudioRendererMixerSink(
       int render_frame_id,
       const media::AudioSinkParameters& params);

content/renderer/media/audio/audio_renderer_mixer_manager.cc

@@ -147,7 +147,8 @@ std::unique_ptr<AudioRendererMixerManager> AudioRendererMixerManager::Create() {
       base::BindRepeating(&AudioDeviceFactory::NewAudioRendererMixerSink)));
 }
 
-media::AudioRendererMixerInput* AudioRendererMixerManager::CreateInput(
+scoped_refptr<media::AudioRendererMixerInput>
+AudioRendererMixerManager::CreateInput(
     int source_render_frame_id,
     int session_id,
     const std::string& device_id,
@@ -155,23 +156,26 @@ media::AudioRendererMixerInput* AudioRendererMixerManager::CreateInput(
   // AudioRendererMixerManager lives on the renderer thread and is destroyed on
   // renderer thread destruction, so it's safe to pass its pointer to a mixer
   // input.
-  return new media::AudioRendererMixerInput(
-      this, source_render_frame_id,
-      media::AudioDeviceDescription::UseSessionIdToSelectDevice(session_id,
-                                                                device_id)
-          ? GetOutputDeviceInfo(source_render_frame_id, session_id, device_id)
-                .device_id()
-          : device_id,
-      latency);
+  //
+  // TODO(olka, grunell): |session_id| is always zero, delete since
+  // NewAudioRenderingMixingStrategy didn't ship, https://crbug.com/870836.
+  DCHECK_EQ(session_id, 0);
+  return base::MakeRefCounted<media::AudioRendererMixerInput>(
+      this, source_render_frame_id, device_id, latency);
 }
 
 media::AudioRendererMixer* AudioRendererMixerManager::GetMixer(
     int source_render_frame_id,
     const media::AudioParameters& input_params,
     media::AudioLatency::LatencyType latency,
-    const std::string& device_id,
-    media::OutputDeviceStatus* device_status) {
-  const MixerKey key(source_render_frame_id, input_params, latency, device_id);
+    const media::OutputDeviceInfo& sink_info,
+    scoped_refptr<media::AudioRendererSink> sink) {
+  // Ownership of the sink must be given to GetMixer().
+  DCHECK(sink->HasOneRef());
+  DCHECK_EQ(sink_info.device_status(), media::OUTPUT_DEVICE_STATUS_OK);
+
+  const MixerKey key(source_render_frame_id, input_params, latency,
+                     sink_info.device_id());
   base::AutoLock auto_lock(mixers_lock_);
 
   // Update latency map when the mixer is requested, i.e. there is an attempt to
@@ -189,27 +193,22 @@ media::AudioRendererMixer* AudioRendererMixerManager::GetMixer(
 
   auto it = mixers_.find(key);
   if (it != mixers_.end()) {
-    if (device_status)
-      *device_status = media::OUTPUT_DEVICE_STATUS_OK;
-
     it->second.ref_count++;
     DVLOG(1) << "Reusing mixer: " << it->second.mixer;
-    return it->second.mixer;
-  }
 
-  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 will now be released unused, but still must be stopped.
+    //
+    // TODO(dalecurtis): Is it worth caching this sink instead for a future
+    // GetSink() call? We should experiment with a few top sites. We can't just
+    // drop in AudioRendererSinkCache here since it doesn't reuse sinks once
+    // they've been vended externally to the class.
     sink->Stop();
-    return nullptr;
+
+    return it->second.mixer;
   }
 
   const media::AudioParameters& mixer_output_params =
-      GetMixerOutputParams(input_params, device_info.output_params(), latency);
+      GetMixerOutputParams(input_params, sink_info.output_params(), latency);
   media::AudioRendererMixer* mixer = new media::AudioRendererMixer(
       mixer_output_params, std::move(sink),
       base::BindRepeating(LogMixerUmaHistogram, latency));
@@ -237,16 +236,11 @@ void AudioRendererMixerManager::ReturnMixer(media::AudioRendererMixer* mixer) {
   }
 }
 
-media::OutputDeviceInfo AudioRendererMixerManager::GetOutputDeviceInfo(
+scoped_refptr<media::AudioRendererSink> AudioRendererMixerManager::GetSink(
     int source_render_frame_id,
-    int session_id,
     const std::string& 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;
+  return create_sink_cb_.Run(source_render_frame_id,
+                             media::AudioSinkParameters(0, device_id));
 }
 
 AudioRendererMixerManager::MixerKey::MixerKey(

content/renderer/media/audio/audio_renderer_mixer_manager.h

@@ -53,7 +53,7 @@ class CONTENT_EXPORT AudioRendererMixerManager
   // 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(
+  scoped_refptr<media::AudioRendererMixerInput> CreateInput(
       int source_render_frame_id,
       int session_id,
       const std::string& device_id,
@@ -65,14 +65,13 @@ class CONTENT_EXPORT AudioRendererMixerManager
       int source_render_frame_id,
       const media::AudioParameters& input_params,
       media::AudioLatency::LatencyType latency,
-      const std::string& device_id,
-      media::OutputDeviceStatus* device_status) final;
+      const media::OutputDeviceInfo& sink_info,
+      scoped_refptr<media::AudioRendererSink> sink) final;
 
   void ReturnMixer(media::AudioRendererMixer* mixer) final;
 
-  media::OutputDeviceInfo GetOutputDeviceInfo(
+  scoped_refptr<media::AudioRendererSink> GetSink(
       int source_render_frame_id,
-      int session_id,
       const std::string& device_id) final;
 
  protected:

media/audio/BUILD.gn

@@ -56,6 +56,8 @@ source_set("audio") {
   visibility = [
     "//media",
 
+    "//media/renderers",
+
     # TODO(dalecurtis): CoreAudioUtil::IsCoreAudioSupported() should probably
     # move into media/base/win.
     "//media/device_monitors",

media/base/audio_renderer_mixer.cc

@@ -155,9 +155,10 @@ void AudioRendererMixer::SetPauseDelayForTesting(base::TimeDelta delay) {
   pause_delay_ = delay;
 }
 
-OutputDeviceInfo AudioRendererMixer::GetOutputDeviceInfo() {
+void AudioRendererMixer::GetOutputDeviceInfoAsync(
+    AudioRendererSink::OutputDeviceInfoCB info_cb) {
   DVLOG(1) << __func__;
-  return audio_sink_->GetOutputDeviceInfo();
+  return audio_sink_->GetOutputDeviceInfoAsync(std::move(info_cb));
 }
 
 bool AudioRendererMixer::CurrentThreadIsRenderingThread() {

media/base/audio_renderer_mixer.h

@@ -48,7 +48,7 @@ class MEDIA_EXPORT AudioRendererMixer
 
   void SetPauseDelayForTesting(base::TimeDelta delay);
 
-  OutputDeviceInfo GetOutputDeviceInfo();
+  void GetOutputDeviceInfoAsync(AudioRendererSink::OutputDeviceInfoCB info_cb);
 
   // Returns true if called on rendering thread, otherwise false.
   bool CurrentThreadIsRenderingThread();

media/base/audio_renderer_mixer_input.cc

@@ -24,14 +24,22 @@ AudioRendererMixerInput::AudioRendererMixerInput(
       owner_id_(owner_id),
       device_id_(device_id),
       latency_(latency),
-      error_cb_(base::Bind(&AudioRendererMixerInput::OnRenderError,
-                           base::Unretained(this))) {
+      error_cb_(base::BindRepeating(&AudioRendererMixerInput::OnRenderError,
+                                    // Unretained is safe here because Stop()
+                                    // must always be called before destruction.
+                                    base::Unretained(this))) {
   DCHECK(mixer_pool_);
 }
 
 AudioRendererMixerInput::~AudioRendererMixerInput() {
+  // Note: This may not happen on the thread the sink was used. E.g., this may
+  // end up destroyed on the render thread despite being used on the media
+  // thread.
+
   DCHECK(!started_);
   DCHECK(!mixer_);
+  if (sink_)
+    sink_->Stop();
 }
 
 void AudioRendererMixerInput::Initialize(
@@ -41,6 +49,12 @@ void AudioRendererMixerInput::Initialize(
   DCHECK(!mixer_);
   DCHECK(callback);
 
+  // Current usage ensures we always call GetOutputDeviceInfoAsync() and wait
+  // for the result before calling this method. We could add support for doing
+  // otherwise here, but it's not needed for now, so for simplicity just DCHECK.
+  DCHECK(sink_);
+  DCHECK(device_info_);
+
   params_ = params;
   callback_ = callback;
 }
@@ -50,13 +64,13 @@ void AudioRendererMixerInput::Start() {
   DCHECK(!mixer_);
   DCHECK(callback_);  // Initialized.
 
+  DCHECK(sink_);
+  DCHECK(device_info_);
+  DCHECK_EQ(device_info_->device_status(), OUTPUT_DEVICE_STATUS_OK);
+
   started_ = true;
-  mixer_ =
-      mixer_pool_->GetMixer(owner_id_, params_, latency_, device_id_, nullptr);
-  if (!mixer_) {
-    callback_->OnRenderError();
-    return;
-  }
+  mixer_ = mixer_pool_->GetMixer(owner_id_, params_, latency_, *device_info_,
+                                 std::move(sink_));
 
   // Note: OnRenderError() may be called immediately after this call returns.
   mixer_->AddErrorCallback(error_cb_);
@@ -102,15 +116,30 @@ bool AudioRendererMixerInput::SetVolume(double volume) {
 }
 
 OutputDeviceInfo AudioRendererMixerInput::GetOutputDeviceInfo() {
-  return mixer_ ? mixer_->GetOutputDeviceInfo()
-                : mixer_pool_->GetOutputDeviceInfo(
-                      owner_id_, 0 /* session_id */, device_id_);
+  NOTREACHED();  // The blocking API is intentionally not supported.
+  return OutputDeviceInfo();
 }
 
 void AudioRendererMixerInput::GetOutputDeviceInfoAsync(
     OutputDeviceInfoCB info_cb) {
-  // TODO(dalecurtis): Implement this for https://crbug.com/905506.
-  NOTREACHED();
+  // If we device information and for a current sink or mixer, just return it
+  // immediately.
+  if (device_info_.has_value() && (sink_ || mixer_)) {
+    std::move(info_cb).Run(*device_info_);
+    return;
+  }
+
+  // We may have |device_info_|, but a Stop() has been called since if we don't
+  // have a |sink_| or a |mixer_|, so request the information again in case it
+  // has changed (which may occur due to browser side device changes).
+  device_info_.reset();
+
+  // If we don't have a sink yet start the process of getting one. Retain a ref
+  // to this sink to ensure it is not destructed while this occurs.
+  sink_ = mixer_pool_->GetSink(owner_id_, device_id_);
+  sink_->GetOutputDeviceInfoAsync(
+      base::BindOnce(&AudioRendererMixerInput::OnDeviceInfoReceived,
+                     base::RetainedRef(this), std::move(info_cb)));
 }
 
 bool AudioRendererMixerInput::IsOptimizedForHardwareParameters() {
@@ -129,38 +158,14 @@ void AudioRendererMixerInput::SwitchOutputDevice(
     return;
   }
 
-  if (mixer_) {
-    OutputDeviceStatus new_mixer_status = OUTPUT_DEVICE_STATUS_ERROR_INTERNAL;
-    AudioRendererMixer* new_mixer = mixer_pool_->GetMixer(
-        owner_id_, params_, latency_, device_id, &new_mixer_status);
-    if (new_mixer_status != OUTPUT_DEVICE_STATUS_OK) {
-      std::move(callback).Run(new_mixer_status);
-      return;
-    }
-
-    bool was_playing = playing_;
-    Stop();
-    device_id_ = device_id;
-    mixer_ = new_mixer;
-    mixer_->AddErrorCallback(error_cb_);
-    started_ = true;
-
-    if (was_playing)
-      Play();
-
-  } else {
-    OutputDeviceStatus new_mixer_status =
-        mixer_pool_
-            ->GetOutputDeviceInfo(owner_id_, 0 /* session_id */, device_id)
-            .device_status();
-    if (new_mixer_status != OUTPUT_DEVICE_STATUS_OK) {
-      std::move(callback).Run(new_mixer_status);
-      return;
-    }
-    device_id_ = device_id;
-  }
-
-  std::move(callback).Run(OUTPUT_DEVICE_STATUS_OK);
+  // Request a new sink using the new device id. This process may fail, so to
+  // avoid interrupting working audio, don't set any class variables until we
+  // know it's a success. Retain a ref to this sink to ensure it is not
+  // destructed while this occurs.
+  auto new_sink = mixer_pool_->GetSink(owner_id_, device_id);
+  new_sink->GetOutputDeviceInfoAsync(
+      base::BindOnce(&AudioRendererMixerInput::OnDeviceSwitchReady,
+                     base::RetainedRef(this), std::move(callback), new_sink));
 }
 
 double AudioRendererMixerInput::ProvideInput(AudioBus* audio_bus,
@@ -191,4 +196,42 @@ void AudioRendererMixerInput::OnRenderError() {
   callback_->OnRenderError();
 }
 
+void AudioRendererMixerInput::OnDeviceInfoReceived(
+    OutputDeviceInfoCB info_cb,
+    OutputDeviceInfo device_info) {
+  device_info_ = device_info;
+  std::move(info_cb).Run(*device_info_);
+}
+
+void AudioRendererMixerInput::OnDeviceSwitchReady(
+    OutputDeviceStatusCB switch_cb,
+    scoped_refptr<AudioRendererSink> sink,
+    OutputDeviceInfo device_info) {
+  if (device_info.device_status() != OUTPUT_DEVICE_STATUS_OK) {
+    sink->Stop();
+    std::move(switch_cb).Run(device_info.device_status());
+    return;
+  }
+
+  const bool has_mixer = !!mixer_;
+  const bool is_playing = playing_;
+
+  // This may occur if Start() hasn't yet been called.
+  if (sink_)
+    sink_->Stop();
+
+  sink_ = std::move(sink);
+  device_info_ = device_info;
+  device_id_ = device_info.device_id();
+
+  Stop();
+  if (has_mixer) {
+    Start();
+    if (is_playing)
+      Play();
+  }
+
+  std::move(switch_cb).Run(device_info.device_status());
+}
+
 }  // namespace media

media/base/audio_renderer_mixer_input.h

@@ -80,9 +80,27 @@ class MEDIA_EXPORT AudioRendererMixerInput
   bool playing_ = false;
   double volume_ GUARDED_BY(volume_lock_) = 1.0;
 
+  scoped_refptr<AudioRendererSink> sink_;
+  base::Optional<OutputDeviceInfo> device_info_;
+
   // AudioConverter::InputCallback implementation.
   double ProvideInput(AudioBus* audio_bus, uint32_t frames_delayed) override;
 
+  void OnDeviceInfoReceived(OutputDeviceInfoCB info_cb,
+                            OutputDeviceInfo device_info);
+
+  // Method to help handle device changes. Must be static to ensure we can still
+  // execute the |switch_cb| even if the pipeline is destructed. Restarts (if
+  // necessary) Start() and Play() state with a new |sink| and |device_info|.
+  //
+  // |switch_cb| is the callback given to the SwitchOutputDevice() call.
+  // |sink| is a fresh sink which should be used if device info is good.
+  // |device_info| is the OutputDeviceInfo for |sink| after
+  // GetOutputDeviceInfoAsync() completes.
+  void OnDeviceSwitchReady(OutputDeviceStatusCB switch_cb,
+                           scoped_refptr<AudioRendererSink> sink,
+                           OutputDeviceInfo device_info);
+
   // AudioParameters received during Initialize().
   AudioParameters params_;
 

media/base/audio_renderer_mixer_pool.h

@@ -13,31 +13,38 @@
 namespace media {
 class AudioParameters;
 class AudioRendererMixer;
+class AudioRendererSink;
 
 // Provides AudioRendererMixer instances for shared usage.
 // Thread safe.
 class MEDIA_EXPORT AudioRendererMixerPool {
  public:
-  AudioRendererMixerPool() {}
-  virtual ~AudioRendererMixerPool() {}
+  AudioRendererMixerPool() = default;
+  virtual ~AudioRendererMixerPool() = default;
 
   // Obtains a pointer to mixer instance based on AudioParameters. The pointer
   // is guaranteed to be valid (at least) until it's rereleased by a call to
   // ReturnMixer().
-  virtual AudioRendererMixer* GetMixer(int owner_id,
-                                       const AudioParameters& params,
-                                       AudioLatency::LatencyType latency,
-                                       const std::string& device_id,
-                                       OutputDeviceStatus* device_status) = 0;
+  //
+  // Ownership of |sink| must be passed to GetMixer(), it will be stopped and
+  // discard if an existing mixer can be reused. Clients must have called
+  // GetOutputDeviceInfoAsync() on |sink| to get |sink_info|, and it must have
+  // a device_status() == OUTPUT_DEVICE_STATUS_OK.
+  virtual AudioRendererMixer* GetMixer(
+      int owner_id,
+      const AudioParameters& input_params,
+      AudioLatency::LatencyType latency,
+      const OutputDeviceInfo& sink_info,
+      scoped_refptr<AudioRendererSink> sink) = 0;
 
   // Returns mixer back to the pool, must be called when the mixer is not needed
   // any more to avoid memory leakage.
   virtual void ReturnMixer(AudioRendererMixer* mixer) = 0;
 
-  // Returns output device information
-  virtual OutputDeviceInfo GetOutputDeviceInfo(
+  // Returns an AudioRendererSink for use with GetMixer(). Inputs must call this
+  // to get a sink to use with a subsequent GetMixer()
+  virtual scoped_refptr<AudioRendererSink> GetSink(
       int owner_id,
-      int session_id,
       const std::string& device_id) = 0;
 
  private:

media/base/audio_renderer_mixer_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/synchronization/waitable_event.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/threading/platform_thread.h"
 #include "media/base/audio_renderer_mixer_input.h"
 #include "media/base/audio_renderer_mixer_pool.h"
@@ -55,7 +56,7 @@ using AudioRendererMixerTestData =
 
 class AudioRendererMixerTest
     : public testing::TestWithParam<AudioRendererMixerTestData>,
-      AudioRendererMixerPool {
+      public AudioRendererMixerPool {
  public:
   AudioRendererMixerTest()
       : epsilon_(std::get<3>(GetParam())), half_fill_(false) {
@@ -93,8 +94,8 @@ class AudioRendererMixerTest
   AudioRendererMixer* GetMixer(int owner_id,
                                const AudioParameters& params,
                                AudioLatency::LatencyType latency,
-                               const std::string& device_id,
-                               OutputDeviceStatus* device_status) final {
+                               const OutputDeviceInfo& sink_info,
+                               scoped_refptr<AudioRendererSink> sink) final {
     return mixer_.get();
   };
 
@@ -102,8 +103,11 @@ class AudioRendererMixerTest
     EXPECT_EQ(mixer_.get(), mixer);
   }
 
-  MOCK_METHOD3(GetOutputDeviceInfo,
-               OutputDeviceInfo(int, int, const std::string&));
+  scoped_refptr<AudioRendererSink> GetSink(
+      int owner_id,
+      const std::string& device_id) override {
+    return sink_;
+  }
 
   void InitializeInputs(int inputs_per_sample_rate) {
     mixer_inputs_.reserve(inputs_per_sample_rate * input_parameters_.size());
@@ -334,15 +338,20 @@ class AudioRendererMixerTest
   }
 
   scoped_refptr<AudioRendererMixerInput> CreateMixerInput() {
-    return new AudioRendererMixerInput(this,
-                                       // Zero frame id, default device ID.
-                                       0, std::string(),
-                                       AudioLatency::LATENCY_PLAYBACK);
+    auto input = base::MakeRefCounted<AudioRendererMixerInput>(
+        this,
+        // Zero frame id, default device ID.
+        0, std::string(), AudioLatency::LATENCY_PLAYBACK);
+    input->GetOutputDeviceInfoAsync(
+        base::DoNothing());  // Primes input, needed for tests.
+    task_env_.RunUntilIdle();
+    return input;
   }
 
  protected:
   virtual ~AudioRendererMixerTest() = default;
 
+  base::test::ScopedTaskEnvironment task_env_;
   scoped_refptr<MockAudioRendererSink> sink_;
   std::unique_ptr<AudioRendererMixer> mixer_;
   AudioRendererSink::RenderCallback* mixer_callback_;
@@ -350,7 +359,7 @@ class AudioRendererMixerTest
   AudioParameters output_parameters_;
   std::unique_ptr<AudioBus> audio_bus_;
   std::unique_ptr<AudioBus> expected_audio_bus_;
-  std::vector< scoped_refptr<AudioRendererMixerInput> > mixer_inputs_;
+  std::vector<scoped_refptr<AudioRendererMixerInput>> mixer_inputs_;
   std::vector<std::unique_ptr<FakeAudioRenderCallback>> fake_callbacks_;
   std::unique_ptr<FakeAudioRenderCallback> expected_callback_;
   double epsilon_;

media/blink/webaudiosourceprovider_impl.cc

@@ -18,7 +18,6 @@
 #include "base/thread_annotations.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "media/audio/null_audio_sink.h"
 #include "media/base/audio_timestamp_helper.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/media_log.h"
@@ -197,22 +196,6 @@ void WebAudioSourceProviderImpl::Initialize(const AudioParameters& params,
   base::AutoLock auto_lock(sink_lock_);
   DCHECK_EQ(state_, kStopped);
 
-  OutputDeviceStatus device_status =
-      sink_ ? sink_->GetOutputDeviceInfo().device_status()
-            : OUTPUT_DEVICE_STATUS_ERROR_NOT_FOUND;
-
-  UMA_HISTOGRAM_ENUMERATION("Media.WebAudioSourceProvider.SinkStatus",
-                            device_status, OUTPUT_DEVICE_STATUS_MAX + 1);
-
-  if (device_status != OUTPUT_DEVICE_STATUS_OK) {
-    // Since null sink is always OK, we will fall back to it once and forever.
-    if (sink_)
-      sink_->Stop();
-    sink_ = CreateFallbackSink();
-    MEDIA_LOG(ERROR, media_log_)
-        << "Output device error, falling back to null sink";
-  }
-
   tee_filter_->Initialize(renderer, params.channels(), params.sample_rate());
 
   sink_->Initialize(params, tee_filter_.get());
@@ -262,9 +245,8 @@ bool WebAudioSourceProviderImpl::SetVolume(double volume) {
 }
 
 OutputDeviceInfo WebAudioSourceProviderImpl::GetOutputDeviceInfo() {
-  base::AutoLock auto_lock(sink_lock_);
-  return sink_ ? sink_->GetOutputDeviceInfo()
-               : OutputDeviceInfo(OUTPUT_DEVICE_STATUS_ERROR_NOT_FOUND);
+  NOTREACHED();  // The blocking API is intentionally not supported.
+  return OutputDeviceInfo();
 }
 
 void WebAudioSourceProviderImpl::GetOutputDeviceInfoAsync(
@@ -324,12 +306,6 @@ void WebAudioSourceProviderImpl::OnSetFormat() {
   client_->SetFormat(tee_filter_->channels(), tee_filter_->sample_rate());
 }
 
-scoped_refptr<SwitchableAudioRendererSink>
-WebAudioSourceProviderImpl::CreateFallbackSink() {
-  // Assuming it is called on media thread.
-  return new NullAudioSink(base::ThreadTaskRunnerHandle::Get());
-}
-
 int WebAudioSourceProviderImpl::TeeFilter::Render(
     base::TimeDelta delay,
     base::TimeTicks delay_timestamp,

media/blink/webaudiosourceprovider_impl.h

@@ -80,7 +80,6 @@ class MEDIA_BLINK_EXPORT WebAudioSourceProviderImpl
   int RenderForTesting(AudioBus* audio_bus);
 
  protected:
-  virtual scoped_refptr<SwitchableAudioRendererSink> CreateFallbackSink();
   ~WebAudioSourceProviderImpl() override;
 
  private:

media/blink/webaudiosourceprovider_impl_unittest.cc

@@ -23,43 +23,10 @@ namespace media {
 namespace {
 const float kTestVolume = 0.25;
 const int kSampleRate = 48000;
-
-class WebAudioSourceProviderImplUnderTest : public WebAudioSourceProviderImpl {
- public:
-  WebAudioSourceProviderImplUnderTest(
-      scoped_refptr<SwitchableAudioRendererSink> sink)
-      : WebAudioSourceProviderImpl(std::move(sink), &media_log_),
-        fallback_sink_(new MockAudioRendererSink()) {}
-
-  MockAudioRendererSink* fallback_sink() { return fallback_sink_.get(); }
-
- protected:
-  scoped_refptr<SwitchableAudioRendererSink> CreateFallbackSink() override {
-    return fallback_sink_;
-  }
-
- private:
-  ~WebAudioSourceProviderImplUnderTest() override = default;
-  MediaLog media_log_;
-  scoped_refptr<MockAudioRendererSink> fallback_sink_;
-
-  DISALLOW_COPY_AND_ASSIGN(WebAudioSourceProviderImplUnderTest);
-};
-
-enum class WaspSinkStatus { WASP_SINK_OK, WASP_SINK_ERROR, WASP_SINK_NULL };
-
-scoped_refptr<MockAudioRendererSink> CreateWaspMockSink(WaspSinkStatus status) {
-  if (status == WaspSinkStatus::WASP_SINK_NULL)
-    return nullptr;
-  return new MockAudioRendererSink(status == WaspSinkStatus::WASP_SINK_OK
-                                       ? OUTPUT_DEVICE_STATUS_OK
-                                       : OUTPUT_DEVICE_STATUS_ERROR_INTERNAL);
-}
-
 }  // namespace
 
 class WebAudioSourceProviderImplTest
-    : public testing::TestWithParam<WaspSinkStatus>,
+    : public testing::Test,
       public blink::WebAudioSourceProviderClient {
  public:
   WebAudioSourceProviderImplTest()
@@ -68,30 +35,27 @@ class WebAudioSourceProviderImplTest
                 kSampleRate,
                 64),
         fake_callback_(0.1, kSampleRate),
-        mock_sink_(CreateWaspMockSink(GetParam())),
-        wasp_impl_(new WebAudioSourceProviderImplUnderTest(mock_sink_)),
-        expected_sink_(GetParam() == WaspSinkStatus::WASP_SINK_OK
-                           ? mock_sink_.get()
-                           : wasp_impl_->fallback_sink()) {}
+        mock_sink_(new MockAudioRendererSink()),
+        wasp_impl_(new WebAudioSourceProviderImpl(mock_sink_, &media_log_)) {}
 
   virtual ~WebAudioSourceProviderImplTest() = default;
 
   void CallAllSinkMethodsAndVerify(bool verify) {
     testing::InSequence s;
 
-    EXPECT_CALL(*expected_sink_, Start()).Times(verify);
+    EXPECT_CALL(*mock_sink_, Start()).Times(verify);
     wasp_impl_->Start();
 
-    EXPECT_CALL(*expected_sink_, Play()).Times(verify);
+    EXPECT_CALL(*mock_sink_, Play()).Times(verify);
     wasp_impl_->Play();
 
-    EXPECT_CALL(*expected_sink_, Pause()).Times(verify);
+    EXPECT_CALL(*mock_sink_, Pause()).Times(verify);
     wasp_impl_->Pause();
 
-    EXPECT_CALL(*expected_sink_, SetVolume(kTestVolume)).Times(verify);
+    EXPECT_CALL(*mock_sink_, SetVolume(kTestVolume)).Times(verify);
     wasp_impl_->SetVolume(kTestVolume);
 
-    EXPECT_CALL(*expected_sink_, Stop()).Times(verify);
+    EXPECT_CALL(*mock_sink_, Stop()).Times(verify);
     wasp_impl_->Stop();
 
     testing::Mock::VerifyAndClear(mock_sink_.get());
@@ -101,14 +65,13 @@ class WebAudioSourceProviderImplTest
     testing::InSequence s;
 
     if (client) {
-      EXPECT_CALL(*expected_sink_, Stop());
+      EXPECT_CALL(*mock_sink_, Stop());
       EXPECT_CALL(*this, SetFormat(params_.channels(), params_.sample_rate()));
     }
     wasp_impl_->SetClient(client);
     base::RunLoop().RunUntilIdle();
 
     testing::Mock::VerifyAndClear(mock_sink_.get());
-    testing::Mock::VerifyAndClear(wasp_impl_->fallback_sink());
     testing::Mock::VerifyAndClear(this);
   }
 
@@ -140,31 +103,23 @@ class WebAudioSourceProviderImplTest
     return wasp_impl_->RenderForTesting(audio_bus);
   }
 
-  void ExpectUnhealthySinkToStop() {
-    if (GetParam() == WaspSinkStatus::WASP_SINK_ERROR)
-      EXPECT_CALL(*mock_sink_.get(), Stop());
-  }
-
  protected:
   AudioParameters params_;
   FakeAudioRenderCallback fake_callback_;
+  MediaLog media_log_;
   scoped_refptr<MockAudioRendererSink> mock_sink_;
-  scoped_refptr<WebAudioSourceProviderImplUnderTest> wasp_impl_;
-  MockAudioRendererSink* expected_sink_;
+  scoped_refptr<WebAudioSourceProviderImpl> wasp_impl_;
 
   DISALLOW_COPY_AND_ASSIGN(WebAudioSourceProviderImplTest);
 };
 
-TEST_P(WebAudioSourceProviderImplTest, SetClientBeforeInitialize) {
+TEST_F(WebAudioSourceProviderImplTest, SetClientBeforeInitialize) {
   // setClient() with a NULL client should do nothing if no client is set.
   wasp_impl_->SetClient(NULL);
 
   // If |mock_sink_| is not null, it should be stopped during setClient(this).
-  // If it is unhealthy, it should also be stopped during fallback in
-  // Initialize().
   if (mock_sink_)
-    EXPECT_CALL(*mock_sink_.get(), Stop())
-        .Times(2 + static_cast<int>(GetParam()));
+    EXPECT_CALL(*mock_sink_.get(), Stop()).Times(2);
 
   wasp_impl_->SetClient(this);
   base::RunLoop().RunUntilIdle();
@@ -189,8 +144,7 @@ TEST_P(WebAudioSourceProviderImplTest, SetClientBeforeInitialize) {
 }
 
 // Verify AudioRendererSink functionality w/ and w/o a client.
-TEST_P(WebAudioSourceProviderImplTest, SinkMethods) {
-  ExpectUnhealthySinkToStop();
+TEST_F(WebAudioSourceProviderImplTest, SinkMethods) {
   wasp_impl_->Initialize(params_, &fake_callback_);
 
   // Without a client all WASP calls should fall through to the underlying sink.
@@ -202,23 +156,22 @@ TEST_P(WebAudioSourceProviderImplTest, SinkMethods) {
   CallAllSinkMethodsAndVerify(false);
 
   // Removing the client should cause WASP to revert to the underlying sink.
-  EXPECT_CALL(*expected_sink_, SetVolume(kTestVolume));
+  EXPECT_CALL(*mock_sink_, SetVolume(kTestVolume));
   SetClient(NULL);
   CallAllSinkMethodsAndVerify(true);
 }
 
 // Verify underlying sink state is restored after client removal.
-TEST_P(WebAudioSourceProviderImplTest, SinkStateRestored) {
-  ExpectUnhealthySinkToStop();
+TEST_F(WebAudioSourceProviderImplTest, SinkStateRestored) {
   wasp_impl_->Initialize(params_, &fake_callback_);
 
   // Verify state set before the client is set propagates back afterward.
-  EXPECT_CALL(*expected_sink_, Start());
+  EXPECT_CALL(*mock_sink_, Start());
   wasp_impl_->Start();
   SetClient(this);
 
-  EXPECT_CALL(*expected_sink_, SetVolume(1.0));
-  EXPECT_CALL(*expected_sink_, Start());
+  EXPECT_CALL(*mock_sink_, SetVolume(1.0));
+  EXPECT_CALL(*mock_sink_, Start());
   SetClient(NULL);
 
   // Verify state set while the client was attached propagates back afterward.
@@ -226,15 +179,14 @@ TEST_P(WebAudioSourceProviderImplTest, SinkStateRestored) {
   wasp_impl_->Play();
   wasp_impl_->SetVolume(kTestVolume);
 
-  EXPECT_CALL(*expected_sink_, SetVolume(kTestVolume));
-  EXPECT_CALL(*expected_sink_, Start());
-  EXPECT_CALL(*expected_sink_, Play());
+  EXPECT_CALL(*mock_sink_, SetVolume(kTestVolume));
+  EXPECT_CALL(*mock_sink_, Start());
+  EXPECT_CALL(*mock_sink_, Play());
   SetClient(NULL);
 }
 
 // Test the AudioRendererSink state machine and its effects on provideInput().
-TEST_P(WebAudioSourceProviderImplTest, ProvideInput) {
-  ExpectUnhealthySinkToStop();
+TEST_F(WebAudioSourceProviderImplTest, ProvideInput) {
   std::unique_ptr<AudioBus> bus1 = AudioBus::Create(params_);
   std::unique_ptr<AudioBus> bus2 = AudioBus::Create(params_);
 
@@ -320,9 +272,7 @@ TEST_P(WebAudioSourceProviderImplTest, ProvideInput) {
 }
 
 // Verify CopyAudioCB is called if registered.
-TEST_P(WebAudioSourceProviderImplTest, CopyAudioCB) {
-  ExpectUnhealthySinkToStop();
-
+TEST_F(WebAudioSourceProviderImplTest, CopyAudioCB) {
   testing::InSequence s;
   wasp_impl_->Initialize(params_, &fake_callback_);
   wasp_impl_->SetCopyAudioCallback(base::Bind(
@@ -339,11 +289,4 @@ TEST_P(WebAudioSourceProviderImplTest, CopyAudioCB) {
   testing::Mock::VerifyAndClear(mock_sink_.get());
 }
 
-INSTANTIATE_TEST_CASE_P(
-    /* prefix intentionally left blank due to only one parameterization */,
-    WebAudioSourceProviderImplTest,
-    testing::Values(WaspSinkStatus::WASP_SINK_OK,
-                    WaspSinkStatus::WASP_SINK_ERROR,
-                    WaspSinkStatus::WASP_SINK_NULL));
-
 }  // namespace media

media/renderers/BUILD.gn

@@ -39,6 +39,7 @@ source_set("renderers") {
     "//components/viz/client",
     "//gpu/command_buffer/client:gles2_interface",
     "//gpu/command_buffer/common",
+    "//media/audio",
     "//media/base",
     "//media/filters",
     "//media/video",

media/renderers/audio_renderer_impl.cc

@@ -15,11 +15,13 @@
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/power_monitor/power_monitor.h"
 #include "base/single_thread_task_runner.h"
 #include "base/time/default_tick_clock.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
+#include "media/audio/null_audio_sink.h"
 #include "media/base/audio_buffer.h"
 #include "media/base/audio_buffer_converter.h"
 #include "media/base/audio_latency.h"
@@ -367,18 +369,53 @@ void AudioRendererImpl::Initialize(DemuxerStream* stream,
 
   // If we are re-initializing playback (e.g. switching media tracks), stop the
   // sink first.
-  if (state_ == kFlushed) {
+  if (state_ == kFlushed)
     sink_->Stop();
-    audio_clock_.reset();
-  }
 
   state_ = kInitializing;
   client_ = client;
 
+  // Always post |init_cb_| because |this| could be destroyed if initialization
+  // failed.
+  init_cb_ = BindToCurrentLoop(init_cb);
+
+  // Retrieve hardware device parameters asynchronously so we don't block the
+  // media thread on synchronous IPC.
+  sink_->GetOutputDeviceInfoAsync(
+      base::BindOnce(&AudioRendererImpl::OnDeviceInfoReceived,
+                     weak_factory_.GetWeakPtr(), stream, cdm_context));
+}
+
+void AudioRendererImpl::OnDeviceInfoReceived(
+    DemuxerStream* stream,
+    CdmContext* cdm_context,
+    OutputDeviceInfo output_device_info) {
+  DVLOG(1) << __func__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(client_);
+  DCHECK(stream);
+  DCHECK_EQ(stream->type(), DemuxerStream::AUDIO);
+  DCHECK(init_cb_);
+  DCHECK_EQ(state_, kInitializing);
+
+  // Fall-back to a fake audio sink if the audio device can't be setup; this
+  // allows video playback in cases where there is no audio hardware.
+  //
+  // TODO(dalecurtis): We could disable the audio track here too.
+  UMA_HISTOGRAM_ENUMERATION("Media.AudioRendererImpl.SinkStatus",
+                            output_device_info.device_status(),
+                            OUTPUT_DEVICE_STATUS_MAX + 1);
+  if (output_device_info.device_status() != OUTPUT_DEVICE_STATUS_OK) {
+    sink_ = new NullAudioSink(task_runner_);
+    output_device_info = sink_->GetOutputDeviceInfo();
+    MEDIA_LOG(ERROR, media_log_)
+        << "Output device error, falling back to null sink. device_status="
+        << output_device_info.device_status();
+  }
+
   current_decoder_config_ = stream->audio_decoder_config();
   DCHECK(current_decoder_config_.IsValidConfig());
 
-  auto output_device_info = sink_->GetOutputDeviceInfo();
   const AudioParameters& hw_params = output_device_info.output_params();
   ChannelLayout hw_channel_layout =
       hw_params.IsValid() ? hw_params.channel_layout() : CHANNEL_LAYOUT_NONE;
@@ -391,10 +428,6 @@ void AudioRendererImpl::Initialize(DemuxerStream* stream,
   audio_decoder_stream_->set_config_change_observer(base::BindRepeating(
       &AudioRendererImpl::OnConfigChange, weak_factory_.GetWeakPtr()));
 
-  // Always post |init_cb_| because |this| could be destroyed if initialization
-  // failed.
-  init_cb_ = BindToCurrentLoop(init_cb);
-
   AudioCodec codec = stream->audio_decoder_config().codec();
   if (auto* mc = GetMediaClient())
     is_passthrough_ = mc->IsSupportedBitstreamAudioCodec(codec);
@@ -532,8 +565,13 @@ void AudioRendererImpl::Initialize(DemuxerStream* stream,
 
   last_decoded_channels_ = stream->audio_decoder_config().channels();
 
-  audio_clock_.reset(
-      new AudioClock(base::TimeDelta(), audio_parameters_.sample_rate()));
+  {
+    // Set the |audio_clock_| under lock in case this is a reinitialize and some
+    // external caller to GetWallClockTimes() exists.
+    base::AutoLock lock(lock_);
+    audio_clock_.reset(
+        new AudioClock(base::TimeDelta(), audio_parameters_.sample_rate()));
+  }
 
   audio_decoder_stream_->Initialize(
       stream,

media/renderers/audio_renderer_impl.h

@@ -125,6 +125,11 @@ class MEDIA_EXPORT AudioRendererImpl
     kPlaying
   };
 
+  // Called after hardware device information is available.
+  void OnDeviceInfoReceived(DemuxerStream* stream,
+                            CdmContext* cdm_context,
+                            OutputDeviceInfo output_device_info);
+
   // Callback from the audio decoder delivering decoded audio samples.
   void DecodedAudioReady(AudioDecoderStream::Status status,
                          const scoped_refptr<AudioBuffer>& buffer);

tools/metrics/histograms/histograms.xml

@@ -45433,6 +45433,18 @@ uploading your change for review.
   <summary>Captures statistics for various AudioRendererImpl events.</summary>
 </histogram>
 
+<histogram name="Media.AudioRendererImpl.SinkStatus" enum="OutputDeviceStatus">
+  <owner>olka@chromium.org</owner>
+  <owner>dalecurtis@chromium.org</owner>
+  <summary>
+    Status of audio sink used by AudioRendererImpl. If not OK, a NullAudioSink
+    will be used for audio output instead. This is logged for every call to
+    AudioRendererImpl::Initialize, which generally occurs once per active audio
+    session (i.e., between a play and pause). If audio track changes are ever
+    enabled, it may additionally be called for every audio track change.
+  </summary>
+</histogram>
+
 <histogram name="Media.AudioRendererIpcStreams">
   <obsolete>
     Deprecated 02/2017. No longer needed.
@@ -48692,6 +48704,9 @@ uploading your change for review.
 
 <histogram name="Media.WebAudioSourceProvider.SinkStatus"
     enum="OutputDeviceStatus">
+  <obsolete>
+    Replaced by Media.AudioRendererImpl.SinkStatus in Nov 2018.
+  </obsolete>
   <owner>olka@chromium.org</owner>
   <owner>dalecurtis@chromium.org</owner>
   <summary>