cc: Send early DidNotProduceFrame if draw isn't expected

Send an early DidNotProduceFrame if the scheduler uses a late deadline,
and no new frame is expected.  The DisplayScheduler uses this as a
signal to draw if other producers have submitted frames.

This is intended as a short-term fix to solve smoothness issues with
surface for video with 30fps videos.  Ideally the DisplayCompositor will
have some followups as well so that one client that is slow to respond
will not cause smoothness issues with another client.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Icf2f067dd08836de24ea5d0cdc896dc433062d5c
Bug: 874676
Reviewed-on: https://chromium-review.googlesource.com/1211917
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592936}

cc/scheduler/scheduler.cc

@@ -265,7 +265,7 @@ void Scheduler::CancelPendingBeginFrameTask() {
   if (pending_begin_frame_args_.IsValid()) {
     TRACE_EVENT_INSTANT0("cc", "Scheduler::BeginFrameDropped",
                          TRACE_EVENT_SCOPE_THREAD);
-    SendBeginFrameAck(pending_begin_frame_args_, kBeginFrameSkipped);
+    SendDidNotProduceFrame(pending_begin_frame_args_);
     // Make pending begin frame invalid so that we don't accidentally use it.
     pending_begin_frame_args_ = viz::BeginFrameArgs();
   }
@@ -312,7 +312,7 @@ bool Scheduler::OnBeginFrameDerivedImpl(const viz::BeginFrameArgs& args) {
     // Since we don't use the BeginFrame, we may later receive the same
     // BeginFrame again. Thus, we can't confirm it at this point, even though we
     // don't have any updates right now.
-    SendBeginFrameAck(args, kBeginFrameSkipped);
+    SendDidNotProduceFrame(args);
     return false;
   }
 
@@ -340,7 +340,7 @@ bool Scheduler::OnBeginFrameDerivedImpl(const viz::BeginFrameArgs& args) {
     if (pending_begin_frame_args_.IsValid()) {
       TRACE_EVENT_INSTANT0("cc", "Scheduler::BeginFrameDropped",
                            TRACE_EVENT_SCOPE_THREAD);
-      SendBeginFrameAck(pending_begin_frame_args_, kBeginFrameSkipped);
+      SendDidNotProduceFrame(pending_begin_frame_args_);
     }
     pending_begin_frame_args_ = args;
     // ProcessScheduledActions() will post the previous frame's deadline if it
@@ -415,7 +415,7 @@ void Scheduler::BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args) {
     TRACE_EVENT_INSTANT0("cc", "Scheduler::MissedBeginFrameDropped",
                          TRACE_EVENT_SCOPE_THREAD);
     skipped_last_frame_missed_exceeded_deadline_ = true;
-    SendBeginFrameAck(args, kBeginFrameSkipped);
+    SendDidNotProduceFrame(args);
     return;
   }
   skipped_last_frame_missed_exceeded_deadline_ = false;
@@ -515,7 +515,7 @@ void Scheduler::BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args) {
     TRACE_EVENT_INSTANT0("cc", "SkipBeginImplFrameToReduceLatency",
                          TRACE_EVENT_SCOPE_THREAD);
     skipped_last_frame_to_reduce_latency_ = true;
-    SendBeginFrameAck(begin_main_frame_args_, kBeginFrameSkipped);
+    SendDidNotProduceFrame(args);
     return;
   }
 
@@ -545,7 +545,9 @@ void Scheduler::FinishImplFrame() {
   // Send ack before calling ProcessScheduledActions() because it might send an
   // ack for any pending begin frame if we are going idle after this. This
   // ensures that the acks are sent in order.
-  SendBeginFrameAck(begin_main_frame_args_, kBeginFrameFinished);
+  if (!state_machine_.did_submit_in_last_frame())
+    SendDidNotProduceFrame(begin_impl_frame_tracker_.Current());
+
   begin_impl_frame_tracker_.Finish();
 
   ProcessScheduledActions();
@@ -554,24 +556,19 @@ void Scheduler::FinishImplFrame() {
     base::AutoReset<bool> mark_inside(&inside_scheduled_action_, true);
     client_->DidFinishImplFrame();
   }
-}
-
-void Scheduler::SendBeginFrameAck(const viz::BeginFrameArgs& args,
-                                  BeginFrameResult result) {
-  bool did_submit = false;
-  if (result == kBeginFrameFinished)
-    did_submit = state_machine_.did_submit_in_last_frame();
-
-  if (!did_submit) {
-    DCHECK(!inside_scheduled_action_);
-    base::AutoReset<bool> mark_inside(&inside_scheduled_action_, true);
-    client_->DidNotProduceFrame(viz::BeginFrameAck(args, did_submit));
-  }
 
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this);
 }
 
