Track ARI sink state separate from ARI state.

Due to how SetPlaybackRate() was implemented it was possible for
the sink state to be "paused" while the ARI state is "playing."

This leads to duplicate Play() and Pause() calls when playback
rate changes occur.  Compounding this is the fact that Pipeline
doesn't have a concept of "Pause" it merely sets playback rate to
zero.

I also fixed these additional issues:
- Duplicate Pause() calls when in Preroll().
- Removed the return of muted data on non-playing states during
Render().  Now handled implicitly by FromInterleaved and later
by AudioRendererMixer.

Found by new DCHECKs in https://codereview.chromium.org/14256009/

BUG=none
TEST=media_unittests, layout tests.
R=acolwell@chromium.org

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

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

3 files changed, 60 insertions, 72 deletions

diff --git a/media/filters/audio_renderer_impl.cc b/media/filters/audio_renderer_impl.cc
index fb5585f..f791eb5 100644
--- a/media/filters/audio_renderer_impl.cc
+++ b/media/filters/audio_renderer_impl.cc
@@ -52,6 +52,7 @@ AudioRendererImpl::AudioRendererImpl(
           message_loop, decoders.Pass(), set_decryptor_ready_cb)),
       now_cb_(base::Bind(&base::Time::Now)),
       state_(kUninitialized),
+      sink_playing_(false),
       pending_read_(false),
       received_end_of_stream_(false),
       rendered_end_of_stream_(false),
