SurfaceManager::GetLatestInFlightSurface should be able to return the primary

GetLatestInFlightSurface should return the primary if it exists and active.
There is no reason for it not to do so and this will make working with
it easier.

Bug: 869507
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Icae9da8527de69555e8c925740c08b187f827b02
Reviewed-on: https://chromium-review.googlesource.com/1157140
Commit-Queue: Andre Kaba <akaba@google.com>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580311}

components/viz/service/display/surface_aggregator.cc

@@ -200,60 +200,48 @@ void SurfaceAggregator::HandleSurfaceQuad(
     gfx::Rect* damage_rect_in_quad_space,
     bool* damage_rect_in_quad_space_valid) {
   SurfaceId primary_surface_id = surface_quad->surface_range.end();
-  Surface* primary_surface = manager_->GetSurfaceForId(primary_surface_id);
-  if (primary_surface && primary_surface->HasActiveFrame()) {
-    EmitSurfaceContent(primary_surface, parent_device_scale_factor,
-                       surface_quad->shared_quad_state, surface_quad->rect,
-                       surface_quad->visible_rect, target_transform, clip_rect,
-                       surface_quad->stretch_content_to_fill_bounds, dest_pass,
-                       ignore_undamaged, damage_rect_in_quad_space,
-                       damage_rect_in_quad_space_valid);
-    return;
-  }
-
-  Surface* latest_surface = nullptr;
-  if (surface_quad->surface_range.start()) {
-    latest_surface =
-        manager_->GetLatestInFlightSurface(surface_quad->surface_range);
-  }
 
   // If there's no fallback surface ID available, then simply emit a
   // SolidColorDrawQuad with the provided default background color. This
   // can happen after a Viz process crash.
+  Surface* latest_surface =
+      manager_->GetLatestInFlightSurface(surface_quad->surface_range);
   if (!latest_surface || !latest_surface->HasActiveFrame()) {
     EmitDefaultBackgroundColorQuad(surface_quad, target_transform, clip_rect,
                                    dest_pass);
     return;
   }
 
-  if (primary_surface_id.frame_sink_id() ==
-          latest_surface->surface_id().frame_sink_id() &&
-      primary_surface_id.local_surface_id().embed_token() ==
-          latest_surface->surface_id().local_surface_id().embed_token()) {
-    UMA_HISTOGRAM_COUNTS_100(
-        kUmaManhattanDistanceToPrimary,
-        latest_surface->surface_id().ManhattanDistanceTo(primary_surface_id));
-  }
+  if (latest_surface->surface_id() != primary_surface_id) {
+    if (primary_surface_id.frame_sink_id() ==
+            latest_surface->surface_id().frame_sink_id() &&
+        primary_surface_id.local_surface_id().embed_token() ==
+            latest_surface->surface_id().local_surface_id().embed_token()) {
+      UMA_HISTOGRAM_COUNTS_100(
+          kUmaManhattanDistanceToPrimary,
+          latest_surface->surface_id().ManhattanDistanceTo(primary_surface_id));
+    }
 
-  if (!surface_quad->stretch_content_to_fill_bounds) {
-    const CompositorFrame& fallback_frame = latest_surface->GetActiveFrame();
+    if (!surface_quad->stretch_content_to_fill_bounds) {
+      const CompositorFrame& fallback_frame = latest_surface->GetActiveFrame();
 
-    gfx::Rect fallback_rect(latest_surface->GetActiveFrame().size_in_pixels());
+      gfx::Rect fallback_rect(
+          latest_surface->GetActiveFrame().size_in_pixels());
 
-    float scale_ratio =
-        parent_device_scale_factor / fallback_frame.device_scale_factor();
-    fallback_rect =
-        gfx::ScaleToEnclosingRect(fallback_rect, scale_ratio, scale_ratio);
-    fallback_rect = gfx::IntersectRects(fallback_rect, surface_quad->rect);
+      float scale_ratio =
+          parent_device_scale_factor / fallback_frame.device_scale_factor();
+      fallback_rect =
+          gfx::ScaleToEnclosingRect(fallback_rect, scale_ratio, scale_ratio);
+      fallback_rect = gfx::IntersectRects(fallback_rect, surface_quad->rect);
 
-    EmitGutterQuadsIfNecessary(
-        surface_quad->rect, fallback_rect, surface_quad->shared_quad_state,
-        target_transform, clip_rect,
-        fallback_frame.metadata.root_background_color, dest_pass);
+      EmitGutterQuadsIfNecessary(
+          surface_quad->rect, fallback_rect, surface_quad->shared_quad_state,
+          target_transform, clip_rect,
+          fallback_frame.metadata.root_background_color, dest_pass);
+    }
+    ++uma_stats_.using_fallback_surface;
   }
 
-  ++uma_stats_.using_fallback_surface;
-
   EmitSurfaceContent(latest_surface, parent_device_scale_factor,
                      surface_quad->shared_quad_state, surface_quad->rect,
                      surface_quad->visible_rect, target_transform, clip_rect,
@@ -997,22 +985,19 @@ gfx::Rect SurfaceAggregator::PrewalkTree(Surface* surface,
                 surface_info.surface_range.end().local_surface_id());
       }
     }
+
+    // TODO(fsamuel): Consider caching this value somewhere so that
+    // HandleSurfaceQuad doesn't need to call it again.
     Surface* child_surface =
-        manager_->GetSurfaceForId(surface_info.surface_range.end());
+        manager_->GetLatestInFlightSurface(surface_info.surface_range);
+
+    // If the primary surface is not available then we assume the damage is
+    // the full size of the SurfaceDrawQuad because we might need to introduce
+    // gutter.
     gfx::Rect surface_damage;
-    if (!child_surface || !child_surface->HasActiveFrame()) {
-      // If the primary surface is not available then we assume the damage is
-      // the full size of the SurfaceDrawQuad because we might need to introduce
-      // gutter.
+    if (!child_surface ||
+        child_surface->surface_id() != surface_info.surface_range.end()) {
       surface_damage = surface_info.quad_rect;
-      if (surface_info.surface_range.start()) {
-        // TODO(fsamuel): Consider caching this value somewhere so that
-        // HandleSurfaceQuad doesn't need to call it again.
-        Surface* fallback_surface =
-            manager_->GetLatestInFlightSurface(surface_info.surface_range);
-        if (fallback_surface && fallback_surface->HasActiveFrame())
-          child_surface = fallback_surface;
-      }
     }
 
     if (child_surface) {

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -2157,9 +2157,8 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurface) {
             GetLatestInFlightSurface(child_id4, child_id1));
   EXPECT_THAT(GetChildReferences(parent_id), UnorderedElementsAre(child_id1));
 
-  // If the primary surface is old, then we shouldn't return an in-flight
-  // surface that is newer than the primary.
-  EXPECT_EQ(GetSurfaceForId(child_id2),
+  // If the primary surface is active, we return it.
+  EXPECT_EQ(GetSurfaceForId(child_id3),
             GetLatestInFlightSurface(child_id3, child_id1));
 }
 
@@ -2168,6 +2167,7 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurface) {
 TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithBogusFallback) {
   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());
@@ -2186,10 +2186,15 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceWithBogusFallback) {
   EXPECT_FALSE(parent_surface()->HasPendingFrame());
   EXPECT_THAT(parent_surface()->activation_dependencies(), IsEmpty());
 
-  // If the fallback surface doesn't exist, then GetLatestInFlightSurface should
-  // always return nullptr.
   const SurfaceId bogus_child_id = MakeSurfaceId(kChildFrameSink1, 10);
-  EXPECT_EQ(nullptr, GetLatestInFlightSurface(child_id1, bogus_child_id));
+
+  // If primary exists and active return it regardless of the fallback.
+  EXPECT_EQ(GetSurfaceForId(child_id1),
+            GetLatestInFlightSurface(child_id1, bogus_child_id));
+
+  // If primary is not active and fallback is doesn't exist, we always return
+  // nullptr.
+  EXPECT_EQ(nullptr, GetLatestInFlightSurface(child_id2, bogus_child_id));
 }
 
 // This test verifies that GetLatestInFlightSurface will not return null if the
