gpu: Refactor GpuMemoryBuffer framework for multi-process support.

gpu/command_buffer/service/command_buffer_service.cc

Index: gpu/command_buffer/service/command_buffer_service.cc
diff --git a/gpu/command_buffer/service/command_buffer_service.cc b/gpu/command_buffer/service/command_buffer_service.cc
index 1cd68f53ea4f2737bbd26993df91bbfba990ed05..2b8627f03c41aa765dd15c8726d7cf3a7e12f397 100644
--- a/gpu/command_buffer/service/command_buffer_service.cc
+++ b/gpu/command_buffer/service/command_buffer_service.cc
@@ -6,18 +6,24 @@
 
 #include <limits>
 
+#include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/debug/trace_event.h"
+#include "gpu/command_buffer/client/gpu_memory_buffer_factory.h"
 #include "gpu/command_buffer/common/cmd_buffer_common.h"
 #include "gpu/command_buffer/common/command_buffer_shared.h"
+#include "gpu/command_buffer/service/image_manager.h"
 #include "gpu/command_buffer/service/transfer_buffer_manager.h"
+#include "ui/gl/gl_image.h"
 
 using ::base::SharedMemory;
 
 namespace gpu {
 
 CommandBufferService::CommandBufferService(
-    TransferBufferManagerInterface* transfer_buffer_manager)
+    TransferBufferManagerInterface* transfer_buffer_manager,
+    gles2::ImageManager* image_manager,
+    GpuMemoryBufferFactory* gpu_memory_buffer_factory)
     : ring_buffer_id_(-1),
       shared_state_(NULL),
       num_entries_(0),
@@ -27,7 +33,9 @@ CommandBufferService::CommandBufferService(
       token_(0),
       generation_(0),
       error_(error::kNoError),
-      context_lost_reason_(error::kUnknown) {
+      context_lost_reason_(error::kUnknown),
+      gpu_memory_buffer_factory_(gpu_memory_buffer_factory),
+      image_manager_(image_manager) {
 }
 
 CommandBufferService::~CommandBufferService() {

[piman, 2013/08/01 21:19:30]

I think if the client isn't well behaved (or crashes), you'll have leftover
buffers in gpu_memory_buffers_ that need deletion here.

[reveman, 2013/08/08 23:19:00]

Done.

@@ -207,4 +215,76 @@ uint32 CommandBufferService::InsertSyncPoint() {
   return 0;
 }
 
+gfx::GpuMemoryBuffer* CommandBufferService::CreateGpuMemoryBuffer(
+    size_t width,
+    size_t height,
+    unsigned internalformat,
+    int32* id) {
+  *id = -1;
+
+  CHECK(gpu_memory_buffer_factory_) << "No GPU memory buffer factory provided";
+  scoped_ptr<gfx::GpuMemoryBuffer> buffer = make_scoped_ptr(

[piman, 2013/08/01 21:19:30]

nit: you could use linked_ptr and put that linked_ptr in the map too so that you
could avoid potential leaks (see above).

[reveman, 2013/08/08 23:19:00]

Done.

+      gpu_memory_buffer_factory_->CreateGpuMemoryBuffer(width,
+                                                        height,
+                                                        internalformat));
+  if (!buffer)

[kaanb, 2013/07/31 23:44:05]

buffer.get()

[reveman, 2013/08/01 13:32:18]

scoped_ptr implements operator Testable() and can be used in boolean
expressions.

+    return NULL;
+
+  static int32 next_id = 1;

[piman, 2013/08/01 21:19:30]

Does this ID need to be shared between contexts?

[reveman, 2013/08/08 23:19:00]

Not before but it does in latest patch. Is this a problem?

+  *id = next_id++;

[no sievers, 2013/08/01 00:38:04]

Does it make sense to establish this 1:1 relationship between GLImage and
GpuMemoryBuffer, and basically saying this is the only way a GL client can use
GL images?

Should we then maybe rather have a GLImageClient interface with GpuMemoryBuffers
being an impl of that (one that allows you to map/unmap a GLImage into the cpu
address space)?

[reveman, 2013/08/01 13:32:18]

I don't necessarily think we are. We can add another set of Create/Register
functions for a different type of GLImage.
This is what the GpuMemoryBuffers interface is at the moment. We could rename it
and even make it more generic in the future if that makes sense. I don't think
we need to do that yet.

If/when we add additional GLImage types that can be created from the renderer,
we should evaluate if they fit into the existing GpuMemoryBuffer category or
should use something new. I think it depends on a number of factors, such as can
the GLImage storage be mapped and how is it serialized.

There should always be 1:1 mapping between client interface and GLImage
instantiate function on the service side. ie.
GLImage::CreateGLImageFromGpuMemoryBuffer and GpuMemoryBuffer. 

+
+  if (!RegisterGpuMemoryBuffer(*id,
+                               buffer->GetHandle(),
+                               width,
+                               height,
+                               internalformat)) {
+    *id = -1;
+    return NULL;
+  }
+
+  gpu_memory_buffers_[*id] = buffer.get();
+  return buffer.release();
+}
+
+void CommandBufferService::DestroyGpuMemoryBuffer(int32 id) {
+  GpuMemoryBufferMap::iterator it = gpu_memory_buffers_.find(id);
+  if (it != gpu_memory_buffers_.end()) {
+    delete it->second;
+    gpu_memory_buffers_.erase(it);
+  }
+
+  image_manager_->RemoveImage(id);
+}
+
+gfx::GLImage* CommandBufferService::GetImage(int32 id) {
+  return image_manager_->LookupImage(id);
+}
+
+bool CommandBufferService::RegisterGpuMemoryBuffer(
+    int32 id,
+    gfx::GpuMemoryBufferHandle buffer,
+    size_t width,
+    size_t height,
+    unsigned internalformat) {
+  if (id <= 0) {
+    DVLOG(0) << "Cannot register GPU memory buffer with non-positive ID.";
+    return false;
+  }
+
+  if (image_manager_->LookupImage(id)) {
+    DVLOG(0) << "GPU memory buffer ID already in use.";
+    return false;
+  }
+
+  scoped_refptr<gfx::GLImage> gl_image =
+      gfx::GLImage::CreateGLImageForGpuMemoryBuffer(buffer,
+                                                    gfx::Size(width, height),
+                                                    internalformat);
+  if (!gl_image)
+    return false;
+
+  image_manager_->AddImage(gl_image.get(), id);

[piman, 2013/08/01 21:19:30]

Mmh, this is the id generated locally for the GpuMemoryBuffer. How does that
interact with the GpuMsg_CreateImage path, where ids get generated client-side?
It seems like a high potential for conflicts.

[reveman, 2013/08/08 23:19:00]

Yes, these will conflict but shouldn't be a problem until we add real
multi-process support and use it together with GpuMsg_CreateImage. This should
be easy to fix by having GpuMsg_CreateImage return an id instead.

+  return true;
+}
+
 }  // namespace gpu