@@ -71,30 +72,31 @@ AudioRendererImpl::~AudioRendererImpl() {
 void AudioRendererImpl::Play(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
-  float playback_rate = 0;
   {
     base::AutoLock auto_lock(lock_);
-    DCHECK_EQ(kPaused, state_);
+    DCHECK_EQ(state_, kPaused);
     state_ = kPlaying;
     callback.Run();
-    playback_rate = algorithm_->playback_rate();
+    earliest_end_time_ = now_cb_.Run();
   }
 
-  if (playback_rate != 0.0f) {
+  if (algorithm_->playback_rate() != 0)
     DoPlay();
-  } else {
-    DoPause();
-  }
+  else
+    DCHECK(!sink_playing_);
 }
 
 void AudioRendererImpl::DoPlay() {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(sink_);
   {
     base::AutoLock auto_lock(lock_);
     earliest_end_time_ = now_cb_.Run();
   }
-  sink_->Play();
+
+  if (state_ == kPlaying && !sink_playing_) {
+    sink_->Play();
+    sink_playing_ = true;
+  }
 }
 
 void AudioRendererImpl::Pause(const base::Closure& callback) {
@@ -103,7 +105,7 @@ void AudioRendererImpl::Pause(const base::Closure& callback) {
   {
     base::AutoLock auto_lock(lock_);
     DCHECK(state_ == kPlaying || state_ == kUnderflow ||
-           state_ == kRebuffering);
+           state_ == kRebuffering) << "state_ == " << state_;
     pause_cb_ = callback;
     state_ = kPaused;
 
@@ -117,8 +119,10 @@ void AudioRendererImpl::Pause(const base::Closure& callback) {
 
 void AudioRendererImpl::DoPause() {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(sink_);
-  sink_->Pause();
+  if (sink_playing_) {
+    sink_->Pause();
+    sink_playing_ = false;
+  }
 }
 
 void AudioRendererImpl::Flush(const base::Closure& callback) {
@@ -165,33 +169,30 @@ void AudioRendererImpl::Stop(const base::Closure& callback) {
 void AudioRendererImpl::Preroll(base::TimeDelta time,
                                 const PipelineStatusCB& cb) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK(sink_);
 
-  {
-    base::AutoLock auto_lock(lock_);
-    DCHECK_EQ(kPaused, state_);
-    DCHECK(!pending_read_) << "Pending read must complete before seeking";
-    DCHECK(pause_cb_.is_null());
-    DCHECK(preroll_cb_.is_null());
-    state_ = kPrerolling;
-    preroll_cb_ = cb;
-    preroll_timestamp_ = time;
-
-    // Throw away everything and schedule our reads.
-    audio_time_buffered_ = kNoTimestamp();
-    current_time_ = kNoTimestamp();
-    received_end_of_stream_ = false;
-    rendered_end_of_stream_ = false;
-    preroll_aborted_ = false;
-
-    splicer_->Reset();
-    algorithm_->FlushBuffers();
-    earliest_end_time_ = now_cb_.Run();
-
-    AttemptRead_Locked();
-  }
+  base::AutoLock auto_lock(lock_);
+  DCHECK(!sink_playing_);
+  DCHECK_EQ(state_, kPaused);
+  DCHECK(!pending_read_) << "Pending read must complete before seeking";
+  DCHECK(pause_cb_.is_null());
+  DCHECK(preroll_cb_.is_null());
+
+  state_ = kPrerolling;
+  preroll_cb_ = cb;
+  preroll_timestamp_ = time;
+
+  // Throw away everything and schedule our reads.
+  audio_time_buffered_ = kNoTimestamp();
+  current_time_ = kNoTimestamp();
+  received_end_of_stream_ = false;
+  rendered_end_of_stream_ = false;
+  preroll_aborted_ = false;
+
+  splicer_->Reset();
+  algorithm_->FlushBuffers();
+  earliest_end_time_ = now_cb_.Run();
 
-  sink_->Pause();
+  AttemptRead_Locked();
 }
 
 void AudioRendererImpl::Initialize(DemuxerStream* stream,
@@ -304,6 +305,10 @@ void AudioRendererImpl::OnDecoderSelected(
   sink_->Initialize(audio_parameters_, weak_this_);
   sink_->Start();
 
+  // Some sinks play on start...
+  sink_->Pause();
+  DCHECK(!sink_playing_);
+
   base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
 }
 
@@ -458,19 +463,17 @@ bool AudioRendererImpl::CanRead_Locked() {
 
 void AudioRendererImpl::SetPlaybackRate(float playback_rate) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_LE(0.0f, playback_rate);
+  DCHECK_GE(playback_rate, 0);
   DCHECK(sink_);
 
   // We have two cases here:
-  // Play: current_playback_rate == 0.0 && playback_rate != 0.0
-  // Pause: current_playback_rate != 0.0 && playback_rate == 0.0
+  // Play: current_playback_rate == 0 && playback_rate != 0
+  // Pause: current_playback_rate != 0 && playback_rate == 0
   float current_playback_rate = algorithm_->playback_rate();
-  if (current_playback_rate == 0.0f && playback_rate != 0.0f) {
+  if (current_playback_rate == 0 && playback_rate != 0)
     DoPlay();
-  } else if (current_playback_rate != 0.0f && playback_rate == 0.0f) {
-    // Pause is easy, we can always pause.
+  else if (current_playback_rate != 0 && playback_rate == 0)
     DoPause();
-  }
 
   base::AutoLock auto_lock(lock_);
   algorithm_->SetPlaybackRate(playback_rate);
@@ -520,26 +523,15 @@ uint32 AudioRendererImpl::FillBuffer(uint8* dest,
       return 0;
 
     float playback_rate = algorithm_->playback_rate();
-    if (playback_rate == 0.0f)
+    if (playback_rate == 0)
       return 0;
 
     if (state_ == kRebuffering && algorithm_->IsQueueFull())
       state_ = kPlaying;
 
     // Mute audio by returning 0 when not playing.
-    if (state_ != kPlaying) {
-      // TODO(scherkus): To keep the audio hardware busy we write at most 8k of
-      // zeros.  This gets around the tricky situation of pausing and resuming
-      // the audio IPC layer in Chrome.  Ideally, we should return zero and then
-      // the subclass can restart the conversation.
-      //
-      // This should get handled by the subclass http://crbug.com/106600
-      const uint32 kZeroLength = 8192;
-      size_t zeros_to_write = std::min(
-          kZeroLength, requested_frames * audio_parameters_.GetBytesPerFrame());
-      memset(dest, 0, zeros_to_write);
-      return zeros_to_write / audio_parameters_.GetBytesPerFrame();
-    }
+    if (state_ != kPlaying)
+      return 0;
 
     // We use the following conditions to determine end of playback:
     //   1) Algorithm can not fill the audio callback buffer
diff --git a/media/filters/audio_renderer_impl.h b/media/filters/audio_renderer_impl.h
index cdadfde..8202751 100644
--- a/media/filters/audio_renderer_impl.h
+++ b/media/filters/audio_renderer_impl.h
@@ -221,6 +221,9 @@ class MEDIA_EXPORT AudioRendererImpl
   };
   State state_;
 
+  // Keep track of whether or not the sink is playing.
+  bool sink_playing_;
+
   // Keep track of our outstanding read to |decoder_|.
   bool pending_read_;
 
diff --git a/media/filters/audio_renderer_impl_unittest.cc b/media/filters/audio_renderer_impl_unittest.cc
index 271220d..4c8004c 100644
--- a/media/filters/audio_renderer_impl_unittest.cc
+++ b/media/filters/audio_renderer_impl_unittest.cc
@@ -229,10 +229,9 @@ class AudioRendererImplTest : public ::testing::Test {
     uint32 frames_read = renderer_->FillBuffer(
         buffer.get(), requested_frames, 0);
 
-    if (frames_read > 0 && muted) {
-      *muted = (buffer[0] == kMutedAudio);
-    }
-    return (frames_read == requested_frames);
+    if (muted)
+      *muted = frames_read < 1 || buffer[0] == kMutedAudio;
+    return frames_read == requested_frames;
   }
 
   // Attempts to consume all data available from the renderer.  Returns the
@@ -440,12 +439,9 @@ TEST_F(AudioRendererImplTest, Underflow) {
   renderer_->ResumeAfterUnderflow(false);
 
   // Verify after resuming that we're still not getting data.
-  //
-  // NOTE: FillBuffer() satisfies the read but returns muted audio, which
-  // is crazy http://crbug.com/106600
   bool muted = false;
   EXPECT_EQ(0u, bytes_buffered());
-  EXPECT_TRUE(ConsumeBufferedData(kDataSize, &muted));
+  EXPECT_FALSE(ConsumeBufferedData(kDataSize, &muted));
   EXPECT_TRUE(muted);
 
   // Deliver data, we should get non-muted audio.
@@ -480,12 +476,9 @@ TEST_F(AudioRendererImplTest, Underflow_EndOfStream) {
   WaitForPendingRead();
 
   // Verify we're getting muted audio during underflow.
-  //
-  // NOTE: FillBuffer() satisfies the read but returns muted audio, which
-  // is crazy http://crbug.com/106600
   bool muted = false;
   EXPECT_EQ(kDataSize, bytes_buffered());
-  EXPECT_TRUE(ConsumeBufferedData(kDataSize, &muted));
+  EXPECT_FALSE(ConsumeBufferedData(kDataSize, &muted));
   EXPECT_TRUE(muted);
 
   // Now deliver end of stream, we should get our little bit of data back.
@@ -497,7 +490,7 @@ TEST_F(AudioRendererImplTest, Underflow_EndOfStream) {
   // Attempt to read to make sure we're truly at the end of stream.
   AdvanceTime(time_until_ended);
   EXPECT_FALSE(ConsumeBufferedData(kDataSize, &muted));
-  EXPECT_FALSE(muted);
+  EXPECT_TRUE(muted);
   WaitForEnded();
 }
 
@@ -520,7 +513,7 @@ TEST_F(AudioRendererImplTest, Underflow_ResumeFromCallback) {
   // Verify after resuming that we're still not getting data.
   bool muted = false;
   EXPECT_EQ(0u, bytes_buffered());
-  EXPECT_TRUE(ConsumeBufferedData(kDataSize, &muted));
+  EXPECT_FALSE(ConsumeBufferedData(kDataSize, &muted));
   EXPECT_TRUE(muted);
 
   // Deliver data, we should get non-muted audio.