Convert mustash use surface references.

services/ui/ws/frame_generator.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 "services/ui/ws/frame_generator.h"
 
 #include "base/containers/adapters.h"
 #include "cc/output/compositor_frame.h"
 #include "cc/quads/render_pass.h"
 #include "cc/quads/render_pass_draw_quad.h"
 #include "cc/quads/shared_quad_state.h"
 #include "cc/quads/surface_draw_quad.h"
-#include "cc/surfaces/surface_id.h"
 #include "gpu/ipc/client/gpu_channel_host.h"
 #include "services/ui/surfaces/display_compositor.h"
 #include "services/ui/surfaces/surfaces_context_provider.h"
 #include "services/ui/ws/frame_generator_delegate.h"
 #include "services/ui/ws/server_window.h"
 #include "services/ui/ws/server_window_compositor_frame_sink_manager.h"
 #include "services/ui/ws/server_window_delegate.h"
 
 namespace ui {
 
 namespace ws {
+namespace {
+
+// Walks up window tree and finds the SurfaceId for the ServerWindow embedding
+// |window|. If |window| is a display root, then return |root_surface_id|.
+cc::SurfaceId FindParentSurfaceId(ServerWindow* window,
+                                  const cc::SurfaceId& root_surface_id) {
+  if (!window->parent())
+    return root_surface_id;
+
+  ServerWindow* current = window;
+  while (current->parent()) {

[Fady Samuel, 2016/11/18 03:46:06]

I'm worried about the correctness of this in general. Today, the display root is
the parent of all surfaces right? Can we just make the code explicit here?

[kylechar, 2016/11/18 17:24:11]

Done.

+    current = current->parent();
+    if (current->compositor_frame_sink_manager() &&
+        current->compositor_frame_sink_manager()->HasCompositorFrameSinkOfType(
+            mojom::CompositorFrameSinkType::DEFAULT)) {
+      return current->compositor_frame_sink_manager()->GetLatestSurfaceId(
+          mojom::CompositorFrameSinkType::DEFAULT);
+    }
+  }
+
+  NOTREACHED();
+  return cc::SurfaceId();
+}
+
+}  // namespace
 
 FrameGenerator::FrameGenerator(FrameGeneratorDelegate* delegate,
                                ServerWindow* root_window)
     : delegate_(delegate),
-      frame_sink_id_(
-          WindowIdToTransportId(root_window->id()),
-          static_cast<uint32_t>(mojom::CompositorFrameSinkType::DEFAULT)),
       root_window_(root_window),
+      root_surface_id_(cc::FrameSinkId(),

[Fady Samuel, 2016/11/18 03:46:06]

Why isn't the root window's FrameSinkId part of the root_surface_id_?

[kylechar, 2016/11/18 17:24:11]

This is different from the display root. I changed the name to make that
clearer.

+                       cc::LocalFrameId(0, base::UnguessableToken::Create())),
       binding_(this),
       weak_factory_(this) {
   DCHECK(delegate_);
-  surface_sequence_generator_.set_frame_sink_id(frame_sink_id_);
 }
 
 FrameGenerator::~FrameGenerator() {
-  ReleaseAllSurfaceReferences();
+  RemoveDeadSurfaceReferences();
+  RemoveAllSurfaceReferences();
   // Invalidate WeakPtrs now to avoid callbacks back into the
   // FrameGenerator during destruction of |compositor_frame_sink_|.
   weak_factory_.InvalidateWeakPtrs();
   compositor_frame_sink_.reset();
 }
 
 void FrameGenerator::OnGpuChannelEstablished(
     scoped_refptr<gpu::GpuChannelHost> channel) {
   if (widget_ != gfx::kNullAcceleratedWidget) {
     cc::mojom::MojoCompositorFrameSinkRequest request =
@@ skipping 28 matching lines @@
         new SurfacesContextProvider(widget_, std::move(gpu_channel_)),
         std::move(request), binding_.CreateInterfacePtrAndBind());
     // TODO(fsamuel): This means we're always requesting a new BeginFrame signal
     // even when we don't need it. Once surface ID propagation work is done,
     // this will not be necessary because FrameGenerator will only need a
     // BeginFrame if the window manager changes.
     compositor_frame_sink_->SetNeedsBeginFrame(true);
   }
 }
 
+void FrameGenerator::OnSurfaceCreated(const cc::SurfaceId& surface_id,
+                                      ServerWindow* window) {
+  DCHECK(surface_id.is_valid());
+
+  // TODO(kylechar): Adding surface references should be synchronized with
+  // SubmitCompositorFrame().
+
+  auto iter = active_references_.find(surface_id.frame_sink_id());
+  if (iter == active_references_.end()) {
+    AddFirstReference(surface_id, window);
+    return;
+  }
+
+  SurfaceReference& ref = iter->second;
+  DCHECK(surface_id.frame_sink_id() == ref.child_id.frame_sink_id());
+
+  // This shouldn't be called multiple times for the same SurfaceId.
+  DCHECK(surface_id.local_frame_id() != ref.child_id.local_frame_id());
+
+  // Add a reference from parent to new surface first.
+  GetDisplayCompositor()->AddSurfaceReference(ref.parent_id, surface_id);
+
+  // If the display root surface has changed, update all references to embedded
+  // surfaces.
+  if (!window->parent())
+    AddNewParentReferences(ref.child_id, surface_id);

[Fady Samuel, 2016/11/18 03:46:06]

I'm confused. Aren't we supposed to remove parent references first?

[kylechar, 2016/11/18 17:24:11]

If something else is referencing the old parent, then we don't want to remove
references from the old parent to children because then GC would children when
they're still being used. (I'm not sure that case ever happens for
FrameGenerator but will for other clients). We do remove our reference to the
old parent, which automatically removes the references it holds (assuming
nothing else is referencing it).

This is just adding references from the new parent to the child surfaces, so
that if nothing else is referencing the old parent and it gets GCd, the children
don't get GCd too.

+
+  // Move the existing reference to list of references to remove after we submit
+  // the next CompositorFrame. Update local reference cache to be the new
+  // reference. If this is the display root surface then removing this reference
+  // will recursively remove any references it held.
+  dead_references_.push_back(ref);
+  ref.child_id = surface_id;
+}
+
 void FrameGenerator::DidReceiveCompositorFrameAck() {}
 
 void FrameGenerator::OnBeginFrame(const cc::BeginFrameArgs& begin_frame_arags) {
   if (!root_window_->visible())
     return;
 
   // TODO(fsamuel): We should add a trace for generating a top level frame.
   cc::CompositorFrame frame(GenerateCompositorFrame(root_window_->bounds()));
-  if (compositor_frame_sink_)
+  if (compositor_frame_sink_) {
     compositor_frame_sink_->SubmitCompositorFrame(std::move(frame));
+
+    // Remove dead references after we submit a frame. This has to happen after
+    // the frame is submitted otherwise we could end up deleting a surface that
+    // is still embedded in the last frame.
+    // TODO(kylechar): This should be synchronized with SubmitCompositorFrame().
+    RemoveDeadSurfaceReferences();
+  }
 }
 
 void FrameGenerator::ReclaimResources(
     const cc::ReturnedResourceArray& resources) {
   // Nothing to do here because FrameGenerator CompositorFrames don't reference
   // any resources.
 }
 
 cc::CompositorFrame FrameGenerator::GenerateCompositorFrame(
     const gfx::Rect& output_rect) {
@@ skipping 73 matching lines @@
     const gfx::Rect bounds_at_origin(window->bounds().size());
     // TODO(fsamuel): These clipping and visible rects are incorrect. They need
     // to be populated from CompositorFrame structs.
     sqs->SetAll(quad_to_target_transform,
                 bounds_at_origin.size() /* layer_bounds */,
                 bounds_at_origin /* visible_layer_bounds */,
                 bounds_at_origin /* clip_rect */, false /* is_clipped */,
                 combined_opacity, SkXfermode::kSrcOver_Mode,
                 0 /* sorting-context_id */);
     auto* quad = pass->CreateAndAppendDrawQuad<cc::SurfaceDrawQuad>();
-    AddOrUpdateSurfaceReference(mojom::CompositorFrameSinkType::DEFAULT,
-                                window);
     quad->SetAll(sqs, bounds_at_origin /* rect */,
                  gfx::Rect() /* opaque_rect */,
                  bounds_at_origin /* visible_rect */, true /* needs_blending*/,
                  default_surface_id);
   }
   if (underlay_surface_id.is_valid()) {
     const gfx::Rect underlay_absolute_bounds =
         absolute_bounds - window->underlay_offset();
     gfx::Transform quad_to_target_transform;
     quad_to_target_transform.Translate(underlay_absolute_bounds.x(),
                                        underlay_absolute_bounds.y());
     cc::SharedQuadState* sqs = pass->CreateAndAppendSharedQuadState();
     const gfx::Rect bounds_at_origin(
         window->compositor_frame_sink_manager()->GetLatestFrameSize(
             mojom::CompositorFrameSinkType::UNDERLAY));
     sqs->SetAll(quad_to_target_transform,
                 bounds_at_origin.size() /* layer_bounds */,
                 bounds_at_origin /* visible_layer_bounds */,
                 bounds_at_origin /* clip_rect */, false /* is_clipped */,
                 combined_opacity, SkXfermode::kSrcOver_Mode,
                 0 /* sorting-context_id */);
 
     auto* quad = pass->CreateAndAppendDrawQuad<cc::SurfaceDrawQuad>();
-    AddOrUpdateSurfaceReference(mojom::CompositorFrameSinkType::UNDERLAY,
-                                window);
     quad->SetAll(sqs, bounds_at_origin /* rect */,
                  gfx::Rect() /* opaque_rect */,
                  bounds_at_origin /* visible_rect */, true /* needs_blending*/,
                  underlay_surface_id);
   }
 }
 
