Revert "Do not throttle BeginMainFrame on SubmitCompositorFrameAck"

This reverts commit 3e2200fa.

Reason for revert: Caused some regressions. piman@ and I have been rethinking how throttling works. Might make sense to hold this off until we have a solid plan.

Original change's description:
> Do not throttle BeginMainFrame on SubmitCompositorFrameAck
>
> The original motivation for this code was to workaround early swap ack
> on freon, but since freon isn't a thing anymore and we have a display
> compositor that actually performs the swap, it should be ok to get rid
> of it. This should improve throughput similar to main frame before
> activation.
>
> R=​brianderson
> BUG=311213
>
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I5108c74a57ba201d25999989de8da4c7f81a5176
> Reviewed-on: https://chromium-review.googlesource.com/1031892
> Reviewed-by: Brian Anderson <brianderson@chromium.org>
> Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#554547}

TBR=brianderson@chromium.org,sunnyps@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 311213,838469,838462,838461,838460,838458
Change-Id: I600b48b3b24a213954b563347dc3dc87dd143ad2
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1054987
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557791}

cc/scheduler/scheduler_settings.cc

@@ -18,6 +18,8 @@ std::unique_ptr<base::trace_event::ConvertableToTraceFormat>
 SchedulerSettings::AsValue() const {
   std::unique_ptr<base::trace_event::TracedValue> state(
       new base::trace_event::TracedValue());
+  state->SetBoolean("main_frame_while_submit_frame_throttled_enabled",
+                    main_frame_while_submit_frame_throttled_enabled);
   state->SetBoolean("main_frame_before_activation_enabled",
                     main_frame_before_activation_enabled);
   state->SetBoolean("commit_to_active_tree", commit_to_active_tree);

cc/scheduler/scheduler_settings.h

@@ -26,6 +26,7 @@ class CC_EXPORT SchedulerSettings {
   SchedulerSettings(const SchedulerSettings& other);
   ~SchedulerSettings();
 
+  bool main_frame_while_submit_frame_throttled_enabled = false;
   bool main_frame_before_activation_enabled = false;
   bool commit_to_active_tree = false;
   bool timeout_and_draw_when_animation_checkerboards = true;

cc/scheduler/scheduler_state_machine.cc

@@ -489,6 +489,19 @@ bool SchedulerStateMachine::ShouldSendBeginMainFrame() const {
   if (!HasInitializedLayerTreeFrameSink())
     return false;
 
+  if (!settings_.main_frame_while_submit_frame_throttled_enabled) {
+    // Throttle the BeginMainFrames on CompositorFrameAck unless we just
+    // submitted a frame to potentially improve impl-thread latency over
+    // main-thread throughput.
+    // TODO(brianderson): Remove this restriction to improve throughput or
+    // make it conditional on ImplLatencyTakesPriority.
+    bool just_submitted_in_deadline =
+        begin_impl_frame_state_ == BeginImplFrameState::INSIDE_DEADLINE &&
+        did_submit_in_last_frame_;
+    if (IsDrawThrottled() && !just_submitted_in_deadline)
+      return false;
+  }
+
   if (skip_next_begin_main_frame_to_reduce_latency_)
     return false;
 

cc/scheduler/scheduler_state_machine_unittest.cc

@@ -2029,6 +2029,16 @@ TEST(SchedulerStateMachineTest, TestImplLatencyTakesPriority) {
   // Finish the previous commit and draw it.
   FinishPreviousCommitAndDrawWithoutExitingDeadline(&state);
   EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::Action::NONE);
+
+  // Verify we do not send another BeginMainFrame if was are submit-frame
+  // throttled and did not just submit one.
+  state.SetNeedsBeginMainFrame();
+  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::Action::NONE);
+  state.IssueNextBeginImplFrame();
+  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::Action::NONE);
+  EXPECT_FALSE(state.ShouldTriggerBeginImplFrameDeadlineImmediately());
+  state.OnBeginImplFrameDeadline();
+  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::Action::NONE);
 }
 
 TEST(SchedulerStateMachineTest,

cc/scheduler/scheduler_unittest.cc

@@ -1852,7 +1852,7 @@ void SchedulerTest::ImplFrameNotSkippedAfterLateAck() {
     scheduler_->SetNeedsBeginMainFrame();
     EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
     SendNextBeginFrame();
-    EXPECT_ACTIONS("WillBeginImplFrame", "ScheduledActionSendBeginMainFrame");
+    EXPECT_ACTIONS("WillBeginImplFrame");
     EXPECT_TRUE(client_->IsInsideBeginImplFrame());
     EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
 
@@ -1864,7 +1864,8 @@ void SchedulerTest::ImplFrameNotSkippedAfterLateAck() {
     task_runner().RunTasksWhile(client_->InsideBeginImplFrame(true));
 
     // Verify that we don't skip the actions of the BeginImplFrame
-    EXPECT_ACTIONS("ScheduledActionCommit", "ScheduledActionActivateSyncTree",
+    EXPECT_ACTIONS("ScheduledActionSendBeginMainFrame", "ScheduledActionCommit",
+                   "ScheduledActionActivateSyncTree",
                    "ScheduledActionDrawIfPossible");
   }
 }
@@ -1940,8 +1941,7 @@ TEST_F(SchedulerTest, MainFrameThenImplFrameSkippedAfterLateCommitAndLateAck) {
                  "ScheduledActionSendBeginMainFrame", "ScheduledActionCommit",
                  "ScheduledActionActivateSyncTree");
 
-  // Draw and swap for first commit, start second commit which also misses
-  // deadline.
+  // Draw and swap for first commit, start second commit.
   client_->Reset();
   scheduler_->SetNeedsBeginMainFrame();
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
@@ -1957,31 +1957,39 @@ TEST_F(SchedulerTest, MainFrameThenImplFrameSkippedAfterLateCommitAndLateAck) {
                  "ScheduledActionActivateSyncTree");
 
   // Don't call scheduler_->DidReceiveCompositorFrameAck() until after next
-  // frame to put the impl thread in a high latency mode.
+  // frame
+  // to put the impl thread in a high latency mode.
   client_->Reset();
+  scheduler_->SetNeedsBeginMainFrame();
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
   EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
   task_runner().RunTasksWhile(client_->InsideBeginImplFrame(true));
+
   EXPECT_ACTIONS("WillBeginImplFrame");
+  // Note: BeginMainFrame and swap are skipped here because of
+  // swap ack backpressure, not because of latency recovery.
+  EXPECT_FALSE(client_->HasAction("ScheduledActionSendBeginMainFrame"));
+  EXPECT_FALSE(client_->HasAction("ScheduledActionDrawIfPossible"));
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
 
   // Lower estimates so that the scheduler will attempt latency recovery.
   fake_compositor_timing_history_->SetAllEstimatesTo(kFastDuration);
 
-  // Now that both threads are in a high latency mode, make sure we skip the
-  // BeginMainFrame, then the BeginImplFrame, but not both at the same time.
+  // Now that both threads are in a high latency mode, make sure we
+  // skip the BeginMainFrame, then the BeginImplFrame, but not both
+  // at the same time.
 
   // Verify we skip BeginMainFrame first.
   client_->Reset();
-  scheduler_->SetNeedsBeginMainFrame();
+  // Previous commit request is still outstanding.
+  EXPECT_TRUE(scheduler_->NeedsBeginMainFrame());
   EXPECT_TRUE(scheduler_->IsDrawThrottled());
   SendNextBeginFrame();
   EXPECT_TRUE(scheduler_->MainThreadMissedLastDeadline());
   scheduler_->DidReceiveCompositorFrameAck();
   task_runner().RunTasksWhile(client_->InsideBeginImplFrame(true));
 
-  // Main thread missed last deadline state is reset after main thread's tree is
-  // drawn.
   EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
   EXPECT_ACTIONS("WillBeginImplFrame", "ScheduledActionDrawIfPossible");
 
@@ -2004,22 +2012,27 @@ TEST_F(SchedulerTest, MainFrameThenImplFrameSkippedAfterLateCommitAndLateAck) {
   EXPECT_TRUE(scheduler_->NeedsBeginMainFrame());
   EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
   SendNextBeginFrame();
+  EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
   scheduler_->NotifyBeginMainFrameStarted(now_src()->NowTicks());
   scheduler_->NotifyReadyToCommit();
   scheduler_->NotifyReadyToActivate();
   task_runner().RunTasksWhile(client_->InsideBeginImplFrame(true));
+  EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
   scheduler_->DidReceiveCompositorFrameAck();
+  EXPECT_FALSE(scheduler_->MainThreadMissedLastDeadline());
 
   EXPECT_ACTIONS("WillBeginImplFrame", "ScheduledActionSendBeginMainFrame",
                  "ScheduledActionCommit", "ScheduledActionActivateSyncTree",
                  "ScheduledActionDrawIfPossible");
 }
 
-TEST_F(SchedulerTest,
-       CommitMakesProgressWhileSwapTrottledAndActiveTreeNeedsFirstDraw) {
-  // Historical note:
+TEST_F(
+    SchedulerTest,
+    Deadlock_CommitMakesProgressWhileSwapTrottledAndActiveTreeNeedsFirstDraw) {
   // NPAPI plugins on Windows block the Browser UI thread on the Renderer main
   // thread. This prevents the scheduler from receiving any pending swap acks.
+
+  scheduler_settings_.main_frame_while_submit_frame_throttled_enabled = true;
   SetUpScheduler(EXTERNAL_BFS);
 
   // Disables automatic swap acks so this test can force swap ack throttling
@@ -2079,6 +2092,7 @@ TEST_F(
 
   // Since we are simulating a long commit, set up a client with draw duration
   // estimates that prevent skipping main frames to get to low latency mode.
+  scheduler_settings_.main_frame_while_submit_frame_throttled_enabled = true;
   scheduler_settings_.main_frame_before_activation_enabled = true;
   SetUpScheduler(EXTERNAL_BFS);
 
@@ -2230,7 +2244,7 @@ void SchedulerTest::BeginFramesNotFromClient_IsDrawThrottled(
   scheduler_->SetNeedsBeginMainFrame();
   scheduler_->SetNeedsRedraw();
   EXPECT_SCOPED(AdvanceFrame());  // Run posted BeginFrame.
-  EXPECT_ACTIONS("WillBeginImplFrame", "ScheduledActionSendBeginMainFrame");
+  EXPECT_ACTIONS("WillBeginImplFrame");
   EXPECT_TRUE(client_->IsInsideBeginImplFrame());
   client_->Reset();
 
@@ -2255,7 +2269,7 @@ void SchedulerTest::BeginFramesNotFromClient_IsDrawThrottled(
 
   // Take us out of a swap throttled state.
   scheduler_->DidReceiveCompositorFrameAck();
-  EXPECT_NO_ACTION();
+  EXPECT_ACTIONS("ScheduledActionSendBeginMainFrame");
   EXPECT_TRUE(client_->IsInsideBeginImplFrame());
   client_->Reset();