Make ChunkDemuxer error handling more consistent and robust.

This fixes a variety of error cases exposed by the WebKit LayoutTests I created to test this code. The ChunkDemuxer unit tests were updated to expose these cases as well.

BUG=86536
TEST=ChunkDemuxerTest.*

Review URL: http://codereview.chromium.org/7538027

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

3 files changed, 170 insertions, 65 deletions

diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index 9a55e15..9da955a 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -323,17 +323,22 @@ void ChunkDemuxer::Stop(FilterCallback* callback) {
 void ChunkDemuxer::Seek(base::TimeDelta time, const FilterStatusCB&  cb) {
   VLOG(1) << "Seek(" << time.InSecondsF() << ")";
 
+  PipelineStatus status = PIPELINE_ERROR_INVALID_STATE;
   {
     base::AutoLock auto_lock(lock_);
 
-    if (seek_waits_for_data_) {
-      VLOG(1) << "Seek() : waiting for more data to arrive.";
-      seek_cb_ = cb;
-      return;
+    if (state_ == INITIALIZED || state_ == ENDED) {
+      if (seek_waits_for_data_) {
+        VLOG(1) << "Seek() : waiting for more data to arrive.";
+        seek_cb_ = cb;
+        return;
+      }
+
+      status = PIPELINE_OK;
     }
   }
 
-  cb.Run(PIPELINE_OK);
+  cb.Run(status);
 }
 
 void ChunkDemuxer::OnAudioRendererDisabled() {
@@ -394,7 +399,7 @@ bool ChunkDemuxer::AppendData(const uint8* data, unsigned length) {
       case INITIALIZING:
         if (!ParseInfoAndTracks_Locked(data, length)) {
           VLOG(1) << "AppendData(): parsing info & tracks failed";
-          return false;
+          ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN);
         }
         return true;
         break;
@@ -402,17 +407,17 @@ bool ChunkDemuxer::AppendData(const uint8* data, unsigned length) {
       case INITIALIZED:
         if (!ParseAndAppendData_Locked(data, length)) {
           VLOG(1) << "AppendData(): parsing data failed";
-          return false;
+          ReportError_Locked(PIPELINE_ERROR_DECODE);
+          return true;
         }
         break;
 
       case WAITING_FOR_INIT:
       case ENDED:
-      case INIT_ERROR:
+      case PARSE_ERROR:
       case SHUTDOWN:
         VLOG(1) << "AppendData(): called in unexpected state " << state_;
         return false;
-        break;
     }
 
     seek_waits_for_data_ = false;
@@ -454,21 +459,21 @@ bool ChunkDemuxer::AppendData(const uint8* data, unsigned length) {
 void ChunkDemuxer::EndOfStream(PipelineStatus status) {
   VLOG(1) << "EndOfStream(" << status << ")";
   base::AutoLock auto_lock(lock_);
-  DCHECK((state_ == INITIALIZING) || (state_ == INITIALIZED) ||
-         (state_ == SHUTDOWN));
+  DCHECK_NE(state_, WAITING_FOR_INIT);
+  DCHECK_NE(state_, ENDED);
 
-  if (state_ == SHUTDOWN)
+  if (state_ == SHUTDOWN || state_ == PARSE_ERROR)
     return;
 
   if (state_ == INITIALIZING) {
-    InitFailed_Locked();
+    ReportError_Locked(DEMUXER_ERROR_COULD_NOT_OPEN);
     return;
   }
 
   ChangeState(ENDED);
 
   if (status != PIPELINE_OK) {
-    host()->SetError(status);
+    ReportError_Locked(status);
     return;
   }
 
@@ -488,7 +493,6 @@ bool ChunkDemuxer::HasEnded() {
   return (state_ == ENDED);
 }
 
-
 void ChunkDemuxer::Shutdown() {
   VLOG(1) << "Shutdown()";
   FilterStatusCB cb;
@@ -531,10 +535,8 @@ bool ChunkDemuxer::ParseInfoAndTracks_Locked(const uint8* data, int size) {
   WebMInfoParser info_parser;
   int res = info_parser.Parse(cur, cur_size);
 
-  if (res <= 0) {
-    InitFailed_Locked();
+  if (res <= 0)
     return false;
-  }
 
   cur += res;
   cur_size -= res;
@@ -542,10 +544,8 @@ bool ChunkDemuxer::ParseInfoAndTracks_Locked(const uint8* data, int size) {
   WebMTracksParser tracks_parser(info_parser.timecode_scale());
   res = tracks_parser.Parse(cur, cur_size);
 
-  if (res <= 0) {
-    InitFailed_Locked();
+  if (res <= 0)
     return false;
-  }
 
   double mult = info_parser.timecode_scale() / 1000.0;
   duration_ = base::TimeDelta::FromMicroseconds(info_parser.duration() * mult);
@@ -560,7 +560,6 @@ bool ChunkDemuxer::ParseInfoAndTracks_Locked(const uint8* data, int size) {
   format_context_ = CreateFormatContext(data, size);
 
   if (!format_context_ || !SetupStreams()) {
-    InitFailed_Locked();
     return false;
   }
 
@@ -677,11 +676,34 @@ bool ChunkDemuxer::ParseAndAppendData_Locked(const uint8* data, int length) {
   return true;
 }
 
-void ChunkDemuxer::InitFailed_Locked() {
-  ChangeState(INIT_ERROR);
+void ChunkDemuxer::ReportError_Locked(PipelineStatus error) {
+  DCHECK_NE(error, PIPELINE_OK);
+
+  ChangeState(PARSE_ERROR);
+
   PipelineStatusCB cb;
-  std::swap(cb, init_cb_);
-  cb.Run(DEMUXER_ERROR_COULD_NOT_OPEN);
+
+  if (!init_cb_.is_null()) {
+    std::swap(cb, init_cb_);
+  } else {
+    if (!seek_cb_.is_null())
+      std::swap(cb, seek_cb_);
+
+    if (audio_.get())
+      audio_->Shutdown();
+
+    if (video_.get())
+      video_->Shutdown();
+  }
+
+  {
+    base::AutoUnlock auto_unlock(lock_);
+    if (cb.is_null()) {
+      host()->SetError(error);
+      return;
+    }
+    cb.Run(error);
+  }
 }
 
 }  // namespace media
diff --git a/media/filters/chunk_demuxer.h b/media/filters/chunk_demuxer.h
index b39e7af..84e224d 100644
--- a/media/filters/chunk_demuxer.h
+++ b/media/filters/chunk_demuxer.h
@@ -41,6 +41,9 @@ class ChunkDemuxer : public Demuxer {
 
   // Methods used by an external object to control this demuxer.
   void FlushData();
+
+  // Appends media data to the stream. Returns false if this method
+  // is called in an invalid state.
   bool AppendData(const uint8* data, unsigned length);
   void EndOfStream(PipelineStatus status);
   bool HasEnded();
@@ -52,7 +55,7 @@ class ChunkDemuxer : public Demuxer {
     INITIALIZING,
     INITIALIZED,
     ENDED,
-    INIT_ERROR,
+    PARSE_ERROR,
     SHUTDOWN,
   };
 
@@ -76,8 +79,9 @@ class ChunkDemuxer : public Demuxer {
   // contain one or more WebM Clusters. Returns false if parsing the data fails.
   bool ParseAndAppendData_Locked(const uint8* data, int length);
 
-  // Called when initialization fails. Handles calling & clearing init_cb_.
-  void InitFailed_Locked();
+  // Reports an error and puts the demuxer in a state where it won't accept more
+  // data.
+  void ReportError_Locked(PipelineStatus error);
 
   base::Lock lock_;
   State state_;
diff --git a/media/filters/chunk_demuxer_unittest.cc b/media/filters/chunk_demuxer_unittest.cc
index 13d0c93..b26b757 100644
--- a/media/filters/chunk_demuxer_unittest.cc
+++ b/media/filters/chunk_demuxer_unittest.cc
@@ -9,11 +9,13 @@
 #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;
@@ -149,6 +151,13 @@ 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());
+    EXPECT_TRUE(demuxer_->AppendData(data, length));
+  }
+
   void AppendInfoTracks(bool has_audio, bool has_video) {
     EXPECT_CALL(mock_ffmpeg_, AVOpenInputFile(_, _, NULL, 0, NULL))
         .WillOnce(DoAll(SetArgumentPointee<0>(&format_context_),
@@ -168,7 +177,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 +201,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() {
@@ -215,6 +227,7 @@ class ChunkDemuxerTest : public testing::Test{
   MOCK_METHOD1(Checkpoint, void(int id));
 
   MockFFmpeg mock_ffmpeg_;
+  MockFilterHost mock_filter_host_;
 
   AVFormatContext format_context_;
   AVCodecContext codecs_[MAX_CODECS_INDEX];
@@ -299,7 +312,7 @@ TEST_F(ChunkDemuxerTest, TestAppendDataAfterSeek) {
 
   Checkpoint(1);
 
-  EXPECT_TRUE(demuxer_->AppendData(cluster->data(), cluster->size()));
+  AppendData(cluster->data(), cluster->size());
 
   Checkpoint(2);
 }
@@ -345,7 +358,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 +376,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 +389,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 +414,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 +463,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 +510,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