Re-land Early Ack Pending Queued Surfaces

Ack a pending surface early the client's scheduler to schedule frames
or 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.

Earlier review: https://chromium-review.googlesource.com/c/chromium/src/+/2222918

Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1060058
Bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1087865
Change-Id: I6707d264064d16357a0ffe5bdfe5521a7e903bec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2222803Reviewed-by: Jonathan Ross <jonross@chromium.org>
Commit-Queue: Nazih Almalki <nalmalki@google.com>
Cr-Commit-Position: refs/heads/master@{#774701}

components/viz/service/frame_sinks/compositor_frame_sink_support.cc

@@ -183,7 +183,7 @@ void CompositorFrameSinkSupport::OnFrameTokenChanged(uint32_t frame_token) {
 }
 
 void CompositorFrameSinkSupport::OnSurfaceProcessed(Surface* surface) {
-  DidReceiveCompositorFrameAck();
+  DidReceiveCompositorFrameAck(/*early_ack*/ false);
 }
 
 void CompositorFrameSinkSupport::OnSurfaceAggregatedDamage(
@@ -555,6 +555,35 @@ 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
+      // early_ack during the call 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.
+      // In some cases the 2 frame comes and require early ack before the
+      // surface sync work is completed add counter early_acked_count_ to make
+      // sure right ack get cleared
+      if (client_needs_begin_frame_) {
+        if ((current_surface == prev_surface) ||
+            last_pending_frame_was_new_surface_) {
+          early_acked_count_++;
+          DidReceiveCompositorFrameAck(/*early_ack*/ 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.
@@ -585,22 +614,39 @@ void CompositorFrameSinkSupport::HandleCallback() {
   surface_returned_resources_.clear();
 }
 
-void CompositorFrameSinkSupport::DidReceiveCompositorFrameAck() {
-  DCHECK_GT(ack_pending_count_, 0);
-  ack_pending_count_--;
-  if (!client_)
-    return;
+void CompositorFrameSinkSupport::DidReceiveCompositorFrameAck(bool early_ack) {
+  // Only do early ack on frame that don't have callback
+  if (early_ack) {
+    DCHECK_GT(early_acked_count_, 0);
+    if (!client_ || compositor_frame_callback_)
+      return;
+    // early Ack client
+    client_->DidReceiveCompositorFrameAck(surface_returned_resources_);
+    // don't clear resources_ as it is still been used by surface_manager
+  } else {  // actual ack
+    DCHECK_GT(ack_pending_count_, 0);
+    ack_pending_count_--;
+    if (!client_)
+      return;
+
+    // If we have a callback, we only return the resource on onBeginFrame.
+    if (compositor_frame_callback_) {
+      if (early_acked_count_ > 0)  // if there was an early ack decrement it.
+        early_acked_count_--;
+      callback_received_receive_ack_ = true;
+      UpdateNeedsBeginFramesInternal();
+      HandleCallback();
+      return;
+    }
 
-  // If we have a callback, we only return the resource on onBeginFrame.
-  if (compositor_frame_callback_) {
-    callback_received_receive_ack_ = true;
-    UpdateNeedsBeginFramesInternal();
-    HandleCallback();
-    return;
+    if (early_acked_count_ == 0) {  // no early ACK has been sent, send an ACK
+      client_->DidReceiveCompositorFrameAck(surface_returned_resources_);
+    } else {  // early count is bigger than zero, decrement it
+      early_acked_count_--;
+    }
+    // for any ack request, that is not early clear resources
+    surface_returned_resources_.clear();
   }
-
-  client_->DidReceiveCompositorFrameAck(surface_returned_resources_);
-  surface_returned_resources_.clear();
 }
 
 void CompositorFrameSinkSupport::DidPresentCompositorFrame(
@@ -639,15 +685,15 @@ void CompositorFrameSinkSupport::DidRejectCompositorFrame(
     std::vector<ui::LatencyInfo> latency_info) {
   TRACE_EVENT_INSTANT0("viz", "DidRejectCompositorFrame",
                        TRACE_EVENT_SCOPE_THREAD);
-  // TODO(eseckler): Should these be stored and attached to the next successful
-  // frame submission instead?
+  // TODO(eseckler): Should these be stored and attached to the next
+  // successful frame submission instead?
   for (ui::LatencyInfo& info : latency_info)
     info.Terminate();
 
   std::vector<ReturnedResource> resources =
       TransferableResource::ReturnResources(frame_resource_list);
   ReturnResources(resources);
-  DidReceiveCompositorFrameAck();
+  DidReceiveCompositorFrameAck(/*early_ack*/ false);
   DidPresentCompositorFrame(frame_token, base::TimeTicks(), gfx::SwapTimings(),
                             gfx::PresentationFeedback::Failure());
 }
@@ -834,8 +880,8 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
     return false;
   }
 
-  // We might throttle this OnBeginFrame() if it's been less than a second since
-  // the last one was sent.
+  // We might throttle this OnBeginFrame() if it's been less than a second
+  // since the last one was sent.
   bool can_throttle =
       (frame_time - last_frame_time_) < base::TimeDelta::FromSeconds(1);
 
@@ -851,8 +897,8 @@ bool CompositorFrameSinkSupport::ShouldSendBeginFrame(
     return true;
   }
 
-  // We should never throttle BeginFrames if there is another client waiting for
-  // this client to submit a frame.
+  // We should never throttle BeginFrames if there is another client waiting
+  // for this client to submit a frame.
   if (surface_manager_->HasBlockedEmbedder(frame_sink_id_)) {
     RecordShouldSendBeginFrame(SendBeginFrameResult::kSendBlockedEmbedded);
     return true;

components/viz/service/frame_sinks/compositor_frame_sink_support.h

@@ -208,7 +208,7 @@ class VIZ_SERVICE_EXPORT CompositorFrameSinkSupport
   // Creates a surface reference from the top-level root to |surface_id|.
   SurfaceReference MakeTopLevelRootReference(const SurfaceId& surface_id);
 
-  void DidReceiveCompositorFrameAck();
+  void DidReceiveCompositorFrameAck(bool early_ack);
   void DidPresentCompositorFrame(uint32_t frame_token,
                                  base::TimeTicks draw_start_timestamp,
                                  const gfx::SwapTimings& swap_timings,
@@ -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;
+  uint8_t early_acked_count_ = 0;
   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 GetEarlyAckCount() const { return support_->early_acked_count_; }
 
   void AddResourcesToFrame(CompositorFrame* frame,
                            ResourceId* resource_ids,
@@ -1437,5 +1438,74 @@ 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;
+  uint8_t EarlyAckCount;
+  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_EQ(GetEarlyAckCount(), 0);
+
+  // 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, count should be 1, NeedsBeginFrame= true, and not new
+  // surface ID
+  EXPECT_EQ(GetEarlyAckCount(), 1);
+  EarlyAckCount = GetEarlyAckCount();
+
+  // 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
+  // hence the count should not drop, it be either 1 or 2
+  EXPECT_GE(GetEarlyAckCount(), EarlyAckCount);
+  EarlyAckCount = GetEarlyAckCount();
+  // 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, hence it the count should not increase, it could
+  // decrease
+  EXPECT_LE(GetEarlyAckCount(), EarlyAckCount);
+}
 }  // namespace viz