viz: Immediately ack clients that submit later than display deadline

When a client misses the display deadline, it should not be blocked
until the next DrawAndSwap because it will forever lag behind the
display by one frame:
- Renderer submits CompositorFrame after display deadline.
- On next BeginFrame, display schedules an immediate deadline because
  the renderer has an undrawn frame.
- Since the renderer can't beat display's immediate deadline, its
  CompositorFrame comes after display's deadline again. Repeat.
Instead, we should immediately ack the client when it misses the
display deadline.

Bug: 951992
Change-Id: I672912c0cef346dd29aa9d6b2e9deb8eb04a4c7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1588436
Auto-Submit: Saman Sami <samans@chromium.org>
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#655983}

components/viz/service/display/display_scheduler.cc

@@ -332,7 +332,7 @@ bool DisplayScheduler::OnSurfaceDamaged(const SurfaceId& surface_id,
   bool damaged = client_->SurfaceDamaged(surface_id, ack);
   ProcessSurfaceDamage(surface_id, ack, damaged);
 
-  return damaged;
+  return damaged && inside_begin_frame_deadline_interval_;
 }
 
 void DisplayScheduler::OnSurfaceDestroyed(const SurfaceId& surface_id) {

components/viz/service/display/display_scheduler_unittest.cc

@@ -48,7 +48,7 @@ class FakeDisplaySchedulerClient : public DisplaySchedulerClient {
 
   bool SurfaceDamaged(const SurfaceId& surface_id,
                       const BeginFrameAck& ack) override {
-    return false;
+    return surface_damaged_return_value_;
   }
 
   void SurfaceDestroyed(const SurfaceId& surface_id) override {}
@@ -61,6 +61,10 @@ class FakeDisplaySchedulerClient : public DisplaySchedulerClient {
 
   void SetNextDrawAndSwapFails() { next_draw_and_swap_fails_ = true; }
 
+  void SetSurfaceDamagedReturnValue(bool surface_damaged_return_value) {
+    surface_damaged_return_value_ = surface_damaged_return_value;
+  }
+
   void SurfaceDamaged(const SurfaceId& surface_id) {
     undrawn_surfaces_.insert(surface_id);
   }
@@ -70,6 +74,7 @@ class FakeDisplaySchedulerClient : public DisplaySchedulerClient {
  protected:
   int draw_and_swap_count_;
   bool next_draw_and_swap_fails_;
+  bool surface_damaged_return_value_ = false;
   std::set<SurfaceId> undrawn_surfaces_;
   BeginFrameAck last_begin_frame_ack_;
 };
@@ -791,5 +796,35 @@ TEST_F(DisplaySchedulerTest, SetNeedsOneBeginFrame) {
   EXPECT_FALSE(scheduler_.inside_begin_frame_deadline_interval());
 }
 
+// This test verifies that if a client submits a frame after display scheduler's
+// deadline, it is acked immediately. https://crbug.com/951992
+TEST_F(DisplaySchedulerTest, AckImmediatelyAfterDeadline) {
+  SurfaceId root_surface_id(
+      kArbitraryFrameSinkId,
+      LocalSurfaceId(1, base::UnguessableToken::Create()));
+  BeginFrameAck ack;
+  ack.has_damage = true;
+
+  // Make the display always report that it is damaged in SurfaceDamaged.
+  client().SetSurfaceDamagedReturnValue(true);
+
+  scheduler_.SetNewRootSurface(root_surface_id);
+  scheduler_.SetVisible(true);
+
+  // Send a BeginFrame and pretend the client responded BEFORE the display
+  // scheduler deadline. OnSurfaceDamaged should return true, meaning that the
+  // ack should not be sent until display deadline.
+  AdvanceTimeAndBeginFrameForTest({root_surface_id});
+  EXPECT_TRUE(scheduler_.OnSurfaceDamaged(root_surface_id, ack));
+  scheduler_.BeginFrameDeadlineForTest();
+
+  // Send a BeginFrame and pretend the client responded AFTER the display
+  // scheduler deadline. OnSurfaceDamaged should return false, meaning that the
+  // ack should be sent immediately.
+  AdvanceTimeAndBeginFrameForTest({root_surface_id});
+  scheduler_.BeginFrameDeadlineForTest();
+  EXPECT_FALSE(scheduler_.OnSurfaceDamaged(root_surface_id, ack));
+}
+
 }  // namespace
 }  // namespace viz

components/viz/test/test_layer_tree_frame_sink.cc

@@ -148,6 +148,7 @@ void TestLayerTreeFrameSink::DetachFromClient() {
   frame_sink_manager_ = nullptr;
   shared_bitmap_manager_ = nullptr;
   test_client_ = nullptr;
+  weak_ptr_factory_.InvalidateWeakPtrs();
   LayerTreeFrameSink::DetachFromClient();
 }
 
@@ -222,7 +223,11 @@ void TestLayerTreeFrameSink::DidReceiveCompositorFrameAck(
   // used.
   if (!display_->has_scheduler())
     return;
-  client_->DidReceiveCompositorFrameAck();
+  // Do a PostTask, because the cc::Scheduler doesn't like a synchronous ack.
+  compositor_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&TestLayerTreeFrameSink::SendCompositorFrameAckToClient,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void TestLayerTreeFrameSink::OnBeginFrame(