Refactor VideoRendererImpl to use VideoFrameScheduler.

media/filters/video_renderer_impl_unittest.cc

Index: media/filters/video_renderer_impl_unittest.cc
diff --git a/media/filters/video_renderer_impl_unittest.cc b/media/filters/video_renderer_impl_unittest.cc
index 84ccc9ed29a9fd2f5890aa9c7a842cd395416a4a..f9e115d182843fd5074375479343dcb2434988bc 100644
--- a/media/filters/video_renderer_impl_unittest.cc
+++ b/media/filters/video_renderer_impl_unittest.cc
@@ -2,62 +2,51 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <utility>
-
 #include "base/bind.h"
-#include "base/callback.h"
-#include "base/callback_helpers.h"
 #include "base/debug/stack_trace.h"
 #include "base/message_loop/message_loop.h"
-#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/stringprintf.h"
-#include "base/synchronization/lock.h"
-#include "base/timer/timer.h"
-#include "media/base/data_buffer.h"
+#include "base/test/simple_test_tick_clock.h"
 #include "media/base/gmock_callback_support.h"
-#include "media/base/limits.h"
 #include "media/base/mock_filters.h"
 #include "media/base/test_helpers.h"
-#include "media/base/video_frame.h"
+#include "media/filters/test_video_frame_scheduler.h"
 #include "media/filters/video_renderer_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
 using ::testing::AnyNumber;
-using ::testing::InSequence;
 using ::testing::Invoke;
 using ::testing::NiceMock;