+void Scheduler::SendDidNotProduceFrame(const viz::BeginFrameArgs& args) {
+  if (last_begin_frame_ack_.source_id == args.source_id &&
+      last_begin_frame_ack_.sequence_number == args.sequence_number)
+    return;
+  last_begin_frame_ack_ = viz::BeginFrameAck(args, false /* has_damage */);
+  client_->DidNotProduceFrame(last_begin_frame_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.
@@ -605,21 +602,18 @@ void Scheduler::BeginImplFrame(const viz::BeginFrameArgs& args,
 }
 
 void Scheduler::ScheduleBeginImplFrameDeadline() {
-  base::TimeTicks new_deadline;
+  using DeadlineMode = SchedulerStateMachine::BeginImplFrameDeadlineMode;
+  deadline_mode_ = state_machine_.CurrentBeginImplFrameDeadlineMode();
 
-  begin_impl_frame_deadline_mode_ =
-      state_machine_.CurrentBeginImplFrameDeadlineMode();
-  // Avoid using Now() for immediate deadlines because it's expensive, and this
-  // method is called in every ProcessScheduledActions() call. Using
-  // base::TimeTicks() achieves the same result.
-  switch (begin_impl_frame_deadline_mode_) {
-    case SchedulerStateMachine::BeginImplFrameDeadlineMode::NONE:
+  base::TimeTicks new_deadline;
+  switch (deadline_mode_) {
+    case DeadlineMode::NONE:
       // NONE is returned when deadlines aren't used (synchronous compositor),
       // or when outside a begin frame. In either case deadline task shouldn't
       // be posted or should be cancelled already.
       DCHECK(begin_impl_frame_deadline_task_.IsCancelled());
       return;
-    case SchedulerStateMachine::BeginImplFrameDeadlineMode::BLOCKED: {
+    case DeadlineMode::BLOCKED: {
       // TODO(sunnyps): Posting the deadline for pending begin frame is required
       // for browser compositor (commit_to_active_tree) to make progress in some
       // cases. Change browser compositor deadline to LATE in state machine to
@@ -631,37 +625,44 @@ void Scheduler::ScheduleBeginImplFrameDeadline() {
       bool has_pending_begin_frame = pending_begin_frame_args_.IsValid();
       if (has_pending_begin_frame) {
         new_deadline = base::TimeTicks();
+        break;
       } else {
         begin_impl_frame_deadline_task_.Cancel();
         return;
       }
-      break;
     }
-    case SchedulerStateMachine::BeginImplFrameDeadlineMode::LATE:
-      // We are waiting for a commit without needing active tree draw or we have
-      // nothing to do.
+    case DeadlineMode::LATE: {
+      // We are waiting for a commit without needing active tree draw or we
+      // have nothing to do.
       new_deadline = begin_impl_frame_tracker_.Current().frame_time +
                      begin_impl_frame_tracker_.Current().interval;
+      // Send early DidNotProduceFrame if we don't expect to produce a frame
+      // soon so that display scheduler doesn't wait unnecessarily.
+      // Note: This will only send one DidNotProduceFrame ack per begin frame.
+      if (!state_machine_.NewActiveTreeLikely())
+        SendDidNotProduceFrame(begin_impl_frame_tracker_.Current());
       break;
-    case SchedulerStateMachine::BeginImplFrameDeadlineMode::REGULAR:
+    }
+    case DeadlineMode::REGULAR:
       // We are animating the active tree but we're also waiting for commit.
       new_deadline = begin_impl_frame_tracker_.Current().deadline;
       break;
-    case SchedulerStateMachine::BeginImplFrameDeadlineMode::IMMEDIATE:
+    case DeadlineMode::IMMEDIATE:
+      // Avoid using Now() for immediate deadlines because it's expensive, and
+      // this method is called in every ProcessScheduledActions() call. Using
+      // base::TimeTicks() achieves the same result.
       new_deadline = base::TimeTicks();
       break;
   }
 
-  bool has_no_deadline_task = begin_impl_frame_deadline_task_.IsCancelled();
   // Post deadline task only if we didn't have one already or something caused
-  // us to change the deadline. Comparing deadline mode is not sufficient
-  // because the calculated deadline also depends on whether we have a pending
-  // begin frame which the state machine doesn't know about.
+  // us to change the deadline.
+  bool has_no_deadline_task = begin_impl_frame_deadline_task_.IsCancelled();
   if (has_no_deadline_task || new_deadline != deadline_) {
     TRACE_EVENT2("cc", "Scheduler::ScheduleBeginImplFrameDeadline",
                  "new deadline", new_deadline, "deadline mode",
                  SchedulerStateMachine::BeginImplFrameDeadlineModeToString(
-                     begin_impl_frame_deadline_mode_));
+                     deadline_mode_));
     deadline_ = new_deadline;
     deadline_scheduled_at_ = Now();
 
@@ -862,9 +863,9 @@ void Scheduler::AsValueInto(base::trace_event::TracedValue* state) const {
                     skipped_last_frame_to_reduce_latency_);
   state->SetString("inside_action",
                    SchedulerStateMachine::ActionToString(inside_action_));
-  state->SetString("begin_impl_frame_deadline_mode",
+  state->SetString("deadline_mode",
                    SchedulerStateMachine::BeginImplFrameDeadlineModeToString(
-                       begin_impl_frame_deadline_mode_));
+                       deadline_mode_));
 
   state->SetDouble("deadline_ms", deadline_.since_origin().InMillisecondsF());
   state->SetDouble("deadline_scheduled_at_ms",

cc/scheduler/scheduler.h

@@ -194,13 +194,12 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
   std::unique_ptr<CompositorTimingHistory> compositor_timing_history_;
 
   // What the latest deadline was, and when it was scheduled.
-  SchedulerStateMachine::BeginImplFrameDeadlineMode
-      begin_impl_frame_deadline_mode_ =
-          SchedulerStateMachine::BeginImplFrameDeadlineMode::NONE;
   base::TimeTicks deadline_;
   base::TimeTicks deadline_scheduled_at_;
+  SchedulerStateMachine::BeginImplFrameDeadlineMode deadline_mode_;
 
   BeginFrameTracker begin_impl_frame_tracker_;
+  viz::BeginFrameAck last_begin_frame_ack_;
   viz::BeginFrameArgs begin_main_frame_args_;
 
   // Task posted for the deadline or drawing phase of the scheduler. This task
@@ -271,9 +270,7 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
   void BeginImplFrameSynchronous(const viz::BeginFrameArgs& args);
   void BeginImplFrame(const viz::BeginFrameArgs& args, base::TimeTicks now);
   void FinishImplFrame();
-  enum BeginFrameResult { kBeginFrameSkipped, kBeginFrameFinished };
-  void SendBeginFrameAck(const viz::BeginFrameArgs& args,
-                         BeginFrameResult result);
+  void SendDidNotProduceFrame(const viz::BeginFrameArgs& args);
   void OnBeginImplFrameDeadline();
   void PollToAdvanceCommitState();
   void BeginMainFrameAnimateAndLayoutOnly(const viz::BeginFrameArgs& args);

cc/scheduler/scheduler_unittest.cc

@@ -4142,5 +4142,23 @@ TEST_F(SchedulerTest, NoInvalidationForAnimateOnlyFrames) {
   EXPECT_ACTIONS();
 }
 
+TEST_F(SchedulerTest, SendEarlyDidNotProduceFrameIfIdle) {
+  SetUpScheduler(EXTERNAL_BFS);
+  scheduler_->SetNeedsBeginMainFrame();
+
+  client_->Reset();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_ACTIONS("WillBeginImplFrame", "ScheduledActionSendBeginMainFrame");
+  auto begin_main_frame_args = client_->last_begin_main_frame_args();
+  EXPECT_NE(client_->last_begin_frame_ack().sequence_number,
+            begin_main_frame_args.sequence_number);
+
+  client_->Reset();
+  scheduler_->NotifyBeginMainFrameStarted(task_runner_->NowTicks());
+  scheduler_->BeginMainFrameAborted(CommitEarlyOutReason::FINISHED_NO_UPDATES);
+  EXPECT_EQ(client_->last_begin_frame_ack().sequence_number,
+            begin_main_frame_args.sequence_number);
+}
+
 }  // namespace
 }  // namespace cc