GetLatestInFlightSurface consider non-temporary references

This CL updates SurfaceManager::GetLatestInFlightSurface to consider
presistent references while computing the latest surface.
In addition, it allow GetLatestInFlightSurface to return something other
than the fallback in case the FrameSinkIds of the range differ.

Bug: 871964
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I9f0de1cbb6bbdd10a69f70305a499123b34d7912
Reviewed-on: https://chromium-review.googlesource.com/1166163
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582019}

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -153,8 +153,15 @@ class SurfaceSynchronizationTest : public testing::Test {
         surface_id);
   }
 
-  Surface* GetLatestInFlightSurface(const SurfaceId& primary_surface_id,
-                                    const SurfaceId& fallback_surface_id) {
+  // Returns true if there is a Persistent reference for |surface_id|.
+  bool HasPersistentReference(const SurfaceId& surface_id) {
+    return frame_sink_manager().surface_manager()->HasPersistentReference(
+        surface_id);
+  }
+
+  Surface* GetLatestInFlightSurface(
+      const SurfaceId& primary_surface_id,
+      const base::Optional<SurfaceId>& fallback_surface_id) {
     return frame_sink_manager().surface_manager()->GetLatestInFlightSurface(
         SurfaceRange(fallback_surface_id, primary_surface_id));
   }
@@ -2197,6 +2204,57 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithBogusFallback) {
   EXPECT_EQ(nullptr, GetLatestInFlightSurface(child_id2, bogus_child_id));
 }
 
+// This test verifies that GetLatestInFlightSurface will return the primary or
+// nullptr if fallback is not specified.
+// TODO(akaba): this would change after https://crbug.com/861769
+TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithoutFallback) {
+  SetFrameSinkHierarchy(kParentFrameSink, kChildFrameSink1);
+  const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
+  const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
+  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
+
+  child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+  // Verify that |child_id1| is active.
+  EXPECT_TRUE(child_surface1()->HasActiveFrame());
+  EXPECT_FALSE(child_surface1()->HasPendingFrame());
+  EXPECT_THAT(child_surface1()->activation_dependencies(), IsEmpty());
+
+  parent_support().SubmitCompositorFrame(
+      parent_id.local_surface_id(),
+      MakeCompositorFrame({child_id2}, {SurfaceRange(child_id1, child_id2)},
+                          std::vector<TransferableResource>()));
+
+  // Verify that the |parent_id| is not active yet.
+  EXPECT_FALSE(parent_surface()->HasActiveFrame());
+  EXPECT_TRUE(parent_surface()->HasPendingFrame());
+  EXPECT_THAT(parent_surface()->activation_dependencies(),
+              UnorderedElementsAre(child_id2));
+
+  // Verify that |child_id1| is the latest active surface.
+  EXPECT_EQ(GetSurfaceForId(child_id1),
+            GetLatestInFlightSurface(child_id2, child_id1));
+
+  // Fallback is not specified and primary doesn't exists so we return nullptr.
+  EXPECT_EQ(nullptr, GetLatestInFlightSurface(child_id2, base::nullopt));
+
+  // Activate |child_id2|
+  child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+  // Verify that child2 is active.
+  EXPECT_TRUE(child_surface1()->HasActiveFrame());
+  EXPECT_FALSE(child_surface1()->HasPendingFrame());
+  EXPECT_THAT(child_surface1()->activation_dependencies(), IsEmpty());
+
+  // Verify that |child_id2| is the latest active surface.
+  EXPECT_EQ(GetSurfaceForId(child_id2),
+            GetLatestInFlightSurface(child_id2, child_id1));
+
+  // Fallback is not specified but primary exists so we return it.
+  EXPECT_EQ(GetSurfaceForId(child_id2),
+            GetLatestInFlightSurface(child_id2, base::nullopt));
+}
+
 // This test verifies that GetLatestInFlightSurface will not return null if the
 // fallback is garbage collected, but instead returns the latest surface older
 // than primary if that exists.
@@ -2279,32 +2337,44 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithGarbageFallback) {
             GetLatestInFlightSurface(child_id4, child_id1));
 }
 
-// This test verifies that GetLatestInFlightSurface will return the fallback
-// surface if the primary and fallback SurfaceIds have different FrameSinkIds.
-// This is important to preserve the property that that latest in-flight surface
-// is no newer than the primary. If the FrameSinkId changes then we cannot be
-// sure of that so we simply return the fallback surface.
+// This test verifies that in the case of different frame sinks
+// GetLatestInFlightSurface will return the latest surface in the primary's
+// FrameSinkId or the latest in the fallback's FrameSinkId if no surface exists
+// in the primary's.
 TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceDifferentFrameSinkIds) {
+  SetFrameSinkHierarchy(kParentFrameSink, kChildFrameSink1);
+  SetFrameSinkHierarchy(kParentFrameSink, kChildFrameSink2);
   const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
   const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
-  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink2, 1);
-  const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink2, 2);
+  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
+  const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink2, 1);
+  const SurfaceId child_id4 = MakeSurfaceId(kChildFrameSink2, 2);
 
