Add framebuffer object related commands to command buffer.

Add framebuffer object related commands to command buffer.

glInvalidateFramebuffer, glInvalidateSubFramebuffer, glReadBuffer.

All other commands in this group have already been added.

Also, code generator has to be modified to remove the incorrect assumption
that the data pointer is the last arg of a function.  This assumption is not
true for glInvalidateSubFramebuffer.

Also, separate the bindings of glDiscardFramebufferEXT and glInvalidateFramebuffer so we have better control of them.

BUG=429053
TEST=gpu_unittests
R=kbr@chromium.org,bajones@chromium.org,sievers@chromium.org

Committed: https://crrev.com/68fcdc610840bc39b061e7842f78089171ed5e90
Cr-Commit-Position: refs/heads/master@{#307088}

[Zhenyao Mo, 2014-12-04 00:06:27]

Patchset #1 (id:1) has been deleted

[Zhenyao Mo, 2014-12-04 00:06:39]

Patchset #1 (id:20001) has been deleted

[Zhenyao Mo, 2014-12-04 00:07:01]

PTAL

[Ken Russell (switch to Gerrit), 2014-12-04 01:42:56]

kbr@chromium.org changed reviewers:
+ vmiura@chromium.org

[Ken Russell (switch to Gerrit), 2014-12-04 01:42:56]

I'm not an owner in this directory so can't provide a full review. One initial
comment.

https://codereview.chromium.org/780433006/diff/40001/gpu/command_buffer/build...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/780433006/diff/40001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:5612: valid_value = "&data[0][0]"
These changes seem really over-specialized, and scattered throughout this file.
Is there any way to encapsulate them better?

[Zhenyao Mo, 2014-12-04 02:29:39]

On 2014/12/04 01:42:56, Ken Russell wrote:
> I'm not an owner in this directory so can't provide a full review. One initial
> comment.
> 
>
https://codereview.chromium.org/780433006/diff/40001/gpu/command_buffer/build...
> File gpu/command_buffer/build_gles2_cmd_buffer.py (right):
> 
>
https://codereview.chromium.org/780433006/diff/40001/gpu/command_buffer/build...
> gpu/command_buffer/build_gles2_cmd_buffer.py:5612: valid_value = "&data[0][0]"
> These changes seem really over-specialized, and scattered throughout this
file.
> Is there any way to encapsulate them better?

I agree, but I feel it should be a separate CL.
Using of &data[0][0] is originally in the generator.  I simply refactor it to be
more flexible.
Removing this special case itself will be a big enough effort.

[Zhenyao Mo, 2014-12-04 17:36:20]

Patchset #2 (id:60001) has been deleted

[Zhenyao Mo, 2014-12-04 17:37:27]

Please review.

So for this CL, on es3, we also report ext_discard_framebuffer and delegate to
glInvalidateFramebuffer.

[Zhenyao Mo, 2014-12-04 17:40:38]

Patchset #2 (id:80001) has been deleted

[Zhenyao Mo, 2014-12-04 22:55:46]

Patchset #3 (id:120001) has been deleted

[Ken Russell (switch to Gerrit), 2014-12-05 03:17:37]

Nice work. LGTM overall. One question and one comment about the removal of one
of the tests.

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

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:5744: elif arg.type == "GLboolean":
Why does GLboolean need to be special-cased? Is this worth a comment?

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation_unittest_autogen.h (right):

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_unittest_autogen.h:847: }
The autogenerated unit tests look correct to me.

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info_unittest.cc (left):

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info_unittest.cc:130:
EXPECT_FALSE(info_->feature_flags().is_angle);
If this has been removed from the base expectations then there should be a
separate test expecting that is_angle = false in some configurations.

[Zhenyao Mo, 2014-12-05 17:59:01]

bajones: please take a look.

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

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:5744: elif arg.type == "GLboolean":
On 2014/12/05 03:17:37, Ken Russell wrote:
> Why does GLboolean need to be special-cased? Is this worth a comment?

Digging further, instead of checking the type is GLboolean, which is really
specific for PUTn and UniformMatrix's "transpose" arg, we should skip constant
arguments instead.

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info_unittest.cc (left):

https://codereview.chromium.org/780433006/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info_unittest.cc:130:
EXPECT_FALSE(info_->feature_flags().is_angle);
On 2014/12/05 03:17:37, Ken Russell wrote:
> If this has been removed from the base expectations then there should be a
> separate test expecting that is_angle = false in some configurations.

Per our offline discussion, I will add gl_version_info_unittests in a new test
target for ui/gl, but do it in a separate CL.

[Ken Russell (switch to Gerrit), 2014-12-05 18:43:07]

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

https://codereview.chromium.org/780433006/diff/160001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:5754: if arg.IsPointer() or
arg.type == "GLboolean":
Shouldn't this also be converted to "arg.IsContstant()"?

[Zhenyao Mo, 2014-12-05 20:02:35]

Please take another look.

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

https://codereview.chromium.org/780433006/diff/160001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:5754: if arg.IsPointer() or
arg.type == "GLboolean":
On 2014/12/05 18:43:07, Ken Russell wrote:
> Shouldn't this also be converted to "arg.IsContstant()"?

Done.

[Ken Russell (switch to Gerrit), 2014-12-05 20:42:09]

Thanks, looks better. LGTM again.

[bajones, 2014-12-05 20:51:55]

lgtm

[Zhenyao Mo, 2014-12-05 21:03:43]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2014-12-05 21:04:34]

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

[commit-bot: I haz the power, 2014-12-05 21:52:00]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2014-12-05 21:52:51]

Patchset 6 (id:??) landed as
https://crrev.com/68fcdc610840bc39b061e7842f78089171ed5e90
Cr-Commit-Position: refs/heads/master@{#307088}

[piman, 2014-12-05 22:44:08]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2014-12-05 22:44:09]

https://codereview.chromium.org/780433006/diff/200001/gpu/command_buffer/serv...
File gpu/command_buffer/service/feature_info.h (right):

https://codereview.chromium.org/780433006/diff/200001/gpu/command_buffer/serv...
gpu/command_buffer/service/feature_info.h:126: CHECK(gl_version_info_.get());
drive-by: no CHECK please. use DCHECK.

[Zhenyao Mo, 2014-12-05 23:50:13]

On 2014/12/05 22:44:09, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/780433006/diff/200001/gpu/command_buffer/serv...
> File gpu/command_buffer/service/feature_info.h (right):
> 
>
https://codereview.chromium.org/780433006/diff/200001/gpu/command_buffer/serv...
> gpu/command_buffer/service/feature_info.h:126: CHECK(gl_version_info_.get());
> drive-by: no CHECK please. use DCHECK.

Will do in a followup CL.