Remove support for audio renderers to disable themselves at run time.

UMA data tells us this code path is rarely hit (~0.01% on stable). It
also complicates a lot of media pipeline/demuxer logic. Replace all of
it with an error callback.

BUG=234708

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

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

9 files changed, 17 insertions, 99 deletions

diff --git a/media/filters/audio_renderer_impl.cc b/media/filters/audio_renderer_impl.cc
index 2b82162..faec58e 100644
--- a/media/filters/audio_renderer_impl.cc
+++ b/media/filters/audio_renderer_impl.cc
@@ -235,7 +235,6 @@ void AudioRendererImpl::Initialize(DemuxerStream* stream,
                                    const base::Closure& underflow_cb,
                                    const TimeCB& time_cb,
                                    const base::Closure& ended_cb,
-                                   const base::Closure& disabled_cb,
                                    const PipelineStatusCB& error_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(stream);
@@ -245,7 +244,6 @@ void AudioRendererImpl::Initialize(DemuxerStream* stream,
   DCHECK(!underflow_cb.is_null());
   DCHECK(!time_cb.is_null());
   DCHECK(!ended_cb.is_null());
-  DCHECK(!disabled_cb.is_null());
   DCHECK(!error_cb.is_null());
   DCHECK_EQ(kUninitialized, state_);
   DCHECK(sink_);
@@ -256,7 +254,6 @@ void AudioRendererImpl::Initialize(DemuxerStream* stream,
   underflow_cb_ = underflow_cb;
   time_cb_ = time_cb;
   ended_cb_ = ended_cb;
-  disabled_cb_ = disabled_cb;
   error_cb_ = error_cb;
 
   expecting_config_changes_ = stream->SupportsConfigChanges();
@@ -682,8 +679,12 @@ void AudioRendererImpl::UpdateEarliestEndTime_Locked(
 }
 
 void AudioRendererImpl::OnRenderError() {
+  // UMA data tells us this happens ~0.01% of the time. Trigger an error instead
+  // of trying to gracefully fall back to a fake sink. It's very likely
+  // OnRenderError() should be removed and the audio stack handle errors without
+  // notifying clients. See http://crbug.com/234708 for details.
   HistogramRendererEvent(RENDER_ERROR);
-  disabled_cb_.Run();
+  error_cb_.Run(PIPELINE_ERROR_DECODE);
 }
 
 void AudioRendererImpl::DisableUnderflowForTesting() {
diff --git a/media/filters/audio_renderer_impl.h b/media/filters/audio_renderer_impl.h
index 4e8ad01..cf5a58a 100644
--- a/media/filters/audio_renderer_impl.h
+++ b/media/filters/audio_renderer_impl.h
@@ -71,7 +71,6 @@ class MEDIA_EXPORT AudioRendererImpl
                           const base::Closure& underflow_cb,
                           const TimeCB& time_cb,
                           const base::Closure& ended_cb,
-                          const base::Closure& disabled_cb,
                           const PipelineStatusCB& error_cb) OVERRIDE;
   virtual void Play(const base::Closure& callback) OVERRIDE;
   virtual void Pause(const base::Closure& callback) OVERRIDE;
@@ -215,7 +214,6 @@ class MEDIA_EXPORT AudioRendererImpl
   base::Closure underflow_cb_;
   TimeCB time_cb_;
   base::Closure ended_cb_;
-  base::Closure disabled_cb_;
   PipelineStatusCB error_cb_;
 
   // Callback provided to Flush().
diff --git a/media/filters/audio_renderer_impl_unittest.cc b/media/filters/audio_renderer_impl_unittest.cc
index 8a9af82..7556fc7 100644
--- a/media/filters/audio_renderer_impl_unittest.cc
+++ b/media/filters/audio_renderer_impl_unittest.cc
@@ -118,7 +118,6 @@ class AudioRendererImplTest : public ::testing::Test {
 
   MOCK_METHOD1(OnStatistics, void(const PipelineStatistics&));
   MOCK_METHOD0(OnUnderflow, void());
-  MOCK_METHOD0(OnDisabled, void());
   MOCK_METHOD1(OnError, void(PipelineStatus));
 
   void OnAudioTimeCallback(TimeDelta current_time, TimeDelta max_time) {
@@ -137,8 +136,6 @@ class AudioRendererImplTest : public ::testing::Test {
         base::Bind(&AudioRendererImplTest::OnAudioTimeCallback,
                    base::Unretained(this)),
         ended_event_.GetClosure(),
-        base::Bind(&AudioRendererImplTest::OnDisabled,
-                   base::Unretained(this)),
         base::Bind(&AudioRendererImplTest::OnError,
                    base::Unretained(this)));
   }
@@ -997,4 +994,13 @@ TEST_F(AudioRendererImplTest, ImmediateEndOfStream) {
   WaitForEnded();
 }
 
+TEST_F(AudioRendererImplTest, OnRenderErrorCausesDecodeError) {
+  Initialize();
+  Preroll();
+  Play();
+
+  EXPECT_CALL(*this, OnError(PIPELINE_ERROR_DECODE));
+  sink_->OnRenderError();
+}
+
 }  // namespace media
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index 7503b22..43aca4f 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -1065,12 +1065,6 @@ void ChunkDemuxer::Seek(TimeDelta time, const PipelineStatusCB& cb) {
   base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 }
 
-void ChunkDemuxer::OnAudioRendererDisabled() {
-  base::AutoLock auto_lock(lock_);
-  audio_->Shutdown();
-  disabled_audio_ = audio_.Pass();
-}
-
 // Demuxer implementation.
 DemuxerStream* ChunkDemuxer::GetStream(DemuxerStream::Type type) {
   DCHECK_NE(type, DemuxerStream::TEXT);
diff --git a/media/filters/chunk_demuxer.h b/media/filters/chunk_demuxer.h
index 6586746..6324d79 100644
--- a/media/filters/chunk_demuxer.h
+++ b/media/filters/chunk_demuxer.h
@@ -160,7 +160,6 @@ class MEDIA_EXPORT ChunkDemuxer : public Demuxer {
                           bool enable_text_tracks) OVERRIDE;
   virtual void Stop(const base::Closure& callback) OVERRIDE;
   virtual void Seek(base::TimeDelta time, const PipelineStatusCB&  cb) OVERRIDE;
-  virtual void OnAudioRendererDisabled() OVERRIDE;
   virtual DemuxerStream* GetStream(DemuxerStream::Type type) OVERRIDE;
   virtual base::TimeDelta GetStartTime() const OVERRIDE;
   virtual base::Time GetTimelineOffset() const OVERRIDE;
@@ -364,9 +363,6 @@ class MEDIA_EXPORT ChunkDemuxer : public Demuxer {
   scoped_ptr<ChunkDemuxerStream> audio_;
   scoped_ptr<ChunkDemuxerStream> video_;
 
-  // Keeps |audio_| alive when audio has been disabled.
-  scoped_ptr<ChunkDemuxerStream> disabled_audio_;
-
   base::TimeDelta duration_;
 
   // The duration passed to the last SetDuration(). If
diff --git a/media/filters/chunk_demuxer_unittest.cc b/media/filters/chunk_demuxer_unittest.cc
index 3bca45f..6eb4eab 100644
--- a/media/filters/chunk_demuxer_unittest.cc
+++ b/media/filters/chunk_demuxer_unittest.cc
@@ -2902,26 +2902,6 @@ TEST_P(ChunkDemuxerTest, Shutdown_BeforeInitialize) {
   message_loop_.RunUntilIdle();
 }
 
-TEST_P(ChunkDemuxerTest, ReadAfterAudioDisabled) {
-  ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO));
-  AppendCluster(kDefaultFirstCluster());
-
-  DemuxerStream* stream = demuxer_->GetStream(DemuxerStream::AUDIO);
-  ASSERT_TRUE(stream);
-
-  // The stream should no longer be present.
-  demuxer_->OnAudioRendererDisabled();
-  ASSERT_FALSE(demuxer_->GetStream(DemuxerStream::AUDIO));
-
-  // Normally this would return an audio buffer at timestamp zero, but
-  // all reads should return EOS buffers when disabled.
-  bool audio_read_done = false;
-  stream->Read(base::Bind(&OnReadDone_EOSExpected, &audio_read_done));
-  message_loop_.RunUntilIdle();
-
-  EXPECT_TRUE(audio_read_done);
-}
-
 // Verifies that signaling end of stream while stalled at a gap
 // boundary does not trigger end of stream buffers to be returned.
 TEST_P(ChunkDemuxerTest, EndOfStreamWhileWaitingForGapToBeFilled) {
diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc
index dd41410..061d631 100644
--- a/media/filters/ffmpeg_demuxer.cc
+++ b/media/filters/ffmpeg_demuxer.cc
@@ -417,7 +417,6 @@ FFmpegDemuxer::FFmpegDemuxer(
       bitrate_(0),
       start_time_(kNoTimestamp()),
       liveness_(LIVENESS_UNKNOWN),
-      audio_disabled_(false),
       text_enabled_(false),
       duration_known_(false),
       need_key_cb_(need_key_cb),
@@ -465,17 +464,6 @@ void FFmpegDemuxer::Seek(base::TimeDelta time, const PipelineStatusCB& cb) {
           &FFmpegDemuxer::OnSeekFrameDone, weak_factory_.GetWeakPtr(), cb));
 }
 
-void FFmpegDemuxer::OnAudioRendererDisabled() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  audio_disabled_ = true;
-  StreamVector::iterator iter;
-  for (iter = streams_.begin(); iter != streams_.end(); ++iter) {
-    if (*iter && (*iter)->type() == DemuxerStream::AUDIO) {
-      (*iter)->Stop();
-    }
-  }
-}
-
 void FFmpegDemuxer::Initialize(DemuxerHost* host,
                                const PipelineStatusCB& status_cb,
                                bool enable_text_tracks) {
@@ -906,10 +894,7 @@ void FFmpegDemuxer::OnReadFrameDone(ScopedAVPacket packet, int result) {
   // Defend against ffmpeg giving us a bad stream index.
   if (packet->stream_index >= 0 &&
       packet->stream_index < static_cast<int>(streams_.size()) &&
-      streams_[packet->stream_index] &&
-      (!audio_disabled_ ||
-       streams_[packet->stream_index]->type() != DemuxerStream::AUDIO)) {
-
+      streams_[packet->stream_index]) {
     // TODO(scherkus): Fix demuxing upstream to never return packets w/o data
     // when av_read_frame() returns success code. See bug comment for ideas:
     //
@@ -1002,10 +987,8 @@ void FFmpegDemuxer::StreamHasEnded() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   StreamVector::iterator iter;
   for (iter = streams_.begin(); iter != streams_.end(); ++iter) {
-    if (!*iter ||
-        (audio_disabled_ && (*iter)->type() == DemuxerStream::AUDIO)) {
+    if (!*iter)
       continue;
-    }
     (*iter)->SetEndOfStream();
   }
 }
@@ -1025,8 +1008,7 @@ void FFmpegDemuxer::NotifyCapacityAvailable() {
 void FFmpegDemuxer::NotifyBufferingChanged() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   Ranges<base::TimeDelta> buffered;
-  FFmpegDemuxerStream* audio =
-      audio_disabled_ ? NULL : GetFFmpegStream(DemuxerStream::AUDIO);
+  FFmpegDemuxerStream* audio = GetFFmpegStream(DemuxerStream::AUDIO);
   FFmpegDemuxerStream* video = GetFFmpegStream(DemuxerStream::VIDEO);
   if (audio && video) {
     buffered = audio->GetBufferedRanges().IntersectionWith(
diff --git a/media/filters/ffmpeg_demuxer.h b/media/filters/ffmpeg_demuxer.h
index 778ab5a..02682bb 100644
--- a/media/filters/ffmpeg_demuxer.h
+++ b/media/filters/ffmpeg_demuxer.h
@@ -154,7 +154,6 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
                           bool enable_text_tracks) OVERRIDE;
   virtual void Stop(const base::Closure& callback) OVERRIDE;
   virtual void Seek(base::TimeDelta time, const PipelineStatusCB& cb) OVERRIDE;
-  virtual void OnAudioRendererDisabled() OVERRIDE;
   virtual DemuxerStream* GetStream(DemuxerStream::Type type) OVERRIDE;
   virtual base::TimeDelta GetStartTime() const OVERRIDE;
   virtual base::Time GetTimelineOffset() const OVERRIDE;
@@ -258,10 +257,6 @@ class MEDIA_EXPORT FFmpegDemuxer : public Demuxer {
   // Liveness of the stream.
   Liveness liveness_;
 
-  // Whether audio has been disabled for this demuxer (in which case this class
-  // drops packets destined for AUDIO demuxer streams on the floor).
-  bool audio_disabled_;
-
   // Whether text streams have been enabled for this demuxer.
   bool text_enabled_;
 
diff --git a/media/filters/ffmpeg_demuxer_unittest.cc b/media/filters/ffmpeg_demuxer_unittest.cc
index cab5963..a1614a4 100644
--- a/media/filters/ffmpeg_demuxer_unittest.cc
+++ b/media/filters/ffmpeg_demuxer_unittest.cc
@@ -600,40 +600,6 @@ TEST_F(FFmpegDemuxerTest, Stop) {
   demuxer_.reset();
 }
 
-TEST_F(FFmpegDemuxerTest, DisableAudioStream) {
-  // We are doing the following things here:
-  // 1. Initialize the demuxer with audio and video stream.
-  // 2. Send a "disable audio stream" message to the demuxer.
-  // 3. Demuxer will free audio packets even if audio stream was initialized.
-  CreateDemuxer("bear-320x240.webm");
-  InitializeDemuxer();
-
-  // Submit a "disable audio stream" message to the demuxer.
-  demuxer_->OnAudioRendererDisabled();
-  message_loop_.RunUntilIdle();
-
-  // Get our streams.
-  DemuxerStream* video = demuxer_->GetStream(DemuxerStream::VIDEO);
-  DemuxerStream* audio = demuxer_->GetStream(DemuxerStream::AUDIO);
-  ASSERT_TRUE(video);
-  ASSERT_TRUE(audio);
-
-  // The audio stream should have been prematurely stopped.
-  EXPECT_FALSE(IsStreamStopped(DemuxerStream::VIDEO));
-  EXPECT_TRUE(IsStreamStopped(DemuxerStream::AUDIO));
-
-  // Attempt a read from the video stream: it should return valid data.
-  video->Read(NewReadCB(FROM_HERE, 22084, 0));
-  message_loop_.Run();
-
-  // Attempt a read from the audio stream: it should immediately return end of
-  // stream without requiring the message loop to read data.
-  bool got_eos_buffer = false;
-  audio->Read(base::Bind(&EosOnReadDone, &got_eos_buffer));
-  message_loop_.RunUntilIdle();
-  EXPECT_TRUE(got_eos_buffer);
-}
-
 // Verify that seek works properly when the WebM cues data is at the start of
 // the file instead of at the end.
 TEST_F(FFmpegDemuxerTest, SeekWithCuesBeforeFirstCluster) {