Update VideoFrameScheduler API and add clockless and testing implementations.

API changes:
  - Frame done callbacks must not be reentrant
  - Reset is fire-and-forget; frames are no longer return via the done callback

BUG=110814

Review URL: https://codereview.chromium.org/257793004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266635 0039d316-1c4b-4281-b951-d872f2087c98

media/filters/clockless_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/clockless_video_frame_scheduler.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "media/base/video_frame.h"
+
+namespace media {
+
+ClocklessVideoFrameScheduler::ClocklessVideoFrameScheduler(
+    const DisplayCB& display_cb)
+    : display_cb_(display_cb) {
+}
+
+ClocklessVideoFrameScheduler::~ClocklessVideoFrameScheduler() {
+}
+
+void ClocklessVideoFrameScheduler::ScheduleVideoFrame(
+    const scoped_refptr<VideoFrame>& frame,
+    base::TimeTicks /* wall_ticks */,
+    const DoneCB& done_cb) {
+  display_cb_.Run(frame);
+  base::MessageLoopProxy::current()->PostTask(
+      FROM_HERE, base::Bind(done_cb, frame, DISPLAYED));
+}
+
+void ClocklessVideoFrameScheduler::Reset() {
+}
+
+}  // namespace media

media/filters/clockless_video_frame_scheduler.h

+// 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.
+
+#ifndef MEDIA_FILTERS_CLOCKLESS_VIDEO_FRAME_SCHEDULER_H_
+#define MEDIA_FILTERS_CLOCKLESS_VIDEO_FRAME_SCHEDULER_H_
+
+#include "media/filters/video_frame_scheduler.h"
+
+namespace media {
+
+// A scheduler that immediately displays frames.
+class ClocklessVideoFrameScheduler : public VideoFrameScheduler {
+ public:
+  typedef base::Callback<void(const scoped_refptr<VideoFrame>&)> DisplayCB;
+
+  explicit ClocklessVideoFrameScheduler(const DisplayCB& display_cb);
+  virtual ~ClocklessVideoFrameScheduler();
+
+  // VideoFrameScheduler implementation.
+  virtual void ScheduleVideoFrame(const scoped_refptr<VideoFrame>& frame,
+                                  base::TimeTicks wall_ticks,
+                                  const DoneCB& done_cb) OVERRIDE;
+  virtual void Reset() OVERRIDE;
+
+ private:
+  DisplayCB display_cb_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClocklessVideoFrameScheduler);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_FILTERS_CLOCKLESS_VIDEO_FRAME_SCHEDULER_H_

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() {
+}
+
+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() {
+  scheduled_frames_.clear();
+}
+
+void TestVideoFrameScheduler::DisplayFramesUpTo(base::TimeTicks wall_ticks) {
+  RunDoneCBForFramesUpTo(wall_ticks, DISPLAYED);
+}
+
+void TestVideoFrameScheduler::DropFramesUpTo(base::TimeTicks wall_ticks) {
+  RunDoneCBForFramesUpTo(wall_ticks, DROPPED);
+}
+
+void TestVideoFrameScheduler::RunDoneCBForFramesUpTo(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]);
+    }
+  }
+
+  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

media/filters/test_video_frame_scheduler.h

