Add R_8 GPU memory buffers format.

Add R_8 GPU memory buffers format.

GpuMemoryBuffer::Format::R_8 should be used to represent textures with
only one channel, the associated image should have GL_R8/GL_R8_EXT
as internal format.

With this CL the only implementation of GpuMemoryBuffer that supports
R_8 is GLImageMemory, it expects a 4 bytes row alignment (4 is the
default value for GL_PACK_ALIGNMENT).

BUG=

Committed: https://crrev.com/51f60b27d0a7c8f1d39674fec24f73e9b55f04e6
Cr-Commit-Position: refs/heads/master@{#324281}

[Daniele Castagna, 2015-04-01 02:06:20]

dcastagna@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2015-04-01 13:14:34]

You also need to update
content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc. Add this new
format as a supported configuration. That will make existing unit tests exercise
the code.

You also need to update GL_CHROMIUM_image.txt to reflect these changes.

Also, you say in the description that stride is expected to be aligned to 4
bytes. That's not really true. An implementation should be allowed to return
whatever stride it prefers from the gfx::GpuMemoryBuffer::GetStride function.

Would also be nice to test this format in our gl_tests
(GpuMemoryBufferTest.Lifecycle). See
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (left):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:110: NOTREACHED();
I like to keep this condition. ie detect if format becomes an invalid value at
runtime instead of silently returning |width| as stride. I would prefer if the
base::CheckedNumeric was moved into only the cases where needed as I think that
makes it more clear when those checks are needed.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:86: checked_stride =
(checked_stride + 3) % 4;
I assume this is a typo and should be "(width + 3) & ~0x3"

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:89: case
GpuMemoryBuffer::DXT5:  // 'checked_stride' is the same as 'width'.
nit: no need for checked_stride in this case

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:94: DCHECK_EQ(width % 2,
0U);
nit: ..., 0u);

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:95: checked_stride /= 2;
no need for checked_stride in this case.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:8: #include <vector>
?

https://codereview.chromium.org/1051503003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/image_factory.cc (left):