+  // Activate |child_id1|.
   child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
                                          MakeDefaultCompositorFrame());
 
+  // Activate |child_id2|.
+  child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
   parent_support().SubmitCompositorFrame(
       parent_id.local_surface_id(),
-      MakeCompositorFrame({child_id2}, {SurfaceRange(child_id1)},
+      MakeCompositorFrame({child_id4}, {SurfaceRange(child_id1, child_id4)},
                           std::vector<TransferableResource>()));
 
-  // Submit a child CompositorFrame without a different FrameSinkId and verify
-  // that if the fallback and primary differ in FrameSinkId then
-  // GetLatestInFlightSurface will always return the specified fallback.
-  child_support2().SubmitCompositorFrame(child_id2.local_surface_id(),
+  // Primary's frame sink id empty and |child_id2| is the latest in fallback's
+  // frame sink.
+  EXPECT_EQ(GetSurfaceForId(child_id2),
+            GetLatestInFlightSurface(child_id4, child_id1));
+
+  // Activate |child_id3| which is in different frame sink.
+  child_support2().SubmitCompositorFrame(child_id3.local_surface_id(),
                                          MakeDefaultCompositorFrame());
-  EXPECT_EQ(GetSurfaceForId(child_id1),
-            GetLatestInFlightSurface(child_id3, child_id1));
+
+  // |child_id3| is the latest in primary's frame sink.
+  EXPECT_EQ(GetSurfaceForId(child_id3),
+            GetLatestInFlightSurface(child_id4, child_id1));
 }
 
 // This test verifies that GetLatestInFlightSurface will return the
@@ -2342,6 +2412,51 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceReturnPrimary) {
             GetLatestInFlightSurface(child_id3, child_id1));
 }
 
