ozone: unify GpuMemoryBufferFactoryOzoneNativePixmap in content/

content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc

Index: content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
diff --git a/content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc b/content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
index e787be94aa3d27b6892d4428ceebdd83e8894e8d..8c985dbcd8c4e51717534ae790dae2c32b0c7e08 100644
--- a/content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
+++ b/content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.cc
@@ -4,8 +4,7 @@
 
 #include "content/common/gpu/gpu_memory_buffer_factory_ozone_native_pixmap.h"
 
-#include "base/logging.h"
-#include "ui/gl/gl_image.h"
+#include "ui/gl/gl_image_ozone_native_pixmap.h"
 #include "ui/ozone/public/ozone_platform.h"
 #include "ui/ozone/public/surface_factory_ozone.h"
 
@@ -16,6 +15,50 @@ const GpuMemoryBufferFactory::Configuration kSupportedConfigurations[] = {
     {gfx::GpuMemoryBuffer::BGRA_8888, gfx::GpuMemoryBuffer::SCANOUT},
     {gfx::GpuMemoryBuffer::RGBX_8888, gfx::GpuMemoryBuffer::SCANOUT}};
 
+ui::SurfaceFactoryOzone::BufferFormat GetOzoneFormatFor(
+    gfx::GpuMemoryBuffer::Format format) {
+  switch (format) {
+    case gfx::GpuMemoryBuffer::BGRA_8888:
+      return ui::SurfaceFactoryOzone::BGRA_8888;
+    case gfx::GpuMemoryBuffer::RGBX_8888:
+      return ui::SurfaceFactoryOzone::RGBX_8888;
+    case gfx::GpuMemoryBuffer::ATC:
+    case gfx::GpuMemoryBuffer::ATCIA:
+    case gfx::GpuMemoryBuffer::DXT1:
+    case gfx::GpuMemoryBuffer::DXT5:
+    case gfx::GpuMemoryBuffer::ETC1:
+    case gfx::GpuMemoryBuffer::R_8:
+    case gfx::GpuMemoryBuffer::RGBA_4444:
+    case gfx::GpuMemoryBuffer::RGBA_8888:
+    case gfx::GpuMemoryBuffer::YUV_420:
+      NOTREACHED();
+      return ui::SurfaceFactoryOzone::BGRA_8888;
+  }
+
+  NOTREACHED();
+  return ui::SurfaceFactoryOzone::BGRA_8888;
+}
+
+ui::SurfaceFactoryOzone::BufferUsage GetOzoneUsageFor(
+    gfx::GpuMemoryBuffer::Usage usage) {
+  switch (usage) {
+    case gfx::GpuMemoryBuffer::MAP:
+      return ui::SurfaceFactoryOzone::MAP;
+    case gfx::GpuMemoryBuffer::PERSISTENT_MAP:
+      return ui::SurfaceFactoryOzone::PERSISTENT_MAP;
+    case gfx::GpuMemoryBuffer::SCANOUT:
+      return ui::SurfaceFactoryOzone::SCANOUT;
+  }
+
+  NOTREACHED();
+  return ui::SurfaceFactoryOzone::MAP;
+}
+
+std::pair<uint32_t, uint32_t> GetIndex(gfx::GpuMemoryBufferId id,
+                                       int client_id) {
+  return std::pair<uint32_t, uint32_t>(id, client_id);
+}

[reveman, 2015/07/24 19:13:17]

nit: remove this function in favor of just using "NativePixmapMapKey key(id,
client_id)" below for consistency with other implementations

[dshwang, 2015/07/24 19:57:20]

Done.

+
 }  // namespace
 
 GpuMemoryBufferFactoryOzoneNativePixmap::
@@ -57,10 +100,19 @@ GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer(
     gfx::GpuMemoryBuffer::Usage usage,
     int client_id,
     gfx::PluginWindowHandle surface_handle) {
-  if (!ozone_native_pixmap_factory_.CreateGpuMemoryBuffer(
-          id, size, format, usage, client_id, surface_handle)) {
+  scoped_refptr<ui::NativePixmap> pixmap =
+      ui::OzonePlatform::GetInstance()
+          ->GetSurfaceFactoryOzone()
+          ->CreateNativePixmap(surface_handle, size, GetOzoneFormatFor(format),
+                               GetOzoneUsageFor(usage));
+  if (!pixmap.get()) {
+    LOG(ERROR) << "Failed to create pixmap " << size.width() << "x"
+               << size.height() << " format " << format << ", usage " << usage;
     return gfx::GpuMemoryBufferHandle();
   }
+  base::AutoLock lock(native_pixmap_map_lock_);
+  native_pixmap_map_[GetIndex(id, client_id)] = pixmap;

[reveman, 2015/07/24 19:13:17]

nit: add DCHECK to ensure that pixmap with this key doesn't already exist

[dshwang, 2015/07/24 19:57:20]

Done.

+
   gfx::GpuMemoryBufferHandle handle;
   handle.type = gfx::OZONE_NATIVE_PIXMAP;
   handle.id = id;
@@ -70,7 +122,8 @@ GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer(
 void GpuMemoryBufferFactoryOzoneNativePixmap::DestroyGpuMemoryBuffer(
     gfx::GpuMemoryBufferId id,
     int client_id) {
-  ozone_native_pixmap_factory_.DestroyGpuMemoryBuffer(id, client_id);
+  base::AutoLock lock(native_pixmap_map_lock_);
+  native_pixmap_map_.erase(GetIndex(id, client_id));

[reveman, 2015/07/24 19:13:17]

nit: add DCHECK to ensure that pixmap with this key exist

[dshwang, 2015/07/24 19:57:20]

Done.

 }
 
 gpu::ImageFactory* GpuMemoryBufferFactoryOzoneNativePixmap::AsImageFactory() {
@@ -85,8 +138,17 @@ GpuMemoryBufferFactoryOzoneNativePixmap::CreateImageForGpuMemoryBuffer(
     unsigned internalformat,
     int client_id) {
   DCHECK_EQ(handle.type, gfx::OZONE_NATIVE_PIXMAP);
-  return ozone_native_pixmap_factory_.CreateImageForGpuMemoryBuffer(
-      handle.id, size, format, internalformat, client_id);
+  ui::NativePixmap* pixmap = nullptr;

[piman, 2015/07/24 18:41:25]

You need to keep a reference here. Otherwise, as soon as you release the lock,
another thread could remove the entry and release the last reference, and you
have a dangling pointer.

[reveman, 2015/07/24 19:13:17]

Maybe fix this by keeping the lock while we create the image as that's what
other implementations do.

[dshwang, 2015/07/24 19:57:20]

good point. changed to "scoped_refptr<ui::NativePixmap> pixmap;"
scoped_refptr<ui::NativePixmap> is enough because "class NativePixmap : public
base::RefCountedThreadSafe<NativePixmap>"
|native_pixmap_map_lock_| is for |native_pixmap_map_|, not NativePixmap.

+  {
+    base::AutoLock lock(native_pixmap_map_lock_);
+    BufferToPixmapMap::iterator it =
+        native_pixmap_map_.find(GetIndex(handle.id, client_id));
+    if (it == native_pixmap_map_.end()) {
+      return scoped_refptr<gfx::GLImage>();
+    }
+    pixmap = it->second.get();
+  }
+  return gfx::CreateImageForNativePixmap(pixmap, size, format, internalformat);
 }
 
 }  // namespace content