https://codereview.chromium.org/1051503003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/image_factory.cc:89: NOTREACHED();
please keep this NOTREACHED

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_linux_dm...
File ui/gl/gl_image_linux_dma_buffer.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_linux_dm...
ui/gl/gl_image_linux_dma_buffer.cc:65: return format ==
gfx::GpuMemoryBuffer::R_8;
this is not really true yet, is it?

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (left):

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:188: if (!ValidInternalFormat(internalformat_)) {
Please make this function consistent with GLImageLinuxDMABuffer::Initialize.

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:20: bool ValidateFormats(gfx::GpuMemoryBuffer::Format
format,
Please make this function consistent with the gl_image_linux_dma_buffer
function.

bool ValidFormat(unsigned internalformat, gfx::GpuMemoryBuffer::Format format) {
...

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:71: switch (format) {
can you move this switch (format) statement into a switch (internalformat)
statement to make it more clear what the expected values of internalformat is
not just for R_8?

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:99: unsigned internalformat) {
hm, I'm not a fan of passing internalformat to this function as it should not be
needed to determine the DataFormat. Maybe we can split these functions into
something like this:

GLenum TextureFormat(gfx::GpuMemoryBuffer::Format format) {
  // As before this patch.
}

GLenum InternalTextureFormat(unsigned internalformat,
gfx::GpuMemoryBuffer::Format format) {
  switch (internalformat) {
    case GL_RGB:
    case GL_RGBA:
      return TextureFormat(format);
    case GL_R8:
    case GL_LUMINANCE:
     return internalformat;
  }
  ...
}

GLenum DataFormat(gfx::GpuMemoryBuffer::Format format) {
  switch (format) {
    case gfx::GpuMemoryBuffer::ATC:
    ...
    case gfx::GpuMemoryBuffer::BGRA_8888:
      return TextureFormat(format);
    case gfx::GpuMemoryBuffer::R_8:
      return GL_RED;
  }
}

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:161: bool GLImageMemory::StrideInBytes(size_t width,
Just make sure this function is consistent with
GpuMemoryBufferImpl::StrideInBytes after making adjustments there..

https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:7: #include
<utility>
?

[Daniele Castagna, 2015-04-01 21:57:42]

On 2015/04/01 at 13:14:34, reveman wrote:
> You also need to update
content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc. Add this new
format as a supported configuration. That will make existing unit tests exercise
the code.
> 
> You also need to update GL_CHROMIUM_image.txt to reflect these changes.
> 
> Also, you say in the description that stride is expected to be aligned to 4
bytes. That's not really true. An implementation should be allowed to return
whatever stride it prefers from the gfx::GpuMemoryBuffer::GetStride function.
> 
> Would also be nice to test this format in our gl_tests
(GpuMemoryBufferTest.Lifecycle). See
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc.
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_memory_buffer_impl.cc (left):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.cc:110: NOTREACHED();
> I like to keep this condition. ie detect if format becomes an invalid value at
runtime instead of silently returning |width| as stride. I would prefer if the
base::CheckedNumeric was moved into only the cases where needed as I think that
makes it more clear when those checks are needed.
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.cc:86: checked_stride =
(checked_stride + 3) % 4;
> I assume this is a typo and should be "(width + 3) & ~0x3"
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.cc:89: case
GpuMemoryBuffer::DXT5:  // 'checked_stride' is the same as 'width'.
> nit: no need for checked_stride in this case
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.cc:94: DCHECK_EQ(width % 2,
0U);
> nit: ..., 0u);
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.cc:95: checked_stride /= 2;
> no need for checked_stride in this case.
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
> content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:8: #include
<vector>
> ?
> 
>
https://codereview.chromium.org/1051503003/diff/20001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/image_factory.cc (left):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/image_factory.cc:89: NOTREACHED();
> please keep this NOTREACHED
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_linux_dm...
> File ui/gl/gl_image_linux_dma_buffer.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_linux_dm...
> ui/gl/gl_image_linux_dma_buffer.cc:65: return format ==
gfx::GpuMemoryBuffer::R_8;
> this is not really true yet, is it?
> 
> https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.cc
> File ui/gl/gl_image_memory.cc (left):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:188: if (!ValidInternalFormat(internalformat_)) {
> Please make this function consistent with GLImageLinuxDMABuffer::Initialize.
> 
> https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.cc
> File ui/gl/gl_image_memory.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:20: bool ValidateFormats(gfx::GpuMemoryBuffer::Format
format,
> Please make this function consistent with the gl_image_linux_dma_buffer
function.
> 
> bool ValidFormat(unsigned internalformat, gfx::GpuMemoryBuffer::Format format)
{ ...
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:71: switch (format) {
> can you move this switch (format) statement into a switch (internalformat)
statement to make it more clear what the expected values of internalformat is
not just for R_8?
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:99: unsigned internalformat) {
> hm, I'm not a fan of passing internalformat to this function as it should not
be needed to determine the DataFormat. Maybe we can split these functions into
something like this:
> 
> GLenum TextureFormat(gfx::GpuMemoryBuffer::Format format) {
>   // As before this patch.
> }
> 
> GLenum InternalTextureFormat(unsigned internalformat,
gfx::GpuMemoryBuffer::Format format) {
>   switch (internalformat) {
>     case GL_RGB:
>     case GL_RGBA:
>       return TextureFormat(format);
>     case GL_R8:
>     case GL_LUMINANCE:
>      return internalformat;
>   }
>   ...
> }
> 
> GLenum DataFormat(gfx::GpuMemoryBuffer::Format format) {
>   switch (format) {
>     case gfx::GpuMemoryBuffer::ATC:
>     ...
>     case gfx::GpuMemoryBuffer::BGRA_8888:
>       return TextureFormat(format);
>     case gfx::GpuMemoryBuffer::R_8:
>       return GL_RED;
>   }
> }
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:161: bool GLImageMemory::StrideInBytes(size_t width,
> Just make sure this function is consistent with
GpuMemoryBufferImpl::StrideInBytes after making adjustments there..
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
> File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
> ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:7: #include
<utility>
> ?

Thank you for the review!
PTAL.

- R_8 format has been added to
content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc and R_8 is now
tested.
- LUMINANCE and R8 have been added to GL_CHROMIUM_image.txt as allowed values
for 'internalformat'.
- The CL description has been reworded to make it clear that the expected
alignment to 4 bytes is only for a specific GpuMemoryBuffer implementation.
- GpuMemoryBufferTest.Lifecycle now tests R_8 in addition to RGBA_8888.

[Daniele Castagna, 2015-04-01 21:58:35]

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (left):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:110: NOTREACHED();
On 2015/04/01 at 13:14:33, reveman wrote:
> I like to keep this condition. ie detect if format becomes an invalid value at
runtime instead of silently returning |width| as stride. I would prefer if the
base::CheckedNumeric was moved into only the cases where needed as I think that
makes it more clear when those checks are needed.

Done as you suggested.

For the sake of arguing: I think leaving CheckedNumeric everywhere clearly
conveys the idea that 'width' is not to be trusted and has to be validated.

Good point about NOTREACHED(). Isn't 'format' potentially coming from the
renderer (from ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer) as for 'width'?
Should we validate it and return false instead of asserting?

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:86: checked_stride =
(checked_stride + 3) % 4;
On 2015/04/01 at 13:14:33, reveman wrote:
> I assume this is a typo and should be "(width + 3) & ~0x3"

Ops, I was trying to break some test to see if this code was tested anywhere
with the new enum and I left it in. After adding R_8 to
GpuMemoryBufferFactorySharedMemory::GetSupportedGpuMemoryBufferConfigurations
the GpuMemoryBufferFactoryTest correctly started to fail.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:89: case
GpuMemoryBuffer::DXT5:  // 'checked_stride' is the same as 'width'.
On 2015/04/01 at 13:14:33, reveman wrote:
> nit: no need for checked_stride in this case

Done.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:94: DCHECK_EQ(width % 2,
0U);
On 2015/04/01 at 13:14:33, reveman wrote:
> nit: ..., 0u);

Done.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:95: checked_stride /= 2;
On 2015/04/01 at 13:14:33, reveman wrote:
> no need for checked_stride in this case.

Done.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:8: #include <vector>
On 2015/04/01 at 13:14:33, reveman wrote:
> ?

std::vector is used in this cc file, the linter was complaining about it.

https://codereview.chromium.org/1051503003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/image_factory.cc (left):

https://codereview.chromium.org/1051503003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/image_factory.cc:89: NOTREACHED();
On 2015/04/01 at 13:14:33, reveman wrote:
> please keep this NOTREACHED

Done.

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_linux_dm...
File ui/gl/gl_image_linux_dma_buffer.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_linux_dm...
ui/gl/gl_image_linux_dma_buffer.cc:65: return format ==
gfx::GpuMemoryBuffer::R_8;
On 2015/04/01 at 13:14:33, reveman wrote:
> this is not really true yet, is it?

Done.

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (left):

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:188: if (!ValidInternalFormat(internalformat_)) {
On 2015/04/01 at 13:14:33, reveman wrote:
> Please make this function consistent with GLImageLinuxDMABuffer::Initialize.

Done.

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:20: bool ValidateFormats(gfx::GpuMemoryBuffer::Format
format,
On 2015/04/01 at 13:14:33, reveman wrote:
> Please make this function consistent with the gl_image_linux_dma_buffer
function.
> 
> bool ValidFormat(unsigned internalformat, gfx::GpuMemoryBuffer::Format format)
{ ...

Done.

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:71: switch (format) {
On 2015/04/01 at 13:14:33, reveman wrote:
> can you move this switch (format) statement into a switch (internalformat)
statement to make it more clear what the expected values of internalformat is
not just for R_8?

Split in two functions TextureFormat and InternalTextureFormat.

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:99: unsigned internalformat) {
On 2015/04/01 at 13:14:33, reveman wrote:
> hm, I'm not a fan of passing internalformat to this function as it should not
be needed to determine the DataFormat. Maybe we can split these functions into
something like this:
> 
> GLenum TextureFormat(gfx::GpuMemoryBuffer::Format format) {
>   // As before this patch.
> }
> 
> GLenum InternalTextureFormat(unsigned internalformat,
gfx::GpuMemoryBuffer::Format format) {
>   switch (internalformat) {
>     case GL_RGB:
>     case GL_RGBA:
>       return TextureFormat(format);
>     case GL_R8:
>     case GL_LUMINANCE:
>      return internalformat;
>   }
>   ...
> }
> 
> GLenum DataFormat(gfx::GpuMemoryBuffer::Format format) {
>   switch (format) {
>     case gfx::GpuMemoryBuffer::ATC:
>     ...
>     case gfx::GpuMemoryBuffer::BGRA_8888:
>       return TextureFormat(format);
>     case gfx::GpuMemoryBuffer::R_8:
>       return GL_RED;
>   }
> }

I'm afraid DataFormat needs to know about internalformat_.
DataFormat should return GL_RED if the internal format is GL_R8 (ARB_texture_rg
available) and GL_LUMINANCE if GL_LUMINANCE.

https://codereview.chromium.org/1051503003/diff/20001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:161: bool GLImageMemory::StrideInBytes(size_t width,
On 2015/04/01 at 13:14:33, reveman wrote:
> Just make sure this function is consistent with
GpuMemoryBufferImpl::StrideInBytes after making adjustments there..

If by "make sure this function is consistent" you mean: "remember to copy and
paste". :)

Done.

https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:7: #include
<utility>
On 2015/04/01 at 13:14:33, reveman wrote:
> ?

Similar to what happened above with vector, the linter was complaining. In this
cc std::pair is used therefore utility should be included.

[reveman, 2015-04-01 23:38:55]

We also need to verify both on the client and service side that ARB_texture_rg
is only used when available. Is GL_LUMINANCE is always available?

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (left):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:110: NOTREACHED();
On 2015/04/01 at 21:58:34, Daniele Castagna wrote:
> On 2015/04/01 at 13:14:33, reveman wrote:
> > I like to keep this condition. ie detect if format becomes an invalid value
at runtime instead of silently returning |width| as stride. I would prefer if
the base::CheckedNumeric was moved into only the cases where needed as I think
that makes it more clear when those checks are needed.
> 
> Done as you suggested.
> 
> For the sake of arguing: I think leaving CheckedNumeric everywhere clearly
conveys the idea that 'width' is not to be trusted and has to be validated.

Ok, fair enough.

> 
> Good point about NOTREACHED(). Isn't 'format' potentially coming from the
renderer (from ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer) as for 'width'?
Should we validate it and return false instead of asserting?

The IPC system will automatically verify that format is a valid enum value as it
arrives. This NOTREACHED is only in case we mess it up in the GPU process after
the IPC arrives.

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:8: #include <vector>
On 2015/04/01 at 21:58:34, Daniele Castagna wrote:
> On 2015/04/01 at 13:14:33, reveman wrote:
> > ?
> 
> std::vector is used in this cc file, the linter was complaining about it.

ok, cool. thanks!

https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):

https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:7: #include
<utility>
On 2015/04/01 at 21:58:35, Daniele Castagna wrote:
> On 2015/04/01 at 13:14:33, reveman wrote:
> > ?
> 
> Similar to what happened above with vector, the linter was complaining. In
this cc std::pair is used therefore utility should be included.

thanks

https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc:43: if
(!GpuMemoryBufferImpl::StrideInBytes(size.width(), format,
I had to stare at this for a few seconds to see that it's just a style change.
Can we avoid it in this patch?

https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt (right):

https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:16: 
Please add this line here:

"ARB_texture_rg affects the definition of this extension."

and this after the "New Procedures and Functions" section:

"
Dependencies on ARB_texture_rg

    If ARB_texture_rg is not supported:
     * delete any reference to the R8 format.
"

https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:66: 10/7/2014   Remove
map/unmap API.
And an entry here

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:89: int
imageBytesPerPixel = GetParam() == gfx::GpuMemoryBuffer::R_8 ? 1 : 4;
Can you refactor this test a bit so this conditional is not needed? Maybe just
memset(mapped_buffer + y * stride, 0xff, ..) instead

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:104: if (GetParam() ==
gfx::GpuMemoryBuffer::R_8) {
I prefer a switch statement where you explicitly handle all formats. and I guess
you can NOTREACHED if the format is invalid even though this is just a test...

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:137: static const
gfx::GpuMemoryBuffer::Format kMemoryBufferFormats[] = {
It would be nice to test all formats here and not just R_8 and RGBA_8888. How
about landing a patch that refactor this test to try all formats prior to this
CL?

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:139:
gfx::GpuMemoryBuffer::R_8};
nit: use the same order as where the enum is defined

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:145:
kMemoryBufferFormats + arraysize(kMemoryBufferFormats))));
How about:

INSTANTIATE_TEST_CASE_P(
    GpuMemoryBufferTests,
    GpuMemoryBufferTest,
    ::testing::Values(gfx::GpuMemoryBuffer::R_8,
                      gfx::GpuMemoryBuffer::RGBA_8888))

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (left):

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:85: 
heh, you don't like my use of blank lines? fine, I'll add them back next time I
touch this file :)

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:20: bool ValidFormat(unsigned internalformat,
gfx::GpuMemoryBuffer::Format format) {
why not two levels of switch statements like GLImageLinuxDMABuffer?

switch (internalformat) {
  case GL_RGBA:
    ...
    switch (format) {
      case ...:

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:30: LOG(ERROR) << "Invalid internal format " <<
internalformat
I prefer if you keep logging in GLImageMemory::Initialize to be consistent with
other GLImage implementations such as GLImageLinuxDMABuffer.

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:106: GLenum DataFormat(unsigned internalformat,
hm, I think the need for internalformat here is a good indication that we should
add a gfx::GpuMemoryBuffer::L_8 format. I think that would make the code a bit
cleaner. Wdyt?

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:108: GLenum data_format =
InternalTextureFormat(internalformat, format);
If we add gfx::GpuMemoryBuffer::L_8 then we can use a switch statement here and
TextureFormat instead of InternalTextureFormat.

[Daniele Castagna, 2015-04-02 23:22:08]

Patchset #4 (id:60001) has been deleted

[Daniele Castagna, 2015-04-02 23:44:58]

Patchset #4 (id:80001) has been deleted

[Daniele Castagna, 2015-04-03 00:58:45]

Patchset #4 (id:100001) has been deleted

[Daniele Castagna, 2015-04-03 01:07:13]

On 2015/04/01 at 23:38:55, reveman wrote:
> We also need to verify both on the client and service side that ARB_texture_rg
is only used when available. Is GL_LUMINANCE is always available?
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_memory_buffer_impl.cc (left):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.cc:110: NOTREACHED();
> On 2015/04/01 at 21:58:34, Daniele Castagna wrote:
> > On 2015/04/01 at 13:14:33, reveman wrote:
> > > I like to keep this condition. ie detect if format becomes an invalid
value at runtime instead of silently returning |width| as stride. I would prefer
if the base::CheckedNumeric was moved into only the cases where needed as I
think that makes it more clear when those checks are needed.
> > 
> > Done as you suggested.
> > 
> > For the sake of arguing: I think leaving CheckedNumeric everywhere clearly
conveys the idea that 'width' is not to be trusted and has to be validated.
> 
> Ok, fair enough.
> 
> > 
> > Good point about NOTREACHED(). Isn't 'format' potentially coming from the
renderer (from ChildProcessHostMsg_SyncAllocateGpuMemoryBuffer) as for 'width'?
Should we validate it and return false instead of asserting?
> 
> The IPC system will automatically verify that format is a valid enum value as
it arrives. This NOTREACHED is only in case we mess it up in the GPU process
after the IPC arrives.
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/content/common/gpu/gpu_...
> content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:8: #include
<vector>
> On 2015/04/01 at 21:58:34, Daniele Castagna wrote:
> > On 2015/04/01 at 13:14:33, reveman wrote:
> > > ?
> > 
> > std::vector is used in this cc file, the linter was complaining about it.
> 
> ok, cool. thanks!
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
> File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/20001/ui/ozone/gpu/gpu_memory...
> ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:7: #include
<utility>
> On 2015/04/01 at 21:58:35, Daniele Castagna wrote:
> > On 2015/04/01 at 13:14:33, reveman wrote:
> > > ?
> > 
> > Similar to what happened above with vector, the linter was complaining. In
this cc std::pair is used therefore utility should be included.
> 
> thanks
> 
>
https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
> content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc:43: if
(!GpuMemoryBufferImpl::StrideInBytes(size.width(), format,
> I had to stare at this for a few seconds to see that it's just a style change.
Can we avoid it in this patch?
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
> File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt (right):
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:16: 
> Please add this line here:
> 
> "ARB_texture_rg affects the definition of this extension."
> 
> and this after the "New Procedures and Functions" section:
> 
> "
> Dependencies on ARB_texture_rg
> 
>     If ARB_texture_rg is not supported:
>      * delete any reference to the R8 format.
> "
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
> gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:66: 10/7/2014   Remove
map/unmap API.
> And an entry here
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
> File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
> gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:89: int
imageBytesPerPixel = GetParam() == gfx::GpuMemoryBuffer::R_8 ? 1 : 4;
> Can you refactor this test a bit so this conditional is not needed? Maybe just
memset(mapped_buffer + y * stride, 0xff, ..) instead
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
> gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:104: if (GetParam()
== gfx::GpuMemoryBuffer::R_8) {
> I prefer a switch statement where you explicitly handle all formats. and I
guess you can NOTREACHED if the format is invalid even though this is just a
test...
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
> gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:137: static const
gfx::GpuMemoryBuffer::Format kMemoryBufferFormats[] = {
> It would be nice to test all formats here and not just R_8 and RGBA_8888. How
about landing a patch that refactor this test to try all formats prior to this
CL?
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
> gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:139:
gfx::GpuMemoryBuffer::R_8};
> nit: use the same order as where the enum is defined
> 
>
https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
> gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:145:
kMemoryBufferFormats + arraysize(kMemoryBufferFormats))));
> How about:
> 
> INSTANTIATE_TEST_CASE_P(
>     GpuMemoryBufferTests,
>     GpuMemoryBufferTest,
>     ::testing::Values(gfx::GpuMemoryBuffer::R_8,
>                       gfx::GpuMemoryBuffer::RGBA_8888))
> 
> https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.cc
> File ui/gl/gl_image_memory.cc (left):
> 
>
https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:85: 
> heh, you don't like my use of blank lines? fine, I'll add them back next time
I touch this file :)
> 
> https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.cc
> File ui/gl/gl_image_memory.cc (right):
> 
>
https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:20: bool ValidFormat(unsigned internalformat,
gfx::GpuMemoryBuffer::Format format) {
> why not two levels of switch statements like GLImageLinuxDMABuffer?
> 
> switch (internalformat) {
>   case GL_RGBA:
>     ...
>     switch (format) {
>       case ...:
> 
>
https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:30: LOG(ERROR) << "Invalid internal format " <<
internalformat
> I prefer if you keep logging in GLImageMemory::Initialize to be consistent
with other GLImage implementations such as GLImageLinuxDMABuffer.
> 
>
https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:106: GLenum DataFormat(unsigned internalformat,
> hm, I think the need for internalformat here is a good indication that we
should add a gfx::GpuMemoryBuffer::L_8 format. I think that would make the code
a bit cleaner. Wdyt?
> 
>
https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
> ui/gl/gl_image_memory.cc:108: GLenum data_format =
InternalTextureFormat(internalformat, format);
> If we add gfx::GpuMemoryBuffer::L_8 then we can use a switch statement here
and TextureFormat instead of InternalTextureFormat.

Please take a look.
Added validation for internal formats in service/in_process_command_buffer.cc,
client/command_buffer_proxy_impl.cc and gpu/gpu_command_buffer_stub.cc. Is there
any other place where we should validate it?

GL_LUMINACE has been deprecated in GL3.
On desktop OpenGL we create a context with the compatibility profile and it
should be available, but GL_RED should be preferred in that case.

[Daniele Castagna, 2015-04-03 01:08:15]

https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc:43: if
(!GpuMemoryBufferImpl::StrideInBytes(size.width(), format,
On 2015/04/01 at 23:38:55, reveman wrote:
> I had to stare at this for a few seconds to see that it's just a style change.
Can we avoid it in this patch?

Sure, this was done by 'git cl format'.
It's a little bit inconvenient not being able to run 'git cl format', or to have
to track down all the diff sections and revert some after it runs. :/

https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt (right):

https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:16: 
On 2015/04/01 at 23:38:55, reveman wrote:
> Please add this line here:
> 
> "ARB_texture_rg affects the definition of this extension."
> 

There is also EXT_texture_rg with its R8_EXT that has the same value as R8.
Should we mention that?

> and this after the "New Procedures and Functions" section:
> 
> "
> Dependencies on ARB_texture_rg
> 
>     If ARB_texture_rg is not supported:
>      * delete any reference to the R8 format.
> "

This is not completely true. R8 is available after OpengGL ES 3 even if there is
no extension.

https://codereview.chromium.org/1051503003/diff/40001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:66: 10/7/2014   Remove
map/unmap API.
On 2015/04/01 at 23:38:55, reveman wrote:
> And an entry here

Done.

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:89: int
imageBytesPerPixel = GetParam() == gfx::GpuMemoryBuffer::R_8 ? 1 : 4;
On 2015/04/01 at 23:38:55, reveman wrote:
> Can you refactor this test a bit so this conditional is not needed? Maybe just
memset(mapped_buffer + y * stride, 0xff, ..) instead

Refactored, refer to crrev.com/1055143003.

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:104: if (GetParam() ==
gfx::GpuMemoryBuffer::R_8) {
On 2015/04/01 at 23:38:55, reveman wrote:
> I prefer a switch statement where you explicitly handle all formats. and I
guess you can NOTREACHED if the format is invalid even though this is just a
test...

This will change after crrev.com/1055143003

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:137: static const
gfx::GpuMemoryBuffer::Format kMemoryBufferFormats[] = {
On 2015/04/01 at 23:38:55, reveman wrote:
> It would be nice to test all formats here and not just R_8 and RGBA_8888. How
about landing a patch that refactor this test to try all formats prior to this
CL?

crrev.com/1055143003

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:139:
gfx::GpuMemoryBuffer::R_8};
On 2015/04/01 at 23:38:55, reveman wrote:
> nit: use the same order as where the enum is defined

Refer to crrev.com/1055143003

https://codereview.chromium.org/1051503003/diff/40001/gpu/command_buffer/test...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:145:
kMemoryBufferFormats + arraysize(kMemoryBufferFormats))));
On 2015/04/01 at 23:38:55, reveman wrote:
> How about:
> 
> INSTANTIATE_TEST_CASE_P(
>     GpuMemoryBufferTests,
>     GpuMemoryBufferTest,
>     ::testing::Values(gfx::GpuMemoryBuffer::R_8,
>                       gfx::GpuMemoryBuffer::RGBA_8888))

Much better.
Used it in crrev.com/1055143003

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (left):

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:85: 
On 2015/04/01 at 23:38:55, reveman wrote:
> heh, you don't like my use of blank lines? fine, I'll add them back next time
I touch this file :)

As discussed on IM, I was trying to minimize vertical space as suggested in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Vertical_Whit....

Putting it back.

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:20: bool ValidFormat(unsigned internalformat,
gfx::GpuMemoryBuffer::Format format) {
On 2015/04/01 at 23:38:55, reveman wrote:
> why not two levels of switch statements like GLImageLinuxDMABuffer?
> 
> switch (internalformat) {
>   case GL_RGBA:
>     ...
>     switch (format) {
>       case ...:

Done.

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:30: LOG(ERROR) << "Invalid internal format " <<
internalformat
On 2015/04/01 at 23:38:55, reveman wrote:
> I prefer if you keep logging in GLImageMemory::Initialize to be consistent
with other GLImage implementations such as GLImageLinuxDMABuffer.

Done.

In this way the error message can't be as specific as in the previous patch
though.

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:106: GLenum DataFormat(unsigned internalformat,
On 2015/04/01 at 23:38:55, reveman wrote:
> hm, I think the need for internalformat here is a good indication that we
should add a gfx::GpuMemoryBuffer::L_8 format. I think that would make the code
a bit cleaner. Wdyt?

LUMINANCE and LUMINANCE_ALPHA have been deprecated in OpengGL 3.0:
https://www.opengl.org/wiki/Image_Format#Legacy_Image_Formats


I think it'd be better to leave this here rather than exposing a deprecated
format in GpuMemoryBuffers.

https://codereview.chromium.org/1051503003/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:108: GLenum data_format =
InternalTextureFormat(internalformat, format);
On 2015/04/01 at 23:38:55, reveman wrote:
> If we add gfx::GpuMemoryBuffer::L_8 then we can use a switch statement here
and TextureFormat instead of InternalTextureFormat.

See previous comment.

[Daniele Castagna, 2015-04-07 22:43:52]

PTAL. All the references to LUMINANCE have been removed as per discussion IRL.

[reveman, 2015-04-07 23:28:03]

lgtm with nits

https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc:43: if
(!GpuMemoryBufferImpl::StrideInBytes(size.width(), format,
On 2015/04/03 01:08:15, Daniele Castagna wrote:
> On 2015/04/01 at 23:38:55, reveman wrote:
> > I had to stare at this for a few seconds to see that it's just a style
change.
> Can we avoid it in this patch?
> 
> Sure, this was done by 'git cl format'.
> It's a little bit inconvenient not being able to run 'git cl format', or to
have
> to track down all the diff sections and revert some after it runs. :/

You should be able to run git cl format as it won't fix style of code that you
didn't change. Are you running it with -f for full file?

https://codereview.chromium.org/1051503003/diff/180001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (left):

https://codereview.chromium.org/1051503003/diff/180001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:245: 
nit: typically a blank line between code and closing namespace bracket in
chromium

https://codereview.chromium.org/1051503003/diff/180001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/180001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.cc:162: case GpuMemoryBuffer::R_8:
nit: I prefer if you save gfx:: prefix removal to a different patch that removes
it everywhere in all gl_image* files

[Daniele Castagna, 2015-04-08 01:00:28]

https://codereview.chromium.org/1051503003/diff/180001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (left):

https://codereview.chromium.org/1051503003/diff/180001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:245: 
On 2015/04/07 at 23:28:03, reveman wrote:
> nit: typically a blank line between code and closing namespace bracket in
chromium

Done.

https://codereview.chromium.org/1051503003/diff/180001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1051503003/diff/180001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.cc:162: case GpuMemoryBuffer::R_8:
On 2015/04/07 at 23:28:03, reveman wrote:
> nit: I prefer if you save gfx:: prefix removal to a different patch that
removes it everywhere in all gl_image* files

Done.

[reveman, 2015-04-08 02:57:27]

reveman@chromium.org changed reviewers:
+ alexst@chromium.org, piman@chromium.org

[reveman, 2015-04-08 02:57:28]

still lgtm but let's land it after https://codereview.chromium.org/1062853002

+piman for gpu/
+alexst for ui/ozone/

https://codereview.chromium.org/1051503003/diff/200001/content/common/gpu/cli...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1051503003/diff/200001/content/common/gpu/cli...
content/common/gpu/client/gpu_memory_buffer_impl.cc:85: case
GpuMemoryBuffer::R_8:
hm, doesn't really matter as we'll clean this up in a follow up but this is now
adding part of the prefix to a function where it was not previously used.
intentional?

https://codereview.chromium.org/1051503003/diff/200001/gpu/GLES2/extensions/C...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt (right):

https://codereview.chromium.org/1051503003/diff/200001/gpu/GLES2/extensions/C...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:80: 4/6/2015    Add BGRA_EXT
format.
thanks for cleaning this up

https://codereview.chromium.org/1051503003/diff/200001/gpu/command_buffer/ser...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1051503003/diff/200001/gpu/command_buffer/ser...
gpu/command_buffer/service/in_process_command_buffer.cc:717:
gpu_memory_buffer->GetFormat(), capabilities_));
thanks

https://codereview.chromium.org/1051503003/diff/200001/gpu/command_buffer/tes...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc (right):

https://codereview.chromium.org/1051503003/diff/200001/gpu/command_buffer/tes...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittest.cc:164:
ASSERT_TRUE((GetParam() != gfx::GpuMemoryBuffer::R_8) ||
We should consider adding a histogram to find out how common it is that this is
missing in the wild. This patch does that:
https://codereview.chromium.org/1068133002

[piman, 2015-04-08 05:05:10]

lgtm

[alexst (slow to review), 2015-04-08 12:22:28]

lgtm

[Daniele Castagna, 2015-04-08 18:24:37]

Patchset #9 (id:220001) has been deleted

[Daniele Castagna, 2015-04-08 18:36:33]

Patchset #9 (id:240001) has been deleted

[Daniele Castagna, 2015-04-08 19:02:09]

Patchset #10 (id:280001) has been deleted

[Daniele Castagna, 2015-04-08 19:14:58]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2015-04-08 19:14:59]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
reveman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1051503003/#ps260001
(title: "Merged on master.")

[Daniele Castagna, 2015-04-08 19:16:00]

https://codereview.chromium.org/1051503003/diff/200001/content/common/gpu/cli...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1051503003/diff/200001/content/common/gpu/cli...
content/common/gpu/client/gpu_memory_buffer_impl.cc:85: case
GpuMemoryBuffer::R_8:
On 2015/04/08 at 02:57:28, reveman wrote:
> hm, doesn't really matter as we'll clean this up in a follow up but this is
now adding part of the prefix to a function where it was not previously used.
intentional?

Not intentional, fixed.

[commit-bot: I haz the power, 2015-04-08 19:17:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1051503003/260001

[commit-bot: I haz the power, 2015-04-08 20:53:08]

Committed patchset #9 (id:260001)

[commit-bot: I haz the power, 2015-04-08 20:53:56]

Patchset 9 (id:??) landed as
https://crrev.com/51f60b27d0a7c8f1d39674fec24f73e9b55f04e6
Cr-Commit-Position: refs/heads/master@{#324281}