content: Move type selection logic out of GpuMemoryBufferImpl.

content/common/gpu/client/gpu_memory_buffer_impl_android.cc

Index: content/common/gpu/client/gpu_memory_buffer_impl_android.cc
diff --git a/content/common/gpu/client/gpu_memory_buffer_impl_android.cc b/content/common/gpu/client/gpu_memory_buffer_impl_android.cc
index 9c0fdb368c67b02d9bb7ac985828663d7b6ccf16..cc82b06c41d3bd4f8f0d166b316caccd171a0422 100644
--- a/content/common/gpu/client/gpu_memory_buffer_impl_android.cc
+++ b/content/common/gpu/client/gpu_memory_buffer_impl_android.cc
@@ -4,36 +4,65 @@
 
 #include "content/common/gpu/client/gpu_memory_buffer_impl.h"
 
+#include "base/logging.h"
 #include "content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h"
 #include "content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.h"
 
 namespace content {
 
 // static
-void GpuMemoryBufferImpl::Create(gfx::GpuMemoryBufferId id,
+void GpuMemoryBufferImpl::GetSupportedTypes(
+    std::vector<gfx::GpuMemoryBufferType>* types) {
+  const gfx::GpuMemoryBufferType supported_types[] = {
+    gfx::SURFACE_TEXTURE_BUFFER,
+    gfx::SHARED_MEMORY_BUFFER
+  };
+  types->assign(supported_types, supported_types + arraysize(supported_types));
+}
+
+// static
+bool GpuMemoryBufferImpl::IsConfigurationSupported(
+    gfx::GpuMemoryBufferType type,
+    Format format,
+    Usage usage) {
+  switch (type) {
+    case gfx::SHARED_MEMORY_BUFFER:
+      return GpuMemoryBufferImplSharedMemory::IsFormatSupported(format) &&
+             GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage);
+    case gfx::SURFACE_TEXTURE_BUFFER:
+      return GpuMemoryBufferImplSurfaceTexture::IsFormatSupported(format) &&
+             GpuMemoryBufferImplSurfaceTexture::IsUsageSupported(usage);

[piman, 2014/11/11 22:38:30]

So, there's a lot of duplication between all the platform-specific files.

I wonder if we could have it all in gpu_memory_buffer_impl.cc, with a single
switch on all the types (with ifdefs if needed).

Actually, we're really aching for a factory pattern, because the mapping from
the enum to the GpuMemoryBufferImpl type is always the same in each of the
GpuMemoryBufferImpl static calls. So it would be nice to have a static map from
the enum to a per-type factory that implements
IsFormatSupported/IsUsageSupported/Create/AllocateForChildProcess/DeletedByChildProcess/CreateFromHandle

Then all the platform-specific implementation has to do is to provide the
GetSupportedTypes. The per-type implementation wouldn't change much
(IsFormatSupported and friends would be member methods of
GpuMemoryBufferImpl*::Factory instead of static methods of
GpuMemoryBufferImpl*), and all the switch logic would be replaced by a single
map (array?) lookup in the common GpuMemoryBufferImpl implementation.

WDYT?

[reveman, 2014/11/12 05:09:20]

I agree that we need a factory here. I don't think we need an array or map. The
manager can simply create factory instances for the preferred and the shared
memory type and keep track of those.

However, instead of introducing a new factory interface (we already have too
many GpuMemoryBuffer related classes) I'd like to find a way to make our
existing GpuMemoryBufferFactory class serve this purpose. This factory interface
is currently used to allocate buffers on the GPU service side but I feel like it
makes some sense to use this on the browser side too. The functions used by the
browser would be different than what's currently used by the GPU service. We
currently use the GpuMemoryBufferImpl API in both the renderer and the browser.
I'd like to keep whatever is used by the renderer (client) in
GpuMemoryBufferImpl (only CreateFromHandle) but move the rest to the
GpuMemoryBufferFactory interface. I think it makes more sense to share the
GpuMemoryBufferFactory interface between the browser and the GPU service than
the current sharing of the GpuMemoryBufferImpl interface between renderer and
browser. The browser and the GPU service are both taking part in the process of
allocating buffers after all. The browser allocation code in some cases just
need to talk to the factory instance in the GPU service side...

I'm going to implement this as a follow up to this patch. I'll wait to land this
patch until that's done if you prefer but it would be nice to land this for now
so we can start testing 1-copy on MacOS and Android in the meantime. Let me know
what you prefer.

+    default:
+      NOTREACHED();
+      return false;
+  }
+}
+
+// static
+void GpuMemoryBufferImpl::Create(gfx::GpuMemoryBufferType type,
+                                 gfx::GpuMemoryBufferId id,
                                  const gfx::Size& size,
                                  Format format,
                                  Usage usage,
                                  int client_id,
                                  const CreationCallback& callback) {
-  if (GpuMemoryBufferImplSurfaceTexture::IsConfigurationSupported(format,
-                                                                  usage)) {
-    GpuMemoryBufferImplSurfaceTexture::Create(
-        id, size, format, client_id, callback);
-    return;
-  }
-
-  if (GpuMemoryBufferImplSharedMemory::IsConfigurationSupported(
-          size, format, usage)) {
-    GpuMemoryBufferImplSharedMemory::Create(id, size, format, callback);
-    return;
+  switch (type) {
+    case gfx::SHARED_MEMORY_BUFFER:
+      GpuMemoryBufferImplSharedMemory::Create(id, size, format, callback);
+      break;
+    case gfx::SURFACE_TEXTURE_BUFFER:
+      GpuMemoryBufferImplSurfaceTexture::Create(
+          id, size, format, client_id, callback);
+      break;
+    default:
+      NOTREACHED();
+      break;
   }
-
-  callback.Run(scoped_ptr<GpuMemoryBufferImpl>());
 }
 
 // static
 void GpuMemoryBufferImpl::AllocateForChildProcess(
+    gfx::GpuMemoryBufferType type,
     gfx::GpuMemoryBufferId id,
     const gfx::Size& size,
     Format format,
@@ -41,21 +70,19 @@ void GpuMemoryBufferImpl::AllocateForChildProcess(
     base::ProcessHandle child_process,
     int child_client_id,
     const AllocationCallback& callback) {
-  if (GpuMemoryBufferImplSurfaceTexture::IsConfigurationSupported(format,
-                                                                  usage)) {
-    GpuMemoryBufferImplSurfaceTexture::AllocateForChildProcess(
-        id, size, format, child_client_id, callback);
-    return;
-  }
-
-  if (GpuMemoryBufferImplSharedMemory::IsConfigurationSupported(
-          size, format, usage)) {
-    GpuMemoryBufferImplSharedMemory::AllocateForChildProcess(
-        id, size, format, child_process, callback);
-    return;
+  switch (type) {
+    case gfx::SHARED_MEMORY_BUFFER:
+      GpuMemoryBufferImplSharedMemory::AllocateForChildProcess(
+          id, size, format, child_process, callback);
+      break;
+    case gfx::SURFACE_TEXTURE_BUFFER:
+      GpuMemoryBufferImplSurfaceTexture::AllocateForChildProcess(
+          id, size, format, child_client_id, callback);
+      break;
+    default:
+      NOTREACHED();
+      break;
   }
-
-  callback.Run(gfx::GpuMemoryBufferHandle());
 }
 
 // static
@@ -92,6 +119,7 @@ scoped_ptr<GpuMemoryBufferImpl> GpuMemoryBufferImpl::CreateFromHandle(
       return GpuMemoryBufferImplSurfaceTexture::CreateFromHandle(
           handle, size, format, callback);
     default:
+      NOTREACHED();
       return scoped_ptr<GpuMemoryBufferImpl>();
   }
 }