[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

cc/surfaces/surface_dependency_tracker.cc

Index: cc/surfaces/surface_dependency_tracker.cc
diff --git a/cc/surfaces/surface_dependency_tracker.cc b/cc/surfaces/surface_dependency_tracker.cc
index 6cfeae8ec2b04abb49551738b222ebd5d5e1d58d..2a2d85d566bcc999944d021e5c142def5a8e0937 100644
--- a/cc/surfaces/surface_dependency_tracker.cc
+++ b/cc/surfaces/surface_dependency_tracker.cc
@@ -20,23 +20,21 @@ SurfaceDependencyTracker::SurfaceDependencyTracker(
     : surface_manager_(surface_manager),
       begin_frame_source_(begin_frame_source) {
   surface_manager_->AddObserver(this);
-  begin_frame_source_->AddObserver(this);
 }
 
 SurfaceDependencyTracker::~SurfaceDependencyTracker() {
   surface_manager_->RemoveObserver(this);
-  begin_frame_source_->RemoveObserver(this);
   for (Surface* pending_surface : pending_surfaces_)
     pending_surface->RemoveObserver(this);
   pending_surfaces_.clear();
+  UpdateNeedsBeginFrames();  // Stop observing the BeginFrameSource.
 }
 
 void SurfaceDependencyTracker::RequestSurfaceResolution(Surface* surface) {
   DCHECK(surface->HasPendingFrame());
 
   const CompositorFrame& pending_frame = surface->GetPendingFrame();
-  bool needs_begin_frame =
-      pending_frame.metadata.can_activate_before_dependencies;
+  bool needs_deadline = pending_frame.metadata.can_activate_before_dependencies;
 
   // Referenced surface IDs that aren't currently known to the surface manager
   // or do not have an active CompsotiorFrame block this frame.
@@ -52,15 +50,52 @@ void SurfaceDependencyTracker::RequestSurfaceResolution(Surface* surface) {
     pending_surfaces_.insert(surface);
   }
 
-  if (needs_begin_frame && !frames_since_deadline_set_)
+  if (needs_deadline && !frames_since_deadline_set_)
     frames_since_deadline_set_ = 0;
+
+  UpdateNeedsBeginFrames();  // |pending_surfaces_| changed.
+}
+
+void SurfaceDependencyTracker::UpdateNeedsBeginFrames() {
+  // We observe while there are pending surfaces, even if we don't have a
+  // deadline set. This way, we can send BeginFrameAcks on behalf of the
+  // pending CompositorFrames.
+  bool needs_begin_frames = !pending_surfaces_.empty();
+
+  if (needs_begin_frames_ == needs_begin_frames)
+    return;
+  needs_begin_frames_ = needs_begin_frames;
+
+  if (needs_begin_frames_)
+    begin_frame_source_->AddObserver(this);
+  else
+    begin_frame_source_->RemoveObserver(this);
 }
 
 void SurfaceDependencyTracker::OnBeginFrame(const BeginFrameArgs& args) {
+  // We only acknowledge the BeginFrame if and when all pending frames have
+  // been activated. While there are pending frames, we will not acknowledge the
+  // BeginFrame. This way, we indicate to the BeginFrameSource that we are still
+  // waiting on blockers to resolve and that the
+  // |latest_confirmed_sequence_number| of each pending frame is not integrated
+  // into any display frame produced before their activation.
+  //
+  // While we are not acknowledging, the BeginFrameSource will use the sequence
+  // number of the last BeginFrame that we did not receive as our
+  // latest_confirmed_sequence_number. This may not be completely accurate (a
+  // pending frame may include older updates), but it is as good as we can get,
+  // since we cannot retroactively unconfirm BeginFrames that we didn't receive.
+
   // If no deadline is set then we have nothing to do.
   if (!frames_since_deadline_set_)
     return;
 
+  if (args.source_id == last_begin_frame_args_.source_id &&
+      args.sequence_number == last_begin_frame_args_.sequence_number) {
+    // We've seen this BeginFrame already.
+    return;
+  }
+
   // TODO(fsamuel, kylechar): We have a single global deadline here. We should
   // scope deadlines to surface subtrees. We cannot do that until
   // SurfaceReferences have been fully implemented
@@ -79,11 +114,30 @@ void SurfaceDependencyTracker::OnBeginFrame(const BeginFrameArgs& args) {
 }
 
 const BeginFrameArgs& SurfaceDependencyTracker::LastUsedBeginFrameArgs() const {
-  return last_begin_frame_args_;
+  // Return invalid args, so that we receive the same BeginFrame multiple times
+  // if we start observing BeginFrames again during the same BeginFrame: Even if
+  // we already received and acknowledged a BeginFrame before, we want to
+  // acknowledge it again on behalf of any newly added pending surfaces.

[brianderson, 2017/04/04 00:29:19]

I'm not familiar with SurfaceDependencyTracker. When could you get a newly added
pending surface after having already acked a BeginFrame. Seems weird to double
ack.

[Eric Seckler, 2017/04/04 08:36:45]

I was erring on the side of caution, i.e. not assuming anything about when
during a BeginFrame a Surface can become pending / unblocked.

Consider this example:
 (1) Surface A becomes blocked on B. We start observing.
 (2) Surface B submits, unblocks A. We ack BeginFrame and stop observing.
 (3) Surface C becomes blocked on D. We start observing.
 (4) Surface D submits, unblocks C. We ack BeginFrame and stop observing.

If (1)-(3) happen within the same BeginFrame, we need to make sure that we
receive the same BeginFrame in (3), which we then have to re-ack
(BeginFrameObserverAckTracker supports that).

If we indicate (via LastUsedBeginFrameArgs) that we already used (and thus
received + acked) this BeginFrame, we would be indicating to the
BeginFrameSource (and thus, to DisplayScheduler) that the updates in the
CompositorFrame for Surface C have already propagated.

What I'm not sure about is whether it is currently possible for (1)-
(3) to happen during one BeginFrame - Fady? If it isn't, maybe we can somehow
DCHECK that we don't start observing twice during the same BeginFrame... Yet,
I'm not sure if we even what to restrict ourselves to that.

+  static BeginFrameArgs args;

[Fady Samuel, 2017/04/03 19:03:02]

This seems kinda icky... What's wrong with always observing a BeginFrameSource?

[Eric Seckler, 2017/04/04 08:36:45]

If we always observe, we have the problem that we don't quite know when to ack a
BeginFrame if we don't have any pending surfaces.

Consider this example (3a and 3b being alternatives):
 (1) no pending surfaces.
 (2) BeginFrame.
 (3a) CFSS for surface A submits a non-blocking frame. All other CFSS report no
damage.
 (3b) CFSS for surface A submits a frame that is blocked on B. All other CFSS
report no damage.

For (3a), SurfaceDependencyTracker should have acked in (2) immediately. For
(3b), SurfaceDependencyTracker shouldn't ack at all b/c a pending frame exist.

But at (2), it doesn't know whether (3a) or (3b) will happen, so can't decide
whether to ack or wait for some surface/frame to become pending.

By starting to observe only when a surface becomes pending, we still receive the
BeginFrame from (2) as missed BeginFrame during (3b). That is, we receive it
only once we know that we shouldn't ack it immediately.

+  return args;
 }
 
 void SurfaceDependencyTracker::OnBeginFrameSourcePausedChanged(bool paused) {}
 
+void SurfaceDependencyTracker::AcknowledgeLastBeginFrame() {
+  DCHECK(needs_begin_frames_);
+  DCHECK(pending_surfaces_.empty());

[Fady Samuel, 2017/04/03 19:03:02]

This makes me kind of sad...but maybe it's OK for now... Maybe a TODO to see if
there's a better way?

[Eric Seckler, 2017/04/04 08:36:45]

Yeah. Added one. FWIW, I'm going to try out tracking the CFSS acks in
SurfaceDependencyTracker. If only to confirm that it would be ugly... :P

+
+  // Some clients submit CompositorFrames without prior BeginFrames.
+  if (!last_begin_frame_args_.IsValid())
+    return;
+
+  begin_frame_source_->DidFinishFrame(
+      this, BeginFrameAck(last_begin_frame_args_.source_id,
+                          last_begin_frame_args_.sequence_number,
+                          last_begin_frame_args_.sequence_number, true));
+}
+
 void SurfaceDependencyTracker::OnReferencedSurfacesChanged(
     Surface* surface,
     const std::vector<SurfaceId>* active_referenced_surfaces,
@@ -117,9 +171,14 @@ void SurfaceDependencyTracker::OnSurfaceDiscarded(Surface* surface) {
   if (blocked_surfaces_.empty())
     frames_since_deadline_set_.reset();
 
-  pending_surfaces_.erase(surface);
+  DCHECK(!pending_surfaces_.empty());
+  DCHECK(pending_surfaces_.erase(surface));
   surface->RemoveObserver(this);
 
+  if (pending_surfaces_.empty())
+    AcknowledgeLastBeginFrame();
+  UpdateNeedsBeginFrames();  // |pending_surfaces_| changed.

[Fady Samuel, 2017/04/03 19:03:02]

nit: put this comment above.

+
   // Pretend that the discarded surface's SurfaceId is now available to unblock
   // dependencies because we now know the surface will never activate.
   NotifySurfaceIdAvailable(surface->surface_id());
@@ -127,7 +186,13 @@ void SurfaceDependencyTracker::OnSurfaceDiscarded(Surface* surface) {
 
 void SurfaceDependencyTracker::OnSurfaceActivated(Surface* surface) {
   surface->RemoveObserver(this);
-  pending_surfaces_.erase(surface);
+  DCHECK(!pending_surfaces_.empty());
+  DCHECK(pending_surfaces_.erase(surface));
+
+  if (pending_surfaces_.empty())
+    AcknowledgeLastBeginFrame();
+  UpdateNeedsBeginFrames();  // |pending_surfaces_| changed.

[Fady Samuel, 2017/04/03 19:03:02]

nit: put this comment above.

+

[Fady Samuel, 2017/04/03 19:03:02]

nit: remove.

   NotifySurfaceIdAvailable(surface->surface_id());
 }