Refactor VideoRendererImpl to use VideoFrameScheduler.

media/filters/video_renderer_impl.cc

Index: media/filters/video_renderer_impl.cc
diff --git a/media/filters/video_renderer_impl.cc b/media/filters/video_renderer_impl.cc
index 7d99dbf14a4bbfda6a7b5c68b8e253ddb46dd4f8..76e4e13654ae5fe14c123cb94198e4f37195c48b 100644
--- a/media/filters/video_renderer_impl.cc
+++ b/media/filters/video_renderer_impl.cc
@@ -5,74 +5,74 @@
 #include "media/filters/video_renderer_impl.h"
 
 #include "base/bind.h"
-#include "base/callback.h"
 #include "base/callback_helpers.h"
-#include "base/debug/trace_event.h"
-#include "base/location.h"
 #include "base/single_thread_task_runner.h"
-#include "base/threading/platform_thread.h"
-#include "media/base/buffers.h"
+#include "base/time/default_tick_clock.h"
 #include "media/base/limits.h"
-#include "media/base/pipeline.h"
 #include "media/base/video_frame.h"
 
 namespace media {
 
 VideoRendererImpl::VideoRendererImpl(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    scoped_ptr<VideoFrameScheduler> scheduler,
     ScopedVector<VideoDecoder> decoders,
-    const SetDecryptorReadyCB& set_decryptor_ready_cb,
-    const PaintCB& paint_cb,
-    bool drop_frames)
+    const SetDecryptorReadyCB& set_decryptor_ready_cb)
     : task_runner_(task_runner),
+      scheduler_(scheduler.Pass()),
+      tick_clock_(new base::DefaultTickClock()),
       video_frame_stream_(task_runner, decoders.Pass(), set_decryptor_ready_cb),
       received_end_of_stream_(false),
-      frame_available_(&lock_),
       state_(kUninitialized),
-      thread_(),
       pending_read_(false),
-      drop_frames_(drop_frames),
       playback_rate_(0),
-      paint_cb_(paint_cb),
-      last_timestamp_(kNoTimestamp()),
       frames_decoded_(0),
       frames_dropped_(0),
       weak_factory_(this) {
-  DCHECK(!paint_cb_.is_null());
 }
 
 VideoRendererImpl::~VideoRendererImpl() {
-  base::AutoLock auto_lock(lock_);
   CHECK(state_ == kStopped || state_ == kUninitialized) << state_;
-  CHECK(thread_.is_null());
 }
 
 void VideoRendererImpl::Play(const base::Closure& callback) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
   DCHECK_EQ(kPrerolled, state_);
   state_ = kPlaying;
+
+  ScheduleVideoFrames();
+  AttemptRead();
+
+  // Changing from non-playing to playing state can trigger ended.
+  if (ShouldTransitionToEnded())
+    TransitionToEnded();
+
   callback.Run();
 }
 
 void VideoRendererImpl::Pause(const base::Closure& callback) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
   DCHECK(state_ != kUninitialized || state_ == kError);

[acolwell GONE FROM CHROMIUM, 2014/04/24 16:43:59]

nit: Add != kPausing ?

[scherkus (not reviewing), 2014/04/25 02:04:47]

kPausing is no more!

-  state_ = kPaused;
-  callback.Run();
+
+  state_ = kPausing;
+  pause_cb_ = callback;
+  scheduler_->Reset();
+
+  if (ShouldTransitionToPaused())
+    TransitionToPaused();
 }
 
 void VideoRendererImpl::Flush(const base::Closure& callback) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
   DCHECK_EQ(state_, kPaused);
+  DCHECK_EQ(scheduled_frames_.size(), 0u);
   flush_cb_ = callback;
   state_ = kFlushing;
 
   // This is necessary if the |video_frame_stream_| has already seen an end of
   // stream and needs to drain it before flushing it.

[acolwell GONE FROM CHROMIUM, 2014/04/24 16:43:59]

nit: Is this comment still correct? It confuses me. We need to do this because
frames could have arrived in the course of pausing right? 

[scherkus (not reviewing), 2014/04/25 02:04:47]

That's my understanding.

I feel the comment might be a bit stale... xhwang: you added this in r211288 --
do you recall whether it still might be relevant?

[xhwang, 2014/04/25 05:44:47]

