Add more ES3 commands to GPU command buffer: ClearBuffer*

Add more ES3 commands to GPU command buffer: ClearBuffer*

glClearBufferiv
glClearBufferuiv
glClearBufferfv
glClearBufferfi

BUG=429053
TEST=gpu_unittests
R=sievers@chromium.org

Committed: https://crrev.com/e1d1c69631fdb7bf71f197f62c963882af31481a
Cr-Commit-Position: refs/heads/master@{#311979}

[Zhenyao Mo, 2015-01-16 01:07:47]

Patchset #1 (id:1) has been deleted

[Zhenyao Mo, 2015-01-16 01:08:23]

Patchset #1 (id:20001) has been deleted

[Zhenyao Mo, 2015-01-16 01:08:32]

Patchset #1 (id:40001) has been deleted

[Zhenyao Mo, 2015-01-16 01:08:41]

Patchset #1 (id:60001) has been deleted

[Zhenyao Mo, 2015-01-16 01:08:50]

Patchset #1 (id:80001) has been deleted

[Zhenyao Mo, 2015-01-16 01:09:04]

Patchset #1 (id:100001) has been deleted

[Zhenyao Mo, 2015-01-16 01:11:30]

zmo@chromium.org changed reviewers:
+ sievers@chromium.org

[Zhenyao Mo, 2015-01-16 01:12:35]

Daniel: I can ask you to review this one?
Others: FYI (and feel free to review if you'd like)

[Zhenyao Mo, 2015-01-16 18:56:14]

On 2015/01/16 01:12:35, Zhenyao Mo wrote:
> Daniel: I can ask you to review this one?
> Others: FYI (and feel free to review if you'd like)

The telemetry failure looks unrelated.  All other bots are green.

[no sievers, 2015-01-16 19:28:46]

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:1548: 'type': 'PUT',
Does using 'PUTn' fix the generated code (see comments)?

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation_impl_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_impl_autogen.h:249:
helper_->ClearBufferfvImmediate(buffer, drawbuffers, value);
It doesn't pass |count| here.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_format_autogen.h:907: return
static_cast<uint32_t>(sizeof(GLfloat) * 4);
So here is a problem too.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:413: if
(!ComputeDataSize(1, sizeof(GLfloat), 4, &data_size)) {
This is weird, since it uses constants only, i.e. count wasn't passed in the
command buffer.
I think we might also be confusing |count| here if I compare it to the other
call sites for ComputeDataSize() (which pass |count| through the command
buffer): For these commands the count is always 1, but |elements_per_unit|
varies (1 vs. 4). Not sure if it matters, but we have to be careful.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:420:
GetImmediateDataAs<const GLfloat*>(c, data_size, immediate_data_size);
This is a bug now: |data_size| will always be 4*4, although we might only be
passing one float.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:439: if
(!ComputeDataSize(1, sizeof(GLint), 4, &data_size)) {
same as above

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:446:
GetImmediateDataAs<const GLint*>(c, data_size, immediate_data_size);
see above

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:465: if
(!ComputeDataSize(1, sizeof(GLuint), 4, &data_size)) {
This is correct since it's always 4, but redundant, since it's constant.

[Zhenyao Mo, 2015-01-16 20:52:33]

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:1548: 'type': 'PUT',
On 2015/01/16 19:28:46, sievers wrote:
> Does using 'PUTn' fix the generated code (see comments)?

PUT is the right command.  PUTn requires a count parameter that's not in the
function signature.  So if we go down the PUTn path, there are some extra
complexities that's not necessary if we go down the PUT path.

The only drawback is in some cases we waste 3 numbers (as no matter what, we
always send over 4 numbers).  But I don't think that's a big deal.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation_impl_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_impl_autogen.h:249:
helper_->ClearBufferfvImmediate(buffer, drawbuffers, value);
On 2015/01/16 19:28:46, sievers wrote:
> It doesn't pass |count| here.

No it didn't.  Because the |buffer| will indicate we need 4 or 1, (or 0 if
buffer or drawbuffers are invalid), so no need for an extra count.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_format_autogen.h:907: return
static_cast<uint32_t>(sizeof(GLfloat) * 4);
On 2015/01/16 19:28:46, sievers wrote:
> So here is a problem too.

See my explanation in the service side decoder.  This is correct as the command
size is 4 numbers.  I added the below function ComputeEffectiveDataSize() to
indicate among this 4 numbers, how many are actually used.  Even this doesn't
matter on the service side, because we just pass a pointer to 4 numbers to the
underlying function, and they will read 4 or less.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:413: if
(!ComputeDataSize(1, sizeof(GLfloat), 4, &data_size)) {
On 2015/01/16 19:28:46, sievers wrote:
> This is weird, since it uses constants only, i.e. count wasn't passed in the
> command buffer.
> I think we might also be confusing |count| here if I compare it to the other
> call sites for ComputeDataSize() (which pass |count| through the command
> buffer): For these commands the count is always 1, but |elements_per_unit|
> varies (1 vs. 4). Not sure if it matters, but we have to be careful.

See the below comment, command is of constant size, always getting 4 numbers,
and in some cases, 3 of them are 0 and ignored.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:420:
GetImmediateDataAs<const GLfloat*>(c, data_size, immediate_data_size);
On 2015/01/16 19:28:46, sievers wrote:
> This is a bug now: |data_size| will always be 4*4, although we might only be
> passing one float.

The design is this:  on the client side, the command always have count=4.  For
GL_DEPTH and GL_STENCIL, only 1 is used, the remaining is set to 0; on the
service side, we always obtain 4 numbers, but the underlying GL will only pick
up whatever it needs, up to 4.

So with this design, the command is of constant length, that's how we could use
Immediate data mode instead of other modes supporting varying data size.  In a
few situations, 3 numbers are wasted (all 0 and never used), but I think the
overhead is very limited, and the code becomes much simpler.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:439: if
(!ComputeDataSize(1, sizeof(GLint), 4, &data_size)) {
On 2015/01/16 19:28:46, sievers wrote:
> same as above

Same answer.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:446:
GetImmediateDataAs<const GLint*>(c, data_size, immediate_data_size);
On 2015/01/16 19:28:46, sievers wrote:
> see above

Same answer.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:465: if
(!ComputeDataSize(1, sizeof(GLuint), 4, &data_size)) {
On 2015/01/16 19:28:46, sievers wrote:
> This is correct since it's always 4, but redundant, since it's constant.

It is still necessary to make sure we at least send over 4 numbers (data_size >
immediate_data_size) so the underlying glClearBufferuiv won't have out of bounds
read.

[no sievers, 2015-01-16 22:06:23]

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_format_autogen.h:926:
ComputeEffectiveDataSize(buffer));
yea sorry, I missed this. The generated code is a bit misleading, but I guess it
works. If we end up with more commands of this type we could revisit and
consider making a PUTn handler version where the |n |is dependent on some of the
input args like here.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:397: GLint stencil =
static_cast<GLint>(c.stencil);
Did it somehow not generate the validation code for any of these functions?

Also, should we validate |drawbuffers| also? I.e. that it's a valid one for
COLOR, and otherwise 0?
Might have to write CustomDoxxx() handlers in gles2_cmd_decoder.cc though for
the additional checking.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h:461: }
Note that it also didn't generate the tests for GL_RENDERBUFFER as an invalid
arg.

[Zhenyao Mo, 2015-01-16 22:17:48]

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
File gpu/command_buffer/common/gles2_cmd_format_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/comm...
gpu/command_buffer/common/gles2_cmd_format_autogen.h:926:
ComputeEffectiveDataSize(buffer));
On 2015/01/16 22:06:23, sievers wrote:
> yea sorry, I missed this. The generated code is a bit misleading, but I guess
it
> works. If we end up with more commands of this type we could revisit and
> consider making a PUTn handler version where the |n |is dependent on some of
the
> input args like here.

Acknowledged.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:397: GLint stencil =
static_cast<GLint>(c.stencil);
On 2015/01/16 22:06:23, sievers wrote:
> Did it somehow not generate the validation code for any of these functions?
> 
> Also, should we validate |drawbuffers| also? I.e. that it's a valid one for
> COLOR, and otherwise 0?
> Might have to write CustomDoxxx() handlers in gles2_cmd_decoder.cc though for
> the additional checking.

Yes it's by design.  Look at the top of this function, it is behind a flag right
now.  All these "unsafe" functions, the validation will all be done by MANGLE,
not in command buffer.  We don't have MANGLE in place yet, so it's behind a
flag.  Ultimately most of the existing validation will all go away.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h:461: }
On 2015/01/16 22:06:23, sievers wrote:
> Note that it also didn't generate the tests for GL_RENDERBUFFER as an invalid
> arg.

That is also by design.  Since we don't validate in command buffer, there is no
way to test incorrect arguments.  No matter what, the underlying GL will be
called.

[Zhenyao Mo, 2015-01-16 22:19:58]

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_1_autogen.h:461: }
On 2015/01/16 22:17:48, Zhenyao Mo wrote:
> On 2015/01/16 22:06:23, sievers wrote:
> > Note that it also didn't generate the tests for GL_RENDERBUFFER as an
invalid
> > arg.
> 
> That is also by design.  Since we don't validate in command buffer, there is
no
> way to test incorrect arguments.  No matter what, the underlying GL will be
> called.

Actually in invalid GL_RENDERBUFFER is misleading.  I only put there so  it is
complete.  All the invalid values are not used to generate InvalidArg tests.

[no sievers, 2015-01-16 22:23:15]

got it, lgtm

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:1418: # count_func:   If True the
actual data count needs to be computed; the count
nit: maybe 'used_count_func' or something?

[no sievers, 2015-01-16 22:33:00]

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:601: 'Bufferiv': {
nit: btw if you are not planning on using the generated validation ever, you
should probably just remove this and make the argument a normal GLenum in
command_buffer_functions.txt, so it doesn't add all that stuff to the validator.

[Zhenyao Mo, 2015-01-16 22:40:20]

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:601: 'Bufferiv': {
On 2015/01/16 22:33:00, sievers wrote:
> nit: btw if you are not planning on using the generated validation ever, you
> should probably just remove this and make the argument a normal GLenum in
> command_buffer_functions.txt, so it doesn't add all that stuff to the
validator.

Unfortunately I still need to define these because I need th valid value to
generate ValidArg tests.  For example, the data size is depending on the buffer
(GL_COLOR maps to 4, GL_STENCIL maps to 1), so I can't just stick any number in
the place of buffer.

But I agree we could avoid generating the validators, but it's not trivia work,
because I need to check if Bufferiv is only used by unsafe functions, and we can
only get rid of validators if yes.

These are all planned work, that we 1) optionally turn off validators, and 2)
optionally not generate them 3) get rid of the generation code in this file.

https://codereview.chromium.org/851183002/diff/120001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:1418: # count_func:   If True the
actual data count needs to be computed; the count
On 2015/01/16 22:23:14, sievers wrote:
> nit: maybe 'used_count_func' or something?

Done.

[Zhenyao Mo, 2015-01-16 22:40:50]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-01-16 22:41:47]

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

[commit-bot: I haz the power, 2015-01-16 23:41:58]

Committed patchset #2 (id:140001)

[commit-bot: I haz the power, 2015-01-16 23:43:04]

Patchset 2 (id:??) landed as
https://crrev.com/e1d1c69631fdb7bf71f197f62c963882af31481a
Cr-Commit-Position: refs/heads/master@{#311979}