Make ChunkDemuxer error handling more consistent and robust.

media/filters/chunk_demuxer.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Implements a Demuxer that can switch among different data sources mid-stream.
 // Uses FFmpegDemuxer under the covers, so see the caveats at the top of
 // ffmpeg_demuxer.h.
 
 #include "media/filters/chunk_demuxer.h"
 
@@ skipping 305 matching lines @@
 
   Shutdown();
 
   callback->Run();
   delete 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() {
   base::AutoLock auto_lock(lock_);
   audio_ = NULL;
 }
 
 void ChunkDemuxer::SetPreload(Preload preload) {}
 
 // Demuxer implementation.
@@ skipping 40 matching lines @@
   int64 buffered_bytes = 0;
   base::TimeDelta buffered_ts = base::TimeDelta::FromSeconds(-1);
 
   FilterStatusCB cb;
   {
     base::AutoLock auto_lock(lock_);
     switch(state_) {
       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;
 
       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;

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

This break; is necessary?

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

No. Done.

     }
 
     seek_waits_for_data_ = false;
 
     base::TimeDelta tmp;
     if (audio_.get() && audio_->GetLastBufferTimestamp(&tmp) &&
         tmp > buffered_ts) {
       buffered_ts = tmp;
     }
 
@@ skipping 21 matching lines @@
 
   if (!cb.is_null())
     cb.Run(PIPELINE_OK);
 
   return true;
 }
 
 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;
   }
 
   // Create an end of stream buffer.
   ChunkDemuxerStream::BufferQueue buffers;
   buffers.push_back(CreateEOSBuffer());
 
   if (audio_.get())
     audio_->AddBuffers(buffers);
 
   if (video_.get())
     video_->AddBuffers(buffers);
 }
 
 bool ChunkDemuxer::HasEnded() {
   base::AutoLock auto_lock(lock_);
   return (state_ == ENDED);
 }
 
-
 void ChunkDemuxer::Shutdown() {
   VLOG(1) << "Shutdown()";
   FilterStatusCB cb;
   {
     base::AutoLock auto_lock(lock_);
 
     if (state_ == SHUTDOWN)
       return;
 
     std::swap(cb, seek_cb_);
@@ skipping 22 matching lines @@
   DCHECK(data);
   DCHECK_GT(size, 0);
 
   DCHECK_EQ(state_, INITIALIZING);
 
   const uint8* cur = data;
   int cur_size = 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;
 
   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);
 
   cluster_parser_.reset(new WebMClusterParser(
       info_parser.timecode_scale(),
       tracks_parser.audio_track_num(),
       tracks_parser.audio_default_duration(),
       tracks_parser.video_track_num(),
       tracks_parser.video_default_duration()));
 
   format_context_ = CreateFormatContext(data, size);
 
   if (!format_context_ || !SetupStreams()) {
-    InitFailed_Locked();
     return false;
   }
 
   ChangeState(INITIALIZED);
   init_cb_.Run(PIPELINE_OK);
   init_cb_.Reset();
   return true;
 }
 
 AVFormatContext* ChunkDemuxer::CreateFormatContext(const uint8* data,
@@ skipping 96 matching lines @@
     cur_size -= res;
   }
 
   // TODO(acolwell) : make this more representative of what is actually
   // buffered.
   buffered_bytes_ += 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