If you look at the history, it existed even before r211288. In r211288 I
modified it a bit when I introduce VFS...

Anyway, this comment is confusing here and I am in favor of removing it :)

[scherkus (not reviewing), 2014/04/25 18:24:25]

Removed.

-  ready_frames_.clear();
+  unscheduled_frames_.clear();
+
   received_end_of_stream_ = false;
   video_frame_stream_.Reset(
       base::Bind(&VideoRendererImpl::OnVideoFrameStreamResetDone,
@@ -81,7 +81,6 @@ void VideoRendererImpl::Flush(const base::Closure& callback) {
 
 void VideoRendererImpl::Stop(const base::Closure& callback) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
   if (state_ == kUninitialized || state_ == kStopped) {
     callback.Run();
     return;
@@ -94,44 +93,42 @@ void VideoRendererImpl::Stop(const base::Closure& callback) {
 
   statistics_cb_.Reset();
   max_time_cb_.Reset();
-  DoStopOrError_Locked();
-
-  // Clean up our thread if present.
-  base::PlatformThreadHandle thread_to_join = base::PlatformThreadHandle();
-  if (!thread_.is_null()) {
-    // 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.is_null()) {
-    base::AutoUnlock auto_unlock(lock_);
-    base::PlatformThread::Join(thread_to_join);
-  }
+  // Leave |scheduled_frames_| untouched. If we get the frames back -- great --
+  // but it's more likely we'll be destroyed before that.
+  unscheduled_frames_.clear();
+  scheduler_->Reset();
 
   video_frame_stream_.Stop(callback);
 }
 
 void VideoRendererImpl::SetPlaybackRate(float playback_rate) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
+  if (playback_rate_ == playback_rate)
+    return;
+
   playback_rate_ = playback_rate;
+
+  scheduler_->Reset();
+  if (state_ == kPlaying)
+    ScheduleVideoFrames();
 }
 
 void VideoRendererImpl::Preroll(base::TimeDelta time,
                                 const PipelineStatusCB& cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
   DCHECK(!cb.is_null());
   DCHECK(preroll_cb_.is_null());
-  DCHECK(state_ == kFlushed || state_== kPaused) << "state_ " << state_;
+  DCHECK(state_ == kFlushed || state_ == kPaused) << "state_ " << state_;
 
   if (state_ == kFlushed) {
     DCHECK(time != kNoTimestamp());
     DCHECK(!pending_read_);
-    DCHECK(ready_frames_.empty());
+    DCHECK(unscheduled_frames_.empty());
+    DCHECK(scheduled_frames_.empty());
   } else {
+    // Prerolling without a timestamp is used for rebuffering. No painting
+    // is required so skip ahead one state.
     DCHECK(time == kNoTimestamp());
   }
 
@@ -139,12 +136,12 @@ void VideoRendererImpl::Preroll(base::TimeDelta time,
   preroll_cb_ = cb;
   preroll_timestamp_ = time;
 
-  if (ShouldTransitionToPrerolled_Locked()) {
-    TransitionToPrerolled_Locked();
+  if (ShouldTransitionToPrerolled()) {
+    TransitionToPrerolled();
     return;
   }
 
-  AttemptRead_Locked();
+  AttemptRead();
 }
 
 void VideoRendererImpl::Initialize(DemuxerStream* stream,
@@ -156,7 +153,6 @@ void VideoRendererImpl::Initialize(DemuxerStream* stream,
                                    const TimeDeltaCB& get_time_cb,
                                    const TimeDeltaCB& get_duration_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
   DCHECK(stream);
   DCHECK_EQ(stream->type(), DemuxerStream::VIDEO);
   DCHECK(!init_cb.is_null());
@@ -185,7 +181,6 @@ void VideoRendererImpl::Initialize(DemuxerStream* stream,
 
 void VideoRendererImpl::OnVideoFrameStreamInitialized(bool success) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::AutoLock auto_lock(lock_);
 
   if (state_ == kStopped)
     return;
@@ -204,137 +199,11 @@ void VideoRendererImpl::OnVideoFrameStreamInitialized(bool success) {
   // have not populated any buffers yet.
   state_ = kFlushed;
 
-  // Create our video thread.
-  if (!base::PlatformThread::Create(0, this, &thread_)) {
-    NOTREACHED() << "Video thread creation failed";
-    state_ = kError;
-    base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_INITIALIZATION_FAILED);
-    return;
-  }
-
-#if defined(OS_WIN)
-  // Bump up our priority so our sleeping is more accurate.
-  // TODO(scherkus): find out if this is necessary, but it seems to help.
-  ::SetThreadPriority(thread_.platform_handle(), THREAD_PRIORITY_ABOVE_NORMAL);
-#endif  // defined(OS_WIN)
   base::ResetAndReturn(&init_cb_).Run(PIPELINE_OK);
 }
 
-// PlatformThread::Delegate implementation.
-void VideoRendererImpl::ThreadMain() {
-  base::PlatformThread::SetName("CrVideoRenderer");
-
-  // 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.
-  //
-  // TODO(scherkus): switch to pure event-driven frame timing instead of this
-  // kIdleTimeDelta business http://crbug.com/106874
-  const base::TimeDelta kIdleTimeDelta =
-      base::TimeDelta::FromMilliseconds(10);
-
-  for (;;) {
-    base::AutoLock auto_lock(lock_);
-
-    // Thread exit condition.
-    if (state_ == kStopped)
-      return;
-
-    // Remain idle as long as we're not playing.
-    if (state_ != kPlaying || playback_rate_ == 0) {
-      UpdateStatsAndWait_Locked(kIdleTimeDelta);
-      continue;
-    }
-
-    // Remain idle until we have the next frame ready for rendering.
-    if (ready_frames_.empty()) {
-      if (received_end_of_stream_) {
-        state_ = kEnded;
-        ended_cb_.Run();
-
-        // No need to sleep here as we idle when |state_ != kPlaying|.
-        continue;
-      }
-
-      UpdateStatsAndWait_Locked(kIdleTimeDelta);
-      continue;
-    }
-
-    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);
-      UpdateStatsAndWait_Locked(remaining_time);
-      continue;
-    }
-
-    // Deadline is defined as the midpoint between this frame and the next
-    // frame, using the delta between this frame and the previous frame as the
-    // assumption for frame duration.
-    //
-    // TODO(scherkus): An improvement over midpoint might be selecting the
-    // minimum and/or maximum between the midpoint and some constants. As a
-    // thought experiment, consider what would be better than the midpoint
-    // for both the 1fps case and 120fps case.
-    //
-    // 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()->timestamp() +
-          (ready_frames_.front()->timestamp() - last_timestamp_) / 2;
-
-      if (now > deadline) {
-        DropNextReadyFrame_Locked();
-        continue;
-      }
-    }
-
-    // Congratulations! You've made it past the video frame timing gauntlet.
-    //
-    // At this point enough time has passed that the next frame that ready for
-    // rendering.
-    PaintNextReadyFrame_Locked();
-  }
-}
-
-void VideoRendererImpl::PaintNextReadyFrame_Locked() {
-  lock_.AssertAcquired();
-
-  scoped_refptr<VideoFrame> next_frame = ready_frames_.front();
-  ready_frames_.pop_front();
-  frames_decoded_++;
-
-  last_timestamp_ = next_frame->timestamp();
-
-  paint_cb_.Run(next_frame);
-
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&VideoRendererImpl::AttemptRead, weak_factory_.GetWeakPtr()));
-}
-
-void VideoRendererImpl::DropNextReadyFrame_Locked() {
-  TRACE_EVENT0("media", "VideoRendererImpl:frameDropped");
-
-  lock_.AssertAcquired();
-
-  last_timestamp_ = ready_frames_.front()->timestamp();
-  ready_frames_.pop_front();
-  frames_decoded_++;
-  frames_dropped_++;
-
-  task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&VideoRendererImpl::AttemptRead, weak_factory_.GetWeakPtr()));
-}
-
 void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
                                    const scoped_refptr<VideoFrame>& frame) {
-  base::AutoLock auto_lock(lock_);
   DCHECK_NE(state_, kUninitialized);
   DCHECK_NE(state_, kFlushed);
 
@@ -367,7 +236,7 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
     // A new preroll will be requested after this one completes so there is no
     // point trying to collect more frames.
     if (state_ == kPrerolling)
-      TransitionToPrerolled_Locked();
+      TransitionToPrerolled();
 
     return;
   }
