Make ChunkDemuxer error handling more consistent and robust.

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 13d0c93d5b09d2c55f151ac28cc09dda810708d9..4dbed5bf1403fc9c0975fd5649ce2977051388de 100644
--- a/media/filters/chunk_demuxer_unittest.cc
+++ b/media/filters/chunk_demuxer_unittest.cc
@@ -5,15 +5,18 @@
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/file_util.h"
+#include "base/message_loop.h"
 #include "base/path_service.h"
 #include "media/base/media.h"
 #include "media/base/mock_callback.h"
 #include "media/base/mock_ffmpeg.h"
+#include "media/base/mock_filter_host.h"
 #include "media/filters/chunk_demuxer.h"
 #include "media/filters/chunk_demuxer_client.h"
 #include "media/webm/cluster_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::AnyNumber;
 using ::testing::InSequence;
 using ::testing::Return;
 using ::testing::SetArgumentPointee;
@@ -72,6 +75,7 @@ class ChunkDemuxerTest : public testing::Test{
   }
 
   virtual ~ChunkDemuxerTest() {
+    message_loop_.RunAllPending();

[Ami GONE FROM CHROMIUM, 2011/08/03 21:06:02]

This makes me worry.  Why do you have it?

[acolwell GONE FROM CHROMIUM, 2011/08/03 22:08:11]

It was just catching the error on EndOfStream() cases. I've removed this and
added calls at the appropriate locations.

     ShutdownDemuxer();
   }
 
@@ -149,6 +153,14 @@ class ChunkDemuxerTest : public testing::Test{
     }
   }
 
