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

cc/scheduler/scheduler.cc

 // Copyright 2011 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 "cc/scheduler/scheduler.h"
 
 #include <algorithm>
 
 #include "base/auto_reset.h"
 #include "base/logging.h"
@@ skipping 304 matching lines @@
   BeginFrameArgs adjusted_args = args;
   // Cancel the missed begin frame task in case the BFS sends a begin frame
   // before the missed frame task runs.
   missed_begin_frame_task_.Cancel();
 
   base::TimeTicks now = Now();
 
   // 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;
   }
 
   // Run the previous deadline if any.
   if (state_machine_.begin_impl_frame_state() ==
       SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_INSIDE_BEGIN_FRAME) {
     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;
     }
   }
   DCHECK_EQ(state_machine_.begin_impl_frame_state(),
             SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
 
   bool main_thread_is_in_high_latency_mode =
       state_machine_.main_thread_missed_last_deadline();
   TRACE_EVENT2("cc,benchmark", "Scheduler::BeginImplFrame", "args",
                adjusted_args.AsValue(), "main_thread_missed_last_deadline",
@@ skipping 38 matching lines @@
                                                  bmf_to_activate_estimate, now);
 
   if (ShouldRecoverMainLatency(adjusted_args, can_activate_before_deadline)) {
     TRACE_EVENT_INSTANT0("cc", "SkipBeginMainFrameToReduceLatency",
                          TRACE_EVENT_SCOPE_THREAD);
     state_machine_.SetSkipNextBeginMainFrameToReduceLatency();
   } else if (ShouldRecoverImplLatency(adjusted_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;
   }
 
   BeginImplFrame(adjusted_args);
 }
 
 void Scheduler::BeginImplFrameSynchronous(const BeginFrameArgs& args) {
   TRACE_EVENT1("cc,benchmark", "Scheduler::BeginImplFrame", "args",
                args.AsValue());
 
   // The main thread currently can't commit before we draw with the
   // synchronous compositor, so never consider the BeginMainFrame fast.
   state_machine_.SetCriticalBeginMainFrameToActivateIsFast(false);
   begin_main_frame_args_ = args;
   begin_main_frame_args_.on_critical_path = !ImplLatencyTakesPriority();
 
   BeginImplFrame(args);
   compositor_timing_history_->WillFinishImplFrame(
       state_machine_.needs_redraw());
   FinishImplFrame();
 }
 
 void Scheduler::FinishImplFrame() {
   state_machine_.OnBeginImplFrameIdle();
   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();
   }
-  begin_impl_frame_tracker_.Finish();
+
+  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.
 void Scheduler::BeginImplFrame(const BeginFrameArgs& args) {
   DCHECK_EQ(state_machine_.begin_impl_frame_state(),
             SchedulerStateMachine::BEGIN_IMPL_FRAME_STATE_IDLE);
   DCHECK(begin_impl_frame_deadline_task_.IsCancelled());
   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());
   client_->WillBeginImplFrame(begin_impl_frame_tracker_.Current());
 
   ProcessScheduledActions();
 }
 
 void Scheduler::ScheduleBeginImplFrameDeadline() {
   // The synchronous compositor does not post a deadline task.
@@ skipping 302 matching lines @@
 }
 
 bool Scheduler::IsBeginMainFrameSentOrStarted() const {
   return (state_machine_.begin_main_frame_state() ==
               SchedulerStateMachine::BEGIN_MAIN_FRAME_STATE_SENT ||
           state_machine_.begin_main_frame_state() ==
               SchedulerStateMachine::BEGIN_MAIN_FRAME_STATE_STARTED);
 }
 
 }  // namespace cc