Fix capturing of background tabs

CopyOutputRequests must be taken from CompositorFrameSinkSupport before
SurfaceAggregator::copy_request_passes_ is populated. This broke at
http://crrev.com/99f8d9a.

Bug: 823393
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I2ee6130e66be5b0a0943df87cd22b40d4e9217c1
Reviewed-on: https://chromium-review.googlesource.com/969503
Commit-Queue: Saman Sami <samans@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544269}

components/viz/service/display/surface_aggregator.cc

@@ -1046,17 +1046,6 @@ gfx::Rect SurfaceAggregator::PrewalkTree(Surface* surface,
 
   CHECK(debug_weak_this.get());
 
-  for (const auto& surface_id : frame.metadata.referenced_surfaces) {
-    if (!contained_surfaces_.count(surface_id)) {
-      result->undrawn_surfaces.insert(surface_id);
-      Surface* undrawn_surface = manager_->GetSurfaceForId(surface_id);
-      if (undrawn_surface)
-        PrewalkTree(undrawn_surface, false, 0, false /* will_draw */, result);
-    }
-  }
-
-  CHECK(debug_weak_this.get());
-
   if (!damage_rect.IsEmpty()) {
     // The following call can cause one or more copy requests to be added to the
     // Surface. Therefore, no code before this point should have assumed
@@ -1064,9 +1053,24 @@ gfx::Rect SurfaceAggregator::PrewalkTree(Surface* surface,
     surface->NotifyAggregatedDamage(damage_rect);
   }
 
+  // If any CopyOutputRequests were made at FrameSink level, make sure we grab
+  // them too.
+  surface->TakeCopyOutputRequestsFromClient();
+
   if (will_draw)
     surface->OnWillBeDrawn();
 
+  CHECK(debug_weak_this.get());
+
+  for (const auto& surface_id : frame.metadata.referenced_surfaces) {
+    if (!contained_surfaces_.count(surface_id)) {
+      result->undrawn_surfaces.insert(surface_id);
+      Surface* undrawn_surface = manager_->GetSurfaceForId(surface_id);
+      if (undrawn_surface)
+        PrewalkTree(undrawn_surface, false, 0, false /* will_draw */, result);
+    }
+  }
+
   CHECK(debug_weak_this.get());
   for (const auto& render_pass : frame.render_pass_list) {
     if (!render_pass->copy_requests.empty()) {

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -640,7 +640,7 @@ TEST_F(CompositorFrameSinkSupportTest, DuplicateCopyRequest) {
   request->set_source(kArbitrarySourceId1);
 
   support_->RequestCopyOfOutput(std::move(request));
-  GetSurfaceForId(surface_id)->OnWillBeDrawn();
+  GetSurfaceForId(surface_id)->TakeCopyOutputRequestsFromClient();
   EXPECT_FALSE(called1);
 
   bool called2 = false;
@@ -650,7 +650,7 @@ TEST_F(CompositorFrameSinkSupportTest, DuplicateCopyRequest) {
   request->set_source(kArbitrarySourceId2);
 
   support_->RequestCopyOfOutput(std::move(request));
-  GetSurfaceForId(surface_id)->OnWillBeDrawn();
+  GetSurfaceForId(surface_id)->TakeCopyOutputRequestsFromClient();
   // Callbacks have different sources so neither should be called.
   EXPECT_FALSE(called1);
   EXPECT_FALSE(called2);
@@ -662,7 +662,7 @@ TEST_F(CompositorFrameSinkSupportTest, DuplicateCopyRequest) {
   request->set_source(kArbitrarySourceId1);
 
   support_->RequestCopyOfOutput(std::move(request));
-  GetSurfaceForId(surface_id)->OnWillBeDrawn();
+  GetSurfaceForId(surface_id)->TakeCopyOutputRequestsFromClient();
   // Two callbacks are from source1, so the first should be called.
   EXPECT_TRUE(called1);
   EXPECT_FALSE(called2);
@@ -810,10 +810,11 @@ TEST_F(CompositorFrameSinkSupportTest, FrameIndexCarriedOverToNewSurface) {
   EXPECT_EQ(frame_index + 1, surface2->GetActiveFrameIndex());
 }
 
-// If the first surface is the one reported being drawn by SurfaceAggregator,
-// move CopyOutputRequest to that surface.
+// Verifies that CopyOutputRequests made at frame sink level are sent to the
+// surface that takes them first. In this test this surface is not the last
+// activated surface.
 TEST_F(CompositorFrameSinkSupportTest,
-       FirstSurfaceIsDrawnAndReceivesCopyRequest) {
+       OldSurfaceTakesCopyOutputRequestsFromClient) {
   LocalSurfaceId local_surface_id1(1, kArbitraryToken);
   LocalSurfaceId local_surface_id2(2, kArbitraryToken);
   SurfaceId id1(support_->frame_sink_id(), local_surface_id1);
@@ -837,9 +838,15 @@ TEST_F(CompositorFrameSinkSupportTest,
   EXPECT_TRUE(GetSurfaceForId(id1)->HasCopyOutputRequests());
   EXPECT_TRUE(GetSurfaceForId(id2)->HasCopyOutputRequests());
 
-  // Notify that the first surface will be drawn. Now only the first surface
-  // should report having CopyOutputRequests.
-  GetSurfaceForId(id1)->OnWillBeDrawn();
+  // First surface takes CopyOutputRequests from its client. Now only the first
+  // surface should report having CopyOutputRequests.
+  GetSurfaceForId(id1)->TakeCopyOutputRequestsFromClient();
+  EXPECT_TRUE(GetSurfaceForId(id1)->HasCopyOutputRequests());
+  EXPECT_FALSE(GetSurfaceForId(id2)->HasCopyOutputRequests());
+
+  // Calling TakeCopyOutputRequestsFromClient() on the second surface should
+  // have no effect.
+  GetSurfaceForId(id2)->TakeCopyOutputRequestsFromClient();
   EXPECT_TRUE(GetSurfaceForId(id1)->HasCopyOutputRequests());
   EXPECT_FALSE(GetSurfaceForId(id2)->HasCopyOutputRequests());
 
@@ -852,10 +859,11 @@ TEST_F(CompositorFrameSinkSupportTest,
   EXPECT_FALSE(requests_map.empty());
 }
 
-// If the second surface is the one reported being drawn by SurfaceAggregator,
-// move CopyOutputRequest to that surface.
+// Verifies that CopyOutputRequests made at frame sink level are sent to the
+// surface that takes them first. In this test this surface is the last
+// activated surface.
 TEST_F(CompositorFrameSinkSupportTest,
-       SecondSurfaceIsDrawnAndReceivesCopyRequest) {
+       LastSurfaceTakesCopyOutputRequestsFromClient) {
   LocalSurfaceId local_surface_id1(1, kArbitraryToken);
   LocalSurfaceId local_surface_id2(2, kArbitraryToken);
   SurfaceId id1(support_->frame_sink_id(), local_surface_id1);
@@ -880,110 +888,25 @@ TEST_F(CompositorFrameSinkSupportTest,
   EXPECT_TRUE(GetSurfaceForId(id1)->HasCopyOutputRequests());
   EXPECT_TRUE(GetSurfaceForId(id2)->HasCopyOutputRequests());
 
-  // Notify that the second surface will be drawn. Now only the second surface
-  // should report having CopyOutputRequests.
-  GetSurfaceForId(id2)->OnWillBeDrawn();
+  // Second surface takes CopyOutputRequests from its client. Now only the
+  // second surface should report having CopyOutputRequests.
+  GetSurfaceForId(id2)->TakeCopyOutputRequestsFromClient();
   EXPECT_FALSE(GetSurfaceForId(id1)->HasCopyOutputRequests());
   EXPECT_TRUE(GetSurfaceForId(id2)->HasCopyOutputRequests());
 
-  // Also try TakeCopyOutputRequests, to see if its output is consistent with
-  // HasCopyOutputRequests.
-  Surface::CopyRequestsMap requests_map;
-  GetSurfaceForId(id1)->TakeCopyOutputRequests(&requests_map);
-  EXPECT_TRUE(requests_map.empty());
-  GetSurfaceForId(id2)->TakeCopyOutputRequests(&requests_map);
-  EXPECT_FALSE(requests_map.empty());
-}
-
-// Move CopyOutputRequests to whatever surface wants it first (in this test, the
-// first surface).
-TEST_F(CompositorFrameSinkSupportTest, FirstSurfaceTakesCopyRequest) {
-  LocalSurfaceId local_surface_id1(1, kArbitraryToken);
-  LocalSurfaceId local_surface_id2(2, kArbitraryToken);
-  SurfaceId id1(support_->frame_sink_id(), local_surface_id1);
-  SurfaceId id2(support_->frame_sink_id(), local_surface_id2);
-
-  // Create the first surface.
-  support_->SubmitCompositorFrame(local_surface_id1,
-                                  MakeDefaultCompositorFrame());
-
-  // Create the second surface.
-  support_->SubmitCompositorFrame(local_surface_id2,
-                                  MakeDefaultCompositorFrame());
-
-  // Send a CopyOutputRequest.
-  auto request = std::make_unique<CopyOutputRequest>(
-      CopyOutputRequest::ResultFormat::RGBA_BITMAP,
-      base::BindOnce(StubResultCallback));
-  support_->RequestCopyOfOutput(std::move(request));
-
-  // Both surfaces should report that they have a CopyOutputRequest.
-  EXPECT_TRUE(GetSurfaceForId(id1)->HasCopyOutputRequests());
-  EXPECT_TRUE(GetSurfaceForId(id2)->HasCopyOutputRequests());
-
-  // Neither surface will be reported drawn. We call TakeCopyOutputRequests()
-  // on |id1| first so it takes it.
-  Surface::CopyRequestsMap requests_map;
-  GetSurfaceForId(id1)->TakeCopyOutputRequests(&requests_map);
-  EXPECT_FALSE(requests_map.empty());
-
-  // Neither surface should report having CopyOutputRequests anymore.
+  // Calling TakeCopyOutputRequestsFromClient() on the first surface should have
+  // no effect.
+  GetSurfaceForId(id1)->TakeCopyOutputRequestsFromClient();
   EXPECT_FALSE(GetSurfaceForId(id1)->HasCopyOutputRequests());
-  EXPECT_FALSE(GetSurfaceForId(id2)->HasCopyOutputRequests());
+  EXPECT_TRUE(GetSurfaceForId(id2)->HasCopyOutputRequests());
 
   // Also try TakeCopyOutputRequests, to see if its output is consistent with
   // HasCopyOutputRequests.
-  requests_map.clear();
-  GetSurfaceForId(id2)->TakeCopyOutputRequests(&requests_map);
-  EXPECT_TRUE(requests_map.empty());
+  Surface::CopyRequestsMap requests_map;
   GetSurfaceForId(id1)->TakeCopyOutputRequests(&requests_map);
   EXPECT_TRUE(requests_map.empty());
-}
-
-// Move CopyOutputRequests to whatever surface wants it first (in this test, the
-// second surface).
-TEST_F(CompositorFrameSinkSupportTest, SecondSurfaceTakesCopyRequest) {
-  LocalSurfaceId local_surface_id1(1, kArbitraryToken);
-  LocalSurfaceId local_surface_id2(2, kArbitraryToken);
-  SurfaceId id1(support_->frame_sink_id(), local_surface_id1);
-  SurfaceId id2(support_->frame_sink_id(), local_surface_id2);
-
-  // Create the first surface.
-  support_->SubmitCompositorFrame(local_surface_id1,
-                                  MakeDefaultCompositorFrame());
-
-  // Send a CopyOutputRequest. Note that the second surface doesn't even exist
-  // yet.
-  auto request = std::make_unique<CopyOutputRequest>(
-      CopyOutputRequest::ResultFormat::RGBA_BITMAP,
-      base::BindOnce(StubResultCallback));
-  support_->RequestCopyOfOutput(std::move(request));
-
-  // Create the second surface.
-  support_->SubmitCompositorFrame(local_surface_id2,
-                                  MakeDefaultCompositorFrame());
-
-  // Both surfaces should report that they have a CopyOutputRequest.
-  EXPECT_TRUE(GetSurfaceForId(id1)->HasCopyOutputRequests());
-  EXPECT_TRUE(GetSurfaceForId(id2)->HasCopyOutputRequests());
-
-  // Neither surface will be reported drawn. We call TakeCopyOutputRequests()
-  // on |id2| first so it takes it.
-  Surface::CopyRequestsMap requests_map;
   GetSurfaceForId(id2)->TakeCopyOutputRequests(&requests_map);
   EXPECT_FALSE(requests_map.empty());
-
-  // Neither surface should report having CopyOutputRequests anymore.
-  EXPECT_FALSE(GetSurfaceForId(id1)->HasCopyOutputRequests());
-  EXPECT_FALSE(GetSurfaceForId(id2)->HasCopyOutputRequests());
-
-  // Also try TakeCopyOutputRequests, to see if its output is consistent with
-  // HasCopyOutputRequests.
-  requests_map.clear();
-  GetSurfaceForId(id2)->TakeCopyOutputRequests(&requests_map);
-  EXPECT_TRUE(requests_map.empty());
-  GetSurfaceForId(id1)->TakeCopyOutputRequests(&requests_map);
-  EXPECT_TRUE(requests_map.empty());
 }
 
 }  // namespace

components/viz/service/surfaces/surface.cc

@@ -423,7 +423,8 @@ void Surface::TakeCopyOutputRequests(Surface::CopyRequestsMap* copy_requests) {
   if (!active_frame_data_)
     return;
 
-  TakeCopyOutputRequestsFromClient();
+  // TakeCopyOutputRequestsFromClient() has to be called before this method.
+  DCHECK(!surface_client_ || !surface_client_->HasCopyOutputRequests());
 
   for (const auto& render_pass : active_frame_data_->frame.render_pass_list) {
     for (auto& request : render_pass->copy_requests) {
@@ -434,6 +435,15 @@ void Surface::TakeCopyOutputRequests(Surface::CopyRequestsMap* copy_requests) {
   }
 }
 
+void Surface::TakeCopyOutputRequestsFromClient() {
+  if (!surface_client_)
+    return;
+  for (std::unique_ptr<CopyOutputRequest>& request :
+       surface_client_->TakeCopyOutputRequests()) {
+    RequestCopyOfOutput(std::move(request));
+  }
+}
+
 bool Surface::HasCopyOutputRequests() {
   if (!active_frame_data_)
     return false;
@@ -543,17 +553,7 @@ void Surface::TakeLatencyInfoFromFrame(
 }
 
 void Surface::OnWillBeDrawn() {
-  TakeCopyOutputRequestsFromClient();
   surface_manager_->SurfaceWillBeDrawn(this);
 }
 
-void Surface::TakeCopyOutputRequestsFromClient() {
-  if (!surface_client_)
-    return;
-  for (std::unique_ptr<CopyOutputRequest>& request :
-       surface_client_->TakeCopyOutputRequests()) {
-    RequestCopyOfOutput(std::move(request));
-  }
-}
-
 }  // namespace viz

components/viz/service/surfaces/surface.h

@@ -150,6 +150,10 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
   // ids.
   void TakeCopyOutputRequests(CopyRequestsMap* copy_requests);
 
+  // Takes CopyOutputRequests made at the client level and adds them to this
+  // Surface.
+  void TakeCopyOutputRequestsFromClient();
+
   // Returns whether there is a CopyOutputRequest inside the active frame or at
   // the client level.
   bool HasCopyOutputRequests();
@@ -260,8 +264,6 @@ class VIZ_SERVICE_EXPORT Surface final : public SurfaceDeadlineClient {
 
   void RequestCopyOfOutput(std::unique_ptr<CopyOutputRequest> copy_request);
 
-  void TakeCopyOutputRequestsFromClient();
-
   const SurfaceInfo surface_info_;
   SurfaceId previous_frame_surface_id_;
   SurfaceManager* const surface_manager_;

components/viz/service/surfaces/surface_unittest.cc

@@ -94,7 +94,7 @@ TEST(SurfaceTest, CopyRequestLifetime) {
   support->RequestCopyOfOutput(std::make_unique<CopyOutputRequest>(
       CopyOutputRequest::ResultFormat::RGBA_BITMAP,
       base::BindOnce(&TestCopyResultCallback, &copy_called)));
-  surface->OnWillBeDrawn();
+  surface->TakeCopyOutputRequestsFromClient();
   EXPECT_TRUE(surface_manager->GetSurfaceForId(surface_id));
   EXPECT_FALSE(copy_called);