-using ::testing::NotNull;
-using ::testing::Return;
-using ::testing::StrictMock;
 
 namespace media {
 
-MATCHER_P(HasTimestamp, ms, "") {
-  *result_listener << "has timestamp " << arg->timestamp().InMilliseconds();
-  return arg->timestamp().InMilliseconds() == ms;
+static void AssignValue(bool* b, bool value) {
+  *b = value;
 }
 
-// Arbitrary value. Has to be larger to cover any timestamp value used in tests.
-static const int kVideoDurationInMs = 1000;
-
 class VideoRendererImplTest : public ::testing::Test {
  public:
   VideoRendererImplTest()
       : decoder_(new MockVideoDecoder()),
-        demuxer_stream_(DemuxerStream::VIDEO) {
+        scheduler_(new TestVideoFrameScheduler()),
+        tick_clock_(new base::SimpleTestTickClock()),
+        demuxer_stream_(DemuxerStream::VIDEO),
+        max_time_(kNoTimestamp()),
+        decoded_frames_(0),
+        dropped_frames_(0),
+        ended_cb_run_(false) {
     ScopedVector<VideoDecoder> decoders;
     decoders.push_back(decoder_);
 
     renderer_.reset(
         new VideoRendererImpl(message_loop_.message_loop_proxy(),
+                              scoped_ptr<VideoFrameScheduler>(scheduler_),
                               decoders.Pass(),
-                              media::SetDecryptorReadyCB(),
-                              base::Bind(&StrictMock<MockDisplayCB>::Display,
-                                         base::Unretained(&mock_display_cb_)),
-                              true));
+                              media::SetDecryptorReadyCB()));
+    renderer_->SetTickClockForTesting(scoped_ptr<base::TickClock>(tick_clock_));
 
     demuxer_stream_.set_video_decoder_config(TestVideoConfig::Normal());
 
@@ -67,17 +56,10 @@ class VideoRendererImplTest : public ::testing::Test {
                                        DecoderBuffer::CreateEOSBuffer()));
     EXPECT_CALL(*decoder_, Stop())
         .WillRepeatedly(Invoke(this, &VideoRendererImplTest::StopRequested));
-    EXPECT_CALL(statistics_cb_object_, OnStatistics(_))
-        .Times(AnyNumber());
-    EXPECT_CALL(*this, OnTimeUpdate(_))
-        .Times(AnyNumber());
   }
 
   virtual ~VideoRendererImplTest() {}
 
-  // Callbacks passed into Initialize().
-  MOCK_METHOD1(OnTimeUpdate, void(base::TimeDelta));
-
   void Initialize() {
     // Monitor decodes from the decoder.
     EXPECT_CALL(*decoder_, Decode(_, _))
@@ -86,8 +68,6 @@ class VideoRendererImplTest : public ::testing::Test {
     EXPECT_CALL(*decoder_, Reset(_))
         .WillRepeatedly(Invoke(this, &VideoRendererImplTest::FlushRequested));
 
-    InSequence s;
-
     EXPECT_CALL(*decoder_, Initialize(_, _))
         .WillOnce(RunCallback<1>(PIPELINE_OK));
 
@@ -109,15 +89,14 @@ class VideoRendererImplTest : public ::testing::Test {
     renderer_->Initialize(
         &demuxer_stream_,
         status_cb,
-        base::Bind(&MockStatisticsCB::OnStatistics,
-                   base::Unretained(&statistics_cb_object_)),
-        base::Bind(&VideoRendererImplTest::OnTimeUpdate,
+        base::Bind(&VideoRendererImplTest::OnStatisticsUpdate,
+                   base::Unretained(this)),
+        base::Bind(&VideoRendererImplTest::OnMaxTimeUpdate,
                    base::Unretained(this)),
-        ended_event_.GetClosure(),
+        base::Bind(&AssignValue, &ended_cb_run_, true),
         error_event_.GetPipelineStatusCB(),
-        base::Bind(&VideoRendererImplTest::GetTime, base::Unretained(this)),
-        base::Bind(&VideoRendererImplTest::GetDuration,
-                   base::Unretained(this)));
+        base::Bind(&VideoRendererImplTest::time, base::Unretained(this)),
+        base::Bind(&VideoRendererImplTest::duration, base::Unretained(this)));
   }
 
   void Play() {
@@ -221,26 +200,6 @@ class VideoRendererImplTest : public ::testing::Test {
     error_event_.RunAndWaitForStatus(expected);
   }
 
-  void WaitForEnded() {
-    SCOPED_TRACE("WaitForEnded()");
-    ended_event_.RunAndWait();
-  }
-
-  void WaitForPendingRead() {
-    SCOPED_TRACE("WaitForPendingRead()");
-    if (!read_cb_.is_null())
-      return;
-
-    DCHECK(wait_for_pending_read_cb_.is_null());
-
-    WaitableMessageLoopEvent event;
-    wait_for_pending_read_cb_ = event.GetClosure();
-    event.RunAndWait();
-
-    DCHECK(!read_cb_.is_null());
-    DCHECK(wait_for_pending_read_cb_.is_null());
-  }
-
   void SatisfyPendingRead() {
     CHECK(!read_cb_.is_null());
     CHECK(!decode_results_.empty());
@@ -255,47 +214,35 @@ class VideoRendererImplTest : public ::testing::Test {
     message_loop_.PostTask(FROM_HERE, closure);
   }
 
-  void AdvanceTimeInMs(int time_ms) {
-    DCHECK_EQ(&message_loop_, base::MessageLoop::current());
-    base::AutoLock l(lock_);
-    time_ += base::TimeDelta::FromMilliseconds(time_ms);
-    DCHECK_LE(time_.InMicroseconds(), GetDuration().InMicroseconds());
+  bool pending_read() { return !read_cb_.is_null(); }
+  base::TimeDelta time() { return time_; }
+  void set_time(base::TimeDelta time) { time_ = time; }
+  base::TimeDelta max_time() { return max_time_; }
+  int decoded_frames() { return decoded_frames_; }
+  int dropped_frames() { return dropped_frames_; }
+  bool ended_cb_run() { return ended_cb_run_; }

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

nit: make getters const?

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

Done.

+
+  base::TimeDelta duration() {
+    // Arbitrary value. Has to be larger to cover any timestamp used in tests.

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

nit: s/larger/large/?

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

Done.

+    return base::TimeDelta::FromMilliseconds(1000);
   }
 
  protected:
-  // Fixture members.
+  base::MessageLoop message_loop_;
+
   scoped_ptr<VideoRendererImpl> renderer_;
   MockVideoDecoder* decoder_;  // Owned by |renderer_|.
+  TestVideoFrameScheduler* scheduler_;     // Owned by |renderer_|.
+  base::SimpleTestTickClock* tick_clock_;  // Owned by |renderer_|.

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

nit: if you align the comments, why not also align the comment on l.234?

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

Done.

(I think clang-format did this)

   NiceMock<MockDemuxerStream> demuxer_stream_;
-  MockStatisticsCB statistics_cb_object_;
-
-  // Use StrictMock<T> to catch missing/extra display callbacks.
-  class MockDisplayCB {
-   public:
-    MOCK_METHOD1(Display, void(const scoped_refptr<VideoFrame>&));
-  };
-  StrictMock<MockDisplayCB> mock_display_cb_;
 
  private:
-  base::TimeDelta GetTime() {
-    base::AutoLock l(lock_);
-    return time_;
-  }
-
-  base::TimeDelta GetDuration() {
-    return base::TimeDelta::FromMilliseconds(kVideoDurationInMs);
-  }
-
   void FrameRequested(const scoped_refptr<DecoderBuffer>& buffer,
                       const VideoDecoder::DecodeCB& read_cb) {
     DCHECK_EQ(&message_loop_, base::MessageLoop::current());
     CHECK(read_cb_.is_null());
     read_cb_ = read_cb;
 
-    // Wake up WaitForPendingRead() if needed.
-    if (!wait_for_pending_read_cb_.is_null())
-      base::ResetAndReturn(&wait_for_pending_read_cb_).Run();
-
     if (decode_results_.empty())
       return;
 
@@ -322,25 +269,28 @@ class VideoRendererImplTest : public ::testing::Test {
     }
   }
 
-  base::MessageLoop message_loop_;
+  void OnMaxTimeUpdate(base::TimeDelta max_time) { max_time_ = max_time; }
 
-  // Used to protect |time_|.
-  base::Lock lock_;
-  base::TimeDelta time_;
+  void OnStatisticsUpdate(const PipelineStatistics& stats) {
+    decoded_frames_ = stats.video_frames_decoded;
+    dropped_frames_ = stats.video_frames_dropped;
+  }
 
   // Used for satisfying reads.
   VideoDecoder::DecodeCB read_cb_;
   base::TimeDelta next_frame_timestamp_;
 
   WaitableMessageLoopEvent error_event_;
-  WaitableMessageLoopEvent ended_event_;
-
-  // Run during FrameRequested() to unblock WaitForPendingRead().
-  base::Closure wait_for_pending_read_cb_;
 
   std::deque<std::pair<
       VideoDecoder::Status, scoped_refptr<VideoFrame> > > decode_results_;
 
+  base::TimeDelta time_;
+  base::TimeDelta max_time_;
+  int decoded_frames_;
+  int dropped_frames_;
+  bool ended_cb_run_;
+
   DISALLOW_COPY_AND_ASSIGN(VideoRendererImplTest);
 };
 
@@ -358,14 +308,6 @@ TEST_F(VideoRendererImplTest, Initialize) {
   Shutdown();
 }
 
-TEST_F(VideoRendererImplTest, InitializeAndPreroll) {
-  Initialize();
-  QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
-  Preroll(0, PIPELINE_OK);
-  Shutdown();
-}
-
 static void ExpectNotCalled(PipelineStatus) {
   base::debug::StackTrace stack;
   ADD_FAILURE() << "Expected callback not to be called\n" << stack.ToString();
@@ -389,31 +331,246 @@ TEST_F(VideoRendererImplTest, StopWhileFlushing) {
   // ~VideoRendererImpl() will CHECK() if we left anything initialized.
 }
 
-TEST_F(VideoRendererImplTest, Play) {
+TEST_F(VideoRendererImplTest, PrerollSchedulesFirstFrame) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+
+  EXPECT_EQ(1u, scheduler_->scheduled_frames().size());
+  EXPECT_EQ(base::TimeDelta(),
+            scheduler_->scheduled_frames().front().frame->timestamp());
+  EXPECT_EQ(base::TimeTicks(),

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

Should base::TimeTicks() be tick_clock_->NowTicks()?

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

Replaced with string format.

+            scheduler_->scheduled_frames().front().wall_ticks);
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, PlaySchedulesAllFrames) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
+
+  EXPECT_EQ(4u, scheduler_->scheduled_frames().size());
+  EXPECT_EQ(base::TimeDelta(),
+            scheduler_->scheduled_frames()[0].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks);

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

String format verification for these too? Either that or a helper function to
make this less cluttered looking.

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

ditto about tick_clock_->NowTicks(). Agreed with acolwell that probably we
should have a function to use string format to verify these. In that function,
we could assert that tick_clock_->NowTicks() is always 0, so the scheduled time
should just be the timestamp of the frames.

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

Done.

+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(10),
+            scheduler_->scheduled_frames()[1].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(10000),
+            scheduler_->scheduled_frames()[1].wall_ticks);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(20),
+            scheduler_->scheduled_frames()[2].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(20000),
+            scheduler_->scheduled_frames()[2].wall_ticks);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(30),
+            scheduler_->scheduled_frames()[3].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(30000),
+            scheduler_->scheduled_frames()[3].wall_ticks);
+
   Shutdown();
 }
 
-TEST_F(VideoRendererImplTest, EndOfStream_ClipDuration) {
+TEST_F(VideoRendererImplTest, PauseResetsScheduledFrames) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
+  Pause();
 
-  // Next frame has timestamp way past duration. Its timestamp will be adjusted
-  // to match the duration of the video.
-  QueueFrames(base::IntToString(kVideoDurationInMs + 1000));
+  EXPECT_EQ(0u, scheduler_->scheduled_frames().size());

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

Also check that "RESET" frames are pushed back to unscheduled_frames_?

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

Isn't needed due to change in Reset() behaviour

+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, PauseWaitsForResetFrames) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+  Play();
+
+  scheduler_->set_should_reset(false);

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

This is for simulating the proxy VFS case right? Any reason not to simply use
that class for this test so it is clearer why the pause would get delayed?

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

Removed this test case due to change in Reset() behaviour

+
+  bool pause_cb_run = false;
+  renderer_->Pause(base::Bind(&AssignValue, &pause_cb_run, true));
+  EXPECT_FALSE(pause_cb_run);
+
+  scheduler_->set_should_reset(true);
+  scheduler_->Reset();

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

See comments above about calling scheduler_->Reset() twice. (Pause() calls
scheduler_->Reset()).

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

Ditto

+  EXPECT_TRUE(pause_cb_run);
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, SetPlaybackRateReschedulesFrames) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+  Play();
+
+  renderer_->SetPlaybackRate(2.0);
+

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

hmm, Reset() and SetPlaybackRate() all finishes synchronously. I am a little
worried about reentrancy but if it works well than I am in favor of it!

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

actually this test started failing once I made Reset() post tasks ... which led
me to re-think what Reset() should do!

+  // Wall times should be cut in half.
+  EXPECT_EQ(4u, scheduler_->scheduled_frames().size());

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

ditto. more concise expectations would be nice.

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

Done.

+  EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks);

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