-void FrameGenerator::AddOrUpdateSurfaceReference(
-    mojom::CompositorFrameSinkType type,
-    ServerWindow* window) {
-  cc::SurfaceId surface_id =
-      window->compositor_frame_sink_manager()->GetLatestSurfaceId(type);
-  if (!surface_id.is_valid())
-    return;
-  auto it = dependencies_.find(surface_id.frame_sink_id());
-  if (it == dependencies_.end()) {
-    SurfaceDependency dependency = {
-        surface_id.local_frame_id(),
-        surface_sequence_generator_.CreateSurfaceSequence()};
-    dependencies_[surface_id.frame_sink_id()] = dependency;
-    GetDisplayCompositor()->AddSurfaceReference(surface_id,
-                                                dependency.sequence);
-    // Observe |window_surface|'s window so that we can release references when
-    // the window is destroyed.
-    Add(window);
-    return;
+void FrameGenerator::AddSurfaceReference(const cc::SurfaceId& parent_id,
+                                         const cc::SurfaceId& child_id) {
+  if (parent_id == root_surface_id_)
+    GetDisplayCompositor()->AddRootSurfaceReference(child_id);
+  else
+    GetDisplayCompositor()->AddSurfaceReference(parent_id, child_id);
+
+  // Add new reference from parent to surface, plus add reference to local
+  // cache.
+  active_references_[child_id.frame_sink_id()] =
+      SurfaceReference({parent_id, child_id});
+}
+
+void FrameGenerator::AddFirstReference(const cc::SurfaceId& surface_id,
+                                       ServerWindow* window) {
+  cc::SurfaceId parent_id = FindParentSurfaceId(window, root_surface_id_);
+
+  if (parent_id == root_surface_id_ || parent_id.is_valid()) {
+    AddSurfaceReference(parent_id, surface_id);
+
+    // For the first display root surface, add references to any child surfaces
+    // that were created before it.
+    if (parent_id == root_surface_id_) {
+      for (auto& child_surface_id : waiting_for_references_)
+        AddSurfaceReference(surface_id, child_surface_id);
+      waiting_for_references_.clear();
+    }
+  } else {
+    // This isn't the display root surface and display root surface hasn't
+    // submitted a CF yet. We can't add a reference to an unknown SurfaceId.
+    waiting_for_references_.push_back(surface_id);
   }
 
-  // We are already holding a reference to this surface so there's no work to do
-  // here.
-  if (surface_id.local_frame_id() == it->second.local_frame_id)
+  // Observe |window| so that we can remove references when it's destroyed.
+  Add(window);
+}
+
+void FrameGenerator::AddNewParentReferences(
+    const cc::SurfaceId& old_surface_id,
+    const cc::SurfaceId& new_surface_id) {
+  DCHECK(old_surface_id.frame_sink_id() == new_surface_id.frame_sink_id());
+
+  DisplayCompositor* display_compositor = GetDisplayCompositor();
+  for (auto& map_entry : active_references_) {
+    SurfaceReference& ref = map_entry.second;
+    if (ref.parent_id == old_surface_id) {
+      display_compositor->AddSurfaceReference(new_surface_id, ref.child_id);
+      ref.parent_id = new_surface_id;
+    }
+  }
+}
+
+void FrameGenerator::RemoveDeadSurfaceReferences() {
+  if (dead_references_.empty())
     return;
 
-  // If we have have an existing reference to a surface from the given
-  // FrameSink, then we should release the reference, and then add this new
-  // reference. This results in a delete and lookup in the map but simplifies
-  // the code.
-  ReleaseFrameSinkReference(surface_id.frame_sink_id());
-
-  // This recursion will always terminate. This line is being called because
-  // there was a stale surface reference. The stale reference has been released
-  // in the previous line and cleared from the dependencies_ map. Thus, in the
-  // recursive call, we'll enter the second if blcok because the FrameSinkId
-  // is no longer referenced in the map.
-  AddOrUpdateSurfaceReference(type, window);
+  DisplayCompositor* display_compositor = GetDisplayCompositor();
+  for (auto& ref : dead_references_) {
+    if (ref.parent_id == root_surface_id_)
+      display_compositor->RemoveRootSurfaceReference(ref.child_id);

[Fady Samuel, 2016/11/18 03:46:06]

That's a lot of potential IPCs in the future. I guess it would be nice to bundle
these into a vector or set or something. It would nice to have an API:

display_compositor->RemoveSurfaceReferences(std::move(dead_references));

[kylechar, 2016/11/18 17:24:11]

Added TODO.

+    else
+      display_compositor->RemoveSurfaceReference(ref.parent_id, ref.child_id);
+  }
+  dead_references_.clear();
 }
 
-void FrameGenerator::ReleaseFrameSinkReference(
+void FrameGenerator::RemoveFrameSinkReference(
     const cc::FrameSinkId& frame_sink_id) {
-  auto it = dependencies_.find(frame_sink_id);
-  if (it == dependencies_.end())
+  auto it = active_references_.find(frame_sink_id);
+  if (it == active_references_.end())
     return;
-  std::vector<uint32_t> sequences;
-  sequences.push_back(it->second.sequence.sequence);
-  GetDisplayCompositor()->ReturnSurfaceReferences(frame_sink_id, sequences);
-  dependencies_.erase(it);
+  dead_references_.push_back(it->second);
+  active_references_.erase(it);
 }
 
-void FrameGenerator::ReleaseAllSurfaceReferences() {
-  std::vector<uint32_t> sequences;
-  for (auto& dependency : dependencies_)
-    sequences.push_back(dependency.second.sequence.sequence);
-  GetDisplayCompositor()->ReturnSurfaceReferences(frame_sink_id_, sequences);
-  dependencies_.clear();
+void FrameGenerator::RemoveAllSurfaceReferences() {
+  DisplayCompositor* display_compositor = GetDisplayCompositor();
+  for (auto& map_entry : active_references_) {
+    const SurfaceReference& ref = map_entry.second;
+    display_compositor->RemoveSurfaceReference(ref.parent_id, ref.child_id);
+  }
+  active_references_.clear();
 }
 
 ui::DisplayCompositor* FrameGenerator::GetDisplayCompositor() {
   return root_window_->delegate()->GetDisplayCompositor();
 }
 
 void FrameGenerator::OnWindowDestroying(ServerWindow* window) {
   Remove(window);
   ServerWindowCompositorFrameSinkManager* compositor_frame_sink_manager =
       window->compositor_frame_sink_manager();
   // If FrameGenerator was observing |window|, then that means it had a
   // CompositorFrame at some point in time and should have a
   // ServerWindowCompositorFrameSinkManager.
   DCHECK(compositor_frame_sink_manager);
 
   cc::SurfaceId default_surface_id =
       window->compositor_frame_sink_manager()->GetLatestSurfaceId(
           mojom::CompositorFrameSinkType::DEFAULT);
   if (default_surface_id.is_valid())
-    ReleaseFrameSinkReference(default_surface_id.frame_sink_id());
+    RemoveFrameSinkReference(default_surface_id.frame_sink_id());
 
   cc::SurfaceId underlay_surface_id =
       window->compositor_frame_sink_manager()->GetLatestSurfaceId(
           mojom::CompositorFrameSinkType::UNDERLAY);
   if (underlay_surface_id.is_valid())
-    ReleaseFrameSinkReference(underlay_surface_id.frame_sink_id());
+    RemoveFrameSinkReference(underlay_surface_id.frame_sink_id());
 }
 
 }  // namespace ws
 
 }  // namespace ui