[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..386d99a1a9110a4d7179f6e700707446656bdca9 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,53 @@ 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;
+
+  // |pending_surfaces_| changed.
+  UpdateNeedsBeginFrames();
+}
+
+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 +115,34 @@ 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.
+  static BeginFrameArgs args;
+  return args;
 }
 
 void SurfaceDependencyTracker::OnBeginFrameSourcePausedChanged(bool paused) {}
 
+void SurfaceDependencyTracker::AcknowledgeLastBeginFrame() {
+  DCHECK(needs_begin_frames_);
+  // TODO(eseckler): By not acking while a pending surface exist, we will
+  // prevent early deadlines in DisplayScheduler, see https://crbug.com/707513.
+  // Maybe there's a way to detect when it is unlikely/impossible for blockers
+  // to resolve during the current BeginFrame.
+  DCHECK(pending_surfaces_.empty());
+
+  // 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 +176,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();
+  // |pending_surfaces_| changed.
+  UpdateNeedsBeginFrames();
   // 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 +191,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();
+  // |pending_surfaces_| changed.
+  UpdateNeedsBeginFrames();
   NotifySurfaceIdAvailable(surface->surface_id());
 }