Replace VideoRendererBase Get/PutCurrentFrame() with a VideoFrame-containing callback.

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 5a17940771120c543902274a13cf15f3011e43c5..3964f1a3dda11af716d8efea3700de2353ea3d7e 100644
--- a/media/filters/video_renderer_base.cc
+++ b/media/filters/video_renderer_base.cc
@@ -25,7 +25,7 @@ base::TimeDelta VideoRendererBase::kMaxLastFrameDuration() {
 VideoRendererBase::VideoRendererBase(
     const scoped_refptr<base::MessageLoopProxy>& message_loop,
     const SetDecryptorReadyCB& set_decryptor_ready_cb,
-    const base::Closure& paint_cb,
+    const PaintCB& paint_cb,
     const SetOpaqueCB& set_opaque_cb,
     bool drop_frames)
     : message_loop_(message_loop),
@@ -34,12 +34,11 @@ VideoRendererBase::VideoRendererBase(
       state_(kUninitialized),
       thread_(base::kNullThreadHandle),
       pending_read_(false),
-      pending_paint_(false),
-      pending_paint_with_last_available_(false),
       drop_frames_(drop_frames),
       playback_rate_(0),
       paint_cb_(paint_cb),
-      set_opaque_cb_(set_opaque_cb) {
+      set_opaque_cb_(set_opaque_cb),
+      last_timestamp_(kNoTimestamp()) {
   DCHECK(!paint_cb_.is_null());
 }
 
@@ -83,32 +82,31 @@ void VideoRendererBase::ResetDecoder() {
 
 void VideoRendererBase::Stop(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
+  base::AutoLock auto_lock(lock_);
   if (state_ == kUninitialized || state_ == kStopped) {
     callback.Run();
     return;
   }
 
+  state_ = kStopped;
+
+  statistics_cb_.Reset();
+  max_time_cb_.Reset();
+  DoStopOrError_Locked();
+
+  // Clean up our thread if present.
   base::PlatformThreadHandle thread_to_join = base::kNullThreadHandle;
-  {
-    base::AutoLock auto_lock(lock_);
-    state_ = kStopped;
-
-    statistics_cb_.Reset();
-    max_time_cb_.Reset();
-    if (!pending_paint_ && !pending_paint_with_last_available_)
-      DoStopOrError_Locked();
-
-    // Clean up our thread if present.
-    if (thread_ != base::kNullThreadHandle) {
-      // Signal the thread since it's possible to get stopped with the video
-      // thread waiting for a read to complete.
-      frame_available_.Signal();
-      thread_to_join = thread_;
-      thread_ = base::kNullThreadHandle;
-    }
+  if (thread_ != base::kNullThreadHandle) {
+    // Signal the thread since it's possible to get stopped with the video
+    // thread waiting for a read to complete.
+    frame_available_.Signal();
+    std::swap(thread_, thread_to_join);
   }
-  if (thread_to_join != base::kNullThreadHandle)
+
+  if (thread_to_join != base::kNullThreadHandle) {
+    base::AutoUnlock auto_unlock(lock_);
     base::PlatformThread::Join(thread_to_join);
+  }
 
   if (decrypting_demuxer_stream_) {
     decrypting_demuxer_stream_->Reset(base::Bind(
@@ -252,8 +250,6 @@ void VideoRendererBase::ThreadMain() {
   const base::TimeDelta kIdleTimeDelta =
       base::TimeDelta::FromMilliseconds(10);
 
-  uint32 frames_dropped = 0;
-
   for (;;) {
     base::AutoLock auto_lock(lock_);
 
@@ -261,14 +257,6 @@ void VideoRendererBase::ThreadMain() {
     if (state_ == kStopped)
       return;
 
-    if (frames_dropped > 0) {
-      PipelineStatistics statistics;
-      statistics.video_frames_dropped = frames_dropped;
-      statistics_cb_.Run(statistics);
-
-      frames_dropped = 0;
-    }
-
     // Remain idle as long as we're not playing.
     if (state_ != kPlaying || playback_rate_ == 0) {
       frame_available_.TimedWait(kIdleTimeDelta);
@@ -281,25 +269,15 @@ void VideoRendererBase::ThreadMain() {
       continue;
     }
 
-    // Remain idle until we've initialized |current_frame_| via prerolling.
-    if (!current_frame_) {
-      // This can happen if our preroll only contains end of stream frames.
-      if (ready_frames_.front()->IsEndOfStream()) {
-        state_ = kEnded;
-        ended_cb_.Run();
-        ready_frames_.clear();

[acolwell GONE FROM CHROMIUM, 2013/02/04 17:35:15]

Shouldn't we keep this line?

[scherkus (not reviewing), 2013/02/05 00:29:39]

Done.

-
-        // No need to sleep here as we idle when |state_ != kPlaying|.
-        continue;
-      }
+    // We've reached the end!
+    if (ready_frames_.front()->IsEndOfStream()) {
+      state_ = kEnded;
+      ended_cb_.Run();
 
-      frame_available_.TimedWait(kIdleTimeDelta);
+      // No need to sleep here as we idle when |state_ != kPlaying|.
       continue;
     }
 
-    // Calculate how long until we should advance the frame, which is
-    // typically negative but for playback rates < 1.0f may be long enough
-    // that it makes more sense to idle and check again.
     base::TimeDelta remaining_time =
         CalculateSleepDuration(ready_frames_.front(), playback_rate_);
 
@@ -311,144 +289,63 @@ void VideoRendererBase::ThreadMain() {
       continue;
     }
 
-
-    // We're almost there!
-    //
-    // At this point we've rendered |current_frame_| for the proper amount
-    // of time and also have the next frame that ready for rendering.
-
-
-    // If the next frame is end of stream then we are truly at the end of the
-    // video stream.
+    // Deadline is defined as the midpoint between this frame and the next

[scherkus (not reviewing), 2013/02/01 22:45:25]

The old code's deadline was defined as:
remaining_time < 0 microseconds && pending_paint_

... meaning we'd only drop frames if painting was so slow (i.e., delta between
GCF() and PCF() calls is large enough) that we'd check the deadline condition.

While this isn't a perfect at least we'll avoid running paint_cb_ with frames
that we know are running late.

[scherkus (not reviewing), 2013/02/05 00:29:39]

Expanded this comment w/ more ideas.

+    // frame, using the delta between this frame and the previous frame as the
+    // assumption for frame duration.
     //
-    // TODO(scherkus): deduplicate this end of stream check after we get rid of
-    // |current_frame_|.
-    if (ready_frames_.front()->IsEndOfStream()) {
-      state_ = kEnded;
-      ended_cb_.Run();
-      ready_frames_.clear();
-
-      // No need to sleep here as we idle when |state_ != kPlaying|.
-      continue;
-    }
-
-    // We cannot update |current_frame_| until we've completed the pending
-    // paint. Furthermore, the pending paint might be really slow: check to
-    // see if we have any ready frames that we can drop if they've already
-    // expired.
-    if (pending_paint_) {
-      while (!ready_frames_.empty()) {
-        // Can't drop anything if we're at the end.
-        if (ready_frames_.front()->IsEndOfStream())
-          break;
-
-        base::TimeDelta remaining_time =
-            ready_frames_.front()->GetTimestamp() - get_time_cb_.Run();
-
-        // Still a chance we can render the frame!
-        if (remaining_time.InMicroseconds() > 0)
-          break;
-
-        if (!drop_frames_)
-          break;
-
-        // Frame dropped: read again.
-        ++frames_dropped;
-        ready_frames_.pop_front();
-        message_loop_->PostTask(FROM_HERE, base::Bind(
-            &VideoRendererBase::AttemptRead, this));
+    // TODO(scherkus): This can be vastly improved. Use a histogram to measure
+    // the accuracy of our frame timing code. http://crbug.com/149829
+    if (drop_frames_ && last_timestamp_ != kNoTimestamp()) {
+      base::TimeDelta now = get_time_cb_.Run();
+      base::TimeDelta deadline = ready_frames_.front()->GetTimestamp() +
+          (ready_frames_.front()->GetTimestamp() - last_timestamp_) / 2;
+
+      if (now > deadline) {
+        DropNextReadyFrame_Locked();
+        continue;
       }
-      // Continue waiting for the current paint to finish.
-      frame_available_.TimedWait(kIdleTimeDelta);
-      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.
-    DCHECK(!pending_paint_);
-    DCHECK(!ready_frames_.empty());
-    SetCurrentFrameToNextReadyFrame();
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &VideoRendererBase::AttemptRead, this));
-
-    base::AutoUnlock auto_unlock(lock_);
-    paint_cb_.Run();
+    // At this point enough time has passed that the next frame that ready for
+    // rendering.
+    PaintNextReadyFrame_Locked();
   }
 }
 
-void VideoRendererBase::SetCurrentFrameToNextReadyFrame() {
-  current_frame_ = ready_frames_.front();
+void VideoRendererBase::PaintNextReadyFrame_Locked() {
+  lock_.AssertAcquired();
+
+  scoped_refptr<VideoFrame> next_frame = ready_frames_.front();
   ready_frames_.pop_front();
 
-  // Notify the pipeline of natural_size() changes.
-  const gfx::Size& natural_size = current_frame_->natural_size();
+  last_timestamp_ = next_frame->GetTimestamp();
+
+  const gfx::Size& natural_size = next_frame->natural_size();
   if (natural_size != last_natural_size_) {
-    size_changed_cb_.Run(natural_size);
     last_natural_size_ = natural_size;
+    size_changed_cb_.Run(natural_size);
   }
-}
-
-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())) {
-    *frame_out = NULL;
-    return;
-  }
+  paint_cb_.Run(next_frame);
 
-  // We should have initialized and have the current frame.
-  DCHECK_NE(state_, kUninitialized);
-  DCHECK_NE(state_, kStopped);
-  DCHECK_NE(state_, kError);
-
-  if (current_frame_) {
-    *frame_out = current_frame_;
-    last_available_frame_ = current_frame_;
-    pending_paint_ = true;
-  } else {
-    DCHECK(last_available_frame_);
-    *frame_out = last_available_frame_;
-    pending_paint_with_last_available_ = true;
-  }
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &VideoRendererBase::AttemptRead, this));
 }
 
-void VideoRendererBase::PutCurrentFrame(scoped_refptr<VideoFrame> frame) {
-  base::AutoLock auto_lock(lock_);
-
-  // Note that we do not claim |pending_paint_| when we return NULL frame, in
-  // that case, |current_frame_| could be changed before PutCurrentFrame.
-  if (pending_paint_) {
-    DCHECK_EQ(current_frame_, frame);
-    DCHECK(!pending_paint_with_last_available_);
-    pending_paint_ = false;
-  } else if (pending_paint_with_last_available_) {
-    DCHECK_EQ(last_available_frame_, frame);
-    DCHECK(!pending_paint_);
-    pending_paint_with_last_available_ = false;
-  } else {
-    DCHECK(!frame);
-  }
+void VideoRendererBase::DropNextReadyFrame_Locked() {
+  lock_.AssertAcquired();
 
-  // 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_ == kFlushingDecoder)
-    return;
+  last_timestamp_ = ready_frames_.front()->GetTimestamp();
+  ready_frames_.pop_front();
 
-  if (state_ == kFlushing) {
-    AttemptFlush_Locked();
-    return;
-  }
+  PipelineStatistics statistics;
+  statistics.video_frames_dropped = 1;
+  statistics_cb_.Run(statistics);
 
-  if (state_ == kError || state_ == kStopped) {
-    DoStopOrError_Locked();
-  }
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &VideoRendererBase::AttemptRead, this));
 }
 
 VideoRendererBase::~VideoRendererBase() {
@@ -532,23 +429,18 @@ void VideoRendererBase::FrameReady(VideoDecoder::Status status,
 
   // If we're at capacity or end of stream while prerolling we need to
   // transition to prerolled.
-  if (state_ == kPrerolling) {
-    DCHECK(!current_frame_);
-    state_ = kPrerolled;
+  if (state_ != kPrerolling)
+    return;
 
-    // Because we might remain in the prerolled state for an undetermined amount
-    // of time (i.e., we were not playing before we started prerolling), we'll
-    // manually update the current frame and notify the subclass below.
-    if (!ready_frames_.front()->IsEndOfStream())
-      SetCurrentFrameToNextReadyFrame();
+  state_ = kPrerolled;
 
-    // ...and we're done prerolling!
-    DCHECK(!preroll_cb_.is_null());
-    base::ResetAndReturn(&preroll_cb_).Run(PIPELINE_OK);
+  // Because we might remain in the prerolled state for an undetermined amount
+  // of time (e.g., we seeked while paused), we'll paint the first prerolled
+  // frame.
+  if (!ready_frames_.front()->IsEndOfStream())
+    PaintNextReadyFrame_Locked();
 
-    base::AutoUnlock ul(lock_);
-    paint_cb_.Run();
-  }
+  base::ResetAndReturn(&preroll_cb_).Run(PIPELINE_OK);
 }
 
 void VideoRendererBase::AddReadyFrame(const scoped_refptr<VideoFrame>& frame) {
@@ -564,10 +456,9 @@ void VideoRendererBase::AddReadyFrame(const scoped_refptr<VideoFrame>& frame) {
       end_timestamp = std::min(
           duration,
           ready_frames_.back()->GetTimestamp() + kMaxLastFrameDuration());
-    } else if (current_frame_) {
+    } else if (last_timestamp_ != kNoTimestamp()) {
       end_timestamp =
-          std::min(duration,
-                   current_frame_->GetTimestamp() + kMaxLastFrameDuration());
+          std::min(duration, last_timestamp_ + kMaxLastFrameDuration());
     }
     frame->SetTimestamp(end_timestamp);
   } else if (frame->GetTimestamp() > duration) {
@@ -636,11 +527,12 @@ void VideoRendererBase::AttemptFlush_Locked() {
   prerolling_delayed_frame_ = NULL;
   ready_frames_.clear();
 
-  if (!pending_paint_ && !pending_read_) {
-    state_ = kFlushed;
-    current_frame_ = NULL;
-    base::ResetAndReturn(&flush_cb_).Run();
-  }
+  if (pending_read_)
+    return;
+
+  state_ = kFlushed;
+  last_timestamp_ = kNoTimestamp();
+  base::ResetAndReturn(&flush_cb_).Run();
 }
 
 base::TimeDelta VideoRendererBase::CalculateSleepDuration(
@@ -657,20 +549,14 @@ base::TimeDelta VideoRendererBase::CalculateSleepDuration(
 }
 
 void VideoRendererBase::DoStopOrError_Locked() {
-  DCHECK(!pending_paint_);
-  DCHECK(!pending_paint_with_last_available_);
   lock_.AssertAcquired();
-  current_frame_ = NULL;
-  last_available_frame_ = NULL;
+  last_timestamp_ = kNoTimestamp();
   ready_frames_.clear();
 }
 
 int VideoRendererBase::NumFrames_Locked() const {
   lock_.AssertAcquired();
-  int outstanding_frames =
-      (current_frame_ ? 1 : 0) + (last_available_frame_ ? 1 : 0) +
-      (current_frame_ && (current_frame_ == last_available_frame_) ? -1 : 0);
-  return ready_frames_.size() + outstanding_frames;
+  return ready_frames_.size();
 }
 
 }  // namespace media