Simplify VideoDecodeEngine interface by making everything synchronous.

media/filters/ffmpeg_video_decoder.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.
 
 #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"
 
 namespace media {
 
 FFmpegVideoDecoder::FFmpegVideoDecoder(MessageLoop* message_loop)
     : message_loop_(message_loop),
-      state_(kUnInitialized),
+      state_(kUninitialized),
       decode_engine_(new FFmpegVideoDecodeEngine()) {
 }
 
 FFmpegVideoDecoder::~FFmpegVideoDecoder() {}
 
 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);
     callback.Run();
     return;
   }
 
   demuxer_stream_ = demuxer_stream;
-  initialize_callback_ = callback;
   statistics_callback_ = stats_callback;
 
   const VideoDecoderConfig& config = demuxer_stream->video_decoder_config();
 
   pts_stream_.Initialize(GetFrameDuration(config));
 
   natural_size_ = config.natural_size();
   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);
-}
+  if (!decode_engine_->Initialize(config)) {
+    host()->SetError(PIPELINE_ERROR_DECODE);
+    callback.Run();
+    return;
+  }
 
-void FFmpegVideoDecoder::OnInitializeComplete(bool success) {
-  DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(!initialize_callback_.is_null());
-
-  if (success) {
-    state_ = kNormal;
-  } else {
-    host()->SetError(PIPELINE_ERROR_DECODE);
-  }
-  ResetAndRunCB(&initialize_callback_);
+  state_ = kNormal;
+  callback.Run();
 }
 
 void FFmpegVideoDecoder::Stop(const base::Closure& callback) {
   if (MessageLoop::current() != message_loop_) {
     message_loop_->PostTask(FROM_HERE, base::Bind(
         &FFmpegVideoDecoder::Stop, this, callback));
     return;
   }
 
   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() {
+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(!uninitialize_callback_.is_null());
 
-  state_ = kStopped;
-
-  ResetAndRunCB(&uninitialize_callback_);
+  pts_stream_.Seek(time);
+  cb.Run(PIPELINE_OK);
 }
 
 void FFmpegVideoDecoder::Pause(const base::Closure& callback) {
   if (MessageLoop::current() != message_loop_) {
     message_loop_->PostTask(FROM_HERE, base::Bind(
         &FFmpegVideoDecoder::Pause, this, callback));
     return;
   }
 
-  state_ = kPausing;
   callback.Run();
 }
 
 void FFmpegVideoDecoder::Flush(const base::Closure& callback) {
   if (MessageLoop::current() != message_loop_) {
     message_loop_->PostTask(FROM_HERE, base::Bind(
         &FFmpegVideoDecoder::Flush, this, callback));
     return;
   }
 
   DCHECK_EQ(MessageLoop::current(), message_loop_);
-  DCHECK(flush_callback_.is_null());
-
-  state_ = kFlushing;
-
-  FlushBuffers();
-
-  flush_callback_ = callback;
 
   decode_engine_->Flush();
+  pts_stream_.Flush();
+  state_ = kNormal;
+  callback.Run();
 }
 
-void FFmpegVideoDecoder::OnFlushComplete() {
+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(!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));
-}
-
-void FFmpegVideoDecoder::OnReadCompleteTask(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);
+  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
   // first end of stream buffer is read, FFmpeg may still have frames queued
   // up in the decoder so we need to go through the decode loop until it stops
   // giving sensible data.  After that, the decoder should output empty
   // frames.  There are three states the decoder can be in:
   //
   //   kNormal: This is the starting state. Buffers are decoded. Decode errors
   //            are discarded.
   //   kFlushCodec: There isn't any more input data. Call avcodec_decode_video2
   //                until no more data is returned to flush out remaining
   //                frames. The input buffer is ignored at this point.
   //   kDecodeFinished: All calls return empty frames.
   //
   // These are the possible state transitions.
   //
   // kNormal -> kFlushCodec:
   //     When buffer->IsEndOfStream() is first true.
   // kNormal -> kDecodeFinished:
   //     A catastrophic failure occurs, and decoding needs to stop.
   // 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()) {
     state_ = kFlushCodec;
   }
 
   // Push all incoming timestamps into the priority queue as long as we have
   // not yet received an end of stream buffer.  It is important that this line
   // stay below the state transition into kFlushCodec done above.
   if (state_ == kNormal) {
     pts_stream_.EnqueuePts(buffer.get());
   }
 
-  // Otherwise, attempt to decode a single frame.
-  decode_engine_->ConsumeVideoSample(buffer);
+  scoped_refptr<VideoFrame> video_frame;
+  if (!decode_engine_->Decode(buffer, &video_frame)) {
+    state_ = kDecodeFinished;
+    host()->SetError(PIPELINE_ERROR_DECODE);
+    return;
+  }
+
+  // Any successful decode counts!
+  if (buffer->GetDataSize()) {
+    PipelineStatistics statistics;
+    statistics.video_bytes_decoded = buffer->GetDataSize();
+    statistics_callback_.Run(statistics);
+  }
+
+  // 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;
+    }
+
+    ReadFromDemuxerStream();
+    return;
+  }
+
+  // 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());
+
+  DeliverFrame(video_frame);
 }
 
-void FFmpegVideoDecoder::ProduceVideoFrame(
-    scoped_refptr<VideoFrame> video_frame) {
+void FFmpegVideoDecoder::Read(const FrameReadyCB& callback) {
   if (MessageLoop::current() != message_loop_) {
-    if (state_ != kStopped) {
-      message_loop_->PostTask(FROM_HERE, base::Bind(
-          &FFmpegVideoDecoder::ProduceVideoFrame, this, video_frame));
-    }
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &FFmpegVideoDecoder::Read, this, callback));
     return;
   }
 
   DCHECK_EQ(MessageLoop::current(), message_loop_);
+  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.";
 
-  // 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.
+  // 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;
   }
 
-  // Notify decode engine the available of new frame.
-  decode_engine_->ProduceVideoFrame(video_frame);
-}
+  // Return empty frames if decoding has finished.
+  if (state_ == kDecodeFinished) {
+    callback.Run(VideoFrame::CreateEmptyFrame());
+    return;
+  }
 
-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();
-      return;
-    }
-
-    // If we actually got data back, enqueue a frame.
-    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;
-
-      // Signal VideoRenderer the end of the stream event.
-      VideoFrameReady(VideoFrame::CreateEmptyFrame());
-    }
-  }
-}
-
-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));
+  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::SetVideoDecodeEngineForTest(
-    VideoDecodeEngine* engine) {
-  decode_engine_.reset(engine);
+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::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