cc: Resource list for headup display

cc/layers/heads_up_display_layer_impl.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/layers/heads_up_display_layer_impl.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/debug/trace_event.h"
@@ skipping 50 matching lines @@
 
 double HeadsUpDisplayLayerImpl::Graph::UpdateUpperBound() {
   double target_upper_bound = std::max(max, default_upper_bound);
   current_upper_bound += (target_upper_bound - current_upper_bound) * 0.5;
   return current_upper_bound;
 }
 
 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() {}
 
 scoped_ptr<LayerImpl> HeadsUpDisplayLayerImpl::CreateLayerImpl(
     LayerTreeImpl* tree_impl) {
   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);
 }
 
 void HeadsUpDisplayLayerImpl::AppendQuads(
     RenderPass* render_pass,
     const OcclusionTracker<LayerImpl>& occlusion_tracker,
     AppendQuadsData* append_quads_data) {
   if (!hud_resource_->id())
     return;
@@ skipping 65 matching lines @@
   DCHECK(pixels);
   gfx::Rect content_rect(content_bounds());
   DCHECK(info.colorType() == kN32_SkColorType);
   resource_provider->SetPixels(hud_resource_->id(),
                                static_cast<const uint8_t*>(pixels),
                                content_rect,
                                content_rect,
                                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();
 
   // Don't update numbers every frame so text is readable.
   base::TimeTicks now = layer_tree_impl()->CurrentBeginFrameArgs().frame_time;
   if (base::TimeDelta(now - time_of_last_graph_update_).InSecondsF() > 0.25f) {
     time_of_last_graph_update_ = now;
 
     if (debug_state.show_fps_counter) {
@@ skipping 562 matching lines @@
   return "cc::HeadsUpDisplayLayerImpl";
 }
 
 void HeadsUpDisplayLayerImpl::AsValueInto(
     base::debug::TracedValue* dict) const {
   LayerImpl::AsValueInto(dict);
   dict->SetString("layer_name", "Heads Up Display Layer");
 }
 
 }  // namespace cc