@@ -2283,6 +2288,7 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceDifferentFrameSinkIds) {
   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);
 
   child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
                                          MakeDefaultCompositorFrame());
@@ -2292,24 +2298,18 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceDifferentFrameSinkIds) {
       MakeCompositorFrame({child_id2}, {SurfaceRange(child_id1)},
                           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(),
-                                         MakeDefaultCompositorFrame());
-
   // 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(),
                                          MakeDefaultCompositorFrame());
   EXPECT_EQ(GetSurfaceForId(child_id1),
-            GetLatestInFlightSurface(child_id2, child_id1));
+            GetLatestInFlightSurface(child_id3, child_id1));
 }
 
-// This test verifies that GetLatestInFlightSurface will never return the
-// primary surface ID even if it is in the temporary reference queue.
-TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceSkipPrimary) {
+// This test verifies that GetLatestInFlightSurface will return the
+// primary surface ID if it is in the temporary reference queue.
+TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceReturnPrimary) {
   const SurfaceId parent_id = MakeSurfaceId(kParentFrameSink, 1);
   const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
   const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
@@ -2336,9 +2336,9 @@ TEST_F(SurfaceSynchronizationTest, LatestInFlightSurfaceSkipPrimary) {
   child_support1().SubmitCompositorFrame(child_id3.local_surface_id(),
                                          MakeDefaultCompositorFrame());
 
-  // GetLatestInFlightSurface will never return the primary surface ID
-  // even if it's available.
-  EXPECT_EQ(GetSurfaceForId(child_id2),
+  // GetLatestInFlightSurface will return the primary surface ID if it's
+  // available.
+  EXPECT_EQ(GetSurfaceForId(child_id3),
             GetLatestInFlightSurface(child_id3, child_id1));
 }
 

components/viz/service/surfaces/surface.cc

@@ -139,8 +139,10 @@ void Surface::RejectCompositorFramesToFallbackSurfaces() {
     // 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)
+    if (fallback_surface &&
+        fallback_surface->surface_id() != surface_range.end()) {
       fallback_surface->Close();
+    }
   }
 }
 

components/viz/service/surfaces/surface_manager.cc

@@ -446,6 +446,11 @@ Surface* SurfaceManager::GetLatestInFlightSurface(
   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);
+  if (primary_surface && primary_surface->HasActiveFrame())
+    return primary_surface;
+
   if (!fallback_surface_id)
     return nullptr;
 
@@ -461,8 +466,7 @@ Surface* SurfaceManager::GetLatestInFlightSurface(
   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 == primary_surface_id.local_surface_id() ||
-        local_surface_id.parent_sequence_number() >
+    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()) {