Simplify VideoDecodeEngine interface by making everything synchronous.

media/filters/video_renderer_base.cc

Index: media/filters/video_renderer_base.cc
diff --git a/media/filters/video_renderer_base.cc b/media/filters/video_renderer_base.cc
index fca51ca80f41cdd234075373512b8549d613c038..3752e080a850007e4323df1e859d276a04de67ac 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -30,10 +30,12 @@ VideoRendererBase::VideoRendererBase()
     : frame_available_(&lock_),
       state_(kUninitialized),
       thread_(base::kNullThreadHandle),
-      pending_reads_(0),
+      pending_read_(false),
       pending_paint_(false),
       pending_paint_with_last_available_(false),
-      playback_rate_(0) {
+      playback_rate_(0),
+      frame_ready_cb_(base::Bind(&VideoRendererBase::FrameReady,
+                                 base::Unretained(this))) {
 }
 
 VideoRendererBase::~VideoRendererBase() {
@@ -61,8 +63,7 @@ void VideoRendererBase::Flush(const base::Closure& callback) {
   flush_callback_ = callback;
   state_ = kFlushing;
 
-  if (!pending_paint_)
-    FlushBuffers_Locked();
+  AttemptFlush_Locked();
 }
 
 void VideoRendererBase::Stop(const base::Closure& callback) {
@@ -72,7 +73,7 @@ void VideoRendererBase::Stop(const base::Closure& callback) {
     state_ = kStopped;
 
     if (!pending_paint_ && !pending_paint_with_last_available_)
-      DoStopOrErrorFlush_Locked();
+      DoStopOrError_Locked();
 
     // Clean up our thread if present.
     if (thread_) {

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

Given the code below shouldn't this be (thread_ != base::kNullThreadHandle) ?

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

Done.

@@ -120,7 +121,7 @@ void VideoRendererBase::Seek(base::TimeDelta time, const FilterStatusCB& cb) {
     }
 
     seek_timestamp_ = time;
-    ScheduleRead_Locked();
+    AttemptRead_Locked();
   }
 
   if (run_callback)
@@ -139,10 +140,6 @@ void VideoRendererBase::Initialize(VideoDecoder* decoder,
 
   statistics_callback_ = stats_callback;
 
-  decoder_->set_consume_video_frame_callback(
-      base::Bind(&VideoRendererBase::ConsumeVideoFrame,
-                 base::Unretained(this)));
-
   // Notify the pipeline of the video dimensions.
   host()->SetNaturalVideoSize(decoder_->natural_size());
 
@@ -269,7 +266,7 @@ void VideoRendererBase::ThreadMain() {
 
     // If we got here then:
     // 1. next frame's timestamp is already current; or
-    // 2. we do not have any current frame yet anyway; or
+    // 2. we do not have a current frame yet; or
     // 3. a special case when the stream is badly formatted and
     //    we got a frame with timestamp greater than overall duration.
     //    In this case we should proceed anyway and try to obtain the
@@ -290,11 +287,10 @@ void VideoRendererBase::ThreadMain() {
         if (remaining_time.InMicroseconds() > 0)
           break;
 
-        // Frame dropped: transfer ready frame into done queue and read again.
-        frames_queue_done_.push_back(frames_queue_ready_.front());
-        frames_queue_ready_.pop_front();
-        ScheduleRead_Locked();
+        // Frame dropped: read again.
         ++frames_dropped;
+        frames_queue_ready_.pop_front();
+        AttemptRead_Locked();
       }
 
       // Continue waiting for the current paint to finish.
@@ -305,10 +301,9 @@ void VideoRendererBase::ThreadMain() {
     //
     // We can now safely update the current frame, request another frame, and
     // signal to the client that a new frame is available.
-    frames_queue_done_.push_back(current_frame_);
     current_frame_ = frames_queue_ready_.front();
     frames_queue_ready_.pop_front();
-    ScheduleRead_Locked();
+    AttemptRead_Locked();
 
     base::AutoUnlock auto_unlock(lock_);
     OnFrameAvailable();
@@ -320,8 +315,7 @@ void VideoRendererBase::GetCurrentFrame(scoped_refptr<VideoFrame>* frame_out) {
   DCHECK(!pending_paint_ && !pending_paint_with_last_available_);
 
   if ((!current_frame_ || current_frame_->IsEndOfStream()) &&
-      (!last_available_frame_ ||
-       last_available_frame_->IsEndOfStream())) {
+      (!last_available_frame_ || last_available_frame_->IsEndOfStream())) {
     *frame_out = NULL;
     return;
   }
@@ -364,161 +358,120 @@ void VideoRendererBase::PutCurrentFrame(scoped_refptr<VideoFrame> frame) {
   // frame when this is true.
   frame_available_.Signal();
   if (state_ == kFlushing) {
-    FlushBuffers_Locked();
+    AttemptFlush_Locked();
   } else if (state_ == kError || state_ == kStopped) {
-    DoStopOrErrorFlush_Locked();
+    DoStopOrError_Locked();
   }
 }
 
-void VideoRendererBase::ConsumeVideoFrame(scoped_refptr<VideoFrame> frame) {
-  if (frame) {
-    PipelineStatistics statistics;
-    statistics.video_frames_decoded = 1;
-    statistics_callback_.Run(statistics);
-  }
+void VideoRendererBase::FrameReady(scoped_refptr<VideoFrame> frame) {
+  DCHECK(frame);
 
   base::AutoLock auto_lock(lock_);
-
-  if (!frame) {
-    EnterErrorState_Locked(PIPELINE_ERROR_DECODE);

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

Why not keep this?

-    return;
-  }
-
-  // Decoder could reach seek state before our Seek() get called.
-  // We will enter kSeeking
-  if (state_ == kFlushed)
-    state_ = kSeeking;
-
-  // Synchronous flush between filters should prevent this from happening.
-  DCHECK_NE(state_, kStopped);
-  if (frame && !frame->IsEndOfStream())
-    --pending_reads_;
-
   DCHECK_NE(state_, kUninitialized);
   DCHECK_NE(state_, kStopped);
   DCHECK_NE(state_, kError);
+  CHECK(pending_read_);

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

DCHECK?

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

Again this one of those things where a callback was executed at a time when it
really shouldn't have -- I'm torn on whether shipping this with the CHECK() in

 
-  if (state_ == kPaused || state_ == kFlushing) {
-    // Decoder are flushing rubbish video frame, we will not display them.
-    if (frame && !frame->IsEndOfStream())
-      frames_queue_done_.push_back(frame);
-    DCHECK_LE(frames_queue_done_.size(),
-              static_cast<size_t>(Limits::kMaxVideoFrames));
+  pending_read_ = false;
 
-    // Excluding kPause here, because in pause state, we will never
-    // transfer out-bounding buffer. We do not flush buffer when Compositor
-    // hold reference to our video frame either.
-    if (state_ == kFlushing && pending_paint_ == false)
-      FlushBuffers_Locked();
+  // Decoder could reach seek state before our Seek() get called.

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

s/get/got/

+  // We will enter kSeeking
+  if (state_ == kFlushed) {
+    CHECK(false) << "I don't think this is possible";

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

CHECK(false) is always better spelled LOG(FATAL)
But if you really don't think so why not replace this clause & comment with
DCHECK_NE(state_, kFlushed);
?

Ditto below.

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

Left these CHECK()s in for local testing -- they were never hit so will change
to DCHECK

+    //state_ = kSeeking;
+    return;
+  }
 
+  if (state_ == kPaused) {

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

Can't this happen based on the "Prerolling while paused" comment below? This
looks like it will regress that feature.

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

I believe it can:
T1: VRB holds lock inside FrameReady()
T2: Pipeline calls Pause() but gets blocked on lock
T1: VRB requests another read and releases lock
T2: kState becomes pause
T1: VRB receives frame

I'll remove the check as it doesn't seem to cause much harm leaving it in

+    CHECK(false) << "I don't think this is possible";
+    //frame_queue_ready_.push_back(video_frame);
+    return;
+  }
+
+  if (state_ == kFlushing) {
+    AttemptFlush_Locked();
     return;
   }
 
   // Discard frames until we reach our desired seek timestamp.
   if (state_ == kSeeking && !frame->IsEndOfStream() &&
       (frame->GetTimestamp() + frame->GetDuration()) <= seek_timestamp_) {
-    frames_queue_done_.push_back(frame);
-    ScheduleRead_Locked();
-  } else {
-    frames_queue_ready_.push_back(frame);
-    DCHECK_LE(frames_queue_ready_.size(),
-              static_cast<size_t>(Limits::kMaxVideoFrames));
-    frame_available_.Signal();
+    AttemptRead_Locked();
+    return;
   }
 
-  // Check for our preroll complete condition.
-  bool new_frame_available = false;
+  // This one's a keeper! Place it in the ready queue.
+  frames_queue_ready_.push_back(frame);
+  DCHECK_LE(frames_queue_ready_.size(),
+            static_cast<size_t>(Limits::kMaxVideoFrames));
+  frame_available_.Signal();
+
+  PipelineStatistics statistics;
+  statistics.video_frames_decoded = 1;
+  statistics_callback_.Run(statistics);
+
+  // Always request more decoded video if we have capacity. This serves two
+  // purposes:
+  //   1) Prerolling while paused
+  //   2) Keeps decoding going if video rendering thread starts falling behind
+  if (frames_queue_ready_.size() < Limits::kMaxVideoFrames &&
+      !frame->IsEndOfStream()) {
+    AttemptRead_Locked();
+    return;
+  }
+
+  // If we're at capacity or end of stream while seeking we need to transition
+  // to prerolled.
   if (state_ == kSeeking) {
-    if (frames_queue_ready_.size() == Limits::kMaxVideoFrames ||
-        frame->IsEndOfStream()) {
-      // We're paused, so make sure we update |current_frame_| to represent
-      // our new location.
-      state_ = kPrerolled;
-
-      // Because we might remain paused (i.e., we were not playing before we
-      // received a seek), we can't rely on ThreadMain() to notify the subclass
-      // the frame has been updated.
-      scoped_refptr<VideoFrame> first_frame;
-      first_frame = frames_queue_ready_.front();
-      if (!first_frame->IsEndOfStream()) {
-        frames_queue_ready_.pop_front();
-        current_frame_ = first_frame;
-      }
-      new_frame_available = true;
+    state_ = kPrerolled;
+
+    // Because we might remain in the prerolled state for an undetermined amount
+    // of time (i.e., we were not playing before we received a seek), we'll
+    // manually update the current frame and notify the subclass below.
+    if (!frames_queue_ready_.front()->IsEndOfStream()) {
+      current_frame_ = frames_queue_ready_.front();
+      frames_queue_ready_.pop_front();
+    }
 
-      // If we reach prerolled state before Seek() is called by pipeline,
-      // |seek_callback_| is not set, we will return immediately during
-      // when Seek() is eventually called.
-      if (!seek_cb_.is_null()) {
-        ResetAndRunCB(&seek_cb_, PIPELINE_OK);
-      }
+    // If we reach prerolled state before Seek() is called by pipeline,
+    // |seek_callback_| is not set, we will return immediately during
+    // when Seek() is eventually called.

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

English.

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

this and the comments up in Seek() are crazy bizarro

I don't believe they apply anymore as buffer recyling is gone

+    if (!seek_cb_.is_null()) {
+      ResetAndRunCB(&seek_cb_, PIPELINE_OK);
     }
-  } else if (state_ == kFlushing && pending_reads_ == 0 && !pending_paint_) {
-    OnFlushDone_Locked();
-  }
 
-  if (new_frame_available) {
-    base::AutoUnlock auto_unlock(lock_);
+    base::AutoUnlock ul(lock_);
     OnFrameAvailable();
   }
 }
 
-void VideoRendererBase::ReadInput(scoped_refptr<VideoFrame> frame) {
-  // We should never return empty frames or EOS frame.
-  DCHECK(frame && !frame->IsEndOfStream());
-
-  decoder_->ProduceVideoFrame(frame);
-  ++pending_reads_;
-}
-
-void VideoRendererBase::ScheduleRead_Locked() {
+void VideoRendererBase::AttemptRead_Locked() {
   lock_.AssertAcquired();
   DCHECK_NE(kEnded, state_);
-  // TODO(jiesun): We use dummy buffer to feed decoder to let decoder to
-  // provide buffer pools. In the future, we may want to implement real
-  // buffer pool to recycle buffers.
-  while (!frames_queue_done_.empty()) {
-    scoped_refptr<VideoFrame> video_frame = frames_queue_done_.front();
-    frames_queue_done_.pop_front();
-    ReadInput(video_frame);
+
+  if (pending_read_ || frames_queue_ready_.size() == Limits::kMaxVideoFrames) {
+    return;
   }
+
+  pending_read_ = true;
+  decoder_->Read(frame_ready_cb_);
 }
 
-void VideoRendererBase::FlushBuffers_Locked() {
+void VideoRendererBase::AttemptFlush_Locked() {
   lock_.AssertAcquired();
-  DCHECK(!pending_paint_);
+  DCHECK_EQ(kFlushing, state_);
 
-  // We should never put EOF frame into "done queue".
+  // Get rid of any ready frames.
   while (!frames_queue_ready_.empty()) {
-    scoped_refptr<VideoFrame> video_frame = frames_queue_ready_.front();
-    if (!video_frame->IsEndOfStream()) {
-      frames_queue_done_.push_back(video_frame);
-    }
     frames_queue_ready_.pop_front();
   }
-  if (current_frame_ && !current_frame_->IsEndOfStream()) {
-    frames_queue_done_.push_back(current_frame_);
-  }
-  current_frame_ = NULL;
-
-  // Flush all buffers out to decoder.
-  ScheduleRead_Locked();
-
-  if (pending_reads_ == 0 && state_ == kFlushing)
-    OnFlushDone_Locked();
-}
-
-void VideoRendererBase::OnFlushDone_Locked() {
-  lock_.AssertAcquired();
-  // Check all buffers are returned to owners.
-  DCHECK_EQ(frames_queue_done_.size(), 0u);
-  DCHECK(!current_frame_);
-  DCHECK(frames_queue_ready_.empty());
 
-  if (!flush_callback_.is_null())  // This ensures callback is invoked once.
+  if (!pending_paint_ && !pending_read_) {

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

I think this may be where you can get into trouble if the decoder drops
callbacks after a decode error. You'll never exit kFlushing if that happens.

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

Correct -- fixed + updated unit tests

+    state_ = kFlushed;
+    current_frame_ = NULL;
     ResetAndRunCB(&flush_callback_);
-
-  state_ = kFlushed;
+  }
 }
 
 base::TimeDelta VideoRendererBase::CalculateSleepDuration(
@@ -560,7 +513,7 @@ void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
   // Flush frames if we aren't in the middle of a paint. If we
   // are painting then flushing will happen when the paint completes.
   if (!pending_paint_ && !pending_paint_with_last_available_)
-    DoStopOrErrorFlush_Locked();
+    DoStopOrError_Locked();
 
   switch (old_state) {
     case kUninitialized:
@@ -596,13 +549,12 @@ void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {
     callback.Run();
 }
 
-void VideoRendererBase::DoStopOrErrorFlush_Locked() {
+void VideoRendererBase::DoStopOrError_Locked() {
   DCHECK(!pending_paint_);
   DCHECK(!pending_paint_with_last_available_);
   lock_.AssertAcquired();
-  FlushBuffers_Locked();
   last_available_frame_ = NULL;
-  DCHECK_EQ(pending_reads_, 0);
+  DCHECK(!pending_read_);

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

Looking at the code this doesn't seem like a safe assumption especially if a
decoder error occurs.

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

I really don't know what this method does but the two callsites either set
pending_read_ to false or guard against it being true before calling this.

Not sure what to do here -- perhaps inline the last_available_frame_ = NULL?

 }
 
 }  // namespace media