Implement SamplerManager in the command buffer

Implement SamplerManager in the command buffer

BUG=563613
Committed: https://crrev.com/5141d0359ff9ac5a624ca298f353fdd99a77861d
Cr-Commit-Position: refs/heads/master@{#363584}

[bajones, 2015-12-04 23:20:00]

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

[bajones, 2015-12-04 23:20:00]

This code only adds the manager and state tracking, it does not do any
validation with it yet. That will come in a follow-up CL. I figured that this
was enough code for one review and didn't want any tricky validation logic to
get skimmed over.

[Zhenyao Mo, 2015-12-04 23:49:03]

Mostly look good with one bug fixed.

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

https://codereview.chromium.org/1498033003/diff/20001/gpu/command_buffer/serv...
gpu/command_buffer/service/sampler_manager.cc:105: case GL_TEXTURE_COMPARE_MODE:
You meant to delegate all the above to SetPartameteri()?

[bajones, 2015-12-05 00:01:27]

On 2015/12/04 23:49:03, Zhenyao Mo wrote:
>
https://codereview.chromium.org/1498033003/diff/20001/gpu/command_buffer/serv...
> gpu/command_buffer/service/sampler_manager.cc:105: case
GL_TEXTURE_COMPARE_MODE:
> You meant to delegate all the above to SetPartameteri()?

Oof. Good catch, thanks! Fixed.

[Zhenyao Mo, 2015-12-05 00:03:47]

LGTM with a minor assertion fixed.

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

https://codereview.chromium.org/1498033003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/sampler_manager.cc:117: NOTREACHED();
Sorry I didn't catch this earlier, but you should not check here.  There isn't
any validation of pname, so this is the place for that validation.

Here and SetParameteri()

[bajones, 2015-12-05 00:20:47]

On 2015/12/05 00:03:47, Zhenyao Mo wrote:
> Sorry I didn't catch this earlier, but you should not check here.  There isn't
> any validation of pname, so this is the place for that validation.
> 
> Here and SetParameteri()

Thanks again, fixed.

piman@: Need an owners review for the .gypi, but I'd appreciate any thoughts you
have on the patch as a whole.

[piman, 2015-12-05 02:45:19]

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/buil...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2262: 'gen_func': 'GenSamplers',
I don't think you need or want gen_func. glBindSampler doesn't implicitly
generate one if it's a new id.

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:2647: 'useHelper': True
nit: can you use a consistent style, 'use_helper'
Can you document what this means at the top of _FUNCTION_INFO ?

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:8225: if func.IsUnsafe() and not
func.HasDecoderFunc():
Why does it matter whether here whether we renamed the decoder func to go
through the decoder or directly to GL?
Can we keep things orthogonal, and base it on UseHelper? What breaks?

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/buil...
gpu/command_buffer/build_gles2_cmd_buffer.py:9290: """Returns whether the
function has service side validation or not."""
Can you fix the doc here? It's currently identical to IsUnsafe()

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

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/context_state.h:247:
std::vector<scoped_refptr<Sampler> > sampler_units;
nit: no need to add a space between the > (we're using c++11 everywhere)

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

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2754: glBindSampler(tt, 0);
nit: is it needed? it's the default state.

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:4946: if
(!group_->bind_generates_resource()) {
Unlike textures or buffers, samplers are not implicitly generated if binding a
new id.

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
File gpu/command_buffer/service/sampler_manager.cc (right):

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sampler_manager.cc:24: deleted_(false) {
Every field needs to be initialized.

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sampler_manager.cc:29: if (manager_) {
manager_ is set in the constructor, DCHECK'ed there and never reset, so is
should always be !NULL here.

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sampler_manager.cc:43: {
nit: style. { goes on previous line.

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sampler_manager.cc:105: {
nit: here too.

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/sampler_manager.cc:141:
std::pair<SamplerMap::iterator, bool> result =
nit: use auto

[bajones, 2015-12-07 19:25:41]

Thank's for the review piman@! Addressed your comments in the latest patch.

[piman, 2015-12-07 19:35:59]

LGTM, but one question. Please consider removing redundant state setting.

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

https://codereview.chromium.org/1498033003/diff/60001/gpu/command_buffer/serv...
gpu/command_buffer/service/gles2_cmd_decoder.cc:2754: glBindSampler(tt, 0);
On 2015/12/05 02:45:19, piman (slow to review) wrote:
> nit: is it needed? it's the default state.

ping

[bajones, 2015-12-07 19:42:14]

On 2015/12/07 19:35:59, piman (slow to review) wrote:
> LGTM, but one question. Please consider removing redundant state setting.

Ah, thanks for the reminder. Just an oversight on my part.

[bajones, 2015-12-07 19:42:18]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-12-07 19:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1498033003/#ps100001
(title: "Fixed unnessecary state initialization")

[commit-bot: I haz the power, 2015-12-07 19:47:31]

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

[commit-bot: I haz the power, 2015-12-07 21:13:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-07 21:15:01]

Description was changed from

==========
Implement SamplerManager in the command buffer

BUG=563613
==========

to

==========
Implement SamplerManager in the command buffer

BUG=563613

Committed: https://crrev.com/5141d0359ff9ac5a624ca298f353fdd99a77861d
Cr-Commit-Position: refs/heads/master@{#363584}
==========

[commit-bot: I haz the power, 2015-12-07 21:15:02]

Patchset 6 (id:??) landed as
https://crrev.com/5141d0359ff9ac5a624ca298f353fdd99a77861d
Cr-Commit-Position: refs/heads/master@{#363584}