Update the logic to determine preferred interval

To avoid smoothness issues, choose one from the supported intervals which is in perfect candence with |min_frame_sink_interval| instead of simply choose the closest one.

Also, add a line to update display_frame_timebase in root_compositor_frame_sink_impl.cc when |use_preferred_interval_| is true but without a meaningful interval.

Bug: 976583
Change-Id: I9e221dd759efc9c1cb0c40102d61695ff075ef9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013469
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745326}

AUTHORS

@@ -433,6 +433,7 @@ Jesper Storm Bache <jsbache@gmail.com>
 Jesse Miller <jesse@jmiller.biz>
 Jesus Sanchez-Palencia <jesus.sanchez-palencia.fernandez.fil@intel.com>
 Jiadong Zhu <jiadong.zhu@linaro.org>
+Jiahe Zhang <jiahe.zhang@intel.com>
 Jiajia Qin <jiajia.qin@intel.com>
 Jiajie Hu <jiajie.hu@intel.com>
 Jianjun Zhu <jianjun.zhu@intel.com>

components/viz/service/display/frame_rate_decider.cc

@@ -114,15 +114,19 @@ void FrameRateDecider::UpdatePreferredFrameIntervalIfNeeded() {
   // ideal refresh rate.
   base::TimeDelta new_preferred_interval = UnspecifiedFrameInterval();
   if (min_frame_sink_interval != BeginFrameArgs::MinInterval()) {
+    constexpr float kMaxIntervalDelta = 0.05;
     for (auto supported_interval : supported_intervals_) {
-      // Pick the display interval which is closest to the preferred interval.
-      // TODO(khushalsagar): This should suffice for the current use-case (based
-      // on supported refresh rates we expect), but we should be picking a frame
-      // rate with the correct tradeoff between running the display at a lower
-      // interval to save power and getting an ideal cadence for the video's
-      // frame rate.
-      if ((min_frame_sink_interval - supported_interval).magnitude() <
-          (min_frame_sink_interval - new_preferred_interval).magnitude()) {
+      // We only use a supported interval if it is in perfect cadence with the
+      // desired interval.
+      float delta_int = 0;
+      float delta = std::modf(min_frame_sink_interval.InMicrosecondsF() /
+                                  supported_interval.InMicrosecondsF(),
+                              &delta_int);
+      bool in_perfect_cadence =
+          delta < kMaxIntervalDelta || delta > (1 - kMaxIntervalDelta);
+      if (in_perfect_cadence && supported_interval > new_preferred_interval) {
+        // Make sure that we select the largest one in the
+        // |supported_intervals_| that meets the requirements
         new_preferred_interval = supported_interval;
       }
     }

components/viz/service/display/frame_rate_decider_unittest.cc

@@ -197,6 +197,50 @@ TEST_F(FrameRateDeciderTest,
   EXPECT_EQ(display_interval_, FrameRateDecider::UnspecifiedFrameInterval());
 }
 
+TEST_F(FrameRateDeciderTest, OptimalFrameSinkIntervelIsPicked) {
+  base::TimeDelta min_supported_interval = base::TimeDelta::FromSeconds(1);
+  const std::vector<base::TimeDelta> supported_intervals = {
+      min_supported_interval * 2, min_supported_interval};
+  frame_rate_decider_->SetSupportedFrameIntervals(supported_intervals);
+  EXPECT_EQ(display_interval_, FrameRateDecider::UnspecifiedFrameInterval());
+
+  FrameSinkId frame_sink_id1(1u, 1u);
+  preferred_intervals_[frame_sink_id1] = min_supported_interval * 2.5;
+  auto* surface1 = CreateAndDrawSurface(frame_sink_id1);
+
+  FrameSinkId frame_sink_id2(1u, 2u);
+  preferred_intervals_[frame_sink_id2] = min_supported_interval * 2.03;
+  auto* surface2 = CreateAndDrawSurface(frame_sink_id2);
+
+  FrameSinkId frame_sink_id3(1u, 3u);
+  preferred_intervals_[frame_sink_id3] = min_supported_interval * 0.5;
+  auto* surface3 = CreateAndDrawSurface(frame_sink_id3);
+
+  UpdateFrame(surface1);
+  {
+    FrameRateDecider::ScopedAggregate scope(frame_rate_decider_.get());
+    frame_rate_decider_->OnSurfaceWillBeDrawn(surface1);
+  }
+  EXPECT_EQ(display_interval_, FrameRateDecider::UnspecifiedFrameInterval());
+
+  UpdateFrame(surface2);
+  {
+    FrameRateDecider::ScopedAggregate scope(frame_rate_decider_.get());
+    frame_rate_decider_->OnSurfaceWillBeDrawn(surface1);
+    frame_rate_decider_->OnSurfaceWillBeDrawn(surface2);
+  }
+  EXPECT_EQ(display_interval_, min_supported_interval * 2);
+
+  UpdateFrame(surface3);
+  {
+    FrameRateDecider::ScopedAggregate scope(frame_rate_decider_.get());
+    frame_rate_decider_->OnSurfaceWillBeDrawn(surface1);
+    frame_rate_decider_->OnSurfaceWillBeDrawn(surface2);
+    frame_rate_decider_->OnSurfaceWillBeDrawn(surface3);
+  }
+  EXPECT_EQ(display_interval_, FrameRateDecider::UnspecifiedFrameInterval());
+}
+
 TEST_F(FrameRateDeciderTest, MinFrameSinkIntervalIsPicked) {
   base::TimeDelta min_supported_interval = base::TimeDelta::FromSeconds(1);
   const std::vector<base::TimeDelta> supported_intervals = {
@@ -206,11 +250,11 @@ TEST_F(FrameRateDeciderTest, MinFrameSinkIntervalIsPicked) {
   EXPECT_EQ(display_interval_, FrameRateDecider::UnspecifiedFrameInterval());
 
   FrameSinkId frame_sink_id1(1u, 1u);
-  preferred_intervals_[frame_sink_id1] = min_supported_interval * 2.75;
+  preferred_intervals_[frame_sink_id1] = min_supported_interval * 3;
   auto* surface1 = CreateAndDrawSurface(frame_sink_id1);
 
   FrameSinkId frame_sink_id2(1u, 2u);
-  preferred_intervals_[frame_sink_id2] = min_supported_interval * 2.2;
+  preferred_intervals_[frame_sink_id2] = min_supported_interval * 2;
   auto* surface2 = CreateAndDrawSurface(frame_sink_id2);
 
   UpdateFrame(surface1);

components/viz/service/frame_sinks/root_compositor_frame_sink_impl.cc

@@ -225,6 +225,11 @@ void RootCompositorFrameSinkImpl::SetDisplayVSyncParameters(
       constexpr float kMaxTimebaseDelta = 0.05;
       if (timebase_delta > display_frame_interval_ * kMaxTimebaseDelta)
         display_frame_timebase_ = timebase;
+    } else {
+      // |display_frame_timebase_| should be still updated as normal in
+      // preferred interval mode without a meaningful
+      // |preferred_frame_interval_|
+      display_frame_timebase_ = timebase;
     }
   } else {
     display_frame_timebase_ = timebase;