[cc webview] Defer finishing a 'frame' when draw is expected to happen.

In the regular async multi-threaded setting, the compositor submits a
CompositorFrame 'during a frame', i.e. between BeginImplFrame() and
FinishImplFrame().

In the sync setting for webview, the CompositorFrame is submitted 'after
a frame', i.e. after both BeginImplFrame() and FinishImplFrame() has
been called. This change defers the FinishImplFrame() to happen after
the CompositorFrame is submitted (when a draw is anticipated). This
change brings the webview configuration a little closer to the regular
configuration. In the process, it also fixes the PercentDroppedFrames
metrics for webview.

BUG=1136519

Change-Id: I924c772d57cf2b5210d324e4da7acbe877aa298f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2469779
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820251}

cc/scheduler/scheduler.cc

@@ -277,7 +277,10 @@ void Scheduler::StartOrStopBeginFrames() {
     if (begin_frame_source_)
       begin_frame_source_->RemoveObserver(this);
     // We're going idle so drop pending begin frame.
+    if (settings_.using_synchronous_renderer_compositor)
+      FinishImplFrameSynchronous();
     CancelPendingBeginFrameTask();
+
     compositor_timing_history_->BeginImplFrameNotExpectedSoon();
     devtools_instrumentation::NeedsBeginFrameChanged(layer_tree_host_id_,
                                                      false);
@@ -404,14 +407,19 @@ void Scheduler::SetVideoNeedsBeginFrames(bool video_needs_begin_frames) {
 void Scheduler::OnDrawForLayerTreeFrameSink(bool resourceless_software_draw,
                                             bool skip_draw) {
   DCHECK(settings_.using_synchronous_renderer_compositor);
-  DCHECK_EQ(state_machine_.begin_impl_frame_state(),
-            SchedulerStateMachine::BeginImplFrameState::IDLE);
+  if (state_machine_.begin_impl_frame_state() ==
+      SchedulerStateMachine::BeginImplFrameState::INSIDE_BEGIN_FRAME) {
+    DCHECK(needs_finish_frame_for_synchronous_compositor_);
+  } else {
+    DCHECK_EQ(state_machine_.begin_impl_frame_state(),
+              SchedulerStateMachine::BeginImplFrameState::IDLE);
+    DCHECK(!needs_finish_frame_for_synchronous_compositor_);
+  }
   DCHECK(begin_impl_frame_deadline_task_.IsCancelled());
 
   state_machine_.SetResourcelessSoftwareDraw(resourceless_software_draw);
   state_machine_.SetSkipDraw(skip_draw);
-  state_machine_.OnBeginImplFrameDeadline();
-  ProcessScheduledActions();
+  OnBeginImplFrameDeadline();
 
   state_machine_.OnBeginImplFrameIdle();
   ProcessScheduledActions();
@@ -561,6 +569,9 @@ void Scheduler::BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args) {
 }
 
 void Scheduler::BeginImplFrameSynchronous(const viz::BeginFrameArgs& args) {
+  // Finish the previous frame (if needed) before starting a new one.
+  FinishImplFrameSynchronous();
+
   TRACE_EVENT1("cc,benchmark", "Scheduler::BeginImplFrame", "args",
                args.AsValue());
   // The main thread currently can't commit before we draw with the
@@ -572,10 +583,17 @@ void Scheduler::BeginImplFrameSynchronous(const viz::BeginFrameArgs& args) {
   BeginImplFrame(args, Now());
   compositor_timing_history_->WillFinishImplFrame(state_machine_.needs_redraw(),
                                                   args.frame_id);
-  FinishImplFrame();
+  // Delay the call to |FinishFrame()| if a draw is anticipated, so that it is
+  // called after the draw happens (in |OnDrawForLayerTreeFrameSink()|).
+  needs_finish_frame_for_synchronous_compositor_ = true;
+  if (!state_machine_.did_invalidate_layer_tree_frame_sink()) {
+    // If there was no invalidation, then finish the frame immediately.
+    FinishImplFrameSynchronous();
+  }
 }
 
 void Scheduler::FinishImplFrame() {
+  DCHECK(!needs_finish_frame_for_synchronous_compositor_);
   state_machine_.OnBeginImplFrameIdle();
 
   // Send ack before calling ProcessScheduledActions() because it might send an
@@ -727,11 +745,26 @@ void Scheduler::OnBeginImplFrameDeadline() {
   //     order to wait for more user-input before starting the next commit.
   // * Creating a new OuputSurface will not occur during the deadline in
   //     order to allow the state machine to "settle" first.
-  compositor_timing_history_->WillFinishImplFrame(
-      state_machine_.needs_redraw(), begin_main_frame_args_.frame_id);
+  if (!settings_.using_synchronous_renderer_compositor) {
+    compositor_timing_history_->WillFinishImplFrame(
+        state_machine_.needs_redraw(), begin_main_frame_args_.frame_id);
+  }
+
   state_machine_.OnBeginImplFrameDeadline();
   ProcessScheduledActions();
-  FinishImplFrame();
+
+  if (settings_.using_synchronous_renderer_compositor)
+    FinishImplFrameSynchronous();
+  else
+    FinishImplFrame();
+}
+
+void Scheduler::FinishImplFrameSynchronous() {
+  DCHECK(settings_.using_synchronous_renderer_compositor);
+  if (needs_finish_frame_for_synchronous_compositor_) {
+    needs_finish_frame_for_synchronous_compositor_ = false;
+    FinishImplFrame();
+  }
 }
 
 void Scheduler::DrawIfPossible() {

cc/scheduler/scheduler.h

@@ -322,6 +322,8 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
 
   bool stopped_ = false;
 
+  bool needs_finish_frame_for_synchronous_compositor_ = false;
+
   // Keeps track of the begin frame interval from the last BeginFrameArgs to
   // arrive so that |client_| can be informed about changes.
   base::TimeDelta last_frame_interval_;
@@ -369,6 +371,7 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
 
   void BeginImplFrameWithDeadline(const viz::BeginFrameArgs& args);
   void BeginImplFrameSynchronous(const viz::BeginFrameArgs& args);
+  void FinishImplFrameSynchronous();
   void BeginImplFrame(const viz::BeginFrameArgs& args, base::TimeTicks now);
   void FinishImplFrame();
   void SendDidNotProduceFrame(const viz::BeginFrameArgs& args,

cc/scheduler/scheduler_state_machine.h

@@ -204,10 +204,17 @@ class CC_EXPORT SchedulerStateMachine {
   bool begin_frame_source_paused() const { return begin_frame_source_paused_; }
 
   // Indicates that a redraw is required, either due to the impl tree changing
-  // or the screen being damaged and simply needing redisplay.
+  // or the screen being damaged and simply needing redisplay. Note that if the
+  // changes in the impl tree has not been activated yet, then |needs_redraw()|
+  // can return false. For checking any invalidations, check
+  // |did_invalidate_layer_tree_frame_sink()|.
   void SetNeedsRedraw();
   bool needs_redraw() const { return needs_redraw_; }
 
+  bool did_invalidate_layer_tree_frame_sink() const {
+    return did_invalidate_layer_tree_frame_sink_;
+  }
+
   bool OnlyImplSideUpdatesExpected() const;
 
   // Indicates that prepare-tiles is required. This guarantees another

cc/scheduler/scheduler_unittest.cc

@@ -3068,7 +3068,8 @@ TEST_F(SchedulerTest, SynchronousCompositorAnimation) {
   AdvanceFrame();
   EXPECT_ACTIONS("WillBeginImplFrame",
                  "ScheduledActionInvalidateLayerTreeFrameSink");
-  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  // Finish frame is deferred until next draw since there was invalidation.
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
   client_->Reset();
 
   // Android onDraw. This doesn't consume the single begin frame request.
@@ -3089,7 +3090,8 @@ TEST_F(SchedulerTest, SynchronousCompositorAnimation) {
   AdvanceFrame();
   EXPECT_ACTIONS("WillBeginImplFrame",
                  "ScheduledActionInvalidateLayerTreeFrameSink");
-  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
+  // Finish frame is deferred until next draw since there was invalidation.
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
   client_->Reset();
 
   // Android onDraw.
@@ -3140,12 +3142,12 @@ TEST_F(SchedulerTest, InvalidateLayerTreeFrameSinkWhenCannotDraw) {
   // Do not invalidate in next BeginFrame.
   EXPECT_SCOPED(AdvanceFrame());
   EXPECT_ACTIONS("WillBeginImplFrame");
-  client_->Reset();
 
   // Redraw is not cleared.
   EXPECT_TRUE(scheduler_->RedrawPending());
 
   scheduler_->SetCanDraw(true);
+  client_->Reset();
 
   // Do invalidate in next BeginFrame.
   EXPECT_SCOPED(AdvanceFrame());
@@ -3332,15 +3334,8 @@ TEST_F(SchedulerTest, SynchronousCompositorCommitAndVerifyBeginFrameAcks) {
   args = SendNextBeginFrame();
   EXPECT_ACTIONS("WillBeginImplFrame",
                  "ScheduledActionInvalidateLayerTreeFrameSink");
-  EXPECT_FALSE(client_->IsInsideBeginImplFrame());
-
-  // 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.
-  has_damage = false;
-  EXPECT_EQ(viz::BeginFrameAck(args, has_damage),
-            client_->last_begin_frame_ack());
+  // Finish frame is deferred until next draw since there was invalidation.
+  EXPECT_TRUE(client_->IsInsideBeginImplFrame());
   client_->Reset();
 
   // Android onDraw.