viz: Take over LatencyInfo from replaced surfaces & terminate on reject

This patch fixes two issues that could previously lead to unterminated
LatencyInfo flows:
1) When we replace a surface (new surface for same frame sink
   activates) before frames on multiple old surfaces were activated or
   aggregated, we didn't correctly propagate the LatencyInfo from all
   old surfaces.
2) When a CompositorFrame is rejected by CompositorFrameSinkSupport
   or Surface::QueueFrame(), we should terminate its LatencyInfo, too.

Bug: 1056604
Change-Id: I1a4e2067cf1d762298cafb7fced6c7bbc9a7bd70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078015Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Eric Seckler <eseckler@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747165}

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -395,11 +395,6 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
   begin_frame_tracker_.ReceivedAck(frame.metadata.begin_frame_ack);
   ++ack_pending_count_;
 
-  base::ScopedClosureRunner frame_rejected_callback(
-      base::BindOnce(&CompositorFrameSinkSupport::DidRejectCompositorFrame,
-                     weak_factory_.GetWeakPtr(), frame.metadata.frame_token,
-                     frame.resource_list));
-
   compositor_frame_callback_ = std::move(callback);
   if (compositor_frame_callback_) {
     callback_received_begin_frame_ = false;
@@ -410,17 +405,6 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
   base::TimeTicks now_time = base::TimeTicks::Now();
   pending_received_frame_times_.emplace(frame.metadata.frame_token, now_time);
 
-  // Ensure no CopyOutputRequests have been submitted if they are banned.
-  if (!allow_copy_output_requests_ && frame.HasCopyOutputRequests()) {
-    TRACE_EVENT_INSTANT0("viz", "CopyOutputRequests not allowed",
-                         TRACE_EVENT_SCOPE_THREAD);
-    return SubmitResult::COPY_OUTPUT_REQUESTS_NOT_ALLOWED;
-  }
-
-  // TODO(crbug.com/846739): It should be possible to use
-  // |frame.metadata.frame_token| instead of maintaining a |last_frame_index_|.
-  uint64_t frame_index = ++last_frame_index_;
-
   // Override the has_damage flag (ignoring invalid data from clients).
   frame.metadata.begin_frame_ack.has_damage = true;
   DCHECK(frame.metadata.begin_frame_ack.frame_id.IsSequenceValid());
@@ -439,6 +423,22 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
     }
   }
 
+  base::ScopedClosureRunner frame_rejected_callback(
+      base::BindOnce(&CompositorFrameSinkSupport::DidRejectCompositorFrame,
+                     weak_factory_.GetWeakPtr(), frame.metadata.frame_token,
+                     frame.resource_list, frame.metadata.latency_info));
+
+  // Ensure no CopyOutputRequests have been submitted if they are banned.
+  if (!allow_copy_output_requests_ && frame.HasCopyOutputRequests()) {
+    TRACE_EVENT_INSTANT0("viz", "CopyOutputRequests not allowed",
+                         TRACE_EVENT_SCOPE_THREAD);
+    return SubmitResult::COPY_OUTPUT_REQUESTS_NOT_ALLOWED;
+  }
+
+  // TODO(crbug.com/846739): It should be possible to use
+  // |frame.metadata.frame_token| instead of maintaining a |last_frame_index_|.
+  uint64_t frame_index = ++last_frame_index_;
+
   if (frame.metadata.preferred_frame_interval) {
     frame_sink_manager_->SetPreferredFrameIntervalForFrameSinkId(
         frame_sink_id_, *frame.metadata.preferred_frame_interval);
@@ -616,7 +616,15 @@ void CompositorFrameSinkSupport::DidPresentCompositorFrame(
 
 void CompositorFrameSinkSupport::DidRejectCompositorFrame(
     uint32_t frame_token,
-    std::vector<TransferableResource> frame_resource_list) {
+    std::vector<TransferableResource> frame_resource_list,
+    std::vector<ui::LatencyInfo> latency_info) {
+  TRACE_EVENT_INSTANT0("viz", "DidRejectCompositorFrame",
+                       TRACE_EVENT_SCOPE_THREAD);
+  // TODO(eseckler): Should these be stored and attached to the next successful
+  // frame submission instead?
+  for (ui::LatencyInfo& info : latency_info)
+    info.Terminate();
+
   std::vector<ReturnedResource> resources =
       TransferableResource::ReturnResources(frame_resource_list);
   ReturnResources(resources);

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -211,7 +211,8 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
                                  const gfx::PresentationFeedback& feedback);
   void DidRejectCompositorFrame(
       uint32_t frame_token,
-      std::vector<TransferableResource> frame_resource_list);
+      std::vector<TransferableResource> frame_resource_list,
+      std::vector<ui::LatencyInfo> latency_info);
 
   // Update the display root reference with |surface|.
   void UpdateDisplayRootReference(const Surface* surface);

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -988,7 +988,7 @@ TEST_F(SurfaceSynchronizationTest,
   const ui::LatencyComponentType latency_type2 =
       ui::LATENCY_COMPONENT_TYPE_LAST;
 
-  // Submit a frame with no unresolved dependecy.
+  // Submit a frame with no unresolved dependency.
   ui::LatencyInfo info;
   info.AddLatencyNumber(latency_type1);
 
@@ -1122,9 +1122,89 @@ TEST_F(SurfaceSynchronizationTest,
       ui::DISPLAY_COMPOSITOR_RECEIVED_FRAME_COMPONENT, nullptr));
 }
 
