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

cc/scheduler/scheduler.cc

Index: cc/scheduler/scheduler.cc
diff --git a/cc/scheduler/scheduler.cc b/cc/scheduler/scheduler.cc
index d8f9440227d4b858b6190b63fe3baab87aa513c9..1464f798d1d98f36b81b4b6c639f2eb5c782e772 100644
--- a/cc/scheduler/scheduler.cc
+++ b/cc/scheduler/scheduler.cc
@@ -322,10 +322,7 @@ void Scheduler::BeginImplFrameWithDeadline(const BeginFrameArgs& args) {
   // Discard missed begin frames if they are too late.
   if (adjusted_args.type == BeginFrameArgs::MISSED &&
       now > adjusted_args.deadline) {
-    // TODO(eseckler): Determine and set correct |ack.latest_confirmed_frame|.
-    BeginFrameAck ack(adjusted_args.source_id, adjusted_args.sequence_number,
-                      adjusted_args.sequence_number, 0, false);
-    begin_frame_source_->DidFinishFrame(this, ack);
+    SendBeginFrameAck(adjusted_args, kBeginFrameSkipped);
     return;
   }
 
@@ -335,10 +332,12 @@ void Scheduler::BeginImplFrameWithDeadline(const BeginFrameArgs& args) {
     OnBeginImplFrameDeadline();
     // We may not need begin frames any longer.
     if (!observing_begin_frame_source_) {

[Eric Seckler, 2017/01/18 18:13:03]

I'm trying to test this branch (see scheduler_unittest.cc), but I can't find a
way to set things up so that I end up with !observing_begin_frame_source_ here.
Is there a way that this can be the case?

We can only end up in BeginImplFrameWithDeadline() if
state_machine_.BeginFrameNeeded() was true during OnBeginFrameDerivedImpl().
Plus a previous BeginImplFrame needs to be started but not yet finished.

Thus, if observing_begin_frame_source_ should be false at this point, that means
that some condition affecting state_machine_.BeginFrameNeeded() would have to
have changed during the BeginImplFrameDeadline. The only thing that I can see is
the state machine's needs_redraw_. Yet, if needs_redraw_ becomes false,
did_draw_in_last_frame_ becomes true, and the state machine will request a
proactive BeginFrame. Thus, in this case, state_machine_.BeginFrameNeeded()
wouldn't change.

I've played around with a few different interleavings of missed BeginFrames etc,
but I can't find one that puts me here with !observing_begin_frame_source.

Can you think of a situation that puts me here?

If not, would it make sense to get rid of this branch (replace by DCHECK with
explanation)? It'd also enable us to get rid of
OnBeginFrameDroppedNotObserving(). It would make this bit of code more brittle
to future StateMachine changes, though.

An alternative might be a DisableProactiveBeginFramesForTest() or something
alike that lets me build a test case that exercises this branch?

[brianderson, 2017/01/18 19:53:54]

I think it doesn't show up in tests because the tests don't use IPCs. With IPC's
there can be a BeginFrame in flight after we stop requesting BeginFrames.

You may need to create a new fake BeginFrameSource that has a delay between when
the BeginFrame is created vs. when it is run; where it will still run even if
BeginFramesNeeded becomes false after creation. Or maybe a
DelayedBeginFrameQueue that forwards BeginFrames from a source with a delay.

Would that help?

[Eric Seckler, 2017/01/18 20:27:23]

Hm, I don't think that would help. If there was an IPC delay during which
BeginFrameNeeded becomes false, the BeginFrame would be dropped during
OnBeginFrameDerivedImpl() already as well.

A delay between "BeginFrame created" and "BeginFrame received" wouldn't make a
difference here, I think. What matters it the point in time when it is received,
which I can choose via triggering the fake BFS at the right time.

What I'd be looking for is a situation where:
 1) A new BeginFrame was accepted when it is received by the Scheduler (i.e.,
BeginFrameNeeded is true) before the previous BeginImplFrameDeadline has run.
 2) Running the previous deadline synchronously in the line above has to then
clear the BeginFrameNeeded, so that we drop the new BeginFrame.

I can set up 1) - by issuing a BeginFrame before running the previous
BeginImplFrameDeadline while needs_redraw_ is true - , but I don't see a way to
achieve 2) without disabling proactive BeginFrames.