So even the first frame is rescheduled because Reset() finishes synchronously,
but the timers fires asynchronously? It might be nice to note this somewhere.

Also, we can test the case where run the message loop first, then reschedule. I
guess the first frame will be displayed and not rescheduled?

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

had to change this stuff

+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(10),
+            scheduler_->scheduled_frames()[1].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(5000),
+            scheduler_->scheduled_frames()[1].wall_ticks);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(20),
+            scheduler_->scheduled_frames()[2].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(10000),
+            scheduler_->scheduled_frames()[2].wall_ticks);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(30),
+            scheduler_->scheduled_frames()[3].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(15000),
+            scheduler_->scheduled_frames()[3].wall_ticks);
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, MediaTimeUsedToScheduleFrames) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+
+  // Introduce an offset in the media time before playing.
+  set_time(base::TimeDelta::FromMilliseconds(20));
+
+  Play();
+
+  // All newly scheduled frames should respect the media time.
+  EXPECT_EQ(4u, scheduler_->scheduled_frames().size());
+  EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[0].wall_ticks);

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

So this is from the ScheduleFirstFrameForImmediateDisplay() call? Worth a
comment.

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

Done.

+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(10),
+            scheduler_->scheduled_frames()[1].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(-10000),
+            scheduler_->scheduled_frames()[1].wall_ticks);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(20),
+            scheduler_->scheduled_frames()[2].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks(), scheduler_->scheduled_frames()[2].wall_ticks);

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

