Replace AudioDecoder::ProduceAudioSamples/ConsumeAudioSamples with read callbacks.

media/filters/audio_renderer_base.cc

Index: media/filters/audio_renderer_base.cc
diff --git a/media/filters/audio_renderer_base.cc b/media/filters/audio_renderer_base.cc
index 7960b07e7db3a5c0e02cb1cb68157c4b2a463752..57bb9fe1288e7f71db99c995a6061050013a3321 100644
--- a/media/filters/audio_renderer_base.cc
+++ b/media/filters/audio_renderer_base.cc
@@ -15,15 +15,13 @@
 
 namespace media {
 
-// Upper bound on the number of pending AudioDecoder reads.
-// TODO(acolwell): Experiment with reducing this to 1.
-const size_t kMaxPendingReads = 4;
-
 AudioRendererBase::AudioRendererBase()
     : state_(kUninitialized),
+      pending_read_(false),
       recieved_end_of_stream_(false),
       rendered_end_of_stream_(false),
-      pending_reads_(0) {
+      read_cb_(base::Bind(&AudioRendererBase::DecodedAudioReady,
+                          base::Unretained(this))) {
 }
 
 AudioRendererBase::~AudioRendererBase() {
@@ -45,8 +43,8 @@ void AudioRendererBase::Pause(const base::Closure& callback) {
   pause_callback_ = callback;
   state_ = kPaused;
 
-  // We'll only pause when we've finished all pending reads.
-  if (pending_reads_ == 0) {
+  // Pause only when we've completed our pending read.
+  if (!pending_read_) {
     pause_callback_.Run();
     pause_callback_.Reset();
   } else {
@@ -69,7 +67,7 @@ void AudioRendererBase::Stop(const base::Closure& callback) {
 void AudioRendererBase::Seek(base::TimeDelta time, const FilterStatusCB& cb) {
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(kPaused, state_);
-  DCHECK_EQ(0u, pending_reads_) << "Pending reads should have completed";
+  DCHECK(!pending_read_) << "Pending read must complete before seeking";
   DCHECK(seek_cb_.is_null());
   state_ = kSeeking;
   seek_cb_ = cb;
@@ -94,11 +92,6 @@ void AudioRendererBase::Initialize(AudioDecoder* decoder,
   decoder_ = decoder;
   underflow_callback_ = underflow_callback;
 
-  // Use base::Unretained() as the decoder doesn't need to ref us.
-  decoder_->set_consume_audio_samples_callback(
-      base::Bind(&AudioRendererBase::ConsumeAudioSamples,
-                 base::Unretained(this)));
-
   // Create a callback so our algorithm can request more reads.
   base::Closure cb = base::Bind(&AudioRendererBase::ScheduleRead_Locked, this);
 
@@ -144,15 +137,13 @@ void AudioRendererBase::ResumeAfterUnderflow(bool buffer_more_audio) {
   }
 }
 
-void AudioRendererBase::ConsumeAudioSamples(scoped_refptr<Buffer> buffer_in) {
+void AudioRendererBase::DecodedAudioReady(scoped_refptr<Buffer> buffer) {
   base::AutoLock auto_lock(lock_);
   DCHECK(state_ == kPaused || state_ == kSeeking || state_ == kPlaying ||
-         state_ == kUnderflow || state_ == kRebuffering ||
-         state_ == kStopped);
-  if (!pending_reads_)

[scherkus (not reviewing), 2011/12/19 19:30:13]

vrk: do you know if replacing this with a CHECK() is OK?

under which circumstances did ConsumeAudioSamples() get called when there wasn't
a pending read?

[vrk (LEFT CHROMIUM), 2011/12/20 01:22:00]

Bah, spent a long time looking through the code today to try to figure it out.
The best answer I can come up with is: if you're 100% sure it's correct to
replace |pending_reads_| with |pending_read_|, then it is correct to add the
CHECK here.
I believe the only ways for |pending_read_| to be false at that time is if a)
DecodedAudioReady is called before ScheduleRead_Locked, or b) the
ScheduleRead_Locked and DecodedAudioReady calls are imbalanced -- that is, the
code assumes ScheduleRead_Locked will not be called again until
DecodedAudioReady is executed, but if that assumption is false then you may
CHECK.

Scenario (a) can't happen. DecodedAudioReady is passed in as a CB to the demuxer
via ScheduleRead_Locked(), so it's not possible for it to be executed before
ScheduleRead_Locked() is executed.

Scenario (b) *shouldn't* happen, ... but I'm a little wary because I don't
understand the threading story completely.

The case I'm worried about is the following: say ScheduleRead_Locked() is called
twice in a row. This posts 2 FFmpegAudioDecoder::Read tasks in the decoder
thread. Then when these read tasks are executed, the demuxer stream queues up 2
read callbacks in its |read_queue_|. Then the decoder decodes for both of these
requests, and for each finished read request, DecodeBuffer is executed... the
first time DecodeBuffer executes, |pending_read_| is true, but the 2nd time it
executes it would CHECK.

Buuuuut yeah, I don't know if that's possible!! If there really can't be more
than one pending read at a time, then you should be able to CHECK here... but
then, wouldn't you also be able to replace |read_queue_| in FFmpegDemuxerStream
with just a |read_cb_| or something?

[scherkus (not reviewing), 2011/12/20 15:27:57]

Thanks for digging in!

Yes all read calls are supposed to be balanced and if they're not I'm more than
happy to have CHECK() calls to catch such violations.

You're correct in observing that FFmpegDemuxerStream is still in the old school
mode of expecting concurrent reads but that should go away as well.

-    return;
+         state_ == kUnderflow || state_ == kRebuffering || state_ == kStopped);
 
-  --pending_reads_;
+  CHECK(pending_read_);
+  pending_read_ = false;
 
   // TODO(scherkus): this happens due to a race, primarily because Stop() is a
   // synchronous call when it should be asynchronous and accept a callback.
@@ -163,20 +154,20 @@ void AudioRendererBase::ConsumeAudioSamples(scoped_refptr<Buffer> buffer_in) {
 
   // Don't enqueue an end-of-stream buffer because it has no data, otherwise
   // discard decoded audio data until we reach our desired seek timestamp.
-  if (buffer_in->IsEndOfStream()) {
+  if (buffer->IsEndOfStream()) {
     recieved_end_of_stream_ = true;
 
     // Transition to kPlaying if we are currently handling an underflow since no
     // more data will be arriving.
     if (state_ == kUnderflow || state_ == kRebuffering)
       state_ = kPlaying;
-  } else if (state_ == kSeeking && !buffer_in->IsEndOfStream() &&
-             (buffer_in->GetTimestamp() + buffer_in->GetDuration()) <
+  } else if (state_ == kSeeking && !buffer->IsEndOfStream() &&
+             (buffer->GetTimestamp() + buffer->GetDuration()) <
                  seek_timestamp_) {
     ScheduleRead_Locked();
   } else {
     // Note: Calling this may schedule more reads.
-    algorithm_->EnqueueBuffer(buffer_in);
+    algorithm_->EnqueueBuffer(buffer);
   }
 
   // Check for our preroll complete condition.
@@ -188,8 +179,8 @@ void AudioRendererBase::ConsumeAudioSamples(scoped_refptr<Buffer> buffer_in) {
       state_ = kPaused;
       ResetAndRunCB(&seek_cb_, PIPELINE_OK);
     }
-  } else if (state_ == kPaused && pending_reads_ == 0) {
-    // No more pending reads!  We're now officially "paused".
+  } else if (state_ == kPaused && !pending_read_) {
+    // No more pending read!  We're now officially "paused".
     if (!pause_callback_.is_null()) {
       pause_callback_.Run();
       pause_callback_.Reset();
@@ -217,6 +208,8 @@ uint32 AudioRendererBase::FillBuffer(uint8* dest,
       // 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;
       dest_written = std::min(kZeroLength, dest_len);
       memset(dest, 0, dest_written);
@@ -275,14 +268,10 @@ uint32 AudioRendererBase::FillBuffer(uint8* dest,
 
 void AudioRendererBase::ScheduleRead_Locked() {
   lock_.AssertAcquired();
-  if (pending_reads_ < kMaxPendingReads) {
-    ++pending_reads_;
-    // TODO(jiesun): We use dummy buffer to feed decoder to let decoder to
-    // provide buffer pools. In the future, we may want to implement real
-    // buffer pool to recycle buffers.
-    scoped_refptr<Buffer> buffer;
-    decoder_->ProduceAudioSamples(buffer);
-  }
+  if (pending_read_)
+    return;
+  pending_read_ = true;
+  decoder_->Read(read_cb_);
 }
 
 void AudioRendererBase::SetPlaybackRate(float playback_rate) {