Add cc::Surface ref counting.

cc/surfaces/surface_manager.cc

Index: cc/surfaces/surface_manager.cc
diff --git a/cc/surfaces/surface_manager.cc b/cc/surfaces/surface_manager.cc
index 0e204a44ed81e61b84010ce0e4d524004cea0d6c..bcc853bcc8ef83f0dc87654f19b8805213c7d6f9 100644
--- a/cc/surfaces/surface_manager.cc
+++ b/cc/surfaces/surface_manager.cc
@@ -7,6 +7,8 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <utility>
+
 #include "base/logging.h"
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory_client.h"
@@ -68,10 +70,8 @@ void SurfaceManager::Destroy(std::unique_ptr<Surface> surface) {
 void SurfaceManager::DidSatisfySequences(const FrameSinkId& frame_sink_id,
                                          std::vector<uint32_t>* sequence) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  for (std::vector<uint32_t>::iterator it = sequence->begin();
-       it != sequence->end();
-       ++it) {
-    satisfied_sequences_.insert(SurfaceSequence(frame_sink_id, *it));
+  for (uint32_t value : *sequence) {
+    satisfied_sequences_.insert(SurfaceSequence(frame_sink_id, value));
   }
   sequence->clear();
   GarbageCollectSurfaces();
@@ -87,22 +87,132 @@ void SurfaceManager::InvalidateFrameSinkId(const FrameSinkId& frame_sink_id) {
   GarbageCollectSurfaces();
 }
 
+void SurfaceManager::AddSurfaceIdReference(const SurfaceId& parent_id,
+                                           const SurfaceId& child_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  LOG(ERROR) << "SurfaceManager::AddSurfaceIdReference\n  parent_id="
+             << parent_id.ToString() << "\n  child_id=" << child_id.ToString();
+
+  // Should we allow multiple references? Maybe? IDK.

[Fady Samuel, 2016/10/27 04:14:48]

To reduce complexity here and IPCs I think we should not allow multiple
references, now that I think about it. That info can be managed in the client I
think? Although that adds complexity in the client...I don't have a strong
opinion either way. let's keep it simple here and add complexity if necessary I
think.

[kylechar, 2016/10/27 21:14:09]

Changed per our offline discussion.

+  DCHECK_EQ(parent_to_child_refs_[parent_id].count(child_id), 0u);
+  DCHECK_EQ(child_to_parent_refs_[child_id].count(parent_id), 0u);
+
+  parent_to_child_refs_[parent_id].insert(child_id);
+  child_to_parent_refs_[child_id].insert(parent_id);
+
+  DCHECK_EQ(parent_to_child_refs_[parent_id].count(child_id), 1u);
+  DCHECK_EQ(child_to_parent_refs_[child_id].count(parent_id), 1u);
+}
+
+void SurfaceManager::RemoveSurfaceIdReference(const SurfaceId& parent_id,

[Fady Samuel, 2016/10/27 04:14:48]

We should garbage collect if a surface ID no longer has references.

+                                              const SurfaceId& child_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  LOG(ERROR) << "SurfaceManager::RemoveSurfaceIdReference\n  parent_id="
+             << parent_id.ToString() << "\n  child_id=" << child_id.ToString();
+
+  // Remove the reference from parent to child. This doesn't change anything
+  // about the validity of the parent.
+  DCHECK_EQ(parent_to_child_refs_.count(parent_id), 1u);

[Fady Samuel, 2016/10/27 04:14:48]

This actually doesn't work very well for restartability. If a reference doesn't
exist, then I think we should just ignore it instead of DCHECK'ing. The client
may end up returning a surface ID that the gpu doesn't know about, and it's
easier to ignore it here than to worry about solving races in the client, in my
opinion. Other cc OWNERS may feel differently, I don't know.

[kylechar, 2016/10/27 21:14:09]

I'm checking the reference exists instead of DCHECK'ing.

+  DCHECK_EQ(parent_to_child_refs_[parent_id].count(child_id), 1u);
+  parent_to_child_refs_[parent_id].erase(child_id);
+
+  // Remove the reference from child to parent. This might drop the number of
+  // references to the child down to zero.
+  DCHECK_EQ(child_to_parent_refs_.count(child_id), 1u);
+  SurfaceIdSet& child_refs = child_to_parent_refs_[child_id];
+  DCHECK_EQ(child_refs.count(parent_id), 1u);
+  child_refs.erase(parent_id);
+
+  // Check if child has zero reference, if so we unref all children it
+  // references.
+  if (child_refs.empty()) {
+    LOG(ERROR) << child_id.ToString() << " dropped to ZERO references.";
+    SurfaceIdSet child_child_refs = parent_to_child_refs_[child_id];  // Copy

[Fady Samuel, 2016/10/27 04:14:48]

Add a comment here that you need to do this because you're modifying the set?

[kylechar, 2016/10/27 21:14:09]

Changed, PTAL.

+    for (const SurfaceId& surface_id : child_child_refs) {
+      RemoveSurfaceIdReference(child_id, surface_id);
+    }
+    DCHECK_EQ(CountSurfaceReferees(child_id), 0);

[Fady Samuel, 2016/10/27 04:14:48]

We should start a garbage collect if a surface ID no longer has references.

[kylechar, 2016/10/27 21:14:09]

Done. I've added an optional parameter to avoid GC'ing more than once per call
top level call to RemoveSurfaceIdReference().

+  }
+}
+
+void SurfaceManager::RemoveAllReferencesForFrameSink(
+    const FrameSinkId& frame_sink_id) {
+  LOG(ERROR) << "SurfaceManager::DropReferencesForFrameSink frame_sink_id="
+             << frame_sink_id.ToString();
+
+  // Find set of surfaces that belong to the FrameSink.
+  SurfaceIdSet frame_sink_surfaces;
+  for (auto& map_entry : parent_to_child_refs_) {

[Fady Samuel, 2016/10/27 04:14:48]

Could you add a description of what we discussed here as to why a FrameSink may
have multiple surface IDs in flight at once? A parent may refer to an old
surface ID which retains all sub references.

[Fady Samuel, 2016/10/27 04:14:48]

nit: const auto&

[kylechar, 2016/10/27 21:14:09]

Done.

+    const SurfaceId& surface_id = map_entry.first;
+    if (surface_id.frame_sink_id() == frame_sink_id)
+      frame_sink_surfaces.insert(surface_id);
+  }
+  for (auto& map_entry : child_to_parent_refs_) {
+    const SurfaceId& surface_id = map_entry.first;
+    if (surface_id.frame_sink_id() == frame_sink_id)
+      frame_sink_surfaces.insert(surface_id);

[Fady Samuel, 2016/10/27 04:14:48]

I'm confused about when we'd hit this case?

[kylechar, 2016/10/27 21:14:09]

This is for leaf surfaces. So if we have A -> B -> C there will be the following
entries:

parent_to_child_refs_[A] = {B}
parent_to_child_refs_[B] = {C}
child_to_parent_refs_[B] = {A}
child_to_parent_refs_[C] = {B}

Since C doesn't reference anything it's not in parent_to_child_refs_ but we
still have a reference from B -> C we need to remove.

+  }
+
+  // Clear all references involving FrameSink surfaces.

[Fady Samuel, 2016/10/27 04:14:48]

nit: This comment is confusing. // Clear all surfaces belonging to FrameSink
|frame_sink_id|.

[kylechar, 2016/10/27 21:14:09]

Done.

+  for (const SurfaceId& surface_id : frame_sink_surfaces) {
+    // Remove any references from parents to this surface, since this surface

[Fady Samuel, 2016/10/27 04:14:48]

It seems like all the code in this block could move to a helper method to easy
readability:

RemoveAllSurfaceIdReferences(surface_id);

[kylechar, 2016/10/27 21:14:09]

Done.

+    // no longer exists. This surface will have zero references when finished.
+    if (child_to_parent_refs_.count(surface_id) > 0) {
+      SurfaceIdSet parent_refs = child_to_parent_refs_[surface_id];  // Copy
+      for (auto& parent_id : parent_refs)
+        RemoveSurfaceIdReference(parent_id, surface_id);
+    }
+    DCHECK_EQ(CountSurfaceReferences(surface_id), 0);
+
+    // Remove any references from this surface to children, since this surface
+    // no longer exists. Any child surfaces may drop to zero references here,
+    // recursively clearing any references it holds.
+    if (parent_to_child_refs_.count(surface_id) > 0) {
+      SurfaceIdSet child_refs = parent_to_child_refs_[surface_id];  // Copy
+      for (auto& child_id : child_refs)
+        RemoveSurfaceIdReference(surface_id, child_id);
+    }
+    DCHECK_EQ(CountSurfaceReferees(surface_id), 0);
+  }
+}
+
+int SurfaceManager::CountSurfaceReferences(const SurfaceId& surface_id) const {

[Fady Samuel, 2016/10/27 04:14:48]

nit: GetSurfaceReferenceCount? Make it uint32_t?

[kylechar, 2016/10/27 21:14:09]

Changed name. If I'm going to use an unsigned type I'll use size_t.

+  auto iter = child_to_parent_refs_.find(surface_id);
+  if (iter == child_to_parent_refs_.end())
+    return 0;
+  return static_cast<int>(iter->second.size());
+}
+
+int SurfaceManager::CountSurfaceReferees(const SurfaceId& surface_id) const {

[Fady Samuel, 2016/10/27 04:14:48]

GetReferencedSurfaceCountBySurface?

[kylechar, 2016/10/27 21:14:09]

Changed name.

+  auto iter = parent_to_child_refs_.find(surface_id);
+  if (iter == parent_to_child_refs_.end())
+    return 0;
+  return static_cast<int>(iter->second.size());
+}
+
 void SurfaceManager::GarbageCollectSurfaces() {
   // Simple mark and sweep GC.
   // TODO(jbauman): Reduce the amount of work when nothing needs to be
   // destroyed.
   std::vector<SurfaceId> live_surfaces;
-  std::set<SurfaceId> live_surfaces_set;
+  std::unordered_set<SurfaceId, SurfaceIdHash> live_surfaces_set;
 
   // GC roots are surfaces that have not been destroyed, or have not had all
   // their destruction dependencies satisfied.
   for (auto& map_entry : surface_map_) {
-    map_entry.second->SatisfyDestructionDependencies(&satisfied_sequences_,
-                                                     &valid_frame_sink_ids_);
-    if (!map_entry.second->destroyed() ||
-        map_entry.second->GetDestructionDependencyCount()) {
-      live_surfaces_set.insert(map_entry.first);
-      live_surfaces.push_back(map_entry.first);
+    const SurfaceId& surface_id = map_entry.first;
+    Surface* surface = map_entry.second;
+    surface->SatisfyDestructionDependencies(&satisfied_sequences_,
+                                            &valid_frame_sink_ids_);
+
+    // Never use both SurfaceSequence and Surface references.
+    DCHECK(surface->GetDestructionDependencyCount() == 0 ||
+           CountSurfaceReferences(surface_id) == 0);
+
+    if (!surface->destroyed() || surface->GetDestructionDependencyCount() > 0 ||
+        CountSurfaceReferences(surface_id) > 0) {
+      live_surfaces_set.insert(surface_id);
+      live_surfaces.push_back(surface_id);
     }
   }
 
@@ -127,15 +237,15 @@ void SurfaceManager::GarbageCollectSurfaces() {
   std::vector<std::unique_ptr<Surface>> to_destroy;
 
   // Destroy all remaining unreachable surfaces.
-  for (SurfaceDestroyList::iterator dest_it = surfaces_to_destroy_.begin();
-       dest_it != surfaces_to_destroy_.end();) {
-    if (!live_surfaces_set.count((*dest_it)->surface_id())) {
-      std::unique_ptr<Surface> surf(std::move(*dest_it));
-      DeregisterSurface(surf->surface_id());
-      dest_it = surfaces_to_destroy_.erase(dest_it);
-      to_destroy.push_back(std::move(surf));
+  for (auto iter = surfaces_to_destroy_.begin();
+       iter != surfaces_to_destroy_.end();) {
+    SurfaceId surface_id = (*iter)->surface_id();
+    if (!live_surfaces_set.count(surface_id)) {
+      DeregisterSurface(surface_id);
+      to_destroy.push_back(std::move(*iter));
+      iter = surfaces_to_destroy_.erase(iter);
     } else {
-      ++dest_it;
+      ++iter;
     }
   }
 
@@ -146,21 +256,17 @@ void SurfaceManager::RegisterSurfaceFactoryClient(
     const FrameSinkId& frame_sink_id,
     SurfaceFactoryClient* client) {
   DCHECK(client);
-  DCHECK(!frame_sink_source_map_[frame_sink_id].client);
   DCHECK_EQ(valid_frame_sink_ids_.count(frame_sink_id), 1u);
 
-  auto iter = frame_sink_source_map_.find(frame_sink_id);
-  if (iter == frame_sink_source_map_.end()) {
-    auto insert_result = frame_sink_source_map_.insert(
-        std::make_pair(frame_sink_id, FrameSinkSourceMapping()));
-    DCHECK(insert_result.second);
-    iter = insert_result.first;
-  }
-  iter->second.client = client;
+  // Will create a new FrameSinkSourceMapping for |frame_sink_id| if necessary.
+  FrameSinkSourceMapping& frame_sink_source =
+      frame_sink_source_map_[frame_sink_id];
+  DCHECK(!frame_sink_source.client);
+  frame_sink_source.client = client;
 
   // Propagate any previously set sources to the new client.
-  if (iter->second.source)
-    client->SetBeginFrameSource(iter->second.source);
+  if (frame_sink_source.source)
+    client->SetBeginFrameSource(frame_sink_source.source);
 }
 
 void SurfaceManager::UnregisterSurfaceFactoryClient(
@@ -168,6 +274,8 @@ void SurfaceManager::UnregisterSurfaceFactoryClient(
   DCHECK_EQ(valid_frame_sink_ids_.count(frame_sink_id), 1u);
   DCHECK_EQ(frame_sink_source_map_.count(frame_sink_id), 1u);
 
+  RemoveAllReferencesForFrameSink(frame_sink_id);
+
   auto iter = frame_sink_source_map_.find(frame_sink_id);
   if (iter->second.source)
     iter->second.client->SetBeginFrameSource(nullptr);
@@ -338,7 +446,7 @@ Surface* SurfaceManager::GetSurfaceForId(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   SurfaceMap::iterator it = surface_map_.find(surface_id);
   if (it == surface_map_.end())
-    return NULL;
+    return nullptr;
   return it->second;
 }