[cc/metrics] Reset 'partial update' flag if main-thread had no updates.

If the main-thread is blocked until a vsync's deadline, then the
reporter is cloned, submitted, and marked as having 'partial update'.
However, it is possible for the main-thread to respond, after the
deadline, with 'no updates' (or 'no damage'). In such cases, the frame
that was submitted did not really have a 'partial update' So reset the
flag that was set, so that the metrics are reported correctly.

To do this:
 . When a reporter is cloned, track the link between the original
   reporter and the cloned reporter.
 . When the original reporter is no longer blocked, check if the frame
   ended with 'no updates'. If it did, then unset the 'partial update'
   flag from the cloned reporter.
 . The cloned reporter can be displayed before the original reporter has
   a response from the main-thread. In such cases, keep the cloned
   reporter alive until the original reporter receives the main-thread
   response, so that the 'partial update' flag can be unset if
   necessary.

BUG=1115376

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

cc/metrics/compositor_frame_reporter.cc

@@ -309,7 +309,7 @@ CompositorFrameReporter::CompositorFrameReporter(
       smooth_thread_(smooth_thread) {}
 
 std::unique_ptr<CompositorFrameReporter>
-CompositorFrameReporter::CopyReporterAtBeginImplStage() const {
+CompositorFrameReporter::CopyReporterAtBeginImplStage() {
   if (stage_history_.empty() ||
       stage_history_.front().stage_type !=
           StageType::kBeginImplFrameToSendBeginMainFrame ||
@@ -327,6 +327,10 @@ CompositorFrameReporter::CopyReporterAtBeginImplStage() const {
   new_reporter->current_stage_.start_time = stage_history_.front().start_time;
   new_reporter->set_tick_clock(tick_clock_);
   new_reporter->SetDroppedFrameCounter(dropped_frame_counter_);
+  new_reporter->cloned_from_ = weak_factory_.GetWeakPtr();
+
+  DCHECK(!cloned_to_);
+  cloned_to_ = new_reporter->GetWeakPtr();
   return new_reporter;
 }
 
@@ -450,8 +454,17 @@ void CompositorFrameReporter::TerminateReporter() {
       EnableReportType(FrameReportType::kDroppedFrame);
       break;
     case FrameTerminationStatus::kDidNotProduceFrame:
-      if (!frame_skip_reason_.has_value() ||
-          frame_skip_reason() != FrameSkippedReason::kNoDamage) {
+      if (frame_skip_reason_.has_value() &&
+          frame_skip_reason() == FrameSkippedReason::kNoDamage) {
+        // If this reporter was cloned, and the cloned repoter was marked as
+        // containing 'partial update' (i.e. missing desired updates from the
+        // main-thread), but this reporter terminated with 'no damage', then
+        // reset the 'partial update' flag from the cloned reporter.
+        if (cloned_to_ && cloned_to_->has_partial_update())
+          cloned_to_->set_has_partial_update(false);
+      } else {
+        // If no frames were produced, it was not due to no-damage, then it is a
+        // dropped frame.
         EnableReportType(FrameReportType::kDroppedFrame);
       }
       break;
@@ -1074,4 +1087,14 @@ bool CompositorFrameReporter::IsDroppedFrameAffectingSmoothness() const {
   return false;
 }
 
+base::WeakPtr<CompositorFrameReporter> CompositorFrameReporter::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void CompositorFrameReporter::AdoptReporter(
+    std::unique_ptr<CompositorFrameReporter> reporter) {
+  DCHECK_EQ(cloned_to_.get(), reporter.get());
+  own_cloned_to_ = std::move(reporter);
+}
+
 }  // namespace cc

cc/metrics/compositor_frame_reporter.h

@@ -154,7 +154,7 @@ class CC_EXPORT CompositorFrameReporter {
   CompositorFrameReporter& operator=(const CompositorFrameReporter& reporter) =
       delete;
 
-  std::unique_ptr<CompositorFrameReporter> CopyReporterAtBeginImplStage() const;
+  std::unique_ptr<CompositorFrameReporter> CopyReporterAtBeginImplStage();
 
   // Note that the started stage may be reported to UMA. If the histogram is
   // intended to be reported then the histograms.xml file must be updated too.
@@ -201,10 +201,26 @@ class CC_EXPORT CompositorFrameReporter {
   void SetDroppedFrameCounter(DroppedFrameCounter* counter) {
     dropped_frame_counter_ = counter;
   }
-  void SetHasPartialUpdate() { has_partial_update_ = true; }
+
+  bool has_partial_update() const { return has_partial_update_; }
+  void set_has_partial_update(bool has_partial_update) {
+    has_partial_update_ = has_partial_update;
+  }
 
   const viz::BeginFrameId& frame_id() const { return args_.frame_id; }
 
+  // Adopts |cloned_reporter|, i.e. keeps |cloned_reporter| alive until after
+  // this reporter terminates. Note that the |cloned_reporter| must have been
+  // created from this reporter using |CopyReporterAtBeginImplStage()|.
+  void AdoptReporter(std::unique_ptr<CompositorFrameReporter> cloned_reporter);
+
+  // If this is a cloned reporter, then this returns a weak-ptr to the original
+  // reporter this was cloned from (using |CopyReporterAtBeginImplStage()|).
+  base::WeakPtr<CompositorFrameReporter> cloned_from() { return cloned_from_; }
+
+ protected:
+  base::WeakPtr<CompositorFrameReporter> GetWeakPtr();
+
  private:
   void TerminateReporter();
   void EndCurrentStage(base::TimeTicks end_time);
@@ -308,6 +324,21 @@ class CC_EXPORT CompositorFrameReporter {
   bool has_partial_update_ = false;
 
   const SmoothThread smooth_thread_;
+
+  // If this is a cloned pointer, then |cloned_from_| is a weak pointer to the
+  // original reporter this was cloned from.
+  base::WeakPtr<CompositorFrameReporter> cloned_from_;
+
+  // If this reporter was cloned, then |cloned_to_| is a weak pointer to the
+  // cloned repoter.
+  base::WeakPtr<CompositorFrameReporter> cloned_to_;
+
+  // A cloned reporter is not originally owned by the original reporter.
+  // However, it can 'adopt' it (using |AdoptReporter()| if the cloned reporter
+  // needs to stay alive until the original reporter terminates.
+  std::unique_ptr<CompositorFrameReporter> own_cloned_to_;
+
+  base::WeakPtrFactory<CompositorFrameReporter> weak_factory_{this};
 };
 
 }  // namespace cc

cc/metrics/compositor_frame_reporting_controller.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "cc/metrics/compositor_frame_reporter.h"
+#include "cc/metrics/dropped_frame_counter.h"
 #include "cc/metrics/latency_ukm_reporter.h"
 #include "components/viz/common/frame_timing_details.h"
 #include "components/viz/common/quads/compositor_frame_metadata.h"
@@ -215,7 +216,7 @@ void CompositorFrameReportingController::DidSubmitCompositorFrame(
       // 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();
+        reporter->set_has_partial_update(true);
       impl_reporter = std::move(reporter);
     }
   }
@@ -325,9 +326,25 @@ void CompositorFrameReportingController::DidPresentCompositorFrame(
       termination_status = FrameTerminationStatus::kDidNotPresentFrame;
     }
 
-    submitted_frame->reporter->SetVizBreakdown(details);
-    submitted_frame->reporter->TerminateFrame(
-        termination_status, details.presentation_feedback.timestamp);
+    auto& reporter = submitted_frame->reporter;
+    reporter->SetVizBreakdown(details);
+    reporter->TerminateFrame(termination_status,
+                             details.presentation_feedback.timestamp);
+
+    // If |reporter| was cloned from a reporter, and the original reporter is
+    // still alive, then check whether the cloned reporter has the 'partial
+    // update' flag set. It is still possible for the original reporter to
+    // terminate with 'no damage', and if that happens, then the cloned
+    // reporter's 'partial update' flag will need to be reset. To allow this to
+    // happen, keep the cloned reporter alive, and hand over its ownership to
+    // the original reporter, so that the cloned reporter stays alive until the
+    // original reporter is terminated, and the cloned reporter's 'partial
+    // update' flag can be unset if necessary.
+    if (reporter->has_partial_update()) {
+      auto orig_reporter = reporter->cloned_from();
+      if (orig_reporter)
+        orig_reporter->AdoptReporter(std::move(reporter));
+    }
     submitted_compositor_frames_.erase(submitted_frame);
   }
 }

cc/metrics/compositor_frame_reporting_controller_unittest.cc

@@ -711,8 +711,10 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame) {
                                                  {});
   reporting_controller_.DidPresentCompositorFrame(2, details);
 
+  // The reporting for the second frame is delayed until the main-thread
+  // responds back.
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 2);
+      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 1);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SendBeginMainFrameToCommit", 1);
   histogram_tester.ExpectTotalCount("CompositorLatency.Commit", 1);