+// This test verifies that GetLatestInFlightSurface can use persistent
+// references to compute the latest surface.
+TEST_F(SurfaceSynchronizationTest,
+       LatestInFlightSurfaceUsesPersistentReferences) {
+  SetFrameSinkHierarchy(kParentFrameSink, kChildFrameSink1);
+  const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
+  const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
+  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
+  const SurfaceId child_id3 = MakeSurfaceId(kChildFrameSink1, 3);
+
+  // Activate |child_id1|.
+  child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // |child_id1| now should have a temporary reference.
+  EXPECT_TRUE(HasTemporaryReference(child_id1));
+  EXPECT_FALSE(HasPersistentReference(child_id1));
+
+  // Activate |child_id2|.
+  child_support1().SubmitCompositorFrame(child_id2.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // |child_id2| now should have a temporary reference.
+  EXPECT_TRUE(HasTemporaryReference(child_id2));
+  EXPECT_FALSE(HasPersistentReference(child_id2));
+
+  // Create a reference from |parent_id| to |child_id2|.
+  parent_support().SubmitCompositorFrame(
+      parent_id.local_surface_id(),
+      MakeCompositorFrame(empty_surface_ids(), {SurfaceRange(child_id2)},
+                          std::vector<TransferableResource>()));
+
+  // |child_id1| have no references and can be garbage collected.
+  EXPECT_FALSE(HasTemporaryReference(child_id1));
+  EXPECT_FALSE(HasPersistentReference(child_id1));
+
+  // |child_id2| has a persistent references now.
+  EXPECT_FALSE(HasTemporaryReference(child_id2));
+  EXPECT_TRUE(HasPersistentReference(child_id2));
+
+  // Verify that GetLatestInFlightSurface returns |child_id2|.
+  EXPECT_EQ(GetSurfaceForId(child_id2),
+            GetLatestInFlightSurface(child_id3, child_id1));
+}
+
 // This test verifies that GetLatestInFlightSurface will skip a surface if
 // its nonce is different.
 TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceSkipDifferentNonce) {

components/viz/service/surfaces/surface_manager.cc

@@ -65,6 +65,7 @@ SurfaceManager::~SurfaceManager() {
   // destroyed.
   temporary_references_.clear();
   temporary_reference_ranges_.clear();
+  persistent_references_by_frame_sink_id_.clear();
   // Create a copy of the children set as RemoveSurfaceReferenceImpl below will
   // mutate that set.
   base::flat_set<SurfaceId> children(
@@ -274,6 +275,43 @@ SurfaceManager::GetSurfacesThatReferenceChildForTesting(
   return parents;
 }
 
+Surface* SurfaceManager::GetLatestInFlightSurfaceForFrameSinkId(
+    const SurfaceRange& surface_range,
+    const FrameSinkId& sink_id) {
+  std::vector<LocalSurfaceId> valid_local_surfaces;
+  // Get all valid temporary references.
+  auto temporary_it = temporary_reference_ranges_.find(sink_id);
+  if (temporary_it != temporary_reference_ranges_.end()) {
+    for (const LocalSurfaceId& local_id : temporary_it->second) {
+      if (surface_range.IsInRangeInclusive(SurfaceId(sink_id, local_id)))
+        valid_local_surfaces.push_back(local_id);
+    }
+  }
+
+  // Get all valid persistent references.
+  auto persistent_it = persistent_references_by_frame_sink_id_.find(sink_id);
+  if (persistent_it != persistent_references_by_frame_sink_id_.end()) {
+    for (const LocalSurfaceId& local_id : persistent_it->second) {
+      if (surface_range.IsInRangeInclusive(SurfaceId(sink_id, local_id)))
+        valid_local_surfaces.push_back(local_id);
+    }
+  }
+
+  // Sort all possible surfaces from newest to oldest, then return the first
+  // surface that has an active frame.
+  std::sort(valid_local_surfaces.begin(), valid_local_surfaces.end(),
+            [](const LocalSurfaceId& first, const LocalSurfaceId& second) {
+              return first > second;
+            });
+
+  for (const LocalSurfaceId& local_surface_id : valid_local_surfaces) {
+    Surface* surface = GetSurfaceForId(SurfaceId(sink_id, local_surface_id));
+    if (surface && surface->HasActiveFrame())
+      return surface;
+  }
+  return nullptr;
+}
+
 SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForReferences() {
   SurfaceIdSet reachable_surfaces;
 
@@ -331,6 +369,10 @@ void SurfaceManager::AddSurfaceReferenceImpl(
 
   references_[parent_id].insert(child_id);
 
+  // Add a real reference to child_id.
+  persistent_references_by_frame_sink_id_[child_id.frame_sink_id()].insert(
+      child_id.local_surface_id());
+
   for (auto& observer : observer_list_)
     observer.OnAddedSurfaceReference(parent_id, child_id);
 
@@ -357,12 +399,36 @@ void SurfaceManager::RemoveSurfaceReferenceImpl(
   iter_parent->second.erase(child_iter);
   if (iter_parent->second.empty())
     references_.erase(iter_parent);
+
+  // Remove the presistent reference.
+  const FrameSinkId& sink_id = child_id.frame_sink_id();
+  const LocalSurfaceId& local_id = child_id.local_surface_id();
+
+  auto sink_it = persistent_references_by_frame_sink_id_.find(sink_id);
+  if (sink_it == persistent_references_by_frame_sink_id_.end())
+    return;
+
+  auto local_surface_it = sink_it->second.find(local_id);
+  if (local_surface_it == sink_it->second.end())
+    return;
+
+  sink_it->second.erase(local_surface_it);
+  if (sink_it->second.empty())
+    persistent_references_by_frame_sink_id_.erase(sink_it);
 }
 
 bool SurfaceManager::HasTemporaryReference(const SurfaceId& surface_id) const {
   return temporary_references_.count(surface_id) != 0;
 }
 
+bool SurfaceManager::HasPersistentReference(const SurfaceId& surface_id) const {
+  auto it =
+      persistent_references_by_frame_sink_id_.find(surface_id.frame_sink_id());
+  if (it == persistent_references_by_frame_sink_id_.end())
+    return false;
+  return it->second.count(surface_id.local_surface_id()) != 0;
+}
+
 void SurfaceManager::AddTemporaryReference(const SurfaceId& surface_id) {
   DCHECK(!HasTemporaryReference(surface_id));
 
@@ -429,53 +495,33 @@ void SurfaceManager::RemoveTemporaryReference(const SurfaceId& surface_id,
 
 Surface* SurfaceManager::GetLatestInFlightSurface(
     const SurfaceRange& surface_range) {
-  const SurfaceId& primary_surface_id = surface_range.end();
-  const base::Optional<SurfaceId>& fallback_surface_id = surface_range.start();
 
   // If primary exists, we return it.
-  Surface* primary_surface = GetSurfaceForId(primary_surface_id);
+  Surface* primary_surface = GetSurfaceForId(surface_range.end());
   if (primary_surface && primary_surface->HasActiveFrame())
     return primary_surface;
 
-  if (!fallback_surface_id)
+  // If fallback is not specified we return nullptr.
+  // TODO(akaba): after fixing https://crbug.com/861769 we need to return
+  // something older than primary.
+  if (!surface_range.start())
     return nullptr;
 
-  DCHECK(fallback_surface_id->is_valid());
-
-  Surface* fallback_surface = GetSurfaceForId(*fallback_surface_id);
+  // If both end of the range exists, we try the primary's FrameSinkId first.
+  Surface* latest_surface = GetLatestInFlightSurfaceForFrameSinkId(
+      surface_range, surface_range.end().frame_sink_id());
 
-  auto it =
-      temporary_reference_ranges_.find(fallback_surface_id->frame_sink_id());
-  if (it == temporary_reference_ranges_.end())
-    return fallback_surface;
-
-  const std::vector<LocalSurfaceId>& temp_surfaces = it->second;
-  for (const LocalSurfaceId& local_surface_id : base::Reversed(temp_surfaces)) {
-    // The in-flight surface must be older than the primary surface ID.
-    if (local_surface_id.parent_sequence_number() >
-            primary_surface_id.local_surface_id().parent_sequence_number() ||
-        local_surface_id.child_sequence_number() >
-            primary_surface_id.local_surface_id().child_sequence_number()) {
-      continue;
-    }
-
-    // If the embed_token doesn't match the primary or fallback's then the
-    // parent does not have permission to embed this surface.
-    if (local_surface_id.embed_token() !=
-            fallback_surface_id->local_surface_id().embed_token() &&
-        local_surface_id.embed_token() !=
-            primary_surface_id.local_surface_id().embed_token()) {
-      continue;
-    }
-
-    SurfaceId surface_id(fallback_surface_id->frame_sink_id(),
-                         local_surface_id);
-    Surface* surface = GetSurfaceForId(surface_id);
-    if (surface && surface->HasActiveFrame())
-      return surface;
+  // If the fallback has a different FrameSinkId, then try that also.
+  if (!latest_surface && surface_range.HasDifferentFrameSinkIds()) {
+    latest_surface = GetLatestInFlightSurfaceForFrameSinkId(
+        surface_range, surface_range.start()->frame_sink_id());
   }
 
-  return fallback_surface;
+  // Fallback might have neither temporary or presistent references, so we
+  // consider it separately.
+  if (!latest_surface)
+    latest_surface = GetSurfaceForId(*surface_range.start());
+  return latest_surface;
 }
 
 void SurfaceManager::ExpireOldTemporaryReferences() {

components/viz/service/surfaces/surface_manager.h

@@ -185,10 +185,9 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   base::flat_set<SurfaceId> GetSurfacesThatReferenceChildForTesting(
       const SurfaceId& surface_id) const;
 
-  // Returns the most recent surface associated with the |fallback_surface_id|'s
-  // FrameSinkId that was created prior to the current primary surface and
-  // verified by the viz host to be owned by the fallback surface's parent. If
-  // |fallback_surface_id| doesn't exist then this method will return nullptr.
+  // Returns the primary surface if it exists. Otherwise, this will return the
+  // most recent surface in |surface_range|. If no surface exists, this will
+  // return nullptr.
   Surface* GetLatestInFlightSurface(const SurfaceRange& surface_range);
 
   // Called by SurfaceAggregator notifying us that it will use |surface| in the
@@ -224,6 +223,11 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
     bool marked_as_old = false;
   };
 
+  // Returns the latest surface in a FrameSinkId that satisfies |is_valid|.
+  Surface* GetLatestInFlightSurfaceForFrameSinkId(
+      const SurfaceRange& surface_range,
+      const FrameSinkId& sink_id);
+
   // Returns set of live surfaces for |lifetime_manager_| is REFERENCES.
   SurfaceIdSet GetLiveSurfacesForReferences();
 
@@ -237,8 +241,12 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   // Removes a reference from a |parent_id| to |child_id|.
   void RemoveSurfaceReferenceImpl(const SurfaceReference& reference);
 
+  // Returns whether |surface_id| has a temporary reference or not.
   bool HasTemporaryReference(const SurfaceId& surface_id) const;
 
+  // Returns whether |surface_id| has a Persistent reference or not.
+  bool HasPersistentReference(const SurfaceId& surface_id) const;
+
   // Adds a temporary reference to |surface_id|. The reference will not have an
   // owner initially.
   void AddTemporaryReference(const SurfaceId& surface_id);
@@ -311,6 +319,11 @@ class VIZ_SERVICE_EXPORT SurfaceManager {
   std::unordered_map<FrameSinkId, std::vector<LocalSurfaceId>, FrameSinkIdHash>
       temporary_reference_ranges_;
 
+  // A list of surfaces with a given FrameSinkId that have a persistent
+  // reference.
+  base::flat_map<FrameSinkId, base::flat_set<LocalSurfaceId>>
+      persistent_references_by_frame_sink_id_;
+
   // Timer to remove old temporary references that aren't removed after an
   // interval of time. The timer will started/stopped so it only runs if there
   // are temporary references. Also the timer isn't used with Android WebView.