viz: Relax child throttling

https://crrev.com/c/1539860 changed child throttling to allow only two
unembedded surfaces. However, this seems to have regressed latency so
revert back to allowing three unemedded surfaces.

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

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -3496,6 +3496,7 @@ TEST_F(SurfaceSynchronizationTest,
   const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1, 1);
   const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 1, 2);
   const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink1, 1, 3);
+  const SurfaceId child_id4 = MakeSurfaceId(kChildFrameSink1, 1, 4);
 
   // |child_id1| Surface should immediately activate because one unembedded
   // surface is allowed.
@@ -3529,8 +3530,8 @@ TEST_F(SurfaceSynchronizationTest,
   EXPECT_FALSE(child_surface2->HasPendingFrame());
   EXPECT_TRUE(child_surface2->HasActiveFrame());
 
-  // The child submits to |child_id3|. Now again we have two unembedded surface
-  // so throttling should kick in.
+  // The child submits to |child_id3|. Throttling should still not kick in due
+  // to https://crbug.com/951992.
   child_support1().SubmitCompositorFrame(
       child_id3.local_surface_id(),
       MakeCompositorFrame(empty_surface_ids(), empty_surface_ranges(),
@@ -3538,8 +3539,19 @@ TEST_F(SurfaceSynchronizationTest,
                           MakeDeadline(1u)));
   Surface* child_surface3 = GetSurfaceForId(child_id3);
   ASSERT_NE(nullptr, child_surface3);
-  EXPECT_TRUE(child_surface3->HasPendingFrame());
-  EXPECT_FALSE(child_surface3->HasActiveFrame());
+  EXPECT_FALSE(child_surface3->HasPendingFrame());
+  EXPECT_TRUE(child_surface3->HasActiveFrame());
+
+  // The child submits to |child_id4|. Throttling should kick in.
+  child_support1().SubmitCompositorFrame(
+      child_id4.local_surface_id(),
+      MakeCompositorFrame(empty_surface_ids(), empty_surface_ranges(),
+                          std::vector<TransferableResource>(),
+                          MakeDeadline(1u)));
+  Surface* child_surface4 = GetSurfaceForId(child_id4);
+  ASSERT_NE(nullptr, child_surface4);
+  EXPECT_TRUE(child_surface4->HasPendingFrame());
+  EXPECT_FALSE(child_surface4->HasActiveFrame());
 }
 
 }  // namespace viz

components/viz/service/surfaces/surface.cc

@@ -744,17 +744,4 @@ void Surface::ActivatePendingFrameForInheritedDeadline() {
   ActivatePendingFrameForDeadline();
 }
 
-void Surface::ResetBlockActivationOnParent() {
-  if (!block_activation_on_parent_)
-    return;
-
-  block_activation_on_parent_ = false;
-
-  if (!activation_dependencies_.empty() || !pending_frame_data_)
-    return;
-
-  // All blockers have been cleared. The surface can be activated now.
-  ActivatePendingFrame();
-}
-
 }  // namespace viz

components/viz/service/surfaces/surface.h

@@ -231,9 +231,6 @@ class VIZ_SERVICE_EXPORT Surface final {
   void OnActivationDependencyResolved(const SurfaceId& activation_dependency,
                                       SurfaceAllocationGroup* group);
 
-  // Called when this surface's activation no longer has to block on the parent.
-  void ResetBlockActivationOnParent();
-
   // Notifies that this surface is no longer the primary surface of the
   // embedder. All future CompositorFrames will activate as soon as they arrive
   // and if a pending frame currently exists it will immediately activate as
@@ -312,8 +309,13 @@ class VIZ_SERVICE_EXPORT Surface final {
   base::Optional<FrameData> active_frame_data_;
   bool seen_first_frame_activation_ = false;
   bool seen_first_surface_embedding_ = false;
+  // Indicates whether another surface adds this surface as a dependency. When
+  // set to true, this surface will be unthrottled and the surface that is
+  // created after it will also not be throttled.
   bool seen_first_surface_dependency_ = false;
   const bool needs_sync_tokens_;
+  // When false, this surface will not be subject to child throttling even if
+  // it's not embedded yet.
   bool block_activation_on_parent_ = false;
 
   // A set of all valid SurfaceIds contained |last_surface_id_for_range_| to

components/viz/service/surfaces/surface_allocation_group.cc

@@ -255,7 +255,11 @@ void SurfaceAllocationGroup::UpdateLastReferenceAndMaybeActivate(
   ++it;
   if (it == surfaces_.end())
     return;
-  (*it)->ResetBlockActivationOnParent();
+  // This ensures the next surface has its seen_first_surface_dependency_ bit
+  // set so that throttling doesn't kick in until 3 surfaces after the surface
+  // that was just embedded. We see regression if throttling kicks in sooner.
+  // See https://crbug.com/951992.
+  (*it)->OnSurfaceDependencyAdded();
 }
 
 }  // namespace viz