Adjust the capture interval to account for post task delays

I've noticed that even on high powered machines with the target
framerate set to 30, the max encode FPS sits at ~28.  Looking at the
capture and encode times, the host should have been able encode at 30
FPS but that wasn't occurring. The same happens for our other standard
framerates (the actual encode rate was always 1-2 FPS below the target).

It looks like the issue is that we are scheduling captures at the target
framerate w/o accounting for common post task delays so the result is
that the capture rate is a bit slower than the interval we expect.

This CL adjusts the capture interval by a couple of MS which allows the
host hit the target rate.

Bug: b:217468304
Change-Id: Ib86e2de22e3c324900de845f16235415e1ba83a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4184285
Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1095938}

remoting/protocol/webrtc_frame_scheduler_constant_rate.cc

@@ -7,11 +7,32 @@
 #include <algorithm>
 
 #include "base/logging.h"
+#include "base/system/sys_info.h"
 #include "base/time/time.h"
 
 namespace remoting::protocol {
 
-WebrtcFrameSchedulerConstantRate::WebrtcFrameSchedulerConstantRate() {
+namespace {
+base::TimeDelta GetPostTaskAdjustment() {
+  int proccessor_count = base::SysInfo::NumberOfProcessors();
+  if (proccessor_count < 16) {
+    // Don't change the scheduler timing on these machines as we don't want to
+    // overload the machine.
+    return base::Milliseconds(0);
+  }
+
+  // We've observed the encoding rate in the client as being a couple of frames
+  // lower than the target. By adjusting the capture rate by ~2ms, the host will
+  // generate frames at, or slightly above, the target frame rate. If a value of
+  // 1ms is used, then the host will generate frames at, or slightly below, the
+  // target. They higher of the two was chosen for better performance on high-
+  // CPU machines.
+  return base::Milliseconds(2);
+}
+}  // namespace
+
+WebrtcFrameSchedulerConstantRate::WebrtcFrameSchedulerConstantRate()
+    : post_task_adjustment_(GetPostTaskAdjustment()) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
@@ -48,8 +69,21 @@ void WebrtcFrameSchedulerConstantRate::OnFrameCaptured(
 
 void WebrtcFrameSchedulerConstantRate::SetMaxFramerateFps(int max_framerate) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK_GE(max_framerate, 0);
+
+  max_framerate_fps_ = std::min(max_framerate, 1000);
+  if (max_framerate_fps_ > 0) {
+    // Calculate the interval used to schedule each frame capture and account
+    // for the time used when posting tasks (~1-2ms per frame). We also set a
+    // floor at 1ms as some of our dependencies have problems when frames are
+    // delivered with the same timestamp.
+    capture_interval_ =
+        std::max(base::Hertz(max_framerate_fps_) - post_task_adjustment_,
+                 base::Milliseconds(1));
+  } else {
+    capture_interval_ = {};
+  }
 
-  max_framerate_fps_ = max_framerate;
   ScheduleNextFrame();
 }
 
@@ -99,25 +133,25 @@ void WebrtcFrameSchedulerConstantRate::ScheduleNextFrame() {
     return;
   }
 
-  // Captures should be scheduled at least 1ms apart, otherwise WebRTC's video
-  // stream encoder complains about non-increasing frame timestamps, which can
-  // affect some unittests.
-  base::TimeDelta capture_interval =
-      std::max(base::Seconds(1) / max_framerate_fps_, base::Milliseconds(1));
-
-  // Use the boosted capture interval if we are within |boost_window_|.
-  if (!boost_window_.is_null()) {
-    if (boost_window_ > now) {
-      capture_interval = boost_capture_interval_;
-    } else {
-      boost_window_ = {};
-    }
-  }
-
   base::TimeDelta delay;
   if (!last_capture_started_time_.is_null()) {
+    auto capture_interval = capture_interval_;
+
+    // Use the boosted capture interval if we are within |boost_window_|.
+    if (!boost_window_.is_null()) {
+      if (boost_window_ > now) {
+        capture_interval = boost_capture_interval_;
+      } else {
+        boost_window_ = {};
+      }
+    }
+
     base::TimeTicks target_capture_time =
-        std::max(last_capture_started_time_ + capture_interval, now);
+        last_capture_started_time_ + capture_interval;
+
+    // Captures should be scheduled at least 1ms apart, otherwise WebRTC's video
+    // stream logic will complain about non-increasing frame timestamps, which
+    // can affect some unittests.
     delay = std::max(target_capture_time - now, base::Milliseconds(1));
   }
 
@@ -134,4 +168,10 @@ void WebrtcFrameSchedulerConstantRate::CaptureNextFrame() {
   capture_callback_.Run();
 }
 
+void WebrtcFrameSchedulerConstantRate::SetPostTaskAdjustmentForTest(
+    base::TimeDelta post_task_adjustment) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  post_task_adjustment_ = post_task_adjustment;
+}
+
 }  // namespace remoting::protocol