This make it harder to read, I'd rather see base::TimeDelta::FromMilliseconds(0)
:) But a function to check all these would be nice.

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

Replaced with string format.

+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(30),
+            scheduler_->scheduled_frames()[3].frame->timestamp());
+  EXPECT_EQ(base::TimeTicks::FromInternalValue(10000),
+            scheduler_->scheduled_frames()[3].wall_ticks);
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, EndedWaitsForLastFrame) {
+  Initialize();
+  QueueFrames("0 10 20 30 eos");
+  Preroll(0, PIPELINE_OK);
+  Play();
+
+  // Display up to last frame.
+  scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(20000));

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

Write a helper function DisplayFrames(ms) so that we don't need to do the 20000
-> 20 conversion?

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

Done.

+  message_loop_.RunUntilIdle();
+  EXPECT_FALSE(ended_cb_run());
+
+  // Display last frame.
+  scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(30000));
+  message_loop_.RunUntilIdle();
+  EXPECT_TRUE(ended_cb_run());
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, EndedWaitsForEndOfStream) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+  Play();
+
+  // Display last frame.
+  scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(30000));
+  message_loop_.RunUntilIdle();
+  EXPECT_FALSE(ended_cb_run());
+
+  // Deliver end of stream.
+  QueueFrames("eos");
+  SatisfyPendingRead();
+  message_loop_.RunUntilIdle();
+  EXPECT_TRUE(ended_cb_run());
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, AdjustTimestampsPastDuration) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+  Play();
 
