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

media/filters/video_renderer_base.cc

 // Copyright (c) 2012 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/video_renderer_base.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/message_loop.h"
 #include "base/threading/platform_thread.h"
 #include "media/base/buffers.h"
 #include "media/base/limits.h"
 #include "media/base/pipeline.h"
 #include "media/base/video_frame.h"
 #include "media/filters/decrypting_demuxer_stream.h"
 #include "media/filters/video_decoder_selector.h"
 
 namespace media {
 
 base::TimeDelta VideoRendererBase::kMaxLastFrameDuration() {
   return base::TimeDelta::FromMilliseconds(250);
 }
 
 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),
       set_decryptor_ready_cb_(set_decryptor_ready_cb),
       frame_available_(&lock_),
       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());
 }
 
 void VideoRendererBase::Play(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
   DCHECK_EQ(kPrerolled, state_);
   state_ = kPlaying;
   callback.Run();
 }
@@ skipping 23 matching lines @@
 }
 
 void VideoRendererBase::ResetDecoder() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   base::AutoLock auto_lock(lock_);
   decoder_->Reset(base::Bind(&VideoRendererBase::OnDecoderResetDone, this));
 }
 
 void VideoRendererBase::Stop(const base::Closure& callback) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   if (state_ == kUninitialized || state_ == kStopped) {

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

I'm pretty sure these checks should be done under lock.

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

Done.

     callback.Run();

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

message_loop_->PostTask(FROM_HERE, callback) ?

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

I'm going to hold off on cleaning up VRB threading semantics is for a different
CL! (I want to combo it w/ adding DCHECKs to SerialRunner)

     return;
   }
 
   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();
+    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;

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

nit: std::swap(thread_to_join, thread_)

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

Done.

     }
   }
   if (thread_to_join != base::kNullThreadHandle)
     base::PlatformThread::Join(thread_to_join);
 
   if (decrypting_demuxer_stream_) {
     decrypting_demuxer_stream_->Reset(base::Bind(
         &VideoRendererBase::StopDecoder, this, callback));
     return;
   }
@@ skipping 156 matching lines @@
       frame_available_.TimedWait(kIdleTimeDelta);
       continue;
     }
 
     // Remain idle until we have the next frame ready for rendering.
     if (ready_frames_.empty()) {
       frame_available_.TimedWait(kIdleTimeDelta);
       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();
+    // 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;
-      }
-
-      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_);
 
     // Sleep up to a maximum of our idle time until we're within the time to
     // render the next frame.
     if (remaining_time.InMicroseconds() > 0) {
       remaining_time = std::min(remaining_time, kIdleTimeDelta);
       frame_available_.TimedWait(remaining_time);
       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() {
   base::AutoLock auto_lock(lock_);
   DCHECK(state_ == kUninitialized || state_ == kStopped) << state_;
 }
 
 void VideoRendererBase::FrameReady(VideoDecoder::Status status,
                                    const scoped_refptr<VideoFrame>& frame) {
   base::AutoLock auto_lock(lock_);
@@ skipping 63 matching lines @@
   // purposes:
   //   1) Prerolling while paused
   //   2) Keeps decoding going if video rendering thread starts falling behind
   if (NumFrames_Locked() < limits::kMaxVideoFrames && !frame->IsEndOfStream()) {
     AttemptRead_Locked();
     return;
   }
 
   // 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
   }
 }
 
 void VideoRendererBase::AddReadyFrame(const scoped_refptr<VideoFrame>& frame) {
   // Adjust the incoming frame if its rendering stop time is past the duration
   // of the video itself. This is typically the last frame of the video and
   // occurs if the container specifies a duration that isn't a multiple of the
   // frame rate.  Another way for this to happen is for the container to state a
   // smaller duration than the largest packet timestamp.
   base::TimeDelta duration = get_duration_cb_.Run();
   if (frame->IsEndOfStream()) {
     base::TimeDelta end_timestamp = kNoTimestamp();
     if (!ready_frames_.empty()) {
       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) {
     frame->SetTimestamp(duration);
   }
 
   ready_frames_.push_back(frame);
   DCHECK_LE(NumFrames_Locked(), limits::kMaxVideoFrames);
 
   base::TimeDelta max_clock_time =
@@ skipping 48 matching lines @@
   AttemptFlush_Locked();
 }
 
 void VideoRendererBase::AttemptFlush_Locked() {
   lock_.AssertAcquired();
   DCHECK_EQ(kFlushing, state_);
 
   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();
   }
 }
 
 base::TimeDelta VideoRendererBase::CalculateSleepDuration(
     const scoped_refptr<VideoFrame>& next_frame,
     float playback_rate) {
   // Determine the current and next presentation timestamps.
   base::TimeDelta now = get_time_cb_.Run();
   base::TimeDelta next_pts = next_frame->GetTimestamp();
 
   // Scale our sleep based on the playback rate.
   base::TimeDelta sleep = next_pts - now;
   return base::TimeDelta::FromMicroseconds(
       static_cast<int64>(sleep.InMicroseconds() / playback_rate));
 }
 
 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