Fix cc metrics calculation error caused by throttling.

This patchset added |frames_throttled_since_last| to BeginFrameArgs.

This member on BeginFrameArgs stores the frames skipped during throttling
since last BeginFrame in each compositor frame sink and is used in
calculating relevant cc metrics based on |sequence_number|.


Bug: 1189893
Change-Id: I80162bb1440ac67d491cee88ad3e8c57145759ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2774267
Reviewed-by: Jonathan Ross <[email protected]>
Reviewed-by: Will Harris <[email protected]>
Commit-Queue: Jun Liu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#870581}

cc/metrics/frame_sequence_tracker.cc

@@ -142,7 +142,8 @@ void FrameSequenceTracker::ReportBeginImplFrame(
   DCHECK(!compositor_frame_submitted_) << TRACKER_DCHECK_MSG;
 
   UpdateTrackedFrameData(&begin_impl_frame_data_, args.frame_id.source_id,
-                         args.frame_id.sequence_number);
+                         args.frame_id.sequence_number,
+                         args.frames_throttled_since_last);
   impl_throughput().frames_expected +=
       begin_impl_frame_data_.previous_sequence_delta;
 #if DCHECK_IS_ON()
@@ -188,7 +189,8 @@ void FrameSequenceTracker::ReportBeginMainFrame(
   awaiting_main_response_sequence_ = args.frame_id.sequence_number;
 
   UpdateTrackedFrameData(&begin_main_frame_data_, args.frame_id.source_id,
-                         args.frame_id.sequence_number);
+                         args.frame_id.sequence_number,
+                         args.frames_throttled_since_last);
   if (!first_received_main_sequence_ ||
       first_received_main_sequence_ <= last_no_main_damage_sequence_) {
     first_received_main_sequence_ = args.frame_id.sequence_number;
@@ -639,12 +641,15 @@ void FrameSequenceTracker::PauseFrameProduction() {
   reset_all_state_ = true;
 }
 
-void FrameSequenceTracker::UpdateTrackedFrameData(TrackedFrameData* frame_data,
-                                                  uint64_t source_id,
-                                                  uint64_t sequence_number) {
+void FrameSequenceTracker::UpdateTrackedFrameData(
+    TrackedFrameData* frame_data,
+    uint64_t source_id,
+    uint64_t sequence_number,
+    uint64_t throttled_frame_count) {
   if (frame_data->previous_sequence &&
       frame_data->previous_source == source_id) {
-    uint32_t current_latency = sequence_number - frame_data->previous_sequence;
+    uint32_t current_latency =
+        sequence_number - frame_data->previous_sequence - throttled_frame_count;
     DCHECK_GT(current_latency, 0u) << TRACKER_DCHECK_MSG;
     frame_data->previous_sequence_delta = current_latency;
   } else {

cc/metrics/frame_sequence_tracker.h

@@ -156,7 +156,8 @@ class CC_EXPORT FrameSequenceTracker {
 
   void UpdateTrackedFrameData(TrackedFrameData* frame_data,
                               uint64_t source_id,
-                              uint64_t sequence_number);
+                              uint64_t sequence_number,
+                              uint64_t throttled_frame_count);
 
   bool ShouldIgnoreBeginFrameSource(uint64_t source_id) const;
 

components/viz/common/frame_sinks/begin_frame_args.cc

@@ -88,10 +88,7 @@ BeginFrameArgs::BeginFrameArgs()
     : frame_time(base::TimeTicks::Min()),
       deadline(base::TimeTicks::Min()),
       interval(base::TimeDelta::FromMicroseconds(-1)),
-      frame_id(BeginFrameId(0, kInvalidFrameNumber)),
-      type(BeginFrameArgs::INVALID),
-      on_critical_path(true),
-      animate_only(false) {}
+      frame_id(BeginFrameId(0, kInvalidFrameNumber)) {}
 
 BeginFrameArgs::BeginFrameArgs(uint64_t source_id,
                                uint64_t sequence_number,
@@ -103,9 +100,7 @@ BeginFrameArgs::BeginFrameArgs(uint64_t source_id,
       deadline(deadline),
       interval(interval),
       frame_id(BeginFrameId(source_id, sequence_number)),
-      type(type),
-      on_critical_path(true),
-      animate_only(false) {
+      type(type) {
   DCHECK_LE(kStartingFrameNumber, sequence_number);
 }
 
@@ -144,6 +139,7 @@ void BeginFrameArgs::AsValueInto(base::trace_event::TracedValue* state) const {
   state->SetString("subtype", TypeToString(type));
   state->SetInteger("source_id", frame_id.source_id);
   state->SetInteger("sequence_number", frame_id.sequence_number);
+  state->SetInteger("frames_throttled_since_last", frames_throttled_since_last);
   state->SetDouble("frame_time_us",
                    frame_time.since_origin().InMicrosecondsF());
   state->SetDouble("deadline_us", deadline.since_origin().InMicrosecondsF());
@@ -160,6 +156,9 @@ void BeginFrameArgs::AsProtozeroInto(
   state->set_type(TypeToProtozeroEnum(type));
   state->set_source_id(frame_id.source_id);
   state->set_sequence_number(frame_id.sequence_number);
+  // TODO(yjliu) add frames_throttled_since_last to third_party
+  // chrome_compositor_scheduler_state.proto
+  // state->set_frames_throttled_since_last(frames_throttled_since_last);
   state->set_frame_time_us(frame_time.since_origin().InMicroseconds());
   state->set_deadline_us(deadline.since_origin().InMicroseconds());
   state->set_interval_delta_us(interval.InMicroseconds());
@@ -185,13 +184,10 @@ std::string BeginFrameArgs::ToString() const {
   return value.ToJSON();
 }
 
-BeginFrameAck::BeginFrameAck() : has_damage(false) {}
-
 BeginFrameAck::BeginFrameAck(const BeginFrameArgs& args, bool has_damage)
-    : BeginFrameAck(args.frame_id.source_id,
-                    args.frame_id.sequence_number,
-                    has_damage,
-                    args.trace_id) {}
+    : frame_id(args.frame_id),
+      trace_id(args.trace_id),
+      has_damage(has_damage) {}
 
 BeginFrameAck::BeginFrameAck(uint64_t source_id,
                              uint64_t sequence_number,

components/viz/common/frame_sinks/begin_frame_args.h

@@ -169,8 +169,8 @@ struct VIZ_COMMON_EXPORT BeginFrameArgs {
   // the client and service as the id for trace-events.
   int64_t trace_id = -1;
 
-  BeginFrameArgsType type;
-  bool on_critical_path;
+  BeginFrameArgsType type = INVALID;
+  bool on_critical_path = true;
 
   // If true, observers of this BeginFrame should not produce a new
   // CompositorFrame, but instead only run the (web-visible) side effects of the
@@ -184,7 +184,11 @@ struct VIZ_COMMON_EXPORT BeginFrameArgs {
   // Designed for use in headless, in conjunction with
   // --disable-threaded-animation, --disable-threaded-scrolling, and
   // --disable-checker-imaging, see bit.ly/headless-rendering.
-  bool animate_only;
+  bool animate_only = false;
+
+  // Number of frames being skipped during throttling since last BeginFrame
+  // sent.
+  uint64_t frames_throttled_since_last = 0;
 
  private:
   BeginFrameArgs(uint64_t source_id,
@@ -197,7 +201,7 @@ struct VIZ_COMMON_EXPORT BeginFrameArgs {
 
 // Sent by a BeginFrameObserver as acknowledgment of completing a BeginFrame.
 struct VIZ_COMMON_EXPORT BeginFrameAck {
-  BeginFrameAck();
+  BeginFrameAck() = default;
 
   // Constructs an instance as a response to the specified BeginFrameArgs.
   BeginFrameAck(const BeginFrameArgs& args, bool has_damage);
@@ -223,7 +227,7 @@ struct VIZ_COMMON_EXPORT BeginFrameAck {
 
   // |true| if the observer has produced damage (e.g. sent a CompositorFrame or
   // damaged a surface) as part of responding to the BeginFrame.
-  bool has_damage;
+  bool has_damage = false;
 };
 
 }  // namespace viz

components/viz/common/frame_sinks/begin_frame_args_unittest.cc

@@ -35,6 +35,7 @@ TEST(BeginFrameArgsTest, Helpers) {
   EXPECT_EQ(k2Usec, args2.deadline.since_origin());
   EXPECT_EQ(k3Usec, args2.interval);
   EXPECT_EQ(BeginFrameArgs::NORMAL, args2.type);
+  EXPECT_EQ(0u, args2.frames_throttled_since_last);
 
   BeginFrameArgs args4 = CreateBeginFrameArgsForTesting(
       BEGINFRAME_FROM_HERE, 234, 20, 1, 2, 3, BeginFrameArgs::MISSED);
@@ -45,6 +46,7 @@ TEST(BeginFrameArgsTest, Helpers) {
   EXPECT_EQ(k2Usec, args4.deadline.since_origin());
   EXPECT_EQ(k3Usec, args4.interval);
   EXPECT_EQ(BeginFrameArgs::MISSED, args4.type);
+  EXPECT_EQ(0u, args4.frames_throttled_since_last);
 
   // operator==
   EXPECT_EQ(
@@ -82,15 +84,15 @@ TEST(BeginFrameArgsTest, Helpers) {
   // operator<<
   std::stringstream out1;
   out1 << args1;
-  EXPECT_EQ("BeginFrameArgs(NORMAL, 0, 1, 0, 0, -1us)", out1.str());
+  EXPECT_EQ("BeginFrameArgs(NORMAL, 0, 1, 0, 0, -1us, 0)", out1.str());
   std::stringstream out2;
   out2 << args2;
-  EXPECT_EQ("BeginFrameArgs(NORMAL, 123, 10, 1, 2, 3us)", out2.str());
+  EXPECT_EQ("BeginFrameArgs(NORMAL, 123, 10, 1, 2, 3us, 0)", out2.str());
 
   // PrintTo
-  EXPECT_EQ(std::string("BeginFrameArgs(NORMAL, 0, 1, 0, 0, -1us)"),
+  EXPECT_EQ(std::string("BeginFrameArgs(NORMAL, 0, 1, 0, 0, -1us, 0)"),
             ::testing::PrintToString(args1));
-  EXPECT_EQ(std::string("BeginFrameArgs(NORMAL, 123, 10, 1, 2, 3us)"),
+  EXPECT_EQ(std::string("BeginFrameArgs(NORMAL, 123, 10, 1, 2, 3us, 0)"),
             ::testing::PrintToString(args2));
 }
 
@@ -113,6 +115,7 @@ TEST(BeginFrameArgsTest, Create) {
   EXPECT_EQ(k2Usec, args2.deadline.since_origin()) << args2;
   EXPECT_EQ(k3Usec, args2.interval) << args2;
   EXPECT_EQ(BeginFrameArgs::NORMAL, args2.type) << args2;
+  EXPECT_EQ(0u, args2.frames_throttled_since_last) << args2;
 }
 
 #ifndef NDEBUG

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -754,6 +754,9 @@ void CompositorFrameSinkSupport::OnBeginFrame(const BeginFrameArgs& args) {
                            TRACE_ID_GLOBAL(copy_args.trace_id),
                            TRACE_EVENT_FLAG_FLOW_OUT, "step",
                            "IssueBeginFrame");
+    copy_args.frames_throttled_since_last = frames_throttled_since_last_;
+    frames_throttled_since_last_ = 0;
+
     last_frame_time_ = args.frame_time;
     client_->OnBeginFrame(copy_args, std::move(frame_timing_details_));
     begin_frame_tracker_.SentBeginFrame(args);
@@ -936,8 +939,7 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
   // |begin_frame_interval_| since the last one was sent because clients have
   // requested to update at such rate.
   const bool should_throttle_as_requested =
-      begin_frame_interval_ > base::TimeDelta() &&
-      (frame_time - last_frame_time_) < begin_frame_interval_;
+      ShouldThrottleBeginFrameAsRequested(frame_time);
   // We might throttle this OnBeginFrame() if it's been less than a second since
   // the last one was sent, either because clients are unresponsive or have
   // submitted too many undrawn frames.
@@ -982,6 +984,7 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
   }
 
   if (should_throttle_as_requested) {
+    ++frames_throttled_since_last_;
     RecordShouldSendBeginFrame("ThrottleRequested");
     return false;
   }
@@ -1029,6 +1032,12 @@ void CompositorFrameSinkSupport::CheckPendingSurfaces() {
   }
 }
 
+bool CompositorFrameSinkSupport::ShouldThrottleBeginFrameAsRequested(
+    base::TimeTicks frame_time) {
+  return begin_frame_interval_ > base::TimeDelta() &&
+         (frame_time - last_frame_time_) < begin_frame_interval_;
+}
+
 void CompositorFrameSinkSupport::OnCompositorFrameTransitionDirectiveProcessed(
     uint32_t sequence_id) {
   if (client_)

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -255,6 +255,12 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   // deadline has passed. This is called every BeginFrame.
   void CheckPendingSurfaces();
 
+  // When throttling is requested by a client, a BeginFrame will not be sent
+  // until the time elapsed has passed the requested throttle interval since the
+  // last sent BeginFrame. This function returns true if such interval has
+  // passed and a BeginFrame should be sent.
+  bool ShouldThrottleBeginFrameAsRequested(base::TimeTicks frame_time);
+
   mojom::CompositorFrameSinkClient* const client_;
 
   FrameSinkManagerImpl* const frame_sink_manager_;
@@ -372,6 +378,9 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   // Represents whether the DocumentTransition feature is enabled.
   bool document_transitions_enabled_;
 
+  // Number of frames skipped during throttling since last BeginFrame sent.
+  uint64_t frames_throttled_since_last_ = 0;
+
   base::WeakPtrFactory<CompositorFrameSinkSupport> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(CompositorFrameSinkSupport);

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -1555,16 +1555,19 @@ TEST_F(CompositorFrameSinkSupportTest, BeginFrameInterval) {
   uint64_t sequence_number = 1;
   int sent_frames = 0;
   BeginFrameArgs args;
-
+  uint64_t frames_throttled_since_last = 0;
   const base::TimeTicks end_time = frame_time + base::TimeDelta::FromSeconds(2);
 
   base::TimeTicks next_expected_begin_frame = frame_time;
   while (frame_time < end_time) {
     args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE, 0,
                                           sequence_number++, frame_time);
     if (frame_time < next_expected_begin_frame) {
-      EXPECT_CALL(mock_client, OnBeginFrame(args, _)).Times(0);
+      EXPECT_CALL(mock_client, OnBeginFrame(_, _)).Times(0);
+      ++frames_throttled_since_last;
     } else {
+      args.frames_throttled_since_last = frames_throttled_since_last;
+      frames_throttled_since_last = 0;
       EXPECT_CALL(mock_client, OnBeginFrame(args, _)).WillOnce([&]() {
         support->SubmitCompositorFrame(local_surface_id_,
                                        MakeDefaultCompositorFrame());

components/viz/test/begin_frame_args_test.cc

@@ -79,7 +79,8 @@ BeginFrameArgs CreateBeginFrameArgsForTesting(
 bool operator==(const BeginFrameArgs& lhs, const BeginFrameArgs& rhs) {
   return (lhs.type == rhs.type) && (lhs.frame_id == rhs.frame_id) &&
          (lhs.frame_time == rhs.frame_time) && (lhs.deadline == rhs.deadline) &&
-         (lhs.interval == rhs.interval);
+         (lhs.interval == rhs.interval) &&
+         (lhs.frames_throttled_since_last == rhs.frames_throttled_since_last);
 }
 
 ::std::ostream& operator<<(::std::ostream& os, const BeginFrameArgs& args) {
@@ -92,7 +93,8 @@ void PrintTo(const BeginFrameArgs& args, ::std::ostream* os) {
       << args.frame_id.source_id << ", " << args.frame_id.sequence_number
       << ", " << args.frame_time.since_origin().InMicroseconds() << ", "
       << args.deadline.since_origin().InMicroseconds() << ", "
-      << args.interval.InMicroseconds() << "us)";
+      << args.interval.InMicroseconds() << "us, "
+      << args.frames_throttled_since_last << ")";
 }
 
 bool operator==(const BeginFrameAck& lhs, const BeginFrameAck& rhs) {

services/viz/public/cpp/compositing/begin_frame_args_mojom_traits.cc

@@ -55,6 +55,7 @@ bool StructTraits<viz::mojom::BeginFrameArgsDataView, viz::BeginFrameArgs>::
   }
   out->frame_id.source_id = data.source_id();
   out->frame_id.sequence_number = data.sequence_number();
+  out->frames_throttled_since_last = data.frames_throttled_since_last();
   out->trace_id = data.trace_id();
   out->on_critical_path = data.on_critical_path();
   out->animate_only = data.animate_only();

services/viz/public/cpp/compositing/begin_frame_args_mojom_traits.h

@@ -42,6 +42,10 @@ struct StructTraits<viz::mojom::BeginFrameArgsDataView, viz::BeginFrameArgs> {
     return args.frame_id.source_id;
   }
 
+  static uint64_t frames_throttled_since_last(const viz::BeginFrameArgs& args) {
+    return args.frames_throttled_since_last;
+  }
+
   static int64_t trace_id(const viz::BeginFrameArgs& args) {
     return args.trace_id;
   }

services/viz/public/cpp/compositing/mojom_traits_unittest.cc

@@ -82,9 +82,11 @@ TEST_F(StructTraitsTest, BeginFrameArgs) {
   const bool on_critical_path = true;
   const uint64_t source_id = 5;
   const uint64_t sequence_number = 10;
+  const uint64_t frames_throttled_since_last = 20;
   const bool animate_only = true;
   BeginFrameArgs input;
   input.frame_id = BeginFrameId(source_id, sequence_number);
+  input.frames_throttled_since_last = frames_throttled_since_last;
   input.frame_time = frame_time;
   input.deadline = deadline;
   input.interval = interval;
@@ -97,6 +99,7 @@ TEST_F(StructTraitsTest, BeginFrameArgs) {
 
   EXPECT_EQ(source_id, output.frame_id.source_id);
   EXPECT_EQ(sequence_number, output.frame_id.sequence_number);
+  EXPECT_EQ(frames_throttled_since_last, output.frames_throttled_since_last);
   EXPECT_EQ(frame_time, output.frame_time);
   EXPECT_EQ(deadline, output.deadline);
   EXPECT_EQ(interval, output.interval);

services/viz/public/mojom/compositing/begin_frame_args.mojom

@@ -19,6 +19,7 @@ struct BeginFrameArgs {
   mojo_base.mojom.TimeDelta interval;
   uint64 source_id;
   uint64 sequence_number;
+  uint64 frames_throttled_since_last;
   int64 trace_id;
   BeginFrameArgsType type;
   bool on_critical_path;