Simplify VideoDecodeEngine interface by making everything synchronous.

media/filters/ffmpeg_video_decoder.cc

Index: media/filters/ffmpeg_video_decoder.cc
diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc
index deb0bf375df950b221ca36766ca7ef817fbfd4a3..a54f95177b82d3dec554d1aa1e3002d20ccb0cd5 100644
--- a/media/filters/ffmpeg_video_decoder.cc
+++ b/media/filters/ffmpeg_video_decoder.cc
@@ -5,12 +5,11 @@
 #include "media/filters/ffmpeg_video_decoder.h"
 
 #include "base/bind.h"
-#include "base/callback.h"
 #include "base/message_loop.h"
-#include "base/task.h"
 #include "media/base/demuxer_stream.h"
 #include "media/base/filter_host.h"
 #include "media/base/limits.h"
+#include "media/base/video_decoder_config.h"
 #include "media/base/video_frame.h"
 #include "media/ffmpeg/ffmpeg_common.h"
 #include "media/video/ffmpeg_video_decode_engine.h"
@@ -19,7 +18,7 @@ namespace media {
 
 FFmpegVideoDecoder::FFmpegVideoDecoder(MessageLoop* message_loop)
     : message_loop_(message_loop),
-      state_(kUnInitialized),
+      state_(kUninitialized),
       decode_engine_(new FFmpegVideoDecodeEngine()) {
 }
 
@@ -29,17 +28,14 @@ void FFmpegVideoDecoder::Initialize(DemuxerStream* demuxer_stream,
                                     const base::Closure& callback,
                                     const StatisticsCallback& stats_callback) {
   if (MessageLoop::current() != message_loop_) {
-    message_loop_->PostTask(
-        FROM_HERE,
-        base::Bind(&FFmpegVideoDecoder::Initialize, this,
-                   make_scoped_refptr(demuxer_stream),
-                   callback, stats_callback));
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &FFmpegVideoDecoder::Initialize, this,
+        make_scoped_refptr(demuxer_stream), callback, stats_callback));
     return;
   }
 
   DCHECK_EQ(MessageLoop::current(), message_loop_);
   DCHECK(!demuxer_stream_);
-  DCHECK(initialize_callback_.is_null());
 
   if (!demuxer_stream) {
     host()->SetError(PIPELINE_ERROR_DECODE);
@@ -48,7 +44,6 @@ void FFmpegVideoDecoder::Initialize(DemuxerStream* demuxer_stream,
   }
 
   demuxer_stream_ = demuxer_stream;
-  initialize_callback_ = callback;
   statistics_callback_ = stats_callback;
 
   const VideoDecoderConfig& config = demuxer_stream->video_decoder_config();
@@ -59,24 +54,19 @@ void FFmpegVideoDecoder::Initialize(DemuxerStream* demuxer_stream,
   if (natural_size_.width() > Limits::kMaxDimension ||
       natural_size_.height() > Limits::kMaxDimension ||
       natural_size_.GetArea() > Limits::kMaxCanvas) {
-    OnInitializeComplete(false);
+    host()->SetError(PIPELINE_ERROR_DECODE);
+    callback.Run();
     return;
   }
 
-  state_ = kInitializing;
-  decode_engine_->Initialize(this, config);
-}
-
-void FFmpegVideoDecoder::OnInitializeComplete(bool success) {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(!initialize_callback_.is_null());
-
-  if (success) {
-    state_ = kNormal;
-  } else {
+  if (!decode_engine_->Initialize(config)) {
     host()->SetError(PIPELINE_ERROR_DECODE);
+    callback.Run();
+    return;
   }
-  ResetAndRunCB(&initialize_callback_);
+
+  state_ = kNormal;
+  callback.Run();
 }
 
 void FFmpegVideoDecoder::Stop(const base::Closure& callback) {
@@ -87,22 +77,23 @@ void FFmpegVideoDecoder::Stop(const base::Closure& callback) {
   }
 
   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(uninitialize_callback_.is_null());
 
-  uninitialize_callback_ = callback;
-  if (state_ != kUnInitialized)
-    decode_engine_->Uninitialize();
-  else
-    OnUninitializeComplete();
+  decode_engine_->Uninitialize();
+  state_ = kUninitialized;
+  callback.Run();
 }
 
-void FFmpegVideoDecoder::OnUninitializeComplete() {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(!uninitialize_callback_.is_null());
+void FFmpegVideoDecoder::Seek(base::TimeDelta time, const FilterStatusCB& cb) {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &FFmpegVideoDecoder::Seek, this, time, cb));
+    return;
+  }
 
-  state_ = kStopped;
+  DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  ResetAndRunCB(&uninitialize_callback_);
+  pts_stream_.Seek(time);
+  cb.Run(PIPELINE_OK);
 }
 
 void FFmpegVideoDecoder::Pause(const base::Closure& callback) {
@@ -112,7 +103,6 @@ void FFmpegVideoDecoder::Pause(const base::Closure& callback) {
     return;
   }
 
-  state_ = kPausing;
   callback.Run();
 }
 