-  // Queue the end of stream frame and wait for the last frame to be rendered.
+  // Last frame has timestamp way past duration. Its timestamp will be adjusted
+  // to match the duration of the video.
+  QueueFrames(base::IntToString((duration() * 10).InMilliseconds()));
   QueueFrames("eos");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(kVideoDurationInMs)));
-  AdvanceTimeInMs(kVideoDurationInMs);
-  WaitForEnded();
+
+  scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(30000));
+  message_loop_.RunUntilIdle();
+
+  EXPECT_EQ(1u, scheduler_->scheduled_frames().size());
+  EXPECT_EQ(duration(),
+            scheduler_->scheduled_frames().front().frame->timestamp());
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, IncomingFramesUpdateMaxTime) {
+  EXPECT_EQ(kNoTimestamp(), max_time());
+
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(30), max_time());
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, EndOfStreamSetsMaxTimeToDuration) {
+  EXPECT_EQ(kNoTimestamp(), max_time());
+
+  Initialize();
+  QueueFrames("0 10 20 eos");
+  Preroll(0, PIPELINE_OK);
+
+  EXPECT_EQ(duration(), max_time());
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, DisplayedFramesIncrementStatistics) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+  Play();
+
+  EXPECT_EQ(0, decoded_frames());
+  EXPECT_EQ(0, dropped_frames());
+
+  scheduler_->DisplayFrames(base::TimeTicks());
+
+  EXPECT_EQ(1, decoded_frames());
+  EXPECT_EQ(0, dropped_frames());
+
+  Shutdown();
+}
+
+TEST_F(VideoRendererImplTest, DroppedFramesIncrementStatistics) {
+  Initialize();
+  QueueFrames("0 10 20 30");
+  Preroll(0, PIPELINE_OK);
+  Play();
+
+  EXPECT_EQ(0, decoded_frames());
+  EXPECT_EQ(0, dropped_frames());
+
+  scheduler_->DropFrames(base::TimeTicks());
+
+  EXPECT_EQ(1, decoded_frames());
+  EXPECT_EQ(1, dropped_frames());
 
   Shutdown();
 }
