Add support for GL_CHROMIUM_pixel_transfer_buffer_object.

Add support for GL_CHROMIUM_pixel_transfer_buffer_object.

This adds two new types of buffer objects,
GL_PIXEL_PACK_TRANSFER_BUFFER_BINDING_CHROMIUM and
GL_PIXEL_UNPACK_TRANSFER_BUFFER_BINDING_CHROMIUM. The PIXEL_PACK buffer
affects API calls that pack pixel data, such as glReadPixels. The
PIXEL_UNPACK buffer affects API calls that unpack pixel data, such as
glTexImage2D. These new buffer object are backed by shared memory, which
allows clients to update them without any unnecessary copying.

BUG=111096, 161337
TEST=unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168999

[reveman, 2012-05-24 18:54:04]

Hi guys,

this is far from ready but I thought it would be good for you to see how this is
shaping up and provide some feedback before this CL grows too large.

The general idea is to allow buffers to be created and mapped using one command
buffer channel and then used for texture uploads by a different channel.

Currently the shared memory used for a pixel buffer object belongs to the client
context that called BufferData. This means that if a client's command buffer
channel is destroyed, so is the shared memory associated with any pixel buffers
created by that client. This is not a problem with how we currently plan to use
this extensions but I'm interested in what you guys think about this limitation?

A few caveats in the current CL:
- The GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM binding doesn't have any effect
right now.
- I'm currently creating GL buffer IDs for these new buffer types even though
they wont be used. This should be fixed but I avoided it in this first version
of the CL as it affects a lot of the unit tests and therefor makes the patch
much larger.
- GL_CHROMIUM_map_sub is currently used on the client side to map these buffers.
New API entries should be added for that instead.
- The BufferData command is currently used to create buffer objects that use
shared memory as storage. I think it would be more appropriate to use a new
command (maybe called TransferBufferData) for this. Sounds good?

Let me know asap if you have any objections to the direction this is going. I'll
upload a webkit patch that takes advantage of this extension asap so you can see
how it will be used.

Thanks,
David

[greggman, 2012-05-24 20:55:29]

On 2012/05/24 18:54:04, David Reveman wrote:
> Hi guys,
> 
> this is far from ready but I thought it would be good for you to see how this
is
> shaping up and provide some feedback before this CL grows too large.
> 
> The general idea is to allow buffers to be created and mapped using one
command
> buffer channel and then used for texture uploads by a different channel.
> 
> Currently the shared memory used for a pixel buffer object belongs to the
client
> context that called BufferData. This means that if a client's command buffer
> channel is destroyed, so is the shared memory associated with any pixel
buffers
> created by that client. This is not a problem with how we currently plan to
use
> this extensions but I'm interested in what you guys think about this
limitation?

I feel like BufferTracker should probably go on ShareGroup and BufferManager on
ContextGroup if possible. That might get rid of the problem of one context
freeing a buffer while the other is using it.

On top of that, it would also allow for OpenGL resource semantics in that of
ctx1 deletes a buffer ctx2 can continue to use if it was still bound.

