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..a8efcec0e4ddfe4e7e8ced541c0347721d450cb7 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();

[acolwell GONE FROM CHROMIUM, 2011/10/28 17:38:47]

I'd like to see callback converted to a PipelineStatusCB. Do it now or add a
TODO so this can happen in another CL.

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

TODO added in filters.h

there are a few related filters.h TODOs I want to do (scoped_refptr<> for
buffers) so I'll tackle them in a future CL

     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,17 @@ 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) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK_NE(state_, kStopped);  // because of Flush() before Stop().
+  DCHECK_NE(state_, kUninitialized);
+  DCHECK_NE(state_, kDecodeFinished);
 
   // 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 +149,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.
 
   // Transition to kFlushCodec on the first end of stream buffer.
   if (state_ == kNormal && buffer->IsEndOfStream()) {
@@ -222,99 +163,88 @@ void FFmpegVideoDecoder::OnReadCompleteTask(scoped_refptr<Buffer> buffer) {
     pts_stream_.EnqueuePts(buffer.get());
   }
 
-  // Otherwise, attempt to decode a single frame.
-  decode_engine_->ConsumeVideoSample(buffer);
-}
+  // XXXXXXXXXXXXXXX
+  // This will cause every frame to get allocated.
+  scoped_refptr<VideoFrame> video_frame;

[scherkus (not reviewing), 2011/10/28 16:54:04]

I'm not entirely sure if I should attempt to keep frames from being re-allocated

It would mean continuing to do a little bit of buffer recycling inside of
VideoRendererBase so the support is there today I just need to hold onto
VideoFrame pointers inside FFmpegVideoDecoder

[acolwell GONE FROM CHROMIUM, 2011/10/28 17:38:47]

I think we should remove the buffer recycling for now and add a new
implementation back in if we need to.

On 2011/10/28 16:54:04, scherkus wrote:

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

as discussed offline it looks like there was no way out of it!

I changed it to one-read-request-at-a-time -- let me know if you have thoughts
otherwise

 
-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));
-    }
+  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;
+      VideoFrameReady(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;
+  VideoFrameReady(video_frame);
+}
 
-      // Signal VideoRenderer the end of the stream event.
-      VideoFrameReady(VideoFrame::CreateEmptyFrame());
-    }
+void FFmpegVideoDecoder::ProduceVideoFrame(
+    scoped_refptr<VideoFrame> video_frame) {
+  if (MessageLoop::current() != message_loop_) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &FFmpegVideoDecoder::ProduceVideoFrame, this, video_frame));
+    return;
   }
-}
 
-void FFmpegVideoDecoder::ProduceVideoSample(
-    scoped_refptr<Buffer> buffer) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK_NE(state_, kStopped);
 
-  demuxer_stream_->Read(base::Bind(&FFmpegVideoDecoder::OnReadComplete,
-                                   this));
+  // This can happen during shutdown after Stop() has been called.
+  if (state_ == kUninitialized) {
+    return;
+  }
+
+  // Return empty frames if decoding has finished.
+  if (state_ == kDecodeFinished) {
+    VideoFrameReady(VideoFrame::CreateEmptyFrame());
+    return;
+  }
+
+  // XXX: Queue frame?
+  // XXXXXXXXXXXXX right now we drop video_frame on the ground, causing all
+  // frame to be re-allocated on the fly.

[scherkus (not reviewing), 2011/10/28 16:54:04]

this is where we could either:
1) cache the video frames locally or if we wanted to get fancy
2) use currying to carry the video_frame object via a Read() to DemuxerStream

I'm hesitant of (2) since I'm not 100% sure how object lifetimes would work out

[acolwell GONE FROM CHROMIUM, 2011/10/28 17:38:47]

Remove the video_frame pointer for now. I want to remove all of the frame
recycling code. Like I said before I think this can be best handled with a
factory and this code would never need to know that buffer recycling is going
on.

On 2011/10/28 16:54:04, scherkus wrote:

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

Done.

+
+  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);
 
-    // Return frames back to the decode engine.
-    decode_engine_->ProduceVideoFrame(video_frame);
-  }
+  demuxer_stream_->Read(base::Bind(&FFmpegVideoDecoder::DecodeBuffer, this));
 }
 
-void FFmpegVideoDecoder::SetVideoDecodeEngineForTest(
-    VideoDecodeEngine* engine) {
-  decode_engine_.reset(engine);
+void FFmpegVideoDecoder::DecodeBuffer(Buffer* buffer) {
+  // TODO(scherkus): change DemuxerStream::Read() to use scoped_refptr<> for
+  // callback.
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &FFmpegVideoDecoder::DoDecodeBuffer, this, make_scoped_refptr(buffer)));
 }
 
 }  // namespace media