Simplify VideoDecodeEngine interface by making everything synchronous.

media/filters/video_renderer_base.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 "base/bind.h"
 #include "base/callback.h"
 #include "base/threading/platform_thread.h"
 #include "media/base/buffers.h"
 #include "media/base/filter_host.h"
 #include "media/base/limits.h"
@@ skipping 12 matching lines @@
 
 // The number of milliseconds to idle when we do not have anything to do.
 // Nothing special about the value, other than we're being more OS-friendly
 // than sleeping for 1 millisecond.
 static const int kIdleMilliseconds = 10;
 
 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() {
   base::AutoLock auto_lock(lock_);
   DCHECK(state_ == kUninitialized || state_ == kStopped) << state_;
 }
 
 void VideoRendererBase::Play(const base::Closure& callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(kPrerolled, state_);
   state_ = kPlaying;
   callback.Run();
 }
 
 void VideoRendererBase::Pause(const base::Closure& callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK(state_ != kUninitialized || state_ == kError);
   state_ = kPaused;
   callback.Run();
 }
 
 void VideoRendererBase::Flush(const base::Closure& callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(state_, kPaused);
   flush_callback_ = callback;
   state_ = kFlushing;
 
-  if (!pending_paint_)
-    FlushBuffers_Locked();
+  AttemptFlush_Locked();
 }
 
 void VideoRendererBase::Stop(const base::Closure& callback) {
   base::PlatformThreadHandle old_thread_handle = base::kNullThreadHandle;
   {
     base::AutoLock auto_lock(lock_);
     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.

       // Signal the thread since it's possible to get stopped with the video
       // thread waiting for a read to complete.
       frame_available_.Signal();
       old_thread_handle = thread_;
       thread_ = base::kNullThreadHandle;
     }
   }
   if (old_thread_handle)

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

ditto

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

Done.

     base::PlatformThread::Join(old_thread_handle);
 
   // Signal the subclass we're stopping.
   OnStop(callback);
 }
 
 void VideoRendererBase::SetPlaybackRate(float playback_rate) {
   base::AutoLock auto_lock(lock_);
   playback_rate_ = playback_rate;
 }
@@ skipping 16 matching lines @@
       // Already get enough buffers from decoder.
       run_callback = true;
     } else {
       // Otherwise we are either kFlushed or kSeeking, but without enough
       // buffers we should save the callback function and call it later.
       state_ = kSeeking;
       seek_cb_ = cb;
     }
 
     seek_timestamp_ = time;
-    ScheduleRead_Locked();
+    AttemptRead_Locked();
   }
 
   if (run_callback)
     cb.Run(PIPELINE_OK);
 }
 
 void VideoRendererBase::Initialize(VideoDecoder* decoder,
                                    const base::Closure& callback,
                                    const StatisticsCallback& stats_callback) {
   base::AutoLock auto_lock(lock_);
   DCHECK(decoder);
   DCHECK(!callback.is_null());
   DCHECK(!stats_callback.is_null());
   DCHECK_EQ(kUninitialized, state_);
   decoder_ = 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());
 
   // Initialize the subclass.
   // TODO(scherkus): do we trust subclasses not to do something silly while
   // we're holding the lock?
   if (!OnInitialize(decoder)) {
     EnterErrorState_Locked(PIPELINE_ERROR_INITIALIZATION_FAILED);
     callback.Run();
     return;
@@ skipping 106 matching lines @@
     // exceptions that cause us to drop a frame and/or consider painting a
     // "next" frame.
     if (next_frame->GetTimestamp() > host()->GetTime() + kIdleTimeDelta &&
         current_frame_ &&
         current_frame_->GetTimestamp() <= host()->GetDuration()) {
       continue;
     }
 
     // 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
     //    end-of-stream packet.
 
     if (pending_paint_) {
       // The pending paint might be really slow. Check if we have any frames
       // available that we can drop if they've already expired.
       while (!frames_queue_ready_.empty()) {
         // Can't drop anything if we're at the end.
         if (frames_queue_ready_.front()->IsEndOfStream())
           break;
 
         base::TimeDelta remaining_time =
             frames_queue_ready_.front()->GetTimestamp() - host()->GetTime();
 
         // Still a chance we can render the frame!
         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());
+        // Frame dropped: read again.
+        ++frames_dropped;
         frames_queue_ready_.pop_front();
-        ScheduleRead_Locked();
-        ++frames_dropped;
+        AttemptRead_Locked();
       }
 
       // Continue waiting for the current paint to finish.
       continue;
     }
 
     // Congratulations! You've made it past the video frame timing gauntlet.
     //
     // 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();
   }
 }
 
 void VideoRendererBase::GetCurrentFrame(scoped_refptr<VideoFrame>* frame_out) {
   base::AutoLock auto_lock(lock_);
   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;
   }
 
   // We should have initialized and have the current frame.
   DCHECK_NE(state_, kUninitialized);
   DCHECK_NE(state_, kStopped);
   DCHECK_NE(state_, kError);
 
   if (current_frame_) {
@@ skipping 22 matching lines @@
     pending_paint_with_last_available_ = false;
   } else {
     DCHECK(!frame);
   }
 
   // We had cleared the |pending_paint_| flag, there are chances that current
   // frame is timed-out. We will wake up our main thread to advance the current
   // 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_);
