cc: Resource list for headup display

cc/layers/heads_up_display_layer_impl.cc

Index: cc/layers/heads_up_display_layer_impl.cc
diff --git a/cc/layers/heads_up_display_layer_impl.cc b/cc/layers/heads_up_display_layer_impl.cc
index fcb2733aee90279959fff97e43148cba86413eb4..7d1be46f98fe97f4d91dd11c36bbab00669defce 100644
--- a/cc/layers/heads_up_display_layer_impl.cc
+++ b/cc/layers/heads_up_display_layer_impl.cc
@@ -68,11 +68,13 @@ double HeadsUpDisplayLayerImpl::Graph::UpdateUpperBound() {
 HeadsUpDisplayLayerImpl::HeadsUpDisplayLayerImpl(LayerTreeImpl* tree_impl,
                                                  int id)
     : LayerImpl(tree_impl, id),
+      hud_resource_(NULL),
       typeface_(skia::AdoptRef(
           SkTypeface::CreateFromName("monospace", SkTypeface::kBold))),
       fps_graph_(60.0, 80.0),
       paint_time_graph_(16.0, 48.0),
-      fade_step_(0) {}
+      fade_step_(0) {
+}
 
 HeadsUpDisplayLayerImpl::~HeadsUpDisplayLayerImpl() {}
 
@@ -81,26 +83,57 @@ scoped_ptr<LayerImpl> HeadsUpDisplayLayerImpl::CreateLayerImpl(
   return HeadsUpDisplayLayerImpl::Create(tree_impl, id()).PassAs<LayerImpl>();
 }
 
+ScopedResource* HeadsUpDisplayLayerImpl::AcquireResource(
+    ResourceProvider* resource_provider) {
+  HUDResourceList::iterator iter;
+  for (iter = resources_.begin(); iter != resources_.end(); ++iter) {
+    if (!resource_provider->InUseByConsumer((*iter)->id()))

[danakj, 2014/08/25 14:51:21]

how come this doesn't check content_bounds?

[JungJik, 2014/08/26 11:16:35]

I was trying to remove all resources which have different size.

+      return *iter;
+  }
+
+  scoped_ptr<ScopedResource> resource =
+      ScopedResource::Create(resource_provider);
+  resource->Allocate(
+      content_bounds(), ResourceProvider::TextureHintImmutable, RGBA_8888);
+  resources_.push_back(resource.release());
+  return resources_.back();
+}
+
+void HeadsUpDisplayLayerImpl::ReleaseUnusedResources(
+    ResourceProvider* resource_provider) {
+  bool found_unused_resource = false;
+
+  HUDResourceList::iterator iter;
+  for (iter = resources_.begin(); iter != resources_.end(); ++iter) {
+    if ((*iter)->size() != content_bounds()) {
+      ReleaseResource(iter);

[danakj, 2014/08/25 14:51:21]

calling erase during iteration, this invalidates the iterator. shouldn't you
store the ones you intend to delete, and then do a separate walk over the
to_delete list to actually do that work? maybe  a vector would be nice for this
reason considering the size of this thing will be like.. 2 or 3.

+      continue;
+    }
+
+    if (!resource_provider->InUseByConsumer((*iter)->id())) {
+      if (found_unused_resource)

[danakj, 2014/08/25 14:51:21]

if you make Acquire move the resource it chooses to the back of the list, and
you do the acquire step before doing the release step, then this method can just
skip freeing the back() resource.

+        ReleaseResource(iter);
+      found_unused_resource = true;
+    }
+  }
+}
+
+void HeadsUpDisplayLayerImpl::ReleaseResource(HUDResourceList::iterator iter) {
+  ScopedResource* resource = *iter;
+  if (resource->id())

[danakj, 2014/08/25 14:51:21]

why would we have a resource without an id in the list?

+    resource->Free();

[danakj, 2014/08/25 14:51:21]

the destructor already calls Free, you just need to destroy the object.

+  resources_.erase(iter);
+  delete resource;

[danakj, 2014/08/25 14:51:21]

manual memory management :(

what if you put the ScopedResource in a scoped_ptr and make the list a
ScopedPtrVector? This whole method could just go away, right? You'd just erase
the element to destroy it properly.

[JungJik, 2014/08/26 11:16:35]

thanks for comments. ScopedPtrVector, this is what I want to use. :) I will make
new patch with ScopedPtrVector soon.

+}
+
 bool HeadsUpDisplayLayerImpl::WillDraw(DrawMode draw_mode,
                                        ResourceProvider* resource_provider) {
   if (draw_mode == DRAW_MODE_RESOURCELESS_SOFTWARE)
     return false;
 
-  if (!hud_resource_)
-    hud_resource_ = ScopedResource::Create(resource_provider);
+  ReleaseUnusedResources(resource_provider);
 
-  // TODO(danakj): The HUD could swap between two textures instead of creating a
-  // texture every frame in ubercompositor.
-  if (hud_resource_->size() != content_bounds() ||
-      (hud_resource_->id() &&
-       resource_provider->InUseByConsumer(hud_resource_->id()))) {
-    hud_resource_->Free();
-  }
-
-  if (!hud_resource_->id()) {
-    hud_resource_->Allocate(
-        content_bounds(), ResourceProvider::TextureHintImmutable, RGBA_8888);
-  }
+  hud_resource_ = AcquireResource(resource_provider);

[danakj, 2014/08/25 14:51:21]

how about just using the resources_.back() and move the resource you'll use to
the back always, instead of adding another member variable to track this.

[JungJik, 2014/08/26 11:16:35]

okay, I will apply it in next patch. thank you.

 
   return LayerImpl::WillDraw(draw_mode, resource_provider);
 }
@@ -186,7 +219,12 @@ void HeadsUpDisplayLayerImpl::UpdateHudTexture(
                                gfx::Vector2d());
 }
 
-void HeadsUpDisplayLayerImpl::ReleaseResources() { hud_resource_.reset(); }
+void HeadsUpDisplayLayerImpl::ReleaseResources() {
+  HUDResourceList::iterator iter;
+  for (iter = resources_.begin(); iter != resources_.end(); ++iter)
+    ReleaseResource(iter);

[danakj, 2014/08/25 14:51:21]

If you erase while iterating the same list, you're going to have a bad time.

[JungJik, 2014/08/26 11:16:35]

I will remember it.

+  hud_resource_ = NULL;
+}
 
 void HeadsUpDisplayLayerImpl::UpdateHudContents() {
   const LayerTreeDebugState& debug_state = layer_tree_impl()->debug_state();