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

components/exo/surface.cc

 // Copyright 2015 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 "components/exo/surface.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/logging.h"
@@ skipping 414 matching lines @@
       has_pending_layer_changes_ = true;
     }
   }
 
   if (delegate_) {
     delegate_->OnSurfaceCommit();
   } else {
     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() {
   DCHECK(needs_commit_surface_hierarchy_);
   needs_commit_surface_hierarchy_ = false;
   has_pending_layer_changes_ = false;
 
   state_ = pending_state_;
   pending_state_.only_visible_on_secure_output = false;
 
@@ skipping 151 matching lines @@
       new base::trace_event::TracedValue());
   value->SetString("name", window_->layer()->name());
   return value;
 }
 
 void Surface::WillDraw() {
   active_frame_callbacks_.splice(active_frame_callbacks_.end(),
                                  frame_callbacks_);
   swapping_presentation_callbacks_.splice(
       swapping_presentation_callbacks_.end(), presentation_callbacks_);
+  UpdateNeedsBeginFrame();
 }
 
-bool Surface::NeedsBeginFrame() const {
-  return !active_frame_callbacks_.empty();
+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();
 }
 
-void Surface::BeginFrame(base::TimeTicks frame_time) {
+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);
+}
+
+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() {
   if (HasLayerHierarchyChanged())
     SetSurfaceHierarchyNeedsCommitToNewSurfaces();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ui::ContextFactoryObserver overrides:
 
@@ skipping 172 matching lines @@
                       gfx::Transform());
 
   gfx::Rect quad_rect = output_rect;
   cc::SharedQuadState* quad_state =
       render_pass->CreateAndAppendSharedQuadState();
   quad_state->quad_layer_bounds = contents_surface_size;
   quad_state->visible_quad_layer_rect = quad_rect;
   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) {
       cc::TextureDrawQuad* texture_quad =
           render_pass->CreateAndAppendDrawQuad<cc::TextureDrawQuad>();
       float vertex_opacity[4] = {1.0, 1.0, 1.0, 1.0};
       gfx::Rect opaque_rect;
       if (!current_resource_has_alpha_ ||
           state_.blend_mode == SkBlendMode::kSrc ||
           state_.opaque_region.contains(gfx::RectToSkIRect(quad_rect))) {
@@ skipping 15 matching lines @@
         render_pass->CreateAndAppendDrawQuad<cc::SolidColorDrawQuad>();
     solid_quad->SetNew(quad_state, quad_rect, quad_rect, SK_ColorBLACK, false);
   }
 
   frame.render_pass_list.push_back(std::move(render_pass));
   compositor_frame_sink_holder_->GetCompositorFrameSink()
       ->SubmitCompositorFrame(local_surface_id_, std::move(frame));
 }
 
 }  // namespace exo