+  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 (!frame) {
-    EnterErrorState_Locked(PIPELINE_ERROR_DECODE);

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

Why not keep this?

+  pending_read_ = false;
+
+  // 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;
   }
 
-  // Decoder could reach seek state before our Seek() get called.
-  // We will enter kSeeking
-  if (state_ == kFlushed)
-    state_ = kSeeking;
+  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;
+  }
 
-  // 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);
-
-  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));
-
-    // 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();
-
+  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;
-  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;
+  // 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();
 
-      // 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;
+  PipelineStatistics statistics;
+  statistics.video_frames_decoded = 1;
+  statistics_callback_.Run(statistics);
 
-      // 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);
-      }
-    }
-  } else if (state_ == kFlushing && pending_reads_ == 0 && !pending_paint_) {
-    OnFlushDone_Locked();
+  // 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 (new_frame_available) {
-    base::AutoUnlock auto_unlock(lock_);
+  // If we're at capacity or end of stream while seeking we need to transition
+  // to prerolled.
+  if (state_ == kSeeking) {
+    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.

[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);
+    }
+
+    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());
+void VideoRendererBase::AttemptRead_Locked() {
+  lock_.AssertAcquired();
+  DCHECK_NE(kEnded, state_);
 
-  decoder_->ProduceVideoFrame(frame);
-  ++pending_reads_;
+  if (pending_read_ || frames_queue_ready_.size() == Limits::kMaxVideoFrames) {
+    return;
+  }
+
+  pending_read_ = true;
+  decoder_->Read(frame_ready_cb_);
 }
 
-void VideoRendererBase::ScheduleRead_Locked() {
+void VideoRendererBase::AttemptFlush_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);
+  DCHECK_EQ(kFlushing, state_);
+
+  // Get rid of any ready frames.
+  while (!frames_queue_ready_.empty()) {
+    frames_queue_ready_.pop_front();
+  }
+
+  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_);
   }
 }
 
-void VideoRendererBase::FlushBuffers_Locked() {
-  lock_.AssertAcquired();
-  DCHECK(!pending_paint_);
-
-  // We should never put EOF frame into "done queue".
-  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.
-    ResetAndRunCB(&flush_callback_);
-
-  state_ = kFlushed;
-}
-
 base::TimeDelta VideoRendererBase::CalculateSleepDuration(
     VideoFrame* next_frame, float playback_rate) {
   // Determine the current and next presentation timestamps.
   base::TimeDelta now = host()->GetTime();
   base::TimeDelta this_pts = current_frame_->GetTimestamp();
   base::TimeDelta next_pts;
   if (next_frame) {
     next_pts = next_frame->GetTimestamp();
   } else {
     next_pts = this_pts + current_frame_->GetDuration();
   }
 
   // Determine our sleep duration based on whether time advanced.
   base::TimeDelta sleep;
   if (now == previous_time_) {
     // Time has not changed, assume we sleep for the frame's duration.
     sleep = next_pts - this_pts;
   } else {
     // Time has changed, figure out real sleep duration.
     sleep = next_pts - now;
     previous_time_ = now;
   }
 
   // Scale our sleep based on the playback rate.
   // TODO(scherkus): floating point badness and degrade gracefully.
   return base::TimeDelta::FromMicroseconds(
       static_cast<int64>(sleep.InMicroseconds() / playback_rate));
 }
 
 void VideoRendererBase::EnterErrorState_Locked(PipelineStatus status) {

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

assuming you don't reinstate the call in FrameReady(), I think you can remove
this method since the only other call sites are in Initialize(). 

There is enough code here that makes me think the FrameReady() call is pretty
important.

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

Done.

   lock_.AssertAcquired();
 
   base::Closure callback;
   State old_state = state_;
   state_ = kError;
 
   // 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:
     case kPrerolled:
     case kPaused:
     case kFlushed:
     case kEnded:
     case kPlaying:
       break;
 
@@ skipping 15 matching lines @@
     case kError:
       return;
   }
 
   host()->SetError(status);
 
   if (!callback.is_null())
     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