[cc] Plumb BeginFrameAcks through SurfaceManager to DisplayScheduler.

cc/surfaces/compositor_frame_sink_support.cc

Index: cc/surfaces/compositor_frame_sink_support.cc
diff --git a/cc/surfaces/compositor_frame_sink_support.cc b/cc/surfaces/compositor_frame_sink_support.cc
index 0fdb6c6855755951a03bcee2ce4fda161f209986..5d9525cdadcf5058006f0c4cd1bec57cd5b56a6e 100644
--- a/cc/surfaces/compositor_frame_sink_support.cc
+++ b/cc/surfaces/compositor_frame_sink_support.cc
@@ -11,7 +11,6 @@
 #include "cc/scheduler/begin_frame_source.h"
 #include "cc/surfaces/compositor_frame_sink_support_client.h"
 #include "cc/surfaces/display.h"
-#include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_info.h"
 #include "cc/surfaces/surface_manager.h"
 #include "cc/surfaces/surface_reference.h"
@@ -87,6 +86,9 @@ void CompositorFrameSinkSupport::SetNeedsBeginFrame(bool needs_begin_frame) {
 
 void CompositorFrameSinkSupport::BeginFrameDidNotSwap(
     const BeginFrameAck& ack) {
+  TRACE_EVENT2("cc", "CompositorFrameSinkSupport::BeginFrameDidNotSwap",
+               "ack.source_id", ack.source_id, "ack.sequence_number",
+               ack.sequence_number);
   // TODO(eseckler): While a pending CompositorFrame exists (see TODO below), we
   // should not acknowledge immediately. Instead, we should update the ack that
   // will be sent to DisplayScheduler when the pending frame is activated.
@@ -94,6 +96,14 @@ void CompositorFrameSinkSupport::BeginFrameDidNotSwap(
 
   // |has_damage| is not transmitted, but false by default.
   DCHECK(!ack.has_damage);
+
+  last_begin_frame_ack_ = ack;
+
+  if (producer_state_ == Surface::PENDING &&

[sunnyps, 2017/05/19 03:46:33]

I was thinking more along the lines of plumbing this to
SurfaceObserver::OnSurfaceDamaged(BeginFrameAck) but only if Surface is
activated. Something like this:

if (current_surface_ && !current_surface_->HasPendingFrame()) {
  surface_manager_->SurfaceModified(surface->surface_id(), ack);
}

DisplayScheduler can do the surface state tracking internally. I think that's
better because this is really specific to DisplayScheduler and we should
encapsulate it there as much as possible. We can make more changes to it later.

+      last_begin_frame_args_.source_id == ack.source_id &&
+      last_begin_frame_args_.sequence_number == ack.sequence_number) {
+    SetProducerState(Surface::IDLE);
+  }
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this, ack);
 }
@@ -112,7 +122,7 @@ void CompositorFrameSinkSupport::SubmitCompositorFrame(
   // |has_damage| is not transmitted.
   frame.metadata.begin_frame_ack.has_damage = true;
 
-  BeginFrameAck ack = frame.metadata.begin_frame_ack;
+  last_begin_frame_ack_ = frame.metadata.begin_frame_ack;
 
   if (!ui::LatencyInfo::Verify(frame.metadata.latency_info,
                                "RenderWidgetHostImpl::OnSwapCompositorFrame")) {
@@ -148,13 +158,15 @@ void CompositorFrameSinkSupport::SubmitCompositorFrame(
   }
   current_surface_ = std::move(surface);
 
+  SetProducerState(Surface::READY);

[sunnyps, 2017/05/19 03:46:34]

Similarly here we already plumb surface activation to
SurfaceObserver::OnSurfaceModified. We can tack on the BeginFrameAck to that and
do the state tracking in DisplayScheduler.

+
   // TODO(eseckler): The CompositorFrame submitted below might not be activated
   // right away b/c of surface synchronization. We should only send the
   // BeginFrameAck to DisplayScheduler when it is activated. This also means
   // that we need to stay an active BFO while a CompositorFrame is pending.
   // See https://crbug.com/703079.
   if (begin_frame_source_)
-    begin_frame_source_->DidFinishFrame(this, ack);
+    begin_frame_source_->DidFinishFrame(this, last_begin_frame_ack_);
 }
 
 void CompositorFrameSinkSupport::UpdateSurfaceReferences(
@@ -223,6 +235,16 @@ void CompositorFrameSinkSupport::ReferencedSurfacesChanged(
 void CompositorFrameSinkSupport::DidReceiveCompositorFrameAck() {
   DCHECK_GT(ack_pending_count_, 0);
   ack_pending_count_--;
+

[sunnyps, 2017/05/19 03:46:34]

See my later comments in DisplayScheduler first.

If the begin frame ack is compared to the scheduler's begin frame args we don't
need this because the scheduler only updates its begin frame args after
finishing the previous frame.

+  if (ack_pending_count_ == 0 && added_frame_observer_ &&
+      (last_begin_frame_args_.source_id != last_begin_frame_ack_.source_id ||
+       last_begin_frame_args_.sequence_number !=
+           last_begin_frame_ack_.sequence_number)) {
+    SetProducerState(Surface::PENDING);
+  } else {
+    SetProducerState(Surface::IDLE);
+  }
+
   if (!client_)
     return;
 
@@ -281,6 +303,8 @@ void CompositorFrameSinkSupport::Init(SurfaceManager* surface_manager) {
 
 void CompositorFrameSinkSupport::OnBeginFrame(const BeginFrameArgs& args) {
   UpdateNeedsBeginFramesInternal();
+  if (producer_state_ == Surface::IDLE)
+    SetProducerState(Surface::PENDING);
   last_begin_frame_args_ = args;
   if (client_)
     client_->OnBeginFrame(args);
@@ -340,9 +364,19 @@ std::unique_ptr<Surface> CompositorFrameSinkSupport::CreateSurface(
 }
 
 void CompositorFrameSinkSupport::DestroyCurrentSurface() {
+  SetProducerState(Surface::IDLE);
   surface_manager_->DestroySurface(std::move(current_surface_));
 }
 
+void CompositorFrameSinkSupport::SetProducerState(
+    Surface::ProducerState state) {
+  TRACE_EVENT1("cc", "CompositorFrameSinkSupport::SetProducerState", "state",
+               state);
+  producer_state_ = state;
+  if (current_surface_)
+    current_surface_->SetProducerState(state);
+}
+
 void CompositorFrameSinkSupport::RequestCopyOfSurface(
     std::unique_ptr<CopyOutputRequest> copy_request) {
   if (!current_surface_)