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..5caefd2b94dec6246b2ff47589cc9b066dcd6471 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,45 @@ 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;
+}
+
 }  // namespace
 
 GpuMemoryBufferFactoryOzoneNativePixmap::
@@ -57,10 +95,24 @@ 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();

[piman, 2015/07/24 21:56:26]

nit: return nullptr

[dshwang, 2015/07/25 05:28:15]

thx, but this method return instance, not pointer :)

   }
+  base::AutoLock lock(native_pixmaps_lock_);
+  if (DCHECK_IS_ON()) {
+    auto it = native_pixmaps_.find(BufferToPixmapMapKey(id, client_id));
+    DCHECK(it == native_pixmaps_.end())
+        << "pixmap with this key must not exist";
+  }
+  native_pixmaps_[BufferToPixmapMapKey(id, client_id)] = pixmap;

[piman, 2015/07/24 21:56:26]

nit: a better way to write this is:
auto result = native_pixmaps_.insert(BufferToPixmapMapKey(id, client_id),
pixmap);
DCHECK(result.second);

That way:
- you don't need to default-construct + copy-assign (as [] does on a map),
instead copy-constructing in place.
- you don't need 2 lookups
- it's shorter

[dshwang, 2015/07/25 05:28:15]

we want to check if the pixmap already exist, instead of if insertion succeeds.

[piman, 2015/07/27 19:07:51]

Insertion fails if the pixmap already exists.

[dshwang, 2015/07/28 12:15:19]

aha, thank you for advice. Done.

+
   gfx::GpuMemoryBufferHandle handle;
   handle.type = gfx::OZONE_NATIVE_PIXMAP;
   handle.id = id;
@@ -70,7 +122,12 @@ GpuMemoryBufferFactoryOzoneNativePixmap::CreateGpuMemoryBuffer(
 void GpuMemoryBufferFactoryOzoneNativePixmap::DestroyGpuMemoryBuffer(
     gfx::GpuMemoryBufferId id,
     int client_id) {
-  ozone_native_pixmap_factory_.DestroyGpuMemoryBuffer(id, client_id);
+  base::AutoLock lock(native_pixmaps_lock_);
+  if (DCHECK_IS_ON()) {
+    auto it = native_pixmaps_.find(BufferToPixmapMapKey(id, client_id));
+    DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist";
+  }
+  native_pixmaps_.erase(BufferToPixmapMapKey(id, client_id));

[piman, 2015/07/24 21:56:26]

nit: similarly, you can do:

auto it = native_pixmaps_.find(BufferToPixmapMapKey(id, client_id));
DCHECK(it != native_pixmaps_.end()) << "pixmap with this key must exist";
native_pixmaps_.erase(it);

That way you don't have the double lookup, and it's shorter.

[dshwang, 2015/07/25 05:28:15]

got it. cool.

 }
 
 gpu::ImageFactory* GpuMemoryBufferFactoryOzoneNativePixmap::AsImageFactory() {
@@ -85,8 +142,18 @@ 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);
+  scoped_refptr<ui::NativePixmap> pixmap;
+  {
+    base::AutoLock lock(native_pixmaps_lock_);
+    BufferToPixmapMap::iterator it =
+        native_pixmaps_.find(BufferToPixmapMapKey(handle.id, client_id));
+    if (it == native_pixmaps_.end()) {
+      return scoped_refptr<gfx::GLImage>();

[piman, 2015/07/24 21:56:26]

nit: return nullptr

[dshwang, 2015/07/25 05:28:15]

Done.

+    }
+    pixmap = it->second;
+  }
+  return gfx::CreateImageForOzoneNativePixmap(pixmap, size, format,
+                                              internalformat);
 }
 
 }  // namespace content