UI Resource Manager

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
===================================================================
--- cc/trees/layer_tree_host_impl.cc	(revision 210393)
+++ cc/trees/layer_tree_host_impl.cc	(working copy)
@@ -40,6 +40,7 @@
 #include "cc/resources/memory_history.h"
 #include "cc/resources/picture_layer_tiling.h"
 #include "cc/resources/prioritized_resource_manager.h"
+#include "cc/resources/ui_resource_bitmap.h"
 #include "cc/scheduler/delay_based_time_source.h"
 #include "cc/scheduler/texture_uploader.h"
 #include "cc/trees/damage_tracker.h"
@@ -49,6 +50,7 @@
 #include "cc/trees/quad_culler.h"
 #include "cc/trees/single_thread_proxy.h"
 #include "cc/trees/tree_synchronizer.h"
+#include "gpu/GLES2/gl2extchromium.h"
 #include "ui/gfx/size_conversions.h"
 #include "ui/gfx/vector2d_conversions.h"
 
@@ -1298,6 +1300,20 @@
 }
 
 void LayerTreeHostImpl::DidLoseOutputSurface() {
+  // When output surface is lost, notify the client of the lost and remove
+  // exisiting entries.

[enne (OOO), 2013/07/22 23:09:15]

typo: exisiting => existing

[powei, 2013/07/24 02:28:29]

Done.

+  for (UIResourceMap::iterator iter = ui_resource_map_.begin();
+      iter != ui_resource_map_.end();
+      iter++) {
+    client_->UIResourceLostOnImplThread(iter->first);

[enne (OOO), 2013/07/22 23:09:15]

I am still a little confused about this path.  It still seems to me that losing
one implies losing all.  Can LayerTreeHost::DidLoseOutputSurface drop all these
resources without needing all of the UIResourceLost message bouncing?

[aelias_OOO_until_Jul13, 2013/07/23 00:06:48]

I agree with Enne, the loop should be in LayerTreeHost.

[powei, 2013/07/24 02:28:29]

Done.

[powei, 2013/07/24 02:28:29]

Done.

+  }
+  // Remove all map from existing UIResourceId to ResourceId.
+  ui_resource_map_.clear();
+  // Remove all pending UI resource requests because there might be
+  // deletion requests, which can no longer be honored (or already
+  // gone anyways).
+  ui_resource_request_queue_.clear();
+
   // TODO(jamesr): The renderer_ check is needed to make some of the
   // LayerTreeHostContextTest tests pass, but shouldn't be necessary (or
   // important) in production. We should adjust the test to not need this.
@@ -1430,6 +1446,19 @@
             stats.total_rasterize_time_for_now_bins_on_pending_tree);
   }
 
+  while (ui_resource_request_queue_.size() > 0) {
+    UIResourceRequest req = ui_resource_request_queue_.front();
+    ui_resource_request_queue_.pop_front();
+    switch (req.type) {
+     case UIResourceCreate:
+       CreateUIResource(req.id, req.bitmap);

[enne (OOO), 2013/07/22 23:09:15]

In general activation is gated on having all of your resources uploaded and
ready, so it's a little odd to create ui resources as a part of activation.

Is the assumption that these resources are not going to be so costly that this
will cause jank? Are these resources always going to be synchronously uploaded?

Is there any way to (at least for impl-side painting) reuse the tile manager's
asynchronous upload and also not make this memory usage invisible to it?

[aelias_OOO_until_Jul13, 2013/07/23 00:06:48]

Yes, the assumption is that for v1 of this patch, it will only be used for small
resources that are unlikely to cause jank or cause memory problems.  In the
future, we'll improve this to use the async upload system (this is one of the
requirements for the Android thumbnail use case).

[powei, 2013/07/24 02:28:29]

Done.

[powei, 2013/07/24 02:28:29]

Done.

+       break;
+     case UIResourceDelete:
+       DeleteUIResource(req.id);
+       break;
+    }
+  }
+
   client_->DidActivatePendingTree();
 }
 
@@ -2362,4 +2391,39 @@
   debug_state_ = debug_state;
 }
 
+void LayerTreeHostImpl::CreateUIResource(
+    UIResourceId uid,
+    scoped_refptr<UIResourceBitmap> bitmap) {
+  ResourceProvider::ResourceId id =
+    resource_provider_->CreateGLTexture(bitmap->GetSize(),

[enne (OOO), 2013/07/22 23:09:15]

CreateGLTexture in LayerTreeHostImpl? What if there's a software renderer?

[powei, 2013/07/24 02:28:29]

Done.

+                                        GL_RGBA,

[enne (OOO), 2013/07/22 23:09:15]

Should you DCHECK that bitmap's format is RGBA8 and not ETC1?

[powei, 2013/07/24 02:28:29]

Done.

+                                        GL_TEXTURE_POOL_UNMANAGED_CHROMIUM,
+                                        ResourceProvider::TextureUsageAny);
+
+  ui_resource_map_[uid] = id;

[enne (OOO), 2013/07/22 23:09:15]

Can you DCHECK that it doesn't previously exist?

[powei, 2013/07/24 02:28:29]

Done.

+  gfx::Size bitmap_size = bitmap->GetSize();
+  resource_provider_->SetPixels(id,
+                                reinterpret_cast<uint8_t*>(bitmap->GetPixels()),
+                                gfx::Rect(bitmap_size),
+                                gfx::Rect(bitmap_size),
+                                gfx::Vector2d(0, 0));
+}
+
+void LayerTreeHostImpl::DeleteUIResource(UIResourceId uid) {
+  ResourceProvider::ResourceId id = ResourceIdForUIResource(uid);
+  if (id) {
+    resource_provider_->DeleteResource(id);
+    ui_resource_map_.erase(uid);
+  }
+}
+
+ResourceProvider::ResourceId
+LayerTreeHostImpl::ResourceIdForUIResource(UIResourceId uid) const {
+  if (ui_resource_map_.find(uid) != ui_resource_map_.end()) {

[enne (OOO), 2013/07/22 23:09:15]

If you're going to find and then do something with the results, please keep the
resulting iterator around rather than doing a second find.

[powei, 2013/07/24 02:28:29]

Done.

+    return ui_resource_map_.find(uid)->second;
+  }
+  return 0;
+}
+
+
 }  // namespace cc