Surface Synchronization: Child Sync Immediately Activates if Deadline in Past

Prior to this CL, if a child initiates a synchronization event and submits a
CompositorFrame with a deadline in the past then the CompositorFrame will
immediately activate, but the surface will not be marked and so future
CompositorFrames to the same surface will result in blocking until embedding
or deadline again and again.

This could slow down activation and skew deadline duration reporting. This CL
marks a surface such that additional CompositorFrames submitted to the surface
will not block on embedding.

Bug: 672962, 890767
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Ia942e39266ac11dd20cea3f3ae4a087511a98c1d
Reviewed-on: https://chromium-review.googlesource.com/c/1281885
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599983}

components/viz/service/frame_sinks/surface_synchronization_unittest.cc

@@ -64,28 +64,6 @@ CompositorFrame MakeCompositorFrame(
 
 }  // namespace
 
-class FakeExternalBeginFrameSourceClient
-    : public FakeExternalBeginFrameSource::Client {
- public:
-  FakeExternalBeginFrameSourceClient() = default;
-  ~FakeExternalBeginFrameSourceClient() = default;
-
-  bool has_observers() const { return observer_count_ > 0; }
-
-  // FakeExternalBeginFrameSource::Client implementation:
-  void OnAddObserver(BeginFrameObserver* obs) override { ++observer_count_; }
-
-  void OnRemoveObserver(BeginFrameObserver* obs) override {
-    DCHECK_GT(observer_count_, 0);
-    --observer_count_;
-  }
-
- private:
-  int observer_count_ = 0;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeExternalBeginFrameSourceClient);
-};
-
 class SurfaceSynchronizationTest : public testing::Test {
  public:
   SurfaceSynchronizationTest()
@@ -185,6 +163,11 @@ class SurfaceSynchronizationTest : public testing::Test {
     return FrameDeadline(Now(), 4u, BeginFrameArgs::DefaultInterval(), false);
   }
 
+  FrameDeadline MakeDeadline(uint32_t deadline_in_frames) {
+    return FrameDeadline(Now(), deadline_in_frames,
+                         BeginFrameArgs::DefaultInterval(), false);
+  }
+
   void SendNextBeginFrame() {
     // Creep the time forward so that any BeginFrameArgs is not equal to the
     // last one otherwise we violate the BeginFrameSource contract.
@@ -211,7 +194,6 @@ class SurfaceSynchronizationTest : public testing::Test {
 
     begin_frame_source_ =
         std::make_unique<FakeExternalBeginFrameSource>(0.f, false);
-    begin_frame_source_->SetClient(&begin_frame_source_client_);
     now_src_ = std::make_unique<base::SimpleTestTickClock>();
     frame_sink_manager_.surface_manager()->SetTickClockForTesting(
         now_src_.get());
@@ -269,7 +251,6 @@ class SurfaceSynchronizationTest : public testing::Test {
   ServerSharedBitmapManager shared_bitmap_manager_;
   FrameSinkManagerImpl frame_sink_manager_;
   FakeSurfaceObserver surface_observer_;
-  FakeExternalBeginFrameSourceClient begin_frame_source_client_;
   std::unique_ptr<FakeExternalBeginFrameSource> begin_frame_source_;
   std::unordered_map<FrameSinkId,
                      std::unique_ptr<CompositorFrameSinkSupport>,
@@ -2953,6 +2934,46 @@ TEST_F(SurfaceSynchronizationTest, ChildBlockedOnParentActivatesAfterDeadline) {
   EXPECT_FALSE(child_surface2->has_deadline());
 }
 
+// This test verifies that if a child-initiated synchronization is initiated
+// with a deadline in the past, then the frame will immediately activate and
+// be marked as having a dependent frame so that it does not block again in
+// the future.
+TEST_F(SurfaceSynchronizationTest, ChildBlockedOnParentDeadlineInPast) {
+  const SurfaceId child_id1 = MakeSurfaceId(kChildFrameSink1, 1, 1);
+  const SurfaceId child_id2 = MakeSurfaceId(kChildFrameSink1, 1, 2);
+
+  // |child_id1| Surface should immediately activate.
+  child_support1().SubmitCompositorFrame(child_id1.local_surface_id(),
+                                         MakeDefaultCompositorFrame());
+  Surface* child_surface1 = GetSurfaceForId(child_id1);
+  ASSERT_NE(nullptr, child_surface1);
+  EXPECT_FALSE(child_surface1->HasPendingFrame());
+  EXPECT_TRUE(child_surface1->HasActiveFrame());
+  EXPECT_FALSE(child_surface1->HasDependentFrame());
+  EXPECT_FALSE(child_surface1->has_deadline());
+
+  // Pick a deadline in the near future.
+  FrameDeadline deadline = MakeDeadline(1u);
+
+  // Advance time beyond the |deadline| above.
+  SendLateBeginFrame();
+
+  // |child_id2| Surface should activate because it was submitted with a
+  // deadline in the past.
+  child_support1().SubmitCompositorFrame(
+      child_id2.local_surface_id(),
+      MakeCompositorFrame(empty_surface_ids(), empty_surface_ranges(),
+                          std::vector<TransferableResource>(), deadline));
+  Surface* child_surface2 = GetSurfaceForId(child_id2);
+  ASSERT_NE(nullptr, child_surface2);
+  EXPECT_FALSE(child_surface2->HasPendingFrame());
+  EXPECT_TRUE(child_surface2->HasActiveFrame());
+  EXPECT_FALSE(child_surface2->has_deadline());
+
+  // This failed before the latest change.
+  EXPECT_TRUE(child_surface2->HasDependentFrame());
+}
+
 // A child may submit CompositorFrame corresponding to a child-initiated
 // synchronization event followed by a CompositorFrame corresponding to a
 // parent-initiated synchronization event.
@@ -2980,6 +3001,7 @@ TEST_F(SurfaceSynchronizationTest,
   ASSERT_NE(nullptr, child_surface2);
   EXPECT_TRUE(child_surface2->HasPendingFrame());
   EXPECT_FALSE(child_surface2->HasActiveFrame());
+  EXPECT_TRUE(child_surface2->has_deadline());
 
   // |child_id3| Surface should activate immediately because it corresponds to a
   // parent-initiated synchronization event. |child_surface3| activating

components/viz/service/surfaces/surface.cc

@@ -257,6 +257,10 @@ bool Surface::QueueFrame(
         FrameData(std::move(frame), frame_index, std::move(presented_callback));
     RejectCompositorFramesToFallbackSurfaces();
 
+    // Ask SurfaceDependencyTracker to inform |this| when it is embedded.
+    if (block_activation)
+      surface_manager_->dependency_tracker()->TrackEmbedding(this);
+
     // If the deadline is in the past, then the CompositorFrame will activate
     // immediately.
     if (deadline_->Set(ResolveFrameDeadline(frame))) {

components/viz/service/surfaces/surface_dependency_tracker.cc

@@ -16,6 +16,18 @@ SurfaceDependencyTracker::SurfaceDependencyTracker(
 
 SurfaceDependencyTracker::~SurfaceDependencyTracker() = default;
 
+void SurfaceDependencyTracker::TrackEmbedding(Surface* surface) {
+  // If |surface| is blocking on the arrival of a parent and the parent frame
+  // has not yet arrived then track this |surface|'s SurfaceId by FrameSinkId so
+  // that if a parent refers to it or a more recent surface, then
+  // SurfaceDependencyTracker reports back that a dependency has been added.
+  if (surface->block_activation_on_parent() && !surface->HasDependentFrame()) {
+    surfaces_blocked_on_parent_by_frame_sink_id_[surface->surface_id()
+                                                     .frame_sink_id()]
+        .insert(surface->surface_id());
+  }
+}
+
 void SurfaceDependencyTracker::RequestSurfaceResolution(Surface* surface) {
   DCHECK(surface->HasPendingFrame());
 
@@ -34,16 +46,6 @@ void SurfaceDependencyTracker::RequestSurfaceResolution(Surface* surface) {
     }
   }
 
-  // If |surface| is blocking on the arrival of a parent and the parent frame
-  // has not yet arrived then track this |surface|'s SurfaceId by FrameSinkId so
-  // that if a parent refers to it or a more recent surface, then
-  // SurfaceDependencyTracker reports back that a dependency has been added.
-  if (surface->block_activation_on_parent() && !surface->HasDependentFrame()) {
-    surfaces_blocked_on_parent_by_frame_sink_id_[surface->surface_id()
-                                                     .frame_sink_id()]
-        .insert(surface->surface_id());
-  }
-
   UpdateSurfaceDeadline(surface);
 }
 

components/viz/service/surfaces/surface_dependency_tracker.h

@@ -30,6 +30,9 @@ class VIZ_SERVICE_EXPORT SurfaceDependencyTracker {
   explicit SurfaceDependencyTracker(SurfaceManager* surface_manager);
   ~SurfaceDependencyTracker();
 
+  // Called when |surface| wishes to track when it is embedded.
+  void TrackEmbedding(Surface* surface);
+
   // Called when |surface| has a pending CompositorFrame and it wishes to be
   // informed when that surface's dependencies are resolved.
   void RequestSurfaceResolution(Surface* surface);