Allow layers to signal that additional sequences are needed before surface destruction

content/browser/compositor/delegated_frame_host.cc

Index: content/browser/compositor/delegated_frame_host.cc
diff --git a/content/browser/compositor/delegated_frame_host.cc b/content/browser/compositor/delegated_frame_host.cc
index ccf962abd6644b36eb1d6baa1428a75910b148c8..f52eb689b8b48309faee0ea722d51b6141b57a37 100644
--- a/content/browser/compositor/delegated_frame_host.cc
+++ b/content/browser/compositor/delegated_frame_host.cc
@@ -12,6 +12,7 @@
 #include "cc/resources/single_release_callback.h"
 #include "cc/resources/texture_mailbox.h"
 #include "cc/surfaces/surface_factory.h"
+#include "cc/surfaces/surface_manager.h"
 #include "content/browser/compositor/resize_lock.h"
 #include "content/browser/gpu/compositor_util.h"
 #include "content/common/gpu/client/gl_helper.h"
@@ -27,6 +28,17 @@
 
 namespace content {
 
+namespace {
+
+void SatisfyCallback(cc::SurfaceManager* manager,
+                     cc::SurfaceSequence sequence) {
+  std::vector<uint32_t> sequences;
+  sequences.push_back(sequence.sequence);
+  manager->DidSatisfySequences(sequence.id_namespace, &sequences);
+}
+
+}  // namespace
+
 ////////////////////////////////////////////////////////////////////////////////
 // DelegatedFrameHostClient
 
@@ -371,9 +383,9 @@ void DelegatedFrameHost::SwapDelegatedFrame(
     EvictDelegatedFrame();
   } else {
     if (use_surfaces_) {
+      ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
+      cc::SurfaceManager* manager = factory->GetSurfaceManager();
       if (!surface_factory_) {
-        ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
-        cc::SurfaceManager* manager = factory->GetSurfaceManager();
         id_allocator_ =
             factory->GetContextFactory()->CreateSurfaceIdAllocator();
         surface_factory_ =
@@ -382,21 +394,19 @@ void DelegatedFrameHost::SwapDelegatedFrame(
       if (surface_id_.is_null() || frame_size != current_surface_size_ ||
           frame_size_in_dip != current_frame_size_in_dip_) {
         if (!surface_id_.is_null()) {
-          if (compositor) {
-            std::set<cc::SurfaceSequence> seq;
-            seq.insert(compositor->InsertSurfaceSequenceForNextFrame());
-            // Destruction of this surface needs to wait for compositors that
-            // have drawn using it to swap frames that don't reference it.
-            // TODO(jbauman): Handle cases where the compositor has been
-            // changed since the last draw.
-            surface_factory_->DestroyOnSequence(surface_id_, seq);
-          } else {
-            surface_factory_->Destroy(surface_id_);
-          }
+          std::set<cc::SurfaceSequence> seq;
+          surface_holder_->GetSequenceSet(&seq);
+
+          // Destruction of this surface needs to wait for compositors that
+          // have drawn using it to swap frames that don't reference it.
+          surface_factory_->DestroyOnSequence(surface_id_, seq);
         }
         surface_id_ = id_allocator_->GenerateId();
+        surface_holder_ = make_scoped_refptr(new cc::SurfaceHolder(

[piman, 2014/11/03 23:49:26]

nit: no need for make_scoped_refptr

+            surface_id_,
+            base::Bind(&SatisfyCallback, base::Unretained(manager))));

[piman, 2014/11/03 23:49:26]

Why is Unretained safe? Can you add comments? In particular, surface_holder_ is
refcounted, so it's really hard to reason about lifetime (e.g. something posting
a task keeping the surface_holder_ alive for longer than you would expect).

         surface_factory_->Create(surface_id_, frame_size);
-        client_->GetLayer()->SetShowSurface(surface_id_, frame_size_in_dip);
+        client_->GetLayer()->SetShowSurface(surface_holder_, frame_size_in_dip);
         current_surface_size_ = frame_size;
       }
       scoped_ptr<cc::CompositorFrame> compositor_frame =
@@ -530,7 +540,10 @@ void DelegatedFrameHost::EvictDelegatedFrame() {
   client_->GetLayer()->SetShowSolidColorContent();
   frame_provider_ = NULL;
   if (!surface_id_.is_null()) {
-    surface_factory_->Destroy(surface_id_);
+    std::set<cc::SurfaceSequence> seq;
+    surface_holder_->GetSequenceSet(&seq);
+    surface_factory_->DestroyOnSequence(surface_id_, seq);
+    surface_holder_ = nullptr;
     surface_id_ = cc::SurfaceId();
   }
   delegated_frame_evictor_->DiscardedFrame();
@@ -922,8 +935,12 @@ void DelegatedFrameHost::OnLostResources() {
 DelegatedFrameHost::~DelegatedFrameHost() {
   ImageTransportFactory::GetInstance()->RemoveObserver(this);
 
-  if (!surface_id_.is_null())
-    surface_factory_->Destroy(surface_id_);
+  if (!surface_id_.is_null()) {
+    std::set<cc::SurfaceSequence> seq;
+    surface_holder_->GetSequenceSet(&seq);
+    surface_factory_->DestroyOnSequence(surface_id_, seq);
+    surface_holder_ = nullptr;
+  }
   if (resource_collection_.get())
     resource_collection_->SetClient(NULL);
 
@@ -998,7 +1015,7 @@ void DelegatedFrameHost::OnLayerRecreated(ui::Layer* old_layer,
                                        current_frame_size_in_dip_);
   }
   if (!surface_id_.is_null()) {
-    new_layer->SetShowSurface(surface_id_, current_frame_size_in_dip_);
+    new_layer->SetShowSurface(surface_holder_, current_frame_size_in_dip_);
   }
 }