not a small change probably because MappedMemoryManager might have to be shared
and whatever is managing mapped memory on the service (currently decoder->engine
would need to get moved to something hanging off of ContextGroup



> 
> A few caveats in the current CL:
> - The GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM binding doesn't have any effect
> right now.
> - I'm currently creating GL buffer IDs for these new buffer types even though
> they wont be used. This should be fixed but I avoided it in this first version
> of the CL as it affects a lot of the unit tests and therefor makes the patch
> much larger.
> - GL_CHROMIUM_map_sub is currently used on the client side to map these
buffers.
> New API entries should be added for that instead.
> - The BufferData command is currently used to create buffer objects that use
> shared memory as storage. I think it would be more appropriate to use a new
> command (maybe called TransferBufferData) for this. Sounds good?
> 
> Let me know asap if you have any objections to the direction this is going.
I'll
> upload a webkit patch that takes advantage of this extension asap so you can
see
> how it will be used.
> 
> Thanks,
> David

[greggman, 2012-05-24 20:56:00]

Hey Al, thoughts on this?

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
File gpu/command_buffer/client/gles2_implementation.cc (right):

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:1511: case
GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
style: case is indented from switch

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:1521:
buffer_tracker_->RemoveBuffer(buffer_id);
Should this happen inside buffer_tracker as in something like
buffer_tracker_->ResizeBuffer or AllocateBuffer ?  It seems like it would be
nice to avoid reallocation if the buffer is the same size.

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:2704: case
GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
style: indent

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:2715: } break;
style: breaks go inside case blocks

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:2715: } break;
style: break goes inside case block

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:2747: case
GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM:
indent

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:2762:
mapped_buffers_.erase(it);
I'm not sure why you'd erase the buffer here but then I guess like you said
you're going to add new entry points instead of using MapSubBuffer and
UnmapSubBuffer

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
File gpu/command_buffer/service/buffer_manager.cc (right):

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
gpu/command_buffer/service/buffer_manager.cc:143: int8* shm_data =
decoder_->GetSharedMemoryAs<int8*>(
I'm confused. Why would you need to copy memory in set range? If the whole point
is that you're using shared memory so that client has already put the data in
the buffer. Am I mis-understanding

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
gpu/command_buffer/service/buffer_manager.cc:159: if (decoder_) {
If this is a shared memory buffer should this go before the shadowed_ check?

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
File gpu/command_buffer/service/buffer_manager.h (right):

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
gpu/command_buffer/service/buffer_manager.h:164: base::WeakPtr<CommonDecoder>
decoder_;
I'm a little leary of tracking decoders per buffer though I don't see another
way a the moment.

But, since shared contexts now have to be in the same process maybe the shared
memory management should move from CommonDecoder to some object hanging off of
ContextGroup

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/gle...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/gle...
gpu/command_buffer/service/gles2_cmd_decoder.cc:3762: if
(bound_pixel_unpack_transfer_buffer_) {
note to gman: revisit. Does this support traditional GL sharing semantics.
Namely if you delete a buffer in ctx1 it's still useable in ctx2 if it's bound

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/gle...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6687: case
GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM:
style: indent

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/gle...
gpu/command_buffer/service/gles2_cmd_decoder.cc:6705: } break;
style: break goes in case block

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/gle...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7398: uint32 pixels_size) {
pixels_size doesn't appear to be used. Was there a reason to add it?

[apatrick_chromium, 2012-05-24 21:48:31]

A high level question, perhaps just indicative of my misunderstanding of what
you're doing here. Why not add support for ARB_pixel_buffer_object and add one
new buffer target:

GL_TRANSFER_BUFFER_CHROMIUM

When a buffer is bound to this target, BufferData would give the buffer bound to
the target the new semantics for allocation, filling, mapping and unmapping.
Then the buffer could be rebound to one of the existing targets
(GL_PIXEL_PACK_BUFFER, GL_PIXEL_UNPACK_BUFFER, GL_ARRAY_BUFFER or
GL_ELEMENT_ARRAY_BUFFER). It seems to me that allocating a buffer in a special
way is orthogonal to the way the buffer will later be used. See
AMD_pinned_memory for an example of something related:

http://www.opengl.org/registry/specs/AMD/pinned_memory.txt

Also, I agree with Gregg that the management of these buffers should be at the
context group level rather than the context level.

[piman, 2012-05-24 21:56:41]

On Thu, May 24, 2012 at 2:48 PM, <apatrick@chromium.org> wrote:

> A high level question, perhaps just indicative of my misunderstanding of
> what
> you're doing here. Why not add support for ARB_pixel_buffer_object and add
> one
> new buffer target:
>

I think part of the issue is that the underlying GL may not support PBOs,
but the semantics of PBOs are that a buffer created for one target (say
ARRAY_BUFFER) can be used with another target (say, PIXEL_UNPACK_BUFFER),
which is something we could not support if we were trying to expose PBO,
hence the call for a different name I guess. This is a feature that is
useful and can be implemented independently of the underlying GL.


>
> GL_TRANSFER_BUFFER_CHROMIUM
>
> When a buffer is bound to this target, BufferData would give the buffer
> bound to
> the target the new semantics for allocation, filling, mapping and
> unmapping.
> Then the buffer could be rebound to one of the existing targets
> (GL_PIXEL_PACK_BUFFER, GL_PIXEL_UNPACK_BUFFER, GL_ARRAY_BUFFER or
> GL_ELEMENT_ARRAY_BUFFER). It seems to me that allocating a buffer in a
> special
> way is orthogonal to the way the buffer will later be used. See
> AMD_pinned_memory for an example of something related:
>
>
http://www.opengl.org/**registry/specs/AMD/pinned_**memory.txt<http://www.ope...
>
> Also, I agree with Gregg that the management of these buffers should be at
> the
> context group level rather than the context level.
>

Agreed too.

Antoine


>
http://codereview.chromium.**org/10440019/<http://codereview.chromium.org/104...
>

[apatrick_chromium, 2012-05-24 22:05:49]

On 2012/05/24 21:56:41, piman wrote:
> On Thu, May 24, 2012 at 2:48 PM, <mailto:apatrick@chromium.org> wrote:
> 
> > A high level question, perhaps just indicative of my misunderstanding of
> > what
> > you're doing here. Why not add support for ARB_pixel_buffer_object and add
> > one
> > new buffer target:
> >
> 
> I think part of the issue is that the underlying GL may not support PBOs,
> but the semantics of PBOs are that a buffer created for one target (say
> ARRAY_BUFFER) can be used with another target (say, PIXEL_UNPACK_BUFFER),
> which is something we could not support if we were trying to expose PBO,
> hence the call for a different name I guess. This is a feature that is
> useful and can be implemented independently of the underlying GL.
> 

I can't think of any reason why we can't emulate PBOs, even if the underlying GL
does not support them?

Independent of that issue, even if GL_PIXEL_PACK_BUFFER and
GL_PIXEL_UNPACK_BUFFER could _only_ be used with buffers created while bound to
GL_TRANSFER_BUFFER_CHROMIUM in the context of this extension, I think that would
be a better abstraction. For example, it will be less awkward if we implement
PBOs in the future. Then we don't have to have rules about which buffer takes
precedence if both GL_PIXEL_PACK_BUFFER and
GL_PIXEL_PACK_TRANSFER_BUFFER_CHROMIUM have buffers bound to them.

[piman, 2012-05-24 22:18:49]

On Thu, May 24, 2012 at 3:05 PM, <apatrick@chromium.org> wrote:

> On 2012/05/24 21:56:41, piman wrote:
>
>  On Thu, May 24, 2012 at 2:48 PM, <mailto:apatrick@chromium.org> wrote:
>>
>
>  > A high level question, perhaps just indicative of my misunderstanding of
>> > what
>> > you're doing here. Why not add support for ARB_pixel_buffer_object and
>> add
>> > one
>> > new buffer target:
>> >
>>
>
>  I think part of the issue is that the underlying GL may not support PBOs,
>> but the semantics of PBOs are that a buffer created for one target (say
>> ARRAY_BUFFER) can be used with another target (say, PIXEL_UNPACK_BUFFER),
>> which is something we could not support if we were trying to expose PBO,
>> hence the call for a different name I guess. This is a feature that is
>> useful and can be implemented independently of the underlying GL.
>>
>
>
> I can't think of any reason why we can't emulate PBOs, even if the
> underlying GL
> does not support them?
>

glBindBuffer(GL_ARRAY_BUFFER, 1)
glBufferData(GL_ARRAY_BUFFER, stuff)  // stuff gets sent to the GPU
glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 1)
glTexImage2D(GL_TEXTURE_2D, ..., NULL) // need to grab the bits from the
GPU buffer back to the CPU if PBO is not supported - we can't on GLES2
without some twisted way since there's no non-WRITE_ONLY map

We could always shadow the buffers, but that's wasteful, and defeats the
purpose of this API which is to save a copy.


> Independent of that issue, even if GL_PIXEL_PACK_BUFFER and
> GL_PIXEL_UNPACK_BUFFER could _only_ be used with buffers created while
> bound to
> GL_TRANSFER_BUFFER_CHROMIUM in the context of this extension, I think that
> would
> be a better abstraction. For example, it will be less awkward if we
> implement
> PBOs in the future. Then we don't have to have rules about which buffer
> takes
> precedence if both GL_PIXEL_PACK_BUFFER and
> GL_PIXEL_PACK_TRANSFER_BUFFER_**CHROMIUM have buffers bound to them.
>

Yeah, I could buy that, if we can make the semantics work.


>
>
http://codereview.chromium.**org/10440019/<http://codereview.chromium.org/104...
>

[google-reveman, 2012-05-25 15:27:56]

On Thu, May 24, 2012 at 4:55 PM,  <gman@chromium.org> wrote:
> On 2012/05/24 18:54:04, David Reveman wrote:
>>
>> Hi guys,
>
>
>> this is far from ready but I thought it would be good for you to see how
>> this
>
> is
>>
>> shaping up and provide some feedback before this CL grows too large.
>
>
>> The general idea is to allow buffers to be created and mapped using one
>
> command
>>
>> buffer channel and then used for texture uploads by a different channel.
>
>
>> Currently the shared memory used for a pixel buffer object belongs to the
>
> client
>>
>> context that called BufferData. This means that if a client's command
>> buffer
>> channel is destroyed, so is the shared memory associated with any pixel
>
> buffers
>>
>> created by that client. This is not a problem with how we currently plan
>> to
>
> use
>>
>> this extensions but I'm interested in what you guys think about this
>
> limitation?
>
> I feel like BufferTracker should probably go on ShareGroup and BufferManager
> on
> ContextGroup if possible. That might get rid of the problem of one context
> freeing a buffer while the other is using it.
>
> On top of that, it would also allow for OpenGL resource semantics in that of
> ctx1 deletes a buffer ctx2 can continue to use if it was still bound.
>
> not a small change probably because MappedMemoryManager might have to be
> shared
> and whatever is managing mapped memory on the service (currently
> decoder->engine
> would need to get moved to something hanging off of ContextGroup

Yea, doing this at the context group level would be much better. I'll
see what I can do.

[gman1, 2012-05-25 15:42:40]

If you want I can look into moving MappedMemoryManager to the ShareGroup
and the things that entails.

That's not a small amount of work I think.

[gman1, 2012-05-25 15:44:01]

On Fri, May 25, 2012 at 8:42 AM, Gregg Tavares (社用) <gman@google.com> wrote:

> If you want I can look into moving MappedMemoryManager to the ShareGroup
> and the things that entails.
>
> That's not a small amount of work I think.
>
>
>
note: that implies I'd also have to look into moving the service side
shared memory management from each decoder
to the the ContextGroup

[reveman, 2012-05-25 15:45:10]

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
File gpu/command_buffer/client/gles2_implementation.cc (right):

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/client/gles...
gpu/command_buffer/client/gles2_implementation.cc:1521:
buffer_tracker_->RemoveBuffer(buffer_id);
On 2012/05/24 20:56:00, greggman wrote:
> Should this happen inside buffer_tracker as in something like
> buffer_tracker_->ResizeBuffer or AllocateBuffer ?  It seems like it would be
> nice to avoid reallocation if the buffer is the same size. 

I agree. will fix this.

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
File gpu/command_buffer/service/buffer_manager.cc (right):

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
gpu/command_buffer/service/buffer_manager.cc:143: int8* shm_data =
decoder_->GetSharedMemoryAs<int8*>(
On 2012/05/24 20:56:00, greggman wrote:
> I'm confused. Why would you need to copy memory in set range? If the whole
point
> is that you're using shared memory so that client has already put the data in
> the buffer. Am I mis-understanding

This is for when a client calls glBufferSubData, which should really be handled
on the client side. This can be removed once that's fixed.

http://codereview.chromium.org/10440019/diff/1/gpu/command_buffer/service/buf...
gpu/command_buffer/service/buffer_manager.cc:159: if (decoder_) {
On 2012/05/24 20:56:00, greggman wrote:
> If this is a shared memory buffer should this go before the shadowed_ check?

Doesn't really matter as making a buffer a shm buffer makes shadowed_ = false.

[google-reveman, 2012-05-25 15:49:36]

On Fri, May 25, 2012 at 11:42 AM, Gregg Tavares (社用) <gman@google.com> wrote:
> If you want I can look into moving MappedMemoryManager to the ShareGroup and
> the things that entails.
>
> That's not a small amount of work I think.

Yes, would be great if you could do that!

[gman1, 2012-05-25 15:50:50]

On Fri, May 25, 2012 at 8:49 AM, David Reveman <reveman@google.com> wrote:

> On Fri, May 25, 2012 at 11:42 AM, Gregg Tavares (社用) <gman@google.com>
> wrote:
> > If you want I can look into moving MappedMemoryManager to the ShareGroup
> and
> > the things that entails.
> >
> > That's not a small amount of work I think.
>
> Yes, would be great if you could do that!
>

Ok, I'm on it

[greggman, 2012-06-01 20:23:27]

On 2012/05/25 15:50:50, gman1 wrote:
> On Fri, May 25, 2012 at 8:49 AM, David Reveman <mailto:reveman@google.com>
wrote:
> 
> > On Fri, May 25, 2012 at 11:42 AM, Gregg Tavares (社用)
<mailto:gman@google.com>
> > wrote:
> > > If you want I can look into moving MappedMemoryManager to the ShareGroup
> > and
> > > the things that entails.
> > >
> > > That's not a small amount of work I think.
> >
> > Yes, would be great if you could do that!
> >
> 
> Ok, I'm on it

So just FYI,

I have the mapped memory shared on the service side which from the client's pov
means if 2 contexts are in the same ShareGroup/ContextGroup then they can use
each other's shared memory ids and offsets.

But, I'm running into problems on the client side and need to think about it a
little. I was going to move MappedMemoryManager to ShareGroup but the problem is
freeing memory in the MappedMemoryManager is dependent on tokens in a specific
context. In other words it needs to know when the memory is no longer in use.

I could make it track a context per free. In other words, FreePendingToken would
take a context for that token. That might be the easiest solution though I want
to think about it a little more.

[google-reveman, 2012-06-01 20:48:06]

On Fri, Jun 1, 2012 at 4:23 PM,  <gman@chromium.org> wrote:
> On 2012/05/25 15:50:50, gman1 wrote:
>>
>> On Fri, May 25, 2012 at 8:49 AM, David Reveman <mailto:reveman@google.com>
>
> wrote:
>
>> > On Fri, May 25, 2012 at 11:42 AM, Gregg Tavares (社用)
>
> <mailto:gman@google.com>
>>
>> > wrote:
>> > > If you want I can look into moving MappedMemoryManager to the
>> > > ShareGroup
>> > and
>> > > the things that entails.
>> > >
>> > > That's not a small amount of work I think.
>> >
>> > Yes, would be great if you could do that!
>> >
>
>
>> Ok, I'm on it
>
>
> So just FYI,
>
> I have the mapped memory shared on the service side which from the client's
> pov
> means if 2 contexts are in the same ShareGroup/ContextGroup then they can
> use
> each other's shared memory ids and offsets.
>
> But, I'm running into problems on the client side and need to think about it
> a
> little. I was going to move MappedMemoryManager to ShareGroup but the
> problem is
> freeing memory in the MappedMemoryManager is dependent on tokens in a
> specific
> context. In other words it needs to know when the memory is no longer in
> use.
>
> I could make it track a context per free. In other words, FreePendingToken
> would
> take a context for that token. That might be the easiest solution though I
> want
> to think about it a little more.
>
>
>
> http://codereview.chromium.org/10440019/

OK, thanks for the update! I haven't had time to work on
pixel_transfer_buffer_object patch this week. I think we should be
able to make this a client side only extension once the mapped memory
is part of the ShareGroup and that should simplify things
significantly.

David

[reveman, 2012-11-15 22:00:09]

Hi,

this a client side only implementation of
GL_CHROMIUM_pixel_transfer_buffer_object. It provides a way to implement the
following functionality needed for impl side painting:

- Perform raster directly to shared memory.
- Abort uploads after shared memory allocation has taken place.
- Manage the lifetime of shared memory in a way that allows texture uploads do
be done when the gpu process is idle or on a separate thread in gpu process.

Please have a look asap.

[apatrick_chromium, 2012-11-15 22:18:38]

LGTM for my part but make sure gman signs off.

https://codereview.chromium.org/10440019/diff/28001/gpu/command_buffer/common...
File gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h (right):

https://codereview.chromium.org/10440019/diff/28001/gpu/command_buffer/common...
gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h:549: {
0x88EC, "GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM", },
Should use IDs between 0x6000 and 0x7FFF for development purposes.

[reveman, 2012-11-16 01:59:38]

https://codereview.chromium.org/10440019/diff/28001/gpu/command_buffer/common...
File gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h (right):

https://codereview.chromium.org/10440019/diff/28001/gpu/command_buffer/common...
gpu/command_buffer/common/gles2_cmd_utils_implementation_autogen.h:549: {
0x88EC, "GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM", },
On 2012/11/15 22:18:38, apatrick_chromium wrote:
> Should use IDs between 0x6000 and 0x7FFF for development purposes.

Done.

[greggman, 2012-11-19 04:50:14]

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
File gpu/command_buffer/client/buffer_tracker_unittest.cc (right):

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/buffer_tracker_unittest.cc:54: BufferTracker::Buffer*
buffer = buffer_tracker_->CreateBuffer(kId1, size);
This isn't really checking the buffer actually allocated shared memory. Should
it?

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/buffer_tracker_unittest.cc:61:
buffer_tracker_->RemoveBuffer(kId1);
This isn't checking the shared memory was freed, should it?

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

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:912: if
(share_group_->bind_generates_resource()) {
don't need this if. As this is a client side implementation you never want to
pass the query on to the service. Just always set *param and return true

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1231: if (target !=
GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM)
this is redundant. You already set this in BindBufferHelper. You can remove the
"'impl_func': False," you added to BindBuffer in build_gles2_cmd_decoder.py

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1666: }
should BufferSubData also work?

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1772: BufferTracker::Buffer*
buffer = buffer_tracker_->GetBuffer(
It seems like this code and the code above and twice below is repeated. should
it be a function? IsBoundPixelUnpackTransferBufferValid(func_name, offset, size)
or something?

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3617: if (target !=
GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM) {
Looking at glMapBuffer you're not aloud to map a buffer twice nor unmap it
twice. You're also not allowed to use a mapped buffer. Meaning you shouldn't be
able to call all the texture functions if the buffer has not been unmapped.
Should you track if it's mapped?

[reveman, 2012-11-20 16:49:30]

The latest patch should address all review feedback. PTAL.

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
File gpu/command_buffer/client/buffer_tracker_unittest.cc (right):

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/buffer_tracker_unittest.cc:54: BufferTracker::Buffer*
buffer = buffer_tracker_->CreateBuffer(kId1, size);
On 2012/11/19 04:50:14, greggman wrote:
> This isn't really checking the buffer actually allocated shared memory. Should
> it?

Done.

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/buffer_tracker_unittest.cc:61:
buffer_tracker_->RemoveBuffer(kId1);
On 2012/11/19 04:50:14, greggman wrote:
> This isn't checking the shared memory was freed, should it?

Done.

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

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:912: if
(share_group_->bind_generates_resource()) {
On 2012/11/19 04:50:14, greggman wrote:
> don't need this if. As this is a client side implementation you never want to
> pass the query on to the service. Just always set *param and return true

Done.

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1231: if (target !=
GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM)
On 2012/11/19 04:50:14, greggman wrote:
> this is redundant. You already set this in BindBufferHelper. You can remove
the
> "'impl_func': False," you added to BindBuffer in build_gles2_cmd_decoder.py

Done.

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1666: }
On 2012/11/19 04:50:14, greggman wrote:
> should BufferSubData also work?

Done. We don't need it but I feel like it should be supported for completeness
sake.

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1772: BufferTracker::Buffer*
buffer = buffer_tracker_->GetBuffer(
On 2012/11/19 04:50:14, greggman wrote:
> It seems like this code and the code above and twice below is repeated. should
> it be a function? IsBoundPixelUnpackTransferBufferValid(func_name, offset,
size)
> or something?

Done. Made it GetBoundPixelUnpackTransferBufferIfValid(function_name, ..)

https://codereview.chromium.org/10440019/diff/35002/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3617: if (target !=
GL_PIXEL_UNPACK_TRANSFER_BUFFER_CHROMIUM) {
On 2012/11/19 04:50:14, greggman wrote:
> Looking at glMapBuffer you're not aloud to map a buffer twice nor unmap it
> twice. You're also not allowed to use a mapped buffer. Meaning you shouldn't
be
> able to call all the texture functions if the buffer has not been unmapped.
> Should you track if it's mapped?

Done.

[greggman, 2012-11-20 18:57:41]

lgtm with a few minor nits

I think the general rule (although it's not 100%)

INVALID_ENUM if it's an enum
INVALID_VALUE if it's always invalid (like -1 for length)
otherwise, if it's dependent on state then
INVALID_OPERATION

I looked up glMapBuffer for mapped errors and glGetUniformLocation for bad id
errors and both seemed to generate INVALID_OPERATION for those situations.

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

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1662: if
(!SafeAddInt32(offset, size, &end) || end > buffer_size) {
technically you don't need safeadd on the renderer side. If the renderer, which
is untrusted code, does something bad it crashes. For example 
glBufferSubData(target, offset, 1, NULL) will crash

no need to change it per-se I just thought I'd point it out.

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1717:
SetGLError(GL_INVALID_VALUE, function_name, "invalid buffer");
INVALID_OPERATION

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1721:
SetGLError(GL_INVALID_VALUE, function_name, "buffer mapped");
GL_INVALID_OPERATION

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3623:
SetGLError(GL_INVALID_VALUE, "glMapBufferCHROMIUM", "invalid buffer");
GL_INVALID_OPERATION

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3627:
SetGLError(GL_INVALID_ENUM, "glMapBufferCHROMIUM", "already mapped");
GL_INVALID_OPERATION

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3648:
SetGLError(GL_INVALID_VALUE, "glMapBufferCHROMIUM", "invalid buffer");
GL_INVALID_OPERATION

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3652:
SetGLError(GL_INVALID_ENUM, "glMapBufferCHROMIUM", "not mapped");
GL_INVALID_OPERATION

[reveman, 2012-11-20 23:24:30]

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

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1662: if
(!SafeAddInt32(offset, size, &end) || end > buffer_size) {
On 2012/11/20 18:57:42, greggman wrote:
> technically you don't need safeadd on the renderer side. If the renderer,
which
> is untrusted code, does something bad it crashes. For example 
> glBufferSubData(target, offset, 1, NULL) will crash
> 
> no need to change it per-se I just thought I'd point it out.

Ok, makes sense.

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1717:
SetGLError(GL_INVALID_VALUE, function_name, "invalid buffer");
On 2012/11/20 18:57:42, greggman wrote:
> INVALID_OPERATION

Done.

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:1721:
SetGLError(GL_INVALID_VALUE, function_name, "buffer mapped");
On 2012/11/20 18:57:42, greggman wrote:
> GL_INVALID_OPERATION

Done.

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3623:
SetGLError(GL_INVALID_VALUE, "glMapBufferCHROMIUM", "invalid buffer");
On 2012/11/20 18:57:42, greggman wrote:
> GL_INVALID_OPERATION

Done.

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3627:
SetGLError(GL_INVALID_ENUM, "glMapBufferCHROMIUM", "already mapped");
On 2012/11/20 18:57:42, greggman wrote:
> GL_INVALID_OPERATION

Done.

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3648:
SetGLError(GL_INVALID_VALUE, "glMapBufferCHROMIUM", "invalid buffer");
On 2012/11/20 18:57:42, greggman wrote:
> GL_INVALID_OPERATION

Done.

https://codereview.chromium.org/10440019/diff/37001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:3652:
SetGLError(GL_INVALID_ENUM, "glMapBufferCHROMIUM", "not mapped");
On 2012/11/20 18:57:42, greggman wrote:
> GL_INVALID_OPERATION

Done.

[commit-bot: I haz the power, 2012-11-20 23:51:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10440019/39003

[commit-bot: I haz the power, 2012-11-20 23:51:44]

Presubmit check for 10440019-39003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ppapi/native_client

Presubmit checks took 2.5s to calculate.

[reveman, 2012-11-21 00:41:03]

ppapi/native_client owner lgtm please

[bbudge, 2012-11-21 00:55:41]

lgtm

[bbudge, 2012-11-21 00:55:41]

lgtm

lgtm

[commit-bot: I haz the power, 2012-11-21 00:58:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10440019/39003

[commit-bot: I haz the power, 2012-11-21 03:57:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10440019/27056

[commit-bot: I haz the power, 2012-11-21 05:17:41]

Retried try job too often for step(s) content_unittests

[commit-bot: I haz the power, 2012-11-21 05:34:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10440019/35046

[commit-bot: I haz the power, 2012-11-21 07:30:32]

Change committed as 168999