Make ChunkDemuxer error handling more consistent and robust.

media/filters/chunk_demuxer.cc

Index: media/filters/chunk_demuxer.cc
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index 9a55e15520ec99609953998f1b63e1cea9e7cc96..5dc4eea96200bebb941aeffab6866c3f45842fc2 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,13 +407,14 @@ 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;
@@ -454,21 +460,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 +494,6 @@ bool ChunkDemuxer::HasEnded() {
   return (state_ == ENDED);
 }
 
-
 void ChunkDemuxer::Shutdown() {
   VLOG(1) << "Shutdown()";
   FilterStatusCB cb;
@@ -531,10 +536,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 +545,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 +561,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 +677,41 @@ bool ChunkDemuxer::ParseAndAppendData_Locked(const uint8* data, int length) {
   return true;
 }
 
-void ChunkDemuxer::InitFailed_Locked() {
-  ChangeState(INIT_ERROR);
+void ChunkDemuxer::ReportError_Locked(PipelineStatus status) {

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

FWIW, when you know status is not OK, I prefer naming the variable 'error'.

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

Done.

+  DCHECK_NE(status, PIPELINE_OK);
+
+  ChangeState(PARSE_ERROR);
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      NewRunnableMethod(this, &ChunkDemuxer::ReportErrorTask, status));
+}
+
+void ChunkDemuxer::ReportErrorTask(PipelineStatus status) {
   PipelineStatusCB cb;
-  std::swap(cb, init_cb_);
-  cb.Run(DEMUXER_ERROR_COULD_NOT_OPEN);
+  {
+    base::AutoLock auto_lock(lock_);
+    if (state_ != PARSE_ERROR)

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

When can this happen?

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

This can happen if Stop() is called before this task executes. In that case we
don't want to report the error because we've been shutdown already.

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

Add a comment?

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

No longer necessary now that we aren't posting a task.

+      return;
+
+    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();
+    }
+  }
+
+  if (cb.is_null()) {
+    host()->SetError(status);
+    return;
+  }
+  cb.Run(status);

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

Seems strange that in this case host()->SetError() isn't called.

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

SetError() is only used for error reporting when we don't have an outstanding
async call. In the context of this class it means that SetError() will only be
called during playback. 

Eventually I'd like to get rid of SetError() and have Play() take an error
callback so that the scope of this alternative error reporting path is easier to
see. Once this happens we'll always call cb to report an error and the code will
look more consistent.

 }
 
 }  // namespace media