[cc] CompositorFrameSinkSupport: Defer BeginFrameAck of pending frames.

cc/surfaces/compositor_frame_sink_support.cc

Index: cc/surfaces/compositor_frame_sink_support.cc
diff --git a/cc/surfaces/compositor_frame_sink_support.cc b/cc/surfaces/compositor_frame_sink_support.cc
index 97c351f2f9acc2d5472b096bf2dcfbbf655ef8d6..b0f21866a1c5ff503954be2afd7ab3a86e948640 100644
--- a/cc/surfaces/compositor_frame_sink_support.cc
+++ b/cc/surfaces/compositor_frame_sink_support.cc
@@ -52,7 +52,7 @@ CompositorFrameSinkSupport::~CompositorFrameSinkSupport() {
   // 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_);
@@ -60,23 +60,45 @@ CompositorFrameSinkSupport::~CompositorFrameSinkSupport() {
 
 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(
@@ -94,18 +116,19 @@ void CompositorFrameSinkSupport::SubmitCompositorFrame(
   // |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(
@@ -171,6 +194,7 @@ void CompositorFrameSinkSupport::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(
@@ -229,11 +253,81 @@ void CompositorFrameSinkSupport::WillDrawSurface(
     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()
@@ -247,11 +341,16 @@ 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);