+// Checks whether SurfaceAllocationGroup properly aggregates LatencyInfo of
+// multiple surfaces. In this variation of the test, multiple older surfaces
+// with pending frames exist during aggregation of an activated frame on a newer
+// surface.
+TEST_F(SurfaceSynchronizationTest,
+       LatencyInfoAggregation_MultipleOldSurfacesWithPendingFrames) {
+  const SurfaceId parent_id1 = MakeSurfaceId(kParentFrameSink, 1);
+  const SurfaceId parent_id2 = MakeSurfaceId(kParentFrameSink, 2);
+  const SurfaceId parent_id3 = MakeSurfaceId(kParentFrameSink, 3);
+  const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1);
+  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 2);
+
+  const ui::LatencyComponentType latency_type1 =
+      ui::INPUT_EVENT_LATENCY_RENDERER_SWAP_COMPONENT;
+  const ui::LatencyComponentType latency_type2 =
+      ui::LATENCY_COMPONENT_TYPE_LAST;
+
+  // Submit a frame with unresolved dependencies to parent_id1.
+  ui::LatencyInfo info1;
+  info1.AddLatencyNumber(latency_type1);
+
+  CompositorFrame frame1 = MakeCompositorFrame(
+      {child_id1}, empty_surface_ranges(), std::vector<TransferableResource>());
+  frame1.metadata.latency_info.push_back(info1);
+
+  parent_support().SubmitCompositorFrame(parent_id1.local_surface_id(),
+                                         std::move(frame1));
+
+  // Submit a frame with unresolved dependencies to parent_id2.
+  ui::LatencyInfo info2;
+  info2.AddLatencyNumber(latency_type2);
+
+  CompositorFrame frame2 = MakeCompositorFrame(
+      {child_id2}, empty_surface_ranges(), std::vector<TransferableResource>());
+  frame2.metadata.latency_info.push_back(info2);
+
+  parent_support().SubmitCompositorFrame(parent_id2.local_surface_id(),
+                                         std::move(frame2));
+
+  // Verify that the both old surfaces have pending frames.
+  Surface* old_surface1 = GetSurfaceForId(parent_id1);
+  Surface* old_surface2 = GetSurfaceForId(parent_id2);
+  ASSERT_NE(nullptr, old_surface1);
+  ASSERT_NE(nullptr, old_surface2);
+  EXPECT_FALSE(old_surface1->HasActiveFrame());
+  EXPECT_FALSE(old_surface2->HasActiveFrame());
+  EXPECT_TRUE(old_surface1->HasPendingFrame());
+  EXPECT_TRUE(old_surface2->HasPendingFrame());
+
+  // Submit a frame with no dependencies to the new surface parent_id3.
+  parent_support().SubmitCompositorFrame(parent_id3.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+
+  // Verify that the new surface has an active frame only.
+  Surface* surface = GetSurfaceForId(parent_id3);
+  ASSERT_NE(nullptr, surface);
+  EXPECT_TRUE(surface->HasActiveFrame());
+  EXPECT_FALSE(surface->HasPendingFrame());
+
+  // Verify that the aggregated LatencyInfo has LatencyInfo from both old
+  // surfaces.
+  std::vector<ui::LatencyInfo> info_list;
+  surface->allocation_group()->TakeAggregatedLatencyInfoUpTo(surface,
+                                                             &info_list);
+  EXPECT_EQ(2u, info_list.size());
+
+  ui::LatencyInfo aggregated_latency_info = info_list[0];
+  aggregated_latency_info.AddNewLatencyFrom(info_list[1]);
+
+  // Two components are the original ones, and the third one is
+  // DISPLAY_COMPOSITOR_RECEIVED_FRAME_COMPONENT, logged on compositor frame
+  // submit.
+  EXPECT_EQ(3u, aggregated_latency_info.latency_components().size());
+
+  EXPECT_TRUE(aggregated_latency_info.FindLatency(latency_type1, nullptr));
+  EXPECT_TRUE(aggregated_latency_info.FindLatency(latency_type2, nullptr));
+  EXPECT_TRUE(aggregated_latency_info.FindLatency(
+      ui::DISPLAY_COMPOSITOR_RECEIVED_FRAME_COMPONENT, nullptr));
+}
+
 // This test verifies that when a new surface is created, the LatencyInfo of the
 // previous surface does not get carried over into the new surface.
