command_buffer: Fix setting samplers with bound uniforms

command_buffer: Fix setting samplers with bound uniforms

Fix a bug with setting samplers using glUniform1i when the sampler
location is bound with glBindUniformLocationCHROMIUM.

The bug appears if the driver optimizes unused unforms away or
if the bound location is higher than the number of active uniforms.

BUG=skia:4454
Committed: https://crrev.com/2e91ed9a9b8b925987a98b6b6c18e67fb6d4edf8
Cr-Commit-Position: refs/heads/master@{#371213}

[Kimmo Kinnunen, 2016-01-20 13:04:23]

kkinnunen@nvidia.com changed reviewers:
+ zmo@google.com

[Zhenyao Mo, 2016-01-20 18:41:12]

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

[Zhenyao Mo, 2016-01-20 18:41:13]

https://codereview.chromium.org/1610613002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/1610613002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:1413: if (location_index >=
uniform_locations_.size())
Correct me if I am wrong.

uniform_locations_ size is defined by the largest client bound location.  If the
client does not bind a uniform location, that location could still be a valid
location - it's just automatically bound to a uniform by the driver.

Also, even if the client binds to a specific location, the underlying uniform
could still be optimized away, or the name does not corresponding to a uniform
at all.

Is that the case?  If yes, then I don't think this change works in general.

[Kimmo Kinnunen, 2016-01-21 12:36:32]

https://codereview.chromium.org/1610613002/diff/1/gpu/command_buffer/service/...
File gpu/command_buffer/service/program_manager.cc (right):

https://codereview.chromium.org/1610613002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:616: 
(1): This binds the automatically bound uniforms to unused client locations.

https://codereview.chromium.org/1610613002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:1262: bool
Program::IsInactiveFragmentInputLocationByFakeLocation(
(2): This is the place that is checking the possibility that the driver
optimized the uniform away.

This will be executed by:
GLES2DecoderImpl::DoUniform1iv
 GLES2DecoderImpl::PrepForSetUniformByLocation
  GLES2DecoderImpl::CheckCurrentProgramForUniform
   Program::GetFragmentInputInfoByFakeLocation

https://codereview.chromium.org/1610613002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:1413: if (location_index >=
uniform_locations_.size())
On 2016/01/20 18:41:13, Zhenyao Mo wrote:
> Correct me if I am wrong.
> 
> uniform_locations_ size is defined by the largest client bound location.  If
the
> client does not bind a uniform location, that location could still be a valid
> location - it's just automatically bound to a uniform by the driver.
> 

No, automatically bound uniforms get their client-side uniform locations
assigned by the command buffer.
See above (1).

> Also, even if the client binds to a specific location, the underlying uniform
> could still be optimized away, or the name does not corresponding to a uniform
> at all.

If the client binds an uniform and driver optimizes the uniform away, the
uniform location still exist -- this was the fix while back. The check is below
and above, (2).

https://codereview.chromium.org/1610613002/diff/1/gpu/command_buffer/service/...
gpu/command_buffer/service/program_manager.cc:1416: if
(!uniform_locations_[location_index].IsActive())
(2): This checks if the uniform has been optimized away by the driver.
This should not be executed by glUniform1i, though.
See the other (2).

I'll add an clarification, see the new patch.

[Zhenyao Mo, 2016-01-22 20:50:29]

I think I understand now.

lgtm

[Kimmo Kinnunen, 2016-01-25 08:18:55]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2016-01-25 08:19:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1610613002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1610613002/20001

[commit-bot: I haz the power, 2016-01-25 09:26:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-25 09:26:54]

Description was changed from

==========
command_buffer: Fix setting samplers with bound uniforms

Fix a bug with setting samplers using glUniform1i when the sampler
location is bound with glBindUniformLocationCHROMIUM.

The bug appears if the driver optimizes unused unforms away or
if the bound location is higher than the number of active uniforms.

BUG=skia:4454
==========

to

==========
command_buffer: Fix setting samplers with bound uniforms

Fix a bug with setting samplers using glUniform1i when the sampler
location is bound with glBindUniformLocationCHROMIUM.

The bug appears if the driver optimizes unused unforms away or
if the bound location is higher than the number of active uniforms.

BUG=skia:4454

Committed: https://crrev.com/2e91ed9a9b8b925987a98b6b6c18e67fb6d4edf8
Cr-Commit-Position: refs/heads/master@{#371213}
==========

[commit-bot: I haz the power, 2016-01-25 09:26:54]

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