[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();
+  PendingFrameDiscarded();

[Fady Samuel, 2017/03/30 12:41:56]

There are other ways for frames to get discarded.

For example:

1. Submit a CompositorFrame that is pending with a given LocalSurfaceId.
2. Submit another CompositorFrame with a different LocalSurfaceId.

The first LocalSurfaceId will get discarded...I think you want to plumb through
OnSurfaceDiscarded.

[Eric Seckler, 2017/03/30 14:20:50]

In that particular case, I don't think we'd want to ack right away though, which
is why I'm not handling it as PendingFrameDiscarded at the moment. Instead, if
this happens, we just update the begin_frame_ack_for_pending_frame_ and wait for
the new frame to be activated before acking.

That said, there may easily be other cases for the surface to be discarded that
we _should_ handle as PendingFrameDiscarded().

I'm now plumbing through OnSurfaceDiscarded - and tracking the case above using
surface_id_for_pending_frame_ so that I don't prematurely call
AcknowledgeLastBeginFrameWithoutUpdates.

 }
 
 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);
+
+  if (has_pending_frame_) {

[Fady Samuel, 2017/03/30 12:41:56]

How do we get here?

[Eric Seckler, 2017/03/30 14:20:50]

A client may continue to receive further BeginFrames while a prior
CompositorFrame remains pending. In such cases, it's likely that it doesn't
submit another CompositorFrame (e.g. because it is CompositorFrameAck-throttled)
and thus acks the BeginFrames through BFDidNotSwap.

+    // We will acknowledge the BeginFrame once the pending frame activates.
+    begin_frame_ack_for_pending_frame_.source_id = ack.source_id;
+    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 send the BeginFrameAck to
+  // DisplayScheduler once it is activated.

[Fady Samuel, 2017/03/30 12:41:56]

I'm confused. We send a AcknowledgeLastBeginFrameWithoutUpdates, don't we?

[Eric Seckler, 2017/03/30 14:20:50]

Not right away. We only send the ack from
AcknowledgeLastBeginFrameWithoutUpdates if the pending frame isn't activated
before we receive the next BeginFrame(s). Otherwise we just postpone forwarding
the CompositorFrame's ack until PendingFrameActivated() - which may happen
before the next BeginFrame :)

+  has_pending_frame_ = true;
+  // 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;
+  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 45 matching lines @@
   // making the next CompositorFrame.
   if (!surface_returned_resources_.empty()) {
     client_->ReclaimResources(surface_returned_resources_);
     surface_returned_resources_.clear();
   }
   client_->DidReceiveCompositorFrameAck();
 }
 
 void CompositorFrameSinkSupport::ForceReclaimResources() {
   surface_factory_.ClearSurface();
+  PendingFrameDiscarded();

[Fady Samuel, 2017/03/30 12:41:56]

There are other ways for a pending frame to get discarded. See above. I think
you need OnSurfaceDiscarded plumbed through.

[Eric Seckler, 2017/03/30 14:20:50]

Done. I'll have to keep this call to PendingFrameDiscarded separately though,
right?

 }
 
 void CompositorFrameSinkSupport::ClaimTemporaryReference(
     const SurfaceId& surface_id) {
   surface_manager_->AssignTemporaryReference(surface_id, frame_sink_id_);
 }
 
 void CompositorFrameSinkSupport::ReferencedSurfacesChanged(
     const LocalSurfaceId& local_surface_id,
     const std::vector<SurfaceId>* active_referenced_surfaces,
@@ skipping 38 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) {
+  if (!has_pending_frame_)

[Fady Samuel, 2017/03/30 12:41:56]

I'm confused about when this can happen? Should this be a DCHECK?

[Eric Seckler, 2017/03/30 14:20:50]

Things like ClearSurface() submit CompositorFrames that I'm not tracking via
has_pending_frame_.

+    return;
+
+  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_);
+  UpdateNeedsBeginFramesInternal();

[Fady Samuel, 2017/03/30 12:41:56]

A comment about how we are no longer waiting for a frame to activate so we need
to check whether we need to continue to observe BeginFrames.

[Eric Seckler, 2017/03/30 14:20:50]

Done.

+}
+
+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() {
+  if (!last_begin_frame_args_.IsValid())

[Fady Samuel, 2017/03/30 12:41:55]

Why would this happen?

[Eric Seckler, 2017/03/30 14:20:50]

Well, not all clients wait for BeginFrames to submit CompositorFrames :P

Some just submit a first CompositorFrame without prior BeginFrame, and some draw
synchronously or don't care about BeginFrames at all (think android_webview).

+    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