GPU client side changes for GpuMemoryBuffers

GPU client side changes for GpuMemoryBuffers:
- Introduces a new GL extension CHROMIUM_map_image that contains Create/Destroy/Map/Unmap/GetImageParameteriv methods.
- A new data structure called GpuMemoryBufferTracker to track these buffers and images on the client side.

BUG=175012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200505

[kaanb1, 2013-04-25 22:46:12]

Friendly ping!

[jamesr, 2013-04-25 23:24:14]

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2040:
GetNativeHandleForBoundGpuMemoryBuffer()));
this really seems like voodoo. are there no better entry points?

[kaanb1, 2013-04-25 23:27:59]

On 2013/04/25 23:24:14, jamesr wrote:
>
https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
> gpu/command_buffer/client/gles2_implementation.cc:2040:
> GetNativeHandleForBoundGpuMemoryBuffer()));
> this really seems like voodoo. are there no better entry points?

Unfortunately I couldn't find anything better. Again here the tradeoff was
between adding a new public API vs. reusing an existing entry point.

[apatrick_chromium, 2013-04-25 23:39:03]

Can you add a spec to gpu\GLES2\extensions\CHROMIUM?

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2040:
GetNativeHandleForBoundGpuMemoryBuffer()));
On 2013/04/25 23:24:15, jamesr wrote:
> this really seems like voodoo. are there no better entry points?

Agreed. I had a "wat?" moment.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gpu...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gpu...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:13: : buffers_() {
buffers_() is redundant

[kaanb1, 2013-04-25 23:42:19]

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2040:
GetNativeHandleForBoundGpuMemoryBuffer()));
On 2013/04/25 23:39:03, apatrick_chromium wrote:
> On 2013/04/25 23:24:15, jamesr wrote:
> > this really seems like voodoo. are there no better entry points?
> 
> Agreed. I had a "wat?" moment.

Would you be okay if I added a new public API that returns a GpuMemoryBuffer
pointer?

[greggman, 2013-04-25 23:45:33]

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:637: case
GL_BOUND_GPU_MEMORY_BUFFER_CHROMIUM:
Seems like following the other ENUMs this should be

  GL_IMAGE_BUFFER_BINDING_CHROMIUM

