Early Ack Pending Queued surfaces

Ack a pending surface early the client's scheduler to schedule frames
for future surface changes This case happens when screen rotation takes
place while there is an active frame when this takes place it has a
unique signature of 2 consecutive submit frame requests with
clients_needs_begin_frame_ is set (high) and 1st frame is not a new
surface and 2nd frame is a new surface.

Bug: 1060058
Change-Id: I7020d10aade334e7fda20749217747cea2a2c8b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2190049
Commit-Queue: Nazih Almalki <nalmalki@google.com>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772486}

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -183,7 +183,10 @@ void CompositorFrameSinkSupport::OnFrameTokenChanged(uint32_t frame_token) {
 }
 
 void CompositorFrameSinkSupport::OnSurfaceProcessed(Surface* surface) {
-  DidReceiveCompositorFrameAck();
+  if (!pending_frame_acked_early_)
+    DidReceiveCompositorFrameAck();
+  else
+    pending_frame_acked_early_ = false;
 }
 
 void CompositorFrameSinkSupport::OnSurfaceAggregatedDamage(
@@ -538,12 +541,10 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
                          "Event.Pipeline", TRACE_ID_GLOBAL(trace_id),
                          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
                          "step", "ReceiveHitTestData");
-
   // QueueFrame can fail in unit tests, so SubmitHitTestRegionList has to be
   // called before that.
   frame_sink_manager()->SubmitHitTestRegionList(
       last_created_surface_id_, frame_index, std::move(hit_test_region_list));
