Report both the main and impl reporter from submitted reporter queue.

The impl reporter is now also added to the submitted reporter queue in
addition to the main reporter, so that now reports from both the main
and impl reporter will be issued.

e.g. For the following frame sequence:
BI_1 BM_1 S_1 P_1 BI_2 C_1 A_1 S_2 P_2

Now the sequence BI_2 -> S_2 -> P_2 will be reported in addition to the
existing reporting of BI_1 -> BM_1 -> C_1 A_1 S_2 P_2

Bug: 1059282
Change-Id: I2665d0df5bab15136ee5325bcd34729a1ae9f0a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2124694Reviewed-by: Behdad Bakhshinategh <behdadb@chromium.org>
Reviewed-by: Sadrul Chowdhury <sadrul@chromium.org>
Commit-Queue: Mingjing Zhang <mjzhang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757893}

cc/metrics/compositor_frame_reporting_controller.cc

@@ -153,11 +153,25 @@ void CompositorFrameReportingController::DidSubmitCompositorFrame(
   // Impl work is finished.
   bool is_activated_frame_new =
       (last_activated_frame_id != last_submitted_frame_id_);
+
+  // Temporarily hold the main and impl reporter until they are moved into
+  // |submitted_compositor_frames_|
+  std::unique_ptr<CompositorFrameReporter> main_reporter;
+  std::unique_ptr<CompositorFrameReporter> impl_reporter;
+
+  // If |is_activated_frame_new| is true, |main_reporter| is guaranteed to
+  // be set, and |impl_reporter| may or may not be set; otherwise,
+  // |impl_reporter| is guaranteed to be set, and |main_reporter| will not be
+  // set.
   if (is_activated_frame_new) {
     DCHECK_EQ(reporters_[PipelineStage::kActivate]->frame_id_,
               last_activated_frame_id);
     // The reporter in activate state can be submitted
+    main_reporter = std::move(reporters_[PipelineStage::kActivate]);
   } else {
+    DCHECK(!reporters_[PipelineStage::kActivate]);
+  }
+
   // There is no Main damage, which is possible if (1) there was no beginMain
   // so the reporter in beginImpl will be submitted or (2) the beginMain is
   // sent and aborted, so the reporter in beginMain will be submitted or (3)
@@ -169,48 +183,66 @@ void CompositorFrameReportingController::DidSubmitCompositorFrame(
                          reporter->impl_frame_finish_time());
     AdvanceReporterStage(PipelineStage::kBeginImplFrame,
                          PipelineStage::kActivate);
+    impl_reporter = std::move(reporters_[PipelineStage::kActivate]);
   } else if (CanSubmitMainFrame(current_frame_id)) {
     auto& reporter = reporters_[PipelineStage::kBeginMainFrame];
     reporter->StartStage(StageType::kEndActivateToSubmitCompositorFrame,
                          reporter->impl_frame_finish_time());
     AdvanceReporterStage(PipelineStage::kBeginMainFrame,
                          PipelineStage::kActivate);
+    impl_reporter = std::move(reporters_[PipelineStage::kActivate]);
   } else {
-      // The submitted frame might have unfinished main thread work, which in
-      // that case the BeginImpl portion can be reported.
+    // No main damage: the submitted frame might have unfinished main thread
+    // work, which in that case the BeginImpl portion can be reported.
     auto reporter = RestoreReporterAtBeginImpl(current_frame_id);
     // The method will return nullptr if Impl reporter has been submitted
     // prior to BeginMainFrame.
-      if (!reporter)
-        return;
+    if (reporter) {
       reporter->StartStage(StageType::kEndActivateToSubmitCompositorFrame,
                            reporter->impl_frame_finish_time());
-      reporters_[PipelineStage::kActivate] = std::move(reporter);
+      impl_reporter = std::move(reporter);
     }
   }
 
-  last_submitted_frame_id_ = last_activated_frame_id;
-  std::unique_ptr<CompositorFrameReporter> submitted_reporter =
-      std::move(reporters_[PipelineStage::kActivate]);
-  submitted_reporter->StartStage(
-      StageType::kSubmitCompositorFrameToPresentationCompositorFrame, Now());
-
-  // TODO(mjzhang): The main and impl vectors are combined to preserve the
-  // current behavior. In a subsequent CL, an impl reporter will also be added
-  // for submission and the two vectors will be kept separate and moved into
-  // their corresponding reporter.
-  std::vector<EventMetrics> combined_event_metrics =
-      std::move(events_metrics.main_event_metrics);
+  if (!events_metrics.main_event_metrics.empty()) {
+    DCHECK(main_reporter);
+  }
 
