viz: Eliminate closing of surfaces

There are a few issues with current implementation of closing surfaces:
1) It rejects any CompositorFrame that comes to the fallback surface.
I'm not sure this is a good idea, because even though the frame is not
for the primary surface, it still allows us to have something to show
for the next display frame. What we really want is activating the frame
immediately so that an ack is sent back to the client and it is
unblocked for the next frame.
2) Closing the surface currently only affects future CompositorFrames.
If a pending frame already exists, it is not activated.
3) It only closes the first surface in the range. We should close all
surfaces in the range other than the last surface.

This CL replaces closing surfaces with a different mechanism that
marks surfaces as fallback and prevents them from ever blocking.

Bug: 938947
Change-Id: Ida86143427f9bad1bb323e6edd9b45116e813735
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1558452
Commit-Queue: Saman Sami <samans@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#651855}

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -1580,39 +1580,34 @@ TEST_F(SurfaceSynchronizationTest, MultiLevelLateArrivingDependency) {
 }
 
 // This test verifies that CompositorFrames submitted to a surface referenced
-// by a parent CompositorFrame as a fallback will be rejected and ACK'ed
-// immediately.
+// by a parent CompositorFrame as a fallback will be ACK'ed immediately.
 TEST_F(SurfaceSynchronizationTest, FallbackSurfacesClosed) {
   const SurfaceId parent_id1 = MakeSurfaceId(kParentFrameSink, 1);
   // This is the fallback child surface that the parent holds a reference to.
   const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
   // This is the primary child surface that the parent wants to block on.
   const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
+  const SurfaceId arbitrary_id = MakeSurfaceId(kChildFrameSink2, 3);
 
-  // child_support1 submits a CompositorFrame without any dependencies.
-  // DidReceiveCompositorFrameAck should call on immediate activation.
-  // However, resources will not be returned because this frame is a candidate
-  // for display.
-  TransferableResource resource;
-  resource.id = 1337;
-  resource.format = ALPHA_8;
-  resource.filter = 1234;
-  resource.size = gfx::Size(1234, 5678);
-  std::vector<ReturnedResource> returned_resources =
-      TransferableResource::ReturnResources({resource});
+  SendNextBeginFrame();
 
-  EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(
-                                   Eq(std::vector<ReturnedResource>())));
+  // child_support1 submits a CompositorFrame with unresolved dependencies.
+  // DidReceiveCompositorFrameAck should not be called because the frame hasn't
+  // activated yet.
+  EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_)).Times(0);
   child_support1().SubmitCompositorFrame(
       child_id1.local_surface_id(),
-      MakeCompositorFrame(empty_surface_ids(), empty_surface_ranges(),
-                          {resource}, MakeDefaultDeadline()));
-  EXPECT_FALSE(child_surface1()->has_deadline());
+      MakeCompositorFrame({arbitrary_id},
+                          {SurfaceRange(base::nullopt, arbitrary_id)}, {},
+                          MakeDefaultDeadline()));
+  EXPECT_TRUE(child_surface1()->has_deadline());
+  EXPECT_TRUE(child_surface1()->HasPendingFrame());
+  EXPECT_FALSE(child_surface1()->HasActiveFrame());
   testing::Mock::VerifyAndClearExpectations(&support_client_);
 
