Surface synchronization: Fix deadline setting

Prior to this patch, if BeginFrameSource::AddObserver is slow, it's
possible for SurfaceDependencyDeadline::Set to return true (there is a
deadline) whereas SurfaceDependencyDeadline::has_deadline would return
false because the deadline passed before Set returned. This CL changes
the return value of Set to be has_deadline(). If a deadline hits before
Set returns then Set will now return false.

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

components/viz/common/quads/frame_deadline.h

@@ -7,6 +7,8 @@
 
 #include "components/viz/common/viz_common_export.h"
 
+#include "base/time/time.h"
+
 namespace viz {
 
 class VIZ_COMMON_EXPORT FrameDeadline {

components/viz/service/BUILD.gn

@@ -286,6 +286,7 @@ viz_source_set("unit_tests") {
     "frame_sinks/video_detector_unittest.cc",
     "gl/gpu_service_impl_unittest.cc",
     "hit_test/hit_test_aggregator_unittest.cc",
+    "surfaces/surface_dependency_deadline_unittest.cc",
     "surfaces/surface_hittest_unittest.cc",
     "surfaces/surface_unittest.cc",
   ]

components/viz/service/surfaces/surface_dependency_deadline.cc

@@ -34,9 +34,8 @@ bool SurfaceDependencyDeadline::Set(const FrameDeadline& frame_deadline) {
   start_time_ = frame_deadline.frame_start_time();
   deadline_ = start_time_ + frame_deadline.deadline_in_frames() *
                                 frame_deadline.frame_interval();
-  bool deadline_in_future = deadline_ > tick_clock_->NowTicks();
   begin_frame_source_->AddObserver(this);
-  return deadline_in_future;
+  return has_deadline();
 }
 
 base::Optional<base::TimeDelta> SurfaceDependencyDeadline::Cancel() {

components/viz/service/surfaces/surface_dependency_deadline.h

@@ -8,6 +8,7 @@
 #include "components/viz/common/frame_sinks/begin_frame_source.h"
 
 #include "components/viz/service/surfaces/surface_deadline_client.h"
+#include "components/viz/service/viz_service_export.h"
 
 namespace base {
 class TickClock;
@@ -17,7 +18,7 @@ namespace viz {
 
 class FrameDeadline;
 
-class SurfaceDependencyDeadline : public BeginFrameObserver {
+class VIZ_SERVICE_EXPORT SurfaceDependencyDeadline : public BeginFrameObserver {
  public:
   SurfaceDependencyDeadline(SurfaceDeadlineClient* client,
                             BeginFrameSource* begin_frame_source,

components/viz/service/surfaces/surface_dependency_deadline_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/viz/service/surfaces/surface_dependency_deadline.h"
+#include "components/viz/common/quads/frame_deadline.h"
+#include "components/viz/service/surfaces/surface_deadline_client.h"
+#include "components/viz/test/begin_frame_args_test.h"
+#include "components/viz/test/fake_external_begin_frame_source.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace viz {
+namespace test {
+
+class FakeSurfaceDeadlineClient : public SurfaceDeadlineClient {
+ public:
+  FakeSurfaceDeadlineClient() = default;
+  ~FakeSurfaceDeadlineClient() = default;
+
+  // SurfaceDeadlineClient implementation:
+  void OnDeadline(base::TimeDelta duration) override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FakeSurfaceDeadlineClient);
+};
+
+class FakeSlowBeginFrameSource : public FakeExternalBeginFrameSource {
+ public:
+  FakeSlowBeginFrameSource(double refresh_rate,
+                           bool tick_automatically,
+                           base::SimpleTestTickClock* tick_clock)
+      : FakeExternalBeginFrameSource(refresh_rate, tick_automatically),
+        tick_clock_(tick_clock) {}
+
+  ~FakeSlowBeginFrameSource() override {}
+
+  // FakeExternalBeginFrameSource overrides:
+  void AddObserver(BeginFrameObserver* obs) override {
+    // Advancing time here simulates a slow AddObserver operation.
+    tick_clock_->Advance(BeginFrameArgs::DefaultInterval());
+    FakeExternalBeginFrameSource::AddObserver(obs);
+  }
+
+ private:
+  base::SimpleTestTickClock* const tick_clock_;
+  DISALLOW_COPY_AND_ASSIGN(FakeSlowBeginFrameSource);
+};
+
+class SurfaceDependencyDeadlineTest : public testing::Test {
+ public:
+  SurfaceDependencyDeadlineTest() = default;
+
+  ~SurfaceDependencyDeadlineTest() override {}
+
+  FakeSlowBeginFrameSource* begin_frame_source() {
+    return begin_frame_source_.get();
+  }
+
+  FrameDeadline MakeDefaultDeadline() {
+    return FrameDeadline(now_src_->NowTicks(), 4u,
+                         BeginFrameArgs::DefaultInterval(), false);
+  }
+
+  SurfaceDependencyDeadline* deadline() { return deadline_.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.
+    now_src_->Advance(frames_late * BeginFrameArgs::DefaultInterval());
+    BeginFrameArgs args = begin_frame_source_->CreateBeginFrameArgs(
+        BEGINFRAME_FROM_HERE, now_src_.get());
+    begin_frame_source_->TestOnBeginFrame(args);
+  }
+
+  // testing::Test:
+  void SetUp() override {
+    testing::Test::SetUp();
+
+    now_src_ = std::make_unique<base::SimpleTestTickClock>();
+    begin_frame_source_ =
+        std::make_unique<FakeSlowBeginFrameSource>(0.f, false, now_src_.get());
+
+    deadline_ = std::make_unique<SurfaceDependencyDeadline>(
+        &client_, begin_frame_source_.get(), now_src_.get());
+  }
+
+  void TearDown() override {
+    deadline_->Cancel();
+    deadline_.reset();
+    begin_frame_source_.reset();
+    now_src_.reset();
+  }
+
+ private:
+  std::unique_ptr<base::SimpleTestTickClock> now_src_;
+  std::unique_ptr<FakeSlowBeginFrameSource> begin_frame_source_;
+  FakeSurfaceDeadlineClient client_;
+  std::unique_ptr<SurfaceDependencyDeadline> deadline_;
+
+  DISALLOW_COPY_AND_ASSIGN(SurfaceDependencyDeadlineTest);
+};
+
+// This test verifies that if the FrameDeadline is in the past then
+// SurfaceDependencyDeadline::Set will return false.
+TEST_F(SurfaceDependencyDeadlineTest, DeadlineInPast) {
+  FrameDeadline frame_deadline = MakeDefaultDeadline();
+  SendLateBeginFrame(4u);
+  EXPECT_FALSE(deadline()->Set(frame_deadline));
+  EXPECT_FALSE(deadline()->has_deadline());
+}
+
+// This test verifies that if Set returns false, then SurfaceDependencyDeadline
+// does not have a pending deadline.
+TEST_F(SurfaceDependencyDeadlineTest, SetMatchesHasDeadlineIfFalse) {
+  FrameDeadline frame_deadline = MakeDefaultDeadline();
+  SendLateBeginFrame(3u);
+  EXPECT_FALSE(deadline()->Set(frame_deadline));
+  EXPECT_FALSE(deadline()->has_deadline());
+}
+
+// This test verifies that if Set returns true, then SurfaceDependencyDeadline
+// has a pending deadline.
+TEST_F(SurfaceDependencyDeadlineTest, SetMatchesHasDeadlineIfTrue) {
+  FrameDeadline frame_deadline = MakeDefaultDeadline();
+  SendLateBeginFrame(2u);
+  EXPECT_TRUE(deadline()->Set(frame_deadline));
+  EXPECT_TRUE(deadline()->has_deadline());
+}
+
+}  // namespace test
+}  // namespace viz