Allow one undrawn frame in ShouldSendBeginFrame

If the renderer ever takes longer than the display deadline to submit
a frame, the previous logic would cause us to throttle one renderer
frame, leading to additional jank.

While this does allow for some "catch-up", we've seen cases where
this throttling can happen multiple times in a short period (<1s). In
these cases the throttling just adds additional jank.

Bug: 935630
Change-Id: I7c2c6d562ee91fbae316185e9e54385a1eebfc08
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1521967
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#642186}

components/viz/service/display/display_unittest.cc

@@ -3392,6 +3392,19 @@ TEST_F(DisplayTest, BeginFrameThrottling) {
       1.f);
   support_->SetNeedsBeginFrame(true);
 
+  // Helper fn to submit a CF.
+  auto submit_frame = [this](RenderPassList* pass_list) {
+    auto pass = RenderPass::Create();
+    pass->output_rect = gfx::Rect(0, 0, 100, 100);
+    pass->damage_rect = gfx::Rect(10, 10, 1, 1);
+    pass->id = 1u;
+    pass_list->push_back(std::move(pass));
+
+    SubmitCompositorFrame(
+        pass_list,
+        id_allocator_.GetCurrentLocalSurfaceIdAllocation().local_surface_id());
+  };
+
   // BeginFrame should not be throttled when the client has not submitted any
   // compositor frames.
   base::TimeTicks frame_time = base::TimeTicks::Now();
