ui: Cleanup gfx::GpuMemoryBuffer interface.

Fix description of interface and improve the comments in general.

Clang format all *gpu_memory_buffer* files.

This also make the interface a bit easier to use by not having Map()
unnecessarily use a parameter for return value.

Also add myself to ui/gfx/OWNERS and content/common/gpu/OWNERS

BUG=

Review URL: https://codereview.chromium.org/216333004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@260566 0039d316-1c4b-4281-b951-d872f2087c98

android_webview/browser/gpu_memory_buffer_factory_impl.cc

@@ -31,8 +31,7 @@ class GpuMemoryBufferImpl : public gfx::GpuMemoryBuffer {
   }
 
   // Overridden from gfx::GpuMemoryBuffer:
-  virtual void Map(gfx::GpuMemoryBuffer::AccessMode mode,
-                   void** vaddr) OVERRIDE {
+  virtual void* Map(gfx::GpuMemoryBuffer::AccessMode mode) OVERRIDE {
     AwMapMode map_mode = MAP_READ_ONLY;
     switch (mode) {
       case GpuMemoryBuffer::READ_ONLY:
@@ -47,9 +46,11 @@ class GpuMemoryBufferImpl : public gfx::GpuMemoryBuffer {
       default:
         LOG(DFATAL) << "Unknown map mode: " << mode;
     }
-    int err = g_gl_draw_functions->map(buffer_id_, map_mode, vaddr);
+    void* vaddr = NULL;
+    int err = g_gl_draw_functions->map(buffer_id_, map_mode, &vaddr);
     DCHECK(!err);
     mapped_ = true;
+    return vaddr;
   }
   virtual void Unmap() OVERRIDE {
     int err = g_gl_draw_functions->unmap(buffer_id_);

content/common/gpu/OWNERS

@@ -2,6 +2,9 @@ kbr@chromium.org
 piman@chromium.org
 sievers@chromium.org
 
+# GPU memory buffer implementations.
+per-file *gpu_memory_buffer*=reveman@chromium.org
+
 # For security review of IPC message files.
 per-file *_messages*.h=set noparent
 per-file *_messages*.h=cdn@chromium.org

content/common/gpu/client/gpu_memory_buffer_impl.cc

@@ -8,16 +8,13 @@
 
 namespace content {
 
-GpuMemoryBufferImpl::GpuMemoryBufferImpl(
-    gfx::Size size, unsigned internalformat)
-    : size_(size),
-      internalformat_(internalformat),
-      mapped_(false) {
+GpuMemoryBufferImpl::GpuMemoryBufferImpl(gfx::Size size,
+                                         unsigned internalformat)
+    : size_(size), internalformat_(internalformat), mapped_(false) {
   DCHECK(IsFormatValid(internalformat));
 }
 
-GpuMemoryBufferImpl::~GpuMemoryBufferImpl() {
-}
+GpuMemoryBufferImpl::~GpuMemoryBufferImpl() {}
 
 // static
 bool GpuMemoryBufferImpl::IsFormatValid(unsigned internalformat) {
@@ -42,9 +39,7 @@ size_t GpuMemoryBufferImpl::BytesPerPixel(unsigned internalformat) {
   }
 }
 
-bool GpuMemoryBufferImpl::IsMapped() const {
-  return mapped_;
-}
+bool GpuMemoryBufferImpl::IsMapped() const { return mapped_; }
 
 uint32 GpuMemoryBufferImpl::GetStride() const {
   return size_.width() * BytesPerPixel(internalformat_);

content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc

@@ -11,14 +11,14 @@
 namespace content {
 
 GpuMemoryBufferImplIOSurface::GpuMemoryBufferImplIOSurface(
-    gfx::Size size, unsigned internalformat)
+    gfx::Size size,
+    unsigned internalformat)
     : GpuMemoryBufferImpl(size, internalformat),
       io_surface_support_(IOSurfaceSupport::Initialize()) {
   CHECK(io_surface_support_);
 }
 
-GpuMemoryBufferImplIOSurface::~GpuMemoryBufferImplIOSurface() {
-}
+GpuMemoryBufferImplIOSurface::~GpuMemoryBufferImplIOSurface() {}
 
 // static
 bool GpuMemoryBufferImplIOSurface::IsFormatSupported(unsigned internalformat) {
@@ -52,11 +52,11 @@ bool GpuMemoryBufferImplIOSurface::Initialize(
   return true;
 }
 
-void GpuMemoryBufferImplIOSurface::Map(AccessMode mode, void** vaddr) {
+void* GpuMemoryBufferImplIOSurface::Map(AccessMode mode) {
   DCHECK(!mapped_);
   io_surface_support_->IOSurfaceLock(io_surface_, 0, NULL);
-  *vaddr = io_surface_support_->IOSurfaceGetBaseAddress(io_surface_);
   mapped_ = true;
+  return io_surface_support_->IOSurfaceGetBaseAddress(io_surface_);
 }
 
 void GpuMemoryBufferImplIOSurface::Unmap() {

content/common/gpu/client/gpu_memory_buffer_impl_io_surface.h

@@ -25,7 +25,7 @@ class GpuMemoryBufferImplIOSurface : public GpuMemoryBufferImpl {
   bool Initialize(gfx::GpuMemoryBufferHandle handle);
 
   // Overridden from gfx::GpuMemoryBuffer:
-  virtual void Map(AccessMode mode, void** vaddr) OVERRIDE;
+  virtual void* Map(AccessMode mode) OVERRIDE;
   virtual void Unmap() OVERRIDE;
   virtual uint32 GetStride() const OVERRIDE;
   virtual gfx::GpuMemoryBufferHandle GetHandle() const OVERRIDE;

content/common/gpu/client/gpu_memory_buffer_impl_shm.cc

@@ -8,13 +8,11 @@
 
 namespace content {
 
-GpuMemoryBufferImplShm::GpuMemoryBufferImplShm(
-    gfx::Size size, unsigned internalformat)
-    : GpuMemoryBufferImpl(size, internalformat) {
-}
+GpuMemoryBufferImplShm::GpuMemoryBufferImplShm(gfx::Size size,
+                                               unsigned internalformat)
+    : GpuMemoryBufferImpl(size, internalformat) {}
 
-GpuMemoryBufferImplShm::~GpuMemoryBufferImplShm() {
-}
+GpuMemoryBufferImplShm::~GpuMemoryBufferImplShm() {}
 
 bool GpuMemoryBufferImplShm::Initialize(gfx::GpuMemoryBufferHandle handle) {
   if (!base::SharedMemory::IsHandleValid(handle.handle))
@@ -31,13 +29,12 @@ bool GpuMemoryBufferImplShm::InitializeFromSharedMemory(
   return true;
 }
 
-void GpuMemoryBufferImplShm::Map(AccessMode mode, void** vaddr) {
+void* GpuMemoryBufferImplShm::Map(AccessMode mode) {
   DCHECK(!mapped_);
-  *vaddr = NULL;
   if (!shared_memory_->Map(size_.GetArea() * BytesPerPixel(internalformat_)))
-    return;
-  *vaddr = shared_memory_->memory();
+    return NULL;
   mapped_ = true;
+  return shared_memory_->memory();
 }
 
 void GpuMemoryBufferImplShm::Unmap() {

content/common/gpu/client/gpu_memory_buffer_impl_shm.h

@@ -17,11 +17,10 @@ class GpuMemoryBufferImplShm : public GpuMemoryBufferImpl {
   virtual ~GpuMemoryBufferImplShm();
 
   bool Initialize(gfx::GpuMemoryBufferHandle handle);
-  bool InitializeFromSharedMemory(
-      scoped_ptr<base::SharedMemory> shared_memory);
+  bool InitializeFromSharedMemory(scoped_ptr<base::SharedMemory> shared_memory);
 
   // Overridden from gfx::GpuMemoryBuffer:
-  virtual void Map(AccessMode mode, void** vaddr) OVERRIDE;
+  virtual void* Map(AccessMode mode) OVERRIDE;
   virtual void Unmap() OVERRIDE;
   virtual gfx::GpuMemoryBufferHandle GetHandle() const OVERRIDE;
 

content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.cc

@@ -65,7 +65,7 @@ bool GpuMemoryBufferImplSurfaceTexture::Initialize(
   return true;
 }
 
-void GpuMemoryBufferImplSurfaceTexture::Map(AccessMode mode, void** vaddr) {
+void* GpuMemoryBufferImplSurfaceTexture::Map(AccessMode mode) {
   TRACE_EVENT0("gpu", "GpuMemoryBufferImplSurfaceTexture::Map");
 
   DCHECK(!mapped_);
@@ -74,14 +74,13 @@ void GpuMemoryBufferImplSurfaceTexture::Map(AccessMode mode, void** vaddr) {
   int status = ANativeWindow_lock(native_window_, &buffer, NULL);
   if (status) {
     VLOG(1) << "ANativeWindow_lock failed with error code: " << status;
-    *vaddr = NULL;
-    return;
+    return NULL;
   }
 
   DCHECK_LE(size_.width(), buffer.stride);
-  *vaddr = buffer.bits;
   stride_ = buffer.stride * BytesPerPixel(internalformat_);
   mapped_ = true;
+  return buffer.bits;
 }
 
 void GpuMemoryBufferImplSurfaceTexture::Unmap() {

content/common/gpu/client/gpu_memory_buffer_impl_surface_texture.h

@@ -23,7 +23,7 @@ class GpuMemoryBufferImplSurfaceTexture : public GpuMemoryBufferImpl {
   static int WindowFormat(unsigned internalformat);
 
   // Overridden from gfx::GpuMemoryBuffer:
-  virtual void Map(AccessMode mode, void** vaddr) OVERRIDE;
+  virtual void* Map(AccessMode mode) OVERRIDE;
   virtual void Unmap() OVERRIDE;
   virtual gfx::GpuMemoryBufferHandle GetHandle() const OVERRIDE;
   virtual uint32 GetStride() const OVERRIDE;

gpu/command_buffer/client/gles2_implementation.cc

@@ -4003,9 +4003,7 @@ void* GLES2Implementation::MapImageCHROMIUMHelper(GLuint image_id,
     return NULL;
   }
 
-  void* mapped_buffer = NULL;
-  gpu_buffer->Map(mode, &mapped_buffer);
-  return mapped_buffer;
+  return gpu_buffer->Map(mode);
 }
 
 void* GLES2Implementation::MapImageCHROMIUM(

gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc

@@ -42,7 +42,7 @@ class MockGpuMemoryBuffer : public gfx::GpuMemoryBuffer {
     Die();
   }
 
-  MOCK_METHOD2(Map, void(gfx::GpuMemoryBuffer::AccessMode, void**));
+  MOCK_METHOD1(Map, void*(gfx::GpuMemoryBuffer::AccessMode));
   MOCK_METHOD0(Unmap, void());
   MOCK_CONST_METHOD0(IsMapped, bool());
   MOCK_CONST_METHOD0(GetStride, uint32());
@@ -149,9 +149,9 @@ TEST_F(MockGpuMemoryBufferTest, Lifecycle) {
   shared_memory.Map(bytes);
   EXPECT_TRUE(shared_memory.memory());
 
-  EXPECT_CALL(*gpu_memory_buffer, Map(_, _))
+  EXPECT_CALL(*gpu_memory_buffer, Map(gfx::GpuMemoryBuffer::READ_WRITE))
       .Times(1)
-      .WillOnce(SetArgPointee<1>(shared_memory.memory()))
+      .WillOnce(Return(shared_memory.memory()))
       .RetiresOnSaturation();
   uint8* mapped_buffer = static_cast<uint8*>(
       glMapImageCHROMIUM(image_id, GL_READ_WRITE));

ui/gfx/OWNERS

@@ -16,5 +16,8 @@ per-file transform*=shawnsingh@chromium.org
 per-file transform*=vollick@chromium.org
 per-file interpolated_transform*=vollick@chromium.org
 
+# GPU memory buffer interface.
+per-file gpu_memory_buffer*=reveman@chromium.org
+
 # If you're doing structural changes get a review from one of the OWNERS.
 per-file *.gyp*=*

ui/gfx/gpu_memory_buffer.h

@@ -39,10 +39,12 @@ struct GpuMemoryBufferHandle {
       : type(EMPTY_BUFFER),
         handle(base::SharedMemory::NULLHandle())
 #if defined(OS_MACOSX)
-        , io_surface_id(0)
+        ,
+        io_surface_id(0u)
 #endif
 #if defined(OS_ANDROID)
-        , native_buffer(NULL)
+        ,
+        native_buffer(NULL)
 #endif
   {
   }
@@ -58,33 +60,25 @@ struct GpuMemoryBufferHandle {
 #endif
 };
 
-// Interface for creating and accessing a zero-copy GPU memory buffer.
-// This design evolved from the generalization of GraphicBuffer API
-// of Android framework.
-//
-// THREADING CONSIDERATIONS:
-//
-// This interface is thread-safe. However, multiple threads mapping
-// a buffer for Write or ReadOrWrite simultaneously may result in undefined
-// behavior and is not allowed.
+// This interface typically correspond to a type of shared memory that is also
+// shared with the GPU. A GPU memory buffer can be written to directly by
+// regular CPU code, but can also be read by the GPU.
 class GFX_EXPORT GpuMemoryBuffer {
  public:
-  enum AccessMode {
-    READ_ONLY,
-    WRITE_ONLY,
-    READ_WRITE,
-  };
+  enum AccessMode { READ_ONLY, WRITE_ONLY, READ_WRITE };
 
   GpuMemoryBuffer();
   virtual ~GpuMemoryBuffer();
 
-  // Maps the buffer so the client can write the bitmap data in |*vaddr|
-  // subsequently. This call may block, for instance if the hardware needs
-  // to finish rendering or if CPU caches need to be synchronized.
-  virtual void Map(AccessMode mode, void** vaddr) = 0;
+  // Maps the buffer into the client's address space so it can be written to by
+  // the CPU. This call may block, for instance if the GPU needs to finish
+  // accessing the buffer or if CPU caches need to be synchronized. |mode|
+  // indicate how the client intends to use the mapped buffer. Returns NULL on
+  // failure.
+  virtual void* Map(AccessMode mode) = 0;
 
-  // Unmaps the buffer. Called after all changes to the buffer are
-  // completed.
+  // Unmaps the buffer. It's illegal to use the pointer returned by Map() after
+  // this has been called.
   virtual void Unmap() = 0;
 
   // Returns true iff the buffer is mapped.