cc: Disable activate needs draw in webview

cc does not have control over when to produce a frame for webview /
synchronous compositor as it does over chrome. There is a lot of down
side to waiting for draw, when the pathological case for happens
frequently causing both poor performance on the main thread and higher
latency.

Synchronous compositor also had a fallback tick that runs "fake" draws
after invalidate to ensure the compositing pipeline is never stalled.
This is no longer necessary with this change, so remove fallback tick
and some unused code in swap messages.

Bug: 1025695
Change-Id: I240f01662ccdf6f9c35912c421ea1ef0e40b1cc2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1986934Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733492}

cc/scheduler/scheduler_state_machine.cc

@@ -412,9 +412,13 @@ bool SchedulerStateMachine::ShouldActivateSyncTree() const {
 
   // We should not activate a second tree before drawing the first one.
   // Even if we need to force activation of the pending tree, we should abort
-  // drawing the active tree first.
-  if (active_tree_needs_first_draw_)
+  // drawing the active tree first. Relax this requirement for synchronous
+  // compositor where scheduler does not control draw, and blocking commit
+  // may lead to bad scheduling.
+  if (!settings_.using_synchronous_renderer_compositor &&
+      active_tree_needs_first_draw_) {
     return false;
+  }
 
   // Delay pending tree activation until paint worklets have completed painting
   // the pending tree. This must occur before the |ShouldAbortCurrentFrame|

cc/scheduler/scheduler_unittest.cc

@@ -3401,6 +3401,55 @@ TEST_F(SchedulerTest, SynchronousCompositorPrepareTilesOnDraw) {
   client_->Reset();
 }
 
+// Synchronous compositor does not require the active tree to be drawn at least
+// once before the next activation. This test verifies two commit-activate
+// cycles without draw work correctly.
+TEST_F(SchedulerTest, SynchronousCompositorAllowsActivateBeforeDraw) {
+  scheduler_settings_.using_synchronous_renderer_compositor = true;
+
+  std::unique_ptr<FakeSchedulerClient> client =
+      base::WrapUnique(new SchedulerClientSetNeedsPrepareTilesOnDraw);
+  SetUpScheduler(EXTERNAL_BFS, std::move(client));
+
+  scheduler_->SetNeedsRedraw();
+  EXPECT_ACTIONS("AddObserver(this)");
+  client_->Reset();
+
+  // Next vsync.
+  scheduler_->SetNeedsBeginMainFrame();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_ACTIONS("WillBeginImplFrame", "ScheduledActionSendBeginMainFrame",
+                 "ScheduledActionInvalidateLayerTreeFrameSink");
+  client_->Reset();
+
+  // Commit and activate.
+  scheduler_->NotifyBeginMainFrameStarted(task_runner_->NowTicks());
+  scheduler_->NotifyReadyToCommit(nullptr);
+  EXPECT_ACTIONS("ScheduledActionCommit");
+  client_->Reset();
+  scheduler_->NotifyReadyToActivate();
+  EXPECT_ACTIONS("ScheduledActionActivateSyncTree");
+  client_->Reset();
+
+  // No Draw.
+
+  // Next vsync.
+  scheduler_->SetNeedsBeginMainFrame();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_ACTIONS("WillBeginImplFrame", "ScheduledActionSendBeginMainFrame",
+                 "ScheduledActionInvalidateLayerTreeFrameSink");
+  client_->Reset();
+
+  // Commit and activate.
+  scheduler_->NotifyBeginMainFrameStarted(task_runner_->NowTicks());
+  scheduler_->NotifyReadyToCommit(nullptr);
+  EXPECT_ACTIONS("ScheduledActionCommit");
+  client_->Reset();
+  scheduler_->NotifyReadyToActivate();
+  EXPECT_ACTIONS("ScheduledActionActivateSyncTree");
+  client_->Reset();
+}
+
 TEST_F(SchedulerTest, SetNeedsRedrawFromWillBeginImplFrame) {
   client_ = std::make_unique<FakeSchedulerClient>();
   CreateScheduler(EXTERNAL_BFS);

cc/trees/layer_tree_host_unittest.cc

@@ -11,6 +11,7 @@
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
@@ -4062,6 +4063,62 @@ class LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor
 MULTI_THREAD_TEST_F(
     LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor);
 
+class LayerTreeHostTestSynchronousCompositorActivateWithoutDraw
+    : public LayerTreeHostTest {
+ protected:
+  void InitializeSettings(LayerTreeSettings* settings) override {
+    settings->using_synchronous_renderer_compositor = true;
+  }
+
+  std::unique_ptr<TestLayerTreeFrameSink> CreateLayerTreeFrameSink(
+      const viz::RendererSettings& renderer_settings,
+      double refresh_rate,
+      scoped_refptr<viz::ContextProvider> compositor_context_provider,
+      scoped_refptr<viz::RasterContextProvider> worker_context_provider)
+      override {
+    // Make |invalidate_callback| do nothing so there is no draw.
+    auto frame_sink = std::make_unique<OnDrawLayerTreeFrameSink>(
+        compositor_context_provider, std::move(worker_context_provider),
+        gpu_memory_buffer_manager(), renderer_settings, ImplThreadTaskRunner(),
+        /*synchronous_composite=*/false, refresh_rate,
+        /*invalidate_callback=*/base::DoNothing());
+    return std::move(frame_sink);
+  }
+
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
+
+  void DidCommit() override { commit_count_++; }
+
+  void DidActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    activate_count_++;
+    if (activate_count_ == 1) {
+      PostSetNeedsCommitToMainThread();
+    } else if (activate_count_ == 2) {
+      EndTest();
+    } else {
+      NOTREACHED();
+    }
+  }
+
+  void DrawLayersOnThread(LayerTreeHostImpl* host_impl) override {
+    // This test specifically tests that two commit-activate cycles without
+    // draw in between them.
+    ADD_FAILURE();
+  }
+
+  void AfterTest() override {
+    // There should be two commits and activations without any draw.
+    EXPECT_EQ(commit_count_, 2);
+    EXPECT_EQ(activate_count_, 2);
+  }
+
+ private:
+  int commit_count_ = 0;
+  int activate_count_ = 0;
+};
+
+MULTI_THREAD_TEST_F(LayerTreeHostTestSynchronousCompositorActivateWithoutDraw);
+
 class LayerTreeHostTestUninvertibleTransformDoesNotBlockActivation
     : public LayerTreeHostTest {
  protected:

content/renderer/android/synchronous_layer_tree_frame_sink.cc

@@ -48,7 +48,6 @@ namespace content {
 
 namespace {
 
-const int64_t kFallbackTickTimeoutInMilliseconds = 100;
 const viz::FrameSinkId kRootFrameSinkId(1, 1);
 const viz::FrameSinkId kChildFrameSinkId(1, 2);
 
@@ -276,7 +275,6 @@ void SynchronousLayerTreeFrameSink::DetachFromClient() {
   compositor_frame_sink_.reset();
 
   cc::LayerTreeFrameSink::DetachFromClient();
-  CancelFallbackTick();
 }
 
 void SynchronousLayerTreeFrameSink::SetLocalSurfaceId(
@@ -292,12 +290,6 @@ void SynchronousLayerTreeFrameSink::SubmitCompositorFrame(
   DCHECK(CalledOnValidThread());
   DCHECK(sync_client_);
 
-  if (fallback_tick_running_) {
-    DCHECK(frame.resource_list.empty());
-    did_submit_frame_ = true;
-    return;
-  }
-
   base::Optional<viz::CompositorFrame> submit_frame;
   gfx::Size child_size = in_software_draw_
                              ? sw_viewport_for_current_draw_.size()
@@ -444,39 +436,10 @@ void SynchronousLayerTreeFrameSink::DidDeleteSharedBitmap(
   NOTREACHED();
 }
 
-void SynchronousLayerTreeFrameSink::CancelFallbackTick() {
-  fallback_tick_.Cancel();
-  fallback_tick_pending_ = false;
-}
-
-void SynchronousLayerTreeFrameSink::FallbackTickFired() {
-  DCHECK(CalledOnValidThread());
-  TRACE_EVENT0("renderer", "SynchronousLayerTreeFrameSink::FallbackTickFired");
-  base::AutoReset<bool> in_fallback_tick(&fallback_tick_running_, true);
-  frame_swap_message_queue_->NotifyFramesAreDiscarded(true);
-  SkBitmap bitmap;
-  bitmap.allocN32Pixels(1, 1);
-  bitmap.eraseColor(0);
-  SkCanvas canvas(bitmap);
-  fallback_tick_pending_ = false;
-  DemandDrawSw(&canvas);
-  frame_swap_message_queue_->NotifyFramesAreDiscarded(false);
-}
-
 void SynchronousLayerTreeFrameSink::Invalidate(bool needs_draw) {
   DCHECK(CalledOnValidThread());
   if (sync_client_)
     sync_client_->Invalidate(needs_draw);
-
-  if (!fallback_tick_pending_) {
-    fallback_tick_.Reset(
-        base::BindOnce(&SynchronousLayerTreeFrameSink::FallbackTickFired,
-                       base::Unretained(this)));
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, fallback_tick_.callback(),
-        base::TimeDelta::FromMilliseconds(kFallbackTickTimeoutInMilliseconds));
-    fallback_tick_pending_ = true;
-  }
 }
 
 void SynchronousLayerTreeFrameSink::DemandDrawHw(
@@ -486,7 +449,6 @@ void SynchronousLayerTreeFrameSink::DemandDrawHw(
   DCHECK(CalledOnValidThread());
   DCHECK(HasClient());
   DCHECK(context_provider_.get());
-  CancelFallbackTick();
 
   client_->SetExternalTilePriorityConstraints(viewport_rect_for_tile_priority,
                                               transform_for_tile_priority);
@@ -497,7 +459,6 @@ void SynchronousLayerTreeFrameSink::DemandDrawSw(SkCanvas* canvas) {
   DCHECK(CalledOnValidThread());
   DCHECK(canvas);
   DCHECK(!current_sw_canvas_);
-  CancelFallbackTick();
 
   base::AutoReset<SkCanvas*> canvas_resetter(&current_sw_canvas_, canvas);
 
@@ -516,7 +477,6 @@ void SynchronousLayerTreeFrameSink::DemandDrawSw(SkCanvas* canvas) {
 }
 
 void SynchronousLayerTreeFrameSink::WillSkipDraw() {
-  CancelFallbackTick();
   client_->OnDraw(gfx::Transform(), gfx::Rect(), in_software_draw_,
                   true /*skip_draw*/);
 }

content/renderer/android/synchronous_layer_tree_frame_sink.h

@@ -150,9 +150,6 @@ class SynchronousLayerTreeFrameSink
   void DeliverMessages();
   bool CalledOnValidThread() const;
 
-  void CancelFallbackTick();
-  void FallbackTickFired();
-
   // Sends DidNotProduceFrame to CompositorFrameSink for the previous BeginFrame
   // if we didn't send DidNotProduceFrame/SubmitCompositorFrame already.
   void SendAckToLastBeginFrameIfNeeded();
@@ -178,10 +175,6 @@ class SynchronousLayerTreeFrameSink
   bool did_submit_frame_ = false;
   scoped_refptr<FrameSwapMessageQueue> frame_swap_message_queue_;
 
-  base::CancelableOnceClosure fallback_tick_;
-  bool fallback_tick_pending_ = false;
-  bool fallback_tick_running_ = false;
-
   mojo::PendingRemote<viz::mojom::CompositorFrameSink>
       unbound_compositor_frame_sink_;
   mojo::PendingReceiver<viz::mojom::CompositorFrameSinkClient> unbound_client_;

content/renderer/frame_swap_message_queue.cc

@@ -154,15 +154,4 @@ void FrameSwapMessageQueue::TransferMessages(
   source->clear();
 }
 
-void FrameSwapMessageQueue::NotifyFramesAreDiscarded(
-    bool frames_are_discarded) {
-  DCHECK_CALLED_ON_VALID_THREAD(impl_thread_checker_);
-  frames_are_discarded_ = frames_are_discarded;
-}
-
-bool FrameSwapMessageQueue::AreFramesDiscarded() {
-  DCHECK_CALLED_ON_VALID_THREAD(impl_thread_checker_);
-  return frames_are_discarded_;
-}
-
 }  // namespace content

content/renderer/frame_swap_message_queue.h

@@ -97,9 +97,6 @@ class CONTENT_EXPORT FrameSwapMessageQueue
 
   int32_t routing_id() const { return routing_id_; }
 
-  void NotifyFramesAreDiscarded(bool frames_are_discarded);
-  bool AreFramesDiscarded();
-
  private:
   friend class base::RefCountedThreadSafe<FrameSwapMessageQueue>;
 
@@ -109,7 +106,6 @@ class CONTENT_EXPORT FrameSwapMessageQueue
   std::unique_ptr<FrameSwapMessageSubQueue> visual_state_queue_;
   std::vector<std::unique_ptr<IPC::Message>> next_drain_messages_;
   int32_t routing_id_ = 0;
-  bool frames_are_discarded_ = false;
   THREAD_CHECKER(impl_thread_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(FrameSwapMessageQueue);

content/renderer/queue_message_swap_promise.cc

@@ -38,20 +38,17 @@ void QueueMessageSwapPromise::DidActivate() {
 void QueueMessageSwapPromise::WillSwap(viz::CompositorFrameMetadata* metadata) {
   message_queue_->DidSwap(source_frame_number_);
 
-  if (!message_queue_->AreFramesDiscarded()) {
-    std::unique_ptr<FrameSwapMessageQueue::SendMessageScope>
-        send_message_scope = message_queue_->AcquireSendMessageScope();
-    std::vector<std::unique_ptr<IPC::Message>> messages;
-    message_queue_->DrainMessages(&messages);
+  std::unique_ptr<FrameSwapMessageQueue::SendMessageScope> send_message_scope =
+      message_queue_->AcquireSendMessageScope();
+  std::vector<std::unique_ptr<IPC::Message>> messages;
+  message_queue_->DrainMessages(&messages);
 
-    std::vector<IPC::Message> messages_to_send;
-    FrameSwapMessageQueue::TransferMessages(&messages, &messages_to_send);
-    if (!messages_to_send.empty()) {
-      metadata->send_frame_token_to_embedder = true;
-      message_sender_->Send(new WidgetHostMsg_FrameSwapMessages(
-          message_queue_->routing_id(), metadata->frame_token,
-          messages_to_send));
-    }
+  std::vector<IPC::Message> messages_to_send;
+  FrameSwapMessageQueue::TransferMessages(&messages, &messages_to_send);
+  if (!messages_to_send.empty()) {
+    metadata->send_frame_token_to_embedder = true;
+    message_sender_->Send(new WidgetHostMsg_FrameSwapMessages(
+        message_queue_->routing_id(), metadata->frame_token, messages_to_send));
   }
 }