[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 (has_pending_frame_) {
+    // We will acknowledge the BeginFrame once the pending frame activates.
+    begin_frame_ack_for_pending_frame_.source_id = ack.source_id;

[Fady Samuel, 2017/03/31 00:09:02]

I wonder if we can modify the pending CompositorFrame's BeginFrameAck directly
instead of messing with a copy here? Would that be messier?

+    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.
+  has_pending_frame_ = true;

[Fady Samuel, 2017/03/31 00:09:02]

Surface::HasPendingFrame exists.

Would it make sense to plumb that out from SurfaceFactory?

bool HasPendingFrame() const {
  return surface_factory_->current_surface() ?
surface_factorysurface_factory_->current_surface()->HasPendingFrame() : false;


 [rename current_surface_for_testing()]

+  surface_id_for_pending_frame_ = local_surface_id;
+  // 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;
+  // 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) {
+  // has_pending_frame_ may be false because ClearSurface() submits
+  // CompositorFrames we don't track.
+  if (!has_pending_frame_)
+    return;
+  DCHECK_EQ(surface_id_for_pending_frame_, local_surface_id);
+  has_pending_frame_ = false;
+
+  if (last_begin_frame_args_.IsValid()) {
+    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_);
+  // We may no longer need BeginFrames, since has_pending_frame_ changed.
+  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 (!has_pending_frame_)
+    return;
+
+  has_pending_frame_ = false;
+  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 (has_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() {
+  // 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));
+}
+
+void CompositorFrameSinkSupport::AcknowledgeBeginFrame(
+    const BeginFrameAck& ack) {
+  latest_confirmed_begin_frame_source_id_ =
+      ack.latest_confirmed_sequence_number;
+  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_ || has_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