To match the target GL_IMAGE_BUFFER_CHROMIUM

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:1255: bool
GLES2Implementation::GetBoundGpuMemoryBuffer(
This function always returns true. Did you mean it to return false if it
generates an error?

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2038: if (name ==
GL_BOUND_GPU_MEMORY_BUFFER_CHROMIUM_POINTER) {
Doesn't this seem like a kind of abuse of this function?

How about adding some other function? Maybe either
glGetMemoryBufferPointerCHROMIUM

Or

void glGetPointervCHROMIUM(GLenum pname, GLvoid** pointer);

Which you can call like this

  GLvoid* buffer = NULL;
  glGetPointervCHROMIUM(
      GL_IMAGE_BUFFER_POINTER_CHROMIUM, &buffer);

I'd prefer the second as it can be used for other things later. What do you
think?

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2052: "GL_CHROMIUM_map_sub "
You should probably add a string here based on which GpuMemoryBufferFactory
you've attached

If a factory exists then append the string 

  "GL_CHROMIUM_gpu_memory_buffer"

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3437:
GetBoundGpuMemoryBuffer("glMapBufferCHROMIUM", &buffer_id)) {
If GetBoundGPUMemoryBuffer returns false (assuming you meant it to return falls
on error), then it just falls through to GetBoundPixelBuffer. Is that correct?

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3466: gpu_buffer->Map(mode,
&mapped_buffer);
The semantics of map buffer require that you can only map once, otherwise
generate GL_INVALID_OPERATION

In other words calling glMapBuffer twice for the same buffer should fail. The
buffer must be unmapped before mapping it again.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3507:
GetBoundGpuMemoryBuffer("glUnmapBufferCHROMIUM", &buffer_id)) {
Same here. If GetBoundGpuMemroyBuffer returns false (assuming it's supposed to
return false) then it doesn't seem like you want it to fall through?

Actually, looking at the fall through works but it seems fragile to me. Maybe
I'm being dumb.

I'd think it might be clearer if the code here at in MapBuffer was more like
either

   switch (target) {
     case GL_IMAGE_BUFFER_CHROMIUM:
       ...
       break;
     case GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
     case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
       ...
       break;
     default:
       SetGLError(..., INVALID_ENUM, ...)

Or at least

   if (target == GL_IMAGE_BUFFER_CHROMIUM) {
     ...

     return;  // don't let the code follow through since it can't
              // possibly be interested in anything since we 
              // know the target is incorrect for any code below.
   }

What do you think? I think I'd prefer the switch as far as readability goes.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3515: gpu_buffer->Unmap();
The semantics of map buffer require if the buffer is not mapped this should
generate GL_INVALID_OPERATION.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.h:19: #include
"../client/gpu_memory_buffer_tracker.h"
Can you just forward declare GPUMemoryBufferTracker?

Looks like some of the other trackers/manager should change to using forward
declarations as well. I think some of them used to be instances instead of
scoped_ptr

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:34: class
GLGpuMemoryBufferTest : public testing::Test {
I'm conflicted on these tests. The test in gpu/command_buffer/tests are supposed
to be functional tests. That means they are supposed to call into the actual
driver. These ones don't call into the actual driver because they're using a
GpuMemoryBufferMock.

Can you move these to gles2_implementation_unittests.cc

If you want you can keep these tests too but remove the mock and use a real
factory.

Change the code to check glGetString (like some of the other tests do) to check
that your extension exists. Then make the tests actually expect to work, not
just mock.

[no sievers, 2013-04-26 00:17:43]

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2040:
GetNativeHandleForBoundGpuMemoryBuffer()));
On 2013/04/25 23:42:19, kaanb1 wrote:
> On 2013/04/25 23:39:03, apatrick_chromium wrote:
> > On 2013/04/25 23:24:15, jamesr wrote:
> > > this really seems like voodoo. are there no better entry points?
> > 
> > Agreed. I had a "wat?" moment.
> 
> Would you be okay if I added a new public API that returns a GpuMemoryBuffer
> pointer? 
 
You don't need a full-fledged GL API. This is only used from inside WGC3DImpl
and your test. Both can get to the GLES2Implementation (the test can get it
through GLManager). So just expose it from GLES2Implementation only.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:34: class
GLGpuMemoryBufferTest : public testing::Test {
On 2013/04/25 23:45:34, greggman wrote:
> I'm conflicted on these tests. The test in gpu/command_buffer/tests are
supposed
> to be functional tests. That means they are supposed to call into the actual
> driver. These ones don't call into the actual driver because they're using a
> GpuMemoryBufferMock.
> 
> Can you move these to gles2_implementation_unittests.cc
> 
> If you want you can keep these tests too but remove the mock and use a real
> factory.
> 
> Change the code to check glGetString (like some of the other tests do) to
check
> that your extension exists. Then make the tests actually expect to work, not
> just mock.

Here is the dilemna: We can't run the real test upstream because of dependencies
on Android (this is for WebVIew) on the actual GpuMemoryBuffer impl.

I like the readability of this test that uses the gl*() APIs and only cares
about how gles2_implementation creates and calls into other objects.

But so obviously it should be a gles2 impl unittest then. But would be nice it
could be pretty much copied 1:1 and not be cluttered with setting up cmdbuffer
expectations.

You think we can somehow set up a new test class with faked/ignored cmdbuffer
and helper calls and is part of gpu_unittests?

[reveman, 2013-04-26 02:47:56]

I think glImageBufferDataCHROMIUM should be separate from
GL_IMAGE_BUFFER_CHROMIUM extension and have a different name. GLImage and
GL_IMAGE_BUFFER_CHROMIUM allows us to reuse as much API and code as possible
between 0-copy implementations but it might be best to use a unique entry point
for image allocation to properly be able to support more than one runtime
implementation.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:68:
TEST_F(GLGpuMemoryBufferTest, Lifecycle) {
The code below doesn't look right to me. The glGetString() stuff is really
abusing the api.

Should glImageBufferDataCHROMIUM be implemented at a higher level
(WebGraphicsContext3DInProcessCommandBufferImpl)? At this level I would expect
you to just call gpu_memory_buffer_tracker_->CreateBuffer(buffer_id, width,
height) and create a GLImage from that. Calling glImageBufferDataCHROMIUM here
and then having dig up a gfx::GpuMemoryBufferHandle seem backwards.

[kaanb1, 2013-04-26 03:01:20]

All done except unittests.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:637: case
GL_BOUND_GPU_MEMORY_BUFFER_CHROMIUM:
On 2013/04/25 23:45:34, greggman wrote:
> Seems like following the other ENUMs this should be
> 
>   GL_IMAGE_BUFFER_BINDING_CHROMIUM
> 
> To match the target GL_IMAGE_BUFFER_CHROMIUM

Done.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:1255: bool
GLES2Implementation::GetBoundGpuMemoryBuffer(
On 2013/04/25 23:45:34, greggman wrote:
> This function always returns true. Did you mean it to return false if it
> generates an error?
GetBoundPixelTransferBuffer only returns false when called with a target it
doesn't know about. Since we don't take target as a parameter here it isn't
applicable. Are there any other error situations?

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2038: if (name ==
GL_BOUND_GPU_MEMORY_BUFFER_CHROMIUM_POINTER) {
On 2013/04/25 23:45:34, greggman wrote:
> Doesn't this seem like a kind of abuse of this function?
> 
> How about adding some other function? Maybe either
> glGetMemoryBufferPointerCHROMIUM
> 
> Or
> 
> void glGetPointervCHROMIUM(GLenum pname, GLvoid** pointer);
> 
> Which you can call like this
> 
>   GLvoid* buffer = NULL;
>   glGetPointervCHROMIUM(
>       GL_IMAGE_BUFFER_POINTER_CHROMIUM, &buffer);
> 
> I'd prefer the second as it can be used for other things later. What do you
> think?

Done.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2040:
GetNativeHandleForBoundGpuMemoryBuffer()));
On 2013/04/26 00:17:43, Daniel Sievers wrote:
> On 2013/04/25 23:42:19, kaanb1 wrote:
> > On 2013/04/25 23:39:03, apatrick_chromium wrote:
> > > On 2013/04/25 23:24:15, jamesr wrote:
> > > > this really seems like voodoo. are there no better entry points?
> > > 
> > > Agreed. I had a "wat?" moment.
> > 
> > Would you be okay if I added a new public API that returns a GpuMemoryBuffer
> > pointer? 
>  
> You don't need a full-fledged GL API. This is only used from inside WGC3DImpl
> and your test. Both can get to the GLES2Implementation (the test can get it
> through GLManager). So just expose it from GLES2Implementation only.

Looks like the test doesn't build when I just expose it from
GLES2Implementation, is there anything I'm missing? Or should I just add a new
command like I did for imageBufferDataCHROMIUM

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:2052: "GL_CHROMIUM_map_sub "
On 2013/04/25 23:45:34, greggman wrote:
> You should probably add a string here based on which GpuMemoryBufferFactory
> you've attached
> 
> If a factory exists then append the string 
> 
>   "GL_CHROMIUM_gpu_memory_buffer"
> 
> 

Done.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3437:
GetBoundGpuMemoryBuffer("glMapBufferCHROMIUM", &buffer_id)) {
On 2013/04/25 23:45:34, greggman wrote:
> If GetBoundGPUMemoryBuffer returns false (assuming you meant it to return
falls
> on error), then it just falls through to GetBoundPixelBuffer. Is that correct?

Converted this function to use a switch statement.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3466: gpu_buffer->Map(mode,
&mapped_buffer);
On 2013/04/25 23:45:34, greggman wrote:
> The semantics of map buffer require that you can only map once, otherwise
> generate GL_INVALID_OPERATION
> 
> In other words calling glMapBuffer twice for the same buffer should fail. The
> buffer must be unmapped before mapping it again.

Done.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3507:
GetBoundGpuMemoryBuffer("glUnmapBufferCHROMIUM", &buffer_id)) {
On 2013/04/25 23:45:34, greggman wrote:
> Same here. If GetBoundGpuMemroyBuffer returns false (assuming it's supposed to
> return false) then it doesn't seem like you want it to fall through?
> 
> Actually, looking at the fall through works but it seems fragile to me. Maybe
> I'm being dumb.
> 
> I'd think it might be clearer if the code here at in MapBuffer was more like
> either
> 
>    switch (target) {
>      case GL_IMAGE_BUFFER_CHROMIUM:
>        ...
>        break;
>      case GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
>      case GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
>        ...
>        break;
>      default:
>        SetGLError(..., INVALID_ENUM, ...)
> 
> Or at least
> 
>    if (target == GL_IMAGE_BUFFER_CHROMIUM) {
>      ...
> 
>      return;  // don't let the code follow through since it can't
>               // possibly be interested in anything since we 
>               // know the target is incorrect for any code below.
>    }
> 
> What do you think? I think I'd prefer the switch as far as readability goes.

Done.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.cc:3515: gpu_buffer->Unmap();
On 2013/04/25 23:45:34, greggman wrote:
> The semantics of map buffer require if the buffer is not mapped this should
> generate GL_INVALID_OPERATION.

Done.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/client/gle...
gpu/command_buffer/client/gles2_implementation.h:19: #include
"../client/gpu_memory_buffer_tracker.h"
On 2013/04/25 23:45:34, greggman wrote:
> Can you just forward declare GPUMemoryBufferTracker?
> 
> Looks like some of the other trackers/manager should change to using forward
> declarations as well. I think some of them used to be instances instead of
> scoped_ptr

Done for gpu_memory_buffer_tracker but we need buffer_tracker.h for the
definition of the BufferTracker::Buffer subclass.

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/1/gpu/command_buffer/tests/gl_g...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:68:
TEST_F(GLGpuMemoryBufferTest, Lifecycle) {
On 2013/04/26 02:47:57, David Reveman wrote:
> The code below doesn't look right to me. The glGetString() stuff is really
> abusing the api.
> 
> Should glImageBufferDataCHROMIUM be implemented at a higher level
> (WebGraphicsContext3DInProcessCommandBufferImpl)? At this level I would expect
> you to just call gpu_memory_buffer_tracker_->CreateBuffer(buffer_id, width,
> height) and create a GLImage from that. Calling glImageBufferDataCHROMIUM here
> and then having dig up a gfx::GpuMemoryBufferHandle seem backwards.

Do we have access to ImageManager from GLES2Implementation? That was the
motivating factor to push a gfx::GpuMemoryBufferHandle to WGC3D.

[no sievers, 2013-04-26 10:03:23]

imageBufferData() actually works as expected and the client does not need to
know about GpuMemoryBuffer (see cc patch). GpuMemoryBuffer should not be exposed
in the GL api. It's an implementation detail and only needs to be visible
outside of GLES2Implementation in the part where you associate it with the
GLImage (client-service boundary). 

That's why it gets confusing a bit in this patch, which only implements
in-process versions where the boundaries between client and service blur.

The command-buffer implementation would probably need to send an IPC to share or
get a handle from the GPU process.

So can this part from WGC3D just move into the factory? Then you could have a
factory for ChromeOS that implements this differently, let's say by getting a
handle from the GPU process via IPC.

For in-process maybe you can just pass in the image_manager ref from
WGC3DInProcessImpl to the in-process factory at setup, since it's not sending an
IPC but registering it with the manager directly.

[kaanb1, 2013-04-26 16:08:26]

On 2013/04/26 10:03:23, Daniel Sievers wrote:
> imageBufferData() actually works as expected and the client does not need to
> know about GpuMemoryBuffer (see cc patch). GpuMemoryBuffer should not be
exposed
> in the GL api. It's an implementation detail and only needs to be visible
> outside of GLES2Implementation in the part where you associate it with the
> GLImage (client-service boundary). 
> 
> That's why it gets confusing a bit in this patch, which only implements
> in-process versions where the boundaries between client and service blur.
> 
> The command-buffer implementation would probably need to send an IPC to share
or
> get a handle from the GPU process.
> 
> So can this part from WGC3D just move into the factory? Then you could have a
> factory for ChromeOS that implements this differently, let's say by getting a
> handle from the GPU process via IPC.
> 
> For in-process maybe you can just pass in the image_manager ref from
> WGC3DInProcessImpl to the in-process factory at setup, since it's not sending
an
> IPC but registering it with the manager directly.

Factory just creates the buffer though, somebody needs to handle the deletion
from the image manager as well. The destructor is in
android_webview::~GpuMemoryBufferImpl, I don't think we want to expose
ImageManager all the way up there. (and probably cannot because of DEPS).

We can refactor things as needed as CrOS starts using this API and once the new
WebView architecture is reasonably tested and well behaving.

[kaanb1, 2013-04-26 20:21:35]

On 2013/04/26 16:08:26, kaanb1 wrote:
> On 2013/04/26 10:03:23, Daniel Sievers wrote:
> > imageBufferData() actually works as expected and the client does not need to
> > know about GpuMemoryBuffer (see cc patch). GpuMemoryBuffer should not be
> exposed
> > in the GL api. It's an implementation detail and only needs to be visible
> > outside of GLES2Implementation in the part where you associate it with the
> > GLImage (client-service boundary). 
> > 
> > That's why it gets confusing a bit in this patch, which only implements
> > in-process versions where the boundaries between client and service blur.
> > 
> > The command-buffer implementation would probably need to send an IPC to
share
> or
> > get a handle from the GPU process.
> > 
> > So can this part from WGC3D just move into the factory? Then you could have
a
> > factory for ChromeOS that implements this differently, let's say by getting
a
> > handle from the GPU process via IPC.
> > 
> > For in-process maybe you can just pass in the image_manager ref from
> > WGC3DInProcessImpl to the in-process factory at setup, since it's not
sending
> an
> > IPC but registering it with the manager directly.
> 
> Factory just creates the buffer though, somebody needs to handle the deletion
> from the image manager as well. The destructor is in
> android_webview::~GpuMemoryBufferImpl, I don't think we want to expose
> ImageManager all the way up there. (and probably cannot because of DEPS).
> 
> We can refactor things as needed as CrOS starts using this API and once the
new
> WebView architecture is reasonably tested and well behaving.

I had a chat with Daniel over IM and we decided on the following:

The factory will create the GpuMemoryBuffer and create the GLImage using the
native pointer and insert the GLImage into the manager.

Removal of an image from the image manager will be when glImageBufferData(id, 0,
0) and deleteBuffers() is decoded on the service side.

Let me know if you see any issues with this approach.

[kaanb1, 2013-04-27 01:26:37]

On 2013/04/26 20:21:35, kaanb1 wrote:
> On 2013/04/26 16:08:26, kaanb1 wrote:
> > On 2013/04/26 10:03:23, Daniel Sievers wrote:
> > > imageBufferData() actually works as expected and the client does not need
to
> > > know about GpuMemoryBuffer (see cc patch). GpuMemoryBuffer should not be
> > exposed
> > > in the GL api. It's an implementation detail and only needs to be visible
> > > outside of GLES2Implementation in the part where you associate it with the
> > > GLImage (client-service boundary). 
> > > 
> > > That's why it gets confusing a bit in this patch, which only implements
> > > in-process versions where the boundaries between client and service blur.
> > > 
> > > The command-buffer implementation would probably need to send an IPC to
> share
> > or
> > > get a handle from the GPU process.
> > > 
> > > So can this part from WGC3D just move into the factory? Then you could
have
> a
> > > factory for ChromeOS that implements this differently, let's say by
getting
> a
> > > handle from the GPU process via IPC.
> > > 
> > > For in-process maybe you can just pass in the image_manager ref from
> > > WGC3DInProcessImpl to the in-process factory at setup, since it's not
> sending
> > an
> > > IPC but registering it with the manager directly.
> > 
> > Factory just creates the buffer though, somebody needs to handle the
deletion
> > from the image manager as well. The destructor is in
> > android_webview::~GpuMemoryBufferImpl, I don't think we want to expose
> > ImageManager all the way up there. (and probably cannot because of DEPS).
> > 
> > We can refactor things as needed as CrOS starts using this API and once the
> new
> > WebView architecture is reasonably tested and well behaving.
> 
> I had a chat with Daniel over IM and we decided on the following:
> 
> The factory will create the GpuMemoryBuffer and create the GLImage using the
> native pointer and insert the GLImage into the manager.
> 
> Removal of an image from the image manager will be when glImageBufferData(id,
0,
> 0) and deleteBuffers() is decoded on the service side.
> 
> Let me know if you see any issues with this approach.

I've uploaded the implementation, can everyone take another look? If it all
looks good, I'll update the unittests.

We don't need the glGetPointerv() API any more.

[joth, 2013-04-29 17:21:21]

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:30:
gpu::GetProcessDefaultImageManager()->AddImage(gl_image, buffer_id);
Is there a RemoveImage() call? where does that happen? Seems odd having one here
in aw/ layer and not the other.
ISTM the factory method needs wrapping with another method that lives in gpu::
namespace and does this for us.

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
File android_webview/browser/gpu_memory_buffer_factory_impl.h (right):

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
android_webview/browser/gpu_memory_buffer_factory_impl.h:14: int id, int width,
int height);
nit: buffer_id here too.

and if they fit, it should be aligned with open paren.
(2nd example in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

[apatrick_chromium, 2013-04-29 19:36:10]

https://codereview.chromium.org/14456004/diff/37002/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt (right):

https://codereview.chromium.org/14456004/diff/37002/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt:52: void
ImageBufferDataCHROMIUM (GLenum target, GLsizei width, GLsizei height)
I think this wants format and type arguments so we can support other formats.
You could just make it generate an error if it is not the combination we
support, GL_RGBA / GL_UNSIGNED_BYTE?

[greggman, 2013-04-29 19:58:33]

https://codereview.chromium.org/14456004/diff/37002/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt (right):

https://codereview.chromium.org/14456004/diff/37002/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt:52: void
ImageBufferDataCHROMIUM (GLenum target, GLsizei width, GLsizei height)
On 2013/04/29 19:36:10, apatrick_chromium wrote:
> I think this wants format and type arguments so we can support other formats.
> You could just make it generate an error if it is not the combination we
> support, GL_RGBA / GL_UNSIGNED_BYTE?

Just to follow up, I talked with Kaanb offline. I pointed out that
MapSubTexImage2DCHROMIUM is pretty much this same signature. You could easily
make that function use GL_IMAGE_BUFFER_CHROMIUM as a target and in that case
required xoffset,yoffset = 0 etc..

Would that be a better way to go rather than make a new API?

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1261:
SetGLError(GL_INVALID_OPERATION, function_name, "no buffer bound");
This function returns only ever returns 'true'. Should it be void?

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:2052: str += " ";
Sadly this probably has to be the same as above, only inserting the space if the
string is not empty.

Maybe a simple static function should be added?

static void AddExtension(
    std::string* str, const char* extension) {
  if (!*str.empty()) {
    *str += " ";
  }
  *str += extension;
}

or something?

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3430: // TODO(kaanb): Do we
need matching CheckGLError() calls for these?
no I don't think you do. CheckGLError is for debug build only checking if the
service generated an error.

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3531:
SetGLError(GL_INVALID_OPERATION, "glMapBufferCHROMIUM", "not mapped");
Here and below s/glMap/glUnmap/

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.h:508: bool
GetPointervCHROMIUMHelper(GLenum pname, GLvoid** pointer);
confused. Your last comment said you didn't need GetPointerv any more. Do you
still need this?

[kaanb, 2013-04-30 03:40:10]

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:30:
gpu::GetProcessDefaultImageManager()->AddImage(gl_image, buffer_id);
On 2013/04/29 17:21:21, joth wrote:
> Is there a RemoveImage() call? where does that happen? Seems odd having one
here
> in aw/ layer and not the other.
> ISTM the factory method needs wrapping with another method that lives in gpu::
> namespace and does this for us.

Let's discuss this when sievers@ comes back.

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
File android_webview/browser/gpu_memory_buffer_factory_impl.h (right):

https://codereview.chromium.org/14456004/diff/37002/android_webview/browser/g...
android_webview/browser/gpu_memory_buffer_factory_impl.h:14: int id, int width,
int height);
On 2013/04/29 17:21:21, joth wrote:
> nit: buffer_id here too.
> 
> and if they fit, it should be aligned with open paren.
> (2nd example in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

Done.

https://codereview.chromium.org/14456004/diff/37002/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt (right):

https://codereview.chromium.org/14456004/diff/37002/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt:52: void
ImageBufferDataCHROMIUM (GLenum target, GLsizei width, GLsizei height)
On 2013/04/29 19:36:10, apatrick_chromium wrote:
> I think this wants format and type arguments so we can support other formats.
> You could just make it generate an error if it is not the combination we
> support, GL_RGBA / GL_UNSIGNED_BYTE?

Done.

https://codereview.chromium.org/14456004/diff/37002/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt:52: void
ImageBufferDataCHROMIUM (GLenum target, GLsizei width, GLsizei height)
On 2013/04/29 19:58:33, greggman wrote:
> On 2013/04/29 19:36:10, apatrick_chromium wrote:
> > I think this wants format and type arguments so we can support other
formats.
> > You could just make it generate an error if it is not the combination we
> > support, GL_RGBA / GL_UNSIGNED_BYTE?
> 
> Just to follow up, I talked with Kaanb offline. I pointed out that
> MapSubTexImage2DCHROMIUM is pretty much this same signature. You could easily
> make that function use GL_IMAGE_BUFFER_CHROMIUM as a target and in that case
> required xoffset,yoffset = 0 etc..
> 
> Would that be a better way to go rather than make a new API?

One concern is we need the method to support deletion of buffers and calling
ImageBufferDataCHROMIUM() with 0 width and height does is pretty naturally. 

Also from the ResourceProvider's perspective having two Map() calls could be
confusing, i.e.

MapSubTexImage2DCHROMIUM() to allocate the buffer and then
MapBufferCHROMIUM() to map the pixels, similary UnmapBufferCHROMIUM() to unmap
pixels and UnmapSubTexImage2DCHROMIUM() to free the buffer seem a bit confusing.

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1261:
SetGLError(GL_INVALID_OPERATION, function_name, "no buffer bound");
On 2013/04/29 19:58:33, greggman wrote:
> This function returns only ever returns 'true'. Should it be void?

Done.

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:2052: str += " ";
On 2013/04/29 19:58:33, greggman wrote:
> Sadly this probably has to be the same as above, only inserting the space if
the
> string is not empty.
> 
> Maybe a simple static function should be added?
> 
> static void AddExtension(
>     std::string* str, const char* extension) {
>   if (!*str.empty()) {
>     *str += " ";
>   }
>   *str += extension;
> }
> 
> or something?

Here str is guaranteed to be non-empty though. We unconditionally append
"GL_CHROMIUM_flipy " and the latter literals.

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3430: // TODO(kaanb): Do we
need matching CheckGLError() calls for these?
On 2013/04/29 19:58:33, greggman wrote:
> no I don't think you do. CheckGLError is for debug build only checking if the
> service generated an error.

Done.

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3531:
SetGLError(GL_INVALID_OPERATION, "glMapBufferCHROMIUM", "not mapped");
On 2013/04/29 19:58:33, greggman wrote:
> Here and below s/glMap/glUnmap/

Done.

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/37002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.h:508: bool
GetPointervCHROMIUMHelper(GLenum pname, GLvoid** pointer);
On 2013/04/29 19:58:33, greggman wrote:
> confused. Your last comment said you didn't need GetPointerv any more. Do you
> still need this?

Removed.

[kaanb, 2013-05-02 22:23:00]

Uploaded the new API please take another look.

[joth, 2013-05-03 00:49:36]

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3705: GLuint image_id, GLint
width, GLint height) {
image_id is not used

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.cc (right):

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.cc:34:
scoped_ptr<GpuMemoryBuffer> GpuMemoryBufferFactory::CreateGpuMemoryBuffer(
// creates a GpuMemoryBuffer and makes it available to the service side
ImageManager identified be image_id.

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:24: GLuint id, GLsizei
width, GLsizei height) {
image_id

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/common...
File gpu/command_buffer/common/image_buffer_manager.h (right):

https://codereview.chromium.org/14456004/diff/75001/gpu/command_buffer/common...
gpu/command_buffer/common/image_buffer_manager.h:14: class ImageManagerProxy {
can we simplify this?
- remove this interface
- no changes needed in ImageManager (service side)
- make GpuMemBufferFactory into a virtual base class
- have WC3D in-process provide a specialized sub-class of GpuMemBufferFactory
that knows how to poke GLImage::CreateFromGpuMemBuffer() and
ImageManager->AddImage() to make the buffer available to the service side
- GLES2 implementation would be exactly as in this patchset: passed in a
GpuMemBufferFactory instance and rely on that to do the service side mapping of
GpuMemBuffer -> GL image.

What do others think?

[joth, 2013-05-03 02:04:10]

great, yep that's what I was thinking of.. I can see what's going on now
(superficially, at least!)

https://codereview.chromium.org/14456004/diff/82001/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.h (right):

https://codereview.chromium.org/14456004/diff/82001/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:24: explicit
GpuMemoryBufferTracker(GpuMemoryBufferFactory* factory);
This is taking ownership of |factory| ? if so pass it in and store it as
scoped_ptr<>

https://codereview.chromium.org/14456004/diff/82001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/82001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:56: namespace
gles2 {
move this class to 
namespace webkit {
namespace gpu {
namespace {
...


i.e. line   228

https://codereview.chromium.org/14456004/diff/82001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:67:
ImageManager* image_manager_;
scoped_refptr<>

[kaanb, 2013-05-03 02:37:31]

https://codereview.chromium.org/14456004/diff/82001/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.h (right):

https://codereview.chromium.org/14456004/diff/82001/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:24: explicit
GpuMemoryBufferTracker(GpuMemoryBufferFactory* factory);
On 2013/05/03 02:04:11, joth wrote:
> This is taking ownership of |factory| ? if so pass it in and store it as
> scoped_ptr<>

GLES2Implementation is taking ownership of it. I made it a scoped_ptr<> there.

https://codereview.chromium.org/14456004/diff/82001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/82001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:56: namespace
gles2 {
On 2013/05/03 02:04:11, joth wrote:
> move this class to 
> namespace webkit {
> namespace gpu {
> namespace {
> ...
> 
> 
> i.e. line   228

Done.

https://codereview.chromium.org/14456004/diff/82001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:67:
ImageManager* image_manager_;
On 2013/05/03 02:04:11, joth wrote:
> scoped_refptr<>

Done.

[kaanb, 2013-05-03 21:58:47]

friendly ping!

[reveman, 2013-05-03 22:45:00]

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:2334:
bound_gpu_memory_buffer_id_ = buffer;
The id returned from GenImageBuffers/GenImage is not a buffer object and we
can't just use it as such. To map a GpuMemoryBuffer I think we need to either
add a new Map interface for images or create some API that bridge it to real
buffer objects. It might be better to do this in a separate patch.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:26: class
GpuMemoryBufferFactory {
The GpuMemoryBuffer::Creator stuff above is platform/single-process specific.
Can we move that out of gpu_memory_buffer_factory.h. Maybe into
gpu_memory_buffer_factory_android.h.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:27:
factory_->CreateGpuMemoryBuffer(width, height, image_id).release();
release()? either use the scoped_ptr or have GpuMemoryBufferFactory return a raw
pointer.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/cmd_bu...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/cmd_bu...
gpu/command_buffer/cmd_buffer_functions.txt:32: GL_APICALL void        
GL_APIENTRY glCreateGpuMemoryBuffer2DCHROMIUM (GLuint image_id, GLsizei width,
GLsizei height);
glImageCreateGpuMemoryBuffer2DCHROMIUM?

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/cmd_bu...
gpu/command_buffer/cmd_buffer_functions.txt:38: GL_APICALL void        
GL_APIENTRY glDeleteImageBuffers (GLsizeiNotNegative n, const GLuint*
imagebuffers);
glDeleteImage? This should not be confused with real buffer objects.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/cmd_bu...
gpu/command_buffer/cmd_buffer_functions.txt:62: GL_APICALL void        
GL_APIENTRY glGenImageBuffers (GLsizeiNotNegative n, GLuint* imagebuffers);
glGenImage?

https://codereview.chromium.org/14456004/diff/85002/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h (right):

https://codereview.chromium.org/14456004/diff/85002/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:478: virtual
void createGpuMemoryBuffer2DCHROMIUM(
createImageGpuMemoryBuffer2DCHROMIUM?

[greggman, 2013-05-03 23:48:00]

I think I'm still confused (sorry)

Maybe this is a stupid question but can all of this happen on the service side?

The problem with doing anything on the client side is it gets out of sync with
the service. The only reason P buffers (the GL_PIXEL_PACK_TRANSFER_BUFFER stuff)
works is because the buffers are 100% managed on the client so there's nothing
to sync. They're created on the client, managed on the client, destroyed on the
client, and then shared_memory/offsets are passed to the service. If those get
out of sync the the service will abort on purpose 

In this case though we've got EGLImage and Texture. We want to associate them.
Once if I've associated them I can delete one Oops! Or I can call TexImage2D
oops. Or CopyTexImage2D. I can try to use the texture in another context

So, it seems like it would be better to go back to the running this all on the
service. Whatever function gives you a pointer to the memory (glMapBuffer?
glMapImage? glGetPointerv?) will end up being a synchronous call but in this
case that's okay because it's single process anyway

What do you think? Am I over thinking it?

https://codereview.chromium.org/14456004/diff/85002/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt:33: This extension
helps avoid that extra copy from user memory to shared memory
This extension is, at least at the moment, specifically for Images. Does that
need mentioning?

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:110:
gpu_memory_buffer_factory_(NULL) {
as this is a scoped_ptr initialization to null is not needed

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:641: *params =
bound_gpu_memory_buffer_id_;
can you gate this on if there is a factory? If no factor return don't set *param
and return false

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1266: return;
no need for return in void function?

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/cmd_bu...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/cmd_bu...
gpu/command_buffer/cmd_buffer_functions.txt:38: GL_APICALL void        
GL_APIENTRY glDeleteImageBuffers (GLsizeiNotNegative n, const GLuint*
imagebuffers);
glDeleteImageBuffersCHROMIUM?

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/cmd_bu...
gpu/command_buffer/cmd_buffer_functions.txt:62: GL_APICALL void        
GL_APIENTRY glGenImageBuffers (GLsizeiNotNegative n, GLuint* imagebuffers);
glGenImageBuffersCHROMIUM?

Specifically I don't care about the name, but whatever it is, as it's Chrome
only it should end with CHROMIUM

[greggman, 2013-05-03 23:55:30]

Some clarification, I think what I'm suggesting is that
gpu_memory_buffer_factory should not be exposed/connected/associated with
GLES2Implementation nor should it appear on client side code at all. Attach it
to ContextGroup so it can do the right thing all on the service side.

[kaanb, 2013-05-04 00:09:48]

On 2013/05/03 23:55:30, greggman wrote:
> Some clarification, I think what I'm suggesting is that
> gpu_memory_buffer_factory should not be exposed/connected/associated with
> GLES2Implementation nor should it appear on client side code at all. Attach it
> to ContextGroup so it can do the right thing all on the service side.

We want to Map/Unmap buffers on the client side as we want to be able to upload
pixels without having access to a GL context. I'll schedule a meeting on Monday
so we can discuss all outstanding issues.

[greggman, 2013-05-04 00:15:09]

On 2013/05/04 00:09:48, kaanb wrote:
> On 2013/05/03 23:55:30, greggman wrote:
> > Some clarification, I think what I'm suggesting is that
> > gpu_memory_buffer_factory should not be exposed/connected/associated with
> > GLES2Implementation nor should it appear on client side code at all. Attach
it
> > to ContextGroup so it can do the right thing all on the service side.
> 
> We want to Map/Unmap buffers on the client side as we want to be able to
upload
> pixels without having access to a GL context.

I'm not sure how that matters in single process. You have access to everything.

> I'll schedule a meeting on Monday
> so we can discuss all outstanding issues.


One more wrench,

For a long time we've wanted to get rid of
WebGraphicsContext3DInProcessCommandBuffer.

Theoretically the only difference between 
  WebGraphicsContext3DInProcessCommandBuffer and
  WebGraphicsContext3DCommandBuffer.

is what happens on Flush. InProcessCommandBuffer runs the command, CommandBuffer
just updates the put pointer on the service.

I tried to consolidate them once but I don't remember what made it more work
than it should have been. Maybe now that we're Blink whatever that was has
disappeared?

My point being that this change makes the diverge more. If we consolidated them
it would also inform this design.

[reveman, 2013-05-04 00:54:25]

On 2013/05/04 00:15:09, greggman wrote:
> On 2013/05/04 00:09:48, kaanb wrote:
> > On 2013/05/03 23:55:30, greggman wrote:
> > > Some clarification, I think what I'm suggesting is that
> > > gpu_memory_buffer_factory should not be exposed/connected/associated with
> > > GLES2Implementation nor should it appear on client side code at all.
Attach
> it
> > > to ContextGroup so it can do the right thing all on the service side.
> > 
> > We want to Map/Unmap buffers on the client side as we want to be able to
> upload
> > pixels without having access to a GL context.
> 
> I'm not sure how that matters in single process. You have access to
everything.

We like to use this on ChromeOS in multi-process mode as well and I would prefer
to reuse as much as possible of code and a 100% of the API.

[kaanb, 2013-05-06 18:20:53]

https://codereview.chromium.org/14456004/diff/85002/gpu/GLES2/extensions/CHRO...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/GLES2/extensions/CHRO...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_gpu_memory_buffer.txt:33: This extension
helps avoid that extra copy from user memory to shared memory
On 2013/05/03 23:48:01, greggman wrote:
> This extension is, at least at the moment, specifically for Images. Does that
> need mentioning?

I'll go over this document once we finalize the names.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:110:
gpu_memory_buffer_factory_(NULL) {
On 2013/05/03 23:48:01, greggman wrote:
> as this is a scoped_ptr initialization to null is not needed

Done.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:641: *params =
bound_gpu_memory_buffer_id_;
On 2013/05/03 23:48:01, greggman wrote:
> can you gate this on if there is a factory? If no factor return don't set
*param
> and return false

Done.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1266: return;
On 2013/05/03 23:48:01, greggman wrote:
> no need for return in void function?

Done.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:2334:
bound_gpu_memory_buffer_id_ = buffer;
On 2013/05/03 22:45:00, David Reveman wrote:
> The id returned from GenImageBuffers/GenImage is not a buffer object and we
> can't just use it as such. To map a GpuMemoryBuffer I think we need to either
> add a new Map interface for images or create some API that bridge it to real
> buffer objects. It might be better to do this in a separate patch. 

I was thinking I could just store the buffer_id and the image_id within a
cc::ResourceProvider::Resource. I think we should discuss it in today's meeting.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:26: class
GpuMemoryBufferFactory {
On 2013/05/03 22:45:00, David Reveman wrote:
> The GpuMemoryBuffer::Creator stuff above is platform/single-process specific.
> Can we move that out of gpu_memory_buffer_factory.h. Maybe into
> gpu_memory_buffer_factory_android.h.

Actually since I refactored the class into WGC3DIPCBI this file is
single-process specific.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:27:
factory_->CreateGpuMemoryBuffer(width, height, image_id).release();
On 2013/05/03 22:45:00, David Reveman wrote:
> release()? either use the scoped_ptr or have GpuMemoryBufferFactory return a
raw
> pointer.

Unfortunately you cannot insert scoped_ptr()s into STL containers and we thought
using scoped_ptr would make sense in the API as it signals ownership of the
buffer is being transferred.

[reveman, 2013-05-07 02:25:03]

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:27:
factory_->CreateGpuMemoryBuffer(width, height, image_id).release();
On 2013/05/06 18:20:53, kaanb wrote:
> On 2013/05/03 22:45:00, David Reveman wrote:
> > release()? either use the scoped_ptr or have GpuMemoryBufferFactory return a
> raw
> > pointer.
> 
> Unfortunately you cannot insert scoped_ptr()s into STL containers and we
thought
> using scoped_ptr would make sense in the API as it signals ownership of the
> buffer is being transferred.

Ah, you'd want to use ScopedPtrHashMap here if that was not specific to cc/.
Please move the release() call to when you insert the buffer in the hash_map.
That will make it easier to understand why you need this.

[kaanb, 2013-05-08 00:32:35]

I've uploaded the new API. Extension document and the unittests are still out of
date, but it would be great if I could get some feedback on the API.

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/85002/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:27:
factory_->CreateGpuMemoryBuffer(width, height, image_id).release();
On 2013/05/07 02:25:03, David Reveman wrote:
> On 2013/05/06 18:20:53, kaanb wrote:
> > On 2013/05/03 22:45:00, David Reveman wrote:
> > > release()? either use the scoped_ptr or have GpuMemoryBufferFactory return
a
> > raw
> > > pointer.
> > 
> > Unfortunately you cannot insert scoped_ptr()s into STL containers and we
> thought
> > using scoped_ptr would make sense in the API as it signals ownership of the
> > buffer is being transferred.
> 
> Ah, you'd want to use ScopedPtrHashMap here if that was not specific to cc/.
> Please move the release() call to when you insert the buffer in the hash_map.
> That will make it easier to understand why you need this.

Done.

[reveman, 2013-05-08 04:23:06]

The GL API is looking good but the factory code needs more work.

You have:
GpuMemoryBuffer::Creator
SetProcessDefaultGpuMemoryBufferFactory
GetProcessDefaultGpuMemoryBufferFactory
GpuMemoryBufferFactory

and they are all in command_buffer/client and it's not clear what their roles
are. This is what it looks like to me:

GpuMemoryBuffer::Creator is a service side GpuMemoryBufferFactory. It constructs
GpuMemoryBuffers on the service side that you can create GLImage instances from.
It's currently a callback but it should really be an abstract interface. I think
this what we should call GpuMemoryBufferFactory and it shouldn't be defined in
command_buffer/client as it lives on the service side.

What you call GpuMemoryBufferFactory in the current patch is a client side
image_id factory. ImageFactory or GLImageFactory as I suggested in the comment
might be a good name for this interface.

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.cc (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.cc:24:
g_gpu_memory_buffer_factory_ = new GpuMemoryBuffer::Creator(factory);
This is leaking memory.

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
GetProcessDefaultGpuMemoryBufferFactory();
This is really confusing. GpuMemoryBuffer::Creator is a callback?!

This looks like a service side GpuMemoryBufferFactory. Should this live in
gpu/command_buffer/service/ and be a proper factory interface, just not a
callback?

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:26: class
GpuMemoryBufferFactory {
I think this should be called ImageFactory or GLImageFactory. That way you get
gpu::client::ImageFactory::CreateGpuMemoryBuffer, which makes sense.

https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:228: width,
height);
The factory being a callback is awkward.

[kaanb1, 2013-05-08 16:13:25]

On 2013/05/08 04:23:06, David Reveman wrote:
> The GL API is looking good but the factory code needs more work.
> 
> You have:
> GpuMemoryBuffer::Creator
> SetProcessDefaultGpuMemoryBufferFactory
> GetProcessDefaultGpuMemoryBufferFactory
> GpuMemoryBufferFactory
> 
> and they are all in command_buffer/client and it's not clear what their roles
> are. This is what it looks like to me:
> 
> GpuMemoryBuffer::Creator is a service side GpuMemoryBufferFactory. It
constructs
> GpuMemoryBuffers on the service side that you can create GLImage instances
from.
> It's currently a callback but it should really be an abstract interface. I
think
> this what we should call GpuMemoryBufferFactory and it shouldn't be defined in
> command_buffer/client as it lives on the service side.
> 
> What you call GpuMemoryBufferFactory in the current patch is a client side
> image_id factory. ImageFactory or GLImageFactory as I suggested in the comment
> might be a good name for this interface.
> 
>
https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
> File gpu/command_buffer/client/gpu_memory_buffer_factory.cc (right):
> 
>
https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
> gpu/command_buffer/client/gpu_memory_buffer_factory.cc:24:
> g_gpu_memory_buffer_factory_ = new GpuMemoryBuffer::Creator(factory);
> This is leaking memory.
> 
>
https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
> File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):
> 
>
https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
> gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
> GetProcessDefaultGpuMemoryBufferFactory();
> This is really confusing. GpuMemoryBuffer::Creator is a callback?!
> 
> This looks like a service side GpuMemoryBufferFactory. Should this live in
> gpu/command_buffer/service/ and be a proper factory interface, just not a
> callback?
> 
>
https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
> gpu/command_buffer/client/gpu_memory_buffer_factory.h:26: class
> GpuMemoryBufferFactory {
> I think this should be called ImageFactory or GLImageFactory. That way you get
> gpu::client::ImageFactory::CreateGpuMemoryBuffer, which makes sense.
> 
>
https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscon...
> File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):
> 
>
https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscon...
> webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:228: width,
> height);
> The factory being a callback is awkward.

The factory is being set within
android_webview::GpuMemoryBufferImpl::SetAwDrawGLFunctionTable as part of static
Android Webview initialization. I think we want to keep the GpuMemoryBuffers on
the client side as we want to Map/Unmap without issuing GL calls. Wouldn't this
work for your use case?

[joth, 2013-05-08 16:23:40]

Note that GpuMemoryBuffer::Creator was purely created to allow the android
implementation to be injected, due to a layering issue we have in the build
(the API is non-public and so not available to the majority of the chromium
code; we inject this dependency from the top level module that does have
visibility of those APIs).

If it's not useful on other platforms, now that we have this factory class
wrapped around it, we can delete it from gpu/ altogether.






On 8 May 2013 17:13, <kaanb@google.com> wrote:

> On 2013/05/08 04:23:06, David Reveman wrote:
>
>> The GL API is looking good but the factory code needs more work.
>>
>
>  You have:
>> GpuMemoryBuffer::Creator
>> SetProcessDefaultGpuMemoryBuff**erFactory
>> GetProcessDefaultGpuMemoryBuff**erFactory
>> GpuMemoryBufferFactory
>>
>
>  and they are all in command_buffer/client and it's not clear what their
>> roles
>> are. This is what it looks like to me:
>>
>
>  GpuMemoryBuffer::Creator is a service side GpuMemoryBufferFactory. It
>>
> constructs
>
>> GpuMemoryBuffers on the service side that you can create GLImage instances
>>
> from.
>
>> It's currently a callback but it should really be an abstract interface. I
>>
> think
>
>> this what we should call GpuMemoryBufferFactory and it shouldn't be
>> defined in
>> command_buffer/client as it lives on the service side.
>>
>
>  What you call GpuMemoryBufferFactory in the current patch is a client side
>> image_id factory. ImageFactory or GLImageFactory as I suggested in the
>> comment
>> might be a good name for this interface.
>>
>
>
> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>
command_buffer/client/gpu_**memory_buffer_factory.cc<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.cc>
>
>> File gpu/command_buffer/client/gpu_**memory_buffer_factory.cc (right):
>>
>
>
> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>
command_buffer/client/gpu_**memory_buffer_factory.cc#**newcode24<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.cc#newcode24>
>
>> gpu/command_buffer/client/gpu_**memory_buffer_factory.cc:24:
>> g_gpu_memory_buffer_factory_ = new GpuMemoryBuffer::Creator(**factory);
>> This is leaking memory.
>>
>
>
> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>
command_buffer/client/gpu_**memory_buffer_factory.h<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h>
>
>> File gpu/command_buffer/client/gpu_**memory_buffer_factory.h (right):
>>
>
>
> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>
command_buffer/client/gpu_**memory_buffer_factory.h#**newcode20<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h#newcode20>
>
>> gpu/command_buffer/client/gpu_**memory_buffer_factory.h:20:
>> GetProcessDefaultGpuMemoryBuff**erFactory();
>> This is really confusing. GpuMemoryBuffer::Creator is a callback?!
>>
>
>  This looks like a service side GpuMemoryBufferFactory. Should this live in
>> gpu/command_buffer/service/ and be a proper factory interface, just not a
>> callback?
>>
>
>
> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>
command_buffer/client/gpu_**memory_buffer_factory.h#**newcode26<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h#newcode26>
>
>> gpu/command_buffer/client/gpu_**memory_buffer_factory.h:26: class
>> GpuMemoryBufferFactory {
>> I think this should be called ImageFactory or GLImageFactory. That way
>> you get
>> gpu::client::ImageFactory::**CreateGpuMemoryBuffer, which makes sense.
>>
>
>
> https://codereview.chromium.**org/14456004/diff/95003/**webkit/gpu/**
>
webgraphicscontext3d_in_**process_command_buffer_impl.cc<https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc>
>
>> File webkit/gpu/**webgraphicscontext3d_in_**
>> process_command_buffer_impl.cc
>>
> (right):
>
>
> https://codereview.chromium.**org/14456004/diff/95003/**webkit/gpu/**
>
webgraphicscontext3d_in_**process_command_buffer_impl.**cc#newcode228<https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc#newcode228>
>
>> webkit/gpu/**webgraphicscontext3d_in_**process_command_buffer_impl.**cc:228:
>> width,
>> height);
>> The factory being a callback is awkward.
>>
>
> The factory is being set within
> android_webview::**GpuMemoryBufferImpl::**SetAwDrawGLFunctionTable as
> part of static
> Android Webview initialization. I think we want to keep the
> GpuMemoryBuffers on
> the client side as we want to Map/Unmap without issuing GL calls. Wouldn't
> this
> work for your use case?
>
>
https://codereview.chromium.**org/14456004/<https://codereview.chromium.org/1...
>

[joth, 2013-05-08 16:26:38]

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
GetProcessDefaultGpuMemoryBufferFactory();
On 2013/05/08 04:23:06, David Reveman wrote:
> This is really confusing. GpuMemoryBuffer::Creator is a callback?!
> 
> This looks like a service side GpuMemoryBufferFactory. Should this live in
> gpu/command_buffer/service/ and be a proper factory interface, just not a
> callback?

jamesr very specifically requested it be a Callback in preference to a
one-method factory. By same rationale, GpuMemoryBufferFactory could actually be
a callback just as well.
(although your comment below about calling it
ImageFactory::CreateGpuMemoryBuffer sounds like you predict other sorts of
methods will be added too? In which case keeping as class makes sense.)

-- see also my other comment that we can actually remove this from gpu if it's
not useful now we have a higher level factory. However.... I'd suggest doing
that as a followup as this patch is already complicated enough.

[kaanb, 2013-05-08 16:43:23]

On 2013/05/08 16:26:38, joth wrote:
>
https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
> File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):
> 
>
https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
> gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
> GetProcessDefaultGpuMemoryBufferFactory();
> On 2013/05/08 04:23:06, David Reveman wrote:
> > This is really confusing. GpuMemoryBuffer::Creator is a callback?!
> > 
> > This looks like a service side GpuMemoryBufferFactory. Should this live in
> > gpu/command_buffer/service/ and be a proper factory interface, just not a
> > callback?
> 
> jamesr very specifically requested it be a Callback in preference to a
> one-method factory. By same rationale, GpuMemoryBufferFactory could actually
be
> a callback just as well.
> (although your comment below about calling it
> ImageFactory::CreateGpuMemoryBuffer sounds like you predict other sorts of
> methods will be added too? In which case keeping as class makes sense.)
> 
> -- see also my other comment that we can actually remove this from gpu if it's
> not useful now we have a higher level factory. However.... I'd suggest doing
> that as a followup as this patch is already complicated enough.

Actually we now have some state (next_id_ and the image_manager_) in the
in-process implementation of the GpuMemoryBufferFactory so we cannot make it a
callback at this stage. 

gpu/ still needs to be allocate android_webview::GpuMemoryBufferImpls though, if
we remove the callback how will that happen?

[joth, 2013-05-08 17:10:23]

On 8 May 2013 17:43, <kaanb@chromium.org> wrote:

> On 2013/05/08 16:26:38, joth wrote:
>
> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>
command_buffer/client/gpu_**memory_buffer_factory.h<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h>
>
>> File gpu/command_buffer/client/gpu_**memory_buffer_factory.h (right):
>>
>
>
> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>
command_buffer/client/gpu_**memory_buffer_factory.h#**newcode20<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h#newcode20>
>
>> gpu/command_buffer/client/gpu_**memory_buffer_factory.h:20:
>> GetProcessDefaultGpuMemoryBuff**erFactory();
>> On 2013/05/08 04:23:06, David Reveman wrote:
>> > This is really confusing. GpuMemoryBuffer::Creator is a callback?!
>> >
>> > This looks like a service side GpuMemoryBufferFactory. Should this live
>> in
>> > gpu/command_buffer/service/ and be a proper factory interface, just not
>> a
>> > callback?
>>
>
>  jamesr very specifically requested it be a Callback in preference to a
>> one-method factory. By same rationale, GpuMemoryBufferFactory could
>> actually
>>
> be
>
>> a callback just as well.
>> (although your comment below about calling it
>> ImageFactory::**CreateGpuMemoryBuffer sounds like you predict other
>> sorts of
>> methods will be added too? In which case keeping as class makes sense.)
>>
>
>  -- see also my other comment that we can actually remove this from gpu if
>> it's
>> not useful now we have a higher level factory. However.... I'd suggest
>> doing
>> that as a followup as this patch is already complicated enough.
>>
>
> Actually we now have some state (next_id_ and the image_manager_) in the
> in-process implementation of the GpuMemoryBufferFactory so we cannot make
> it a
> callback at this stage.
>
> Callbacks can carry state - just bind it to a member function of a class
that holds the state. (Although, it means the class needs to be ref-counted
to resolve the lifetime ambiguity, which may not be what you want here)



> gpu/ still needs to be allocate android_webview::**GpuMemoryBufferImpls
> though, if
> we remove the callback how will that happen?
>
>
https://codereview.chromium.**org/14456004/<https://codereview.chromium.org/1...
>

[jamesr1, 2013-05-08 17:13:58]

My feedback was specifically to use callbacks for stateless calls.


On Wed, May 8, 2013 at 10:09 AM, Jonathan Dixon <joth@chromium.org> wrote:

>
>
>
> On 8 May 2013 17:43, <kaanb@chromium.org> wrote:
>
>> On 2013/05/08 16:26:38, joth wrote:
>>
>> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>>
command_buffer/client/gpu_**memory_buffer_factory.h<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h>
>>
>>> File gpu/command_buffer/client/gpu_**memory_buffer_factory.h (right):
>>>
>>
>>
>> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>>
command_buffer/client/gpu_**memory_buffer_factory.h#**newcode20<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h#newcode20>
>>
>>> gpu/command_buffer/client/gpu_**memory_buffer_factory.h:20:
>>> GetProcessDefaultGpuMemoryBuff**erFactory();
>>> On 2013/05/08 04:23:06, David Reveman wrote:
>>> > This is really confusing. GpuMemoryBuffer::Creator is a callback?!
>>> >
>>> > This looks like a service side GpuMemoryBufferFactory. Should this
>>> live in
>>> > gpu/command_buffer/service/ and be a proper factory interface, just
>>> not a
>>> > callback?
>>>
>>
>>  jamesr very specifically requested it be a Callback in preference to a
>>> one-method factory. By same rationale, GpuMemoryBufferFactory could
>>> actually
>>>
>> be
>>
>>> a callback just as well.
>>> (although your comment below about calling it
>>> ImageFactory::**CreateGpuMemoryBuffer sounds like you predict other
>>> sorts of
>>> methods will be added too? In which case keeping as class makes sense.)
>>>
>>
>>  -- see also my other comment that we can actually remove this from gpu
>>> if it's
>>> not useful now we have a higher level factory. However.... I'd suggest
>>> doing
>>> that as a followup as this patch is already complicated enough.
>>>
>>
>> Actually we now have some state (next_id_ and the image_manager_) in the
>> in-process implementation of the GpuMemoryBufferFactory so we cannot make
>> it a
>> callback at this stage.
>>
>> Callbacks can carry state - just bind it to a member function of a class
> that holds the state. (Although, it means the class needs to be ref-counted
> to resolve the lifetime ambiguity, which may not be what you want here)
>
>
>
>> gpu/ still needs to be allocate android_webview::**GpuMemoryBufferImpls
>> though, if
>> we remove the callback how will that happen?
>>
>>
https://codereview.chromium.**org/14456004/<https://codereview.chromium.org/1...
>>
>
>

[kaanb1, 2013-05-08 17:51:19]

Ok, I'll rename GpuMemoryBufferFactory interface to ImageFactory and leave
global callback in place (at least for this patch). How does that sound?


On Wed, May 8, 2013 at 10:13 AM, James Robinson <jamesr@google.com> wrote:

> My feedback was specifically to use callbacks for stateless calls.
>
>
> On Wed, May 8, 2013 at 10:09 AM, Jonathan Dixon <joth@chromium.org> wrote:
>
>>
>>
>>
>> On 8 May 2013 17:43, <kaanb@chromium.org> wrote:
>>
>>> On 2013/05/08 16:26:38, joth wrote:
>>>
>>> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>>>
command_buffer/client/gpu_**memory_buffer_factory.h<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h>
>>>
>>>> File gpu/command_buffer/client/gpu_**memory_buffer_factory.h (right):
>>>>
>>>
>>>
>>> https://codereview.chromium.**org/14456004/diff/95003/gpu/**
>>>
command_buffer/client/gpu_**memory_buffer_factory.h#**newcode20<https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client/gpu_memory_buffer_factory.h#newcode20>
>>>
>>>> gpu/command_buffer/client/gpu_**memory_buffer_factory.h:20:
>>>> GetProcessDefaultGpuMemoryBuff**erFactory();
>>>> On 2013/05/08 04:23:06, David Reveman wrote:
>>>> > This is really confusing. GpuMemoryBuffer::Creator is a callback?!
>>>> >
>>>> > This looks like a service side GpuMemoryBufferFactory. Should this
>>>> live in
>>>> > gpu/command_buffer/service/ and be a proper factory interface, just
>>>> not a
>>>> > callback?
>>>>
>>>
>>>  jamesr very specifically requested it be a Callback in preference to a
>>>> one-method factory. By same rationale, GpuMemoryBufferFactory could
>>>> actually
>>>>
>>> be
>>>
>>>> a callback just as well.
>>>> (although your comment below about calling it
>>>> ImageFactory::**CreateGpuMemoryBuffer sounds like you predict other
>>>> sorts of
>>>> methods will be added too? In which case keeping as class makes sense.)
>>>>
>>>
>>>  -- see also my other comment that we can actually remove this from gpu
>>>> if it's
>>>> not useful now we have a higher level factory. However.... I'd suggest
>>>> doing
>>>> that as a followup as this patch is already complicated enough.
>>>>
>>>
>>> Actually we now have some state (next_id_ and the image_manager_) in the
>>> in-process implementation of the GpuMemoryBufferFactory so we cannot
>>> make it a
>>> callback at this stage.
>>>
>>> Callbacks can carry state - just bind it to a member function of a class
>> that holds the state. (Although, it means the class needs to be ref-counted
>> to resolve the lifetime ambiguity, which may not be what you want here)
>>
>>
>>
>>> gpu/ still needs to be allocate android_webview::**GpuMemoryBufferImpls
>>> though, if
>>> we remove the callback how will that happen?
>>>
>>>
https://codereview.chromium.**org/14456004/<https://codereview.chromium.org/1...
>>>
>>
>>
>


-- 
Thanks,
Kaan

[reveman, 2013-05-08 20:02:40]

Ok, sounds good. We can refactor this as needed when adding multi-process mode.

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
GetProcessDefaultGpuMemoryBufferFactory();
If it's a callback, should we call it a callback and not a
GpuMemoryBufferFactory or GpuMemoryBuffer::Creator? Code like this:
GetProcessDefaultGpuMemoryBufferFactory().Run(width, height);
in webgraphicscontext3d_in_process_command_buffer_impl.cc is confusing.

[kaanb, 2013-05-08 20:41:35]

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
GetProcessDefaultGpuMemoryBufferFactory();
On 2013/05/08 20:02:41, David Reveman wrote:
> If it's a callback, should we call it a callback and not a
> GpuMemoryBufferFactory or GpuMemoryBuffer::Creator? Code like this:
> GetProcessDefaultGpuMemoryBufferFactory().Run(width, height);
> in webgraphicscontext3d_in_process_command_buffer_impl.cc is confusing.

How about GpuMemoryBufferCreateCallback?

[no sievers, 2013-05-08 21:54:20]

https://codereview.chromium.org/14456004/diff/95003/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/GLES2/gl2extchromium....
gpu/GLES2/gl2extchromium.h:99: #define GL_CHROMIUM_gpu_memory_buffer 1
Should this be renamed now that GpuMemoryBuffer is a concept entirely internal
to the GL implementation?

The new GL APIs only uses the term 'Image'.
How about GL_CHROMIUM_map_image?

[kaanb, 2013-05-08 21:55:32]

On 2013/05/08 21:54:20, Daniel Sievers wrote:
> https://codereview.chromium.org/14456004/diff/95003/gpu/GLES2/gl2extchromium.h
> File gpu/GLES2/gl2extchromium.h (right):
> 
>
https://codereview.chromium.org/14456004/diff/95003/gpu/GLES2/gl2extchromium....
> gpu/GLES2/gl2extchromium.h:99: #define GL_CHROMIUM_gpu_memory_buffer 1
> Should this be renamed now that GpuMemoryBuffer is a concept entirely internal
> to the GL implementation?
> 
> The new GL APIs only uses the term 'Image'.
> How about GL_CHROMIUM_map_image?

sgtm.

[reveman, 2013-05-08 23:33:25]

CHROMIUM_map_image sgtm

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
GetProcessDefaultGpuMemoryBufferFactory();
sgtm

[kaanb, 2013-05-09 05:13:13]

It is ready for another look, I haven't updated the extension doc but everything
else is ready for review.

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.cc (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.cc:24:
g_gpu_memory_buffer_factory_ = new GpuMemoryBuffer::Creator(factory);
On 2013/05/08 04:23:06, David Reveman wrote:
> This is leaking memory.

It's one pointer per process so we felt it was reasonable. It is being
initialized as part of static initialization of WebView.

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:20:
GetProcessDefaultGpuMemoryBufferFactory();
On 2013/05/08 23:33:25, David Reveman wrote:
> sgtm

I realized that to rename it I'll need to touch android_webview package, so
let's punt it to a smaller follow-up patch.

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:26: class
GpuMemoryBufferFactory {
On 2013/05/08 04:23:06, David Reveman wrote:
> I think this should be called ImageFactory or GLImageFactory. That way you get
> gpu::client::ImageFactory::CreateGpuMemoryBuffer, which makes sense.

Done.

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:26: class
GpuMemoryBufferFactory {
On 2013/05/08 04:23:06, David Reveman wrote:
> I think this should be called ImageFactory or GLImageFactory. That way you get
> gpu::client::ImageFactory::CreateGpuMemoryBuffer, which makes sense.

Done.

https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/95003/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:228: width,
height);
On 2013/05/08 04:23:06, David Reveman wrote:
> The factory being a callback is awkward.

Acknowledged.

[kaanb, 2013-05-09 18:31:06]

Uploaded the extension documentation as well. All ready for another look.

[greggman, 2013-05-10 00:12:22]

looks great. Just a few things

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:51: GLuint
CreateImageCHROMIUM (GLsizei width, GLsizei height)
no format? How do you know the format of the image

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:82: permanent. Returns true
iff the operation was successful.
s/iff/if/

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/gl2extchromium...
gpu/GLES2/gl2extchromium.h:102: #define GL_IMAGE_ROWBYTES 0x78F0
GL_IMAGE_ROWBYTES_CHROMIUM?

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp
File gpu/gpu.gyp (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp#newcode19
gpu/gpu.gyp:19: '../testing/gmock.gyp:gmock',
Mock should not be here. Mock is only used in unit tests, not in shipping code.

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp#newcode44
gpu/gpu.gyp:44: '../testing/gmock.gyp:gmock',
Mock should not be here. Mock is only used in unit tests, not in shipping code

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp#newcode67
gpu/gpu.gyp:67: '../testing/gmock.gyp:gmock',
Mock should not be here. Mock is only used in unit tests, not in shipping code

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_common.gypi
File gpu/gpu_common.gypi (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_common.gypi#newc...
gpu/gpu_common.gypi:42: 'command_buffer/client/gpu_memory_buffer_mock.cc',
Mocks do no belong here. They should get put in the testing target

If you use the mocks in more than one test target add a new target gpu_mocks or
something and make the test targets depend on that

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_untrusted.gyp
File gpu/gpu_untrusted.gyp (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_untrusted.gyp#ne...
gpu/gpu_untrusted.gyp:36: '../testing/gmock.gyp:gmock',
Mock should not be here. Mock is only used in unit tests, not in shipping code

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h (right):

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:406: /* virtual
void deleteImageBuffer(WebGLId); */
If this is no longer needed delete it? We don't generally leave commented out
code in

[no sievers, 2013-05-10 01:21:51]

API is looking good. A couple nits, mostly on the spec wording.

I don't understand the two factories. I thought reveman was suggesting this
because of how one thing should live on the service-side, but both are still in
the client/ directory.
The only service-side part is where you access image_manager right?

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:19: This extension allows
for more efficiently uploading of buffer or texture
nit: more efficient uploading.

buffer data too? Doesn't this only support textures/images?

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:31: telling the GPU process
to use that shared memory to upload the data.
nit: I'm not sure if I'd detour so much here. 

And maybe also not say what copies exactly are avoided? Because it depends a bit
on the platform and implementation. You could just say 'it allows for the direct
manipulation of texture memory' or something like that.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:43: OpenGL ES 2.0 and GPU
implementations live in different processes.
nit: Maybe also leave this out. Where the extension is not available it is
simply not advertised.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:53: Allocates GPU memory
with width equal to <width> and height equal
nit: 'allocates an image'?

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:56: Returns a unique
identifier for the allocated buffer that could be used
nit: 'for the allocated image'

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:63: Frees GPU memory
previously allocated by a call to CreateImageCHROMIUM.
nit: 'Free the image'

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:69: Returns a pointer to in
the user memory for the application to access
nit: I find that sentence a bit hard to read :)

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:77: INVALID_ENUM is
generated if <access> is not WRITE_ONLY and not READ_ONLY.
What about READ_WRITE?

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:82: permanent. Returns true
iff the operation was successful.
Hmm, does it really make anything permanent? There is no implicit
synchronization, so the changes possibly take effect when you touch the memory
already. It really just removes the mapping, right?
Might give people the wrong impression about synchronization otherwise.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:99: suitably initialized
before calling this method.
nit: suitably initialized? maybe you are trying to indicate that *params will
not be set if image_id or pname was invalid?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/build...
File gpu/command_buffer/build_gles2_cmd_buffer.py (left):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:1783: 'chromium': True,
why remove this?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:2018: 'chromium': True,
why remove this?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:109: image_factory_() {
nit: remove this

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:2040: str += " ";
str += std::string(str.empty() ? "" : " ") + "GL_CHROMIUM_map_image";

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3585: return
gpu_memory_buffer_tracker_->CreateBuffer(width, height);
Could that fail? Should we set OUT_OF_MEMORY or something in that case?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3603: "invalid GPU memory
buffer");
nit: "invalid image" here and below

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3655:
GpuMemoryBuffer::AccessMode mode = GpuMemoryBuffer::READ_ONLY;
nit: no need to initialize

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3663: // TODO(kaanb): should
we add GL_READ_WRITE to gl2ext.h?
Yes, isn't readwrite what we need to raster into this for tiles?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3677: gpu_buffer->Map(mode,
&mapped_buffer);
Can this fail and should we set a GL error if so?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3697: "invalid target");
nit: invalid parameter

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:229: // GLES2Implementation
owns the |tracker|.
nit: maybe put 'for testing' here?

Because it seems there should always be only one kind of tracker in the real
world.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:422: void
GenImageBuffersHelper(GLsizei n, const GLuint* imagebuffers);
not used or defined?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:16: // renderer are within
the same process.
nit: Can the android webview comment go now that we convinced ourselves this
architecture is generic enough?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.cc:32:
gl_->Flush();
Hmm I wonder if the flush here and below should be in gles2_implementation.cc

And it can also be a shallow flush, i.e. CommandBufferHelper::Flush() without
the glFlush()).

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.h:24: class
GLES2_IMPL_EXPORT GpuMemoryBufferTrackerInProcess
I don't understand the in-process notion here. GpuMemoryBuffer will always be
the client-side object, right?
Regardless of whether it's the client to a GLImage in another process or not.

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:49: using
gpu::gles2::ImageFactory;
nit: order

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:661: new
ImageFactoryInProcess(GetImageManager()));
That means different clients in the share group will generate overlapping IDs
and will try to use them with the same image manager.

[no sievers, 2013-05-10 01:47:00]

Also, I'd find it nicer if the buffer tracker was not a virtual interface, which
seems to be only for testing. It's weird to mock internals of the
gles2_implementation, esp. when they are part of what you want to test.

[reveman, 2013-05-10 02:06:16]

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
File gpu/command_buffer/client/gpu_memory_buffer_factory.cc (right):

https://codereview.chromium.org/14456004/diff/95003/gpu/command_buffer/client...
gpu/command_buffer/client/gpu_memory_buffer_factory.cc:24:
g_gpu_memory_buffer_factory_ = new GpuMemoryBuffer::Creator(factory);
Please move this leak into WebView by making the caller responsible for managing
the memory of g_gpu_memory_buffer_factory_ asap.

Comment with a TODO here would be appreciated.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:182: new
GpuMemoryBufferTrackerInProcess(image_factory_.get(), this));
You can't make assumptions about single/multi-process here. I think the
GpuMemoryBufferTracker should be generic. You already have the ImageFactory for
single-process specific code.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:225: void
SetImageFactory(scoped_ptr<ImageFactory> factory) {
Changing the ImageFactory during the lifetime of a GLES2Implementation seem bad.
Can you pass this to the constructor instead?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.cc:15:
GpuMemoryBufferTrackerInProcess::GpuMemoryBufferTrackerInProcess(
Why is this in-process specific? You already have the ImageFactory interface for
single process specific code?

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/image_factory.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:19: // Craete a GpuMemoryBuffer and
makes it available to the
nit: Create

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:23: virtual void
RemoveFromManager(unsigned int image_id) = 0;
I prefer DeleteGpuMemoryBuffer(unsigned image_id). The "Delete" prefix as that's
consistent with BrowserGpuChannelHostFactory and GpuChannel. The idea that
there's an image manager for which something is inserted/removed is an
implementation detail that I don't think this API should expose.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:47: image_factory_ =
image_factory.get();
Saving a pointer and passing ownership to a different object is not great. I
think all this would be much cleaner if GLES2Implementation didn't take
ownership of the ImageFactory.

[kaanb, 2013-05-13 23:00:36]

+brettw as reviewer for ppapi/shared_impl/ppb_graphics_3d_shared.cc

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:19: This extension allows
for more efficiently uploading of buffer or texture
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: more efficient uploading.
> 
> buffer data too? Doesn't this only support textures/images?

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:31: telling the GPU process
to use that shared memory to upload the data.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: I'm not sure if I'd detour so much here. 
> 
> And maybe also not say what copies exactly are avoided? Because it depends a
bit
> on the platform and implementation. You could just say 'it allows for the
direct
> manipulation of texture memory' or something like that.

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:43: OpenGL ES 2.0 and GPU
implementations live in different processes.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: Maybe also leave this out. Where the extension is not available it is
> simply not advertised.

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:51: GLuint
CreateImageCHROMIUM (GLsizei width, GLsizei height)
On 2013/05/10 00:12:22, greggman wrote:
> no format? How do you know the format of the image

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:53: Allocates GPU memory
with width equal to <width> and height equal
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: 'allocates an image'?

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:56: Returns a unique
identifier for the allocated buffer that could be used
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: 'for the allocated image'

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:63: Frees GPU memory
previously allocated by a call to CreateImageCHROMIUM.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: 'Free the image'

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:69: Returns a pointer to in
the user memory for the application to access
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: I find that sentence a bit hard to read :)

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:77: INVALID_ENUM is
generated if <access> is not WRITE_ONLY and not READ_ONLY.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> What about READ_WRITE?

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:82: permanent. Returns true
iff the operation was successful.
On 2013/05/10 00:12:22, greggman wrote:
> s/iff/if/

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:82: permanent. Returns true
iff the operation was successful.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> Hmm, does it really make anything permanent? There is no implicit
> synchronization, so the changes possibly take effect when you touch the memory
> already. It really just removes the mapping, right?
> Might give people the wrong impression about synchronization otherwise.

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:99: suitably initialized
before calling this method.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: suitably initialized? maybe you are trying to indicate that *params will
> not be set if image_id or pname was invalid?

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/GLES2/gl2extchromium...
gpu/GLES2/gl2extchromium.h:102: #define GL_IMAGE_ROWBYTES 0x78F0
On 2013/05/10 00:12:22, greggman wrote:
> GL_IMAGE_ROWBYTES_CHROMIUM?

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/build...
File gpu/command_buffer/build_gles2_cmd_buffer.py (left):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:1783: 'chromium': True,
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> why remove this?

Because it's redundant, see line 1781.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:2018: 'chromium': True,
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> why remove this?

Because it's redundant, see line 2016

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:109: image_factory_() {
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: remove this

We're now passing the image_factory via the constructor.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:182: new
GpuMemoryBufferTrackerInProcess(image_factory_.get(), this));
On 2013/05/10 02:06:16, David Reveman wrote:
> You can't make assumptions about single/multi-process here. I think the
> GpuMemoryBufferTracker should be generic. You already have the ImageFactory
for
> single-process specific code.

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:2040: str += " ";
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> str += std::string(str.empty() ? "" : " ") + "GL_CHROMIUM_map_image";

str.empty() is guaranteed to be false here, so I simplified your suggestion a
bit.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3585: return
gpu_memory_buffer_tracker_->CreateBuffer(width, height);
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> Could that fail? Should we set OUT_OF_MEMORY or something in that case?

Yes, it could fail, right now we DCHECK() in
android_webview::GpuMemoryBufferImpl() if that happens. I can put a TODO to
plumb it here.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3603: "invalid GPU memory
buffer");
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: "invalid image" here and below

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3655:
GpuMemoryBuffer::AccessMode mode = GpuMemoryBuffer::READ_ONLY;
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: no need to initialize

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3663: // TODO(kaanb): should
we add GL_READ_WRITE to gl2ext.h?
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> Yes, isn't readwrite what we need to raster into this for tiles?

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3677: gpu_buffer->Map(mode,
&mapped_buffer);
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> Can this fail and should we set a GL error if so?

From what I understand the documentation, the only failure mode is if the flags
used to create the buffer are inconsistent with the map access mode. Since it
indicated a setup failure, currently I DCHECK in android_webview::Map() if Map()
(i.e. Lock()) fails.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3697: "invalid target");
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: invalid parameter

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:225: void
SetImageFactory(scoped_ptr<ImageFactory> factory) {
On 2013/05/10 02:06:16, David Reveman wrote:
> Changing the ImageFactory during the lifetime of a GLES2Implementation seem
bad.
> Can you pass this to the constructor instead?

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:229: // GLES2Implementation
owns the |tracker|.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: maybe put 'for testing' here?
> 
> Because it seems there should always be only one kind of tracker in the real
> world.

Removed this method.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:422: void
GenImageBuffersHelper(GLsizei n, const GLuint* imagebuffers);
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> not used or defined?

Removed.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_factory.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_factory.h:16: // renderer are within
the same process.
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: Can the android webview comment go now that we convinced ourselves this
> architecture is generic enough?

They'll go away when I remove this file and fold it into the ImageManager.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.cc:15:
GpuMemoryBufferTrackerInProcess::GpuMemoryBufferTrackerInProcess(
On 2013/05/10 02:06:16, David Reveman wrote:
> Why is this in-process specific? You already have the ImageFactory interface
for
> single process specific code?

Removed this file.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.cc:32:
gl_->Flush();
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> Hmm I wonder if the flush here and below should be in gles2_implementation.cc
> 
> And it can also be a shallow flush, i.e. CommandBufferHelper::Flush() without
> the glFlush()).

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker_in_process.h:24: class
GLES2_IMPL_EXPORT GpuMemoryBufferTrackerInProcess
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> I don't understand the in-process notion here. GpuMemoryBuffer will always be
> the client-side object, right?
> Regardless of whether it's the client to a GLImage in another process or not.

Yes, removed this file.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/image_factory.h (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:19: // Craete a GpuMemoryBuffer and
makes it available to the
On 2013/05/10 02:06:16, David Reveman wrote:
> nit: Create

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:23: virtual void
RemoveFromManager(unsigned int image_id) = 0;
On 2013/05/10 02:06:16, David Reveman wrote:
> I prefer DeleteGpuMemoryBuffer(unsigned image_id). The "Delete" prefix as
that's
> consistent with BrowserGpuChannelHostFactory and GpuChannel. The idea that
> there's an image manager for which something is inserted/removed is an
> implementation detail that I don't think this API should expose.

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:47: image_factory_ =
image_factory.get();
On 2013/05/10 02:06:16, David Reveman wrote:
> Saving a pointer and passing ownership to a different object is not great. I
> think all this would be much cleaner if GLES2Implementation didn't take
> ownership of the ImageFactory.

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp
File gpu/gpu.gyp (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp#newcode19
gpu/gpu.gyp:19: '../testing/gmock.gyp:gmock',
On 2013/05/10 00:12:22, greggman wrote:
> Mock should not be here. Mock is only used in unit tests, not in shipping
code. 

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp#newcode44
gpu/gpu.gyp:44: '../testing/gmock.gyp:gmock',
On 2013/05/10 00:12:22, greggman wrote:
> Mock should not be here. Mock is only used in unit tests, not in shipping code

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu.gyp#newcode67
gpu/gpu.gyp:67: '../testing/gmock.gyp:gmock',
On 2013/05/10 00:12:22, greggman wrote:
> Mock should not be here. Mock is only used in unit tests, not in shipping code

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_common.gypi
File gpu/gpu_common.gypi (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_common.gypi#newc...
gpu/gpu_common.gypi:42: 'command_buffer/client/gpu_memory_buffer_mock.cc',
On 2013/05/10 00:12:22, greggman wrote:
> Mocks do no belong here. They should get put in the testing target
> 
> If you use the mocks in more than one test target add a new target gpu_mocks
or
> something and make the test targets depend on that

Done.

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_untrusted.gyp
File gpu/gpu_untrusted.gyp (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/gpu_untrusted.gyp#ne...
gpu/gpu_untrusted.gyp:36: '../testing/gmock.gyp:gmock',
On 2013/05/10 00:12:22, greggman wrote:
> Mock should not be here. Mock is only used in unit tests, not in shipping code

Done.

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:49: using
gpu::gles2::ImageFactory;
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> nit: order

Done.

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:661: new
ImageFactoryInProcess(GetImageManager()));
On 2013/05/10 01:21:51, Daniel Sievers wrote:
> That means different clients in the share group will generate overlapping IDs
> and will try to use them with the same image manager.

Let me know how this change looks.

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h (right):

https://codereview.chromium.org/14456004/diff/127001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:406: /* virtual
void deleteImageBuffer(WebGLId); */
On 2013/05/10 00:12:22, greggman wrote:
> If this is no longer needed delete it? We don't generally leave commented out
> code in

Done.

[reveman, 2013-05-14 01:21:04]

lgtm with nits

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3667: mode =
GpuMemoryBuffer::READ_OR_WRITE;
Can we call this GpuMemoryBuffer::READ_WRITE instead?

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:624: ImageFactory*
image_factory_;  // weak
nit: no need for "weak" comment. that's already implied by not using scoped_ptr
or scoped_refptr.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:37: ImageFactory*
factory_;  // weak
nit: no need for "weak" comment. that's already implied by not using scoped_ptr
or scoped_refptr.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/image_factory.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:24: int width, int height, GLenum
internalformat, unsigned int* image_id) = 0;
nit: s/unsigned int/unsigned/

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:25: virtual void
DeleteGpuMemoryBuffer(unsigned int image_id) = 0;
nit: s/unsigned int/unsigned/

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/image_factory_mock.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory_mock.h:25: int width, int height, GLenum
internalformat, unsigned int* image_id));
nit: unsigned

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory_mock.h:26:
MOCK_METHOD1(DeleteGpuMemoryBuffer, void(unsigned int));
nit: unsigned

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory_mock.h:30: unsigned int* image_id)
OVERRIDE {
nit: unsigned

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:213: unsigned
int* image_id) OVERRIDE;
nit: unsigned

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:214: virtual
void DeleteGpuMemoryBuffer(unsigned int image_id) OVERRIDE;
nit: unsigned

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:217: unsigned
int next_id_;
nit: unsigned

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:251: width,
height);
nit: factory being a callback might be less confusing if you use temporary
variable and do:
create_gpu_memory_buffer_callback =
    ::gpu::gles2::GetProcessDefaultGpuMemoryBufferFactory();
buffer = create_gpu_memory_buffer_callback.Run(
    width, height);

[kaanb, 2013-05-14 18:12:11]

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3667: mode =
GpuMemoryBuffer::READ_OR_WRITE;
On 2013/05/14 01:21:04, David Reveman wrote:
> Can we call this GpuMemoryBuffer::READ_WRITE instead?

Done.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:624: ImageFactory*
image_factory_;  // weak
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: no need for "weak" comment. that's already implied by not using
scoped_ptr
> or scoped_refptr.

Done.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:37: ImageFactory*
factory_;  // weak
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: no need for "weak" comment. that's already implied by not using
scoped_ptr
> or scoped_refptr.

Done.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/image_factory.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:24: int width, int height, GLenum
internalformat, unsigned int* image_id) = 0;
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: s/unsigned int/unsigned/

Done.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory.h:25: virtual void
DeleteGpuMemoryBuffer(unsigned int image_id) = 0;
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: s/unsigned int/unsigned/

Done.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
File gpu/command_buffer/client/image_factory_mock.h (right):

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory_mock.h:25: int width, int height, GLenum
internalformat, unsigned int* image_id));
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: unsigned

Done.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory_mock.h:26:
MOCK_METHOD1(DeleteGpuMemoryBuffer, void(unsigned int));
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: unsigned

Done.

https://codereview.chromium.org/14456004/diff/142001/gpu/command_buffer/clien...
gpu/command_buffer/client/image_factory_mock.h:30: unsigned int* image_id)
OVERRIDE {
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: unsigned

Done.

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:213: unsigned
int* image_id) OVERRIDE;
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: unsigned

Done.

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:214: virtual
void DeleteGpuMemoryBuffer(unsigned int image_id) OVERRIDE;
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: unsigned

Done.

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:217: unsigned
int next_id_;
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: unsigned

Done.

https://codereview.chromium.org/14456004/diff/142001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:251: width,
height);
On 2013/05/14 01:21:04, David Reveman wrote:
> nit: factory being a callback might be less confusing if you use temporary
> variable and do:
> create_gpu_memory_buffer_callback =
>     ::gpu::gles2::GetProcessDefaultGpuMemoryBufferFactory();
> buffer = create_gpu_memory_buffer_callback.Run(
>     width, height);

Done.

[no sievers, 2013-05-14 21:10:02]

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/127001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3677: gpu_buffer->Map(mode,
&mapped_buffer);
On 2013/05/13 23:00:36, kaanb wrote:
> On 2013/05/10 01:21:51, Daniel Sievers wrote:
> > Can this fail and should we set a GL error if so?
> 
> From what I understand the documentation, the only failure mode is if the
flags
> used to create the buffer are inconsistent with the map access mode. Since it
> indicated a setup failure, currently I DCHECK in android_webview::Map() if
Map()
> (i.e. Lock()) fails.

If it's something the client can do wrong, then it should not be a DCHECK() but
a properly handled error.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3583: // that may refer to the
image_id are executed on the service side.
nit: There is actually no image_id yet. If the image_ids are always unique
enough, this flush is probably not needed. Maybe rather say that you are
ensuring ordering in case the newly returned image_id has recently been in use
with a different buffer.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:98: #define GL_READ_WRITE      
                             0x88BA
Can't you add it to the third party glext header where it's missing (I assume
it's missing in either khronos or Angle headers, whatever is included here)?

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:28: GLuint image_id = 0;
Maybe this should check for factory_ being NULL in here and in RemoveBuffer,
since you specifically allow this in GLES2Implementation now?

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.h (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:5: #ifndef
GPU_COMMAND_BUFFER_CLIENT_GPU_MEMORY_BUFFER_TRACKER_IN_PROCESS_H_
nit: update macro name to reflect class name

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:21: // Tracks GPU memory
buffer objects for client side of command buffer.
nit: 'for the client side of the' or 'on the client side' or so

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:24: // Ownership of
|factory| and |gl| remain with caller.
nit: |gl| seems to be obsolete

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:28: // Methods from
GpuMemoryBufferTracker
nit: comment is obsolete since these methods are not virtual anymore

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:31: virtual
GpuMemoryBuffer* GetBuffer(GLuint image_id) OVERRIDE;
make non-virtual and remove 'override'

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:37: class
GLGpuMemoryBufferTest : public testing::Test {
nit: MockMemoryBufferTest

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:73:
.WillOnce(DoAll(SetArgPointee<3>(345u),
nit: make a constant (i.e. 'const GLuint kImageId = ...') for this

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:85:
image_manager_->AddImage(gl_image.get(), image_id);
nit: AddImage() could be another action set up in the expectation above, which
will make things clearer and remove the need for the comment above why this was
done here.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:88:
.WillOnce(Return(false))  // before Map()
nit: If you split up and set the expectations separately before the call to
Map() and Unmap() it becomes clearer what is happening (the GLES2Implementation
checking that it's not already mapped).

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:184:
scoped_ptr< ::gpu::gles2::ImageFactory> image_factory_;
Instead of having the scoped_ptr here that only gets set when not sharing
resources, can you make ImageFactoryInProcess refcounted?

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:208: void
SetImageManager(ImageManager* image_manager);
Then this could go too.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:225:
next_id_(0) {
This could go.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:242:
base::AutoLock scoped_lock(lock_);
I think you actually need the global lock (AutoLockAndDecoderDetachThread) here,
because you are accessing the image manager here.

Imagine the case where the decoder looks up an image (from bindTexImage2D) on
one thread, while you are adding or removing an image through a client on
another thread.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:329:
LAZY_INSTANCE_INITIALIZER;
You probably don't need this if you go with the refcounting (see above) and...

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:677:
g_image_factory.Pointer()->SetImageManager(GetImageManager());
...if sharing resources grab the factory from the first context in
g_all_shared_contexts. But you need the lock to access that (see below).

[no sievers, 2013-05-14 21:22:06]

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:242:
base::AutoLock scoped_lock(lock_);
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> I think you actually need the global lock (AutoLockAndDecoderDetachThread)
here,
> because you are accessing the image manager here.
> 
> Imagine the case where the decoder looks up an image (from bindTexImage2D) on
> one thread, while you are adding or removing an image through a client on
> another thread.

Damn, this is slightly ugly. There is at least one case where we'd potentially
end up attempting to recursively acquire the mutex:
GLInProcessContext::Destroy() holds g_decoder_lock while destroying
GLES2Implementation, which owns the buffer tracker, which tries to call into
ImageFactoryInProcess::DeleteGpuMemoryBuffer() for leaked buffers.
It's possible that deleting gles2_implementation_ does not need the mutex.

[no sievers, 2013-05-14 21:55:53]

On 2013/05/14 21:22:06, Daniel Sievers wrote:
>
https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
> File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):
> 
>
https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
> webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:242:
> base::AutoLock scoped_lock(lock_);
> On 2013/05/14 21:10:03, Daniel Sievers wrote:
> > I think you actually need the global lock (AutoLockAndDecoderDetachThread)
> here,
> > because you are accessing the image manager here.
> > 
> > Imagine the case where the decoder looks up an image (from bindTexImage2D)
on
> > one thread, while you are adding or removing an image through a client on
> > another thread.
> 
> Damn, this is slightly ugly. There is at least one case where we'd potentially
> end up attempting to recursively acquire the mutex:
> GLInProcessContext::Destroy() holds g_decoder_lock while destroying
> GLES2Implementation, which owns the buffer tracker, which tries to call into
> ImageFactoryInProcess::DeleteGpuMemoryBuffer() for leaked buffers.
> It's possible that deleting gles2_implementation_ does not need the mutex.

Actually, yup that's a bug. ~GLES2Implementation() might flush and we would also
recursively acquire the lock.

You can scope the lock in GLInProcessContext::Destroy() around access to
decoder_.

[no sievers, 2013-05-14 22:18:08]

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:216:
scoped_refptr<ImageManager> image_manager_;
This is a problem too, since ImageManager is not RefCountedThreadSafe.

[no sievers, 2013-05-14 22:26:31]

For now, you could make it one ImageManager+ImageFactory per context. There does
not seem to be any need for sharing images across contexts right now with
WGC3DInProcessCB.

[kaanb, 2013-05-14 23:37:09]

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.cc:3583: // that may refer to the
image_id are executed on the service side.
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: There is actually no image_id yet. If the image_ids are always unique
> enough, this flush is probably not needed. Maybe rather say that you are
> ensuring ordering in case the newly returned image_id has recently been in use
> with a different buffer.

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:98: #define GL_READ_WRITE      
                             0x88BA
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> Can't you add it to the third party glext header where it's missing (I assume
> it's missing in either khronos or Angle headers, whatever is included here)?

I'll defer to gman@ on that

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.cc (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.cc:28: GLuint image_id = 0;
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> Maybe this should check for factory_ being NULL in here and in RemoveBuffer,
> since you specifically allow this in GLES2Implementation now?

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gpu_memory_buffer_tracker.h (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:5: #ifndef
GPU_COMMAND_BUFFER_CLIENT_GPU_MEMORY_BUFFER_TRACKER_IN_PROCESS_H_
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: update macro name to reflect class name

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:21: // Tracks GPU memory
buffer objects for client side of command buffer.
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: 'for the client side of the' or 'on the client side' or so

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:24: // Ownership of
|factory| and |gl| remain with caller.
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: |gl| seems to be obsolete

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:28: // Methods from
GpuMemoryBufferTracker
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: comment is obsolete since these methods are not virtual anymore

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/clien...
gpu/command_buffer/client/gpu_memory_buffer_tracker.h:31: virtual
GpuMemoryBuffer* GetBuffer(GLuint image_id) OVERRIDE;
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> make non-virtual and remove 'override'

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:37: class
GLGpuMemoryBufferTest : public testing::Test {
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: MockMemoryBufferTest

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:73:
.WillOnce(DoAll(SetArgPointee<3>(345u),
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: make a constant (i.e. 'const GLuint kImageId = ...') for this

Done.

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:85:
image_manager_->AddImage(gl_image.get(), image_id);
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: AddImage() could be another action set up in the expectation above, which
> will make things clearer and remove the need for the comment above why this
was
> done here.

So, do you want me to mock the ImageManager?

https://codereview.chromium.org/14456004/diff/156001/gpu/command_buffer/tests...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:88:
.WillOnce(Return(false))  // before Map()
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> nit: If you split up and set the expectations separately before the call to
> Map() and Unmap() it becomes clearer what is happening (the
GLES2Implementation
> checking that it's not already mapped).

Done.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:184:
scoped_ptr< ::gpu::gles2::ImageFactory> image_factory_;
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> Instead of having the scoped_ptr here that only gets set when not sharing
> resources, can you make ImageFactoryInProcess refcounted?

Done.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:208: void
SetImageManager(ImageManager* image_manager);
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> Then this could go too.

Done.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:216:
scoped_refptr<ImageManager> image_manager_;
On 2013/05/14 22:18:08, Daniel Sievers wrote:
> This is a problem too, since ImageManager is not RefCountedThreadSafe.

I believe this is fine since ImageManager is ref'd by the ContextGroup?

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:225:
next_id_(0) {
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> This could go.

Done.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:242:
base::AutoLock scoped_lock(lock_);
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> I think you actually need the global lock (AutoLockAndDecoderDetachThread)
here,
> because you are accessing the image manager here.
> 
> Imagine the case where the decoder looks up an image (from bindTexImage2D) on
> one thread, while you are adding or removing an image through a client on
> another thread.

Done.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:329:
LAZY_INSTANCE_INITIALIZER;
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> You probably don't need this if you go with the refcounting (see above) and...


Done.

https://codereview.chromium.org/14456004/diff/156001/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:677:
g_image_factory.Pointer()->SetImageManager(GetImageManager());
On 2013/05/14 21:10:03, Daniel Sievers wrote:
> ...if sharing resources grab the factory from the first context in
> g_all_shared_contexts. But you need the lock to access that (see below).

Done.

[greggman, 2013-05-15 00:26:52]

lgtm

https://codereview.chromium.org/14456004/diff/164001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/164001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:98: #define GL_READ_WRITE      
                             0x88BA
this should probably go in gpu/GLES2/gl2chromium.h or gl2extchromium.h

[no sievers, 2013-05-15 01:09:40]

lgtm. thanks!

https://codereview.chromium.org/14456004/diff/118002/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/118002/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:322:
ImageManager* image_manager_;
nit: I'd say 'the ContextGroup outlives the client' to indicate why the weak ptr
is fine.

https://codereview.chromium.org/14456004/diff/118002/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:338:
AutoLockAndDecoderDetachThread lock(g_decoder_lock.Get(),
nit: add a comment here and below that you are grabbing the lock here because of
access to the context group's shared image_manager_ (and for id generation
although that could probably be an atomic inc).

[kaanb, 2013-05-15 01:44:26]

Friendly ping for brettw, viettrungluu and raymes. I only have a one-line diff
in ppapi/ Please take a look.

https://codereview.chromium.org/14456004/diff/164001/gpu/command_buffer/clien...
File gpu/command_buffer/client/gles2_implementation.h (right):

https://codereview.chromium.org/14456004/diff/164001/gpu/command_buffer/clien...
gpu/command_buffer/client/gles2_implementation.h:98: #define GL_READ_WRITE      
                             0x88BA
On 2013/05/15 00:26:52, greggman wrote:
> this should probably go in gpu/GLES2/gl2chromium.h or gl2extchromium.h

I moved it gl2extchromium.h under the CHROMIUM_map_image guard.

https://codereview.chromium.org/14456004/diff/118002/webkit/gpu/webgraphicsco...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14456004/diff/118002/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:322:
ImageManager* image_manager_;
On 2013/05/15 01:09:41, Daniel Sievers wrote:
> nit: I'd say 'the ContextGroup outlives the client' to indicate why the weak
ptr
> is fine.

Done.

https://codereview.chromium.org/14456004/diff/118002/webkit/gpu/webgraphicsco...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:338:
AutoLockAndDecoderDetachThread lock(g_decoder_lock.Get(),
On 2013/05/15 01:09:41, Daniel Sievers wrote:
> nit: add a comment here and below that you are grabbing the lock here because
of
> access to the context group's shared image_manager_ (and for id generation
> although that could probably be an atomic inc).

Done.

[commit-bot: I haz the power, 2013-05-15 01:45:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/14456004/175001

[commit-bot: I haz the power, 2013-05-15 01:55:39]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[raymes, 2013-05-15 16:05:01]

I assume you know what you're doing but I don't really know anything about the
change. Could you explain what the ImageFactory is for and why it is ok for it
to be NULL when using GLES2Implementation in PPB_Graphics_3D? (or maybe add a
comment in the code if it makes sense).
Thanks!

[kaanb, 2013-05-15 17:17:43]

On 2013/05/15 16:05:01, raymes wrote:
> I assume you know what you're doing but I don't really know anything about the
> change. Could you explain what the ImageFactory is for and why it is ok for it
> to be NULL when using GLES2Implementation in PPB_Graphics_3D? (or maybe add a
> comment in the code if it makes sense).
> Thanks!

This patch is adding an interface to directly modify texture memory. It will be
initially used only for Android WebView. An ImageFactory manages the allocation
and the destruction of the buffers. Since your module isn't using this
optimization, ImageFactory can be NULL.

I have added a comment.

[raymes, 2013-05-15 17:26:29]

lgtm

[commit-bot: I haz the power, 2013-05-15 22:51:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/14456004/178002

[joth, 2013-05-15 23:10:21]

lgtm

[commit-bot: I haz the power, 2013-05-16 00:16:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/14456004/214001

[commit-bot: I haz the power, 2013-05-16 01:53:28]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-16 05:30:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/14456004/214001

[commit-bot: I haz the power, 2013-05-16 10:46:03]

Change committed as 200505