Report VideoDecoder status through ReadCB instead of through FilterHost.

BUG=108340
TEST=media_unittests

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

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

content/renderer/media/capture_video_decoder.cc

@@ -279,7 +279,5 @@ void CaptureVideoDecoder::OnBufferReadyOnDecoderThread(
 void CaptureVideoDecoder::DeliverFrame(
     const scoped_refptr<media::VideoFrame>& video_frame) {
   // Reset the callback before running to protect against reentrancy.
-  ReadCB read_cb = read_cb_;
-  read_cb_.Reset();
-  read_cb.Run(video_frame);
+  base::ResetAndReturn(&read_cb_).Run(kOk, video_frame);
 }

content/renderer/media/capture_video_decoder_unittest.cc

@@ -9,7 +9,6 @@
 #include "media/base/filters.h"
 #include "media/base/limits.h"
 #include "media/base/mock_callback.h"
-#include "media/base/mock_filter_host.h"
 #include "media/base/mock_filters.h"
 #include "media/base/pipeline_status.h"
 #include "media/video/capture/video_capture_types.h"
@@ -98,7 +97,6 @@ class CaptureVideoDecoderTest : public ::testing::Test {
 
     decoder_ = new CaptureVideoDecoder(message_loop_proxy_,
                                        kVideoStreamId, vc_manager_, capability);
-    decoder_->set_host(&host_);
     EXPECT_CALL(statistics_cb_object_, OnStatistics(_))
         .Times(AnyNumber());
 
@@ -120,7 +118,7 @@ class CaptureVideoDecoderTest : public ::testing::Test {
 
   void Initialize() {
     // Issue a read.
-    EXPECT_CALL(*this, FrameReady(_));
+    EXPECT_CALL(*this, FrameReady(media::VideoDecoder::kOk, _));
     decoder_->Read(read_cb_);
 
     EXPECT_CALL(*vc_manager_, AddDevice(_, _))
@@ -148,7 +146,7 @@ class CaptureVideoDecoderTest : public ::testing::Test {
 
   void Flush() {
     // Issue a read.
-    EXPECT_CALL(*this, FrameReady(_));
+    EXPECT_CALL(*this, FrameReady(media::VideoDecoder::kOk, _));
     decoder_->Read(read_cb_);
 
     decoder_->Pause(media::NewExpectedClosure());
@@ -170,14 +168,14 @@ class CaptureVideoDecoderTest : public ::testing::Test {
     return static_cast<media::VideoCapture::EventHandler*>(decoder_);
   }
 
-  MOCK_METHOD1(FrameReady, void(scoped_refptr<media::VideoFrame>));
+  MOCK_METHOD2(FrameReady, void(media::VideoDecoder::DecoderStatus status,
+                                scoped_refptr<media::VideoFrame>));
 
   // Fixture members.
   scoped_refptr<CaptureVideoDecoder> decoder_;
   scoped_refptr<MockVideoCaptureImplManager> vc_manager_;
   scoped_ptr<MockVideoCaptureImpl> vc_impl_;
   media::MockStatisticsCB statistics_cb_object_;
-  StrictMock<media::MockFilterHost> host_;
   scoped_ptr<MessageLoop> message_loop_;
   scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
   media::VideoDecoder::ReadCB read_cb_;

content/renderer/media/rtc_video_decoder.cc

@@ -10,7 +10,6 @@
 #include "base/callback.h"
 #include "base/message_loop.h"
 #include "media/base/demuxer.h"
-#include "media/base/filter_host.h"
 #include "media/base/filters.h"
 #include "media/base/limits.h"
 #include "media/base/video_frame.h"
@@ -107,7 +106,7 @@ void RTCVideoDecoder::Flush(const base::Closure& callback) {
     scoped_refptr<media::VideoFrame> video_frame =
         media::VideoFrame::CreateBlackFrame(visible_size_.width(),
                                             visible_size_.height());
-    read_cb.Run(video_frame);
+    read_cb.Run(kOk, video_frame);
   }
 
   VideoDecoder::Flush(callback);
@@ -161,13 +160,6 @@ const gfx::Size& RTCVideoDecoder::natural_size() {
 
 bool RTCVideoDecoder::SetSize(int width, int height, int reserved) {
   visible_size_.SetSize(width, height);
-
-  // TODO(vrk): Provide natural size when aspect ratio support is implemented.
-
-  // TODO(xhwang) host() can be NULL after r128289.  Remove this check when
-  // it is no longer needed.
-  if (host())
-    host()->SetNaturalVideoSize(visible_size_);
   return true;
 }
 
@@ -224,6 +216,6 @@ bool RTCVideoDecoder::RenderFrame(const cricket::VideoFrame* frame) {
   CopyUPlane(frame->GetUPlane(), frame->GetUPitch(), uv_rows, video_frame);
   CopyVPlane(frame->GetVPlane(), frame->GetVPitch(), uv_rows, video_frame);
 
-  read_cb.Run(video_frame);
+  read_cb.Run(kOk, video_frame);
   return true;
 }

content/renderer/media/rtc_video_decoder_unittest.cc

@@ -14,7 +14,6 @@
 #include "media/base/filters.h"
 #include "media/base/limits.h"
 #include "media/base/mock_callback.h"
-#include "media/base/mock_filter_host.h"
 #include "media/base/mock_filters.h"
 #include "media/base/video_frame.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -32,7 +31,6 @@ using ::testing::WithArg;
 using ::testing::Invoke;
 using media::MockStatisticsCB;
 using media::MockVideoRenderer;
-using media::MockFilterHost;
 using media::NewExpectedClosure;
 using media::NewExpectedStatusCB;
 using media::PipelineStatistics;
@@ -43,8 +41,8 @@ namespace {
 
 class NullVideoFrame : public cricket::VideoFrame {
  public:
-  NullVideoFrame() {};
-  virtual ~NullVideoFrame() {};
+  NullVideoFrame() {}
+  virtual ~NullVideoFrame() {}
 
   virtual bool Reset(uint32 fourcc, int w, int h, int dw, int dh,
                      uint8 *sample, size_t sample_size,
@@ -135,9 +133,6 @@ class RTCVideoDecoderTest : public testing::Test {
 
     DCHECK(decoder_);
 
-    // Inject mocks and prepare a demuxer stream.
-    decoder_->set_host(&host_);
-
     EXPECT_CALL(stats_callback_object_, OnStatistics(_))
         .Times(AnyNumber());
   }
@@ -159,13 +154,13 @@ class RTCVideoDecoderTest : public testing::Test {
                       base::Unretained(&stats_callback_object_));
   }
 
-  MOCK_METHOD1(FrameReady, void(scoped_refptr<media::VideoFrame>));
+  MOCK_METHOD2(FrameReady, void(media::VideoDecoder::DecoderStatus status,
+                                scoped_refptr<media::VideoFrame>));
 
   // Fixture members.
   scoped_refptr<RTCVideoDecoder> decoder_;
   scoped_refptr<MockVideoRenderer> renderer_;
   MockStatisticsCB stats_callback_object_;
-  StrictMock<MockFilterHost> host_;
   MessageLoop message_loop_;
   media::VideoDecoder::ReadCB read_cb_;
 
@@ -204,7 +199,7 @@ TEST_F(RTCVideoDecoderTest, DoFlush) {
 
   InitializeDecoderSuccessfully();
 
-  EXPECT_CALL(*this, FrameReady(_));
+  EXPECT_CALL(*this, FrameReady(media::VideoDecoder::kOk, _));
   decoder_->Read(read_cb_);
   decoder_->Pause(media::NewExpectedClosure());
   decoder_->Flush(media::NewExpectedClosure());
@@ -215,7 +210,6 @@ TEST_F(RTCVideoDecoderTest, DoFlush) {
 
 TEST_F(RTCVideoDecoderTest, DoRenderFrame) {
   const base::TimeDelta kZero;
-  EXPECT_CALL(host_, GetTime()).WillRepeatedly(Return(base::TimeDelta()));
 
   InitializeDecoderSuccessfully();
 
@@ -237,9 +231,6 @@ TEST_F(RTCVideoDecoderTest, DoSetSize) {
   gfx::Size new_natural_size(new_width, new_height);
   int new_reserved = 0;
 
-  EXPECT_CALL(host_,
-              SetNaturalVideoSize(new_natural_size)).WillRepeatedly(Return());
-
   decoder_->SetSize(new_width, new_height, new_reserved);
 
   EXPECT_EQ(new_width, decoder_->natural_size().width());

media/base/filters.cc

@@ -61,6 +61,8 @@ VideoDecoder::VideoDecoder() {}
 
 VideoDecoder::~VideoDecoder() {}
 
+// TODO(xhwang): Remove the following four functions when VideoDecoder is not a
+// Filter any more. See bug: http://crbug.com/108340
 void VideoDecoder::Play(const base::Closure& /* callback */) {
   LOG(FATAL) << "VideoDecoder::Play is not supposed to be called.";
 }
@@ -74,6 +76,11 @@ void VideoDecoder::Seek(base::TimeDelta /* time */,
   LOG(FATAL) << "VideoDecoder::Seek is not supposed to be called.";
 }
 
+FilterHost* VideoDecoder::host() {
+  LOG(FATAL) << "VideoDecoder::host is not supposed to be called.";
+  return NULL;
+}
+
 bool VideoDecoder::HasAlpha() const {
   return false;
 }

media/base/filters.h

@@ -109,6 +109,13 @@ class MEDIA_EXPORT Filter : public base::RefCountedThreadSafe<Filter> {
 
 class MEDIA_EXPORT VideoDecoder : public Filter {
  public:
+  // Status codes for read operations on VideoDecoder.
+  enum DecoderStatus {
+    kOk,  // Everything went as planned.
+    kDecodeError,  // Decoding error happened.
+    kDecryptError  // Decrypting error happened.
+  };
+
   // Initialize a VideoDecoder with the given DemuxerStream, executing the
   // callback upon completion.
   // statistics_cb is used to update global pipeline statistics.
@@ -116,16 +123,21 @@ class MEDIA_EXPORT VideoDecoder : public Filter {
                           const PipelineStatusCB& status_cb,
                           const StatisticsCB& statistics_cb) = 0;
 
-  // Request a frame to be decoded and returned via the provided callback.
-  // Only one read may be in flight at any given time.
+  // Requests a frame to be decoded. The status of the decoder and decoded frame
+  // are returned via the provided callback. Only one read may be in flight at
+  // any given time.
   //
   // Implementations guarantee that the callback will not be called from within
   // this method.
   //
-  // Non-NULL frames contain decoded video data or may indicate the  end of
-  // the stream. NULL video frames indicate an aborted read. This can happen if
-  // the DemuxerStream gets flushed and doesn't have any more data to return.
-  typedef base::Callback<void(scoped_refptr<VideoFrame>)> ReadCB;
+  // If the returned status is not kOk, some error has occurred in the video
+  // decoder. In this case, the returned frame should always be NULL.
+  //
+  // Otherwise, the video decoder is in good shape. In this case, Non-NULL
+  // frames contain decoded video data or may indicate the end of the stream.
+  // NULL video frames indicate an aborted read. This can happen if the
+  // DemuxerStream gets flushed and doesn't have any more data to return.
+  typedef base::Callback<void(DecoderStatus, scoped_refptr<VideoFrame>)> ReadCB;
   virtual void Read(const ReadCB& read_cb) = 0;
 
   // Returns the natural width and height of decoded video in pixels.
@@ -158,11 +170,12 @@ class MEDIA_EXPORT VideoDecoder : public Filter {
   // These functions will be removed later. Declare here to make sure they are
   // not called from VideoDecoder interface anymore.
   // TODO(xhwang): Remove them when VideoDecoder is not a Filter any more.
-  // See bug: crbug.com/108340
+  // See bug: http://crbug.com/108340
   virtual void Play(const base::Closure& callback) OVERRIDE;
   virtual void Pause(const base::Closure& callback) OVERRIDE;
   virtual void Seek(base::TimeDelta time,
                     const PipelineStatusCB& callback) OVERRIDE;
+  virtual FilterHost* host() OVERRIDE;
 };
 
 class MEDIA_EXPORT VideoRenderer : public Filter {

media/filters/ffmpeg_video_decoder.cc

@@ -5,11 +5,11 @@
 #include "media/filters/ffmpeg_video_decoder.h"
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/message_loop.h"
 #include "base/string_number_conversions.h"
 #include "media/base/demuxer_stream.h"
-#include "media/base/filter_host.h"
 #include "media/base/limits.h"
 #include "media/base/media_switches.h"
 #include "media/base/pipeline.h"
@@ -212,7 +212,7 @@ void FFmpegVideoDecoder::DoRead(const ReadCB& read_cb) {
 
   // Return empty frames if decoding has finished.
   if (state_ == kDecodeFinished) {
-    read_cb.Run(VideoFrame::CreateEmptyFrame());
+    read_cb.Run(kOk, VideoFrame::CreateEmptyFrame());
     return;
   }
 
@@ -288,8 +288,7 @@ void FFmpegVideoDecoder::DoDecodeBuffer(const scoped_refptr<Buffer>& buffer) {
     unencrypted_buffer = decryptor_.Decrypt(buffer);
     if (!unencrypted_buffer || !unencrypted_buffer->GetDataSize()) {
       state_ = kDecodeFinished;
-      DeliverFrame(VideoFrame::CreateEmptyFrame());
-      host()->SetError(PIPELINE_ERROR_DECRYPT);
+      base::ResetAndReturn(&read_cb_).Run(kDecryptError, NULL);
       return;
     }
   }
@@ -297,8 +296,7 @@ void FFmpegVideoDecoder::DoDecodeBuffer(const scoped_refptr<Buffer>& buffer) {
   scoped_refptr<VideoFrame> video_frame;
   if (!Decode(unencrypted_buffer, &video_frame)) {
     state_ = kDecodeFinished;
-    DeliverFrame(VideoFrame::CreateEmptyFrame());
-    host()->SetError(PIPELINE_ERROR_DECODE);
+    base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
     return;
   }
 
@@ -423,9 +421,7 @@ bool FFmpegVideoDecoder::Decode(
 void FFmpegVideoDecoder::DeliverFrame(
     const scoped_refptr<VideoFrame>& video_frame) {
   // Reset the callback before running to protect against reentrancy.
-  ReadCB read_cb = read_cb_;
-  read_cb_.Reset();
-  read_cb.Run(video_frame);
+  base::ResetAndReturn(&read_cb_).Run(kOk, video_frame);
 }
 
 void FFmpegVideoDecoder::ReleaseFFmpegResources() {

media/filters/gpu_video_decoder.cc

@@ -53,7 +53,8 @@ GpuVideoDecoder::GpuVideoDecoder(
       decoder_texture_target_(0),
       next_picture_buffer_id_(0),
       next_bitstream_buffer_id_(0),
-      shutting_down_(false) {
+      shutting_down_(false),
+      error_occured_(false) {
   DCHECK(gvd_loop_proxy_ && factories_);
 }
 
@@ -192,8 +193,13 @@ void GpuVideoDecoder::Read(const ReadCB& read_cb) {
     return;
   }
 
+  if (error_occured_) {
+    read_cb.Run(kDecodeError, NULL);
+    return;
+  }
+
   if (!vda_) {
-    read_cb.Run(VideoFrame::CreateEmptyFrame());
+    read_cb.Run(kOk, VideoFrame::CreateEmptyFrame());
     return;
   }
 
@@ -233,7 +239,7 @@ void GpuVideoDecoder::RequestBufferDecode(const scoped_refptr<Buffer>& buffer) {
       return;
 
     gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-        pending_read_cb_, scoped_refptr<VideoFrame>()));
+        pending_read_cb_, kOk, scoped_refptr<VideoFrame>()));
     pending_read_cb_.Reset();
     return;
   }
@@ -415,7 +421,7 @@ void GpuVideoDecoder::EnqueueFrameAndTriggerFrameDelivery(
     return;
 
   gvd_loop_proxy_->PostTask(FROM_HERE, base::Bind(
-      pending_read_cb_, ready_video_frames_.front()));
+      pending_read_cb_, kOk, ready_video_frames_.front()));
   pending_read_cb_.Reset();
   ready_video_frames_.pop_front();
 }
@@ -536,8 +542,13 @@ void GpuVideoDecoder::NotifyError(media::VideoDecodeAccelerator::Error error) {
     return;
   vda_ = NULL;
   DLOG(ERROR) << "VDA Error: " << error;
-  if (host())
-    host()->SetError(PIPELINE_ERROR_DECODE);
+
+  error_occured_ = true;
+
+  if (!pending_read_cb_.is_null()) {
+    base::ResetAndReturn(&pending_read_cb_).Run(kDecodeError, NULL);
+    return;
+  }
 }
 
 }  // namespace media

media/filters/gpu_video_decoder.h

@@ -208,6 +208,9 @@ class MEDIA_EXPORT GpuVideoDecoder
   // this class not require the render thread's loop to be processing.
   bool shutting_down_;
 
+  // Indicates decoding error occurred.
+  bool error_occured_;
+
   DISALLOW_COPY_AND_ASSIGN(GpuVideoDecoder);
 };
 

media/filters/video_frame_generator.cc

@@ -80,7 +80,7 @@ void VideoFrameGenerator::ReadOnDecoderThread(const ReadCB& read_cb) {
   // TODO(wjia): set pixel data to pre-defined patterns if it's desired to
   // verify frame content.
 
-  read_cb.Run(video_frame);
+  read_cb.Run(kOk, video_frame);
 }
 
 void VideoFrameGenerator::StopOnDecoderThread(const base::Closure& callback) {

media/filters/video_renderer_base.cc

@@ -359,13 +359,28 @@ void VideoRendererBase::PutCurrentFrame(scoped_refptr<VideoFrame> frame) {
   }
 }
 
-void VideoRendererBase::FrameReady(scoped_refptr<VideoFrame> frame) {
+void VideoRendererBase::FrameReady(VideoDecoder::DecoderStatus status,
+                                   scoped_refptr<VideoFrame> frame) {
   base::AutoLock auto_lock(lock_);
   DCHECK_NE(state_, kUninitialized);
 
   CHECK(pending_read_);
   pending_read_ = false;
 
+  if (status == VideoDecoder::kDecodeError) {
+    DCHECK(!frame);
+    host()->SetError(PIPELINE_ERROR_DECODE);
+    return;
+  }
+
+  if (status == VideoDecoder::kDecryptError) {
+    DCHECK(!frame);
+    host()->SetError(PIPELINE_ERROR_DECRYPT);
+    return;
+  }
+
+  DCHECK_EQ(status, VideoDecoder::kOk);
+
   // Already-queued Decoder ReadCB's can fire after various state transitions
   // have happened; in that case just drop those frames immediately.
   if (state_ == kStopped || state_ == kError || state_ == kFlushed ||
@@ -460,8 +475,8 @@ void VideoRendererBase::AttemptRead_Locked() {
       (!ready_frames_.empty() && ready_frames_.back()->IsEndOfStream()) ||
       state_ == kFlushingDecoder ||
       state_ == kFlushing) {
-     return;
-   }
+    return;
+  }
 
   pending_read_ = true;
   decoder_->Read(base::Bind(&VideoRendererBase::FrameReady, this));

media/filters/video_renderer_base.h

@@ -74,8 +74,10 @@ class MEDIA_EXPORT VideoRendererBase
   void PutCurrentFrame(scoped_refptr<VideoFrame> frame);
 
  private:
-  // Callback from the video decoder delivering decoded video frames.
-  void FrameReady(scoped_refptr<VideoFrame> frame);
+  // Callback from the video decoder delivering decoded video frames and
+  // reporting video decoder status.
+  void FrameReady(VideoDecoder::DecoderStatus status,
+                  scoped_refptr<VideoFrame> frame);
 
   // Helper method that schedules an asynchronous read from the decoder as long
   // as there isn't a pending read and we have capacity.

media/filters/video_renderer_base_unittest.cc

@@ -131,7 +131,7 @@ class VideoRendererBaseTest : public ::testing::Test {
     VideoDecoder::ReadCB read_cb(queued_read_cb_);
     queued_read_cb_.Reset();
     base::AutoUnlock u(lock_);
-    read_cb.Run(VideoFrame::CreateEmptyFrame());
+    read_cb.Run(VideoDecoder::kOk, VideoFrame::CreateEmptyFrame());
   }
 
   void StartSeeking(int64 timestamp) {
@@ -192,27 +192,36 @@ class VideoRendererBaseTest : public ::testing::Test {
     VideoDecoder::ReadCB read_cb;
     {
       base::AutoLock l(lock_);
-      CHECK(!read_cb_.is_null()) << "Can't deliver a frame without a callback";
       std::swap(read_cb, read_cb_);
     }
 
     if (timestamp == kEndOfStream) {
-      read_cb.Run(VideoFrame::CreateEmptyFrame());
+      read_cb.Run(VideoDecoder::kOk, VideoFrame::CreateEmptyFrame());
     } else {
-      read_cb.Run(CreateFrame(timestamp, kFrameDuration));
+      read_cb.Run(VideoDecoder::kOk, CreateFrame(timestamp, kFrameDuration));
     }
   }
 
+  void DecoderError() {
+    // Lock+swap to avoid re-entrancy issues.
+    VideoDecoder::ReadCB read_cb;
+    {
+      base::AutoLock l(lock_);
+      std::swap(read_cb, read_cb_);
+    }
+
+    read_cb.Run(VideoDecoder::kDecodeError, NULL);
+  }
+
   void AbortRead() {
     // Lock+swap to avoid re-entrancy issues.
     VideoDecoder::ReadCB read_cb;
     {
       base::AutoLock l(lock_);
-      CHECK(!read_cb_.is_null()) << "Can't deliver a frame without a callback";
       std::swap(read_cb, read_cb_);
     }
 
-    read_cb.Run(NULL);
+    read_cb.Run(VideoDecoder::kOk, NULL);
   }
 
   void ExpectCurrentFrame(bool present) {
@@ -335,7 +344,7 @@ class VideoRendererBaseTest : public ::testing::Test {
 
     // Abort pending read.
     if (!read_cb.is_null())
-      read_cb.Run(NULL);
+      read_cb.Run(VideoDecoder::kOk, NULL);
 
     callback.Run();
   }
@@ -364,9 +373,10 @@ class VideoRendererBaseTest : public ::testing::Test {
         // Unlock to deliver the frame to avoid re-entrancy issues.
         base::AutoUnlock ul(lock_);
         if (timestamp == kEndOfStream) {
-          read_cb.Run(VideoFrame::CreateEmptyFrame());
+          read_cb.Run(VideoDecoder::kOk, VideoFrame::CreateEmptyFrame());
         } else {
-          read_cb.Run(CreateFrame(i * kFrameDuration, kFrameDuration));
+          read_cb.Run(VideoDecoder::kOk,
+                      CreateFrame(i * kFrameDuration, kFrameDuration));
           i++;
         }
       } else {
@@ -444,6 +454,15 @@ TEST_F(VideoRendererBaseTest, EndOfStream) {
   Shutdown();
 }
 
+TEST_F(VideoRendererBaseTest, DecoderError) {
+  Initialize();
+  Play();
+  RenderFrame(kFrameDuration);
+  EXPECT_CALL(host_, SetError(PIPELINE_ERROR_DECODE));
+  DecoderError();
+  Shutdown();
+}
+
 TEST_F(VideoRendererBaseTest, Seek_Exact) {
   Initialize();
   Pause();