-
   Surface::QueueFrameResult result = current_surface->QueueFrame(
       std::move(frame), frame_index, std::move(frame_rejected_callback));
   switch (result) {
@@ -555,6 +556,32 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
       // Make sure we periodically check if the frame should activate.
       pending_surfaces_.insert(current_surface);
       UpdateNeedsBeginFramesInternal();
+      // Ack a pending surface early the client's scheduler to schedule frames
+      // for future surface changes This case happens when screen rotation takes
+      // place while there is an active frame when this takes place it has a
+      // unique signature of 2 consecutive submit frame requests with
+      // clients_needs_begin_frame_ is set (high) and 1st frame is not a new
+      // surface and 2nd frame is a new surface. For only this sequence send an
+      // early ack to the client by using OnSurfaceProcessed. Setting the
+      // pending_frame_acked_early_ to denote that an early ACK took place. This
+      // prevents sending a second ack for the frame when it is activated. To
+      // ensure only this sequence is early acked, the
+      // last_pending_frame_was_new_surface_ flag is used to ensure the sequence
+      // is correct using current_surface == prev_surface => NOT new_surface ID,
+      // to limit the when the early ack is sent.
+      if (client_needs_begin_frame_) {
+        if ((current_surface == prev_surface) ||
+            last_pending_frame_was_new_surface_) {
+          OnSurfaceProcessed(current_surface);
+          pending_frame_acked_early_ = true;
+          last_pending_frame_was_new_surface_ =
+              !last_pending_frame_was_new_surface_;
+        } else {
+          if (current_surface != prev_surface) {
+            last_pending_frame_was_new_surface_ = false;
+          }
+        }
+      }
       break;
     case Surface::QueueFrameResult::ACCEPTED_ACTIVE:
       // Nothing to do here.
@@ -567,7 +594,7 @@ SubmitResult CompositorFrameSinkSupport::MaybeSubmitCompositorFrame(
   }
 
   return SubmitResult::ACCEPTED;
-}
+}  // namespace viz
 
 SurfaceReference CompositorFrameSinkSupport::MakeTopLevelRootReference(
     const SurfaceId& surface_id) {

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -322,6 +322,8 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
       compositor_frame_callback_;
   bool callback_received_begin_frame_ = true;
   bool callback_received_receive_ack_ = true;
+  bool last_pending_frame_was_new_surface_ = false;
+  bool pending_frame_acked_early_ = false;
   uint32_t trace_sequence_ = 0;
 
   BeginFrameTracker begin_frame_tracker_;

components/viz/service/frame_sinks/compositor_frame_sink_support_unittest.cc

@@ -28,12 +28,12 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/khronos/GLES2/gl2.h"
 
-using testing::UnorderedElementsAre;
-using testing::IsEmpty;
-using testing::SizeIs;
-using testing::Invoke;
 using testing::_;
 using testing::Eq;
+using testing::Invoke;
+using testing::IsEmpty;
+using testing::SizeIs;
+using testing::UnorderedElementsAre;
 
 namespace viz {
 namespace {
@@ -111,6 +111,7 @@ class CompositorFrameSinkSupportTest : public testing::Test {
     manager_.InvalidateFrameSinkId(kArbitraryFrameSinkId);
     manager_.surface_manager()->RemoveObserver(&surface_observer_);
   }
+  bool GotEarlyAck() const { return support_->pending_frame_acked_early_; }
 
   void AddResourcesToFrame(CompositorFrame* frame,
                            ResourceId* resource_ids,
@@ -1438,4 +1439,69 @@ TEST_F(CompositorFrameSinkSupportTest, ThrottleUnresponsiveClient) {
   support->SetNeedsBeginFrame(false);
 }
 
+// Check CompositorFrame get an early ack if the surface id needs to have
+// pending child needsBeginFrame = true, and NOT new surface ID, then followed
+// by needsBeginFrame = true, and new Surface ID, other classes should not get
+// an early ACK. crbug.com/1060058)
+TEST_F(CompositorFrameSinkSupportTest, PassesEarlyAcks) {
+  SurfaceId id(support_->frame_sink_id(), local_surface_id_);
+
+  LocalSurfaceId child_local_surface_id(1, kAnotherArbitraryToken);
+  SurfaceId child_id(kAnotherArbitraryFrameSinkId, child_local_surface_id);
+  MockCompositorFrameSinkClient mock_client;
+
+  support_->SetNeedsBeginFrame(true);
+
+  // Submit a frame with size (5,5).
+  auto frame = CompositorFrameBuilder()
+                   .AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
+                   .SetActivationDependencies({child_id})
+                   .Build();
+  auto result = support_->MaybeSubmitCompositorFrame(
+      local_surface_id_, std::move(frame), base::nullopt, 0,
+      mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
+  // should get NOT early ACK, NeedsBeginFrame= true, and new surface ID (need
+  // to have a NOT new Surface ID)
+  EXPECT_FALSE(GotEarlyAck());
+
+  // send a same surface id with new dependencies.
+  LocalSurfaceId child_local_surface_id2(2, kAnotherArbitraryToken);
+  SurfaceId child_id2(kAnotherArbitraryFrameSinkId, child_local_surface_id2);
+  frame = CompositorFrameBuilder()
+              .AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
+              .SetActivationDependencies({child_id2})
+              .Build();
+  result = support_->MaybeSubmitCompositorFrame(
+      local_surface_id_, std::move(frame), base::nullopt, 0,
+      mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
+  EXPECT_EQ(SubmitResult::ACCEPTED, result);
+  // should get early ACK, NeedsBeginFrame= true, and not new surface ID
+  EXPECT_TRUE(GotEarlyAck());
+
+  // send new surface id with dependencies.
+  LocalSurfaceId local_surface_id2(6, kArbitraryToken);
+  LocalSurfaceId child_local_surface_id3(2, kAnotherArbitraryToken);
+  SurfaceId child_id3(kAnotherArbitraryFrameSinkId, child_local_surface_id3);
+
+  frame = CompositorFrameBuilder()
+              .AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
+              .SetActivationDependencies({child_id3})
+              .Build();
+
+  result = support_->MaybeSubmitCompositorFrame(
+      local_surface_id2, std::move(frame), base::nullopt, 0,
+      mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
+  // should get early ACK, NeedsBeginFrame= true, and 1st new surface ID
+  EXPECT_TRUE(GotEarlyAck());
+  // set a surface with needbeginframe = false, should not get early ack
+  support_->SetNeedsBeginFrame(false);
+  frame = CompositorFrameBuilder()
+              .AddRenderPass(gfx::Rect(5, 5), gfx::Rect())
+              .Build();
+  result = support_->MaybeSubmitCompositorFrame(
+      local_surface_id2, std::move(frame), base::nullopt, 0,
+      mojom::CompositorFrameSink::SubmitCompositorFrameSyncCallback());
+  // should get early NOT ACK, NeedsBeginFrame= FALSE
+  EXPECT_FALSE(GotEarlyAck());
+}
 }  // namespace viz