[remoting host] Add unittest for capturing at 30FPS.

This adds a unittest for the bug fixed in
http://crrev.com/65f6394de68d8e96184234f5b7c8bf1e80f699f1

This updates the scheduler and unittests to use a mock clock
implementation from TestMockTimeTaskRunner, instead of just
mocking the current time directly.

Bug: 816727
Change-Id: I194af8e65bae70f0af3a202e0bbb7653059abcaf
Reviewed-on: https://chromium-review.googlesource.com/947575
Commit-Queue: Lambros Lambrou <lambroslambrou@chromium.org>
Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542666}

remoting/protocol/webrtc_frame_scheduler_simple.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/time/default_tick_clock.h"
 #include "remoting/base/constants.h"
 #include "remoting/protocol/frame_stats.h"
 #include "remoting/protocol/webrtc_bandwidth_estimator.h"
@@ -62,7 +63,8 @@ int64_t GetRegionArea(const webrtc::DesktopRegion& region) {
 // TODO(zijiehe): Use |options| to select bandwidth estimator.
 WebrtcFrameSchedulerSimple::WebrtcFrameSchedulerSimple(
     const SessionOptions& options)
-    : pacing_bucket_(LeakyBucket::kUnlimitedDepth, 0),
+    : tick_clock_(base::DefaultTickClock::GetInstance()),
+      pacing_bucket_(LeakyBucket::kUnlimitedDepth, 0),
       updated_region_area_(kStatsWindow),
       bandwidth_estimator_(new WebrtcBandwidthEstimator()),
       weak_factory_(this) {}
@@ -92,8 +94,8 @@ void WebrtcFrameSchedulerSimple::OnTargetBitrateChanged(int bandwidth_kbps) {
   bandwidth_estimator_->OnBitrateEstimation(bandwidth_kbps);
   processing_time_estimator_.SetBandwidthKbps(
       bandwidth_estimator_->GetBitrateKbps());
-  pacing_bucket_.UpdateRate(
-      bandwidth_estimator_->GetBitrateKbps() * 1000 / 8, Now());
+  pacing_bucket_.UpdateRate(bandwidth_estimator_->GetBitrateKbps() * 1000 / 8,
+                            tick_clock_->NowTicks());
   ScheduleNextFrame();
 }
 
@@ -122,7 +124,7 @@ bool WebrtcFrameSchedulerSimple::OnFrameCaptured(
     WebrtcVideoEncoder::FrameParams* params_out) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  base::TimeTicks now = Now();
+  base::TimeTicks now = tick_clock_->NowTicks();
 
   // Null |frame| indicates a capturer error.
   if (!frame) {
@@ -204,7 +206,7 @@ void WebrtcFrameSchedulerSimple::OnFrameEncoded(
   DCHECK(frame_pending_);
   frame_pending_ = false;
 
-  base::TimeTicks now = Now();
+  base::TimeTicks now = tick_clock_->NowTicks();
 
   if (frame_stats) {
     // Calculate |send_pending_delay| before refilling |pacing_bucket_|.
@@ -237,13 +239,14 @@ void WebrtcFrameSchedulerSimple::OnFrameEncoded(
   bandwidth_estimator_->OnSendingFrame(*encoded_frame);
 }
 
-void WebrtcFrameSchedulerSimple::SetCurrentTimeForTest(base::TimeTicks now) {
-  fake_now_for_test_ = now;
+void WebrtcFrameSchedulerSimple::SetTickClockForTest(
+    base::TickClock* tick_clock) {
+  tick_clock_ = tick_clock;
 }
 
 void WebrtcFrameSchedulerSimple::ScheduleNextFrame() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  base::TimeTicks now = Now();
+  base::TimeTicks now = tick_clock_->NowTicks();
 
   if (!encoder_ready_ || paused_ || pacing_bucket_.rate() == 0 ||
       capture_callback_.is_null() || frame_pending_) {
@@ -274,16 +277,11 @@ void WebrtcFrameSchedulerSimple::ScheduleNextFrame() {
 void WebrtcFrameSchedulerSimple::CaptureNextFrame() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!frame_pending_);
-  last_capture_started_time_ = Now();
+  last_capture_started_time_ = tick_clock_->NowTicks();
   processing_time_estimator_.StartFrame();
   frame_pending_ = true;
   capture_callback_.Run();
 }
 
-base::TimeTicks WebrtcFrameSchedulerSimple::Now() {
-  return fake_now_for_test_.is_null() ? base::TimeTicks::Now()
-                                      : fake_now_for_test_;
-}
-
 }  // namespace protocol
 }  // namespace remoting

remoting/protocol/webrtc_frame_scheduler_simple.h

@@ -11,6 +11,7 @@
 
 #include "base/containers/queue.h"
 #include "base/threading/thread_checker.h"
+#include "base/time/tick_clock.h"
 #include "base/timer/timer.h"
 #include "remoting/base/leaky_bucket.h"
 #include "remoting/base/running_samples.h"
@@ -47,19 +48,16 @@ class WebrtcFrameSchedulerSimple : public VideoChannelStateObserver,
   void OnFrameEncoded(const WebrtcVideoEncoder::EncodedFrame* encoded_frame,
                       HostFrameStats* frame_stats) override;
 
-  // Allows unit-tests to fake the current time.
-  void SetCurrentTimeForTest(base::TimeTicks now);
+  // Allows unit-tests to provide a mock clock.
+  void SetTickClockForTest(base::TickClock* tick_clock);
 
  private:
   void ScheduleNextFrame();
   void CaptureNextFrame();
 
-  // Returns the current time according to base::TimeTicks::Now(),
-  // or a fake time provided by a unit-test.
-  base::TimeTicks Now();
-
-  // Non-null if a fake current time is set by unit-test.
-  base::TimeTicks fake_now_for_test_;
+  // A TimeTicks provider which defaults to using a real system clock, but can
+  // be replaced for unittests.
+  base::TickClock* tick_clock_;
 
   base::Closure capture_callback_;
   bool paused_ = false;

remoting/protocol/webrtc_frame_scheduler_unittest.cc

@@ -4,7 +4,7 @@
 
 #include "remoting/protocol/webrtc_frame_scheduler.h"
 
-#include "base/test/test_simple_task_runner.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "remoting/base/session_options.h"
 #include "remoting/protocol/webrtc_dummy_video_encoder.h"
@@ -22,35 +22,46 @@ namespace protocol {
 class WebrtcFrameSchedulerTest : public ::testing::Test {
  public:
   WebrtcFrameSchedulerTest()
-      : task_runner_(new base::TestSimpleTaskRunner()),
+      : task_runner_(
+            // Default ctor starts clock with null TimeTicks, which confuses
+            // the scheduler, so use the current time as a baseline.
+            new base::TestMockTimeTaskRunner(base::Time::Now(),
+                                             base::TimeTicks::Now())),
         task_runner_handle_(task_runner_.get()),
-        now_(base::TimeTicks::Now()) {
+        frame_(DesktopSize(1, 1)) {
     video_encoder_factory_.reset(new WebrtcDummyVideoEncoderFactory());
     scheduler_.reset(new WebrtcFrameSchedulerSimple(SessionOptions()));
-    scheduler_->SetCurrentTimeForTest(now_);
+    scheduler_->SetTickClockForTest(task_runner_->GetMockTickClock());
     scheduler_->Start(video_encoder_factory_.get(),
                       base::Bind(&WebrtcFrameSchedulerTest::CaptureCallback,
                                  base::Unretained(this)));
   }
   ~WebrtcFrameSchedulerTest() override = default;
 
-  void CaptureCallback() { capture_callback_called_ = true; }
-
-  void VerifyCaptureCallbackCalled() {
-    EXPECT_TRUE(capture_callback_called_);
-    capture_callback_called_ = false;
+  void CaptureCallback() {
+    capture_callback_count_++;
+
+    if (simulate_capture_) {
+      // Simulate a completed capture and encode.
+      WebrtcVideoEncoder::FrameParams out_params;
+      scheduler_->OnFrameCaptured(&frame_, &out_params);
+      WebrtcVideoEncoder::EncodedFrame encoded;
+      encoded.key_frame = out_params.key_frame;
+      encoded.data = 'X';
+      scheduler_->OnFrameEncoded(&encoded, nullptr);
+    }
   }
 
  protected:
-  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
 
   std::unique_ptr<WebrtcDummyVideoEncoderFactory> video_encoder_factory_;
   std::unique_ptr<WebrtcFrameSchedulerSimple> scheduler_;
 
-  bool capture_callback_called_ = false;
-
-  base::TimeTicks now_;
+  int capture_callback_count_ = 0;
+  bool simulate_capture_ = false;
+  BasicDesktopFrame frame_;
 };
 
 TEST_F(WebrtcFrameSchedulerTest, UpdateBitrateWhenPending) {
@@ -61,9 +72,9 @@ TEST_F(WebrtcFrameSchedulerTest, UpdateBitrateWhenPending) {
   video_channel_observer->OnTargetBitrateChanged(100);
 
   EXPECT_TRUE(task_runner_->HasPendingTask());
-  task_runner_->RunPendingTasks();
+  task_runner_->FastForwardUntilNoTasksRemain();
 
-  VerifyCaptureCallbackCalled();
+  EXPECT_EQ(1, capture_callback_count_);
 
   video_channel_observer->OnTargetBitrateChanged(1001);
 
@@ -79,13 +90,13 @@ TEST_F(WebrtcFrameSchedulerTest, EmptyFrameUpdate_ShouldNotBeSentImmediately) {
   video_channel_observer->OnTargetBitrateChanged(100);
 
   WebrtcVideoEncoder::FrameParams out_params;
-  BasicDesktopFrame frame(DesktopSize(1, 1));
+
   // Initial capture, full frame.
-  frame.mutable_updated_region()->SetRect(DesktopRect::MakeWH(1, 1));
-  scheduler_->OnFrameCaptured(&frame, &out_params);
+  frame_.mutable_updated_region()->SetRect(DesktopRect::MakeWH(1, 1));
+  scheduler_->OnFrameCaptured(&frame_, &out_params);
   // Empty frame.
-  frame.mutable_updated_region()->Clear();
-  bool result = scheduler_->OnFrameCaptured(&frame, &out_params);
+  frame_.mutable_updated_region()->Clear();
+  bool result = scheduler_->OnFrameCaptured(&frame_, &out_params);
 
   // Should not be sent, because of throttling of empty frames.
   EXPECT_FALSE(result);
@@ -99,22 +110,40 @@ TEST_F(WebrtcFrameSchedulerTest, EmptyFrameUpdate_ShouldBeSentAfter2000ms) {
   video_channel_observer->OnTargetBitrateChanged(100);
 
   WebrtcVideoEncoder::FrameParams out_params;
-  BasicDesktopFrame frame(DesktopSize(1, 1));
+
   // Initial capture, full frame.
-  frame.mutable_updated_region()->SetRect(DesktopRect::MakeWH(1, 1));
-  scheduler_->OnFrameCaptured(&frame, &out_params);
+  frame_.mutable_updated_region()->SetRect(DesktopRect::MakeWH(1, 1));
+  scheduler_->OnFrameCaptured(&frame_, &out_params);
   // Wait more than 2000ms.
-  scheduler_->SetCurrentTimeForTest(now_ +
-                                    base::TimeDelta::FromMilliseconds(3000));
+  task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(3000));
   // Empty frame.
-  frame.mutable_updated_region()->Clear();
-  bool result = scheduler_->OnFrameCaptured(&frame, &out_params);
+  frame_.mutable_updated_region()->Clear();
+  bool result = scheduler_->OnFrameCaptured(&frame_, &out_params);
 
   // Empty frames should be sent at the throttled rate.
   EXPECT_TRUE(result);
 };
 
-// TODO(sergeyu): Add more unittests.
+TEST_F(WebrtcFrameSchedulerTest, Capturer_RunsAt30Fps) {
+  simulate_capture_ = true;
+
+  auto video_channel_observer =
+      video_encoder_factory_->get_video_channel_state_observer_for_tests();
+  video_channel_observer->OnTargetBitrateChanged(100);
+
+  // Have the capturer return non-empty frames each time.
+  frame_.mutable_updated_region()->SetRect(DesktopRect::MakeWH(1, 1));
+
+  // Ensure the encoder is ready, otherwise the scheduler will not trigger
+  // repeated captures.
+  video_channel_observer->OnKeyFrameRequested();
+
+  task_runner_->FastForwardBy(base::TimeDelta::FromSeconds(1));
+
+  // There should be approximately 30 captures in 1 second.
+  EXPECT_LE(29, capture_callback_count_);
+  EXPECT_LE(capture_callback_count_, 31);
+}
 
 }  // namespace protocol
 }  // namespace remoting