Refactor VideoRendererImpl to use VideoFrameScheduler.

media/filters/test_video_frame_scheduler.cc

+// Copyright 2014 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/test_video_frame_scheduler.h"
+
+#include "media/base/video_frame.h"
+
+namespace media {
+
+TestVideoFrameScheduler::ScheduledFrame::ScheduledFrame(
+    const scoped_refptr<VideoFrame> frame,
+    base::TimeTicks wall_ticks,
+    const DoneCB& done_cb)
+    : frame(frame), wall_ticks(wall_ticks), done_cb(done_cb) {
+}
+
+TestVideoFrameScheduler::ScheduledFrame::~ScheduledFrame() {
+}
+
+TestVideoFrameScheduler::TestVideoFrameScheduler() : should_reset_(true) {
+}
+
+TestVideoFrameScheduler::~TestVideoFrameScheduler() {
+}
+
+void TestVideoFrameScheduler::ScheduleVideoFrame(
+    const scoped_refptr<VideoFrame>& frame,
+    base::TimeTicks wall_ticks,
+    const DoneCB& done_cb) {
+  scheduled_frames_.push_back(ScheduledFrame(frame, wall_ticks, done_cb));
+}
+
+void TestVideoFrameScheduler::Reset() {
+  if (!should_reset_)

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

Why? It seems like this could result in a violation of the VideoFrameScheduler
interface contract.

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

No longer applicable!

+    return;

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

The API says Reset() should "Causes the scheduler to release all previously
scheduled frames. Frames will be returned as RESET."

According to this API, caller should only need to call Reset() once to reset the
frame scheduler. Therefore, I think the following call sequence violates this
API:

set_should_reset(false);
Reset();
set_should_reset(false);
Reset();

Instead, we could probably do something like:

HoldReset();
Reset(); // Reset will be pending.
SatisfyReset();

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

Ditto -- Reset() has been reworked

+
+  std::vector<ScheduledFrame> frames_to_reset;
+  frames_to_reset.swap(scheduled_frames_);
+
+  for (size_t i = 0; i < frames_to_reset.size(); ++i) {
+    frames_to_reset[i].done_cb.Run(frames_to_reset[i].frame, RESET);
+  }

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

Can we just use:

RunDoneCBForFrames(TimeDelta::Max(), RESET);

?

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

not anymore now that we have a contract for reentrancy

+}
+
+void TestVideoFrameScheduler::DisplayFrames(base::TimeTicks wall_ticks) {
+  RunDoneCBForFrames(wall_ticks, DISPLAYED);
+}
+
+void TestVideoFrameScheduler::DropFrames(base::TimeTicks wall_ticks) {
+  RunDoneCBForFrames(wall_ticks, DROPPED);
+}
+
+void TestVideoFrameScheduler::RunDoneCBForFrames(base::TimeTicks wall_ticks,
+                                                 Reason reason) {
+  std::vector<ScheduledFrame> done_frames;
+  std::vector<ScheduledFrame> remaining_frames;
+
+  for (size_t i = 0; i < scheduled_frames_.size(); ++i) {
+    if (scheduled_frames_[i].wall_ticks <= wall_ticks) {
+      done_frames.push_back(scheduled_frames_[i]);
+    } else {
+      remaining_frames.push_back(scheduled_frames_[i]);

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

You have to do this because you aren't using a priority queue here? Is that just
because you want to be able to verify ScheduleVideoFrame() call order?

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

a scheduler doesn't *have* to use a priority queue -- for this particular
scheduler using a vector makes it easier for verifying the contents for tests

+    }
+  }

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

nit: If the frames are sorted we don't have to go through the whole list.

+
+  scheduled_frames_.swap(remaining_frames);
+
+  for (size_t i = 0; i < done_frames.size(); ++i) {
+    done_frames[i].done_cb.Run(done_frames[i].frame, reason);
+  }
+}
+
+}  // namespace media