@@ -124,66 +114,18 @@ void FFmpegVideoDecoder::Flush(const base::Closure& callback) {
   }
 
   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(flush_callback_.is_null());
-
-  state_ = kFlushing;
-
-  FlushBuffers();
-
-  flush_callback_ = callback;
 
   decode_engine_->Flush();
-}
-
-void FFmpegVideoDecoder::OnFlushComplete() {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(!flush_callback_.is_null());
-
-  // Everything in the presentation time queue is invalid, clear the queue.
   pts_stream_.Flush();
-
-  // Mark flush operation had been done.
   state_ = kNormal;
-
-  ResetAndRunCB(&flush_callback_);
-}
-
-void FFmpegVideoDecoder::Seek(base::TimeDelta time, const FilterStatusCB& cb) {
-  if (MessageLoop::current() != message_loop_) {
-     message_loop_->PostTask(FROM_HERE,
-                             base::Bind(&FFmpegVideoDecoder::Seek, this,
-                                               time, cb));
-     return;
-  }
-
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(seek_cb_.is_null());
-
-  pts_stream_.Seek(time);
-  seek_cb_ = cb;
-  decode_engine_->Seek();
-}
-
-void FFmpegVideoDecoder::OnSeekComplete() {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(!seek_cb_.is_null());
-
-  ResetAndRunCB(&seek_cb_, PIPELINE_OK);
-}
-
-void FFmpegVideoDecoder::OnError() {
-  VideoFrameReady(NULL);
-}
-
-void FFmpegVideoDecoder::OnReadComplete(Buffer* buffer_in) {
-  scoped_refptr<Buffer> buffer(buffer_in);
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &FFmpegVideoDecoder::OnReadCompleteTask, this, buffer));
+  callback.Run();
 }
 
