[Metrics] Fix a bug that may lead to over-reporting of jank count

Jank reporter now uses |vsync_interval| to determine the frame length
increment threshold, instead of directly using the frame interval
reported by the presentation feedback; the latter could be zero and
cause erroneous jank reporting.

Bug: 1154492
Change-Id: I616a77d85bf91436ebc0b6982a278c5763225993
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2569794
Commit-Queue: Mingjing Zhang <mjzhang@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841095}

cc/metrics/frame_sequence_tracker.cc

@@ -449,13 +449,12 @@ void FrameSequenceTracker::ReportFramePresented(
   uint32_t impl_frames_ontime = 0;
   uint32_t main_frames_ontime = 0;
 
-  const auto& vsync_interval =
+  const auto vsync_interval =
       (feedback.interval.is_zero() ? viz::BeginFrameArgs::DefaultInterval()
-                                   : feedback.interval) *
-      1.5;
+                                   : feedback.interval);
   DCHECK(!vsync_interval.is_zero()) << TRACKER_DCHECK_MSG;
   base::TimeTicks safe_deadline_for_frame =
-      last_frame_presentation_timestamp_ + vsync_interval;
+      last_frame_presentation_timestamp_ + vsync_interval * 1.5;
 
   const bool was_presented = !feedback.failed();
   if (was_presented && submitted_frame_since_last_presentation) {
@@ -478,7 +477,7 @@ void FrameSequenceTracker::ReportFramePresented(
     }
 
     metrics()->ComputeJank(FrameSequenceMetrics::ThreadType::kCompositor,
-                           frame_token, feedback.timestamp, feedback.interval);
+                           frame_token, feedback.timestamp, vsync_interval);
   }
 
   if (was_presented) {
@@ -500,8 +499,7 @@ void FrameSequenceTracker::ReportFramePresented(
       }
 
       metrics()->ComputeJank(FrameSequenceMetrics::ThreadType::kMain,
-                             frame_token, feedback.timestamp,
-                             feedback.interval);
+                             frame_token, feedback.timestamp, vsync_interval);
     }
     if (main_frames_.size() < size_before_erase) {
       if (!last_frame_presentation_timestamp_.is_null() &&

cc/metrics/frame_sequence_tracker_unittest.cc

@@ -353,6 +353,92 @@ TEST_F(FrameSequenceTrackerTest, TestNotifyFramePresented) {
   EXPECT_EQ(NumberOfRemovalTrackers(), 0u);
 }
 
+TEST_F(FrameSequenceTrackerTest, TestJankWithZeroIntervalInFeedback) {
+  // Test if jank can be correctly counted if presentation feedback reports
+  // zero frame interval.
+  const uint64_t source = 1;
+  uint64_t sequence = 1;
+  uint64_t frame_token = sequence;
+  const char* histogram_name =
+      "Graphics.Smoothness.Jank.Compositor.TouchScroll";
+  const base::TimeDelta zero_interval = base::TimeDelta::FromMilliseconds(0);
+  base::HistogramTester histogram_tester;
+
+  CreateNewTracker();
+  base::TimeTicks args_timestamp = base::TimeTicks::Now();
+  auto args = CreateBeginFrameArgs(source, sequence, args_timestamp);
+
+  // Frame 1
+  collection_.NotifyBeginImplFrame(args);
+  collection_.NotifySubmitFrame(sequence, false, viz::BeginFrameAck(args, true),
+                                args);
+  collection_.NotifyFrameEnd(args, args);
+
+  collection_.NotifyFramePresented(
+      frame_token,
+      /*feedback=*/{args_timestamp, zero_interval, 0});
+
+  // Frame 2
+  ++sequence;
+  ++frame_token;
+  args_timestamp += base::TimeDelta::FromMillisecondsD(16.67);
+  args = CreateBeginFrameArgs(source, sequence, args_timestamp);
+  collection_.NotifyBeginImplFrame(args);
+  collection_.NotifySubmitFrame(sequence, false, viz::BeginFrameAck(args, true),
+                                args);
+  collection_.NotifyFrameEnd(args, args);
+  collection_.NotifyFramePresented(
+      frame_token,
+      /*feedback=*/{args_timestamp, zero_interval, 0});
+  ImplThroughput().frames_expected = 100u;
+  ReportMetrics();
+  histogram_tester.ExpectTotalCount(histogram_name, 1u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(histogram_name),
+              testing::ElementsAre(base::Bucket(0, 1)));
+
+  // Frame 3: There is one jank (frame interval incremented from 16.67ms
+  // to 30.0ms)
+  ++sequence;
+  ++frame_token;
+  args_timestamp += base::TimeDelta::FromMillisecondsD(30.0);
+  args = CreateBeginFrameArgs(source, sequence, args_timestamp);
+  collection_.NotifyBeginImplFrame(args);
+  collection_.NotifySubmitFrame(sequence, false, viz::BeginFrameAck(args, true),
+                                args);
+  collection_.NotifyFrameEnd(args, args);
+  collection_.NotifyFramePresented(
+      frame_token,
+      /*feedback=*/{args_timestamp, zero_interval, 0});
+
+  ImplThroughput().frames_expected = 100u;
+  ReportMetrics();
+  histogram_tester.ExpectTotalCount(
+      "Graphics.Smoothness.Jank.Compositor.TouchScroll", 2u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(histogram_name),
+              testing::ElementsAre(base::Bucket(0, 1), base::Bucket(1, 1)));
+
+  // Frame 4: There is no jank since the increment from 30ms to  31ms is too
+  // small. This tests if |NotifyFramePresented| can correctly handle the
+  // situation when the frame interval reported in presentation feedback is 0.
+  ++sequence;
+  ++frame_token;
+  args_timestamp += base::TimeDelta::FromMillisecondsD(31.0);
+  args = CreateBeginFrameArgs(source, sequence, args_timestamp);
+  collection_.NotifyBeginImplFrame(args);
+  collection_.NotifySubmitFrame(sequence, false, viz::BeginFrameAck(args, true),
+                                args);
+  collection_.NotifyFrameEnd(args, args);
+  collection_.NotifyFramePresented(
+      frame_token,
+      /*feedback=*/{args_timestamp, zero_interval, 0});
+  ImplThroughput().frames_expected = 100u;
+  ReportMetrics();
+  histogram_tester.ExpectTotalCount(
+      "Graphics.Smoothness.Jank.Compositor.TouchScroll", 3u);
+  EXPECT_THAT(histogram_tester.GetAllSamples(histogram_name),
+              testing::ElementsAre(base::Bucket(0, 1), base::Bucket(1, 2)));
+}
+
 // Base case for checkerboarding: present a single frame with checkerboarding,
 // followed by a non-checkerboard frame.
 TEST_F(FrameSequenceTrackerTest, CheckerboardingSimple) {