Fix ChunkDemuxer seek deadlock

media/filters/audio_renderer_base.cc

 // Copyright (c) 2012 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"
@@ skipping 49 matching lines @@
   }
   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(!pending_read_) << "Pending read must complete before seeking";
+  DCHECK(pause_callback_.is_null());
   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;
 
@@ skipping 57 matching lines @@
 }
 
 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);
 
   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;
+  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.
+    if (state_ == kUnderflow || state_ == kRebuffering)
+      state_ = kPlaying;
   }
 
-  // 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()) {
-    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->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;
   }
 }
 
 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;
@@ skipping 67 matching lines @@
   }
 
   if (!underflow_cb.is_null())
     underflow_cb.Run();
 
   return dest_written;
 }
 
 void AudioRendererBase::ScheduleRead_Locked() {
   lock_.AssertAcquired();
-  if (pending_read_)
+  if (pending_read_ || state_ == kPaused)
     return;
   pending_read_ = true;
   decoder_->Read(read_cb_);
 }
 
 void AudioRendererBase::SetPlaybackRate(float playback_rate) {
   base::AutoLock auto_lock(lock_);
   algorithm_->SetPlaybackRate(playback_rate);
 }
 
 float AudioRendererBase::GetPlaybackRate() {
   base::AutoLock auto_lock(lock_);
   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