cc: Move ownership of surfaces to SurfaceManager

cc/surfaces/compositor_frame_sink_support.cc

 // Copyright 2016 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/compositor_frame_sink_support.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "cc/output/compositor_frame.h"
@@ skipping 59 matching lines @@
     BeginFrameSource* begin_frame_source) {
   if (begin_frame_source_ && added_frame_observer_) {
     begin_frame_source_->RemoveObserver(this);
     added_frame_observer_ = false;
   }
   begin_frame_source_ = begin_frame_source;
   UpdateNeedsBeginFramesInternal();
 }
 
 void CompositorFrameSinkSupport::EvictCurrentSurface() {
-  if (!current_surface_)
+  Surface* current_surface = GetCurrentSurface();

[danakj, 2017/06/21 21:24:09]

It feels like in general this class is dealing with a Surface* when it should be
dealing with surface ids, cuz it is using the Surface* basically to get an ID
from it and to test if the surface exists.

If you had SurfaceManager::SurfaceExists(SurfaceId) then this class wouldn't
need a pointer at all except for RequestCopyOfOutput and QueueFrame?

[Saman Sami, 2017/06/26 20:44:05]

I just keep the current SurfaceId and reset it when we're destroying the
surface.

+  if (!current_surface)
     return;
-  DestroyCurrentSurface();
+  surface_manager_->DestroySurface(current_surface->surface_id());

[danakj, 2017/06/21 21:24:09]

This could take a surface id and just ignore ids that don't exist.

[Saman Sami, 2017/06/26 20:44:05]

I'll just check current_surface_id_ is valid.

 }
 
 void CompositorFrameSinkSupport::SetNeedsBeginFrame(bool needs_begin_frame) {
   needs_begin_frame_ = needs_begin_frame;
   UpdateNeedsBeginFramesInternal();
 }
 
 void CompositorFrameSinkSupport::DidNotProduceFrame(const BeginFrameAck& ack) {
   TRACE_EVENT2("cc", "CompositorFrameSinkSupport::DidNotProduceFrame",
                "ack.source_id", ack.source_id, "ack.sequence_number",
                ack.sequence_number);
   DCHECK_GE(ack.sequence_number, BeginFrameArgs::kStartingFrameNumber);
 
   // |has_damage| is not transmitted, but false by default.
   DCHECK(!ack.has_damage);
 
-  if (current_surface_)
-    surface_manager_->SurfaceModified(current_surface_->surface_id(), ack);
+  Surface* current_surface = GetCurrentSurface();
+  if (current_surface)
+    surface_manager_->SurfaceModified(current_surface->surface_id(), ack);

[danakj, 2017/06/21 21:24:09]

This could also early out if the surface didn't exist. The thing is I don't also
get why this is checking with SurfaceManager if the surface exists at all. Would
the CompositorFrameSinkSupport not have a null current_surface_local_id_ and it
could just check that?

[Saman Sami, 2017/06/26 20:44:05]

I'll just check current_surface_id_ is valid.

+
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this);
 }
 
 bool CompositorFrameSinkSupport::SubmitCompositorFrame(
     const LocalSurfaceId& local_surface_id,
     CompositorFrame frame) {
   TRACE_EVENT0("cc", "CompositorFrameSinkSupport::SubmitCompositorFrame");
   DCHECK(local_surface_id.is_valid());
   DCHECK(!frame.render_pass_list.empty());
 
   ++ack_pending_count_;
 
   // |has_damage| is not transmitted.
   frame.metadata.begin_frame_ack.has_damage = true;
   BeginFrameAck ack = frame.metadata.begin_frame_ack;
   DCHECK_LE(BeginFrameArgs::kStartingFrameNumber, ack.sequence_number);
 
   if (!ui::LatencyInfo::Verify(frame.metadata.latency_info,
                                "RenderWidgetHostImpl::OnSwapCompositorFrame")) {
     std::vector<ui::LatencyInfo>().swap(frame.metadata.latency_info);
   }
   for (ui::LatencyInfo& latency : frame.metadata.latency_info) {
     if (latency.latency_components().size() > 0) {
       latency.AddLatencyNumber(ui::DISPLAY_COMPOSITOR_RECEIVED_FRAME_COMPONENT,
                                0, 0);
     }
   }
 
-  std::unique_ptr<Surface> surface;
-  bool create_new_surface =
-      (!current_surface_ ||
-       local_surface_id != current_surface_->surface_id().local_surface_id());
-  if (!create_new_surface) {
-    surface = std::move(current_surface_);
-  } else {
+  Surface* current_surface = GetCurrentSurface();
+  Surface* prev_surface = current_surface;
+  if (!current_surface ||
+      surface_manager()->IsMarkedForDestruction(
+          current_surface->surface_id()) ||
+      local_surface_id != current_local_surface_id_) {
     SurfaceId surface_id(frame_sink_id_, local_surface_id);
     gfx::Size frame_size = frame.render_pass_list.back()->output_rect.size();
     float device_scale_factor = frame.metadata.device_scale_factor;
     SurfaceInfo surface_info(surface_id, device_scale_factor, frame_size);
 
     if (!surface_info.is_valid()) {
       TRACE_EVENT_INSTANT0("cc", "Invalid SurfaceInfo",
                            TRACE_EVENT_SCOPE_THREAD);
-      if (current_surface_)
-        DestroyCurrentSurface();
+      if (current_surface)
+        EvictCurrentSurface();
       ReturnedResourceArray resources;
       TransferableResource::ReturnResources(frame.resource_list, &resources);
       ReturnResources(resources);
       DidReceiveCompositorFrameAck();
       return true;
     }
 
-    surface = CreateSurface(surface_info);
-    surface_manager_->SurfaceDamageExpected(surface->surface_id(),
+    current_surface = CreateSurface(surface_info);
+    current_local_surface_id_ = local_surface_id;
+    surface_manager_->SurfaceDamageExpected(current_surface->surface_id(),
                                             last_begin_frame_args_);
   }
 
-  bool result = surface->QueueFrame(
+  bool result = current_surface->QueueFrame(
       std::move(frame),
       base::Bind(&CompositorFrameSinkSupport::DidReceiveCompositorFrameAck,
                  weak_factory_.GetWeakPtr()),
       base::BindRepeating(&CompositorFrameSinkSupport::WillDrawSurface,
                           weak_factory_.GetWeakPtr()));
 
   if (!result) {
-    surface_manager_->DestroySurface(std::move(surface));
+    EvictCurrentSurface();
     return false;
   }
 
-  if (current_surface_) {
-    surface->SetPreviousFrameSurface(current_surface_.get());
-    DestroyCurrentSurface();
+  if (prev_surface && prev_surface != current_surface) {
+    current_surface->SetPreviousFrameSurface(prev_surface);
+    surface_manager_->DestroySurface(prev_surface->surface_id());
   }
-  current_surface_ = std::move(surface);
 
   if (begin_frame_source_)
     begin_frame_source_->DidFinishFrame(this);
 
   return true;
 }
 
 void CompositorFrameSinkSupport::UpdateSurfaceReferences(
     const LocalSurfaceId& local_surface_id,
     const std::vector<SurfaceId>& active_referenced_surfaces) {
@@ skipping 95 matching lines @@
 
 void CompositorFrameSinkSupport::Init(SurfaceManager* surface_manager) {
   surface_manager_ = surface_manager;
   if (handles_frame_sink_id_invalidation_)
     surface_manager_->RegisterFrameSinkId(frame_sink_id_);
   surface_manager_->RegisterFrameSinkManagerClient(frame_sink_id_, this);
 }
 
 void CompositorFrameSinkSupport::OnBeginFrame(const BeginFrameArgs& args) {
   UpdateNeedsBeginFramesInternal();
-  if (current_surface_) {
-    surface_manager_->SurfaceDamageExpected(current_surface_->surface_id(),
+  Surface* current_surface = GetCurrentSurface();
+  if (current_surface)
+    surface_manager_->SurfaceDamageExpected(current_surface->surface_id(),
                                             args);
-  }
   last_begin_frame_args_ = args;
   if (client_)
     client_->OnBeginFrame(args);
 }
 
 const BeginFrameArgs& CompositorFrameSinkSupport::LastUsedBeginFrameArgs()
     const {
   return last_begin_frame_args_;
 }
 
@@ skipping 30 matching lines @@
   if (needs_begin_frame_ == added_frame_observer_)
     return;
 
   added_frame_observer_ = needs_begin_frame_;
   if (needs_begin_frame_)
     begin_frame_source_->AddObserver(this);
   else
     begin_frame_source_->RemoveObserver(this);
 }
 
-std::unique_ptr<Surface> CompositorFrameSinkSupport::CreateSurface(
+Surface* CompositorFrameSinkSupport::CreateSurface(
     const SurfaceInfo& surface_info) {
   seen_first_frame_activation_ = false;
   return surface_manager_->CreateSurface(weak_factory_.GetWeakPtr(),
                                          surface_info);
 }
 
-void CompositorFrameSinkSupport::DestroyCurrentSurface() {
-  surface_manager_->DestroySurface(std::move(current_surface_));
+void CompositorFrameSinkSupport::RequestCopyOfSurface(
+    std::unique_ptr<CopyOutputRequest> copy_request) {
+  Surface* current_surface = GetCurrentSurface();
+  if (!current_surface)
+    return;
+  DCHECK(current_surface->compositor_frame_sink_support().get() == this);
+  current_surface->RequestCopyOfOutput(std::move(copy_request));
+  BeginFrameAck ack;
+  ack.has_damage = true;
+  if (current_surface->HasActiveFrame())
+    surface_manager_->SurfaceModified(current_surface->surface_id(), ack);
 }
 
-void CompositorFrameSinkSupport::RequestCopyOfSurface(
-    std::unique_ptr<CopyOutputRequest> copy_request) {
-  if (!current_surface_)
-    return;
-
-  DCHECK(current_surface_->compositor_frame_sink_support().get() == this);
-  current_surface_->RequestCopyOfOutput(std::move(copy_request));
-  BeginFrameAck ack;
-  ack.has_damage = true;
-  if (current_surface_->HasActiveFrame())
-    surface_manager_->SurfaceModified(current_surface_->surface_id(), ack);
+Surface* CompositorFrameSinkSupport::GetCurrentSurface() {
+  return surface_manager_->GetSurfaceForId(
+      SurfaceId(frame_sink_id_, current_local_surface_id_));
 }
 
 }  // namespace cc