Add glGetFragDataLocation to gpu command buffer.

Add glGetFragDataLocation to gpu command buffer.

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

Committed: https://crrev.com/5393fb5df017dcf7ef8c8e2129b62c842556f525
Cr-Commit-Position: refs/heads/master@{#313201}

[Zhenyao Mo, 2015-01-26 23:07:21]

Patchset #4 (id:60001) has been deleted

[Zhenyao Mo, 2015-01-26 23:07:28]

Patchset #3 (id:40001) has been deleted

[Zhenyao Mo, 2015-01-26 23:07:34]

Patchset #2 (id:20001) has been deleted

[Zhenyao Mo, 2015-01-26 23:07:40]

Patchset #1 (id:1) has been deleted

[Zhenyao Mo, 2015-01-26 23:12:53]

sievers: please review.
Otherse: FYI (feel free to review)

[Zhenyao Mo, 2015-01-26 23:17:09]

Most of program info (like active attributes, active uniforms) are fetched from
service side altogether.

However, this implementation doesn't include FragData locations as part of that.
 Each location is cached independently on the client side.

This is because GL doesn't expose an API to query all active FragData variables,
so we have to depend on ANGLE to expose such info if we want to cache them all
at once after program link on the service side.  ANGLE implementation isn't
ready yet.

I created crbug.com/452104 to track this.

[bajones, 2015-01-26 23:38:56]

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

[bajones, 2015-01-26 23:38:57]

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/program_info_manager.cc (right):

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/program_info_manager.cc:241: // TODO(gman): Switch to
a faster container.
Does this change satisfy gman's TODO here? If so this comment should be removed.
Hash maps aren't always faster than a traditional map, but it seems to me that
in this use case it should be an improvement.

[Ken Russell (switch to Gerrit), 2015-01-26 23:39:11]

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

[Ken Russell (switch to Gerrit), 2015-01-26 23:39:12]

I'm not an OWNER, but LGTM overall. Couple of minor comments.

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

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_unittest_autogen.h:796: // TODO:
Implement unit test for GetFragDataLocation
Add OWNER for TODO -- and ideally file a bug.

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/program_info_manager.cc (right):

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/program_info_manager.cc:242: typedef
base::hash_map<GLuint, Program> ProgramInfoMap;
Does this new container type have all the same guarantees that std::map does?

Is the TODO still necessary after this switch?

[Zhenyao Mo, 2015-01-26 23:47:30]

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

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_unittest_autogen.h:796: // TODO:
Implement unit test for GetFragDataLocation
On 2015/01/26 23:39:12, Ken Russell wrote:
> Add OWNER for TODO -- and ideally file a bug.

OK, will add TODO(zmo).

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/program_info_manager.cc (right):

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/program_info_manager.cc:241: // TODO(gman): Switch to
a faster container.
On 2015/01/26 23:38:57, bajones wrote:
> Does this change satisfy gman's TODO here? If so this comment should be
removed.
> Hash maps aren't always faster than a traditional map, but it seems to me that
> in this use case it should be an improvement.

I am not what gman has in mind originally, but I switched to hash_map because
for frag_data_locations_ with std::string as keys, hash_map seems a better
choice.  For this one I don't think it's a big deal for the key to be sorted or
not, so I switch to hash_table to be consistent.

[no sievers, 2015-01-27 00:17:20]

lgtm

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

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8146: if (!program) {
Should this do LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glFragDataLocation",
"Invalid program")?

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_programs.cc (right):

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_programs.cc:916:
EXPECT_EQ(GL_INVALID_VALUE, GetGLError());
Is this error set from the client?

[Zhenyao Mo, 2015-01-27 00:36:46]

Thanks for the quick review turnaround.

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

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:8146: if (!program) {
On 2015/01/27 00:17:20, sievers wrote:
> Should this do LOCAL_SET_GL_ERROR(GL_INVALID_OPERATION, "glFragDataLocation",
> "Invalid program")?

It's done inside GetProgramInfoNotShader.  This is because it can be
INVALID_OPERATION (if id is a valid shader id) or INVALID_VALUE (else).

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest_programs.cc (right):

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder_unittest_programs.cc:916:
EXPECT_EQ(GL_INVALID_VALUE, GetGLError());
On 2015/01/27 00:17:20, sievers wrote:
> Is this error set from the client?

It's set by decoder in GetProgramInfoNotShader().  See my explanation in the
other reply.

[Zhenyao Mo, 2015-01-27 00:48:38]

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
File gpu/command_buffer/client/program_info_manager.cc (right):

https://codereview.chromium.org/867123008/diff/100001/gpu/command_buffer/clie...
gpu/command_buffer/client/program_info_manager.cc:242: typedef
base::hash_map<GLuint, Program> ProgramInfoMap;
On 2015/01/26 23:39:12, Ken Russell wrote:
> Does this new container type have all the same guarantees that std::map does?
> 
> Is the TODO still necessary after this switch?

I think hash_map and map both work in this situation.  I'll delete the TODO here
because I can't think of a better container unless we write a customized one.

[Zhenyao Mo, 2015-01-27 00:50:12]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2015-01-27 00:51:11]

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

[commit-bot: I haz the power, 2015-01-27 01:51:19]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2015-01-27 01:53:07]

Patchset 3 (id:??) landed as
https://crrev.com/5393fb5df017dcf7ef8c8e2129b62c842556f525
Cr-Commit-Position: refs/heads/master@{#313201}