[cc/metrics] Fix setting partial updates.

If a CompositorFrame includes new updates from the main-thread, then
that frame should be treated as having complete update, rather than
partial update, even if the main-thread update is not for that
specific begin-frame.

BUG=1115376

Change-Id: I9987e77a3357c1c5eeb895514e584e780628fcc2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2381731Reviewed-by: Xida Chen <xidachen@chromium.org>
Reviewed-by: Behdad Bakhshinategh <behdadb@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803197}

cc/metrics/compositor_frame_reporting_controller.cc

@@ -154,15 +154,12 @@ void CompositorFrameReportingController::DidSubmitCompositorFrame(
     const viz::BeginFrameId& current_frame_id,
     const viz::BeginFrameId& last_activated_frame_id,
     EventMetricsSet events_metrics) {
-  // If the last_activated_frame_id from scheduler is the same as
-  // last_submitted_frame_id_ in reporting controller, this means that we are
-  // submitting the Impl frame. In this case the frame will be submitted if
-  // 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_|
+  // It is possible to submit a CompositorFrame containing outputs from two
+  // different begin-frames: an begin-main-frame that was blocked on the
+  // main-thread, and another one for the compositor thread.
   std::unique_ptr<CompositorFrameReporter> main_reporter;
   std::unique_ptr<CompositorFrameReporter> impl_reporter;
 
@@ -175,15 +172,25 @@ void CompositorFrameReportingController::DidSubmitCompositorFrame(
               last_activated_frame_id);
     // The reporter in activate state can be submitted
     main_reporter = std::move(reporters_[PipelineStage::kActivate]);
+    last_submitted_frame_id_ = last_activated_frame_id;
   } 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)
-  // the main thread work is not done yet and the impl portion should be
-  // reported.
+  // |main_reporter| can be for a previous BeginFrameArgs (i.e. not for
+  // |current_frame_id|), in which case it is necessary to also report metrics
+  // for the reporter representing |current_frame_id|. Following are the
+  // possibilities:
+  //  1) the main-thread did not request any updates (i.e. a 'begin main frame'
+  //     was not issued). The reporter for |current_frame_id| should still be in
+  //     the 'impl frame' stage.
+  //  2) the 'begin main frame' was issued, but the main-thread did not have any
+  //     updates (i.e. the 'begin main frame' was aborted). The reporter for
+  //     |current_frame_id| should be in the 'main frame' stage, and it will
+  //     have been aborted.
+  //  3) main-thread is still processing 'begin main frame'. The reporter for
+  //     |current_frame_id| should be in either the 'main frame' or 'commit'
+  //     stage.
   if (CanSubmitImplFrame(current_frame_id)) {
     auto& reporter = reporters_[PipelineStage::kBeginImplFrame];
     reporter->StartStage(StageType::kEndActivateToSubmitCompositorFrame,
@@ -199,23 +206,33 @@ void CompositorFrameReportingController::DidSubmitCompositorFrame(
                          PipelineStage::kActivate);
     impl_reporter = std::move(reporters_[PipelineStage::kActivate]);
   } else {
-    // 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) {
       reporter->StartStage(StageType::kEndActivateToSubmitCompositorFrame,
                            reporter->impl_frame_finish_time());
-      reporter->SetHasPartialUpdate();
+      // If the frame does not include any new updates from the main thread,
+      // then flag the frame as containing only partial updates.
+      if (!is_activated_frame_new)
+        reporter->SetHasPartialUpdate();
       impl_reporter = std::move(reporter);
     }
   }
 
+#if DCHECK_IS_ON()
   if (!events_metrics.main_event_metrics.empty()) {
     DCHECK(main_reporter);
   }
 
+  if (impl_reporter) {
+    DCHECK_EQ(impl_reporter->frame_id(), current_frame_id);
+    if (main_reporter) {
+      DCHECK_NE(main_reporter->frame_id(), current_frame_id);
+    }
+  }
+#endif
+
   // 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|.
@@ -233,7 +250,6 @@ void CompositorFrameReportingController::DidSubmitCompositorFrame(
         events_metrics.impl_event_metrics.end());
   }
 
-  last_submitted_frame_id_ = last_activated_frame_id;
   if (main_reporter) {
     main_reporter->StartStage(
         StageType::kSubmitCompositorFrameToPresentationCompositorFrame, Now());
@@ -382,10 +398,14 @@ void CompositorFrameReportingController::AdvanceReporterStage(
 
 bool CompositorFrameReportingController::CanSubmitImplFrame(
     const viz::BeginFrameId& id) const {
-  if (!reporters_[PipelineStage::kBeginImplFrame])
-    return false;
+#if DCHECK_IS_ON()
   auto& reporter = reporters_[PipelineStage::kBeginImplFrame];
-  return (reporter->frame_id() == id && reporter->did_finish_impl_frame());
+  if (reporter) {
+    DCHECK_EQ(reporter->frame_id(), id);
+    DCHECK(reporter->did_finish_impl_frame());
+  }
+#endif
+  return reporters_[PipelineStage::kBeginImplFrame].get() != nullptr;
 }
 
 bool CompositorFrameReportingController::CanSubmitMainFrame(

cc/metrics/compositor_frame_reporting_controller_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/strings/strcat.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/simple_test_tick_clock.h"
+#include "cc/metrics/dropped_frame_counter.h"
 #include "cc/metrics/event_metrics.h"
 #include "components/viz/common/frame_timing_details.h"
 #include "components/viz/common/quads/compositor_frame_metadata.h"
@@ -1237,5 +1238,40 @@ TEST_F(CompositorFrameReportingControllerTest,
               IsEmpty());
 }
 
+TEST_F(CompositorFrameReportingControllerTest,
+       NewMainUpdateIsNotPartialUpdate) {
+  DroppedFrameCounter dropped_counter;
+  reporting_controller_.SetDroppedFrameCounter(&dropped_counter);
+
+  SimulateBeginMainFrame();
+  reporting_controller_.OnFinishImplFrame(current_id_);
+  reporting_controller_.DidSubmitCompositorFrame(1u, current_id_, {}, {});
+  viz::FrameTimingDetails details = {};
+  details.presentation_feedback.timestamp = AdvanceNowByMs(10);
+  reporting_controller_.DidPresentCompositorFrame(1u, details);
+  EXPECT_EQ(1u, dropped_counter.total_frames());
+  EXPECT_EQ(1u, dropped_counter.total_main_dropped());
+
+  reporting_controller_.WillCommit();
+  reporting_controller_.DidCommit();
+  reporting_controller_.WillActivate();
+  reporting_controller_.DidActivate();
+
+  const auto previous_id = current_id_;
+
+  SimulateBeginMainFrame();
+  reporting_controller_.OnFinishImplFrame(current_id_);
+  reporting_controller_.DidSubmitCompositorFrame(1u, current_id_, previous_id,
+                                                 {});
+  details.presentation_feedback.timestamp = AdvanceNowByMs(10);
+  reporting_controller_.DidPresentCompositorFrame(1u, details);
+
+  EXPECT_EQ(3u, dropped_counter.total_frames());
+  EXPECT_EQ(1u, dropped_counter.total_main_dropped());
+
+  reporting_controller_.ResetReporters();
+  reporting_controller_.SetDroppedFrameCounter(nullptr);
+}
+
 }  // namespace
 }  // namespace cc