Fix ChunkDemuxer seek deadlock

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 049a39a1005856fc38032ddc3da4b879636bccf5..c7f556290b62d171b49f9da6d5c35725b7534364 100644
--- a/media/filters/audio_renderer_base.cc
+++ b/media/filters/audio_renderer_base.cc
@@ -67,6 +67,7 @@ void AudioRendererBase::Seek(base::TimeDelta time, const FilterStatusCB& cb) {
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(kPaused, state_);
   DCHECK(!pending_read_) << "Pending read must complete before seeking";
+  DCHECK(pause_callback_.is_null());
   DCHECK(seek_cb_.is_null());
   state_ = kSeeking;
   seek_cb_ = cb;
@@ -144,46 +145,43 @@ void AudioRendererBase::DecodedAudioReady(scoped_refptr<Buffer> buffer) {
   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->IsEndOfStream()) {
+  if (buffer && buffer->IsEndOfStream()) {
     recieved_end_of_stream_ = true;
 
-    // Transition to kPlaying if we are currently handling an underflow since no
-    // more data will be arriving.
+    // 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->IsEndOfStream() &&
-             (buffer->GetTimestamp() + buffer->GetDuration()) <
-                 seek_timestamp_) {
-    ScheduleRead_Locked();
-  } else {
-    // Note: Calling this may schedule more reads.
-    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_read_) {
-    // No more pending read!  We're now officially "paused".
-    if (!pause_callback_.is_null()) {
-      pause_callback_.Run();
-      pause_callback_.Reset();
-    }
+  switch (state_) {
+    case kUninitialized:
+      NOTREACHED();
+      break;

[Ami GONE FROM CHROMIUM, 2012/01/29 22:12:38]

s/break/return/ here and everywhere below, for uniformity with the few return's
already here.

[acolwell GONE FROM CHROMIUM, 2012/01/30 00:14:14]

Done.

+    case kPaused:
+      EnqueueBuffer(buffer);
+      DCHECK(!pending_read_);
+      ResetAndRunCB(&pause_callback_);

[Ami GONE FROM CHROMIUM, 2012/01/29 22:12:38]

Before your CL the code could handle receiving multiple buffers in kPaused (by
checking for pause_callback_.is_null()).  Is the change ok?

[acolwell GONE FROM CHROMIUM, 2012/01/30 00:14:14]

That was because algorithm->EnqueueBuffer() could blindly call
ScheduleRead_Locked() to request another read. Now that I modified
ScheduleRead_Locked() to block such a request, we don't have to worry about more
buffers coming in.

+      break;
+    case kSeeking:
+      if (IsBeforeSeekTime(buffer)) {
+        ScheduleRead_Locked();
+        return;
+      }
+      EnqueueBuffer(buffer);
+      if (!buffer || buffer->IsEndOfStream() || algorithm_->IsQueueFull()) {

[Ami GONE FROM CHROMIUM, 2012/01/29 22:12:38]

That the first two clauses repeat the test in EnqueueBuffer, and that
EnqueueBuffer's name is unconditional while its body is conditional, makes me
think it can be improved.
For example, make EB's body unconditional, but only call it in the right
condition.  Or else make it return a bool indicating successful enqueuing.

Either way, it smells funny that only some of the callsites to EnqueueBuffer in
this switch check buffer's condition.  Is it really expected/OK to get a NULL
buffer in kPaused/kPlaying/kUnderflow/kRebuffering/kStopped?

[acolwell GONE FROM CHROMIUM, 2012/01/30 00:14:14]

Removed EB and just put the conditional checks in the appropriate place. 
Yes. The demuxer doesn't have a Pause() so it doesn't know that one is in
progress. kPlaying/kUnderflow/kRebuffering are all "playing states" so the NULL
could come in before the renderer get's its Pause() call. We don't do anything
for kStopped so that sound be fine as well. 

The ChunkDemuxer gets the FlushData() call before the Pipeline is even notified
of the seek so we can be in any of these states when the demuxer sends the
NULLs.

+        state_ = kPaused;
+        ResetAndRunCB(&seek_cb_, PIPELINE_OK);
+        return;
+      }
+      break;
+    case kPlaying:
+    case kUnderflow:
+    case kRebuffering:
+      EnqueueBuffer(buffer);
+      break;
+    case kStopped:
+      return;
   }
 }
 
@@ -271,7 +269,7 @@ uint32 AudioRendererBase::FillBuffer(uint8* dest,
 
 void AudioRendererBase::ScheduleRead_Locked() {
   lock_.AssertAcquired();
-  if (pending_read_)
+  if (pending_read_ || state_ == kPaused)
     return;
   pending_read_ = true;
   decoder_->Read(read_cb_);
@@ -287,4 +285,14 @@ float AudioRendererBase::GetPlaybackRate() {
   return algorithm_->playback_rate();
 }
 
+bool AudioRendererBase::IsBeforeSeekTime(const scoped_refptr<Buffer>& buffer) {
+  return (state_ == kSeeking) && buffer && !buffer->IsEndOfStream() &&
+      (buffer->GetTimestamp() + buffer->GetDuration()) < seek_timestamp_;
+}
+
+void AudioRendererBase::EnqueueBuffer(const scoped_refptr<Buffer>& buffer) {
+  if (buffer && !buffer->IsEndOfStream())
+    algorithm_->EnqueueBuffer(buffer);
+}
+
 }  // namespace media