headless/viz: Force full frame when surface is absent

This is a refinement of previous fix for issue 1149042, that also takes
care of another problem. The check that forces full frames is now moved
downstream to the CompositorFrameSinkSupport, where we're able to use
a more adequate criteria for whether full frame is needed.

Drive-by: discard pending output requests after an externally-driven
frame is done.

Bug: 1149042, b/173651870
Change-Id: I26cd851315c3d9059403ef96f9a9bb5f21dfaafd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2557734
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835531}

cc/trees/layer_tree_host_unittest.cc

@@ -3745,15 +3745,17 @@ SINGLE_AND_MULTI_THREAD_TEST_F(
 // updates, i.e. abort commits after the paint stage and only request layer
 // tree updates for layout.
 //
-// The tests sends four Begin(Main)Frames in sequence: three animate_only
-// Begin(Main)Frames followed by a normal Begin(Main)Frame. The first three
-// should result in aborted commits, whereas the last one should complete the
+// The tests sends five Begin(Main)Frames in sequence: four animate_only
+// Begin(Main)Frames followed by a normal Begin(Main)Frame. The first frame
+// has animate_only force to false by CompositorFrameSinkSupport, since it
+// needs a complete frame to assure surface activation. Frames 2-4 should
+// result in aborted commits, whereas the last one should complete the
 // previously aborted commits.
 //
-// Between BeginMainFrames 2 and 3, the client also requests a new commit
-// (SetNeedsCommit), but not between BeginMainFrames 1 and 2. In multi-threaded
+// Between BeginMainFrames 3 and 4, the client also requests a new commit
+// (SetNeedsCommit), but not between BeginMainFrames 1-3. In multi-threaded
 // mode, ProxyMain will run the animate pipeline stage only for BeginMainFrames
-// 1 and 3, as no new commit was requested between 1 and 2.
+// 2 and 4, as no new commit was requested between 2 and 3.
 //
 // The test uses the full-pipeline mode to ensure that each BeginFrame also
 // incurs a BeginMainFrame.
@@ -3810,31 +3812,52 @@ class LayerTreeHostTestAnimateOnlyBeginFrames
 
   void DidSendBeginMainFrameOnThread(LayerTreeHostImpl* host_impl) override {
     ++sent_begin_main_frame_count_;
-    EXPECT_EQ(begin_frame_count_, sent_begin_main_frame_count_);
+    EXPECT_GE(begin_frame_count_, sent_begin_main_frame_count_);
+  }
+
+  void WillBeginImplFrameOnThread(LayerTreeHostImpl* host_impl,
+                                  const viz::BeginFrameArgs& args) override {
+    EXPECT_EQ(args.animate_only,
+              (begin_frame_count_ >= 2 && begin_frame_count_ <= 4));
   }
 
   void DidFinishImplFrameOnThread(LayerTreeHostImpl* host_impl) override {
+    EXPECT_LE(sent_begin_main_frame_count_, begin_frame_count_);
     if (begin_frame_count_ < 3) {
-      if (begin_frame_count_ == 2) {
-        // Request another commit before sending the third BeginMainFrame.
-        PostSetNeedsCommitToMainThread();
-      }
-
       // Send another animation_only BeginFrame.
       PostIssueBeginFrame(true);
     } else if (begin_frame_count_ == 3) {
+      MainThreadTaskRunner()->PostTaskAndReply(
+          FROM_HERE,
+          base::BindOnce(&LayerTreeHostTestAnimateOnlyBeginFrames::
+                             SetNeedsCommitOnMainThread,
+                         base::Unretained(this)),
+          base::BindOnce(
+              &LayerTreeHostTestAnimateOnlyBeginFrames::PostIssueBeginFrame,
+              base::Unretained(this), true));
+    } else if (begin_frame_count_ == 4) {
       PostIssueBeginFrame(false);
     }
   }
 
+  void SetNeedsCommitOnMainThread() { layer_tree_host()->SetNeedsCommit(); }
+
   void UpdateLayerTreeHost() override { ++update_layer_tree_host_count_; }
 
-  void DidCommit() override {
+  void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) override {
+    if (begin_frame_count_ == 1)
+      host_impl->NotifyReadyToDraw();
+
     ++commit_count_;
 
+    // First commit is due to first frame having animate_only forced to false
+    // in ordser to create the surface.
+    if (commit_count_ == 1)
+      return;
+
     // Fourth BeginMainFrame should lead to commit.
-    EXPECT_EQ(4, begin_frame_count_);
-    EXPECT_EQ(4, sent_begin_main_frame_count_);
+    EXPECT_EQ(5, begin_frame_count_);
+    EXPECT_EQ(3, sent_begin_main_frame_count_);
 
     EndTest();
   }
@@ -3844,23 +3867,15 @@ class LayerTreeHostTestAnimateOnlyBeginFrames
   }
 
   void AfterTest() override {
-    EXPECT_EQ(1, commit_count_);
-    EXPECT_EQ(4, begin_frame_count_);
-    EXPECT_EQ(4, sent_begin_main_frame_count_);
+    EXPECT_EQ(2, commit_count_);
+    EXPECT_EQ(5, begin_frame_count_);
+    EXPECT_EQ(3, sent_begin_main_frame_count_);
 
-    if (HasImplThread()) {
-      // ProxyMain aborts the second BeginMainFrame before running the animate
-      // pipeline stage, since no further updates were made since the first one.
-      // Thus, we expect not to be called for the second BeginMainFrame.
-      EXPECT_EQ(3, will_begin_main_frame_count_);
-      EXPECT_EQ(3, update_layer_tree_host_count_);
-    } else {
-      EXPECT_EQ(4, will_begin_main_frame_count_);
-      EXPECT_EQ(4, update_layer_tree_host_count_);
-    }
+    EXPECT_EQ(3, will_begin_main_frame_count_);
+    EXPECT_EQ(3, update_layer_tree_host_count_);
 
     // The final commit should not have been aborted.
-    EXPECT_EQ(1, ready_to_commit_count_);
+    EXPECT_EQ(2, ready_to_commit_count_);
   }
 
  protected:

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -687,6 +687,10 @@ void CompositorFrameSinkSupport::OnBeginFrame(const BeginFrameArgs& args) {
     last_begin_frame_args_ = args;
 
     BeginFrameArgs copy_args = args;
+    // Force full frame if surface not yet activated to ensure surface creation.
+    if (!last_activated_surface_id_.is_valid())
+      copy_args.animate_only = false;
+
     copy_args.trace_id = ComputeTraceId();
     TRACE_EVENT_WITH_FLOW1("viz,benchmark", "Graphics.Pipeline",
                            TRACE_ID_GLOBAL(copy_args.trace_id),

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "components/viz/service/frame_sinks/compositor_frame_sink_support.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "base/test/simple_test_tick_clock.h"
@@ -1505,4 +1507,32 @@ TEST_F(CompositorFrameSinkSupportTest, BeginFrameInterval) {
   EXPECT_EQ(sent_frames, 10);
   support->SetNeedsBeginFrame(false);
 }
+
+TEST_F(CompositorFrameSinkSupportTest, ForceFullFrameToActivateSurface) {
+  FakeExternalBeginFrameSource begin_frame_source(0.f, false);
+  testing::NiceMock<MockCompositorFrameSinkClient> mock_client;
+  auto support = std::make_unique<CompositorFrameSinkSupport>(
+      &mock_client, &manager_, kAnotherArbitraryFrameSinkId, /*is_root=*/true);
+  SurfaceId id(kAnotherArbitraryFrameSinkId, local_surface_id_);
+  support->SetBeginFrameSource(&begin_frame_source);
+  support->SetNeedsBeginFrame(true);
+  const base::TimeTicks frame_time;
+  const int64_t sequence_number = 1;
+
+  // ComposterFrameSink hasn't had a surface activate yet.
+  EXPECT_FALSE(support->last_activated_surface_id().is_valid());
+
+  BeginFrameArgs args = CreateBeginFrameArgsForTesting(
+      BEGINFRAME_FROM_HERE, 0, sequence_number, frame_time);
+  EXPECT_FALSE(args.animate_only);
+  BeginFrameArgs args_animate_only = args;
+  args_animate_only.animate_only = true;
+  // Verify |animate_only| is toggled back to false before sending to client.
+  EXPECT_CALL(mock_client,
+              OnBeginFrame(testing::Field(&BeginFrameArgs::animate_only,
+                                          testing::IsFalse()),
+                           _));
+  begin_frame_source.TestOnBeginFrame(args_animate_only);
+}
+
 }  // namespace viz

components/viz/service/frame_sinks/external_begin_frame_source_mojo.cc

@@ -33,12 +33,7 @@ void ExternalBeginFrameSourceMojo::IssueExternalBeginFrame(
   DCHECK(!pending_frame_callback_) << "Got overlapping IssueExternalBeginFrame";
   original_source_id_ = args.frame_id.source_id;
 
-  BeginFrameArgs args_copy(args);
-  // Force full frame till we have first real draw.
-  if (!last_frame_acknowledged_)
-    args_copy.animate_only = false;
-  last_frame_acknowledged_ = false;
-  OnBeginFrame(args_copy);
+  OnBeginFrame(args);
 
   pending_frame_callback_ = std::move(callback);
 
@@ -105,10 +100,13 @@ void ExternalBeginFrameSourceMojo::MaybeProduceFrameCallback() {
 
 void ExternalBeginFrameSourceMojo::OnDisplayDidFinishFrame(
     const BeginFrameAck& ack) {
-  last_frame_acknowledged_ = true;
   if (!pending_frame_callback_)
     return;
   std::move(pending_frame_callback_).Run(ack);
+  // If there are pending copy output requests that have not been fulfilled,
+  // cancel them, as they won't be served till the next frame. This prevents
+  // the client for waiting for them indefinitely.
+  frame_sink_manager_->DiscardPendingCopyOfOutputRequests(this);
 }
 
 void ExternalBeginFrameSourceMojo::OnDisplayDestroyed() {

components/viz/service/frame_sinks/external_begin_frame_source_mojo.h

@@ -82,7 +82,6 @@ class VIZ_SERVICE_EXPORT ExternalBeginFrameSourceMojo
   // IssueExternalBeginFrame. Note this is likely to be different from our
   // source id, but this is what will be reported to FrameSinkObserver methods.
   uint64_t original_source_id_ = BeginFrameArgs::kStartingSourceId;
-  bool last_frame_acknowledged_ = false;
 
   base::flat_set<FrameSinkId> pending_frame_sinks_;
   Display* display_ = nullptr;

headless/test/data/protocol/helpers/virtual-time-controller.js

@@ -104,6 +104,8 @@
             {frameTimeTicks, screenshot: {format: 'png'}}))
         .result.screenshotData;
     // Advance virtual time a bit so that next frame timestamp is greater.
+    if (!screenshotData)
+      return null;
     this.virtualTimeBase_ += 0.01;
     const image = new Image();
     await new Promise(fulfill => {

headless/test/data/protocol/sanity/screenshot-after-metrics-override-expected.txt

@@ -0,0 +1,4 @@
+Tests that screenshot right after viewport resize doesn't hang
+requested url: http://green.com/
+Screenshot size: 1024 x 1024
+rgba @(25,25) : 0,128,0,255

headless/test/data/protocol/sanity/screenshot-after-metrics-override.js

@@ -0,0 +1,45 @@
+// Copyright 2020 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.
+
+(async function(testRunner) {
+  const {page, session, dp} = await testRunner.startWithFrameControl(
+      'Tests that screenshot right after viewport resize doesn\'t hang');
+
+  await dp.Runtime.enable();
+  await dp.Debugger.enable();
+  await dp.HeadlessExperimental.enable();
+
+  const RendererTestHelper =
+      await testRunner.loadScript('../helpers/renderer-test-helper.js');
+  const {httpInterceptor, frameNavigationHelper, virtualTimeController} =
+      await (new RendererTestHelper(testRunner, dp, page)).init();
+
+  httpInterceptor.addResponse(
+      `http://green.com/`,
+      `<style>
+         body { background-color: green; }
+       </style>
+       <body></body>`
+  );
+
+  await new Promise(async fulfill => {
+    await virtualTimeController.grantInitialTime(500, 100, null, fulfill);
+    frameNavigationHelper.navigate('http://green.com/');
+  });
+  await dp.Emulation.setDeviceMetricsOverride({
+      deviceScaleFactor: 1,
+      width: 1024, height: 1024,
+      mobile: false,
+      screenWidth: 1024, screenHeight: 1024,
+      viewport: { x: 0, y: 0, width: 1024, height: 1024, scale: 1 }
+  });
+  const ctx = await virtualTimeController.captureScreenshot();
+  if (ctx) {
+    let rgba = ctx.getImageData(25, 25, 1, 1).data;
+    testRunner.log(`rgba @(25,25) : ${rgba}`);
+  } else {
+    testRunner.log('FAIL: screenshot data missing!');
+  }
+  testRunner.completeTest();
+})

headless/test/headless_compositor_browsertest.cc

@@ -155,5 +155,7 @@ HEADLESS_COMPOSITOR_TEST(RendererCanvas, "sanity/renderer-canvas.js")
 
 HEADLESS_COMPOSITOR_TEST(RendererOpacityAnimation,
                          "sanity/renderer-opacity-animation.js")
+HEADLESS_COMPOSITOR_TEST(ScreenshotAfterMetricsOverride,
+                         "sanity/screenshot-after-metrics-override.js")
 
 }  // namespace headless