Mus+Ash: Use standard cc mechanism for surface lifetime.

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 d5b8c3cd6866c88bdd3cbbc3f5643dad597e5563..9c9e674d8435a66631ab37ce25b49bcc4702d043 100644
--- a/services/ui/ws/frame_generator.cc
+++ b/services/ui/ws/frame_generator.cc
@@ -10,6 +10,7 @@
 #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/compositor_frame_sink.h"
 #include "services/ui/ws/frame_generator_delegate.h"
@@ -30,9 +31,11 @@ FrameGenerator::FrameGenerator(
       draw_timer_(false, false),
       weak_factory_(this) {
   DCHECK(delegate_);
+  surface_sequence_generator_.set_frame_sink_id(frame_sink_id_);
 }
 
 FrameGenerator::~FrameGenerator() {
+  ReleaseAllSurfaceReferences();
   // Invalidate WeakPtrs now to avoid callbacks back into the
   // FrameGenerator during destruction of |compositor_frame_sink_|.
   weak_factory_.InvalidateWeakPtrs();
@@ -109,13 +112,12 @@ void FrameGenerator::Draw() {
 
 void FrameGenerator::DidDraw() {
   frame_pending_ = false;
-  delegate_->OnCompositorFrameDrawn();
   if (!dirty_rect_.IsEmpty())
     WantToDraw();
 }
 
 cc::CompositorFrame FrameGenerator::GenerateCompositorFrame(
-    const gfx::Rect& output_rect) const {
+    const gfx::Rect& output_rect) {
   const cc::RenderPassId render_pass_id(1, 1);
   std::unique_ptr<cc::RenderPass> render_pass = cc::RenderPass::Create();
   render_pass->SetNew(render_pass_id, output_rect, dirty_rect_,
@@ -159,7 +161,7 @@ void FrameGenerator::DrawWindowTree(
     ServerWindow* window,
     const gfx::Vector2d& parent_to_root_origin_offset,
     float opacity,
-    bool* may_contain_video) const {
+    bool* may_contain_video) {
   if (!window->visible())
     return;
 
@@ -201,6 +203,7 @@ void FrameGenerator::DrawWindowTree(
                 combined_opacity, SkXfermode::kSrcOver_Mode,
                 0 /* sorting-context_id */);
     auto* quad = pass->CreateAndAppendDrawQuad<cc::SurfaceDrawQuad>();
+    AddOrUpdateSurfaceReference(default_surface);
     quad->SetAll(sqs, bounds_at_origin /* rect */,
                  gfx::Rect() /* opaque_rect */,
                  bounds_at_origin /* visible_rect */, true /* needs_blending*/,
@@ -225,6 +228,7 @@ void FrameGenerator::DrawWindowTree(
                 0 /* sorting-context_id */);
 
     auto* quad = pass->CreateAndAppendDrawQuad<cc::SurfaceDrawQuad>();
+    AddOrUpdateSurfaceReference(underlay_surface);
     quad->SetAll(sqs, bounds_at_origin /* rect */,
                  gfx::Rect() /* opaque_rect */,
                  bounds_at_origin /* visible_rect */, true /* needs_blending*/,
@@ -233,6 +237,80 @@ void FrameGenerator::DrawWindowTree(
   }
 }
 
+void FrameGenerator::AddOrUpdateSurfaceReference(
+    ServerWindowSurface* window_surface) {
+  if (!window_surface->has_frame())
+    return;
+  cc::SurfaceId surface_id = window_surface->GetSurfaceId();
+  cc::SurfaceManager* surface_manager = display_compositor_->manager();
+  auto it = dependencies_.find(surface_id.frame_sink_id());
+  if (it == dependencies_.end()) {
+    cc::Surface* surface = surface_manager->GetSurfaceForId(surface_id);
+    if (!surface) {
+      LOG(ERROR) << "Attempting to add dependency to nonexistent surface "
+                 << surface_id.ToString();
+      return;
+    }
+    SurfaceDependency dependency = {
+        surface_id.local_frame_id(),
+        surface_sequence_generator_.CreateSurfaceSequence()};
+    surface->AddDestructionDependency(dependency.sequence);

[rjkroege, 2016/10/07 19:34:50]

I think that this logic could be refactored in the future

[Fady Samuel, 2016/10/07 20:31:33]

This is going away once kylechar@ finishes his surface refcounting work.

[rjkroege, 2016/10/07 20:48:13]

can you add a TODO before landing?

[Fady Samuel, 2016/10/07 21:04:32]

Done.

+    dependencies_[surface_id.frame_sink_id()] = dependency;
+    // Observe |window_surface|'s window so that we can release references when
+    // the window is destroyed.
+    window_surface->window()->AddObserver(this);
+    return;
+  }
+
+  // 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)
+    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());
+
+  AddOrUpdateSurfaceReference(window_surface);

[rjkroege, 2016/10/07 19:34:50]

What is going on with this recursion? Is this ever reached? It's not clear to me
if it will always stop.

[Fady Samuel, 2016/10/07 20:31:33]

It will. The reason we got here is because we already had a stale dependency. We
released the reference in the line above so it has been cleared from the
dependencies_ map and so we'll enter the if statement above.

[rjkroege, 2016/10/07 20:48:13]

Please insert this something like this as a comment before landing.

[Fady Samuel, 2016/10/07 21:04:32]

Done.

+}
+
+void FrameGenerator::ReleaseFrameSinkReference(
+    const cc::FrameSinkId& frame_sink_id) {
+  auto it = dependencies_.find(frame_sink_id);
+  if (it == dependencies_.end())
+    return;
+  std::vector<uint32_t> sequences;
+  sequences.push_back(it->second.sequence.sequence);
+  cc::SurfaceManager* surface_manager = display_compositor_->manager();
+  surface_manager->DidSatisfySequences(frame_sink_id_, &sequences);
+  dependencies_.erase(it);
+}
+
+void FrameGenerator::ReleaseAllSurfaceReferences() {
+  cc::SurfaceManager* surface_manager = display_compositor_->manager();
+  std::vector<uint32_t> sequences;
+  for (auto& dependency : dependencies_)
+    sequences.push_back(dependency.second.sequence.sequence);
+  surface_manager->DidSatisfySequences(frame_sink_id_, &sequences);
+  dependencies_.clear();
+}
+
+void FrameGenerator::OnWindowDestroying(ServerWindow* window) {
+  window->RemoveObserver(this);
+  ServerWindowSurfaceManager* surface_manager = window->surface_manager();
+  // If FrameGenerator was observing |window|, then that means it had a surface
+  // at some point in time and should have a ServerWindowSurfaceManager.
+  DCHECK(surface_manager);
+  ServerWindowSurface* default_surface = surface_manager->GetDefaultSurface();
+  if (default_surface)
+    ReleaseFrameSinkReference(default_surface->frame_sink_id());
+  ServerWindowSurface* underlay_surface = surface_manager->GetUnderlaySurface();
+  if (underlay_surface)
+    ReleaseFrameSinkReference(underlay_surface->frame_sink_id());
+}
+
 }  // namespace ws
 
 }  // namespace ui