+  void AppendData(const uint8* data, unsigned length) {
+    EXPECT_CALL(mock_filter_host_, SetBufferedBytes(_)).Times(AnyNumber());
+    EXPECT_CALL(mock_filter_host_, SetBufferedTime(_)).Times(AnyNumber());
+    EXPECT_CALL(mock_filter_host_, SetNetworkActivity(true)).Times(AnyNumber());

[Ami GONE FROM CHROMIUM, 2011/08/03 21:06:02]

Ugh to the above three lines, and the other green EXPECT_CALLs in this change. 
Can you say "brittle"?

[acolwell GONE FROM CHROMIUM, 2011/08/03 22:08:11]

Yes. Yes I can...These are on the must die list along with other FilterHost
behavior.

+    EXPECT_TRUE(demuxer_->AppendData(data, length));
+    message_loop_.RunAllPending();
+  }
+
   void AppendInfoTracks(bool has_audio, bool has_video) {
     EXPECT_CALL(mock_ffmpeg_, AVOpenInputFile(_, _, NULL, 0, NULL))
         .WillOnce(DoAll(SetArgumentPointee<0>(&format_context_),
@@ -168,7 +180,7 @@ class ChunkDemuxerTest : public testing::Test{
 
     SetupAVFormatContext(has_audio, has_video);
 
-    demuxer_->AppendData(info_tracks.get(), info_tracks_size);
+    AppendData(info_tracks.get(), info_tracks_size);
   }
 
   static void InitDoneCalled(bool* was_called, PipelineStatus expectedStatus,
@@ -192,6 +204,9 @@ class ChunkDemuxerTest : public testing::Test{
     AppendInfoTracks(has_audio, has_video);
 
     EXPECT_TRUE(init_done_called);
+    EXPECT_CALL(mock_filter_host_, SetDuration(_));
+    EXPECT_CALL(mock_filter_host_, SetCurrentReadPosition(_));
+    demuxer_->set_host(&mock_filter_host_);
   }
 
   void ShutdownDemuxer() {
@@ -214,7 +229,9 @@ class ChunkDemuxerTest : public testing::Test{
 
   MOCK_METHOD1(Checkpoint, void(int id));
 
+  MessageLoop message_loop_;

[Ami GONE FROM CHROMIUM, 2011/08/03 21:06:02]

I don't get where this is populated, only drained.  What?

[acolwell GONE FROM CHROMIUM, 2011/08/03 22:08:11]

The constructor is magical and initializes the thread local storage. This
declaration causes MessageLoop::current() to no longer be null.

I can add a message_loop_ member to ChunkDemuxer & pass this in if you'd prefer
that to just using MessageLoop::current() in the one place I need it.

[Ami GONE FROM CHROMIUM, 2011/08/03 23:41:35]

Per offline convo, I think you can avoid ML entirely.

[acolwell GONE FROM CHROMIUM, 2011/08/04 00:22:16]

Removed

   MockFFmpeg mock_ffmpeg_;
+  MockFilterHost mock_filter_host_;
 
   AVFormatContext format_context_;
   AVCodecContext codecs_[MAX_CODECS_INDEX];
@@ -299,7 +316,7 @@ TEST_F(ChunkDemuxerTest, TestAppendDataAfterSeek) {
 
   Checkpoint(1);
 
-  EXPECT_TRUE(demuxer_->AppendData(cluster->data(), cluster->size()));
+  AppendData(cluster->data(), cluster->size());
 
   Checkpoint(2);
 }
@@ -345,7 +362,7 @@ TEST_F(ChunkDemuxerTest, TestRead) {
   AddSimpleBlock(&cb, kVideoTrackNum, 123);
   scoped_ptr<Cluster> cluster(cb.Finish());
 
-  EXPECT_TRUE(demuxer_->AppendData(cluster->data(), cluster->size()));
+  AppendData(cluster->data(), cluster->size());
 
   EXPECT_TRUE(audio_read_done);
   EXPECT_TRUE(video_read_done);
@@ -363,7 +380,7 @@ TEST_F(ChunkDemuxerTest, TestOutOfOrderClusters) {
   AddSimpleBlock(&cb, kVideoTrackNum, 43);
   scoped_ptr<Cluster> clusterA(cb.Finish());
 
-  EXPECT_TRUE(demuxer_->AppendData(clusterA->data(), clusterA->size()));
+  AppendData(clusterA->data(), clusterA->size());
 
   // Cluster B starts before clusterA and has data
   // that overlaps.
@@ -376,35 +393,24 @@ TEST_F(ChunkDemuxerTest, TestOutOfOrderClusters) {
 
   // Make sure that AppendData() fails because this cluster data
   // is before previous data.
-  EXPECT_FALSE(demuxer_->AppendData(clusterB->data(), clusterB->size()));
+  EXPECT_CALL(mock_filter_host_, SetError(PIPELINE_ERROR_DECODE));
+  AppendData(clusterB->data(), clusterB->size());
 
-  // Cluster C starts after clusterA.
-  cb.SetClusterTimecode(56);
-  AddSimpleBlock(&cb, kAudioTrackNum, 56);
-  AddSimpleBlock(&cb, kVideoTrackNum, 76);
-  AddSimpleBlock(&cb, kAudioTrackNum, 79);
-  AddSimpleBlock(&cb, kVideoTrackNum, 109);
+  // Verify that AppendData() doesn't accept more data now.
+  cb.SetClusterTimecode(45);
+  AddSimpleBlock(&cb, kAudioTrackNum, 45);
+  AddSimpleBlock(&cb, kVideoTrackNum, 45);
   scoped_ptr<Cluster> clusterC(cb.Finish());
-
-  // Verify that clusterC is accepted.
-  EXPECT_TRUE(demuxer_->AppendData(clusterC->data(), clusterC->size()));
-
-  // Flush and try clusterB again.
-  demuxer_->FlushData();
-  EXPECT_TRUE(demuxer_->AppendData(clusterB->data(), clusterB->size()));
-
-  // Following that with clusterC should work too since it doesn't
-  // overlap with clusterB.
-  EXPECT_TRUE(demuxer_->AppendData(clusterC->data(), clusterC->size()));
+  EXPECT_FALSE(demuxer_->AppendData(clusterC->data(), clusterC->size()));
 }
 
-TEST_F(ChunkDemuxerTest, TestInvalidBlockSequences) {
+TEST_F(ChunkDemuxerTest, TestNonMonotonicButAboveClusterTimecode) {
   InitDemuxer(true, true);
 
   ClusterBuilder cb;
 
-  // Test the case where timecode is not monotonically
-  // increasing but stays above the cluster timecode.
+  // Test the case where block timecodes are not monotonically
+  // increasing but stay above the cluster timecode.
   cb.SetClusterTimecode(5);
   AddSimpleBlock(&cb, kAudioTrackNum, 5);
   AddSimpleBlock(&cb, kVideoTrackNum, 10);
@@ -412,17 +418,47 @@ TEST_F(ChunkDemuxerTest, TestInvalidBlockSequences) {
   AddSimpleBlock(&cb, kVideoTrackNum, 15);
   scoped_ptr<Cluster> clusterA(cb.Finish());
 
-  EXPECT_FALSE(demuxer_->AppendData(clusterA->data(), clusterA->size()));
+  EXPECT_CALL(mock_filter_host_, SetError(PIPELINE_ERROR_DECODE));
+  AppendData(clusterA->data(), clusterA->size());
+
+  // Verify that AppendData() doesn't accept more data now.
+  cb.SetClusterTimecode(20);
+  AddSimpleBlock(&cb, kAudioTrackNum, 20);
+  AddSimpleBlock(&cb, kVideoTrackNum, 20);
+  scoped_ptr<Cluster> clusterB(cb.Finish());
+  EXPECT_FALSE(demuxer_->AppendData(clusterB->data(), clusterB->size()));
+}
+
+TEST_F(ChunkDemuxerTest, TestBackwardsAndBeforeClusterTimecode) {
+  InitDemuxer(true, true);
+
+  ClusterBuilder cb;
 
-  // Test timecodes going backwards before cluster timecode.
+  // Test timecodes going backwards and including values less than the cluster
+  // timecode.
   cb.SetClusterTimecode(5);
   AddSimpleBlock(&cb, kAudioTrackNum, 5);
   AddSimpleBlock(&cb, kVideoTrackNum, 5);
   AddSimpleBlock(&cb, kAudioTrackNum, 3);
   AddSimpleBlock(&cb, kVideoTrackNum, 3);
-  scoped_ptr<Cluster> clusterB(cb.Finish());
+  scoped_ptr<Cluster> clusterA(cb.Finish());
+
+  EXPECT_CALL(mock_filter_host_, SetError(PIPELINE_ERROR_DECODE));
+  AppendData(clusterA->data(), clusterA->size());
 
+  // Verify that AppendData() doesn't accept more data now.
+  cb.SetClusterTimecode(6);
+  AddSimpleBlock(&cb, kAudioTrackNum, 6);
+  AddSimpleBlock(&cb, kVideoTrackNum, 6);
+  scoped_ptr<Cluster> clusterB(cb.Finish());
   EXPECT_FALSE(demuxer_->AppendData(clusterB->data(), clusterB->size()));
+}
+
+
+TEST_F(ChunkDemuxerTest, TestPerStreamMonotonicallyIncreasingTimestamps) {
+  InitDemuxer(true, true);
+
+  ClusterBuilder cb;
 
   // Test strict monotonic increasing timestamps on a per stream
   // basis.
@@ -431,25 +467,40 @@ TEST_F(ChunkDemuxerTest, TestInvalidBlockSequences) {
   AddSimpleBlock(&cb, kVideoTrackNum, 5);
   AddSimpleBlock(&cb, kAudioTrackNum, 5);
   AddSimpleBlock(&cb, kVideoTrackNum, 7);
-  scoped_ptr<Cluster> clusterC(cb.Finish());
+  scoped_ptr<Cluster> cluster(cb.Finish());
 
-  EXPECT_FALSE(demuxer_->AppendData(clusterC->data(), clusterC->size()));
+  EXPECT_CALL(mock_filter_host_, SetError(PIPELINE_ERROR_DECODE));
+  AppendData(cluster->data(), cluster->size());
+}
+
+TEST_F(ChunkDemuxerTest, TestMonotonicallyIncreasingTimestampsAcrossClusters) {
+  InitDemuxer(true, true);
+
+  ClusterBuilder cb;
 
   // Test strict monotonic increasing timestamps on a per stream
   // basis across clusters.
   cb.SetClusterTimecode(5);
   AddSimpleBlock(&cb, kAudioTrackNum, 5);
   AddSimpleBlock(&cb, kVideoTrackNum, 5);
-  scoped_ptr<Cluster> clusterD(cb.Finish());
+  scoped_ptr<Cluster> clusterA(cb.Finish());
 
-  EXPECT_TRUE(demuxer_->AppendData(clusterD->data(), clusterD->size()));
+  AppendData(clusterA->data(), clusterA->size());
 
   cb.SetClusterTimecode(5);
   AddSimpleBlock(&cb, kAudioTrackNum, 5);
   AddSimpleBlock(&cb, kVideoTrackNum, 7);
-  scoped_ptr<Cluster> clusterE(cb.Finish());
+  scoped_ptr<Cluster> clusterB(cb.Finish());
 
-  EXPECT_FALSE(demuxer_->AppendData(clusterE->data(), clusterE->size()));
+  EXPECT_CALL(mock_filter_host_, SetError(PIPELINE_ERROR_DECODE));
+  AppendData(clusterB->data(), clusterB->size());
+
+  // Verify that AppendData() doesn't accept more data now.
+  cb.SetClusterTimecode(10);
+  AddSimpleBlock(&cb, kAudioTrackNum, 10);
+  AddSimpleBlock(&cb, kVideoTrackNum, 10);
+  scoped_ptr<Cluster> clusterC(cb.Finish());
+  EXPECT_FALSE(demuxer_->AppendData(clusterC->data(), clusterC->size()));
 }
 
 // Test the case where a cluster is passed to AppendData() before
@@ -463,14 +514,46 @@ TEST_F(ChunkDemuxerTest, TestClusterBeforeInfoTracks) {
   AddSimpleBlock(&cb, kVideoTrackNum, 0);
   scoped_ptr<Cluster> cluster(cb.Finish());
 
-  EXPECT_FALSE(demuxer_->AppendData(cluster->data(), cluster->size()));
+  AppendData(cluster->data(), cluster->size());
 }
 
-
 // Test cases where we get an EndOfStream() call during initialization.
 TEST_F(ChunkDemuxerTest, TestEOSDuringInit) {
   EXPECT_CALL(*client_, DemuxerOpened(_));
   demuxer_->Init(NewExpectedStatusCB(DEMUXER_ERROR_COULD_NOT_OPEN));
   demuxer_->EndOfStream(PIPELINE_OK);
 }
+
+TEST_F(ChunkDemuxerTest, TestDecodeErrorEndOfStream) {
+  InitDemuxer(true, true);
+
+  ClusterBuilder cb;
+  cb.SetClusterTimecode(0);
+  AddSimpleBlock(&cb, kAudioTrackNum, 0);
+  AddSimpleBlock(&cb, kVideoTrackNum, 0);
+  AddSimpleBlock(&cb, kAudioTrackNum, 23);
+  AddSimpleBlock(&cb, kVideoTrackNum, 33);
+  scoped_ptr<Cluster> cluster(cb.Finish());
+  AppendData(cluster->data(), cluster->size());
+
+  EXPECT_CALL(mock_filter_host_, SetError(PIPELINE_ERROR_DECODE));
+  demuxer_->EndOfStream(PIPELINE_ERROR_DECODE);
+}
+
+TEST_F(ChunkDemuxerTest, TestNetworkErrorEndOfStream) {
+  InitDemuxer(true, true);
+
+  ClusterBuilder cb;
+  cb.SetClusterTimecode(0);
+  AddSimpleBlock(&cb, kAudioTrackNum, 0);
+  AddSimpleBlock(&cb, kVideoTrackNum, 0);
+  AddSimpleBlock(&cb, kAudioTrackNum, 23);
+  AddSimpleBlock(&cb, kVideoTrackNum, 33);
+  scoped_ptr<Cluster> cluster(cb.Finish());
+  AppendData(cluster->data(), cluster->size());
+
+  EXPECT_CALL(mock_filter_host_, SetError(PIPELINE_ERROR_NETWORK));
+  demuxer_->EndOfStream(PIPELINE_ERROR_NETWORK);
+}
+
 }  // namespace media