@@ -378,7 +247,10 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
     max_time_cb_.Run(get_duration_cb_.Run());
 
     if (state_ == kPrerolling)
-      TransitionToPrerolled_Locked();
+      TransitionToPrerolled();
+
+    if (ShouldTransitionToEnded())
+      TransitionToEnded();
 
     return;
   }
@@ -387,32 +259,9 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
   // prerolling has completed.
   if (state_ == kPrerolling && preroll_timestamp_ != kNoTimestamp() &&
       frame->timestamp() <= preroll_timestamp_) {
-    ready_frames_.clear();
+    unscheduled_frames_.clear();
   }
 
-  AddReadyFrame_Locked(frame);
-
-  if (ShouldTransitionToPrerolled_Locked())
-    TransitionToPrerolled_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
-  AttemptRead_Locked();
-}
-
-bool VideoRendererImpl::ShouldTransitionToPrerolled_Locked() {
-  return state_ == kPrerolling &&
-      (!video_frame_stream_.CanReadWithoutStalling() ||
-       ready_frames_.size() >= static_cast<size_t>(limits::kMaxVideoFrames));
-}
-
-void VideoRendererImpl::AddReadyFrame_Locked(
-    const scoped_refptr<VideoFrame>& frame) {
-  lock_.AssertAcquired();
-  DCHECK(!frame->end_of_stream());
-
   // 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
@@ -423,32 +272,64 @@ void VideoRendererImpl::AddReadyFrame_Locked(
     frame->set_timestamp(duration);
   }
 
-  ready_frames_.push_back(frame);
-  DCHECK_LE(ready_frames_.size(),
-            static_cast<size_t>(limits::kMaxVideoFrames));
+  unscheduled_frames_.push_back(frame);
+  DCHECK_LE(video_frame_count(), limits::kMaxVideoFrames);
 
   max_time_cb_.Run(frame->timestamp());
 
-  // Avoid needlessly waking up |thread_| unless playing.
+  if (ShouldTransitionToPrerolled())
+    TransitionToPrerolled();
+
   if (state_ == kPlaying)
-    frame_available_.Signal();
+    ScheduleVideoFrames();
+
+  if (ShouldTransitionToEnded())
+    TransitionToEnded();
+
+  // 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
+  AttemptRead();
 }
 