-TEST_F(SurfaceSynchronizationTest, LatencyInfoNotCarriedOVer) {
+TEST_F(SurfaceSynchronizationTest, LatencyInfoNotCarriedOver) {
   const SurfaceId parent_id1 = MakeSurfaceId(kParentFrameSink, 1);
   const SurfaceId parent_id2 = MakeSurfaceId(kParentFrameSink, 2);
   const ui::LatencyComponentType latency_type1 =

components/viz/service/surfaces/surface.cc

@@ -647,10 +647,16 @@ void Surface::UnrefFrameResourcesAndRunCallbacks(
 
   // If we won't be getting a presented notification, we'll notify the client
   // when the frame is unref'd.
-  if (!frame_data->will_be_notified_of_presentation && surface_client_)
+  if (!frame_data->will_be_notified_of_presentation && surface_client_) {
     surface_client_->OnSurfacePresented(frame_data->frame.metadata.frame_token,
                                         base::TimeTicks(), gfx::SwapTimings(),
                                         gfx::PresentationFeedback::Failure());
+  }
+
+  // Usually the LatencyInfo was already taken during aggregation or when the
+  // surface was replaced. If neither happened, terminate the LatencyInfo now.
+  for (ui::LatencyInfo& info : frame_data->frame.metadata.latency_info)
+    info.Terminate();
 }
 
 void Surface::ClearCopyRequests() {

components/viz/service/surfaces/surface_allocation_group.cc

@@ -150,8 +150,8 @@ void SurfaceAllocationGroup::TakeAggregatedLatencyInfoUpTo(
   surface->TakeActiveLatencyInfo(out);
   auto it = FindLatestSurfaceUpTo(surface->surface_id());
   DCHECK_EQ(*it, surface);
-  while (it > surfaces_.begin() && !(*it)->is_latency_info_taken())
-    (*--it)->TakeActiveAndPendingLatencyInfo(out);
+  while (it > surfaces_.begin() && !(*--it)->is_latency_info_taken())
+    (*it)->TakeActiveAndPendingLatencyInfo(out);
 }
 
 void SurfaceAllocationGroup::OnFirstSurfaceActivation(Surface* surface) {