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..178cbc7ccdb8f5c7ca32ef89e79e1559c7d44a6b 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());
 }
 
@@ -95,8 +94,7 @@ void VideoRendererBase::Stop(const base::Closure& callback) {
 
     statistics_cb_.Reset();
     max_time_cb_.Reset();
-    if (!pending_paint_ && !pending_paint_with_last_available_)
-      DoStopOrError_Locked();
+    DoStopOrError_Locked();
 
     // Clean up our thread if present.
     if (thread_ != base::kNullThreadHandle) {
@@ -281,19 +279,13 @@ 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();
-
-        // No need to sleep here as we idle when |state_ != kPlaying|.
-        continue;
-      }
+    // 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();
 
-      frame_available_.TimedWait(kIdleTimeDelta);
+      // No need to sleep here as we idle when |state_ != kPlaying|.
       continue;
     }
 
@@ -311,144 +303,28 @@ 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.
-    //
-    // 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));
-      }
-      // 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();
+    // At this point enough time has passed that the next frame that ready for
+    // rendering.
+    PaintWithNextReadyFrame();
     message_loop_->PostTask(FROM_HERE, base::Bind(
         &VideoRendererBase::AttemptRead, this));
-
-    base::AutoUnlock auto_unlock(lock_);
-    paint_cb_.Run();
   }
 }
 
-void VideoRendererBase::SetCurrentFrameToNextReadyFrame() {
-  current_frame_ = ready_frames_.front();
+void VideoRendererBase::PaintWithNextReadyFrame() {

[acolwell GONE FROM CHROMIUM, 2013/02/01 00:24:34]

nit: Add _Locked() & corresponding assert since this is expected to be called
under lock.

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

Done.

+  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();
+  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;
   }
-}
-
-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;
-  }
-
-  // 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;
-  }
-}
-
-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);
-  }
-
-  // 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;
-
-  if (state_ == kFlushing) {
-    AttemptFlush_Locked();
-    return;
-  }
-
-  if (state_ == kError || state_ == kStopped) {
-    DoStopOrError_Locked();
-  }
+  paint_cb_.Run(next_frame);

[acolwell GONE FROM CHROMIUM, 2013/02/01 00:24:34]

This calls the callback under lock now. Are you sure that is ok?

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

Yes. Previously it was terrifying because the most common use case for callees
was to turn around and call GetCurrentFrame().

Callees (i.e., WMPProxy, the player_x11 thingies) don't even maintain a
reference to a VRB anymore!

 }
 
 VideoRendererBase::~VideoRendererBase() {
@@ -533,21 +409,22 @@ 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) {

[acolwell GONE FROM CHROMIUM, 2013/02/01 00:24:34]

nit: reverse check and unindent?

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

Done.

-    DCHECK(!current_frame_);
     state_ = kPrerolled;
 
     // 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();
+      PaintWithNextReadyFrame();
 
     // ...and we're done prerolling!
-    DCHECK(!preroll_cb_.is_null());
     base::ResetAndReturn(&preroll_cb_).Run(PIPELINE_OK);
 
+#if 0
+    // XXX do we still need this!?

[scherkus (not reviewing), 2013/01/31 17:54:05]

PWNRF() fires the paint_cb_ ... I suppose this might be here to handle the case
where we preroll to nothing but EOS frames?

[acolwell GONE FROM CHROMIUM, 2013/02/01 00:24:34]

Not sure. What did we use to paint in the case where there wasn't a current
frame?

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

current_frame_ would have been null, so SkCanvasVideoRenderer would have painted
black

If at all possible I'd like for us to guarantee that a PaintCB won't get called
with a null frame. I definitely don't want VRB to generate black frames as I
feel that the decision on what the application should render if it doesn't have
a frame should be left up to the application, not VRB!

Anyway ... I dug into the history of this code and the paint_cb_.Run() below is
actually handled by PaintNextReadyFrame_Locked() so we can safely delete. The
previous code would update current_frame_ and rely on the paint_cb_.Run() below
to be called (refer to old school comment about "manually update the current
frame and notify the subclass below").

     base::AutoUnlock ul(lock_);
     paint_cb_.Run();
+#endif
   }
 }
 
@@ -564,10 +441,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,9 +512,9 @@ void VideoRendererBase::AttemptFlush_Locked() {
   prerolling_delayed_frame_ = NULL;
   ready_frames_.clear();
 
-  if (!pending_paint_ && !pending_read_) {
+  if (!pending_read_) {
     state_ = kFlushed;
-    current_frame_ = NULL;
+    last_timestamp_ = kNoTimestamp();
     base::ResetAndReturn(&flush_cb_).Run();
   }
 }
@@ -657,20 +533,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);

[acolwell GONE FROM CHROMIUM, 2013/02/01 00:24:34]

o.O  I'm not going to miss this.

-  return ready_frames_.size() + outstanding_frames;
+  return ready_frames_.size();
 }
 
 }  // namespace media