@@ -720,12 +722,12 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame) {
                                     1);
   histogram_tester.ExpectTotalCount("CompositorLatency.Activation", 1);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.EndActivateToSubmitCompositorFrame", 2);
+      "CompositorLatency.EndActivateToSubmitCompositorFrame", 1);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SubmitCompositorFrameToPresentationCompositorFrame",
-      2);
+      1);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.CompositorOnlyFrame.BeginImplFrameToFinishImpl", 1);
+      "CompositorLatency.CompositorOnlyFrame.BeginImplFrameToFinishImpl", 0);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.CompositorOnlyFrame."
       "SendBeginMainFrameToBeginMainAbort",
@@ -733,11 +735,11 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame) {
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.CompositorOnlyFrame."
       "ImplFrameDoneToSubmitCompositorFrame",
-      1);
+      0);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.CompositorOnlyFrame."
       "SubmitCompositorFrameToPresentationCompositorFrame",
-      1);
+      0);
 
   reporting_controller_.WillBeginImplFrame(args_3);
   reporting_controller_.OnFinishImplFrame(current_id_3);
@@ -749,6 +751,8 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame) {
                                                  {});
   reporting_controller_.DidPresentCompositorFrame(3, details);
 
+  // The main-thread responded, so the metrics for |args_2| should now be
+  // reported.
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 4);
   histogram_tester.ExpectTotalCount(
@@ -812,8 +816,7 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame2) {
       "CompositorLatency.SubmitCompositorFrameToPresentationCompositorFrame",
       1);
 