@@ -421,13 +578,14 @@ TEST_F(VideoRendererImplTest, EndOfStream_ClipDuration) {
 TEST_F(VideoRendererImplTest, DecodeError_Playing) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
 
+  scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(30000));
+  EXPECT_TRUE(pending_read());
+
   QueueFrames("error");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(10)));
-  AdvanceTimeInMs(10);
+  SatisfyPendingRead();
   WaitForError(PIPELINE_ERROR_DECODE);
   Shutdown();
 }
@@ -442,54 +600,51 @@ TEST_F(VideoRendererImplTest, DecodeError_DuringPreroll) {
 TEST_F(VideoRendererImplTest, Preroll_Exact) {
   Initialize();
   QueueFrames("50 60 70 80 90");
-
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(60)));
   Preroll(60, PIPELINE_OK);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(60),
+            scheduler_->scheduled_frames().front().frame->timestamp());
   Shutdown();
 }
 
 TEST_F(VideoRendererImplTest, Preroll_RightBefore) {
   Initialize();
   QueueFrames("50 60 70 80 90");
-
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(50)));
   Preroll(59, PIPELINE_OK);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(50),
+            scheduler_->scheduled_frames().front().frame->timestamp());
   Shutdown();
 }
 
 TEST_F(VideoRendererImplTest, Preroll_RightAfter) {
   Initialize();
   QueueFrames("50 60 70 80 90");
-
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(60)));
   Preroll(61, PIPELINE_OK);
+  EXPECT_EQ(base::TimeDelta::FromMilliseconds(60),
+            scheduler_->scheduled_frames().front().frame->timestamp());
   Shutdown();
 }
 
 TEST_F(VideoRendererImplTest, PlayAfterPreroll) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
 
-  // Advance time past prerolled time to trigger a Read().
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(10)));
-  AdvanceTimeInMs(10);
-  WaitForPendingRead();
+  scheduler_->DisplayFrames(base::TimeTicks());
+  EXPECT_TRUE(pending_read());
+
   Shutdown();
 }
 
 TEST_F(VideoRendererImplTest, Rebuffer) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
 
-  // Advance time past prerolled time drain the ready frame queue.
-  AdvanceTimeInMs(50);
-  WaitForPendingRead();
+  // Display all frames to empty frame buffer.
+  scheduler_->DisplayFrames(base::TimeTicks::FromInternalValue(30000));
+  EXPECT_TRUE(pending_read());
 
   // Simulate a Pause/Preroll/Play rebuffer sequence.
   Pause();