@@ -3401,15 +3414,7 @@ TEST_F(DisplayTest, BeginFrameThrottling) {
   // Submit the first frame for the client. Begin-frame should still not be
   // throttled since it has not been embedded yet.
   RenderPassList pass_list;
-  auto pass = RenderPass::Create();
-  pass->output_rect = gfx::Rect(0, 0, 100, 100);
-  pass->damage_rect = gfx::Rect(10, 10, 1, 1);
-  pass->id = 1u;
-  pass_list.push_back(std::move(pass));
-
-  SubmitCompositorFrame(
-      &pass_list,
-      id_allocator_.GetCurrentLocalSurfaceIdAllocation().local_surface_id());
+  submit_frame(&pass_list);
   frame_time = base::TimeTicks::Now();
   EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
   UpdateBeginFrameTime(support_.get(), frame_time);
@@ -3419,17 +3424,17 @@ TEST_F(DisplayTest, BeginFrameThrottling) {
   EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
   UpdateBeginFrameTime(support_.get(), frame_time);
 
-  // Submit a second frame. This time, begin-frame should be throttled after the
-  // first begin-frame is sent because of presentation-feedbacks, until the next
-  // draw happens.
-  pass = RenderPass::Create();
-  pass->output_rect = gfx::Rect(0, 0, 100, 100);
-  pass->damage_rect = gfx::Rect(10, 10, 1, 1);
-  pass->id = 1u;
-  pass_list.push_back(std::move(pass));
-  SubmitCompositorFrame(
-      &pass_list,
-      id_allocator_.GetCurrentLocalSurfaceIdAllocation().local_surface_id());
+  // Submit a second frame. This frame should not be throttled, even after
+  // presentation-feedbacks, as we allow up to two undrawn frames.
+  submit_frame(&pass_list);
+  frame_time = base::TimeTicks::Now();
+  EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
+  UpdateBeginFrameTime(support_.get(), frame_time);
+  EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
+
+  // Submit a third frame. This frame should be throttled after
+  // presentation-feedbacks, as we throttle at two undrawn frames.
+  submit_frame(&pass_list);
   frame_time = base::TimeTicks::Now();
   EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
   UpdateBeginFrameTime(support_.get(), frame_time);
@@ -3441,16 +3446,13 @@ TEST_F(DisplayTest, BeginFrameThrottling) {
   EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
   UpdateBeginFrameTime(support_.get(), frame_time);
 
-  // Submit a third frame. Again, begin-frame should be throttled after the
+  // Submit two more frames. Begin-frame should be throttled after the
   // begin-frame for presenatation-feedback.
-  pass = RenderPass::Create();
-  pass->output_rect = gfx::Rect(0, 0, 100, 100);
-  pass->damage_rect = gfx::Rect(10, 10, 1, 1);
-  pass->id = 1u;
-  pass_list.push_back(std::move(pass));
-  SubmitCompositorFrame(
-      &pass_list,
-      id_allocator_.GetCurrentLocalSurfaceIdAllocation().local_surface_id());
+  submit_frame(&pass_list);
+  frame_time = base::TimeTicks::Now();
+  EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
+  UpdateBeginFrameTime(support_.get(), frame_time);
+  submit_frame(&pass_list);
   frame_time = base::TimeTicks::Now();
   EXPECT_TRUE(ShouldSendBeginFrame(support_.get(), frame_time));
   UpdateBeginFrameTime(support_.get(), frame_time);

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -150,6 +150,10 @@ void CompositorFrameSinkSupport::OnSurfaceActivated(Surface* surface) {
   MaybeEvictSurfaces();
 }
 
+void CompositorFrameSinkSupport::OnSurfaceDrawn(Surface* surface) {
+  last_drawn_frame_index_ = surface->GetActiveFrameIndex();
+}
+
 void CompositorFrameSinkSupport::OnFrameTokenChanged(uint32_t frame_token) {
   frame_sink_manager_->OnFrameTokenChanged(frame_sink_id_, frame_token);
 }
@@ -760,12 +764,30 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
   if (!surface || !surface->seen_first_surface_embedding())
     return true;
 
-  // Send begin-frames if the client has requested for it, and the previously
-  // submitted frame has already been drawn, or if the previous begin-frame was
-  // sent more than 1 second ago.
+  // If the embedded surface doesn't have an active frame, send begin frame.
+  if (!surface->HasActiveFrame())
+    return true;
+
+  uint64_t active_frame_index = surface->GetActiveFrameIndex();
+
+  // Since we have an active frame, and frame indexes strictly increase during
+  // the lifetime of the CompositorFrameSinkSupport, our active frame index
+  // must be at least as large as our last drawn frame index.
+  DCHECK_GE(active_frame_index, last_drawn_frame_index_);
+
+  // Determine the number of undrawn frames. If this is below our limit, send
+  // begin frame. Limit must be at least 1, as the relative ordering of
+  // renderer / browser frame submissions allows us to have one outstanding
+  // undrawn frame under normal operation.
+  constexpr uint64_t undrawn_frame_limit = 1;
+  uint64_t num_undrawn_frames = active_frame_index - last_drawn_frame_index_;
+  if (num_undrawn_frames <= undrawn_frame_limit)
+    return true;
+
+  // Send begin-frames if the previous begin-frame was sent more than 1 second
+  // ago.
   constexpr base::TimeDelta throttled_rate = base::TimeDelta::FromSeconds(1);
-  return !surface->HasUndrawnActiveFrame() ||
-         (frame_time - last_frame_time_) >= throttled_rate;
+  return (frame_time - last_frame_time_) >= throttled_rate;
 }
 
 }  // namespace viz

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -101,6 +101,7 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   // SurfaceClient implementation.
   void OnSurfaceActivated(Surface* surface) override;
   void OnSurfaceDiscarded(Surface* surface) override;
+  void OnSurfaceDrawn(Surface* surface) override;
   void RefResources(
       const std::vector<TransferableResource>& resources) override;
   void UnrefResources(const std::vector<ReturnedResource>& resources) override;
@@ -311,6 +312,12 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
 
   base::TimeTicks last_frame_time_;
 
+  // Initialize |last_drawn_frame_index_| as though the frame before the first
+  // has been drawn.
+  static_assert(kFrameIndexStart > 1,
+                "|last_drawn_frame_index| relies on kFrameIndexStart > 1");
+  uint64_t last_drawn_frame_index_ = kFrameIndexStart - 1;
+
   base::WeakPtrFactory<CompositorFrameSinkSupport> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CompositorFrameSinkSupport);

components/viz/service/surfaces/surface.cc

@@ -699,8 +699,11 @@ void Surface::SendAckToClient() {
 }
 
 void Surface::MarkAsDrawn() {
-  if (active_frame_data_)
-    active_frame_data_->frame_drawn = true;
+  if (!active_frame_data_)
+    return;
+  active_frame_data_->frame_drawn = true;
+  if (surface_client_)
+    surface_client_->OnSurfaceDrawn(this);
 }
 
 void Surface::NotifyAggregatedDamage(const gfx::Rect& damage_rect,

components/viz/service/surfaces/surface_client.h

@@ -27,6 +27,9 @@ class VIZ_SERVICE_EXPORT SurfaceClient {
   // Called when |surface| is about to be destroyed.
   virtual void OnSurfaceDiscarded(Surface* surface) = 0;
 
+  // Called when a |surface| is about to be drawn.
+  virtual void OnSurfaceDrawn(Surface* surface) = 0;
+
   // Increments the reference count on resources specified by |resources|.
   virtual void RefResources(
       const std::vector<TransferableResource>& resources) = 0;