Deliver RenderCallbackErrors even when mixer inputs are paused.

See the bug for more details.  Fixes potential stuck playback.

- Separates error callback registration from mixer input addition.
- Removes |mixer_inputs_| in favor of letting AudioConverter manage.
- Adds a new unittest to verify paused inputs receive errors.

This fixes the problem implicitly because AudioRendererImpl will
deliver a decode error which shuts down the pipeline.

BUG=379374
TEST=new unittest.

Review URL: https://codereview.chromium.org/301223012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@275585 0039d316-1c4b-4281-b951-d872f2087c98

media/base/audio_converter.h

@@ -90,6 +90,8 @@ class MEDIA_EXPORT AudioConverter {
   // to each input's ProvideInput for more data.
   int ChunkSize() const;
 
+  bool empty() const { return transform_inputs_.empty(); }
+
  private:
   // Provides input to the MultiChannelResampler.  Called by the resampler when
   // more data is necessary.

media/base/audio_renderer_mixer.cc

@@ -29,44 +29,57 @@ AudioRendererMixer::~AudioRendererMixer() {
   // AudioRendererSinks must be stopped before being destructed.
   audio_sink_->Stop();
 
-  // Ensures that all mixer inputs have stopped themselves prior to destruction
-  // and have called RemoveMixerInput().
-  DCHECK_EQ(mixer_inputs_.size(), 0U);
+  // Ensure that all mixer inputs have removed themselves prior to destruction.
+  DCHECK(audio_converter_.empty());
+  DCHECK_EQ(error_callbacks_.size(), 0U);
 }
 
-void AudioRendererMixer::AddMixerInput(AudioConverter::InputCallback* input,
-                                       const base::Closure& error_cb) {
-  base::AutoLock auto_lock(mixer_inputs_lock_);
-
+void AudioRendererMixer::AddMixerInput(AudioConverter::InputCallback* input) {
+  base::AutoLock auto_lock(lock_);
   if (!playing_) {
     playing_ = true;
     last_play_time_ = base::TimeTicks::Now();
     audio_sink_->Play();
   }
 
-  DCHECK(mixer_inputs_.find(input) == mixer_inputs_.end());
-  mixer_inputs_[input] = error_cb;
   audio_converter_.AddInput(input);
 }
 
 void AudioRendererMixer::RemoveMixerInput(
     AudioConverter::InputCallback* input) {
-  base::AutoLock auto_lock(mixer_inputs_lock_);
+  base::AutoLock auto_lock(lock_);
   audio_converter_.RemoveInput(input);
+}
 
-  DCHECK(mixer_inputs_.find(input) != mixer_inputs_.end());
-  mixer_inputs_.erase(input);
+void AudioRendererMixer::AddErrorCallback(const base::Closure& error_cb) {
+  base::AutoLock auto_lock(lock_);
+  error_callbacks_.push_back(error_cb);
+}
+
+void AudioRendererMixer::RemoveErrorCallback(const base::Closure& error_cb) {
+  base::AutoLock auto_lock(lock_);
+  for (ErrorCallbackList::iterator it = error_callbacks_.begin();
+       it != error_callbacks_.end();
+       ++it) {
+    if (it->Equals(error_cb)) {
+      error_callbacks_.erase(it);
+      return;
+    }
+  }
+
+  // An error callback should always exist when called.
+  NOTREACHED();
 }
 
 int AudioRendererMixer::Render(AudioBus* audio_bus,
                                int audio_delay_milliseconds) {
-  base::AutoLock auto_lock(mixer_inputs_lock_);
+  base::AutoLock auto_lock(lock_);
 
   // If there are no mixer inputs and we haven't seen one for a while, pause the
   // sink to avoid wasting resources when media elements are present but remain
   // in the pause state.
   const base::TimeTicks now = base::TimeTicks::Now();
-  if (!mixer_inputs_.empty()) {
+  if (!audio_converter_.empty()) {
     last_play_time_ = now;
   } else if (now - last_play_time_ >= pause_delay_ && playing_) {
     audio_sink_->Pause();
@@ -79,12 +92,12 @@ int AudioRendererMixer::Render(AudioBus* audio_bus,
 }
 
 void AudioRendererMixer::OnRenderError() {
-  base::AutoLock auto_lock(mixer_inputs_lock_);
-
   // Call each mixer input and signal an error.
-  for (AudioRendererMixerInputSet::iterator it = mixer_inputs_.begin();
-       it != mixer_inputs_.end(); ++it) {
-    it->second.Run();
+  base::AutoLock auto_lock(lock_);
+  for (ErrorCallbackList::const_iterator it = error_callbacks_.begin();
+       it != error_callbacks_.end();
+       ++it) {
+    it->Run();
   }
 }
 

media/base/audio_renderer_mixer.h

@@ -26,10 +26,15 @@ class MEDIA_EXPORT AudioRendererMixer
   virtual ~AudioRendererMixer();
 
   // Add or remove a mixer input from mixing; called by AudioRendererMixerInput.
-  void AddMixerInput(AudioConverter::InputCallback* input,
-                     const base::Closure& error_cb);
+  void AddMixerInput(AudioConverter::InputCallback* input);
   void RemoveMixerInput(AudioConverter::InputCallback* input);
 
+  // Since errors may occur even when no inputs are playing, an error callback
+  // must be registered separately from adding a mixer input.  The same callback
+  // must be given to both the functions.
+  void AddErrorCallback(const base::Closure& error_cb);
+  void RemoveErrorCallback(const base::Closure& error_cb);
+
   void set_pause_delay_for_testing(base::TimeDelta delay) {
     pause_delay_ = delay;
   }
@@ -43,12 +48,12 @@ class MEDIA_EXPORT AudioRendererMixer
   // Output sink for this mixer.
   scoped_refptr<AudioRendererSink> audio_sink_;
 
-  // Set of mixer inputs to be mixed by this mixer.  Access is thread-safe
-  // through |mixer_inputs_lock_|.
-  typedef std::map<AudioConverter::InputCallback*, base::Closure>
-      AudioRendererMixerInputSet;
-  AudioRendererMixerInputSet mixer_inputs_;
-  base::Lock mixer_inputs_lock_;
+  // ---------------[ All variables below protected by |lock_| ]---------------
+  base::Lock lock_;
+
+  // List of error callbacks used by this mixer.
+  typedef std::list<base::Closure> ErrorCallbackList;
+  ErrorCallbackList error_callbacks_;
 
   // Handles mixing and resampling between input and output parameters.
   AudioConverter audio_converter_;

media/base/audio_renderer_mixer_input.cc

@@ -25,8 +25,10 @@ AudioRendererMixerInput::AudioRendererMixerInput(
 
 AudioRendererMixerInput::~AudioRendererMixerInput() {
   // Mixer is no longer safe to use after |remove_mixer_cb_| has been called.
-  if (initialized_)
+  if (initialized_) {
+    mixer_->RemoveErrorCallback(error_cb_);
     remove_mixer_cb_.Run(params_);
+  }
 }
 
 void AudioRendererMixerInput::Initialize(
@@ -35,6 +37,7 @@ void AudioRendererMixerInput::Initialize(
   DCHECK(!initialized_);
   params_ = params;
   mixer_ = get_mixer_cb_.Run(params_);
+  mixer_->AddErrorCallback(error_cb_);
   callback_ = callback;
   initialized_ = true;
 }
@@ -60,7 +63,7 @@ void AudioRendererMixerInput::Play() {
   if (playing_)
     return;
 
-  mixer_->AddMixerInput(this, error_cb_);
+  mixer_->AddMixerInput(this);
   playing_ = true;
 }
 

media/base/audio_renderer_mixer_unittest.cc

@@ -51,8 +51,8 @@ class AudioRendererMixerTest
         std::tr1::get<1>(GetParam()), 16, kLowLatencyBufferSize);
 
     sink_ = new MockAudioRendererSink();
-    EXPECT_CALL(*sink_.get(), Start());
-    EXPECT_CALL(*sink_.get(), Stop());
+    EXPECT_CALL(*sink_, Start());
+    EXPECT_CALL(*sink_, Stop());
 
     mixer_.reset(new AudioRendererMixer(
         input_parameters_, output_parameters_, sink_));
@@ -393,6 +393,20 @@ TEST_P(AudioRendererMixerBehavioralTest, OnRenderError) {
     mixer_inputs_[i]->Stop();
 }
 
+TEST_P(AudioRendererMixerBehavioralTest, OnRenderErrorPausedInput) {
+  InitializeInputs(kMixerInputs);
+
+  for (size_t i = 0; i < mixer_inputs_.size(); ++i)
+    EXPECT_CALL(*fake_callbacks_[i], OnRenderError()).Times(1);
+
+  // Fire the error before attaching any inputs.  Ensure an error is recieved
+  // even if the input is not connected.
+  mixer_callback_->OnRenderError();
+
+  for (size_t i = 0; i < mixer_inputs_.size(); ++i)
+    mixer_inputs_[i]->Stop();
+}
+
 // Ensure constructing an AudioRendererMixerInput, but not initializing it does
 // not call RemoveMixer().
 TEST_P(AudioRendererMixerBehavioralTest, NoInitialize) {