Make SurfaceFactory lifetime match frame_sink_id_ registration in BlimpCompositor

blimp/client/core/compositor/blimp_compositor.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 "blimp/client/core/compositor/blimp_compositor.h"
 
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 131 matching lines @@
       layer_(cc::Layer::Create()),
       remote_proto_channel_receiver_(nullptr),
       outstanding_commits_(0U),
       weak_ptr_factory_(this) {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 void BlimpCompositor::Initialize() {
   surface_id_allocator_ = base::MakeUnique<cc::SurfaceIdAllocator>();
   GetEmbedderDeps()->GetSurfaceManager()->RegisterFrameSinkId(frame_sink_id_);
+  surface_factory_ = base::MakeUnique<cc::SurfaceFactory>(
+      frame_sink_id_, GetEmbedderDeps()->GetSurfaceManager(), this);
   host_ = CreateLayerTreeHost();
 
   if (use_threaded_layer_tree_host_) {
     std::unique_ptr<cc::ClientPictureCache> client_picture_cache =
         compositor_dependencies_->GetImageSerializationProcessor()
             ->CreateClientPictureCache();
     compositor_state_deserializer_ =
         base::MakeUnique<cc::CompositorStateDeserializer>(
             host_.get(), std::move(client_picture_cache), this);
   }
@@ skipping 15 matching lines @@
 }
 
 bool BlimpCompositor::HasPendingFrameUpdateFromEngine() const {
   return pending_frame_update_.get() != nullptr;
 }
 
 void BlimpCompositor::RequestCopyOfOutput(
     std::unique_ptr<cc::CopyOutputRequest> copy_request,
     bool flush_pending_update) {
   // If we don't have a FrameSink, fail right away.
-  if (!surface_factory_)
+  if (!proxy_client_)

[piman, 2016/11/07 22:31:38]

Here and other places: even though proxy_client_ is a WeakPtr, it looks to have
exactly the same lifetime as what surface_factory_ used to have. It is set to a
!null value in BlimpCompositor::BindToProxyClient (which is called from the
ProxyClient), and reset to null in BlimpCompositor::UnbindProxyClient (which is
also called from the ProxyClient, i.e. it hasn't been destroyed yet). So I think
this transformation is valid, and preferable to add another boolean state
("bound to proxy"), but LMK if I missed something.

[Khushal, 2016/11/08 00:17:22]

Won't be able to de-reference the weak ptr here since its bound to the
compositor thread. For this place, I think if we checked if the local_frame_id_
is null, that should be good enough to early out.

For the other places it is being used only for DCHECKs for calls that should be
made only when a frame sink is bound. I think adding a bool to do this should be
fine and may be more explicit. Wdyt?

     return;
 
   if (!use_threaded_layer_tree_host_) {
     RequestCopyOfOutputDeprecated(std::move(copy_request));
     return;
   }
 
   if (flush_pending_update) {
     // Always request a commit when queuing the promise to make sure that any
     // frames pending draws are cleared from the pipeline.
@@ skipping 50 matching lines @@
     const gfx::Vector2dF& elastic_overscroll_delta,
     float page_scale,
     float top_controls_delta) {
   DCHECK(use_threaded_layer_tree_host_);
   compositor_state_deserializer_->ApplyViewportDeltas(
       inner_delta, outer_delta, elastic_overscroll_delta, page_scale,
       top_controls_delta);
 }
 
 void BlimpCompositor::RequestNewCompositorFrameSink() {
-  DCHECK(!surface_factory_);
+  DCHECK(!proxy_client_);
   DCHECK(!compositor_frame_sink_request_pending_);
 
   compositor_frame_sink_request_pending_ = true;
   GetEmbedderDeps()->GetContextProviders(
       base::Bind(&BlimpCompositor::OnContextProvidersCreated,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BlimpCompositor::DidInitializeCompositorFrameSink() {
   compositor_frame_sink_request_pending_ = false;
 }
 
 void BlimpCompositor::DidCommitAndDrawFrame() {
   if (use_threaded_layer_tree_host_)
     return;
 
   DCHECK_GT(outstanding_commits_, 0U);
   outstanding_commits_--;
 
   for (auto it = pending_commit_trackers_.begin();
        it != pending_commit_trackers_.end();) {
     if (--it->first == 0) {
-      if (surface_factory_)
+      if (proxy_client_) {
         surface_factory_->RequestCopyOfSurface(local_frame_id_,
                                                std::move(it->second));
+      }
       it = pending_commit_trackers_.erase(it);
     } else {
       ++it;
     }
   }
 }
 
 void BlimpCompositor::SetProtoReceiver(ProtoReceiver* receiver) {
   DCHECK(!use_threaded_layer_tree_host_);
   remote_proto_channel_receiver_ = receiver;
@@ skipping 70 matching lines @@
 }
 
 // Returns a reference to the InputHandler owned by layer tree host.
 const base::WeakPtr<cc::InputHandler>& BlimpCompositor::GetInputHandler() {
   return host_->GetInputHandler();
 }
 
 void BlimpCompositor::OnContextProvidersCreated(
     const scoped_refptr<cc::ContextProvider>& compositor_context_provider,
     const scoped_refptr<cc::ContextProvider>& worker_context_provider) {
-  DCHECK(!surface_factory_) << "Any connection to the old CompositorFrameSink "
-                               "should have been destroyed";
+  DCHECK(!proxy_client_) << "Any connection to the old CompositorFrameSink "
+                            "should have been destroyed";
 
   // Make sure we still have a host and we're still expecting a
   // CompositorFrameSink. This can happen if the host dies while the request is
   // outstanding and we build a new one that hasn't asked for a surface yet.
   if (!compositor_frame_sink_request_pending_)
     return;
 
   // Try again if the context creation failed.
   if (!compositor_context_provider) {
     GetEmbedderDeps()->GetContextProviders(
         base::Bind(&BlimpCompositor::OnContextProvidersCreated,
                    weak_ptr_factory_.GetWeakPtr()));
     return;
   }
 
   auto compositor_frame_sink = base::MakeUnique<BlimpCompositorFrameSink>(
       std::move(compositor_context_provider),
       std::move(worker_context_provider),
       GetEmbedderDeps()->GetGpuMemoryBufferManager(), nullptr,
       base::ThreadTaskRunnerHandle::Get(), weak_ptr_factory_.GetWeakPtr());
 
   host_->SetCompositorFrameSink(std::move(compositor_frame_sink));
 }
 
 void BlimpCompositor::BindToProxyClient(
     base::WeakPtr<BlimpCompositorFrameSinkProxyClient> proxy_client) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!surface_factory_);
+  DCHECK(!proxy_client_);
 
   proxy_client_ = proxy_client;
-  surface_factory_ = base::MakeUnique<cc::SurfaceFactory>(
-      frame_sink_id_, GetEmbedderDeps()->GetSurfaceManager(), this);
 }
 
 void BlimpCompositor::SubmitCompositorFrame(cc::CompositorFrame frame) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
 
   cc::RenderPass* root_pass = frame.render_pass_list.back().get();
   gfx::Size surface_size = root_pass->output_rect.size();
 
   if (local_frame_id_.is_null() || current_surface_size_ != surface_size) {
     DestroyDelegatedContent();
     DCHECK(layer_->children().empty());
 
     local_frame_id_ = surface_id_allocator_->GenerateId();
     surface_factory_->Create(local_frame_id_);
@@ skipping 21 matching lines @@
                  weak_ptr_factory_.GetWeakPtr()));
 
   for (auto& copy_request : copy_requests_for_next_swap_) {
     surface_factory_->RequestCopyOfSurface(local_frame_id_,
                                            std::move(copy_request));
   }
   copy_requests_for_next_swap_.clear();
 }
 
 void BlimpCompositor::SubmitCompositorFrameAck() {
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
   compositor_dependencies_->GetCompositorTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(&BlimpCompositorFrameSinkProxyClient::SubmitCompositorFrameAck,
                  proxy_client_));
 }
 
 void BlimpCompositor::MakeCopyRequestOnNextSwap(
     std::unique_ptr<cc::CopyOutputRequest> copy_request) {
   copy_requests_for_next_swap_.push_back(std::move(copy_request));
 }
 
 void BlimpCompositor::UnbindProxyClient() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
 
   DestroyDelegatedContent();
-  surface_factory_.reset();
+  surface_factory_->Reset();
   proxy_client_ = nullptr;
 }
 
 void BlimpCompositor::ReturnResources(
     const cc::ReturnedResourceArray& resources) {
-  DCHECK(surface_factory_);
+  DCHECK(proxy_client_);
   compositor_dependencies_->GetCompositorTaskRunner()->PostTask(
       FROM_HERE,
       base::Bind(
           &BlimpCompositorFrameSinkProxyClient::ReclaimCompositorResources,
           proxy_client_, resources));
 }
 
 void BlimpCompositor::DidUpdateLocalState() {
   client_state_dirty_ = true;
 }
@@ skipping 65 matching lines @@
 
   return host;
 }
 
 void BlimpCompositor::DestroyLayerTreeHost() {
   DCHECK(host_);
 
   // Tear down the output surface connection with the old LayerTreeHost
   // instance.
   DestroyDelegatedContent();
-  surface_factory_.reset();

[piman, 2016/11/07 22:31:38]

I believe there's no need to do an explicit Reset here, because we reset the
host below, which synchronously unbinds and destroys the CompositorFrameSink
(which is the ProxyClient, and so, will call UnbindProxyClient which will do the
Reset). Either way, this is only called from the destructor, so it's probably
not necessary anyway.

[Khushal, 2016/11/08 00:17:22]

Makes sense. Thanks for the clean up!

 
   // Destroy the old LayerTreeHost state.
   host_.reset();
 
   // Cancel any outstanding CompositorFrameSink requests.  That way if we get an
   // async callback related to the old request we know to drop it.
   compositor_frame_sink_request_pending_ = false;
 
   // Make sure we don't have a receiver at this point.
   DCHECK(!remote_proto_channel_receiver_);
 }
 
 }  // namespace client
 }  // namespace blimp