-  combined_event_metrics.reserve(combined_event_metrics.size() +
+  // When |impl_reporter| does not exist, but there are still impl-side metrics,
+  // merge the main and impl metrics and pass the combined vector into
+  // |main_reporter|.
+  if (!impl_reporter && !events_metrics.impl_event_metrics.empty()) {
+    DCHECK(main_reporter);
+    // If there are impl events, there must be a reporter with
+    // |current_frame_id|.
+    DCHECK_EQ(main_reporter->frame_id_, current_frame_id);
+    events_metrics.main_event_metrics.reserve(
+        events_metrics.main_event_metrics.size() +
         events_metrics.impl_event_metrics.size());
-  combined_event_metrics.insert(combined_event_metrics.end(),
+    events_metrics.main_event_metrics.insert(
+        events_metrics.main_event_metrics.end(),
         events_metrics.impl_event_metrics.begin(),
         events_metrics.impl_event_metrics.end());
+  }
+
+  last_submitted_frame_id_ = last_activated_frame_id;
+  if (main_reporter) {
+    main_reporter->StartStage(
+        StageType::kSubmitCompositorFrameToPresentationCompositorFrame, Now());
+    main_reporter->SetEventsMetrics(
+        std::move(events_metrics.main_event_metrics));
+    submitted_compositor_frames_.emplace_back(frame_token,
+                                              std::move(main_reporter));
+  }
 
-  submitted_reporter->SetEventsMetrics(std::move(combined_event_metrics));
+  if (impl_reporter) {
+    impl_reporter->StartStage(
+        StageType::kSubmitCompositorFrameToPresentationCompositorFrame, Now());
+    impl_reporter->SetEventsMetrics(
+        std::move(events_metrics.impl_event_metrics));
     submitted_compositor_frames_.emplace_back(frame_token,
-                                            std::move(submitted_reporter));
+                                              std::move(impl_reporter));
+  }
 }
 
 void CompositorFrameReportingController::DidNotProduceFrame(

cc/metrics/compositor_frame_reporting_controller_unittest.cc

@@ -355,7 +355,7 @@ TEST_F(CompositorFrameReportingControllerTest, DidNotProduceFrame) {
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.DroppedFrame.SendBeginMainFrameToCommit", 0);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 1);
+      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 2);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SendBeginMainFrameToCommit", 1);
   histogram_tester.ExpectTotalCount("CompositorLatency.Commit", 1);
@@ -363,10 +363,10 @@ TEST_F(CompositorFrameReportingControllerTest, DidNotProduceFrame) {
                                     1);
   histogram_tester.ExpectTotalCount("CompositorLatency.Activation", 1);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.EndActivateToSubmitCompositorFrame", 1);
+      "CompositorLatency.EndActivateToSubmitCompositorFrame", 2);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SubmitCompositorFrameToPresentationCompositorFrame",
-      1);
+      2);
 }
 
 TEST_F(CompositorFrameReportingControllerTest, MainFrameAborted) {
@@ -422,18 +422,18 @@ TEST_F(CompositorFrameReportingControllerTest, MainFrameAborted2) {
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.DroppedFrame.BeginImplFrameToSendBeginMainFrame", 0);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 1);
+      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 2);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.SendBeginMainFrameToCommit", 1);
+      "CompositorLatency.SendBeginMainFrameToCommit", 2);
   histogram_tester.ExpectTotalCount("CompositorLatency.Commit", 1);
   histogram_tester.ExpectTotalCount("CompositorLatency.EndCommitToActivation",
                                     1);
   histogram_tester.ExpectTotalCount("CompositorLatency.Activation", 1);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.EndActivateToSubmitCompositorFrame", 1);
+      "CompositorLatency.EndActivateToSubmitCompositorFrame", 2);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SubmitCompositorFrameToPresentationCompositorFrame",
-      1);
+      2);
   reporting_controller_.DidSubmitCompositorFrame(2, current_id_2, current_id_1,
                                                  {});
   reporting_controller_.DidPresentCompositorFrame(2, details);
@@ -482,6 +482,9 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame) {
   viz::BeginFrameId current_id_2(1, 2);
   viz::BeginFrameArgs args_2 = SimulateBeginFrameArgs(current_id_2);
 
+  viz::BeginFrameId current_id_3(1, 3);
+  viz::BeginFrameArgs args_3 = SimulateBeginFrameArgs(current_id_3);
+
   viz::FrameTimingDetails details = {};
   reporting_controller_.WillBeginImplFrame(args_1);
   reporting_controller_.OnFinishImplFrame(current_id_1);
@@ -530,6 +533,30 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame) {
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SubmitCompositorFrameToPresentationCompositorFrame",
       2);
+
+  reporting_controller_.WillBeginImplFrame(args_3);
+  reporting_controller_.OnFinishImplFrame(current_id_3);
+  reporting_controller_.WillCommit();
+  reporting_controller_.DidCommit();
+  reporting_controller_.WillActivate();
+  reporting_controller_.DidActivate();
+  reporting_controller_.DidSubmitCompositorFrame(3, current_id_3, current_id_2,
+                                                 {});
+  reporting_controller_.DidPresentCompositorFrame(3, details);
+
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 4);
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.SendBeginMainFrameToCommit", 2);
+  histogram_tester.ExpectTotalCount("CompositorLatency.Commit", 2);
+  histogram_tester.ExpectTotalCount("CompositorLatency.EndCommitToActivation",
+                                    2);
+  histogram_tester.ExpectTotalCount("CompositorLatency.Activation", 2);
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.EndActivateToSubmitCompositorFrame", 4);
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.SubmitCompositorFrameToPresentationCompositorFrame",
+      4);
 }
 
 TEST_F(CompositorFrameReportingControllerTest, LongMainFrame2) {