Move pending seek cancellation logic from ChunkDemuxerStream to ChunkDemuxer.

TEST=All existing unittests still pass.

Review URL: https://chromiumcodereview.appspot.com/16867010

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

media/filters/chunk_demuxer.cc

@@ -205,9 +205,8 @@ class ChunkDemuxerStream : public DemuxerStream {
                      const LogCB& log_cb);
   virtual ~ChunkDemuxerStream();
 
-  void StartWaitingForSeek();
+  void AbortReads();
   void Seek(TimeDelta time);
-  void CancelPendingSeek();
   bool IsSeekWaitingForData() const;
 
   // Add buffers to this stream.  Buffers are stored in SourceBufferStreams,
@@ -245,9 +244,9 @@ class ChunkDemuxerStream : public DemuxerStream {
 
  private:
   enum State {
+    UNINITIALIZED,
     RETURNING_DATA_FOR_READS,
-    WAITING_FOR_SEEK,
-    CANCELED,
+    RETURNING_ABORT_FOR_READS,
     SHUTDOWN,
   };
 
@@ -283,23 +282,23 @@ class ChunkDemuxerStream : public DemuxerStream {
 ChunkDemuxerStream::ChunkDemuxerStream(const AudioDecoderConfig& audio_config,
                                        const LogCB& log_cb)
     : type_(AUDIO),
-      state_(RETURNING_DATA_FOR_READS) {
+      state_(UNINITIALIZED) {
   stream_.reset(new SourceBufferStream(audio_config, log_cb));
 }
 
 ChunkDemuxerStream::ChunkDemuxerStream(const VideoDecoderConfig& video_config,
                                        const LogCB& log_cb)
     : type_(VIDEO),
-      state_(RETURNING_DATA_FOR_READS) {
+      state_(UNINITIALIZED) {
   stream_.reset(new SourceBufferStream(video_config, log_cb));
 }
 
-void ChunkDemuxerStream::StartWaitingForSeek() {
-  DVLOG(1) << "ChunkDemuxerStream::StartWaitingForSeek()";
+void ChunkDemuxerStream::AbortReads() {
+  DVLOG(1) << "ChunkDemuxerStream::AbortReads()";
   ReadCBQueue read_cbs;
   {
     base::AutoLock auto_lock(lock_);
-    ChangeState_Locked(WAITING_FOR_SEEK);
+    ChangeState_Locked(RETURNING_ABORT_FOR_READS);
     std::swap(read_cbs_, read_cbs);
   }
 
@@ -309,30 +308,11 @@ void ChunkDemuxerStream::StartWaitingForSeek() {
 
 void ChunkDemuxerStream::Seek(TimeDelta time) {
   base::AutoLock auto_lock(lock_);
-
   DCHECK(read_cbs_.empty());
-
-  // Ignore seek requests when canceled.
-  if (state_ == CANCELED)
-    return;
+  DCHECK(state_ == UNINITIALIZED || state_ == RETURNING_ABORT_FOR_READS);
 
   stream_->Seek(time);
-
-  if (state_ == WAITING_FOR_SEEK)
-    ChangeState_Locked(RETURNING_DATA_FOR_READS);
-}
-
-void ChunkDemuxerStream::CancelPendingSeek() {
-  DVLOG(1) << "ChunkDemuxerStream::CancelPendingSeek()";
-  ReadCBQueue read_cbs;
-  {
-    base::AutoLock auto_lock(lock_);
-    ChangeState_Locked(CANCELED);
-    std::swap(read_cbs_, read_cbs);
-  }
-
-  for (ReadCBQueue::iterator it = read_cbs.begin(); it != read_cbs.end(); ++it)
-    it->Run(kAborted, NULL);
+  ChangeState_Locked(RETURNING_DATA_FOR_READS);
 }
 
 bool ChunkDemuxerStream::IsSeekWaitingForData() const {
@@ -449,14 +429,15 @@ static void RunOnMessageLoop(
 
 // DemuxerStream methods.
 void ChunkDemuxerStream::Read(const ReadCB& read_cb) {
+  base::AutoLock auto_lock(lock_);
+  DCHECK_NE(state_, UNINITIALIZED);
+
   DemuxerStream::Status status = kOk;
   scoped_refptr<StreamParserBuffer> buffer;
-  {
-    base::AutoLock auto_lock(lock_);
-    if (!read_cbs_.empty() || !GetNextBuffer_Locked(&status, &buffer)) {
-      DeferRead_Locked(read_cb);
-      return;
-    }
+
+  if (!read_cbs_.empty() || !GetNextBuffer_Locked(&status, &buffer)) {
+    DeferRead_Locked(read_cb);
+    return;
   }
 
   base::MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
@@ -520,6 +501,9 @@ bool ChunkDemuxerStream::GetNextBuffer_Locked(
   lock_.AssertAcquired();
 
   switch (state_) {
+    case UNINITIALIZED:
+      NOTREACHED();
+      return false;
     case RETURNING_DATA_FOR_READS:
       switch (stream_->GetNextBuffer(buffer)) {
         case SourceBufferStream::kSuccess:
@@ -538,8 +522,7 @@ bool ChunkDemuxerStream::GetNextBuffer_Locked(
           return true;
       }
       break;
-    case CANCELED:
-    case WAITING_FOR_SEEK:
+    case RETURNING_ABORT_FOR_READS:
       // Null buffers should be returned in this state since we are waiting
       // for a seek. Any buffers in the SourceBuffer should NOT be returned
       // because they are associated with the seek.
@@ -563,6 +546,7 @@ ChunkDemuxer::ChunkDemuxer(const base::Closure& open_cb,
                            const AddTextTrackCB& add_text_track_cb,
                            const LogCB& log_cb)
     : state_(WAITING_FOR_INIT),
+      cancel_next_seek_(false),
       host_(NULL),
       open_cb_(open_cb),
       need_key_cb_(need_key_cb),
@@ -601,28 +585,34 @@ void ChunkDemuxer::Stop(const base::Closure& callback) {
 void ChunkDemuxer::Seek(TimeDelta time, const PipelineStatusCB& cb) {
   DVLOG(1) << "Seek(" << time.InSecondsF() << ")";
   DCHECK(time >= TimeDelta());
-  DCHECK(seek_cb_.is_null());
 
-  PipelineStatus status = PIPELINE_ERROR_INVALID_STATE;
   base::AutoLock auto_lock(lock_);
+  DCHECK(seek_cb_.is_null());
 
   seek_cb_ = BindToCurrentLoop(cb);
-  if (state_ == INITIALIZED || state_ == ENDED) {
-    if (audio_)
-      audio_->Seek(time);
+  if (state_ != INITIALIZED && state_ != ENDED) {
+    base::ResetAndReturn(&seek_cb_).Run(PIPELINE_ERROR_INVALID_STATE);
+    return;
+  }
 
-    if (video_)
-      video_->Seek(time);
+  if (cancel_next_seek_) {
+    cancel_next_seek_ = false;
+    base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
+    return;
+  }
 
-    if (IsSeekWaitingForData_Locked()) {
-      DVLOG(1) << "Seek() : waiting for more data to arrive.";
-      return;
-    }
+  if (audio_)
+    audio_->Seek(time);
 
-    status = PIPELINE_OK;
+  if (video_)
+    video_->Seek(time);
+
+  if (IsSeekWaitingForData_Locked()) {
+    DVLOG(1) << "Seek() : waiting for more data to arrive.";
+    return;
   }
 
-  base::ResetAndReturn(&seek_cb_).Run(status);
+  base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 }
 
 void ChunkDemuxer::OnAudioRendererDisabled() {
@@ -651,29 +641,42 @@ void ChunkDemuxer::StartWaitingForSeek() {
   DVLOG(1) << "StartWaitingForSeek()";
   base::AutoLock auto_lock(lock_);
   DCHECK(state_ == INITIALIZED || state_ == ENDED || state_ == SHUTDOWN);
+  DCHECK(seek_cb_.is_null());
 
   if (state_ == SHUTDOWN)
     return;
 
   if (audio_)
-    audio_->StartWaitingForSeek();
+    audio_->AbortReads();
 
   if (video_)
-    video_->StartWaitingForSeek();
+    video_->AbortReads();
+
+  // Cancel state set in CancelPendingSeek() since we want to
+  // accept the next Seek().
+  cancel_next_seek_ = false;
 }
 
 void ChunkDemuxer::CancelPendingSeek() {
   base::AutoLock auto_lock(lock_);
-  DCHECK(seek_cb_.is_null() != IsSeekWaitingForData_Locked());
+  DCHECK_NE(state_, INITIALIZING);
+  DCHECK(seek_cb_.is_null() || IsSeekWaitingForData_Locked());
+
+  if (cancel_next_seek_)
+    return;
 
   if (audio_)
-    audio_->CancelPendingSeek();
+    audio_->AbortReads();
 
   if (video_)
-    video_->CancelPendingSeek();
+    video_->AbortReads();
 
-  if (!seek_cb_.is_null())
-    base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
+  if (seek_cb_.is_null()) {
+    cancel_next_seek_ = true;
+    return;
+  }
+
+  base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 }
 
 ChunkDemuxer::Status ChunkDemuxer::AddId(const std::string& id,
@@ -1084,11 +1087,12 @@ void ChunkDemuxer::OnSourceInitDone(bool success, TimeDelta duration) {
       (!source_id_video_.empty() && !video_))
     return;
 
+  TimeDelta start_time = GetStartTime();
   if (audio_)
-    audio_->Seek(TimeDelta());
+    audio_->Seek(start_time);
 
   if (video_)
-    video_->Seek(TimeDelta());
+    video_->Seek(start_time);
 
   if (duration_ == kNoTimestamp())
     duration_ = kInfiniteDuration();

media/filters/chunk_demuxer.h

@@ -62,7 +62,22 @@ class MEDIA_EXPORT ChunkDemuxer : public Demuxer {
   virtual base::TimeDelta GetStartTime() const OVERRIDE;
 
   // Methods used by an external object to control this demuxer.
+  //
+  // Indicates that a new Seek() call is on its way. Any pending Reads on the
+  // DemuxerStream objects should be aborted immediately inside this call and
+  // future Read calls should return kAborted until the Seek() call occurs.
+  // This method MUST ALWAYS be called before Seek() is called to signal that
+  // the next Seek() call represents the seek point we actually want to return
+  // data for.
   void StartWaitingForSeek();
+
+  // Indicates that a Seek() call is on its way, but another seek has been
+  // requested that will override the impending Seek() call. Any pending Reads
+  // on the DemuxerStream objects should be aborted immediately inside this call
+  // and future Read calls should return kAborted until the next
+  // StartWaitingForSeek() call. This method also arranges for the next Seek()
+  // call received before a StartWaitingForSeek() call to immediately call its
+  // callback without waiting for any data.
   void CancelPendingSeek();
 
   // Registers a new |id| to use for AppendData() calls. |type| indicates
@@ -176,6 +191,7 @@ class MEDIA_EXPORT ChunkDemuxer : public Demuxer {
 
   mutable base::Lock lock_;
   State state_;
+  bool cancel_next_seek_;
 
   DemuxerHost* host_;
   base::Closure open_cb_;

media/filters/chunk_demuxer_unittest.cc

@@ -790,6 +790,12 @@ class ChunkDemuxerTest : public testing::Test {
     return scoped_ptr<TextTrack>();
   }
 
+  void Seek(base::TimeDelta seek_time) {
+    demuxer_->StartWaitingForSeek();
+    demuxer_->Seek(seek_time, NewExpectedStatusCB(PIPELINE_OK));
+    message_loop_.RunUntilIdle();
+  }
+
   base::MessageLoop message_loop_;
   MockDemuxerHost host_;
 
@@ -884,8 +890,7 @@ TEST_F(ChunkDemuxerTest, TestAppendDataAfterSeek) {
 
   EXPECT_CALL(*this, Checkpoint(1));
 
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(46),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(46));
 
   EXPECT_CALL(*this, Checkpoint(2));
 
@@ -935,9 +940,7 @@ TEST_F(ChunkDemuxerTest, TestSeekWhileParsingCluster) {
   // in the cluster the cluster.
   ExpectRead(DemuxerStream::AUDIO, 2 * kAudioBlockDuration);
 
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromSeconds(5),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromSeconds(5));
 
   // Append the rest of the cluster.
   AppendData(cluster_a->data() + first_append_size, second_append_size);
@@ -1268,20 +1271,14 @@ TEST_F(ChunkDemuxerTest, TestEndOfStreamDuringCanceledSeek) {
   demuxer_->EndOfStream(PIPELINE_OK);
 
   // Start the first seek.
-  demuxer_->StartWaitingForSeek();
-
-  // Simulate the pipeline finally calling Seek().
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(20),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(20));
 
   // Simulate another seek being requested before the first
   // seek has finished prerolling.
   demuxer_->CancelPendingSeek();
 
   // Finish second seek.
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(30),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(30));
 
   DemuxerStream::Status status;
   base::TimeDelta last_timestamp;
@@ -1601,9 +1598,7 @@ TEST_F(ChunkDemuxerTest, TestSeekCanceled) {
   AppendData(start_cluster->data(), start_cluster->size());
 
   // Seek to an unbuffered region.
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromSeconds(50),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromSeconds(50));
 
   // Attempt to read in unbuffered area; should not fulfill the read.
   bool audio_read_done = false;
@@ -1620,9 +1615,7 @@ TEST_F(ChunkDemuxerTest, TestSeekCanceled) {
   EXPECT_TRUE(video_read_done);
 
   // A seek back to the buffered region should succeed.
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromSeconds(0),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromSeconds(0));
   GenerateExpectedReads(0, 4);
 }
 
@@ -1650,9 +1643,7 @@ TEST_F(ChunkDemuxerTest, TestSeekCanceledWhileWaitingForSeek) {
   EXPECT_TRUE(video_read_done);
 
   // A seek back to the buffered region should succeed.
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromSeconds(0),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromSeconds(0));
   GenerateExpectedReads(0, 4);
 }
 
@@ -1684,9 +1675,7 @@ TEST_F(ChunkDemuxerTest, TestSeekAudioAndVideoSources) {
   EXPECT_TRUE(video_read_done);
 
   // Seek to 3 (an unbuffered region).
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromSeconds(3),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromSeconds(3));
 
   audio_read_done = false;
   video_read_done = false;
@@ -1971,14 +1960,11 @@ TEST_F(ChunkDemuxerTest, TestDifferentStreamTimecodes) {
   scoped_ptr<Cluster> start_cluster(GenerateCluster(0, 25, 8));
   AppendData(start_cluster->data(), start_cluster->size());
 
-  demuxer_->Seek(base::TimeDelta::FromSeconds(0),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromSeconds(0));
   GenerateExpectedReads(0, 25, 8);
 
   // Seek to 5 seconds.
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromSeconds(5),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromSeconds(5));
 
   // Generate a cluster to fulfill this seek, where audio timecode begins 25ms
   // after the video.
@@ -2004,8 +1990,7 @@ TEST_F(ChunkDemuxerTest, TestDifferentStreamTimecodesSeparateSources) {
   AppendData(video_id, cluster_v->data(), cluster_v->size());
 
   // Both streams should be able to fulfill a seek to 25.
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(25),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(25));
   GenerateAudioStreamExpectedReads(25, 4);
   GenerateVideoStreamExpectedReads(30, 4);
 }
@@ -2027,6 +2012,7 @@ TEST_F(ChunkDemuxerTest, TestDifferentStreamTimecodesOutOfRange) {
   AppendData(video_id, cluster_v->data(), cluster_v->size());
 
   // Should not be able to fulfill a seek to 0.
+  demuxer_->StartWaitingForSeek();
   demuxer_->Seek(base::TimeDelta::FromMilliseconds(0),
                  NewExpectedStatusCB(PIPELINE_ERROR_ABORT));
   ExpectRead(DemuxerStream::AUDIO, 0);
@@ -2116,9 +2102,7 @@ TEST_F(ChunkDemuxerTest, TestEndOfStreamStillSetAfterSeek) {
   EXPECT_EQ(kLastVideoTimestamp, last_timestamp);
 
   // Seek back to 0 and verify that we can read to the end again..
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(0),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(0));
 
   ReadUntilNotOkOrEndOfStream(DemuxerStream::AUDIO, &status, &last_timestamp);
   EXPECT_EQ(DemuxerStream::kOk, status);
@@ -2278,8 +2262,7 @@ TEST_F(ChunkDemuxerTest, TestConfigChange_Seek) {
   ExpectRead(DemuxerStream::VIDEO, 0);
 
   // Seek to a location with a different config.
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(527),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(527));
 
   // Verify that the config change is signalled.
   ExpectConfigChanged(DemuxerStream::VIDEO);
@@ -2294,8 +2277,7 @@ TEST_F(ChunkDemuxerTest, TestConfigChange_Seek) {
   ExpectRead(DemuxerStream::VIDEO, 527);
 
   // Seek back to the beginning and verify we get another config change.
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(0),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(0));
   ExpectConfigChanged(DemuxerStream::VIDEO);
   ASSERT_TRUE(video_config_1.Matches(video->video_decoder_config()));
   ExpectRead(DemuxerStream::VIDEO, 0);
@@ -2303,10 +2285,8 @@ TEST_F(ChunkDemuxerTest, TestConfigChange_Seek) {
   // Seek to a location that requires a config change and then
   // seek to a new location that has the same configuration as
   // the start of the file without a Read() in the middle.
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(527),
-                 NewExpectedStatusCB(PIPELINE_OK));
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(801),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(527));
+  Seek(base::TimeDelta::FromMilliseconds(801));
 
   // Verify that no config change is signalled.
   ExpectRead(DemuxerStream::VIDEO, 801);
@@ -2321,9 +2301,7 @@ TEST_F(ChunkDemuxerTest, TestTimestampPositiveOffset) {
   scoped_ptr<Cluster> cluster(GenerateCluster(0, 2));
   AppendData(cluster->data(), cluster->size());
 
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(30000),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(30000));
 
   GenerateExpectedReads(30000, 2);
 }
@@ -2371,9 +2349,7 @@ TEST_F(ChunkDemuxerTest, TestTimestampOffsetSeparateStreams) {
   GenerateAudioStreamExpectedReads(0, 4);
   GenerateVideoStreamExpectedReads(0, 4);
 
-  demuxer_->StartWaitingForSeek();
-  demuxer_->Seek(base::TimeDelta::FromMilliseconds(27300),
-                 NewExpectedStatusCB(PIPELINE_OK));
+  Seek(base::TimeDelta::FromMilliseconds(27300));
 
   ASSERT_TRUE(demuxer_->SetTimestampOffset(
       audio_id, base::TimeDelta::FromMilliseconds(27300)));
@@ -2573,4 +2549,18 @@ TEST_F(ChunkDemuxerTest, TestEndOfStreamWhileWaitingForGapToBeFilled) {
   EXPECT_TRUE(video_read_done);
 }
 
+TEST_F(ChunkDemuxerTest, TestCanceledSeekDuringInitialPreroll) {
+  ASSERT_TRUE(InitDemuxer(true, true));
+
+  // Cancel preroll.
+  demuxer_->CancelPendingSeek();
+
+  // Initiate the seek to the new location.
+  int seek_time_in_ms = 200;
+  Seek(base::TimeDelta::FromMilliseconds(seek_time_in_ms));
+
+  // Append data to satisfy the seek.
+  AppendCluster(seek_time_in_ms, 10);
+}
+
 }  // namespace media