Switch to use ANGLE's new APIs to query shader variables.

Switch to use ANGLE's new APIs to query shader variables.

This is necessary for link time validation. The old APIs do not
provide struct name, field order, etc.

BUG=401311
TEST=gpu_unittests,webgl_conformance
R=bajones@chromium.org,kbr@chromium.org

Committed: https://crrev.com/88043c08c14810aa176e690b2e86f88c67728569
Cr-Commit-Position: refs/heads/master@{#298970}

[Zhenyao Mo, 2014-10-06 22:23:42]

zmo@chromium.org changed reviewers:
+ bajones@chromium.org, kbr@chromium.org

[Zhenyao Mo, 2014-10-06 22:23:55]

Patchset #1 (id:1) has been deleted

[Zhenyao Mo, 2014-10-06 22:24:03]

Patchset #1 (id:20001) has been deleted

[Zhenyao Mo, 2014-10-06 22:30:26]

Thanks to jmadill's work in ANGLE, we are able to fix the design fault in the
current ANGLE shader variable query APIs.

bajones,kbr: please review
jmadill: for your information (feel free to review if you like)

I tested with gpu_unittests and webgl conformance tests, no regression, and
conformance/glsl/misc/shaders-with-uniform-structs.html is passing with CL (was
failing without)

[Zhenyao Mo, 2014-10-06 22:31:08]

Can't try without landing ANGLE CL and roll ANGLE, but I think this CL is ready
for review.

ANGLE side CL: https://chromium-review.googlesource.com/#/c/221388/

[Zhenyao Mo, 2014-10-06 22:50:54]

Patchset #1 (id:40001) has been deleted

[Zhenyao Mo, 2014-10-06 22:51:03]

Patchset #1 (id:60001) has been deleted

[Ken Russell (switch to Gerrit), 2014-10-07 03:35:26]

This is excellent work overall. A few questions.

Meta-question: since there aren't any #ifdefs in this CL, does that mean the old
and new shader APIs live side-by-side for the moment? Is there a bug filed for
deleting the old APIs once this CL lands?

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/disk_cache_proto.proto (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/disk_cache_proto.proto:3: message ShaderVariableProto
{
What prevents us from attempting to parse an old version of the on-disk metadata
for a shader / program?

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.cc:765: *original_name = name;
Why is it OK to silently pass through the incoming name as corrected_name and
original_name in this case, not assign "type" or "size", and not return any
error code to the caller?

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.cc:774: const sh::Attribute* info =
shader->GetAttribInfo(name);
Somewhere it should be documented that vertex attributes cannot be arrays or
structures, and that's why further fixups to the sh::Attribute aren't necessary
like they are for uniforms, above.

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.cc:781: *original_name = name;
Why is it OK to silently pass through the incoming name as original_name in this
case, not assign "type", and not return any error code to the caller?

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.h:284: void GetCorrectedUniformData(
"Corrected" how? Could you please describe in more detail what these two methods
do?

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager_unittest.cc:900:
EXPECT_STREQ(original_name.c_str(), attrib_info->name.c_str());
Can you not use EXPECT_EQ(original_name, attrib_info->name) here and throughout?

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_manager.cc:112: AttributeMap::const_iterator
it = attrib_map_.find(GetTopVariableName(name));
Per comment above: vertex shader inputs can't be arrays or structures (GLSL ES
3.00.4, section 4.3.4, "Input Variables", and similar section in the GLSL ES
1.0.17 spec). Why is it necessary to call GetTopVariableName for vertex
attributes then?

[Zhenyao Mo, 2014-10-07 18:23:41]

I filed the ANGLE bug to track retiring of the old ANGLE APIs:
https://code.google.com/p/angleproject/issues/detail?id=775

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/disk_cache_proto.proto (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/disk_cache_proto.proto:3: message ShaderVariableProto
{
On 2014/10/07 03:35:26, Ken Russell wrote:
> What prevents us from attempting to parse an old version of the on-disk
metadata
> for a shader / program?

When we are ready to land this CL, we should land together with an ANGLE roll,
which includes a version update.  That will invalidate all the cached programs.

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.cc:765: *original_name = name;
On 2014/10/07 03:35:26, Ken Russell wrote:
> Why is it OK to silently pass through the incoming name as corrected_name and
> original_name in this case, not assign "type" or "size", and not return any
> error code to the caller?

Good question.  In theory this path should never be hit, because ANGLE
translator should always return the correct information.  I thought about this
while working on this CL, but decided not to add extra complication in it. Add a
TODO() for now.

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.cc:774: const sh::Attribute* info =
shader->GetAttribInfo(name);
On 2014/10/07 03:35:26, Ken Russell wrote:
> Somewhere it should be documented that vertex attributes cannot be arrays or
> structures, and that's why further fixups to the sh::Attribute aren't
necessary
> like they are for uniforms, above.

Done.

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.cc:781: *original_name = name;
On 2014/10/07 03:35:26, Ken Russell wrote:
> Why is it OK to silently pass through the incoming name as original_name in
this
> case, not assign "type", and not return any error code to the caller?

Same reply as above.

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager.h:284: void GetCorrectedUniformData(
On 2014/10/07 03:35:26, Ken Russell wrote:
> "Corrected" how? Could you please describe in more detail what these two
methods
> do?

Done.

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_manager_unittest.cc (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_manager_unittest.cc:900:
EXPECT_STREQ(original_name.c_str(), attrib_info->name.c_str());
On 2014/10/07 03:35:26, Ken Russell wrote:
> Can you not use EXPECT_EQ(original_name, attrib_info->name) here and
throughout?

I guess no good reason.  It's just my habit to use EXPECT_STREQ.  I'll switch
back to EXPECT_EQ.

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
File gpu/command_buffer/service/shader_manager.cc (right):

https://codereview.chromium.org/619723008/diff/80001/gpu/command_buffer/servi...
gpu/command_buffer/service/shader_manager.cc:112: AttributeMap::const_iterator
it = attrib_map_.find(GetTopVariableName(name));
On 2014/10/07 03:35:26, Ken Russell wrote:
> Per comment above: vertex shader inputs can't be arrays or structures (GLSL ES
> 3.00.4, section 4.3.4, "Input Variables", and similar section in the GLSL ES
> 1.0.17 spec). Why is it necessary to call GetTopVariableName for vertex
> attributes then?

Done.

[Zhenyao Mo, 2014-10-07 18:25:17]

Patchset #2 (id:100001) has been deleted

[Ken Russell (switch to Gerrit), 2014-10-08 04:20:12]

LGTM overall, and assuming this is landed along with an ANGLE roll. Excellent
work. Two more caveats / questions:

1. Could you please do an ASAN run on Linux of gpu_unittests or whatever target
will exercise much of this new code? Given that shader variable information
handling has been a problematic area in the past I think this will save a lot of
pain.

2. What will happen to shader disk cache entries for old ANGLE versions? Will
they be expired and flushed somehow? Or will they stick around forever chewing
up disk space until the disk cache is cleared?

[bajones, 2014-10-08 17:45:50]

LGTM too. Good work!

[Zhenyao Mo, 2014-10-08 17:55:11]

On 2014/10/08 04:20:12, Ken Russell wrote:
> LGTM overall, and assuming this is landed along with an ANGLE roll. Excellent
> work. Two more caveats / questions:
> 
> 1. Could you please do an ASAN run on Linux of gpu_unittests or whatever
target
> will exercise much of this new code? Given that shader variable information
> handling has been a problematic area in the past I think this will save a lot
of
> pain.

Yes I tested with ASAN gpu_unittests and angle_unittests

> 
> 2. What will happen to shader disk cache entries for old ANGLE versions? Will
> they be expired and flushed somehow? Or will they stick around forever chewing
> up disk space until the disk cache is cleared?

There is an upper limit of the cache we use, and we have an eviction strategy
implemented.

See MemoryProgramCache::SaveLinkedProgram for details.

[Ken Russell (switch to Gerrit), 2014-10-08 17:56:14]

On 2014/10/08 17:55:11, Zhenyao Mo wrote:
> On 2014/10/08 04:20:12, Ken Russell wrote:
> > LGTM overall, and assuming this is landed along with an ANGLE roll.
Excellent
> > work. Two more caveats / questions:
> > 
> > 1. Could you please do an ASAN run on Linux of gpu_unittests or whatever
> target
> > will exercise much of this new code? Given that shader variable information
> > handling has been a problematic area in the past I think this will save a
lot
> of
> > pain.
> 
> Yes I tested with ASAN gpu_unittests and angle_unittests
> 
> > 
> > 2. What will happen to shader disk cache entries for old ANGLE versions?
Will
> > they be expired and flushed somehow? Or will they stick around forever
chewing
> > up disk space until the disk cache is cleared?
> 
> There is an upper limit of the cache we use, and we have an eviction strategy
> implemented.
> 
> See MemoryProgramCache::SaveLinkedProgram for details.

Great. Thank you for being diligent with this change. LGTM again.

[Zhenyao Mo, 2014-10-09 13:53:05]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2014-10-09 13:54:33]

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

[Zhenyao Mo, 2014-10-09 18:58:48]

The CQ bit was checked by zmo@chromium.org

[commit-bot: I haz the power, 2014-10-09 18:59:01]

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

[commit-bot: I haz the power, 2014-10-09 20:39:02]

Committed patchset #4 (id:390001)

[commit-bot: I haz the power, 2014-10-09 20:39:46]

Patchset 4 (id:??) landed as
https://crrev.com/88043c08c14810aa176e690b2e86f88c67728569
Cr-Commit-Position: refs/heads/master@{#298970}