Forward decoder errors from FFmpegVideoDecodeEngine to FFmpegVideoDecoder.

Previously they were getting dropped on the floor, which could leave the media pipeline in lame duck mode.

BUG=80187
TEST=media_unittests

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

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

8 files changed, 113 insertions, 32 deletions

diff --git a/media/base/filters.h b/media/base/filters.h
index a3e5afb..7c13bea 100644
--- a/media/base/filters.h
+++ b/media/base/filters.h
@@ -204,6 +204,8 @@ class VideoDecoder : public Filter {
   virtual void ProduceVideoFrame(scoped_refptr<VideoFrame> frame) = 0;
 
   // Installs a permanent callback for passing decoded video output.
+  //
+  // A NULL frame represents a decoding error.
   typedef base::Callback<void(scoped_refptr<VideoFrame>)> ConsumeVideoFrameCB;
   void set_consume_video_frame_callback(const ConsumeVideoFrameCB& callback) {
     consume_video_frame_callback_ = callback;
diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc
index 81af909..2120022 100644
--- a/media/filters/ffmpeg_video_decoder.cc
+++ b/media/filters/ffmpeg_video_decoder.cc
@@ -209,7 +209,7 @@ void FFmpegVideoDecoder::OnSeekComplete() {
 }
 
 void FFmpegVideoDecoder::OnError() {
-  NOTIMPLEMENTED();
+  VideoFrameReady(NULL);
 }
 
 void FFmpegVideoDecoder::OnFormatChange(VideoStreamInfo stream_info) {
diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc
index 75c1c2d..54fb1be 100644
--- a/media/filters/ffmpeg_video_decoder_unittest.cc
+++ b/media/filters/ffmpeg_video_decoder_unittest.cc
@@ -145,10 +145,11 @@ class FFmpegVideoDecoderTest : public testing::Test {
     renderer_ = new MockVideoRenderer();
     engine_ = new StrictMock<MockVideoDecodeEngine>();
 
-    DCHECK(decoder_);
-
     // Inject mocks and prepare a demuxer stream.
     decoder_->set_host(&host_);
+    decoder_->set_consume_video_frame_callback(
+        base::Bind(&MockVideoRenderer::ConsumeVideoFrame,
+                   base::Unretained(renderer_.get())));
     decoder_->SetVideoDecodeEngineForTest(engine_);
     demuxer_ = new StrictMock<MockFFmpegDemuxerStream>();
 
@@ -269,6 +270,15 @@ TEST_F(FFmpegVideoDecoderTest, Initialize_Successful) {
   EXPECT_EQ(kHeight, height);
 }
 
+TEST_F(FFmpegVideoDecoderTest, OnError) {
+  InitializeDecoderSuccessfully();
+
+  scoped_refptr<VideoFrame> null_frame;
+  EXPECT_CALL(*renderer_, ConsumeVideoFrame(null_frame));
+  engine_->event_handler_->OnError();
+}
+
+
 ACTION_P2(ReadFromDemux, decoder, buffer) {
   decoder->ProduceVideoSample(buffer);
 }
@@ -311,10 +321,6 @@ TEST_F(FFmpegVideoDecoderTest, DoDecode_TestStateTransition) {
   //   5) All state transitions happen as expected.
   InitializeDecoderSuccessfully();
 
-  decoder_->set_consume_video_frame_callback(
-      base::Bind(&MockVideoRenderer::ConsumeVideoFrame,
-                 base::Unretained(renderer_.get())));
-
   // Setup initial state and check that it is sane.
   ASSERT_EQ(FFmpegVideoDecoder::kNormal, decoder_->state_);
   ASSERT_TRUE(base::TimeDelta() == decoder_->pts_stream_.current_pts());
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index 78fa6f4..92bedba 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -175,8 +175,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder,
                         &surface_type_,
                         &surface_format_,
                         &width_, &height_)) {
-    host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED);
-    state_ = kError;
+    EnterErrorState_Locked(PIPELINE_ERROR_INITIALIZATION_FAILED);
     return;
   }
   host()->SetVideoSize(width_, height_);
@@ -185,8 +184,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder,
   // TODO(scherkus): do we trust subclasses not to do something silly while
   // we're holding the lock?
   if (!OnInitialize(decoder)) {
-    host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED);
-    state_ = kError;
+    EnterErrorState_Locked(PIPELINE_ERROR_INITIALIZATION_FAILED);
     return;
   }
 
@@ -199,8 +197,7 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder,
   // Create our video thread.
   if (!base::PlatformThread::Create(0, this, &thread_)) {
     NOTREACHED() << "Video thread creation failed";
-    host()->SetError(PIPELINE_ERROR_INITIALIZATION_FAILED);
-    state_ = kError;
+    EnterErrorState_Locked(PIPELINE_ERROR_INITIALIZATION_FAILED);
     return;
   }
 
@@ -389,12 +386,19 @@ void VideoRendererBase::PutCurrentFrame(scoped_refptr<VideoFrame> frame) {
 }
 
 void VideoRendererBase::ConsumeVideoFrame(scoped_refptr<VideoFrame> frame) {
-  PipelineStatistics statistics;
-  statistics.video_frames_decoded = 1;
-  statistics_callback_->Run(statistics);
+  if (frame) {
+    PipelineStatistics statistics;
+    statistics.video_frames_decoded = 1;
+    statistics_callback_->Run(statistics);
+  }
 
   base::AutoLock auto_lock(lock_);
 
+  if (!frame) {
+    EnterErrorState_Locked(PIPELINE_ERROR_DECODE);
+    return;
+  }
+
   // Decoder could reach seek state before our Seek() get called.
   // We will enter kSeeking
   if (kFlushed == state_)
@@ -570,4 +574,41 @@ base::TimeDelta VideoRendererBase::CalculateSleepDuration(
       static_cast<int64>(sleep.InMicroseconds() / playback_rate));
 }
 
+void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
+  lock_.AssertAcquired();
+
+  scoped_ptr<FilterCallback> callback;
+  switch (state_) {
+    case kUninitialized:
+    case kPrerolled:
+    case kPaused:
+    case kFlushed:
+    case kEnded:
+    case kPlaying:
+      break;
+
+    case kFlushing:
+      CHECK(flush_callback_.get());
+      callback.swap(flush_callback_);
+      break;
+
+    case kSeeking:
+      CHECK(seek_callback_.get());
+      callback.swap(seek_callback_);
+      break;
+
+    case kStopped:
+      NOTREACHED() << "Should not error if stopped.";
+      return;
+
+    case kError:
+      return;
+  }
+  state_ = kError;
+  host()->SetError(status);
+
+  if (callback.get())
+    callback->Run();
+}
+
 }  // namespace media
