[exo] Clean up BeginFrame distribution & add acks in Surface.

components/exo/surface.cc

Index: components/exo/surface.cc
diff --git a/components/exo/surface.cc b/components/exo/surface.cc
index 87653987af4a8b2a2d96c1f5e6963245096f863c..b49c7c4690d054a424b26b13a7ccb58a70ef0b10 100644
--- a/components/exo/surface.cc
+++ b/components/exo/surface.cc
@@ -432,6 +432,13 @@ void Surface::Commit() {
     CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces();
     CommitSurfaceHierarchy();
   }
+
+  if (begin_frame_source_ && current_begin_frame_ack_.sequence_number !=
+                                 cc::BeginFrameArgs::kInvalidFrameNumber) {
+    begin_frame_source_->DidFinishFrame(this, current_begin_frame_ack_);

[Eric Seckler, 2017/03/07 14:01:59]

reveman@: Since I'm not very familiar with wayland, I'm wondering if this is the
right (and only) place that we should send the ack.

Generally, an observer should send an ack once it has completed processing a
BeginFrame that was sent to it. Many observers have to do this in two places:
(a) after "drawing" (submitting a CompositorFrame in this case) and (b) if they
decide not to draw during a BeginFrame (for whatever reason).

At the moment, the exo::Surface is acknowledging after Commit(), which I assume
covers (a). From what I can tell, the common behavior of wayland clients is to
request a frame callback, do some work when the callback is executed during the
next BeginFrame, and then perform a commit. I'm also guessing that the commit
can happen asynchronous to the frame callback execution?

Is there some way for the clients to tell the surface that they decided not to
perform a commit after the frame callback is executed, which would cover (b)? Or
is that not common?

[reveman, 2017/03/07 14:16:21]

Yes, all wayland protocol is async. 
No, there's no way to tell the compositor that the client will not to perform a
commit after the frame callback. It's expected that the client doesn't request a
callback unless needed. However, we can't trust that the client will commit even
if it requests this callback. How are we handling a failure to respond to a
begin frame by a renderer? Same will likely apply to wayland clients as both are
external clients that can be malicious and misbehave.

 

+    current_begin_frame_ack_.sequence_number =
+        cc::BeginFrameArgs::kInvalidFrameNumber;
+  }
 }
 
 void Surface::CommitSurfaceHierarchy() {
@@ -603,17 +610,42 @@ void Surface::WillDraw() {
                                  frame_callbacks_);
   swapping_presentation_callbacks_.splice(
       swapping_presentation_callbacks_.end(), presentation_callbacks_);
+  UpdateNeedsBeginFrame();
+}
+
+void Surface::SetBeginFrameSource(cc::BeginFrameSource* begin_frame_source) {
+  if (begin_frame_observer_added_) {
+    DCHECK(begin_frame_source_);
+    begin_frame_source_->RemoveObserver(this);
+    begin_frame_observer_added_ = false;
+  }
+  begin_frame_source_ = begin_frame_source;
+  UpdateNeedsBeginFrame();
 }
 
-bool Surface::NeedsBeginFrame() const {
-  return !active_frame_callbacks_.empty();
+void Surface::UpdateNeedsBeginFrame() {
+  if (!begin_frame_source_)
+    return;
+
+  bool needs_begin_frame = !active_frame_callbacks_.empty();
+  if (needs_begin_frame == begin_frame_observer_added_)
+    return;
+
+  begin_frame_observer_added_ = needs_begin_frame;
+  if (begin_frame_observer_added_)
+    begin_frame_source_->AddObserver(this);
+  else
+    begin_frame_source_->RemoveObserver(this);
 }
 
-void Surface::BeginFrame(base::TimeTicks frame_time) {
+bool Surface::OnBeginFrameDerivedImpl(const cc::BeginFrameArgs& args) {
+  current_begin_frame_ack_ = cc::BeginFrameAck(
+      args.source_id, args.sequence_number, args.sequence_number, 0, false);
   while (!active_frame_callbacks_.empty()) {
-    active_frame_callbacks_.front().Run(frame_time);
+    active_frame_callbacks_.front().Run(args.frame_time);
     active_frame_callbacks_.pop_front();
   }
+  return true;
 }
 
 void Surface::CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces() {
@@ -806,6 +838,9 @@ void Surface::UpdateSurface(bool full_damage) {
   quad_state->opacity = state_.alpha;
 
   cc::CompositorFrame frame;
+  current_begin_frame_ack_.has_damage = true;
+  frame.metadata.begin_frame_ack = current_begin_frame_ack_;
+
   if (current_resource_.id) {
     // Texture quad is only needed if buffer is not fully transparent.
     if (state_.alpha) {