Replace AudioDecoder::ProduceAudioSamples/ConsumeAudioSamples with read callbacks.

media/filters/audio_renderer_base.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/audio_renderer_base.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "media/base/filter_host.h"
 #include "media/filters/audio_renderer_algorithm_ola.h"
 
 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() {
   // Stop() should have been called and |algorithm_| should have been destroyed.
   DCHECK(state_ == kUninitialized || state_ == kStopped);
   DCHECK(!algorithm_.get());
 }
 
 void AudioRendererBase::Play(const base::Closure& callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(kPaused, state_);
   state_ = kPlaying;
   callback.Run();
 }
 
 void AudioRendererBase::Pause(const base::Closure& callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK(state_ == kPlaying || state_ == kUnderflow || state_ == kRebuffering);
   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 {
     state_ = kPaused;
   }
 }
 
 void AudioRendererBase::Stop(const base::Closure& callback) {
   OnStop();
   {
     base::AutoLock auto_lock(lock_);
     state_ = kStopped;
     algorithm_.reset(NULL);
   }
   if (!callback.is_null()) {
     callback.Run();
   }
 }
 
 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;
   seek_timestamp_ = time;
 
   // Throw away everything and schedule our reads.
   last_fill_buffer_time_ = base::TimeDelta();
   recieved_end_of_stream_ = false;
   rendered_end_of_stream_ = false;
 
   // |algorithm_| will request more reads.
   algorithm_->FlushBuffers();
 }
 
 void AudioRendererBase::Initialize(AudioDecoder* decoder,
                                    const base::Closure& init_callback,
                                    const base::Closure& underflow_callback) {
   DCHECK(decoder);
   DCHECK(!init_callback.is_null());
   DCHECK(!underflow_callback.is_null());
   DCHECK_EQ(kUninitialized, state_);
   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);
 
   // Construct the algorithm.
   algorithm_.reset(new AudioRendererAlgorithmOLA());
 
   // Initialize our algorithm with media properties, initial playback rate,
   // and a callback to request more reads from the data source.
   ChannelLayout channel_layout = decoder_->channel_layout();
   int channels = ChannelLayoutToChannelCount(channel_layout);
@@ skipping 25 matching lines @@
 void AudioRendererBase::ResumeAfterUnderflow(bool buffer_more_audio) {
   base::AutoLock auto_lock(lock_);
   if (state_ == kUnderflow) {
     if (buffer_more_audio)
       algorithm_->IncreaseQueueCapacity();
 
     state_ = kRebuffering;
   }
 }
 
-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.
   // Refer to http://crbug.com/16059
   if (state_ == kStopped) {
     return;
   }
 
   // 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.
   if (state_ == kSeeking) {
     DCHECK(!seek_cb_.is_null());
     if (algorithm_->IsQueueFull() || recieved_end_of_stream_) {
       // Transition into paused whether we have data in |algorithm_| or not.
       // FillBuffer() will play silence if there's nothing to fill.
       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();
     }
   }
 }
 
 uint32 AudioRendererBase::FillBuffer(uint8* dest,
                                      uint32 dest_len,
                                      const base::TimeDelta& playback_delay,
                                      bool buffers_empty) {
   // The timestamp of the last buffer written during the last call to
   // FillBuffer().
   base::TimeDelta last_fill_buffer_time;
   size_t dest_written = 0;
   {
     base::AutoLock auto_lock(lock_);
 
     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;
       dest_written = std::min(kZeroLength, dest_len);
       memset(dest, 0, dest_written);
       return dest_written;
     }
 
     // Save a local copy of last fill buffer time and reset the member.
     last_fill_buffer_time = last_fill_buffer_time_;
     last_fill_buffer_time_ = base::TimeDelta();
 
@@ skipping 38 matching lines @@
     // finer time update events.
     last_fill_buffer_time -= playback_delay;
     host()->SetTime(last_fill_buffer_time);
   }
 
   return dest_written;
 }
 
 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) {
   algorithm_->set_playback_rate(playback_rate);
 }
 
 float AudioRendererBase::GetPlaybackRate() {
   return algorithm_->playback_rate();
 }
 
 }  // namespace media