Convert mustash use surface references.

services/ui/ws/frame_generator.cc

Index: services/ui/ws/frame_generator.cc
diff --git a/services/ui/ws/frame_generator.cc b/services/ui/ws/frame_generator.cc
index 5fee960f0670265361fc9d33a07dddba10c16f31..5598def5c02d1558ca469d898c663cf4351a4dc3 100644
--- a/services/ui/ws/frame_generator.cc
+++ b/services/ui/ws/frame_generator.cc
@@ -10,7 +10,6 @@
 #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"
@@ -26,18 +25,17 @@ namespace ws {
 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),
+      top_level_root_(cc::FrameSinkId(0, 0),
+                      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();
@@ -86,6 +84,41 @@ void FrameGenerator::OnAcceleratedWidgetAvailable(
   }
 }
 
+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());

[sky, 2016/11/19 16:38:56]

DCHECK_EQ and NE?

[kylechar, 2016/11/21 14:48:22]

I don't think that works? Maybe I just don't understand those logging macros
fully but FrameSinkId and LocalFrameId don't have << operator defined for them.
It looks like DCHECK_EQ/NE fail to compile when there is no way to print out the
values being checked?

../base/logging.h:558:3: error: no matching function for call to
'MakeCheckOpValueString'
+long template compile error

[sky, 2016/11/21 16:51:48]

My mistake, you are right.

[kylechar, 2016/11/21 16:54:20]

I talked with fsamuel about this and since SurfaceId/FrameSinkId/LocalFrameId
are all pretty widely used and debugging them is important, adding << operator
will be helpful and worth the tiny bit of extra code that is generated. I'll do
that in another CL and update here after.

+
+  // 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.

[Fady Samuel, 2016/11/18 22:27:24]

nit: specify when this can happen.

[kylechar, 2016/11/21 14:48:22]

Done.

+  if (!window->parent())
+    AddNewParentReferences(ref.child_id, surface_id);
+
+  // 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) {
@@ -94,8 +127,15 @@ void FrameGenerator::OnBeginFrame(const cc::BeginFrameArgs& begin_frame_arags) {
 
   // 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(
@@ -189,8 +229,6 @@ void FrameGenerator::DrawWindowTree(
                 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*/,
@@ -214,8 +252,6 @@ void FrameGenerator::DrawWindowTree(
                 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*/,
@@ -223,63 +259,97 @@ void FrameGenerator::DrawWindowTree(
   }
 }
 
-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;
+cc::SurfaceId FrameGenerator::FindParentSurfaceId(ServerWindow* window) {
+  if (window == root_window_)
+    return top_level_root_;
+
+  return root_window_->compositor_frame_sink_manager()->GetLatestSurfaceId(

[sky, 2016/11/19 16:38:56]

How do you know the latest surface id corresponds to window? Comment?

[kylechar, 2016/11/21 14:48:22]

Added comment.

+      mojom::CompositorFrameSinkType::DEFAULT);
+}
+
+void FrameGenerator::AddSurfaceReference(const cc::SurfaceId& parent_id,
+                                         const cc::SurfaceId& child_id) {
+  if (parent_id == top_level_root_)
+    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);
+
+  if (parent_id == top_level_root_ || 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 == top_level_root_) {
+      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 == top_level_root_)
+      display_compositor->RemoveRootSurfaceReference(ref.child_id);
+    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() {
+  // TODO(kylechar): Remove multiple surfaces with one IPC call.
+  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() {
@@ -299,13 +369,13 @@ void FrameGenerator::OnWindowDestroying(ServerWindow* window) {
       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