Fix ChunkDemuxer seek deadlock

media/filters/pipeline_integration_test.cc

Index: media/filters/pipeline_integration_test.cc
diff --git a/media/filters/pipeline_integration_test.cc b/media/filters/pipeline_integration_test.cc
index b64ed091c6ecc155dfc184345c16de9af433ff8f..2da35c85b0721eeae5ae8268304fd2a4971367d8 100644
--- a/media/filters/pipeline_integration_test.cc
+++ b/media/filters/pipeline_integration_test.cc
@@ -9,6 +9,9 @@
 #include "media/base/message_loop_factory_impl.h"
 #include "media/base/pipeline.h"
 #include "media/base/test_data_util.h"
+#include "media/filters/chunk_demuxer.h"
+#include "media/filters/chunk_demuxer_client.h"
+#include "media/filters/chunk_demuxer_factory.h"
 #include "media/filters/ffmpeg_audio_decoder.h"
 #include "media/filters/ffmpeg_demuxer_factory.h"
 #include "media/filters/ffmpeg_video_decoder.h"
@@ -22,6 +25,71 @@ using ::testing::AnyNumber;
 
 namespace media {
 
+// Helper class for testing ChunkDemuxer.

[Ami GONE FROM CHROMIUM, 2012/01/27 23:44:58]

IMO it would be more helpful to describe what this class mimics (a decoder?).

[acolwell GONE FROM CHROMIUM, 2012/01/29 03:00:41]

Done.

+class TestChunkDemuxerClient : public ChunkDemuxerClient {
+ public:
+  TestChunkDemuxerClient(const std::string& filename, int initial_append_size)
+      : url_(GetTestDataURL(filename)),
+        current_position_(0),
+        initial_append_size_(initial_append_size) {
+    ReadTestDataFile(filename, &file_data_, &file_data_size_);
+
+    DCHECK_GT(initial_append_size_, 0);
+    DCHECK_LE(initial_append_size_, file_data_size_);
+  }
+
+  virtual ~TestChunkDemuxerClient() {}
+
+  const std::string& url() { return url_; }
+
+  void FlushData() {
+    DCHECK(chunk_demuxer_.get());
+    chunk_demuxer_->FlushData();
+  }
+
+  void Seek(int new_position) {
+    DCHECK_GE(new_position, 0);
+    DCHECK_LT(new_position, file_data_size_);
+    current_position_ = new_position;
+  }
+
+  void AppendData(int size) {
+    DCHECK(chunk_demuxer_.get());
+    DCHECK_LT(current_position_, file_data_size_);
+    DCHECK_LE(current_position_ + size, file_data_size_);
+    chunk_demuxer_->AppendData(file_data_.get() + current_position_, size);
+    current_position_ += size;
+  }
+
+  void EndOfStream() {
+    chunk_demuxer_->EndOfStream(PIPELINE_OK);
+  }
+
+  void Abort() {
+    if (!chunk_demuxer_.get())
+      return;
+    chunk_demuxer_->Shutdown();
+  }
+
+  // ChunkDemuxerClient methods.
+  virtual void DemuxerOpened(ChunkDemuxer* demuxer) {
+    chunk_demuxer_ = demuxer;
+    AppendData(initial_append_size_);
+  }
+
+  virtual void DemuxerClosed() {
+    chunk_demuxer_ = NULL;
+  }
+
+ private:
+  std::string url_;
+  scoped_array<uint8> file_data_;
+  int file_data_size_;
+  int current_position_;
+  int initial_append_size_;
+  scoped_refptr<ChunkDemuxer> chunk_demuxer_;
+};
+
 // Integration tests for Pipeline. Real demuxers, real decoders, and
 // base renderer implementations are used to verify pipeline functionality. The
 // renderers used in these tests rely heavily on the AudioRendererBase &
@@ -31,7 +99,9 @@ namespace media {
 // display or audio device. Both of these devices are simulated since they have
 // little effect on verifying pipeline behavior and allow tests to run faster
 // than real-time.
-class PipelineIntegrationTest : public testing::Test {
+class PipelineIntegrationTest
+    : public testing::Test {

[Ami GONE FROM CHROMIUM, 2012/01/27 23:44:58]

unnecessary newline

[acolwell GONE FROM CHROMIUM, 2012/01/29 03:00:41]

Done.

+
  public:
   PipelineIntegrationTest()
       : message_loop_factory_(new MessageLoopFactoryImpl()),
@@ -43,7 +113,7 @@ class PipelineIntegrationTest : public testing::Test {
         Pipeline::NetworkEventCB());
 
     EXPECT_CALL(*this, OnVideoRendererPaint()).Times(AnyNumber());
-    EXPECT_CALL(*this, OnSetOpaque(true));
+    EXPECT_CALL(*this, OnSetOpaque(true)).Times(AnyNumber());
   }
 
   virtual ~PipelineIntegrationTest() {
@@ -138,11 +208,20 @@ class PipelineIntegrationTest : public testing::Test {
   scoped_ptr<FilterCollection> CreateFilterCollection(const std::string& url) {
     scoped_refptr<FileDataSource> data_source = new FileDataSource();
     CHECK_EQ(PIPELINE_OK, data_source->Initialize(url));
-
-    scoped_ptr<FilterCollection> collection(
-        new FilterCollection());
-    collection->SetDemuxerFactory(scoped_ptr<DemuxerFactory>(
+    return CreateFilterCollection(scoped_ptr<DemuxerFactory>(
         new FFmpegDemuxerFactory(data_source, &message_loop_)));
+  }
+
+  scoped_ptr<FilterCollection> CreateFilterCollection(
+      ChunkDemuxerClient* client) {
+    return CreateFilterCollection(scoped_ptr<DemuxerFactory>(
+        new ChunkDemuxerFactory(client)));
+  }
+
+  scoped_ptr<FilterCollection> CreateFilterCollection(
+      scoped_ptr<DemuxerFactory> demuxer_factory) {
+    scoped_ptr<FilterCollection> collection(new FilterCollection());
+    collection->SetDemuxerFactory(demuxer_factory.Pass());
     collection->AddAudioDecoder(new FFmpegAudioDecoder(
         message_loop_factory_->GetMessageLoop("AudioDecoderThread")));
     collection->AddVideoDecoder(new FFmpegVideoDecoder(
@@ -156,6 +235,39 @@ class PipelineIntegrationTest : public testing::Test {
     return collection.Pass();
   }
 
+  // Verifies that seeking works properly for ChunkDemuxer when the
+  // seek happens while there is a pending read on the ChunkDemuxer
+  // and no data is available.
+  void TestChunkDemuxerSeekDuringRead(const std::string& filename,

[Ami GONE FROM CHROMIUM, 2012/01/27 23:44:58]

#damnson !

[acolwell GONE FROM CHROMIUM, 2012/01/29 03:00:41]

removed ChunkDemuxer to make the name a little shorter. :)

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

Sorry, I didn't mean the name was too long; I was pointing out your commitment
to testing.
Since this (chunkdemuxer-specific) helper is contained in a
non-ChunkDemuxer-specific class, I think including ChunkDemuxer in the method
name makes sense.

+                                      int initial_append_size,
+                                      base::TimeDelta start_seek_time,
+                                      base::TimeDelta seek_time,
+                                      int seek_file_position,
+                                      int seek_append_size) {
+    TestChunkDemuxerClient client(filename, initial_append_size);
+
+    pipeline_->Start(CreateFilterCollection(&client), client.url(),
+                     QuitOnStatusCB(PIPELINE_OK));
+    message_loop_.Run();
+
+    Play();
+    WaitUntilCurrentTimeIsAfter(start_seek_time);
+
+    client.FlushData();
+
+    ended_ = false;
+    pipeline_->Seek(seek_time, QuitOnStatusCB(PIPELINE_OK));
+    client.Seek(seek_file_position);
+    client.AppendData(seek_append_size);
+
+    message_loop_.Run();
+
+    client.EndOfStream();
+
+    client.Abort();
+    Stop();
+  }
+
  protected:
   MessageLoop message_loop_;
   scoped_ptr<MessageLoopFactory> message_loop_factory_;
@@ -165,6 +277,7 @@ class PipelineIntegrationTest : public testing::Test {
  private:
   MOCK_METHOD0(OnVideoRendererPaint, void());
   MOCK_METHOD1(OnSetOpaque, void(bool));
+

[Ami GONE FROM CHROMIUM, 2012/01/27 23:44:58]

extra newline

[acolwell GONE FROM CHROMIUM, 2012/01/29 03:00:41]

Done.

 };
 
 
@@ -218,4 +331,20 @@ TEST_F(PipelineIntegrationTest, SeekWhilePlaying) {
   WaitUntilOnEnded();
 }
 
+// Verify audio decoder & renderer can handle aborted demuxer reads.
+TEST_F(PipelineIntegrationTest, ChunkDemuxerAbortRead_AudioOnly) {
+  TestChunkDemuxerSeekDuringRead("bear-320x240-audio-only.webm", 8192,
+                                 base::TimeDelta::FromMilliseconds(477),
+                                 base::TimeDelta::FromMilliseconds(617),
+                                 0x10CA, 19730);
+}
+
+// Verify video decoder & renderer can handle aborted demuxer reads.
+TEST_F(PipelineIntegrationTest, ChunkDemuxerAbortRead_VideoOnly) {
+  TestChunkDemuxerSeekDuringRead("bear-320x240-video-only.webm", 32768,
+                                 base::TimeDelta::FromMilliseconds(200),
+                                 base::TimeDelta::FromMilliseconds(1668),
+                                 0x1C896, 65536);
+}
+
 }  // namespace media