gpu: Autogenerate glHint state

gpu: Autogenerate glHint state

This patch implements automated code generation for state that is
set with the glHint function. Currently this state includes
GL_GENERATE_MIPMAP_HINT and GL_FRAGMENT_SHADER_DERIVATIVE_HINT_OES.

In order to support functions like glHint that take an enum identifying
the state variable whose value is being set, we add a new
'NamedParameter' state type to the command buffer code generator.
Functions of this type are expected to take the parameter enum as the
first argument and the value as the second one. This same mechanism can
also be used with glPixelStorei and other similar functions.

BUG=245228
TEST=glHint test in [1] passes repeatedly with --enable-virtual-gl-contexts.

[1] https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/conformance/extensions/oes-standard-derivatives.html

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203463

[greggman, 2013-05-30 17:15:23]

A couple of issues. Sorry I don't have any easy suggestions to deal with it.

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/co...
File gpu/command_buffer/service/context_state_impl_autogen.h (right):

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/co...
gpu/command_buffer/service/context_state_impl_autogen.h:114: glHint(
You can't init state that is dependent on extensions being present or not. It
will generate a GL error.

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_validation_implementation_autogen.h
(right):

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_validation_implementation_autogen.h:194:
GL_FRAGMENT_SHADER_DERIVATIVE_HINT_OES,
Adding extension based values to this list means they will pass validation when
they should not. Extension gated values should be added in feature_info.cc when
the extension exists.

[greggman, 2013-05-30 17:25:09]

Random ideas

1) Maybe add some flags to NamedParameter that say if the state is an extension

    'extension': true

To help the generator add or skip stuff

2) Maybe add a flag 'extension_flag' which says which flag in
FeatureInfo::FeatureFlag the state is gated on?

To help the generator add or skip stuff

I'm not sure it's worth it to auto-generate until there's 3 or 4 things that
need to be generated but just throwing out ideas.

[Sami, 2013-05-30 17:51:10]

On 2013/05/30 17:25:09, greggman wrote:
> Random ideas
> 
> 1) Maybe add some flags to NamedParameter that say if the state is an
extension
> 
>     'extension': true
> 
> To help the generator add or skip stuff
> 
> 2) Maybe add a flag 'extension_flag' which says which flag in
> FeatureInfo::FeatureFlag the state is gated on?
> 
> To help the generator add or skip stuff

> I'm not sure it's worth it to auto-generate until there's 3 or 4 things that
> need to be generated but just throwing out ideas.

Thanks, I'll give the second idea a try first. I agree that auto-generation may
be overkill, but I'd still like to try it out if it can be done without too much
complexity to avoid the kind of bugs in the hand-written code that prompted this
in the first place :)

[Sami, 2013-05-30 18:59:45]

PTAL

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/co...
File gpu/command_buffer/service/context_state_impl_autogen.h (right):

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/co...
gpu/command_buffer/service/context_state_impl_autogen.h:114: glHint(
On 2013/05/30 17:15:24, greggman wrote:
> You can't init state that is dependent on extensions being present or not. It
> will generate a GL error.

Done.

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/gl...
File gpu/command_buffer/service/gles2_cmd_validation_implementation_autogen.h
(right):

https://codereview.chromium.org/15792007/diff/1/gpu/command_buffer/service/gl...
gpu/command_buffer/service/gles2_cmd_validation_implementation_autogen.h:194:
GL_FRAGMENT_SHADER_DERIVATIVE_HINT_OES,
On 2013/05/30 17:15:24, greggman wrote:
> Adding extension based values to this list means they will pass validation
when
> they should not. Extension gated values should be added in feature_info.cc
when
> the extension exists.

Done.

[greggman, 2013-05-30 19:57:16]

looks good. just one question

https://codereview.chromium.org/15792007/diff/7001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder_autogen.h (right):

https://codereview.chromium.org/15792007/diff/7001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:1410: if (target ==
GL_GENERATE_MIPMAP_HINT &&
Does it make sense to change this to a switch/case or at least make the ifs
cascade? As it is, if the mode is the same it will end up checking the other
targets

In other words

    if (target == GL_GENERATE_MIPMAP_HINT) {
      if (state_.hint_generate_mipmap != mode) {
        state_.hint_generate_mapmap = mode
      }
    } else if (target == GL_FRAGMENT_SHADER_DERIVATIVE_HINT) {
      ...

or

    switch (target) {
      case GL_GENERATE_MIPMAP_HINT:
        if (state_.hint_generate_mipmap != mode) {
          state_.hint_generate_mapmap = mode
        }
        break;

etc..

I know this code is not perf sensitive but someone will copy it later to a place
that is.

[Sami, 2013-05-31 09:40:20]

https://codereview.chromium.org/15792007/diff/7001/gpu/command_buffer/service...
File gpu/command_buffer/service/gles2_cmd_decoder_autogen.h (right):

https://codereview.chromium.org/15792007/diff/7001/gpu/command_buffer/service...
gpu/command_buffer/service/gles2_cmd_decoder_autogen.h:1410: if (target ==
GL_GENERATE_MIPMAP_HINT &&
On 2013/05/30 19:57:16, greggman wrote:
> Does it make sense to change this to a switch/case or at least make the ifs
> cascade? As it is, if the mode is the same it will end up checking the other
> targets
> 
> In other words
> 
>     if (target == GL_GENERATE_MIPMAP_HINT) {
>       if (state_.hint_generate_mipmap != mode) {
>         state_.hint_generate_mapmap = mode
>       }
>     } else if (target == GL_FRAGMENT_SHADER_DERIVATIVE_HINT) {
>       ...
> 
> or
> 
>     switch (target) {
>       case GL_GENERATE_MIPMAP_HINT:
>         if (state_.hint_generate_mipmap != mode) {
>           state_.hint_generate_mapmap = mode
>         }
>         break;
> 
> etc..
> 
> I know this code is not perf sensitive but someone will copy it later to a
place
> that is.

Great point, let's try to maintain a high standard for generated code. I've made
this into a switch-case.

[greggman, 2013-05-31 17:43:48]

lgtm

[commit-bot: I haz the power, 2013-05-31 17:51:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/15792007/11001

[commit-bot: I haz the power, 2013-05-31 20:10:20]

Change committed as 203463