@@ -501,12 +656,11 @@ TEST_F(VideoRendererImplTest, Rebuffer) {
   // Queue enough frames to satisfy preroll.
   QueueFrames("40 50 60 70");
   SatisfyPendingRead();
+  event.RunAndWaitForStatus(PIPELINE_OK);
 
   // TODO(scherkus): We shouldn't display the next ready frame in a rebuffer
   // situation, see http://crbug.com/365516
-  EXPECT_CALL(mock_display_cb_, Display(_));
-
-  event.RunAndWaitForStatus(PIPELINE_OK);
+  EXPECT_EQ(1u, scheduler_->scheduled_frames().size());
 
   Play();
 
@@ -516,27 +670,21 @@ TEST_F(VideoRendererImplTest, Rebuffer) {
 TEST_F(VideoRendererImplTest, Rebuffer_AlreadyHaveEnoughFrames) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
-
-  // Queue an extra frame so that we'll have enough frames to satisfy
-  // preroll even after the first frame is painted.
-  QueueFrames("40");
   Play();
 
   // Simulate a Pause/Preroll/Play rebuffer sequence.
   Pause();
 
-  // TODO(scherkus): We shouldn't display the next ready frame in a rebuffer
-  // situation, see http://crbug.com/365516
-  EXPECT_CALL(mock_display_cb_, Display(_));
-
   WaitableMessageLoopEvent event;
   renderer_->Preroll(kNoTimestamp(),
                      event.GetPipelineStatusCB());
-
   event.RunAndWaitForStatus(PIPELINE_OK);
 
+  // TODO(scherkus): We shouldn't display the next ready frame in a rebuffer
+  // situation, see http://crbug.com/365516
+  EXPECT_EQ(1u, scheduler_->scheduled_frames().size());
+
   Play();
 
   Shutdown();
@@ -546,14 +694,11 @@ TEST_F(VideoRendererImplTest, Rebuffer_AlreadyHaveEnoughFrames) {
 TEST_F(VideoRendererImplTest, StopDuringOutstandingRead) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
 
-  // Advance time a bit to trigger a Read().
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(10)));
-  AdvanceTimeInMs(10);
-  WaitForPendingRead();
+  scheduler_->DisplayFrames(base::TimeTicks());
+  EXPECT_TRUE(pending_read());
 
   WaitableMessageLoopEvent event;
   renderer_->Stop(event.GetClosure());
@@ -563,21 +708,17 @@ TEST_F(VideoRendererImplTest, StopDuringOutstandingRead) {
 TEST_F(VideoRendererImplTest, AbortPendingRead_Playing) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
 
-  // Advance time a bit to trigger a Read().
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(10)));
-  AdvanceTimeInMs(10);
-  WaitForPendingRead();
+  scheduler_->DisplayFrames(base::TimeTicks());
+  EXPECT_TRUE(pending_read());
   QueueFrames("abort");
   SatisfyPendingRead();
 
   Pause();
   Flush();
   QueueFrames("60 70 80 90");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(60)));
   Preroll(60, PIPELINE_OK);
   Shutdown();
 }
@@ -585,14 +726,11 @@ TEST_F(VideoRendererImplTest, AbortPendingRead_Playing) {
 TEST_F(VideoRendererImplTest, AbortPendingRead_Flush) {
   Initialize();
   QueueFrames("0 10 20 30");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Play();
 
-  // Advance time a bit to trigger a Read().
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(10)));
-  AdvanceTimeInMs(10);
-  WaitForPendingRead();
+  scheduler_->DisplayFrames(base::TimeTicks());
+  EXPECT_TRUE(pending_read());
 
   Pause();
   Flush();
@@ -602,14 +740,11 @@ TEST_F(VideoRendererImplTest, AbortPendingRead_Flush) {
 TEST_F(VideoRendererImplTest, AbortPendingRead_Preroll) {
   Initialize();
   QueueFrames("0 10 abort");
-  EXPECT_CALL(mock_display_cb_, Display(HasTimestamp(0)));
   Preroll(0, PIPELINE_OK);
   Shutdown();
 }
 
 TEST_F(VideoRendererImplTest, VideoDecoder_InitFailure) {
-  InSequence s;
-
   EXPECT_CALL(*decoder_, Initialize(_, _))
       .WillOnce(RunCallback<1>(DECODER_ERROR_NOT_SUPPORTED));
   InitializeRenderer(DECODER_ERROR_NOT_SUPPORTED);