-void VideoRendererImpl::AttemptRead() {
-  base::AutoLock auto_lock(lock_);
-  AttemptRead_Locked();
+bool VideoRendererImpl::ShouldTransitionToPrerolled() {
+  return state_ == kPrerolling &&
+         (!video_frame_stream_.CanReadWithoutStalling() ||
+          video_frame_count() >= limits::kMaxVideoFrames);
 }
 
-void VideoRendererImpl::AttemptRead_Locked() {
+bool VideoRendererImpl::ShouldTransitionToPaused() {
+  return state_ == kPausing && scheduled_frames_.empty();
+}
+
+void VideoRendererImpl::TransitionToPaused() {
+  DCHECK(ShouldTransitionToPaused());
+  state_ = kPaused;
+  base::ResetAndReturn(&pause_cb_).Run();
+}

[xhwang, 2014/04/24 18:48:44]

nit: I found it's easier to understand if we just expand some
ShouldTransitionToXxx() and TransitionToXxx() functions. But maybe that's just
my problem :)

[scherkus (not reviewing), 2014/04/25 02:04:47]

yeah ... the biggest thing I wanted out of these tiny helper functions is to
enforce doing the DCHECK()

+
+bool VideoRendererImpl::ShouldTransitionToEnded() {
+  return state_ == kPlaying && unscheduled_frames_.empty() &&
+         scheduled_frames_.empty() && received_end_of_stream_;
+}
+
+void VideoRendererImpl::TransitionToEnded() {
+  DCHECK(ShouldTransitionToEnded());
+  state_ = kEnded;
+  ended_cb_.Run();
+}
+
+void VideoRendererImpl::AttemptRead() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  lock_.AssertAcquired();
 
   if (pending_read_ || received_end_of_stream_ ||
-      ready_frames_.size() == static_cast<size_t>(limits::kMaxVideoFrames)) {
+      video_frame_count() == limits::kMaxVideoFrames) {
     return;
   }
 
   switch (state_) {
+    case kPausing:
     case kPaused:
     case kPrerolling:
     case kPlaying:
@@ -470,41 +351,20 @@ void VideoRendererImpl::AttemptRead_Locked() {
 }
 
 void VideoRendererImpl::OnVideoFrameStreamResetDone() {
-  base::AutoLock auto_lock(lock_);
   if (state_ == kStopped)
     return;
 
   DCHECK_EQ(kFlushing, state_);
   DCHECK(!pending_read_);
-  DCHECK(ready_frames_.empty());
+  DCHECK(unscheduled_frames_.empty());
+  DCHECK(scheduled_frames_.empty());
   DCHECK(!received_end_of_stream_);
 
   state_ = kFlushed;
-  last_timestamp_ = kNoTimestamp();
   base::ResetAndReturn(&flush_cb_).Run();
 }
 
-base::TimeDelta VideoRendererImpl::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->timestamp();
-
-  // 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 VideoRendererImpl::DoStopOrError_Locked() {
-  lock_.AssertAcquired();
-  last_timestamp_ = kNoTimestamp();
-  ready_frames_.clear();
-}
-
-void VideoRendererImpl::TransitionToPrerolled_Locked() {
-  lock_.AssertAcquired();
+void VideoRendererImpl::TransitionToPrerolled() {
   DCHECK_EQ(state_, kPrerolling);
 
   state_ = kPrerolled;
@@ -512,29 +372,105 @@ void VideoRendererImpl::TransitionToPrerolled_Locked() {
   // 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_.empty())
-    PaintNextReadyFrame_Locked();
+  if (!unscheduled_frames_.empty()) {
+    ScheduleFirstFrameForImmediateDisplay();
+    AttemptRead();
+  }
 
   base::ResetAndReturn(&preroll_cb_).Run(PIPELINE_OK);
 }
 
-void VideoRendererImpl::UpdateStatsAndWait_Locked(
-    base::TimeDelta wait_duration) {
-  lock_.AssertAcquired();
-  DCHECK_GE(frames_decoded_, 0);
-  DCHECK_LE(frames_dropped_, frames_decoded_);
+void VideoRendererImpl::ScheduleVideoFrames() {
+  DCHECK_EQ(state_, kPlaying);
+
+  if (playback_rate_ == 0)
+    return;
 
-  if (frames_decoded_) {
-    PipelineStatistics statistics;
-    statistics.video_frames_decoded = frames_decoded_;
-    statistics.video_frames_dropped = frames_dropped_;
-    statistics_cb_.Run(statistics);
+  base::TimeDelta current_media_time = get_time_cb_.Run();
+  base::TimeTicks current_wall_ticks = tick_clock_->NowTicks();

[acolwell GONE FROM CHROMIUM, 2014/04/24 16:43:59]

hmm.. ISTM that pipeline's Clock should be the sole source of timestamp -> wall
clock mapping. This essentially creates a second mapping that could potentially
introduce drift. Perhaps we need to change get_time_cb_'s signature so we can
get the correct wall clock time from pipeline's Clock.

[scherkus (not reviewing), 2014/04/25 02:04:47]

I agree -- we're not quite done with how we want clocks sorted out but I'd
rather leave that for future CLs / investigation.

As it stands today, everyone is using base::DefaultTickClock so I don't think
we'll run into trouble.

 
-    frames_decoded_ = 0;
-    frames_dropped_ = 0;
+  while (!unscheduled_frames_.empty()) {
+    base::TimeDelta delta =
+        (unscheduled_frames_.front()->timestamp() - current_media_time) /
+        playback_rate_;

[xhwang, 2014/04/24 18:48:44]

So even if delta < 0 we still ScheduleVideoFrame()? Isn't that frame guaranteed
to be dropped? Are we trying to make the code cleaner (less branches)? This may
be worth a comment.

[scherkus (not reviewing), 2014/04/25 02:04:47]

Done.

It's up to the scheduler to decide whether it wants to drop the frame, otherwise
we're splitting frame dropping logic between two classes. It would also make it
more difficult to implement something like ClocklessVideoFrameScheduler is
VideoRendererImpl was making its own scheduling decisions.

+    base::TimeTicks target_wall_ticks = current_wall_ticks + delta;
+
+    scheduled_frames_.push_back(unscheduled_frames_.front());
+    unscheduled_frames_.pop_front();
+
+    scheduler_->ScheduleVideoFrame(
+        scheduled_frames_.back(),
+        target_wall_ticks,
+        base::Bind(&VideoRendererImpl::OnVideoFrameFinished,
+                   weak_factory_.GetWeakPtr()));
   }
+}
+
+void VideoRendererImpl::ScheduleFirstFrameForImmediateDisplay() {

[xhwang, 2014/04/24 18:48:44]

DCHECK_EQ(state_, kPrerolled);

[scherkus (not reviewing), 2014/04/25 02:04:47]

Done.

+  scheduled_frames_.push_back(unscheduled_frames_.front());
+  unscheduled_frames_.pop_front();
+
+  scheduler_->ScheduleVideoFrame(
+      scheduled_frames_.back(),
+      tick_clock_->NowTicks(),
+      base::Bind(&VideoRendererImpl::OnVideoFrameFinished,
+                 weak_factory_.GetWeakPtr()));
+}

[xhwang, 2014/04/24 18:48:44]

Probably we can have a helper function like 

ScheduleNextFrame(base::TimeTicks target_wall_ticks)

which can be used by both ScheduleVideoFrames() and
ScheduleFirstFrameForImmediateDisplay().

[scherkus (not reviewing), 2014/04/25 02:04:47]

I tried it out but didn't like how ScheduleAllFrames() still has to access
unscheduled_frames_.front() to calculate wall_ticks and then hope that
ScheduleNextFrame() will reference the same frame

+
+void VideoRendererImpl::OnVideoFrameFinished(
+    const scoped_refptr<VideoFrame>& frame,
+    VideoFrameScheduler::Reason reason) {
+  VideoFrameQueue::iterator iter =
+      std::find(scheduled_frames_.begin(), scheduled_frames_.end(), frame);
+  CHECK(iter != scheduled_frames_.end());
+
+  switch (reason) {
+    case VideoFrameScheduler::DISPLAYED: {
+      ++frames_decoded_;
+      scheduled_frames_.erase(iter);

[acolwell GONE FROM CHROMIUM, 2014/04/24 16:43:59]

nit: An reason not to do this before the switch since you do it in all branches
and you could simply push |frame| in the RESET case.

[scherkus (not reviewing), 2014/04/25 02:04:47]

Done.

+
+      PipelineStatistics stats;
+      stats.video_frames_decoded = frames_decoded_;
+      stats.video_frames_dropped = frames_dropped_;
+      statistics_cb_.Run(stats);
+      break;
+    }
+
+    case VideoFrameScheduler::DROPPED: {
+      ++frames_decoded_;
+      ++frames_dropped_;
+      scheduled_frames_.erase(iter);
+
+      PipelineStatistics stats;

[acolwell GONE FROM CHROMIUM, 2014/04/24 16:43:59]

nit: Move this below w/ a bool guard so we don't have to duplicate the code in
the switch and we only have to worry about one location where the CB is
dispatched?

[scherkus (not reviewing), 2014/04/25 02:04:47]

With change in Reset() behaviour this function cleaned up a bunch!

+      stats.video_frames_decoded = frames_decoded_;
+      stats.video_frames_dropped = frames_dropped_;
+      statistics_cb_.Run(stats);
+      break;
+    }
+
+    case VideoFrameScheduler::RESET: {
+      unscheduled_frames_.push_back(*iter);

[xhwang, 2014/04/24 18:48:44]

ha, It surprised me a bit that the "reset" frames will be rescheduled. It might
worth a comment at:

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/vide...

[scherkus (not reviewing), 2014/04/25 02:04:47]

out of curiosity, what did you think "reset" was for?

perhaps we can use a different function name? ("release", "return", etc...)

anyway Reset()'s behaviour has been changed to no longer call back

+      scheduled_frames_.erase(iter);
+      break;
+    }
+  }
+
+  if (ShouldTransitionToPaused()) {
+    TransitionToPaused();
+    return;
+  }
+
+  if (ShouldTransitionToEnded()) {
+    TransitionToEnded();
+    return;
+  }
+
+  AttemptRead();
+}
 
-  frame_available_.TimedWait(wait_duration);
+void VideoRendererImpl::SetTickClockForTesting(
+    scoped_ptr<base::TickClock> tick_clock) {
+  tick_clock_.swap(tick_clock);
 }
 
 }  // namespace media