+// 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.
+
+#ifndef MEDIA_FILTERS_TEST_VIDEO_FRAME_SCHEDULER_H_
+#define MEDIA_FILTERS_TEST_VIDEO_FRAME_SCHEDULER_H_
+
+#include <vector>
+
+#include "media/filters/video_frame_scheduler.h"
+
+namespace media {
+
+// A scheduler that queues frames until told otherwise.
+class TestVideoFrameScheduler : public VideoFrameScheduler {
+ public:
+  struct ScheduledFrame {
+    ScheduledFrame(const scoped_refptr<VideoFrame> frame,
+                   base::TimeTicks wall_ticks,
+                   const DoneCB& done_cb);
+    ~ScheduledFrame();
+
+    scoped_refptr<VideoFrame> frame;
+    base::TimeTicks wall_ticks;
+    DoneCB done_cb;
+  };
+
+  TestVideoFrameScheduler();
+  virtual ~TestVideoFrameScheduler();
+
+  // VideoFrameScheduler implementation.
+  virtual void ScheduleVideoFrame(const scoped_refptr<VideoFrame>& frame,
+                                  base::TimeTicks wall_ticks,
+                                  const DoneCB& done_cb) OVERRIDE;
+  virtual void Reset() OVERRIDE;
+
+  // Displays all frames with scheduled times <= |wall_ticks|.
+  void DisplayFramesUpTo(base::TimeTicks wall_ticks);
+
+  // Drops all frames with scheduled times <= |wall_ticks|.
+  void DropFramesUpTo(base::TimeTicks wall_ticks);
+
+  const std::vector<ScheduledFrame>& scheduled_frames() const {
+    return scheduled_frames_;
+  }
+
+ private:
+  void RunDoneCBForFramesUpTo(base::TimeTicks wall_ticks, Reason reason);
+
+  std::vector<ScheduledFrame> scheduled_frames_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestVideoFrameScheduler);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_FILTERS_TEST_VIDEO_FRAME_SCHEDULER_H_

media/filters/video_frame_scheduler.h

@@ -7,6 +7,7 @@
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "base/time/time.h"
 #include "media/base/media_export.h"
 
 namespace media {
@@ -23,18 +24,22 @@ class MEDIA_EXPORT VideoFrameScheduler {
   enum Reason {
     DISPLAYED,  // Frame was displayed.
     DROPPED,    // Frame was dropped.
-    RESET,      // Scheduler was reset before frame was scheduled for display.
   };
   typedef base::Callback<void(const scoped_refptr<VideoFrame>&, Reason)> DoneCB;
 
   // Schedule |frame| to be displayed at |wall_ticks|, firing |done_cb| when
   // the scheduler has finished with the frame.
+  //
+  // To avoid reentrancy issues, |done_cb| is run on a separate calling stack.
   virtual void ScheduleVideoFrame(const scoped_refptr<VideoFrame>& frame,
                                   base::TimeTicks wall_ticks,
                                   const DoneCB& done_cb) = 0;
 
-  // Causes the scheduler to release all previously scheduled frames. Frames
-  // will be returned as RESET.
+  // Causes the scheduler to cancel any previously scheduled frames.
+  //
+  // There is no guarantee that |done_cb|'s for previously scheduled frames
+  // will not be run. Clients should implement callback tracking/cancellation
+  // if they are sensitive to old callbacks being run.
   virtual void Reset() = 0;
 };
 

media/filters/video_frame_scheduler_impl.cc

@@ -34,11 +34,8 @@ void VideoFrameSchedulerImpl::ScheduleVideoFrame(
 }
 
 void VideoFrameSchedulerImpl::Reset() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  while (!pending_frames_.empty()) {
-    pending_frames_.top().done_cb.Run(pending_frames_.top().frame, RESET);
-    pending_frames_.pop();
-  }
+  pending_frames_ = PendingFrameQueue();
+  timer_.Stop();
 }
 
 void VideoFrameSchedulerImpl::SetTickClockForTesting(

media/filters/video_frame_scheduler_impl_unittest.cc

@@ -12,6 +12,8 @@
 
 namespace media {
 
+using testing::_;
+
 // NOTE: millisecond-level resolution is used for times as real delayed tasks
 // are posted. Don't use large values if you want to keep tests running fast.
 class VideoFrameSchedulerImplTest : public testing::Test {
@@ -138,8 +140,8 @@ TEST_F(VideoFrameSchedulerImplTest, Reset) {
       VideoFrame::CreateBlackFrame(gfx::Size(8, 8));
   Schedule(frame, 10);
 
-  // Despite being on time, frames are returned immediately.
-  EXPECT_CALL(*this, OnFrameDone(frame, VideoFrameScheduler::RESET));
+  // Despite being on time, frame callback isn't run.
+  EXPECT_CALL(*this, OnFrameDone(_, _)).Times(0);
   AdvanceTime(10);
   Reset();
   RunUntilTimeHasElapsed(10);

media/filters/video_frame_scheduler_unittest.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 "base/bind.h"
+#include "base/debug/stack_trace.h"
+#include "base/run_loop.h"
+#include "media/base/video_frame.h"
+#include "media/filters/clockless_video_frame_scheduler.h"
+#include "media/filters/test_video_frame_scheduler.h"
+#include "media/filters/video_frame_scheduler_impl.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+
+static void DoNothing(const scoped_refptr<VideoFrame>& frame) {
+}
+
+static void CheckForReentrancy(std::string* stack_trace,
+                               const scoped_refptr<VideoFrame>& frame,
+                               VideoFrameScheduler::Reason reason) {
+  *stack_trace = base::debug::StackTrace().ToString();
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         base::MessageLoop::QuitClosure());
+}
+
+// Type parameterized test harness for validating API contract of
+// VideoFrameScheduler implementations.
+//
+// NOTE: C++ requires using "this" for derived class templates when referencing
+// class members.
+template <typename T>
+class VideoFrameSchedulerTest : public testing::Test {
+ public:
+  VideoFrameSchedulerTest() {}
+  virtual ~VideoFrameSchedulerTest() {}
+
+  base::MessageLoop message_loop_;
+  T scheduler_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(VideoFrameSchedulerTest);
+};
+
+template <>
+VideoFrameSchedulerTest<ClocklessVideoFrameScheduler>::VideoFrameSchedulerTest()
+    : scheduler_(base::Bind(&DoNothing)) {
+}
+
+template <>
+VideoFrameSchedulerTest<VideoFrameSchedulerImpl>::VideoFrameSchedulerTest()
+    : scheduler_(message_loop_.message_loop_proxy(), base::Bind(&DoNothing)) {
+}
+
+TYPED_TEST_CASE_P(VideoFrameSchedulerTest);
+
+TYPED_TEST_P(VideoFrameSchedulerTest, ScheduleVideoFrameIsntReentrant) {
+  scoped_refptr<VideoFrame> frame =
+      VideoFrame::CreateBlackFrame(gfx::Size(8, 8));
+
+  std::string stack_trace;
+  this->scheduler_.ScheduleVideoFrame(
+      frame, base::TimeTicks(), base::Bind(&CheckForReentrancy, &stack_trace));
+  EXPECT_TRUE(stack_trace.empty()) << "Reentracy detected:\n" << stack_trace;
+}
+
+REGISTER_TYPED_TEST_CASE_P(VideoFrameSchedulerTest,
+                           ScheduleVideoFrameIsntReentrant);
+
+INSTANTIATE_TYPED_TEST_CASE_P(ClocklessVideoFrameScheduler,
+                              VideoFrameSchedulerTest,
+                              ClocklessVideoFrameScheduler);
+INSTANTIATE_TYPED_TEST_CASE_P(VideoFrameSchedulerImpl,
+                              VideoFrameSchedulerTest,
+                              VideoFrameSchedulerImpl);
+INSTANTIATE_TYPED_TEST_CASE_P(TestVideoFrameScheduler,
+                              VideoFrameSchedulerTest,
+                              TestVideoFrameScheduler);
+
+}  // namespace media

media/media.gyp

@@ -1043,6 +1043,7 @@
         'filters/source_buffer_stream_unittest.cc',
         'filters/video_decoder_selector_unittest.cc',
         'filters/video_frame_scheduler_impl_unittest.cc',
+        'filters/video_frame_scheduler_unittest.cc',
         'filters/video_frame_stream_unittest.cc',
         'filters/video_renderer_impl_unittest.cc',
         'midi/midi_manager_usb_unittest.cc',
@@ -1263,8 +1264,12 @@
         'base/test_data_util.h',
         'base/test_helpers.cc',
         'base/test_helpers.h',
+        'filters/clockless_video_frame_scheduler.cc',
+        'filters/clockless_video_frame_scheduler.h',
         'filters/mock_gpu_video_accelerator_factories.cc',
         'filters/mock_gpu_video_accelerator_factories.h',
+        'filters/test_video_frame_scheduler.cc',
+        'filters/test_video_frame_scheduler.h',
         'video/mock_video_decode_accelerator.cc',
         'video/mock_video_decode_accelerator.h',
       ],