DirectCompositorFrameSink Uses CompositorFrameSinkSupport

cc/surfaces/direct_compositor_frame_sink.cc

 // Copyright 2014 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/direct_compositor_frame_sink.h"
 
 #include "base/bind.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/output/compositor_frame_sink_client.h"
 #include "cc/surfaces/display.h"
@@ skipping 11 matching lines @@
     scoped_refptr<ContextProvider> context_provider,
     scoped_refptr<ContextProvider> worker_context_provider,
     gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
     SharedBitmapManager* shared_bitmap_manager)
     : CompositorFrameSink(std::move(context_provider),
                           std::move(worker_context_provider),
                           gpu_memory_buffer_manager,
                           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(
     const FrameSinkId& frame_sink_id,
     SurfaceManager* surface_manager,
     Display* display,
     scoped_refptr<VulkanContextProvider> vulkan_context_provider)
     : 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;
 }
 
 DirectCompositorFrameSink::~DirectCompositorFrameSink() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 bool DirectCompositorFrameSink::BindToClient(
     CompositorFrameSinkClient* client) {
   DCHECK(thread_checker_.CalledOnValidThread());

[xlai (Olivia), 2017/02/02 06:48:44]

Duplicate DCHECK as in CompositorFrameSink::BindToClient?

[Alex Z., 2017/02/02 17:01:56]

danakj thinks it makes sense to get rid of the thread checker in the 
base class. I will do it in a separate CL.

[xlai (Olivia), 2017/02/02 19:25:52]

Okay.

 
   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());
 
+  begin_frame_source_ = base::MakeUnique<ExternalBeginFrameSource>(this);
+  client_->SetBeginFrameSource(begin_frame_source_.get());
+
+  support_ = base::MakeUnique<CompositorFrameSinkSupport>(

[xlai (Olivia), 2017/02/02 06:48:44]

Ah, I missed that here creates a unique pointer of CompositorFrameSinkSupport,
instead of being in the constructor like the other use cases. I notice that all
the arguments needed for creating the support_ are already provided in
DirectCompositorFrameSink's constructor; is there any specific reason why you
delay the creation until this function? If this is intentional then I think you
should put "if (support_) {...}" in the rest of functions that use support_ or
DCHECK to prevent nullpointer access.

[Alex Z., 2017/02/02 16:18:00]

It is intentional. 
Originally, DirectCompositorFrameSink registers and unregisters 
SurfaceFactoryClient (and other initializations) in BindToClient and 
DetachFromClient() to ensure that only one client is alive for the 
namespace at any given time. It makes sense to create 
CompositorFrameSinkSupport in BindToClient() because most of the 
initializations are in CompositorFrameSinkSupport().

The if (support_) is not needed since DirectCompositorFrameSink has
support_ whenever it is bound to a client and those methods are 
called by its client only.

[danakj, 2017/02/02 17:03:34]

FWIW I guess isn't spelled out very well in comments here
https://cs.chromium.org/chromium/src/cc/output/compositor_frame_sink.h but this
class is not able to be used until BindToClient is called. That should always be
the first method used.

[xlai (Olivia), 2017/02/02 19:25:52]

I see. lgtm on this and for offscreen_canvas_compositor_frame_sink. 

+      this, surface_manager_, frame_sink_id_, display_,
+      capabilities_.delegated_sync_points_required);
+  support_->SetNeedsBeginFrame(needs_begin_frame_);
+
   // Avoid initializing GL context here, as this should be sharing the
   // Display's context.
-  display_->Initialize(this, surface_manager_);
+
   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();
 }
 
 void DirectCompositorFrameSink::SubmitCompositorFrame(CompositorFrame frame) {
   gfx::Size frame_size = frame.render_pass_list.back()->output_rect.size();
   if (frame_size.IsEmpty() || frame_size != last_swap_frame_size_) {
     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() {
   is_lost_ = true;
   client_->DidLoseCompositorFrameSink();
 }
 
 void DirectCompositorFrameSink::DisplayWillDrawAndSwap(
     bool will_draw_and_swap,
     const RenderPassList& render_passes) {
   // This notification is not relevant to our client outside of tests.
 }
 
 void DirectCompositorFrameSink::DisplayDidDrawAndSwap() {
   // This notification is not relevant to our client outside of tests. We
   // unblock the client from DidDrawCallback() when the surface is going to
   // be drawn.
 }
 
+void DirectCompositorFrameSink::DidReceiveCompositorFrameAck() {
+  client_->DidReceiveCompositorFrameAck();
+}
+
+void DirectCompositorFrameSink::OnBeginFrame(const BeginFrameArgs& args) {
+  begin_frame_source_->OnBeginFrame(args);
+}
+
+void DirectCompositorFrameSink::ReclaimResources(
+    const ReturnedResourceArray& resources) {
+  client_->ReclaimResources(resources);
+}
+
+void DirectCompositorFrameSink::WillDrawSurface() {
+  // TODO(staraz): Implement this.
+}
+
 void DirectCompositorFrameSink::DidDrawCallback() {
   client_->DidReceiveCompositorFrameAck();
 }
 
+void DirectCompositorFrameSink::OnNeedsBeginFrames(bool needs_begin_frame) {
+  needs_begin_frame_ = needs_begin_frame;
+  if (support_)
+    support_->SetNeedsBeginFrame(needs_begin_frame);
+}
+
 }  // namespace cc