-  // The parent is blocked on |child_id2| and references |child_id1|. The
-  // surface corresponding to |child_id1| will not accept new CompositorFrames
-  // while the parent CompositorFrame is blocked.
+  // The parent is blocked on |child_id2| and references |child_id1|.
+  // |child_id1| should immediately activate and the ack must be sent.
+  EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_));
   parent_support().SubmitCompositorFrame(
       parent_id1.local_surface_id(),
       MakeCompositorFrame({child_id2}, {SurfaceRange(child_id1, child_id2)},
@@ -1621,43 +1616,22 @@ TEST_F(SurfaceSynchronizationTest, FallbackSurfacesClosed) {
   EXPECT_TRUE(parent_surface()->has_deadline());
   EXPECT_TRUE(parent_surface()->HasPendingFrame());
   EXPECT_FALSE(parent_surface()->HasActiveFrame());
-
-  // Resources will be returned immediately because |child_id1|'s surface is
-  // closed.
-  TransferableResource resource2;
-  resource2.id = 1246;
-  resource2.format = ALPHA_8;
-  resource2.filter = 1357;
-  resource2.size = gfx::Size(8765, 4321);
-  std::vector<ReturnedResource> returned_resources2 =
-      TransferableResource::ReturnResources({resource2});
-  EXPECT_CALL(support_client_,
-              DidReceiveCompositorFrameAck(Eq(returned_resources2)));
-  child_support1().SubmitCompositorFrame(
-      child_id1.local_surface_id(),
-      MakeCompositorFrame(empty_surface_ids(), empty_surface_ranges(),
-                          {resource2}, MakeDefaultDeadline()));
+  EXPECT_FALSE(child_surface1()->HasPendingFrame());
+  EXPECT_TRUE(child_surface1()->HasActiveFrame());
   testing::Mock::VerifyAndClearExpectations(&support_client_);
 
-  // Advance BeginFrames to trigger a deadline. This activates the
-  // CompositorFrame submitted to the parent.
-  for (int i = 0; i < 3; ++i) {
-    SendNextBeginFrame();
-    EXPECT_TRUE(parent_surface()->has_deadline());
-  }
+  // Any further CompositorFrames sent to |child_id1| will also activate
+  // immediately so that the child can submit another frame and catch up with
+  // the parent.
   SendNextBeginFrame();
-  EXPECT_FALSE(parent_surface()->has_deadline());
-  EXPECT_FALSE(parent_surface()->HasPendingFrame());
-  EXPECT_TRUE(parent_surface()->HasActiveFrame());
-
-  // Resources will be returned immediately because |child_id1|'s surface is
-  // closed forever.
-  EXPECT_CALL(support_client_,
-              DidReceiveCompositorFrameAck(Eq(returned_resources2)));
+  EXPECT_CALL(support_client_, DidReceiveCompositorFrameAck(_));
   child_support1().SubmitCompositorFrame(
       child_id1.local_surface_id(),
-      MakeCompositorFrame(empty_surface_ids(), empty_surface_ranges(),
-                          {resource2}, MakeDefaultDeadline()));
+      MakeCompositorFrame({arbitrary_id},
+                          {SurfaceRange(base::nullopt, arbitrary_id)}, {},
+                          MakeDefaultDeadline()));
+  EXPECT_FALSE(child_surface1()->HasPendingFrame());
+  EXPECT_TRUE(child_surface1()->HasActiveFrame());
   testing::Mock::VerifyAndClearExpectations(&support_client_);
 }
 

components/viz/service/surfaces/surface.cc

@@ -41,6 +41,8 @@ Surface::Surface(const SurfaceInfo& surface_info,
                            "Surface", this, "surface_info",
                            surface_info.ToString());
   allocation_group_->RegisterSurface(this);
+  is_fallback_ =
+      allocation_group_->GetLastReference().IsNewerThan(surface_id());
 }
 
 Surface::~Surface() {
@@ -89,31 +91,6 @@ void Surface::UnrefResources(const std::vector<ReturnedResource>& resources) {
     surface_client_->UnrefResources(resources);
 }
 
-void Surface::RejectCompositorFramesToFallbackSurfaces() {
-  for (const SurfaceRange& surface_range :
-       GetPendingFrame().metadata.referenced_surfaces) {
-    // Only close the fallback surface if it exists, has a different
-    // LocalSurfaceId than the primary surface but has the same FrameSinkId
-    // as the primary surface.
-    if (!surface_range.start() ||
-        surface_range.start() == surface_range.end() ||
-        surface_range.start()->frame_sink_id() !=
-            surface_range.end().frame_sink_id()) {
-      continue;
-    }
-    Surface* fallback_surface =
-        surface_manager_->GetLatestInFlightSurface(surface_range);
-
-    // A misbehaving client may report a non-existent surface ID as a
-    // |referenced_surface|. In that case, |surface| would be nullptr, and
-    // there is nothing to do here.
-    if (fallback_surface &&
-        fallback_surface->surface_id() != surface_range.end()) {
-      fallback_surface->Close();
-    }
-  }
-}
-
 void Surface::UpdateSurfaceReferences() {
   const base::flat_set<SurfaceId>& existing_referenced_surfaces =
       surface_manager_->GetSurfacesReferencedByParent(surface_id());
@@ -202,8 +179,10 @@ void Surface::OnSurfaceDependencyAdded() {
   ActivatePendingFrame(base::nullopt);
 }
 
-void Surface::Close() {
-  closed_ = true;
+void Surface::SetIsFallbackAndMaybeActivate() {
+  is_fallback_ = true;
+  if (HasPendingFrame())
+    ActivatePendingFrameForDeadline(base::nullopt);
 }
 
 bool Surface::QueueFrame(
@@ -218,9 +197,6 @@ bool Surface::QueueFrame(
     return false;
   }
 
-  if (closed_)
-    return true;
-
   is_latency_info_taken_ = false;
 
   if (active_frame_data_ || pending_frame_data_)
@@ -255,7 +231,6 @@ bool Surface::QueueFrame(
   } else {
     pending_frame_data_ =
         FrameData(std::move(frame), frame_index, std::move(presented_callback));
-    RejectCompositorFramesToFallbackSurfaces();
 
     // If the deadline is in the past, then the CompositorFrame will activate
     // immediately.
@@ -481,14 +456,18 @@ void Surface::ActivateFrame(FrameData frame_data,
 
 FrameDeadline Surface::ResolveFrameDeadline(
     const CompositorFrame& current_frame) {
+  // Fallback surfaces should activate immediately so that the client receives
+  // the ack and can submit a frame to the primary surface.
+  if (is_fallback_)
+    return FrameDeadline::MakeZero();
+
   // If there is an embedder of this surface that has already activated, that
   // means the embedder doesn't wish to block on this surface, i.e. either it
   // had a zero deadline or its deadline has already passed. If we don't have an
   // active frame already, active this frame immediately so we have something to
   // show.
   if (!HasActiveFrame() &&
-      allocation_group_->GetLastActiveReference().IsSameOrNewerThan(
-          surface_id())) {
+      allocation_group_->GetLastActiveReference() == surface_id()) {
     return FrameDeadline::MakeZero();
   }
 

components/viz/service/surfaces/surface.h

@@ -238,6 +238,13 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
   // 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
+  // well. This allows the client to not wait for acks from the fallback
+  // surfaces and be able to submit to the primary surface.
+  void SetIsFallbackAndMaybeActivate();
+
  private:
   struct FrameData {
     FrameData(CompositorFrame&& frame,
@@ -256,20 +263,11 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
     PresentedCallback presented_callback;
   };
 
-  // Rejects CompositorFrames submitted to surfaces referenced from this
-  // CompositorFrame as fallbacks. This saves some CPU cycles to allow
-  // children to catch up to the parent.
-  void RejectCompositorFramesToFallbackSurfaces();
-
   // Updates surface references of the surface using the referenced
   // surfaces from the most recent CompositorFrame.
   // Modifies surface references stored in SurfaceManager.
   void UpdateSurfaceReferences();
 
-  // Called to prevent additional CompositorFrames from being accepted into this
-  // surface. Once a Surface is closed, it cannot accept CompositorFrames again.
-  void Close();
-
   // Updates the set of allocation groups referenced by the active frame. Calls
   // RegisterEmbedder and UnregisterEmbedder on the allocation groups as
   // appropriate.
@@ -314,7 +312,6 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
 
   base::Optional<FrameData> pending_frame_data_;
   base::Optional<FrameData> active_frame_data_;
-  bool closed_ = false;
   bool seen_first_frame_activation_ = false;
   bool seen_first_surface_embedding_ = false;
   bool seen_first_surface_dependency_ = false;
@@ -344,6 +341,8 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
   // surface to let us know.
   base::flat_set<SurfaceAllocationGroup*> blocking_allocation_groups_;
 
+  bool is_fallback_ = false;
+
   bool is_latency_info_taken_ = false;
 
   SurfaceAllocationGroup* const allocation_group_;

components/viz/service/surfaces/surface_allocation_group.cc

@@ -241,7 +241,17 @@ void SurfaceAllocationGroup::UpdateLastReferenceAndMaybeActivate(
   auto it = FindLatestSurfaceUpTo(surface_id);
   if (it == surfaces_.end())
     return;
+  // Notify that |*it| is now embedded. This might unblock |*it| if it was
+  // blocked due to child throttling.
   (*it)->OnSurfaceDependencyAdded();
+  // If |surface_id| does not exist yet, notify the surface immediately prior to
+  // it that it is a fallback. This might activate the surface immediately
+  // because fallback surfaces never block.
+  if ((*it)->surface_id() != surface_id)
+    (*it)->SetIsFallbackAndMaybeActivate();
+  // Since child throttling allows up to two unembedded surfaces, the surface
+  // immediately after |it| should not be throttled even if it is not embedded
+  // yet.
   ++it;
   if (it == surfaces_.end())
     return;