diff --git a/media/filters/video_renderer_base.h b/media/filters/video_renderer_base.h
index 5047191..66eba6e 100644
--- a/media/filters/video_renderer_base.h
+++ b/media/filters/video_renderer_base.h
@@ -22,6 +22,7 @@
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
 #include "media/base/filters.h"
+#include "media/base/pipeline_status.h"
 #include "media/base/video_frame.h"
 
 namespace media {
@@ -130,6 +131,9 @@ class VideoRendererBase : public VideoRenderer,
   base::TimeDelta CalculateSleepDuration(VideoFrame* next_frame,
                                          float playback_rate);
 
+  // Safely handles entering to an error state.
+  void EnterErrorState_Locked(PipelineStatus status);
+
   // Used for accessing data members.
   base::Lock lock_;
 
diff --git a/media/filters/video_renderer_base_unittest.cc b/media/filters/video_renderer_base_unittest.cc
index 3e0578c..c21d6c7 100644
--- a/media/filters/video_renderer_base_unittest.cc
+++ b/media/filters/video_renderer_base_unittest.cc
@@ -110,16 +110,20 @@ class VideoRendererBaseTest : public ::testing::Test {
     Seek(0);
   }
 
-  void Seek(int64 timestamp) {
-    EXPECT_CALL(*renderer_, OnFrameAvailable());
+  void StartSeeking(int64 timestamp) {
     EXPECT_FALSE(seeking_);
 
-    // Now seek to trigger prerolling.
+    // Seek to trigger prerolling.
     seeking_ = true;
     renderer_->Seek(base::TimeDelta::FromMicroseconds(timestamp),
                     NewCallback(this, &VideoRendererBaseTest::OnSeekComplete));
+  }
+
+  void FinishSeeking() {
+    EXPECT_CALL(*renderer_, OnFrameAvailable());
+    EXPECT_TRUE(seeking_);
 
-    // Now satisfy the read requests.  The callback must be executed in order
+    // Satisfy the read requests.  The callback must be executed in order
     // to exit the loop since VideoRendererBase can read a few extra frames
     // after |timestamp| in order to preroll.
     for (int64 i = 0; seeking_; ++i) {
@@ -127,6 +131,11 @@ class VideoRendererBaseTest : public ::testing::Test {
     }
   }
 
+  void Seek(int64 timestamp) {
+    StartSeeking(timestamp);
+    FinishSeeking();
+  }
+
   void Flush() {
     renderer_->Pause(NewExpectedCallback());
 
@@ -136,6 +145,10 @@ class VideoRendererBaseTest : public ::testing::Test {
     renderer_->Flush(NewExpectedCallback());
   }
 
+  void CreateError() {
+    decoder_->VideoFrameReadyForTest(NULL);
+  }
+
   void CreateFrame(int64 timestamp, int64 duration) {
     const base::TimeDelta kZero;
     scoped_refptr<VideoFrame> frame;
@@ -244,6 +257,25 @@ TEST_F(VideoRendererBaseTest, Play) {
   Flush();
 }
 
+TEST_F(VideoRendererBaseTest, Error_Playing) {
+  Initialize();
+  renderer_->Play(NewExpectedCallback());
+
+  EXPECT_CALL(host_, SetError(PIPELINE_ERROR_DECODE));
+  CreateError();
+  Flush();
+}
+
+TEST_F(VideoRendererBaseTest, Error_Seeking) {
+  Initialize();
+  Flush();
+  StartSeeking(0);
+
+  EXPECT_CALL(host_, SetError(PIPELINE_ERROR_DECODE));
+  CreateError();
+  Flush();
+}
+
 TEST_F(VideoRendererBaseTest, Seek_Exact) {
   Initialize();
   Flush();
diff --git a/media/video/ffmpeg_video_decode_engine.cc b/media/video/ffmpeg_video_decode_engine.cc
index 4ebfb30..0413092 100644
--- a/media/video/ffmpeg_video_decode_engine.cc
+++ b/media/video/ffmpeg_video_decode_engine.cc
@@ -244,12 +244,11 @@ void FFmpegVideoDecodeEngine::DecodeFrame(scoped_refptr<Buffer> buffer) {
 
   // Log the problem if we can't decode a video frame and exit early.
   if (result < 0) {
-    VLOG(1) << "Error decoding a video frame with timestamp: "
-            << buffer->GetTimestamp().InMicroseconds() << " us, duration: "
-            << buffer->GetDuration().InMicroseconds() << " us, packet size: "
-            << buffer->GetDataSize() << " bytes";
-    // TODO(jiesun): call event_handler_->OnError() instead.
-    event_handler_->ConsumeVideoFrame(video_frame, statistics);
+    LOG(ERROR) << "Error decoding a video frame with timestamp: "
+               << buffer->GetTimestamp().InMicroseconds() << " us, duration: "
+               << buffer->GetDuration().InMicroseconds() << " us, packet size: "
+               << buffer->GetDataSize() << " bytes";
+    event_handler_->OnError();
     return;
   }
 
@@ -274,8 +273,7 @@ void FFmpegVideoDecodeEngine::DecodeFrame(scoped_refptr<Buffer> buffer) {
   if (!av_frame_->data[VideoFrame::kYPlane] ||
       !av_frame_->data[VideoFrame::kUPlane] ||
       !av_frame_->data[VideoFrame::kVPlane]) {
-    // TODO(jiesun): call event_handler_->OnError() instead.
-    event_handler_->ConsumeVideoFrame(video_frame, statistics);
+    event_handler_->OnError();
     return;
   }
 
diff --git a/media/video/ffmpeg_video_decode_engine_unittest.cc b/media/video/ffmpeg_video_decode_engine_unittest.cc
index 994d2c5..8a53f6d 100644
--- a/media/video/ffmpeg_video_decode_engine_unittest.cc
+++ b/media/video/ffmpeg_video_decode_engine_unittest.cc
@@ -290,11 +290,9 @@ TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_DecodeError) {
 
   EXPECT_CALL(*this, ProduceVideoSample(_))
       .WillOnce(DemuxComplete(test_engine_.get(), buffer_));
-  EXPECT_CALL(*this, ConsumeVideoFrame(_, _))
-      .WillOnce(DecodeComplete(this));
-  test_engine_->ProduceVideoFrame(video_frame_);
+  EXPECT_CALL(*this, OnError());
 
-  EXPECT_FALSE(video_frame_.get());
+  test_engine_->ProduceVideoFrame(video_frame_);
 }
 
 TEST_F(FFmpegVideoDecodeEngineTest, DecodeFrame_LargerWidth) {