-      // TODO(eseckler): Determine and set correct |ack.latest_confirmed_frame|.
-      BeginFrameAck ack(adjusted_args.source_id, adjusted_args.sequence_number,
-                        adjusted_args.sequence_number, 0, false);
-      begin_frame_source_->DidFinishFrame(this, ack);
+      // We need to confirm the ignored BeginFrame, since we don't have updates.
+      // To persist the confirmation for future BeginFrameAcks, we let the state
+      // machine know about the BeginFrame.
+      state_machine_.OnBeginFrameDroppedNotObserving(args.source_id,
+                                                     args.sequence_number);
+      SendBeginFrameAck(adjusted_args, kBeginFrameSkipped);
       return;
     }
   }
@@ -397,12 +396,7 @@ void Scheduler::BeginImplFrameWithDeadline(const BeginFrameArgs& args) {
                                       can_activate_before_deadline)) {
     TRACE_EVENT_INSTANT0("cc", "SkipBeginImplFrameToReduceLatency",
                          TRACE_EVENT_SCOPE_THREAD);
-    if (begin_frame_source_) {
-      // TODO(eseckler): Determine and set correct |ack.latest_confirmed_frame|.
-      BeginFrameAck ack(adjusted_args.source_id, adjusted_args.sequence_number,
-                        adjusted_args.sequence_number, 0, false);
-      begin_frame_source_->DidFinishFrame(this, ack);
-    }
+    SendBeginFrameAck(begin_main_frame_args_, kBeginFrameSkipped);
     return;
   }
 
@@ -430,17 +424,32 @@ void Scheduler::FinishImplFrame() {
   ProcessScheduledActions();
 
   client_->DidFinishImplFrame();
-  if (begin_frame_source_) {
-    // TODO(eseckler): Determine and set correct |ack.latest_confirmed_frame|.
-    BeginFrameAck ack(begin_main_frame_args_.source_id,
-                      begin_main_frame_args_.sequence_number,
-                      begin_main_frame_args_.sequence_number, 0,
-                      state_machine_.did_submit_in_last_frame());
-    begin_frame_source_->DidFinishFrame(this, ack);
-  }
+  SendBeginFrameAck(begin_main_frame_args_, kBeginFrameFinished);
   begin_impl_frame_tracker_.Finish();
 }
 
+void Scheduler::SendBeginFrameAck(const BeginFrameArgs& args,
+                                  BeginFrameResult result) {
+  if (!begin_frame_source_)
+    return;
+
+  uint64_t latest_confirmed_frame = BeginFrameArgs::kInvalidFrameNumber;
+  if (args.source_id == state_machine_.begin_frame_source_id()) {
+    latest_confirmed_frame =
+        state_machine_
+            .last_begin_frame_sequence_number_compositor_frame_was_fresh();
+  }
+
+  bool did_submit = false;
+  if (result == kBeginFrameFinished) {
+    did_submit = state_machine_.did_submit_in_last_frame();
+  }
+
+  BeginFrameAck ack(args.source_id, args.sequence_number,
+                    latest_confirmed_frame, 0, did_submit);
+  begin_frame_source_->DidFinishFrame(this, ack);
+}
+
 // BeginImplFrame starts a compositor frame that will wait up until a deadline
 // for a BeginMainFrame+activation to complete before it times out and draws
 // any asynchronous animation and scroll/pinch updates.
@@ -451,7 +460,7 @@ void Scheduler::BeginImplFrame(const BeginFrameArgs& args) {
   DCHECK(state_machine_.HasInitializedCompositorFrameSink());
 
   begin_impl_frame_tracker_.Start(args);
-  state_machine_.OnBeginImplFrame();
+  state_machine_.OnBeginImplFrame(args.source_id, args.sequence_number);
   devtools_instrumentation::DidBeginFrame(layer_tree_host_id_);
   compositor_timing_history_->WillBeginImplFrame(
       state_machine_.NewActiveTreeLikely());