Fix various operations in ChunkDemuxer that were not being applied to text tracks.

media/filters/chunk_demuxer_unittest.cc

Index: media/filters/chunk_demuxer_unittest.cc
diff --git a/media/filters/chunk_demuxer_unittest.cc b/media/filters/chunk_demuxer_unittest.cc
index be8a2a225129dd10a1665280dd11e2755763699b..00fac77c9881876be299da558cd3e83e75adf243 100644
--- a/media/filters/chunk_demuxer_unittest.cc
+++ b/media/filters/chunk_demuxer_unittest.cc
@@ -1060,7 +1060,7 @@ TEST_F(ChunkDemuxerTest, InitText) {
 
 // Make sure that the demuxer reports an error if Shutdown()
 // is called before all the initialization segments are appended.
-TEST_F(ChunkDemuxerTest, ShutdownBeforeAllInitSegmentsAppended) {
+TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppended) {

[xhwang, 2014/01/08 19:03:26]

In this test and the next one, it's not clear how Shutdown() happens. Add a
comment at least?

[acolwell GONE FROM CHROMIUM, 2014/01/09 00:21:13]

I've added explicit ShutdownDemuxer() calls to make this clear.

   EXPECT_CALL(*this, DemuxerOpened());
   demuxer_->Initialize(
       &host_, CreateInitDoneCB(
@@ -1072,7 +1072,7 @@ TEST_F(ChunkDemuxerTest, ShutdownBeforeAllInitSegmentsAppended) {
   AppendInitSegmentWithSourceId("audio", true, false, false);

[xhwang, 2014/01/08 19:03:26]

Can we also use the new HAS_VIDEO | HAS_XXX pattern for
AppendInitSegmentWithSourceId?

The same applies to CreateInitSegment I guess.

[acolwell GONE FROM CHROMIUM, 2014/01/09 00:21:13]

Done. This resulted in a fair amount of changes, but they are pretty trivial.

 }
 
-TEST_F(ChunkDemuxerTest, ShutdownBeforeAllInitSegmentsAppendedText) {
+TEST_F(ChunkDemuxerTest, Shutdown_BeforeAllInitSegmentsAppendedText) {
   EXPECT_CALL(*this, DemuxerOpened());
   demuxer_->Initialize(
       &host_, CreateInitDoneCB(
@@ -1087,6 +1087,35 @@ TEST_F(ChunkDemuxerTest, ShutdownBeforeAllInitSegmentsAppendedText) {
   AppendInitSegmentWithSourceId("video", false, true, true);

[xhwang, 2014/01/08 19:03:26]

s/video/video_and_text?

[acolwell GONE FROM CHROMIUM, 2014/01/09 00:21:13]

These are just ID names so they don't really matter. I've updated it here and
above.

 }
 
+TEST_F(ChunkDemuxerTest, Shutdown_AllPendingReadsEOS) {

[xhwang, 2014/01/08 19:03:26]

This name is ambiguous. It reads like "shutdown during pending EOS reads". But
really it's testing that when we shutdown during pending reads, the reads will
be aborted with EOS (correct me if I misunderstand). I don't have a better name,
but at least it should match EndOfStreamWhileWaitingForGapToBeFilled somehow.

[acolwell GONE FROM CHROMIUM, 2014/01/09 00:21:13]

Changed the name and added a comment.

+  DemuxerStream* text_stream = NULL;
+  EXPECT_CALL(host_, AddTextStream(_, _))
+      .WillOnce(SaveArg<0>(&text_stream));
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO | HAS_TEXT));
+
+  DemuxerStream* audio_stream = demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* video_stream = demuxer_->GetStream(DemuxerStream::VIDEO);
+
+  bool audio_read_done = false;
+  bool video_read_done = false;
+  bool text_read_done = false;
+  audio_stream->Read(base::Bind(&OnReadDone_EOSExpected, &audio_read_done));
+  video_stream->Read(base::Bind(&OnReadDone_EOSExpected, &video_read_done));
+  text_stream->Read(base::Bind(&OnReadDone_EOSExpected, &text_read_done));
+  message_loop_.RunUntilIdle();
+
+  EXPECT_FALSE(audio_read_done);
+  EXPECT_FALSE(video_read_done);
+  EXPECT_FALSE(text_read_done);
+
+  ShutdownDemuxer();
+  demuxer_.reset();
+
+  EXPECT_TRUE(audio_read_done);
+  EXPECT_TRUE(video_read_done);
+  EXPECT_TRUE(text_read_done);
+}
+
 // Test that Seek() completes successfully when the first cluster
 // arrives.
 TEST_F(ChunkDemuxerTest, AppendDataAfterSeek) {
@@ -2668,7 +2697,7 @@ TEST_F(ChunkDemuxerTest, AppendAfterEndOfStream) {
 // Test receiving a Shutdown() call before we get an Initialize()
 // call. This can happen if video element gets destroyed before
 // the pipeline has a chance to initialize the demuxer.
-TEST_F(ChunkDemuxerTest, ShutdownBeforeInitialize) {
+TEST_F(ChunkDemuxerTest, Shutdown_BeforeInitialize) {
   demuxer_->Shutdown();
   demuxer_->Initialize(
       &host_, CreateInitDoneCB(DEMUXER_ERROR_COULD_NOT_OPEN), true);
@@ -2941,4 +2970,99 @@ TEST_F(ChunkDemuxerTest, StartWaitingForSeekAfterParseError) {
   demuxer_->StartWaitingForSeek(seek_time);
 }
 
+TEST_F(ChunkDemuxerTest, Remove_AudioVideoText) {
+  DemuxerStream* text_stream = NULL;
+  EXPECT_CALL(host_, AddTextStream(_, _))
+      .WillOnce(SaveArg<0>(&text_stream));
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO | HAS_TEXT));
+
+  DemuxerStream* audio_stream = demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* video_stream = demuxer_->GetStream(DemuxerStream::VIDEO);
+
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum,
+                            "0K 20K 40K 60K 80K 100K 120K 140K");
+  AppendSingleStreamCluster(kSourceId, kVideoTrackNum,
+                            "0K 30 60 90 120K 150 180");
+  AppendSingleStreamCluster(kSourceId, kTextTrackNum, "0K 100K 200K");
+
+  CheckExpectedBuffers(audio_stream, "0 20 40 60 80 100 120 140");
+  CheckExpectedBuffers(video_stream, "0 30 60 90 120 150 180");
+  CheckExpectedBuffers(text_stream, "0 100 200");
+
+  // Remove the buffers that were added.
+  demuxer_->Remove(kSourceId, base::TimeDelta(),
+                   base::TimeDelta::FromMilliseconds(300));
+

[xhwang, 2014/01/08 19:03:26]

CheckExpectedBuffers here to make sure the buffers are removed (not overwritten
by the newly appended buffers below?

[acolwell GONE FROM CHROMIUM, 2014/01/09 00:21:13]

I can't use CheckExpectedBuffers here because there aren't any buffers and the
reads will just wait for more data. I've added a CheckExpectedRanges("{ }") to
verify that the source buffer is actually empty though.

+  // Append new buffers that are clearly different than the original
+  // ones and verify that only the new buffers are returned.
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum,
+                            "1K 21K 41K 61K 81K 101K 121K 141K");
+  AppendSingleStreamCluster(kSourceId, kVideoTrackNum,
+                            "1K 31 61 91 121K 151 181");
+  AppendSingleStreamCluster(kSourceId, kTextTrackNum, "1K 101K 201K");
+
+  Seek(base::TimeDelta());
+  CheckExpectedBuffers(audio_stream, "1 21 41 61 81 101 121 141");
+  CheckExpectedBuffers(video_stream, "1 31 61 91 121 151 181");
+  CheckExpectedBuffers(text_stream, "1 101 201");
+}
+
+// Verifies that a Seek() will complete without text cues for
+// the seek point and will return cues after the seek position
+// when they are eventually appended.
+TEST_F(ChunkDemuxerTest, SeekCompletesWithoutTextCues) {
+  DemuxerStream* text_stream = NULL;
+  EXPECT_CALL(host_, AddTextStream(_, _))
+      .WillOnce(SaveArg<0>(&text_stream));
+  ASSERT_TRUE(InitDemuxer(HAS_AUDIO | HAS_VIDEO | HAS_TEXT));
+
+  DemuxerStream* audio_stream = demuxer_->GetStream(DemuxerStream::AUDIO);
+  DemuxerStream* video_stream = demuxer_->GetStream(DemuxerStream::VIDEO);
+
+  base::TimeDelta seek_time = base::TimeDelta::FromMilliseconds(120);
+  bool seek_cb_was_called = false;
+  demuxer_->StartWaitingForSeek(seek_time);
+  demuxer_->Seek(seek_time,
+                 base::Bind(OnSeekDone_OKExpected, &seek_cb_was_called));
+  message_loop_.RunUntilIdle();
+
+  EXPECT_FALSE(seek_cb_was_called);
+
+  bool text_read_done = false;
+  text_stream->Read(base::Bind(&OnReadDone,
+                               base::TimeDelta::FromMilliseconds(125),
+                               &text_read_done));
+
+  // Append audio & video data so the seek completes.
+  AppendSingleStreamCluster(kSourceId, kAudioTrackNum,
+                            "0K 20K 40K 60K 80K 100K 120K 140K 160K 180K");
+  AppendSingleStreamCluster(kSourceId, kVideoTrackNum,
+                            "0K 30 60 90 120K 150 180 210");
+
+  message_loop_.RunUntilIdle();
+  EXPECT_TRUE(seek_cb_was_called);
+  EXPECT_FALSE(text_read_done);
+
+  // Read some audio & video buffers to further verify seek completion.
+  CheckExpectedBuffers(audio_stream, "120 140");
+  CheckExpectedBuffers(video_stream, "120 150");
+
+  EXPECT_FALSE(text_read_done);
+
+  // Append text cues that start after the seek point and verify that
+  // they are returned by Read() calls.
+  AppendSingleStreamCluster(kSourceId, kTextTrackNum, "125K 175K 225K");
+
+  message_loop_.RunUntilIdle();
+  EXPECT_TRUE(text_read_done);
+
+  // NOTE: we start at 175 here because the buffer at 125 was returned
+  // to the pending read initiated above.
+  CheckExpectedBuffers(text_stream, "175 225");
+
+  // Verify that audio & video streams contiue to return expected values.
+  CheckExpectedBuffers(audio_stream, "160 180");
+  CheckExpectedBuffers(video_stream, "180 210");
+}

[xhwang, 2014/01/08 19:03:26]

I like this test!

[acolwell GONE FROM CHROMIUM, 2014/01/09 00:21:13]

thanks. :)

+
 }  // namespace media