Add multi-process GpuMemoryBuffer framework.

content/common/gpu/client/command_buffer_proxy_impl.cc

Index: content/common/gpu/client/command_buffer_proxy_impl.cc
diff --git a/content/common/gpu/client/command_buffer_proxy_impl.cc b/content/common/gpu/client/command_buffer_proxy_impl.cc
index 94abbe1f4fec261c6df14da355b5069e2ee9e7e3..52dee5bfa67056a98e53ae635e405965e2081011 100644
--- a/content/common/gpu/client/command_buffer_proxy_impl.cc
+++ b/content/common/gpu/client/command_buffer_proxy_impl.cc
@@ -367,7 +367,7 @@ void CommandBufferProxyImpl::SetContextLostReason(
 }
 
 bool CommandBufferProxyImpl::SupportsGpuMemoryBuffer() {
-  return false;
+  return true;

[piman, 2013/10/21 21:57:00]

Won't this force impl-side painting to go through this path, which removes all
scheduling of texture uploads? Isn't that a bad thing?

[reveman, 2013/10/22 16:26:11]

It's disabled by default. You need to use --enable-map-image for this to be
used.

 }
 
 gfx::GpuMemoryBuffer* CommandBufferProxyImpl::CreateGpuMemoryBuffer(
@@ -375,12 +375,55 @@ gfx::GpuMemoryBuffer* CommandBufferProxyImpl::CreateGpuMemoryBuffer(
     size_t height,
     unsigned internalformat,
     int32* id) {
-  NOTREACHED();
-  return NULL;
+  *id = -1;
+
+  if (last_state_.error != gpu::error::kNoError)
+    return NULL;
+
+  int32 new_id = channel_->ReserveGpuMemoryBufferId();
+  DCHECK(gpu_memory_buffers_.find(new_id) == gpu_memory_buffers_.end());
+
+  scoped_ptr<gfx::GpuMemoryBuffer> gpu_memory_buffer(
+      channel_->factory()->AllocateGpuMemoryBuffer(width,
+                                                   height,
+                                                   internalformat));
+  if (!gpu_memory_buffer)
+    return NULL;
+

[piman, 2013/10/21 21:57:00]

nit: can we DCHECK the handle type here?

[reveman, 2013/10/22 16:26:11]

Hm, I'd prefer to keep this code type agnostic. Can you explain a bit more why
you think this is necessary? ShareGpuMemoryBufferToGpuProcess already has a
NOTREACHED() for invalid types.

+  // This handle is owned by the GPU process and must be passed to it or it
+  // will leak. In otherwords, do not early out on error between here and the
+  // sending of the RegisterGpuMemoryBuffer IPC below.
+  gfx::GpuMemoryBufferHandle handle =
+      channel_->ShareGpuMemoryBufferToGpuProcess(
+          gpu_memory_buffer->GetHandle());
+
+  if (!Send(new GpuCommandBufferMsg_RegisterGpuMemoryBuffer(
+                route_id_,
+                new_id,
+                handle,
+                width,
+                height,
+                internalformat))) {
+    return NULL;
+  }
+
+  *id = new_id;
+  gpu_memory_buffers_[new_id] = gpu_memory_buffer.get();
+  return gpu_memory_buffer.release();

[piman, 2013/10/21 21:57:00]

nit: these 2 lines would be a bit clearer if written something like:

gpu_memory_buffers_[new_id] = gpu_memory_buffer.release();
return gpu_memory_buffers_[new_id];

You're transferring ownership to the gpu_memory_buffers_ map. As you have it
written, it looks as if you're transferring ownership to the caller.

Have you considered using linked_ptr instead, that can be put it in a map?

[reveman, 2013/10/22 16:26:11]

switched to this in latest patch.
I think that's a good idea but for consistency we should also change the
TransferBufferMap at the same time and as this will touch a few files I prefer
to do it as a follow up.

 }
 
 void CommandBufferProxyImpl::DestroyGpuMemoryBuffer(int32 id) {
-  NOTREACHED();
+  if (last_state_.error != gpu::error::kNoError)
+    return;
+
+  // Remove the gpu memory buffer from the client side cache.
+  GpuMemoryBufferMap::iterator it = gpu_memory_buffers_.find(id);
+  if (it != gpu_memory_buffers_.end()) {
+    delete it->second;
+    gpu_memory_buffers_.erase(it);
+  }
+
+  Send(new GpuCommandBufferMsg_DestroyGpuMemoryBuffer(route_id_, id));
 }
 
 int CommandBufferProxyImpl::GetRouteID() const {