-void FFmpegVideoDecoder::OnReadCompleteTask(scoped_refptr<Buffer> buffer) {
+void FFmpegVideoDecoder::DoDecodeBuffer(const scoped_refptr<Buffer>& buffer) {

[Ami GONE FROM CHROMIUM, 2011/11/01 22:17:40]

Why have this be a separate function?  It makes DecodeBuffer() an empty shell.

(given the ugliness of having to trampoline explicitly, I prefer doing it
in-line rather than having extra methods)

[scherkus (not reviewing), 2011/11/03 04:55:59]

Done.

   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK_NE(state_, kStopped);  // because of Flush() before Stop().
+  DCHECK_NE(state_, kUninitialized);
+  DCHECK_NE(state_, kDecodeFinished);
+  DCHECK(!frame_ready_cb_.is_null());
 
   // During decode, because reads are issued asynchronously, it is possible to
   // receive multiple end of stream buffers since each read is acked. When the
@@ -208,7 +150,7 @@ void FFmpegVideoDecoder::OnReadCompleteTask(scoped_refptr<Buffer> buffer) {
   // kFlushCodec -> kDecodeFinished:
   //     When avcodec_decode_video2() returns 0 data or errors out.
   // (any state) -> kNormal:
-  //     Any time buffer->IsDiscontinuous() is true.
+  //     Any time Flush() is called.

[Ami GONE FROM CHROMIUM, 2011/11/01 22:17:40]

I'm surprised a Flush() can recover from a "catastrophic failure".  Perhaps
downgrade that phrase to "decode error"?

[scherkus (not reviewing), 2011/11/03 04:55:59]

Done.

 
   // Transition to kFlushCodec on the first end of stream buffer.
   if (state_ == kNormal && buffer->IsEndOfStream()) {
@@ -222,99 +164,95 @@ void FFmpegVideoDecoder::OnReadCompleteTask(scoped_refptr<Buffer> buffer) {
     pts_stream_.EnqueuePts(buffer.get());
   }
 
-  // Otherwise, attempt to decode a single frame.
-  decode_engine_->ConsumeVideoSample(buffer);
-}
-
-void FFmpegVideoDecoder::ProduceVideoFrame(
-    scoped_refptr<VideoFrame> video_frame) {
-  if (MessageLoop::current() != message_loop_) {
-    if (state_ != kStopped) {
-      message_loop_->PostTask(FROM_HERE, base::Bind(
-          &FFmpegVideoDecoder::ProduceVideoFrame, this, video_frame));
-    }
+  scoped_refptr<VideoFrame> video_frame;
+  if (!decode_engine_->Decode(buffer, &video_frame)) {
+    state_ = kDecodeFinished;
+    host()->SetError(PIPELINE_ERROR_DECODE);
     return;
   }
 
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-
-  // Synchronized flushing before stop should prevent this.
-  DCHECK_NE(state_, kStopped);
-
-  // If the decoding is finished, we just always return empty frames.
-  if (state_ == kDecodeFinished) {
-    // Signal VideoRenderer the end of the stream event.
-    VideoFrameReady(VideoFrame::CreateEmptyFrame());
-
-    // Fall through, because we still need to keep record of this frame.
+  // Any successful decode counts!
+  if (buffer->GetDataSize()) {
+    PipelineStatistics statistics;
+    statistics.video_bytes_decoded = buffer->GetDataSize();
+    statistics_callback_.Run(statistics);
   }
 
-  // Notify decode engine the available of new frame.
-  decode_engine_->ProduceVideoFrame(video_frame);
-}
-
-void FFmpegVideoDecoder::ConsumeVideoFrame(
-    scoped_refptr<VideoFrame> video_frame,
-    const PipelineStatistics& statistics) {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK_NE(state_, kStopped);
-
-  statistics_callback_.Run(statistics);
-
-  if (video_frame.get()) {
-    if (kPausing == state_ || kFlushing == state_) {
-      frame_queue_flushed_.push_back(video_frame);
-      if (kFlushing == state_)
-        FlushBuffers();
+  // If we didn't get a frame then we've either completely finished decoding or
+  // we need more data.
+  if (!video_frame) {
+    if (state_ == kFlushCodec) {
+      state_ = kDecodeFinished;
+      DeliverFrame(VideoFrame::CreateEmptyFrame());
       return;
     }
 
-    // If we actually got data back, enqueue a frame.
-    pts_stream_.UpdatePtsAndDuration(video_frame.get());
+    ReadFromDemuxerStream();
+    return;
+  }
 
-    video_frame->SetTimestamp(pts_stream_.current_pts());
-    video_frame->SetDuration(pts_stream_.current_duration());
+  // If we got a frame make sure its timestamp is correct before sending it off.
+  pts_stream_.UpdatePtsAndDuration(video_frame.get());
+  video_frame->SetTimestamp(pts_stream_.current_pts());
+  video_frame->SetDuration(pts_stream_.current_duration());
 
-    VideoFrameReady(video_frame);
-  } else {
-    // When in kFlushCodec, any errored decode, or a 0-lengthed frame,
-    // is taken as a signal to stop decoding.
-    if (state_ == kFlushCodec) {
-      state_ = kDecodeFinished;
+  DeliverFrame(video_frame);
+}
 
-      // Signal VideoRenderer the end of the stream event.
-      VideoFrameReady(VideoFrame::CreateEmptyFrame());
-    }
+void FFmpegVideoDecoder::Read(const FrameReadyCB& callback) {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &FFmpegVideoDecoder::Read, this, callback));
+    return;
   }
-}
 
-void FFmpegVideoDecoder::ProduceVideoSample(
-    scoped_refptr<Buffer> buffer) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK_NE(state_, kStopped);
+  CHECK(!callback.is_null());

[Ami GONE FROM CHROMIUM, 2011/11/01 22:17:40]

Here and below s/CHECK/DCHECK/?

[scherkus (not reviewing), 2011/11/03 04:55:59]

maybe... I feel like this is programmer error and that the discovery of a null
read callback will be quite far from where it originates

+  CHECK(frame_ready_cb_.is_null()) << "Overlapping decodes are not supported.";
+
+  // This can happen during shutdown after Stop() has been called.
+  if (state_ == kUninitialized) {
+    // XXXXXXX
+    // just drop the callback?  do we want to honour every callback no matter
+    // what?

[scherkus (not reviewing), 2011/11/01 04:18:26]

FYI

[acolwell GONE FROM CHROMIUM, 2011/11/01 18:31:03]

Will the caller ever block progress if it doesn't get called back here? If so
then it is probably best to just send empty frames back. 

At some point we might want to change the callback signature to include a
PipelineStatus so we could report errors for calls made in an invalid state or
at EOS.

[Ami GONE FROM CHROMIUM, 2011/11/01 22:17:40]

I say drop the callback.

[scherkus (not reviewing), 2011/11/03 04:55:59]

Done.

+    return;
+  }
+
+  // Return empty frames if decoding has finished.
+  if (state_ == kDecodeFinished) {
+    callback.Run(VideoFrame::CreateEmptyFrame());
+    return;
+  }
 
-  demuxer_stream_->Read(base::Bind(&FFmpegVideoDecoder::OnReadComplete,
-                                   this));
+  frame_ready_cb_ = callback;
+  ReadFromDemuxerStream();
 }
 
 gfx::Size FFmpegVideoDecoder::natural_size() {
   return natural_size_;
 }
 
-void FFmpegVideoDecoder::FlushBuffers() {
-  while (!frame_queue_flushed_.empty()) {
-    scoped_refptr<VideoFrame> video_frame;
-    video_frame = frame_queue_flushed_.front();
-    frame_queue_flushed_.pop_front();
+void FFmpegVideoDecoder::ReadFromDemuxerStream() {
+  DCHECK_NE(state_, kUninitialized);
+  DCHECK_NE(state_, kDecodeFinished);
+  DCHECK(!frame_ready_cb_.is_null());
 
-    // Return frames back to the decode engine.
-    decode_engine_->ProduceVideoFrame(video_frame);
-  }
+  demuxer_stream_->Read(base::Bind(&FFmpegVideoDecoder::DecodeBuffer, this));
+}
+
+void FFmpegVideoDecoder::DecodeBuffer(Buffer* buffer) {
+  // TODO(scherkus): change DemuxerStream::Read() to use scoped_refptr<> for

[acolwell GONE FROM CHROMIUM, 2011/11/01 18:31:03]

This TODO should be in demuxer_stream.h

[scherkus (not reviewing), 2011/11/03 04:55:59]

TODO was fixed in r108178

+  // callback.
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &FFmpegVideoDecoder::DoDecodeBuffer, this, make_scoped_refptr(buffer)));
 }
 
-void FFmpegVideoDecoder::SetVideoDecodeEngineForTest(
-    VideoDecodeEngine* engine) {
-  decode_engine_.reset(engine);
+void FFmpegVideoDecoder::DeliverFrame(
+    const scoped_refptr<VideoFrame>& video_frame) {
+  // Reset the callback before running to protect against reentrancy.
+  FrameReadyCB frame_ready_cb = frame_ready_cb_;
+  frame_ready_cb_.Reset();
+  frame_ready_cb.Run(video_frame);
 }
 
 }  // namespace media