[cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames.

cc/surfaces/compositor_frame_sink_support.cc

 // Copyright 2016 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.
 
 #include "cc/surfaces/compositor_frame_sink_support.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "cc/output/compositor_frame.h"
@@ skipping 34 matching lines @@
 
   // For display root surfaces, the surface is no longer going to be visible
   // so make it unreachable from the top-level root.
   if (surface_manager_->using_surface_references() && is_root_ &&
       reference_tracker_.current_surface_id().is_valid())
     RemoveTopLevelRootReference(reference_tracker_.current_surface_id());
 
   // SurfaceFactory's destructor will attempt to return resources which will
   // call back into here and access |client_| so we should destroy
   // |surface_factory_|'s resources early on.
-  surface_factory_.EvictSurface();
+  EvictFrame();
   surface_manager_->UnregisterSurfaceFactoryClient(frame_sink_id_);
   if (handles_frame_sink_id_invalidation_)
     surface_manager_->InvalidateFrameSinkId(frame_sink_id_);
 }
 
 void CompositorFrameSinkSupport::EvictFrame() {
   surface_factory_.EvictSurface();
 }
 
 void CompositorFrameSinkSupport::SetNeedsBeginFrame(bool needs_begin_frame) {
-  needs_begin_frame_ = needs_begin_frame;
+  client_needs_begin_frame_ = needs_begin_frame;
+  bool was_observing = added_frame_observer_;
   UpdateNeedsBeginFramesInternal();
+
+  // If we are observing because of a pending CompositorFrame, we may have
+  // previously received a BeginFrame that we should now forward to the client.
+  // BackToBackBeginFrameSource sends a new BeginFrame in a separate task
+  // provided we weren't observing before, so we special-case it here.
+  if (needs_begin_frame && !last_begin_frame_sent_to_client_ &&
+      (was_observing || begin_frame_source_->IsThrottled())) {
+    last_begin_frame_sent_to_client_ = true;
+    client_->OnBeginFrame(last_begin_frame_args_);
+  }
 }
 
 void CompositorFrameSinkSupport::BeginFrameDidNotSwap(
     const BeginFrameAck& ack) {
-  // TODO(eseckler): While a pending CompositorFrame exists (see TODO below), we
-  // should not acknowledge immediately. Instead, we should update the ack that
-  // will be sent to DisplayScheduler when the pending frame is activated.
   DCHECK_LE(BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);
+
+  if (ack.sequence_number < BeginFrameArgs::kStartingFrameNumber) {
+    DLOG(ERROR) << "Received BeginFrameDidNotSwap with invalid BeginFrameAck.";
+    return;
+  }
   // |has_damage| is not transmitted, but false by default.
   DCHECK(!ack.has_damage);
-  if (begin_frame_source_)
-    begin_frame_source_->DidFinishFrame(this, ack);
+
+  // Client can continue to request BeginFrames after submitting a
+  // CompositorFrame, so we may end up with a pending frame here.
+  if (begin_frame_ack_for_pending_frame_) {
+    // We will acknowledge the BeginFrame once the pending frame activates.
+    begin_frame_ack_for_pending_frame_->source_id = ack.source_id;

[brianderson, 2017/03/31 19:21:27]

Is it possible for the source_id to change?

If not, should this be a DCHECK?
If yes, do we need to store a collection of acks?

[Eric Seckler, 2017/04/03 11:21:00]

Considering headless use cases (dynamically replacing the BFS): yeah, it's
possible :)

We don't need to store multiple acks though, since we can always only ack the
latest BeginFrame. If the source changes, we just update the
latest_confirmed_sequence_number of the begin_frame_ack_for_pending_frame_ (that
we will eventually send when the CompositorFrame activates).

+    begin_frame_ack_for_pending_frame_->latest_confirmed_sequence_number =
+        ack.latest_confirmed_sequence_number;
+    return;
+  }
+
+  AcknowledgeBeginFrame(ack);
 }
 
 void CompositorFrameSinkSupport::SubmitCompositorFrame(
     const LocalSurfaceId& local_surface_id,
     CompositorFrame frame) {
   ++ack_pending_count_;
 
   if (frame.metadata.begin_frame_ack.sequence_number <
       BeginFrameArgs::kStartingFrameNumber) {
     DLOG(ERROR) << "Received CompositorFrame with invalid BeginFrameAck.";
     frame.metadata.begin_frame_ack.source_id = BeginFrameArgs::kManualSourceId;
     frame.metadata.begin_frame_ack.sequence_number =
         BeginFrameArgs::kStartingFrameNumber;
   }
   // |has_damage| is not transmitted.
   frame.metadata.begin_frame_ack.has_damage = true;
 
+  // The CompositorFrame submitted below might not be activated right away b/c
+  // of surface synchronization. We only forward the CompositorFrame's
+  // BeginFrameAck to DisplayScheduler once it is activated. While waiting for
+  // activation, we continue to observe BeginFrames. If the CompositorFrame is
+  // not activated before we receive the next BeginFrame, we will acknowledge
+  // the current one without updates. If there was a previous pending frame that
+  // didn't get activated, this overrides the pending BeginFrameAck.
+  begin_frame_ack_for_pending_frame_ = frame.metadata.begin_frame_ack;
+  surface_id_for_pending_frame_ = local_surface_id;
+  // Ensure that we continue observing BeginFrames while the frame is pending.
+  UpdateNeedsBeginFramesInternal();
+
   surface_factory_.SubmitCompositorFrame(
       local_surface_id, std::move(frame),
       base::Bind(&CompositorFrameSinkSupport::DidReceiveCompositorFrameAck,
                  weak_factory_.GetWeakPtr()));
-
-  // TODO(eseckler): The CompositorFrame submitted below might not be activated
-  // right away b/c of surface synchronization. We should only send the
-  // BeginFrameAck to DisplayScheduler when it is activated. This also means
-  // that we need to stay an active BFO while a CompositorFrame is pending.
-  // See https://crbug.com/703079.
-  if (begin_frame_source_)
-    begin_frame_source_->DidFinishFrame(this, frame.metadata.begin_frame_ack);
 }
 
 void CompositorFrameSinkSupport::UpdateSurfaceReferences(
     const SurfaceId& last_surface_id,
     const LocalSurfaceId& local_surface_id) {
   const bool surface_id_changed =
       last_surface_id.local_surface_id() != local_surface_id;
 
   // If this is a display root surface and the SurfaceId is changing, make the
   // new SurfaceId reachable from the top-level root.
@@ skipping 44 matching lines @@
   // We return the resources before sending an ack so they can be reused in
   // making the next CompositorFrame.
   if (!surface_returned_resources_.empty()) {
     client_->ReclaimResources(surface_returned_resources_);
     surface_returned_resources_.clear();
   }
   client_->DidReceiveCompositorFrameAck();
 }
 
 void CompositorFrameSinkSupport::ForceReclaimResources() {
+  PendingFrameDiscarded();
   surface_factory_.ClearSurface();
 }
 
 void CompositorFrameSinkSupport::ClaimTemporaryReference(
     const SurfaceId& surface_id) {
   surface_manager_->AssignTemporaryReference(surface_id, frame_sink_id_);
 }
 
 void CompositorFrameSinkSupport::ReferencedSurfacesChanged(
     const LocalSurfaceId& local_surface_id,
@@ skipping 39 matching lines @@
   UpdateNeedsBeginFramesInternal();
 }
 
 void CompositorFrameSinkSupport::WillDrawSurface(
     const LocalSurfaceId& local_surface_id,
     const gfx::Rect& damage_rect) {
   if (client_)
     client_->WillDrawSurface(local_surface_id, damage_rect);
 }
 
