DirectCompositorFrameSink Uses CompositorFrameSinkSupport

cc/surfaces/direct_compositor_frame_sink.cc

Index: cc/surfaces/direct_compositor_frame_sink.cc
diff --git a/cc/surfaces/direct_compositor_frame_sink.cc b/cc/surfaces/direct_compositor_frame_sink.cc
index 10c37ee7cc3842ae45d3b48987aead9a936d9612..b2f8ad40ef3dc4f6900b36937ac3d0a7dc74698f 100644
--- a/cc/surfaces/direct_compositor_frame_sink.cc
+++ b/cc/surfaces/direct_compositor_frame_sink.cc
@@ -29,14 +29,12 @@ DirectCompositorFrameSink::DirectCompositorFrameSink(
                           shared_bitmap_manager),
       frame_sink_id_(frame_sink_id),
       surface_manager_(surface_manager),
-      display_(display),
-      factory_(frame_sink_id, surface_manager, this) {
+      display_(display) {
   DCHECK(thread_checker_.CalledOnValidThread());
   capabilities_.can_force_reclaim_resources = true;
   // Display and DirectCompositorFrameSink share a GL context, so sync
   // points aren't needed when passing resources between them.
   capabilities_.delegated_sync_points_required = false;
-  factory_.set_needs_sync_points(false);
 }
 
 DirectCompositorFrameSink::DirectCompositorFrameSink(
@@ -47,8 +45,7 @@ DirectCompositorFrameSink::DirectCompositorFrameSink(
     : CompositorFrameSink(std::move(vulkan_context_provider)),
       frame_sink_id_(frame_sink_id),
       surface_manager_(surface_manager),
-      display_(display),
-      factory_(frame_sink_id_, surface_manager, this) {
+      display_(display) {
   DCHECK(thread_checker_.CalledOnValidThread());
   capabilities_.can_force_reclaim_resources = true;
 }
@@ -64,26 +61,27 @@ bool DirectCompositorFrameSink::BindToClient(
   if (!CompositorFrameSink::BindToClient(client))
     return false;
 
-  surface_manager_->RegisterSurfaceFactoryClient(frame_sink_id_, this);
-
   // We want the Display's output surface to hear about lost context, and since
   // this shares a context with it, we should not be listening for lost context
   // callbacks on the context here.
   if (auto* cp = context_provider())
     cp->SetLostContextCallback(base::Closure());
 
+  support_ = base::MakeUnique<CompositorFrameSinkSupport>(
+      this, surface_manager_, frame_sink_id_, display_);
+  support_->SetNeedsSyncPoints(capabilities_.delegated_sync_points_required);

[Fady Samuel, 2017/01/29 15:54:54]

Another question to danakj@. Does it hurt to always use sync points at this
point and not introduce this API? DirectCompositorFrameSink will no longer be
direct and so we'll always need sync points. Can we drop this API now and just
have sync points always on?

   // Avoid initializing GL context here, as this should be sharing the
   // Display's context.
-  display_->Initialize(this, surface_manager_);
+
+  begin_frame_source_ = base::MakeUnique<ExternalBeginFrameSource>(this);
+  client_->SetBeginFrameSource(begin_frame_source_.get());
   return true;
 }
 
 void DirectCompositorFrameSink::DetachFromClient() {
-  // Unregister the SurfaceFactoryClient here instead of the dtor so that only
-  // one client is alive for this namespace at any given time.
-  surface_manager_->UnregisterSurfaceFactoryClient(frame_sink_id_);
-  factory_.EvictSurface();
-
+  support_.reset();
+  client_->SetBeginFrameSource(nullptr);
+  begin_frame_source_.reset();
   CompositorFrameSink::DetachFromClient();
 }
 
@@ -93,30 +91,12 @@ void DirectCompositorFrameSink::SubmitCompositorFrame(CompositorFrame frame) {
     delegated_local_surface_id_ = surface_id_allocator_.GenerateId();
     last_swap_frame_size_ = frame_size;
   }
-  display_->SetLocalSurfaceId(delegated_local_surface_id_,
-                              frame.metadata.device_scale_factor);
-
-  factory_.SubmitCompositorFrame(
-      delegated_local_surface_id_, std::move(frame),
-      base::Bind(&DirectCompositorFrameSink::DidDrawCallback,
-                 base::Unretained(this)));
+  support_->SubmitCompositorFrame(delegated_local_surface_id_,
+                                  std::move(frame));
 }
 
 void DirectCompositorFrameSink::ForceReclaimResources() {
-  if (delegated_local_surface_id_.is_valid())
-    factory_.ClearSurface();
-}
-
-void DirectCompositorFrameSink::ReturnResources(
-    const ReturnedResourceArray& resources) {
-  if (client_)
-    client_->ReclaimResources(resources);
-}
-
-void DirectCompositorFrameSink::SetBeginFrameSource(
-    BeginFrameSource* begin_frame_source) {
-  DCHECK(client_);
-  client_->SetBeginFrameSource(begin_frame_source);
+  support_->ForceReclaimResources();
 }
 
 void DirectCompositorFrameSink::DisplayOutputSurfaceLost() {
@@ -136,8 +116,32 @@ void DirectCompositorFrameSink::DisplayDidDrawAndSwap() {
   // be drawn.
 }
 
+void DirectCompositorFrameSink::DidReceiveCompositorFrameAck() {
+  if (client_)
+    client_->DidReceiveCompositorFrameAck();
+}
+
+void DirectCompositorFrameSink::OnBeginFrame(const BeginFrameArgs& args) {
+  begin_frame_source_->OnBeginFrame(args);
+}
+
+void DirectCompositorFrameSink::ReclaimResources(
+    const ReturnedResourceArray& resources) {
+  if (client_)
+    client_->ReclaimResources(resources);
+}
+
+void DirectCompositorFrameSink::WillDrawSurface() {
+  // TODO(staraz): Implement this.
+}
+
 void DirectCompositorFrameSink::DidDrawCallback() {
   client_->DidReceiveCompositorFrameAck();
 }
 
+void DirectCompositorFrameSink::OnNeedsBeginFrames(bool needs_begin_frame) {
+  if (support_)

[Fady Samuel, 2017/01/29 15:52:56]

I'm guessing this is probably broken. If you need this guard then that suggests
you probably have a start up or tear down issue. Either way, I think you should
store |needs_begin_frame| in a local variable, and pass it into |support_|
immediately when you create |support_|. That way, you don't miss a
NeedsBeginFrame and neglect to generate new frames.

[Alex Z., 2017/01/30 20:37:49]

Done.

+    support_->SetNeedsBeginFrame(needs_begin_frame);
+}
+
 }  // namespace cc