-  // Second frame will not have the main frame update ready and will only submit
-  // the Impl update
+  // The reporting for the second frame is delayed until activation happens.
   reporting_controller_.WillBeginImplFrame(args_2);
   reporting_controller_.WillBeginMainFrame(args_2);
   reporting_controller_.WillCommit();
@@ -824,7 +827,7 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame2) {
   reporting_controller_.DidPresentCompositorFrame(2, details);
 
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 2);
+      "CompositorLatency.BeginImplFrameToSendBeginMainFrame", 1);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SendBeginMainFrameToCommit", 1);
   histogram_tester.ExpectTotalCount("CompositorLatency.Commit", 1);
@@ -832,12 +835,12 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame2) {
                                     1);
   histogram_tester.ExpectTotalCount("CompositorLatency.Activation", 1);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.EndActivateToSubmitCompositorFrame", 2);
+      "CompositorLatency.EndActivateToSubmitCompositorFrame", 1);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.SubmitCompositorFrameToPresentationCompositorFrame",
-      2);
+      1);
   histogram_tester.ExpectTotalCount(
-      "CompositorLatency.CompositorOnlyFrame.BeginImplFrameToFinishImpl", 1);
+      "CompositorLatency.CompositorOnlyFrame.BeginImplFrameToFinishImpl", 0);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.CompositorOnlyFrame."
       "SendBeginMainFrameToBeginMainAbort",