+void CompositorFrameSinkSupport::PendingFrameActivated(
+    const LocalSurfaceId& local_surface_id) {
+  // |begin_frame_ack_for_pending_frame_| may not be set because ClearSurface()
+  // submits CompositorFrames we don't track.
+  if (!begin_frame_ack_for_pending_frame_)
+    return;
+  DCHECK_EQ(surface_id_for_pending_frame_, local_surface_id);
+
+  if (last_begin_frame_args_.IsValid()) {

[brianderson, 2017/03/31 19:21:27]

Can you add a comment explaining why this block is needed?

[Eric Seckler, 2017/04/03 11:21:00]

Done.

+    if (last_begin_frame_args_.source_id !=
+        begin_frame_ack_for_pending_frame_->source_id) {
+      // BeginFrameSource has changed, we cannot confirm any sequence number.
+      begin_frame_ack_for_pending_frame_->source_id =
+          last_begin_frame_args_.source_id;
+      begin_frame_ack_for_pending_frame_->latest_confirmed_sequence_number =
+          BeginFrameArgs::kInvalidFrameNumber;
+    }
+
+    begin_frame_ack_for_pending_frame_->sequence_number =
+        last_begin_frame_args_.sequence_number;
+  }
+
+  DCHECK(begin_frame_ack_for_pending_frame_->has_damage);
+  AcknowledgeBeginFrame(*begin_frame_ack_for_pending_frame_);
+
+  begin_frame_ack_for_pending_frame_.reset();
+  // We may no longer need BeginFrames, since we no longer have a pending frame.
+  UpdateNeedsBeginFramesInternal();
+}
+
+void CompositorFrameSinkSupport::SurfaceDiscarded(
+    const LocalSurfaceId& local_surface_id) {
+  // Don't call PendingFrameDiscarded() if an older surface was discarded and a
+  // CompositorFrame may still be pending on the new surface.
+  if (local_surface_id != surface_id_for_pending_frame_)
+    return;
+
+  PendingFrameDiscarded();
+}
+
+void CompositorFrameSinkSupport::PendingFrameDiscarded() {
+  if (!begin_frame_ack_for_pending_frame_)
+    return;
+
+  begin_frame_ack_for_pending_frame_.reset();
+  AcknowledgeLastBeginFrameWithoutUpdates();
+  UpdateNeedsBeginFramesInternal();
+}
+
 void CompositorFrameSinkSupport::OnBeginFrame(const BeginFrameArgs& args) {
   UpdateNeedsBeginFramesInternal();
+
+  // If we are observing BeginFrames while a pending frame exists, we are
+  // deferring acknowledging until the pending frame activates. Thus, we may not
+  // have acknowledged the last BeginFrame yet. Do that now.
+  if (begin_frame_ack_for_pending_frame_)
+    AcknowledgeLastBeginFrameWithoutUpdates();
+
   last_begin_frame_args_ = args;
-  if (client_)
+  if (client_ && client_needs_begin_frame_) {
+    last_begin_frame_sent_to_client_ = true;
     client_->OnBeginFrame(args);
+  } else {
+    last_begin_frame_sent_to_client_ = false;
+  }
+}
+
+void CompositorFrameSinkSupport::AcknowledgeLastBeginFrameWithoutUpdates() {

[brianderson, 2017/03/31 19:21:27]

It looks like this acknowledges the most recently sent begin frame even before
the pending frame hasn't been acknowledged. When would you want to acknowledge
out of order? Am I misunderstanding something?

[Eric Seckler, 2017/04/03 11:21:00]

Correct. In order to eventually acknowledge the activated CompositorFrame's
updates (to latest_confirmed_sequence_number), we need to continue observing the
BeginFrame source. We use this method to acknowledge BeginFrames that pass while
the frame is still pending and to ack the current BeginFrame if the pending
frame is discarded for some reason.
You're right, I guess we could decide not to send acks for past BeginFrames
(during OnBeginFrame), since these acks are for not for the current BeginFrame
and contain no updates to latest_confirmed_sequence_number.

+  // Not all clients wait for a BeginFrame before submitting CompositorFrames.
+  if (!last_begin_frame_args_.IsValid())
+    return;
+
+  uint64_t confirmed_sequence_number = BeginFrameArgs::kInvalidFrameNumber;
+  if (last_begin_frame_args_.source_id ==
+      latest_confirmed_begin_frame_source_id_) {
+    confirmed_sequence_number = latest_confirmed_begin_frame_sequence_number_;
+  }
+  AcknowledgeBeginFrame(BeginFrameAck(last_begin_frame_args_.source_id,
+                                      last_begin_frame_args_.sequence_number,
+                                      confirmed_sequence_number, false));
+
+  // If we have a pending ack and the client doesn't currently need BeginFrames
+  // (thus is up-to-date), we should update the pending ack's
+  // latest_confirmed_frame_number now.
+  if (begin_frame_ack_for_pending_frame_ && !client_needs_begin_frame_) {
+    begin_frame_ack_for_pending_frame_->source_id =
+        last_begin_frame_args_.source_id;
+    begin_frame_ack_for_pending_frame_->latest_confirmed_sequence_number =
+        last_begin_frame_args_.sequence_number;
+  }
+}
+
+void CompositorFrameSinkSupport::AcknowledgeBeginFrame(
+    const BeginFrameAck& ack) {
+  latest_confirmed_begin_frame_source_id_ = ack.source_id;
+  latest_confirmed_begin_frame_sequence_number_ =
+      ack.latest_confirmed_sequence_number;
+  if (begin_frame_source_)
+    begin_frame_source_->DidFinishFrame(this, ack);
 }
 
 const BeginFrameArgs& CompositorFrameSinkSupport::LastUsedBeginFrameArgs()
     const {
   return last_begin_frame_args_;
 }
 
 void CompositorFrameSinkSupport::OnBeginFrameSourcePausedChanged(bool paused) {}
 
 void CompositorFrameSinkSupport::UpdateNeedsBeginFramesInternal() {
   if (!begin_frame_source_)
     return;
 
-  if (needs_begin_frame_ == added_frame_observer_)
+  // We continue observing BeginFrames while a frame is pending to be activated,
+  // so that we can eventually forward the corresponding BeginFrameAck to the
+  // source.
+  bool needs_begin_frame =
+      client_needs_begin_frame_ || begin_frame_ack_for_pending_frame_;
+
+  if (needs_begin_frame == added_frame_observer_)
     return;
 
-  added_frame_observer_ = needs_begin_frame_;
-  if (needs_begin_frame_)
+  added_frame_observer_ = needs_begin_frame;
+  if (added_frame_observer_)
     begin_frame_source_->AddObserver(this);
   else
     begin_frame_source_->RemoveObserver(this);
 }
 
 void CompositorFrameSinkSupport::RequestCopyOfSurface(
     std::unique_ptr<CopyOutputRequest> request) {
   surface_factory_.RequestCopyOfSurface(std::move(request));
 }
 
 }  // namespace cc