Add multi-planar functions to GpuMemoryBuffer

ui/gfx/gpu_memory_buffer.h

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef UI_GFX_GPU_MEMORY_BUFFER_H_
 #define UI_GFX_GPU_MEMORY_BUFFER_H_
 
 #include "base/memory/shared_memory.h"
 #include "build/build_config.h"
 #include "ui/gfx/gfx_export.h"
@@ skipping 43 matching lines @@
 
     FORMAT_LAST = BGRA_8888
   };
 
   // The usage mode affects how a buffer can be used. Only buffers created with
   // MAP can be mapped into the client's address space and accessed by the CPU.
   enum Usage { MAP, SCANOUT, USAGE_LAST = SCANOUT };
 
   virtual ~GpuMemoryBuffer() {}
 
-  // 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. Returns NULL
-  // on failure.
-  virtual void* Map() = 0;
+  // Maps each plane of the buffer into the client's address space so it can be
+  // written to by the CPU. A pointer to plane K is stored at index K-1 of the
+  // |data| array. This call may block, for instance if the GPU needs to finish
+  // accessing the buffer or if CPU caches need to be synchronized. Returns
+  // false on failure.
+  virtual bool Map(void** data) = 0;
 
   // 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.
   virtual bool IsMapped() const = 0;
 
+  // Returns the number of planes based on the format of the buffer.
+  virtual size_t GetNumberOfPlanes() const = 0;

[reveman, 2015/03/23 22:24:01]

I still think it's better to not have this function in the API. What if
GetNumberOfPlanes() doesn't match what you'd expect from a specific format? I'd
rather avoid this ambiguity in the API and simply have GetFormat() imply a
number of planes.

[emircan, 2015/03/24 16:52:27]

I see your point, but I don't see the case where it wouldn't match?
GpuMemoryBuffer is initialized with a "const format" and I am initializing a
"const num_planes" from it. So, format would always match the number.

On GpuMemoryBufferImpl child classes, I can call:
  DCHECK_EQ(num_planes_, 1u);
Where I have a GpuMemoryBuffer, I can call:
  DCHECK_EQ(buffer.GetNumberOfPlanes(), 1u);
If I had access to the format only I would need to call a static function, which
needs to be included in every file, that needs num_planes down the line:
DCHECK_EQ(GpuMemoryBufferImpl::NumberOfPlanesForGpuMemoryBufferFormat(buffer.GetFormat()),
1u); 

[reveman, 2015/03/24 20:13:59]

I think most of these DCHECKs are not needed. Either way, gfx::GpuMemoryBuffer
is the high level API that users of the GpuMemoryBuffer framework is exposed to.
It should be as simple and clean as possible and not include API that happens to
be useful for internal debugging purposes.

The point I was trying to make is that if the number of planes is implied from
the format then it's superfluous to also have a GetNumberOfPlanes() in the API.
The API should include everything necessary but nothing more. When there's two
ways of determining the number of planes, then the user of the API is forced to
choose a way and there's in theory a chance that these two different mechanisms
diverge. 

[emircan, 2015/03/24 21:43:44]

Done. Thanks for the explanation.

+
   // Returns the format for the buffer.
   virtual Format GetFormat() const = 0;
 
-  // Returns the stride in bytes for the buffer.
-  virtual uint32 GetStride() const = 0;
+  // Fills the stride in bytes for the each plane of the buffer. The stride of
+  // plane K is stored at index K-1 of the |stride| array. Returns
+  // false on failure.
+  virtual bool GetStride(uint32* stride) const = 0;

[reveman, 2015/03/23 22:24:01]

When would this fail? Please remove the return value if it can't fail.

[emircan, 2015/03/24 16:52:27]

GpuMemoryBufferImplSharedMemory::GetStride() calls
GpuMemoryBufferImpl::StrideInBytes() which returns a bool and may fail. 
GpuMemoryBufferImplOzoneNativeBuffer::GetStride() also fails. 
I am not keen on the usage of the bool either, but it looked like a better
option than filling the array with 0. What do you think?

[reveman, 2015/03/24 20:13:59]

GpuMemoryBufferImpl::StrideInBytes() having a return value is useful internally
but there's no need for a return value here.
GpuMemoryBufferImplSharedMemory::GetStride() can crash if StrideInBytes()
returns false.

GpuMemoryBufferImplOzoneNativeBuffer::GetStride() should never be called as it
has a NOTREACHED statement in the body. There's no need for it to return false.
This function should just keep the NOTREACHED statement and do nothing else.

[emircan, 2015/03/24 21:43:44]

Done.

 
   // Returns a platform specific handle for this buffer.
   virtual GpuMemoryBufferHandle GetHandle() const = 0;
 
   // Type-checking downcast routine.
   virtual ClientBuffer AsClientBuffer() = 0;
 };
 
 }  // namespace gfx
 
 #endif  // UI_GFX_GPU_MEMORY_BUFFER_H_