Add ES3 commands GetVertexAttribI{u}iv to GPU command buffer.

Add ES3 commands GetVertexAttribI{u}iv to GPU command buffer.

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

Committed: https://crrev.com/5ee097e78c8c254607c02ee7614bba0abe41d956
Cr-Commit-Position: refs/heads/master@{#329892}

[Zhenyao Mo, 2015-05-08 22:06:47]

piman: please review.
Others: FYI (feel free to review).

[Zhenyao Mo, 2015-05-08 22:20:56]

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/clie...
File gpu/command_buffer/client/vertex_array_object_manager.cc (right):

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/clie...
gpu/command_buffer/client/vertex_array_object_manager.cc:286: // TODO(zmo):
cache this on the client side.
Same here, to implement this, we need to modify a few other commands' handling,
so I feel it's better to do it in a separate CL.

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/vertex_attrib_manager.cc (right):

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/vertex_attrib_manager.cc:38: integer_(GL_FALSE),
We don't have a path to set this to true in this CL.  I decided to do it in a
followup CL because that requires interaction with a bunch of other commands'
handling.

[piman, 2015-05-11 21:12:50]

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7708:
DoGetVertexAttribImpl<GLint>(index, pname, params);
So, this is not quite right. DoVertexAttribI4i and friends need to store the
value as an int (not converting to float - i.e. state_.attrib_values[i] should
probably be a union), and retrieve that value. Otherwise data is lost (e.g. an
int value that is not representable as a float) - that's the whole point of
glGetVertexAttribIiv vs  glGetVertexAttribiv (which takes the float and converts
to int).

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/vertex_attrib_manager.h (right):

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/vertex_attrib_manager.h:231: void SetInteger(GLuint
index, GLboolean integer) {
Does anything call this? The DoVertexAttribI* functions probably should.

[Zhenyao Mo, 2015-05-12 23:57:45]

piman: please take another look at patch set 3 (diff with patch set 2 is the
update). patch set 4 is pure rebasing.

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:7708:
DoGetVertexAttribImpl<GLint>(index, pname, params);
On 2015/05/11 21:12:50, piman (Very slow to review) wrote:
> So, this is not quite right. DoVertexAttribI4i and friends need to store the
> value as an int (not converting to float - i.e. state_.attrib_values[i] should
> probably be a union), and retrieve that value. Otherwise data is lost (e.g. an
> int value that is not representable as a float) - that's the whole point of
> glGetVertexAttribIiv vs  glGetVertexAttribiv (which takes the float and
converts
> to int).

per our discussion, use double as internal representation, so no data loss.

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
File gpu/command_buffer/service/vertex_attrib_manager.h (right):

https://codereview.chromium.org/1136713003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/vertex_attrib_manager.h:231: void SetInteger(GLuint
index, GLboolean integer) {
On 2015/05/11 21:12:50, piman (Very slow to review) wrote:
> Does anything call this? The DoVertexAttribI* functions probably should.

Yes, they would.  As I mentioned in a previous comment, I will hook up the
setters with the related functions in the next CL.  This CL focuses on adding
the Get* commands.

https://codereview.chromium.org/1136713003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/1136713003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:121: DataType type;
Note that in this CL, we don't hook up the setting of this |type| field with any
of the glVertexAttrib* calls.  This CL limits to the glGetVertexAttrib handling.
 An followup CL will fix such issues.

[piman, 2015-05-13 01:47:45]

lgtm

https://codereview.chromium.org/1136713003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1136713003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:177: Vec4f(Vec4 data) {
nit: explicit

https://codereview.chromium.org/1136713003/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/1136713003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:119: // or GLuint, there will be no
data loss.
Or you could use a union.

[Zhenyao Mo, 2015-05-13 16:52:25]

piman: I switched to use union as you suggested, so there are some revisions
(plus unit tests) that I think you should take another look.

https://codereview.chromium.org/1136713003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/1136713003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:177: Vec4f(Vec4 data) {
On 2015/05/13 01:47:44, piman (Very slow to review) wrote:
> nit: explicit

Done.

https://codereview.chromium.org/1136713003/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/1136713003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:119: // or GLuint, there will be no
data loss.
On 2015/05/13 01:47:45, piman (Very slow to review) wrote:
> Or you could use a union.

Done.

https://codereview.chromium.org/1136713003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/context_state.cc (right):

https://codereview.chromium.org/1136713003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.cc:120: values[ii] =
static_cast<GLfloat>(v_[ii].int_value);
Note that the casting behavior might change depending on how WebGL 2 decides to
get rid of the undefined behavior.

[piman, 2015-05-13 19:33:27]

https://codereview.chromium.org/1136713003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/context_state.h (right):

https://codereview.chromium.org/1136713003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/context_state.h:120: void GetValues(GLfloat* values)
const;
nit: we typically disallow overloading per style guide.

How about GetFloats/GetInts/GetUInts and SetFloats/SetInts/SetUInts?

Or templates are fine too, you just need to make sure the specializations are
exported.

[Zhenyao Mo, 2015-05-14 00:22:53]

piman: revised using template.  I think all bots compiled OK. please take
another look.

[piman, 2015-05-14 00:52:35]

lgtm

[Zhenyao Mo, 2015-05-14 03:46:08]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-05-14 03:47:04]

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

[commit-bot: I haz the power, 2015-05-14 03:56:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-14 03:56:05]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Zhenyao Mo, 2015-05-14 07:21:36]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-05-14 07:21:52]

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

[commit-bot: I haz the power, 2015-05-14 07:28:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-14 07:28:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Zhenyao Mo, 2015-05-14 18:06:11]

The CQ bit was checked by zmo@chromium.org

[Zhenyao Mo, 2015-05-14 18:06:12]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1136713003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-14 18:06:43]

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

[commit-bot: I haz the power, 2015-05-14 19:14:18]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-05-14 19:14:56]

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