[cc] Acknowledge BeginFrames in SurfaceDependencyTracker.

cc/surfaces/surface_dependency_tracker.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/surface_dependency_tracker.h"
 
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_info.h"
 #include "cc/surfaces/surface_manager.h"
 
 namespace cc {
 
 namespace {
 constexpr uint32_t kMaxBeginFrameCount = 4;
 }
 
 SurfaceDependencyTracker::SurfaceDependencyTracker(
     SurfaceManager* surface_manager,
     BeginFrameSource* begin_frame_source)
     : 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.
   for (const SurfaceId& surface_id :
        pending_frame.metadata.referenced_surfaces) {
     Surface* surface_dependency = surface_manager_->GetSurfaceForId(surface_id);
     if (!surface_dependency || !surface_dependency->HasActiveFrame())
       blocked_surfaces_[surface_id].insert(surface);
   }
 
   if (!pending_surfaces_.count(surface)) {
     surface->AddObserver(this);
     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
   // (see https://crbug.com/689719).
   last_begin_frame_args_ = args;
   // Nothing to do if we haven't hit a deadline yet.
   if (++(*frames_since_deadline_set_) != kMaxBeginFrameCount)
     return;
 
   // Activate all surfaces that respect the deadline.
   PendingSurfaceSet pending_surfaces(pending_surfaces_);
   for (Surface* pending_surface : pending_surfaces)
     pending_surface->ActivatePendingFrameForDeadline();
 
   frames_since_deadline_set_.reset();
 }
 
 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,
     const std::vector<SurfaceId>* pending_referenced_surfaces) {}
 
 void SurfaceDependencyTracker::OnSurfaceDiscarded(Surface* surface) {
   // If the surface being destroyed doesn't have a pending frame then we have
   // nothing to do here.
   if (!surface->HasPendingFrame())
     return;
@@ skipping 13 matching lines @@
     if (pending_surface_it != pending_surface_set.end()) {
       pending_surface_set.erase(surface);
       if (pending_surface_set.empty())
         blocked_surfaces_.erase(surface_id);
     }
   }
 
   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());
 }
 
 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());
 }
 
 void SurfaceDependencyTracker::OnSurfaceDependenciesChanged(
     Surface* surface,
     const SurfaceDependencies& added_dependencies,
     const SurfaceDependencies& removed_dependencies) {
   // Update the |blocked_surfaces_| map with the changes in dependencies.
   for (const SurfaceId& surface_id : added_dependencies)
     blocked_surfaces_[surface_id].insert(surface);
@@ skipping 37 matching lines @@
   // have a deadline.
   if (blocked_surfaces_.empty())
     frames_since_deadline_set_.reset();
 
   // Tell each surface about the availability of its blocker.
   for (Surface* blocked_pending_surface : blocked_pending_surface_set)
     blocked_pending_surface->NotifySurfaceIdAvailable(surface_id);
 }
 
 }  // namespace cc