command_buffer: Make inactive bound uniforms reserve the location

command_buffer: Make inactive bound uniforms reserve the location

Specifying uniform values with locations bound with
CHROMIUM_bind_uniform_location should work, regardless whether
the underlying driver optimized the variable away or not.

Fixes the case where updating an inactive uniform would cause
INVALID_OPERATION, because the location was empty.

Fixes the case where updating an uniform would cause wrong
uniform to be updated, if the uniform was inactive and bound
to a location that would be assigned to an unbound uniform.

Makes the Program::uniform_infos_ array linear (non-sparse).  The
location bindings are now handled with Program::uniform_locations_
sparse array.

Simplifies the correction of driver-supplied uniform names and
determining whether an uniform is an array or not.

Fixes a cc bug exposed by the implementation (cc_unittests):
VideoGLRendererPixelTest.YUVAEdgeBleed
VideoGLRendererPixelTest.SimpleYUVARect

BUG=542355
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/8fbcba14c3ad7a6e27293dce819450e5aa44e4aa
Cr-Commit-Position: refs/heads/master@{#361076}

[Kimmo Kinnunen, 2015-11-06 11:22:51]

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

[Kimmo Kinnunen, 2015-11-06 11:37:29]

The CQ bit was checked by kkinnunen@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-06 11:38:15]

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

[commit-bot: I haz the power, 2015-11-06 11:38:16]

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

[commit-bot: I haz the power, 2015-11-06 11:38:17]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Zhenyao Mo, 2015-11-20 00:14:15]

Personally I am a little bit reserved about
BindUniformLocation("non-existing-name", location), and that location becomes
valid (the same as -1).

I talked with kbr, he thought that's acceptable, so I'll keep my reservation to
myself.

[Zhenyao Mo, 2015-11-20 00:15:00]

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

[Zhenyao Mo, 2015-11-20 00:15:01]

lgtm

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

https://codereview.chromium.org/1419393005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.h:121: uniform_ = uniform;
Maybe DCHECK(uniform)?

https://codereview.chromium.org/1419393005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.h:202: // -1 or bound, non-existing
uniform.
nit: or -> for

[zmo, 2015-11-20 00:23:42]

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

https://codereview.chromium.org/1419393005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1036: return false;
Question: why false here?

[Kimmo Kinnunen, 2015-11-20 07:56:54]

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

https://codereview.chromium.org/1419393005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.cc:1036: return false;
On 2015/11/20 00:23:42, zmo wrote:
> Question: why false here?

The function returns true only for locations of inactive uniforms, i.e. for
location that that behave like -1. When location_index >=
uniform_locations_.size(), it's guaranteed to be unused and non inactive.

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

https://codereview.chromium.org/1419393005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.h:121: uniform_ = uniform;
On 2015/11/20 00:15:01, Zhenyao Mo wrote:
> Maybe DCHECK(uniform)?

Done.

https://codereview.chromium.org/1419393005/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/program_manager.h:202: // -1 or bound, non-existing
uniform.
On 2015/11/20 00:15:01, Zhenyao Mo wrote:
> nit: or -> for

Done.

[Kimmo Kinnunen, 2015-11-20 08:13:05]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-11-20 08:13:06]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1419393005/#ps60001
(title: "address review comments")

[commit-bot: I haz the power, 2015-11-20 08:13:40]

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

[commit-bot: I haz the power, 2015-11-20 08:24:49]

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

[commit-bot: I haz the power, 2015-11-20 08:24:49]

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

[Kimmo Kinnunen, 2015-11-20 08:44:10]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-11-20 08:44:10]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1419393005/#ps80001
(title: "fix uninitialized var warning that should not happen")

[commit-bot: I haz the power, 2015-11-20 08:44:26]

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

[commit-bot: I haz the power, 2015-11-20 09:19:45]

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

[commit-bot: I haz the power, 2015-11-20 09:19:46]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Kimmo Kinnunen, 2015-11-20 09:24:49]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-11-20 09:24:50]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1419393005/#ps100001 (title: "fix msvc")

[commit-bot: I haz the power, 2015-11-20 09:24:59]

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

[commit-bot: I haz the power, 2015-11-20 10:10:33]

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

[commit-bot: I haz the power, 2015-11-20 10:10:33]

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

[Kimmo Kinnunen, 2015-11-20 12:25:39]

Description was changed from

==========
command_buffer: Make inactive bound uniforms reserve the location

Specifying uniform values with locations bound with
CHROMIUM_bind_uniform_location should work, regardless whether
the underlying driver optimized the variable away or not.

Fixes the case where updating an inactive uniform would cause
INVALID_OPERATION, because the location was empty.

Fixes the case where updating an uniform would cause wrong
uniform to be updated, if the uniform was inactive and bound
to a location that would be assigned to an unbound uniform.

Makes the Program::uniform_infos_ array linear (non-sparse).  The
location bindings are now handled with Program::uniform_locations_
sparse array.

Simplifies the correction of driver-supplied uniform names and
determining whether an uniform is an array or not.

BUG=542355
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
command_buffer: Make inactive bound uniforms reserve the location

Specifying uniform values with locations bound with
CHROMIUM_bind_uniform_location should work, regardless whether
the underlying driver optimized the variable away or not.

Fixes the case where updating an inactive uniform would cause
INVALID_OPERATION, because the location was empty.

Fixes the case where updating an uniform would cause wrong
uniform to be updated, if the uniform was inactive and bound
to a location that would be assigned to an unbound uniform.

Makes the Program::uniform_infos_ array linear (non-sparse).  The
location bindings are now handled with Program::uniform_locations_
sparse array.

Simplifies the correction of driver-supplied uniform names and
determining whether an uniform is an array or not.

Fixes a cc bug exposed by the implementation (cc_unittests):
VideoGLRendererPixelTest.YUVAEdgeBleed
VideoGLRendererPixelTest.SimpleYUVARect

BUG=542355
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Kimmo Kinnunen, 2015-11-20 12:25:39]

kkinnunen@nvidia.com changed reviewers:
+ enne@chromium.org

[Kimmo Kinnunen, 2015-11-20 12:26:09]

enne@: would you have time to look at the cc/ change?

[enne (OOO), 2015-11-20 18:26:15]

lgtm, thanks!

https://codereview.chromium.org/1419393005/diff/120001/cc/output/shader.cc
File cc/output/shader.cc (left):

https://codereview.chromium.org/1419393005/diff/120001/cc/output/shader.cc#ol...
cc/output/shader.cc:2113: "cc_matrix",
Oops!

[Kimmo Kinnunen, 2015-11-23 06:47:14]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-11-23 06:47:15]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/1419393005/#ps120001
(title: "fix a cc bug exposed by the implementation")

[commit-bot: I haz the power, 2015-11-23 06:47:22]

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

[commit-bot: I haz the power, 2015-11-23 08:25:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-23 08:26:21]

Patchset 7 (id:??) landed as
https://crrev.com/8fbcba14c3ad7a6e27293dce819450e5aa44e4aa
Cr-Commit-Position: refs/heads/master@{#361076}