Surface Synchronization: Cleanup deadline inheritance

I stumbled upon the need for some cleanup in deadlines while investigating
a surface sync bug.

SurfaceDependencyDeadline::InheritFrom returned a bool that wasn't
used anywhere anymore. This CL removes it.

This CL also clarifies the InheritFrom contract. When deadline object
A inherits from deadline object B, A gets B's deadline but does not
get B's start time. This was already true but this CL adds direct test
coverage of this.

The other oddity is InheritFrom only supported deadline inheritance if
the object already had a deadline set. This is how it's used by surface
sync in practice but seems unnecessarily brittle. With a small change,
the contract is fully specified and this special case is removed.

Bug: 672962
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Icd6a078ef90298af578c7267ca38f5c0f3ab5d37
Reviewed-on: https://chromium-review.googlesource.com/964257
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543447}

components/viz/service/surfaces/surface.cc

@@ -57,13 +57,13 @@ void Surface::Reset(base::WeakPtr<SurfaceClient> client) {
   active_frame_data_.reset();
 }
 
-bool Surface::InheritActivationDeadlineFrom(Surface* surface) {
+void Surface::InheritActivationDeadlineFrom(Surface* surface) {
   TRACE_EVENT1("viz", "Surface::InheritActivationDeadlineFrom", "FrameSinkId",
                surface_id().frame_sink_id().ToString());
   if (!deadline_ || !surface->deadline_)
-    return false;
+    return;
 
-  return deadline_->InheritFrom(*surface->deadline_);
+  deadline_->InheritFrom(*surface->deadline_);
 }
 
 void Surface::SetPreviousFrameSurface(Surface* surface) {

components/viz/service/surfaces/surface.h

@@ -102,7 +102,12 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
 
   bool has_deadline() const { return deadline_ && deadline_->has_deadline(); }
 
-  bool InheritActivationDeadlineFrom(Surface* surface);
+  // Inherits the same deadline as the one specified by |surface|. A deadline
+  // may be set further out in order to avoid doing unnecessary work while a
+  // parent surface is blocked on dependencies. A deadline may be shortened
+  // in order to minimize guttering (by unblocking children blocked on their
+  // grandchildren sooner).
+  void InheritActivationDeadlineFrom(Surface* surface);
 
   void SetPreviousFrameSurface(Surface* surface);
 

components/viz/service/surfaces/surface_dependency_deadline.cc

@@ -42,20 +42,20 @@ base::Optional<base::TimeDelta> SurfaceDependencyDeadline::Cancel() {
   return CancelInternal(false);
 }
 
-bool SurfaceDependencyDeadline::InheritFrom(
+void SurfaceDependencyDeadline::InheritFrom(
     const SurfaceDependencyDeadline& other) {
   if (*this == other)
-    return false;
-
-  DCHECK(has_deadline());
+    return;
 
-  CancelInternal(false);
+  base::Optional<base::TimeDelta> duration = CancelInternal(false);
   last_begin_frame_args_ = other.last_begin_frame_args_;
   begin_frame_source_ = other.begin_frame_source_;
   deadline_ = other.deadline_;
-  if (deadline_)
+  if (deadline_) {
+    if (!duration)
+      start_time_ = tick_clock_->NowTicks();
     begin_frame_source_->AddObserver(this);
-  return true;
+  }
 }
 
 bool SurfaceDependencyDeadline::operator==(

components/viz/service/surfaces/surface_dependency_deadline.h

@@ -38,9 +38,12 @@ class VIZ_SERVICE_EXPORT SurfaceDependencyDeadline : public BeginFrameObserver {
 
   bool has_deadline() const { return deadline_.has_value(); }
 
-  // Takes on the same BeginFrameSource and deadline as |other|. Returns
-  // false if they're already the same, and true otherwise.
-  bool InheritFrom(const SurfaceDependencyDeadline& other);
+  base::Optional<base::TimeTicks> deadline_for_testing() const {
+    return deadline_;
+  }
+
+  // Takes on the same BeginFrameSource and deadline as |other|.
+  void InheritFrom(const SurfaceDependencyDeadline& other);
 
   bool operator==(const SurfaceDependencyDeadline& other);
   bool operator!=(const SurfaceDependencyDeadline& other) {

components/viz/service/surfaces/surface_dependency_deadline_unittest.cc

@@ -64,6 +64,8 @@ class SurfaceDependencyDeadlineTest : public testing::Test {
 
   SurfaceDependencyDeadline* deadline() { return deadline_.get(); }
 
+  SurfaceDependencyDeadline* deadline2() { return deadline2_.get(); }
+
   void SendLateBeginFrame(uint32_t frames_late) {
     // Creep the time forward so that any BeginFrameArgs is not equal to the
     // last one otherwise we violate the BeginFrameSource contract.
@@ -83,9 +85,14 @@ class SurfaceDependencyDeadlineTest : public testing::Test {
 
     deadline_ = std::make_unique<SurfaceDependencyDeadline>(
         &client_, begin_frame_source_.get(), now_src_.get());
+
+    deadline2_ = std::make_unique<SurfaceDependencyDeadline>(
+        &client_, begin_frame_source_.get(), now_src_.get());
   }
 
   void TearDown() override {
+    deadline2_->Cancel();
+    deadline2_.reset();
     deadline_->Cancel();
     deadline_.reset();
     begin_frame_source_.reset();
@@ -97,6 +104,7 @@ class SurfaceDependencyDeadlineTest : public testing::Test {
   std::unique_ptr<FakeSlowBeginFrameSource> begin_frame_source_;
   FakeSurfaceDeadlineClient client_;
   std::unique_ptr<SurfaceDependencyDeadline> deadline_;
+  std::unique_ptr<SurfaceDependencyDeadline> deadline2_;
 
   DISALLOW_COPY_AND_ASSIGN(SurfaceDependencyDeadlineTest);
 };
@@ -128,5 +136,65 @@ TEST_F(SurfaceDependencyDeadlineTest, SetMatchesHasDeadlineIfTrue) {
   EXPECT_TRUE(deadline()->has_deadline());
 }
 
+// This test verifies that inheriting a deadline with no pre-existing deadline
+// sets up the start time of the event to the time of inheritance.
+TEST_F(SurfaceDependencyDeadlineTest, InheritDeadline) {
+  FrameDeadline frame_deadline = MakeDefaultDeadline();
+  SendLateBeginFrame(1u);
+  EXPECT_TRUE(deadline()->Set(frame_deadline));
+  EXPECT_TRUE(deadline()->has_deadline());
+
+  SendLateBeginFrame(1u);
+  EXPECT_FALSE(deadline2()->has_deadline());
+  deadline2()->InheritFrom(*deadline());
+  EXPECT_TRUE(deadline()->has_deadline());
+  EXPECT_EQ(deadline()->deadline_for_testing(),
+            deadline2()->deadline_for_testing());
+
+  base::Optional<base::TimeDelta> duration1 = deadline()->Cancel();
+  base::Optional<base::TimeDelta> duration2 = deadline2()->Cancel();
+  ASSERT_TRUE(duration1.has_value());
+  ASSERT_TRUE(duration2.has_value());
+
+  // We inject time on BeginFrameSource::AddObserver and in practice we cannot
+  // know the exact difference in duration between two events a priori so we
+  // just verify that the first event was longer than the second.
+  EXPECT_GT(duration1, duration2);
+}
+
+// This test verifies that if an active deadline object inherits a deadline
+// from another object, it does not inherit the start time of the event.
+TEST_F(SurfaceDependencyDeadlineTest, InheritDeadlineWithActiveDeadline) {
+  {
+    FrameDeadline frame_deadline = MakeDefaultDeadline();
+    SendLateBeginFrame(1u);
+    EXPECT_TRUE(deadline()->Set(frame_deadline));
+    EXPECT_TRUE(deadline()->has_deadline());
+  }
+
+  {
+    FrameDeadline frame_deadline = MakeDefaultDeadline();
+    SendLateBeginFrame(1u);
+    // deadline2's start time is later than deadline2.
+    EXPECT_TRUE(deadline2()->Set(frame_deadline));
+    EXPECT_TRUE(deadline2()->has_deadline());
+  }
+
+  deadline()->InheritFrom(*deadline2());
+  EXPECT_TRUE(deadline()->has_deadline());
+  EXPECT_EQ(deadline()->deadline_for_testing(),
+            deadline2()->deadline_for_testing());
+
+  base::Optional<base::TimeDelta> duration1 = deadline()->Cancel();
+  base::Optional<base::TimeDelta> duration2 = deadline2()->Cancel();
+  ASSERT_TRUE(duration1.has_value());
+  ASSERT_TRUE(duration2.has_value());
+
+  // We inject time on BeginFrameSource::AddObserver and in practice we cannot
+  // know the exact difference in duration between two events a priori so we
+  // just verify that the first event was longer than the second.
+  EXPECT_GT(duration1, duration2);
+}
+
 }  // namespace test
 }  // namespace viz

components/viz/service/surfaces/surface_dependency_tracker.cc

@@ -121,9 +121,6 @@ void SurfaceDependencyTracker::UpdateSurfaceDeadline(Surface* surface) {
 
   const CompositorFrame& pending_frame = surface->GetPendingFrame();
 
-  // Determine an activation deadline for the pending CompositorFrame.
-  bool deadline_changed = false;
-
   // Inherit the deadline from the first parent blocked on this surface.
   auto it = blocked_surfaces_from_dependency_.find(
       surface->surface_id().frame_sink_id());
@@ -133,7 +130,7 @@ void SurfaceDependencyTracker::UpdateSurfaceDeadline(Surface* surface) {
       Surface* parent = surface_manager_->GetSurfaceForId(parent_id);
       if (parent && parent->has_deadline() &&
           parent->activation_dependencies().count(surface->surface_id())) {
-        deadline_changed = surface->InheritActivationDeadlineFrom(parent);
+        surface->InheritActivationDeadlineFrom(parent);
         break;
       }
     }