remoting/protocol/webrtc_frame_scheduler_constant_rate.h

@@ -38,6 +38,8 @@ class WebrtcFrameSchedulerConstantRate : public WebrtcFrameScheduler {
   void BoostCaptureRate(base::TimeDelta capture_interval,
                         base::TimeDelta duration);
 
+  void SetPostTaskAdjustmentForTest(base::TimeDelta post_task_adjustment);
+
  private:
   void ScheduleNextFrame();
   void CaptureNextFrame();
@@ -56,6 +58,8 @@ class WebrtcFrameSchedulerConstantRate : public WebrtcFrameScheduler {
   // Framerate for scheduling frames. Initially 0 to prevent scheduling before
   // the output sink has been added.
   int max_framerate_fps_ GUARDED_BY_CONTEXT(sequence_checker_) = 0;
+  base::TimeDelta capture_interval_ GUARDED_BY_CONTEXT(sequence_checker_);
+  base::TimeDelta post_task_adjustment_ GUARDED_BY_CONTEXT(sequence_checker_);
 
   base::TimeDelta boost_capture_interval_ GUARDED_BY_CONTEXT(sequence_checker_);
   base::TimeTicks boost_window_ GUARDED_BY_CONTEXT(sequence_checker_);

remoting/protocol/webrtc_frame_scheduler_unittest.cc

@@ -27,6 +27,7 @@ class WebrtcFrameSchedulerTest : public ::testing::Test {
 
   void InitConstantRateScheduler() {
     scheduler_ = std::make_unique<WebrtcFrameSchedulerConstantRate>();
+    scheduler_->SetPostTaskAdjustmentForTest(base::Milliseconds(0));
     scheduler_->Start(base::BindRepeating(
         &WebrtcFrameSchedulerTest::CaptureCallback, base::Unretained(this)));
   }
@@ -43,7 +44,7 @@ class WebrtcFrameSchedulerTest : public ::testing::Test {
   base::test::TaskEnvironment task_environment_{
       base::test::TaskEnvironment::TimeSource::MOCK_TIME};
 
-  std::unique_ptr<WebrtcFrameScheduler> scheduler_;
+  std::unique_ptr<WebrtcFrameSchedulerConstantRate> scheduler_;
 
   int capture_callback_count_ = 0;
   bool simulate_capture_ = true;
@@ -71,6 +72,19 @@ TEST_F(WebrtcFrameSchedulerTest, CapturesAtRequestedFramerate) {
   EXPECT_LE(capture_callback_count_, 61);
 }
 
+TEST_F(WebrtcFrameSchedulerTest, PostTaskAdjustmentApplied) {
+  InitConstantRateScheduler();
+  scheduler_->SetPostTaskAdjustmentForTest(base::Milliseconds(3));
+  scheduler_->SetMaxFramerateFps(30);
+
+  task_environment_.FastForwardBy(base::Seconds(1));
+
+  // There should be approximately ~33 captures in 1 second, making an allowance
+  // for any off-by-one artifacts in timing.
+  EXPECT_GE(capture_callback_count_, 32);
+  EXPECT_LE(capture_callback_count_, 34);
+}
+
 TEST_F(WebrtcFrameSchedulerTest, NoCaptureWhileCapturePending) {
   InitConstantRateScheduler();
   simulate_capture_ = false;