[cc] Calculate the correct latest_confirmed_sequence_number in cc::Scheduler.

cc/scheduler/scheduler_unittest.cc

Index: cc/scheduler/scheduler_unittest.cc
diff --git a/cc/scheduler/scheduler_unittest.cc b/cc/scheduler/scheduler_unittest.cc
index f3bf54b89eb0b412472126725fd315fe5ac97e16..3120133d6d651e1769e40eba0aebda2d3037a931 100644
--- a/cc/scheduler/scheduler_unittest.cc
+++ b/cc/scheduler/scheduler_unittest.cc
@@ -238,8 +238,7 @@ class SchedulerTest : public testing::Test {
       : now_src_(new base::SimpleTestTickClock()),
         task_runner_(new OrderedSimpleTaskRunner(now_src_.get(), true)),
         fake_external_begin_frame_source_(nullptr),
-        fake_compositor_timing_history_(nullptr),
-        next_begin_frame_number_(BeginFrameArgs::kStartingFrameNumber) {
+        fake_compositor_timing_history_(nullptr) {
     now_src_->Advance(base::TimeDelta::FromMicroseconds(10000));
     // A bunch of tests require NowTicks()
     // to be > BeginFrameArgs::DefaultInterval()
@@ -408,10 +407,9 @@ class SchedulerTest : public testing::Test {
     // Creep the time forward so that any BeginFrameArgs is not equal to the
     // last one otherwise we violate the BeginFrameSource contract.
     now_src_->Advance(BeginFrameArgs::DefaultInterval());
-    BeginFrameArgs args = CreateBeginFrameArgsForTesting(
-        BEGINFRAME_FROM_HERE, fake_external_begin_frame_source_->source_id(),
-        next_begin_frame_number_, now_src());
-    next_begin_frame_number_++;
+    BeginFrameArgs args =
+        fake_external_begin_frame_source_->CreateBeginFrameArgs(
+            BEGINFRAME_FROM_HERE, now_src());
     fake_external_begin_frame_source_->TestOnBeginFrame(args);
     return args;
   }
@@ -440,7 +438,6 @@ class SchedulerTest : public testing::Test {
   std::unique_ptr<FakeSchedulerClient> client_;
   std::unique_ptr<TestScheduler> scheduler_;
   FakeCompositorTimingHistory* fake_compositor_timing_history_;
-  uint64_t next_begin_frame_number_;
 };
 
 TEST_F(SchedulerTest, InitializeCompositorFrameSinkDoesNotBeginImplFrame) {
@@ -1338,10 +1335,8 @@ TEST_F(SchedulerTest, MainFrameNotSkippedAfterLateBeginFrame) {
 
   // Advance frame and create a begin frame.
   now_src_->Advance(BeginFrameArgs::DefaultInterval());
-  BeginFrameArgs args = CreateBeginFrameArgsForTesting(
-      BEGINFRAME_FROM_HERE, fake_external_begin_frame_source_->source_id(),
-      next_begin_frame_number_, now_src());
-  next_begin_frame_number_++;
+  BeginFrameArgs args = fake_external_begin_frame_source_->CreateBeginFrameArgs(
+      BEGINFRAME_FROM_HERE, now_src());
 
   // Deliver this begin frame super late.
   now_src_->Advance(BeginFrameArgs::DefaultInterval() * 100);
@@ -2858,30 +2853,53 @@ TEST_F(SchedulerTest, SetNeedsOneBeginImplFrame) {
   EXPECT_ACTION("SendBeginMainFrameNotExpectedSoon", client_, 1, 2);
 }
 
-TEST_F(SchedulerTest, SynchronousCompositorCommit) {
+TEST_F(SchedulerTest, SynchronousCompositorCommitAndVerifyBeginFrameAcks) {
   scheduler_settings_.using_synchronous_renderer_compositor = true;
   SetUpScheduler(EXTERNAL_BFS);
 
+  // Expect the last ack to be for last BeginFrame, which didn't cause damage.
+  uint64_t last_begin_frame_number =
+      fake_external_begin_frame_source_->next_begin_frame_number() - 1;
+  uint64_t latest_confirmed_sequence_number = last_begin_frame_number;
+  bool has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(fake_external_begin_frame_source_->source_id(),
+                    last_begin_frame_number, latest_confirmed_sequence_number,
+                    0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+
   scheduler_->SetNeedsBeginMainFrame();
   EXPECT_SINGLE_ACTION("AddObserver(this)", client_);
   client_->Reset();
 
   // Next vsync.
-  AdvanceFrame();
+  BeginFrameArgs args = SendNextBeginFrame();
   EXPECT_ACTION("WillBeginImplFrame", client_, 0, 2);
   EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 1, 2);
   EXPECT_FALSE(client_->IsInsideBeginImplFrame());
   client_->Reset();
 
+  has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+
   scheduler_->NotifyBeginMainFrameStarted(base::TimeTicks());
   EXPECT_NO_ACTION(client_);
 
   // Next vsync.
-  AdvanceFrame();
+  args = SendNextBeginFrame();
   EXPECT_SINGLE_ACTION("WillBeginImplFrame", client_);
   EXPECT_FALSE(client_->IsInsideBeginImplFrame());
   client_->Reset();
 
+  has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+
   scheduler_->NotifyReadyToCommit();
   EXPECT_SINGLE_ACTION("ScheduledActionCommit", client_);
   client_->Reset();
@@ -2891,12 +2909,22 @@ TEST_F(SchedulerTest, SynchronousCompositorCommit) {
   client_->Reset();
 
   // Next vsync.
-  AdvanceFrame();
+  args = SendNextBeginFrame();
   EXPECT_ACTION("WillBeginImplFrame", client_, 0, 2);
   EXPECT_ACTION("ScheduledActionInvalidateCompositorFrameSink", client_, 1, 2);
   EXPECT_FALSE(client_->IsInsideBeginImplFrame());
   client_->Reset();
 
+  // Not confirmed yet and no damage, since not drawn yet.
+  // TODO(eseckler): In the future, |has_damage = false| will prevent us from
+  // filtering this ack (in CompositorExternalBeginFrameSource) and instead
+  // forwarding the one attached to the later submitted CompositorFrame.

[Eric Seckler, 2017/02/22 12:09:01]

Here's the problem I mentioned :)

Because cc::Scheduler finishes the BeginFrame during
BeginImplFrameSynchronous(), no drawing has happened yet, so has_damage in the
ack is false - and the BeginFrame is not confirmed.

I wouldn't mind that it isn't confirmed yet, because the actual ack for cases
where drawing happens would (eventually) be provided together with the
CompositorFrame through the Draw. However, for that to work, this Ack needs to
be ignored by CompositorExternalBeginFrameSource (so that it is not forwarded to
the browser compositor). In my prototype, I use the has_damage field to decide
whether the ack should be packaged with the CompositorFrame. But that's |false|
here...

I'm considering whether it makes sense to change the cc::Scheduler to wait with
finishing the BeginFrame in sync-renderer-comp mode in case a draw is pending?

In any case, I guess it's not super important since sync-renderer-comp mode
isn't used with a DisplayScheduler or in headless mode, so there's currently no
use case for BeginFrameAcks there. Should we leave it as TODO?

[brianderson, 2017/02/22 23:27:05]

We can leave it as a TODO for now and follow up when needed.

+boliu and sunnyps as FYI: Does WebView ever have to wait for multiple
producers, where a BeginFrameAck when there was no damage might be useful.

[boliu, 2017/02/23 05:28:24]

Webview does not have a "browser" compositor, so the "display" compositor only
needs to wait for the renderer. So only thing I can think of that can use
multiple producers is OOPIF, which isn't hooked up in webview rendering yet, so
I guess no?

OTOH, it's really not possible to determine damage in BeginFrame in webview,
because input to that frame could still be changing. So the best cc can hope to
do is "is there damage so far", which probably isn't all that useful.

+  has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+
   // Android onDraw.
   scheduler_->SetNeedsRedraw();
   bool resourceless_software_draw = false;
@@ -2906,12 +2934,19 @@ TEST_F(SchedulerTest, SynchronousCompositorCommit) {
   client_->Reset();
 
   // Idle on next vsync.
-  AdvanceFrame();
+  args = SendNextBeginFrame();
   EXPECT_ACTION("WillBeginImplFrame", client_, 0, 3);
   EXPECT_ACTION("RemoveObserver(this)", client_, 1, 3);
   EXPECT_ACTION("SendBeginMainFrameNotExpectedSoon", client_, 2, 3);
   EXPECT_FALSE(client_->IsInsideBeginImplFrame());
   client_->Reset();
+
+  latest_confirmed_sequence_number = args.sequence_number;
+  has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
 }
 
 TEST_F(SchedulerTest, SynchronousCompositorDoubleCommitWithoutDraw) {
@@ -3256,5 +3291,261 @@ TEST_F(SchedulerTest, BeginMainFrameOnCriticalPath_AHS) {
       ScrollHandlerState::SCROLL_AFFECTS_SCROLL_HANDLER, kSlowDuration));
 }
 
+TEST_F(SchedulerTest, BeginFrameAckForFinishedImplFrame) {
+  // Sets up scheduler and sends two BeginFrames, both finished.
+  SetUpScheduler(EXTERNAL_BFS);
+
+  // Expect the last ack to be for last BeginFrame, which didn't cause damage.
+  uint64_t last_begin_frame_number =
+      fake_external_begin_frame_source_->next_begin_frame_number() - 1;
+  uint64_t latest_confirmed_sequence_number = last_begin_frame_number;
+  bool has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(fake_external_begin_frame_source_->source_id(),
+                    last_begin_frame_number, latest_confirmed_sequence_number,
+                    0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+
+  // Run a successful redraw and verify that a new ack is sent.
+  scheduler_->SetNeedsRedraw();
+  client_->Reset();
+
+  BeginFrameArgs args = SendNextBeginFrame();
+  EXPECT_LT(latest_confirmed_sequence_number, args.sequence_number);
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 1);
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  task_runner().RunPendingTasks();  // Run posted deadline.
+  EXPECT_ACTION("ScheduledActionDrawIfPossible", client_, 0, 1);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  // Successful draw caused damage.
+  latest_confirmed_sequence_number = args.sequence_number;
+  has_damage = true;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+
+  // Request another redraw, but fail it. Verify that a new ack is sent, but
+  // that its |latest_confirmed_sequence_number| didn't change.
+  scheduler_->SetNeedsRedraw();
+  client_->Reset();
+
+  args = SendNextBeginFrame();
+  EXPECT_LT(latest_confirmed_sequence_number, args.sequence_number);
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 1);
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  client_->SetDrawWillHappen(false);
+  task_runner().RunPendingTasks();  // Run posted deadline.
+  EXPECT_ACTION("ScheduledActionDrawIfPossible", client_, 0, 2);
+  // Failed draw triggers SendBeginMainFrame.
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 1, 2);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  // Failed draw: no damage and unconfirmed frame.
+  has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+}
+
+TEST_F(SchedulerTest, BeginFrameAckForSkippedImplFrame) {
+  SetUpScheduler(EXTERNAL_BFS);
+
+  // To get into a high latency state, this test disables automatic swap acks.
+  client_->SetAutomaticSubmitCompositorFrameAck(false);
+  fake_compositor_timing_history_->SetAllEstimatesTo(kFastDuration);
+
+  // Run a successful redraw that submits a compositor frame but doesn't receive
+  // a swap ack. Verify that a BeginFrameAck is sent for it.
+  scheduler_->SetNeedsRedraw();
+  client_->Reset();
+
+  BeginFrameArgs args = SendNextBeginFrame();
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 1);
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  task_runner().RunPendingTasks();  // Run posted deadline.
+  EXPECT_ACTION("ScheduledActionDrawIfPossible", client_, 0, 1);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  // Successful draw caused damage.
+  uint64_t latest_confirmed_sequence_number = args.sequence_number;
+  bool has_damage = true;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+
+  // Request another redraw that will be skipped because the swap ack is still
+  // missing. Verify that a new BeginFrameAck is sent.
+  scheduler_->SetNeedsRedraw();
+  client_->Reset();
+
+  args = SendNextBeginFrame();
+  EXPECT_LT(latest_confirmed_sequence_number, args.sequence_number);
+  EXPECT_NO_ACTION(client_);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  // Skipped draw: no damage and unconfirmed frame.
+  has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+}
+
+TEST_F(SchedulerTest, BeginFrameAckForBeginFrameBeforeLastDeadline) {
+  SetUpScheduler(EXTERNAL_BFS);
+
+  // Request tile preparation to schedule a proactive BeginFrame.
+  scheduler_->SetNeedsPrepareTiles();
+  client_->Reset();
+
+  SendNextBeginFrame();
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 1);
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
+  // Until tiles were prepared, further proactive BeginFrames are expected.
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  // Send the next BeginFrame before the previous one's deadline was executed.
+  // This should trigger the previous BeginFrame's deadline synchronously,
+  // during which tiles will be prepared. As a result of that, no further
+  // BeginFrames will be needed, and the new BeginFrame should be dropped.
+  BeginFrameArgs args = SendNextBeginFrame();
+  EXPECT_ACTION("ScheduledActionPrepareTiles", client_, 0, 3);
+  EXPECT_ACTION("RemoveObserver(this)", client_, 1, 3);
+  EXPECT_ACTION("SendBeginMainFrameNotExpectedSoon", client_, 2, 3);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  EXPECT_FALSE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  // Latest ack should be for the dropped BeginFrame. Since we don't have
+  // further updates, its |latest_confirmed_sequence_number| should be for the
+  // dropped BeginFrame, too.
+  uint64_t latest_confirmed_sequence_number = args.sequence_number;
+  bool has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+}
+
+TEST_F(SchedulerTest, BeginFrameAckForLateMissedBeginFrame) {
+  SetUpScheduler(EXTERNAL_BFS);
+
+  // Last confirmed frame was last BeginFrame.
+  uint64_t latest_confirmed_sequence_number =
+      fake_external_begin_frame_source_->next_begin_frame_number() - 1;
+
+  scheduler_->SetNeedsRedraw();
+  client_->Reset();
+
+  // Send a missed BeginFrame with a passed deadline.
+  now_src_->Advance(BeginFrameArgs::DefaultInterval());
+  BeginFrameArgs args = fake_external_begin_frame_source_->CreateBeginFrameArgs(
+      BEGINFRAME_FROM_HERE, now_src());
+  args.type = BeginFrameArgs::MISSED;
+  now_src_->Advance(BeginFrameArgs::DefaultInterval());
+  EXPECT_GT(now_src_->NowTicks(), args.deadline);
+  fake_external_begin_frame_source_->TestOnBeginFrame(args);
+
+  EXPECT_NO_ACTION(client_);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  client_->Reset();
+
+  // Latest ack should be for the missed BeginFrame that was too late: no damage
+  // and unconfirmed frame.
+  bool has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+}
+
+TEST_F(SchedulerTest, BeginFrameAckForFinishedBeginFrameWithNewSourceId) {
+  SetUpScheduler(EXTERNAL_BFS);
+
+  scheduler_->SetNeedsRedraw();
+  client_->Reset();
+
+  // Send a BeginFrame with a different source_id.
+  now_src_->Advance(BeginFrameArgs::DefaultInterval());
+  uint32_t source_id = fake_external_begin_frame_source_->source_id() + 1;
+  BeginFrameArgs args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE,
+                                                       source_id, 1, now_src());
+  fake_external_begin_frame_source_->TestOnBeginFrame(args);
+
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 1);
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  task_runner().RunPendingTasks();  // Run posted deadline.
+  EXPECT_ACTION("ScheduledActionDrawIfPossible", client_, 0, 1);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  EXPECT_TRUE(scheduler_->begin_frames_expected());
+  client_->Reset();
+
+  // Successful draw caused damage.
+  uint64_t latest_confirmed_sequence_number = args.sequence_number;
+  bool has_damage = true;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+}
+
+TEST_F(SchedulerTest,
+       BeginFrameAckForLateMissedBeginFrameWithDifferentSourceId) {
+  SetUpScheduler(EXTERNAL_BFS);
+
+  scheduler_->SetNeedsRedraw();
+  client_->Reset();
+
+  // Send a missed BeginFrame with a passed deadline and different source_id.
+  now_src_->Advance(BeginFrameArgs::DefaultInterval());
+  uint32_t source_id = fake_external_begin_frame_source_->source_id() + 1;
+  BeginFrameArgs args = CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE,
+                                                       source_id, 1, now_src());
+  args.type = BeginFrameArgs::MISSED;
+  now_src_->Advance(BeginFrameArgs::DefaultInterval());
+  EXPECT_GT(now_src_->NowTicks(), args.deadline);
+  fake_external_begin_frame_source_->TestOnBeginFrame(args);
+
+  EXPECT_NO_ACTION(client_);
+  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  client_->Reset();
+
+  // Latest ack should be for the missed BeginFrame that was too late: no damage
+  // and unconfirmed frame. Because the source_id changed, the
+  // |latest_confirmed_sequence_number| should be set to invalid.
+  uint64_t latest_confirmed_sequence_number =
+      BeginFrameArgs::kInvalidFrameNumber;
+  bool has_damage = false;
+  EXPECT_EQ(
+      BeginFrameAck(args.source_id, args.sequence_number,
+                    latest_confirmed_sequence_number, 0, has_damage),
+      fake_external_begin_frame_source_->LastAckForObserver(scheduler_.get()));
+}
+
 }  // namespace
 }  // namespace cc