@@ -845,11 +848,51 @@ TEST_F(CompositorFrameReportingControllerTest, LongMainFrame2) {
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.CompositorOnlyFrame."
       "ImplFrameDoneToSubmitCompositorFrame",
-      1);
+      0);
   histogram_tester.ExpectTotalCount(
       "CompositorLatency.CompositorOnlyFrame."
       "SubmitCompositorFrameToPresentationCompositorFrame",
-      1);
+      0);
+
+  viz::BeginFrameId current_id_3(1, 3);
+  viz::BeginFrameArgs args_3 = SimulateBeginFrameArgs(current_id_3);
+
+  // The metrics are reported for |args_2| after activation finally happens and
+  // a new frame is submitted.
+  reporting_controller_.WillActivate();
+  reporting_controller_.DidActivate();
+  reporting_controller_.WillBeginImplFrame(args_3);
+  reporting_controller_.OnFinishImplFrame(current_id_3);
+  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);
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.CompositorOnlyFrame.BeginImplFrameToFinishImpl", 2);
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.CompositorOnlyFrame."
+      "SendBeginMainFrameToBeginMainAbort",
+      0);
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.CompositorOnlyFrame."
+      "ImplFrameDoneToSubmitCompositorFrame",
+      2);
+  histogram_tester.ExpectTotalCount(
+      "CompositorLatency.CompositorOnlyFrame."
+      "SubmitCompositorFrameToPresentationCompositorFrame",
+      2);
 }
 
 TEST_F(CompositorFrameReportingControllerTest, BlinkBreakdown) {
@@ -1279,8 +1322,6 @@ TEST_F(CompositorFrameReportingControllerTest,
   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();

cc/metrics/dropped_frame_counter_unittest.cc

@@ -7,6 +7,8 @@
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "cc/animation/animation_host.h"
+#include "cc/test/fake_content_layer_client.h"
+#include "cc/test/fake_picture_layer.h"
 #include "cc/test/layer_tree_test.h"
 
 namespace cc {
@@ -23,6 +25,18 @@ class DroppedFrameCounterTestBase : public LayerTreeTest {
     settings->commit_to_active_tree = false;
   }
 
+  void SetupTree() override {
+    LayerTreeTest::SetupTree();
+
+    Layer* root_layer = layer_tree_host()->root_layer();
+    scroll_layer_ = FakePictureLayer::Create(&client_);
+    // Set up the layer so it always has something to paint.
+    scroll_layer_->set_always_update_resources(true);
+    scroll_layer_->SetBounds({3, 3});
+    client_.set_bounds({3, 3});
+    root_layer->AddChild(scroll_layer_);
+  }
+
   void RunTest(CompositorMode mode) override {
     SetUpTestConfigAndExpectations();
     LayerTreeTest::RunTest(mode);
@@ -49,13 +63,20 @@ class DroppedFrameCounterTestBase : public LayerTreeTest {
   // Compositor thread function overrides:
   void WillBeginImplFrameOnThread(LayerTreeHostImpl* host_impl,
                                   const viz::BeginFrameArgs& args) override {
+    if (TestEnded())
+      return;
+
     // Request a re-draw, and set a non-empty damage region (otherwise the
     // draw is aborted with 'no damage').
     host_impl->SetNeedsRedraw();
     host_impl->SetViewportDamage(gfx::Rect(0, 0, 10, 20));
 
-    // Request update from the main-thread too.
-    host_impl->SetNeedsCommit();
+    if (skip_main_thread_next_frame_) {
+      skip_main_thread_next_frame_ = false;
+    } else {
+      // Request update from the main-thread too.
+      host_impl->SetNeedsCommit();
+    }
   }
 
   void DrawLayersOnThread(LayerTreeHostImpl* host_impl) override {
@@ -68,6 +89,10 @@ class DroppedFrameCounterTestBase : public LayerTreeTest {
     }
 
     if (wait) {
+      // When the main-thread blocks during a frame, skip the main-thread for
+      // the next frame, so that the main-thread can be in sync with the
+      // compositor thread again.
+      skip_main_thread_next_frame_ = true;
       wait->Signal();
     }
   }
@@ -92,6 +117,9 @@ class DroppedFrameCounterTestBase : public LayerTreeTest {
 
   // Main-thread function overrides:
   void BeginMainFrame(const viz::BeginFrameArgs& args) override {
+    if (TestEnded())
+      return;
+
     bool should_wait = false;
     if (config_.should_drop_main_every > 0) {
       should_wait =
@@ -110,12 +138,14 @@ class DroppedFrameCounterTestBase : public LayerTreeTest {
         base::AutoLock lock(wait_lock_);
         wait_ = nullptr;
       }
-    } else if (!TestEnded()) {
-      // Make sure BeginMainFrame keeps being issued.
-      layer_tree_host()->SetNeedsAnimate();
-      if (config_.should_register_main_thread_animation) {
-        animation_host()->SetAnimationCounts(1, true, true);
-      }
+    }
+
+    // Make some changes so that the main-thread needs to push updates to the
+    // compositor thread (i.e. force a commit).
+    auto const bounds = scroll_layer_->bounds();
+    scroll_layer_->SetBounds({bounds.width(), bounds.height() + 1});
+    if (config_.should_register_main_thread_animation) {
+      animation_host()->SetAnimationCounts(1, true, true);
     }
   }
 
@@ -139,6 +169,12 @@ class DroppedFrameCounterTestBase : public LayerTreeTest {
   } expect_;
 
  private:
+  // Set up a dummy picture layer so that every begin-main frame requires a
+  // commit (without the dummy layer, the main-thread never has to paint, which
+  // causes an early 'no damage' abort of the main-frame.
+  FakeContentLayerClient client_;
+  scoped_refptr<FakePictureLayer> scroll_layer_;
+
   // This field is used only on the compositor thread to track how many frames
   // have been processed.
   uint32_t presented_frames_ = 0;
@@ -155,6 +191,8 @@ class DroppedFrameCounterTestBase : public LayerTreeTest {
   uint32_t dropped_main_ = 0;
   uint32_t dropped_compositor_ = 0;
   uint32_t dropped_smoothness_ = 0;
+
+  bool skip_main_thread_next_frame_ = false;
 };
 
 class DroppedFrameCounterNoDropTest : public DroppedFrameCounterTestBase {