[Cogl] [PATCH 2/2] progend-glsl: Fix handling of the builtin uniforms on non-GLES2

[Robert Bragg]

this looks good to land to me:

Reviewed-by: Robert Bragg <robert at linux.intel.com>

thanks,
- Robert

On Mon, Oct 1, 2012 at 3:03 PM, Neil Roberts <neil at linux.intel.com> wrote:
> The ‘builtin uniforms’ are added to the GLSL code generated for the
> GLES2 driver to implement missing fixed functionality such as the
> builtin point sprite size and the alpha test reference. Previously the
> code that accessed these was #ifdef'd to be compiled only when GLES2
> was enabled. However since 2701b93f part of this code is now always
> used even for non-GLES2 drivers. The code that accessed the builtin
> uniforms array was however no longer #ifdef'd which meant that it
> wouldn't compile any more if GLES2 was not enabled. This was further
> broken becase the GL3 driver actually should be using the alpha test
> uniform because that also does not provide any fixed functionality for
> alpha testing.
>
> To fix this the builtin uniform array is now always compiled in and
> the code to access it is always used. A new member has been added to
> the array to mark which private feature the uniform is used to
> replace. That is checked before updating the uniform so that under
> GLES2 it will update both uniforms but under GL3 it will only update
> the alpha test reference.
> ---
>  cogl/driver/gl/cogl-pipeline-progend-glsl.c | 51 ++++++++++++++---------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/cogl/driver/gl/cogl-pipeline-progend-glsl.c b/cogl/driver/gl/cogl-pipeline-progend-glsl.c
> index 3300a2e..26504eb 100644
> --- a/cogl/driver/gl/cogl-pipeline-progend-glsl.c
> +++ b/cogl/driver/gl/cogl-pipeline-progend-glsl.c
> @@ -50,10 +50,9 @@
>  #include "cogl-framebuffer-private.h"
>  #include "cogl-pipeline-progend-glsl-private.h"
>
> -#ifdef HAVE_COGL_GLES2
> -
>  /* These are used to generalise updating some uniforms that are
> -   required when building for GLES2 */
> +   required when building for drivers missing some fixed function
> +   state that we use */
>
>  typedef void (* UpdateUniformFunc) (CoglPipeline *pipeline,
>                                      int uniform_location,
> @@ -69,20 +68,24 @@ typedef struct
>    void *getter_func;
>    UpdateUniformFunc update_func;
>    CoglPipelineState change;
> +
> +  /* This builtin is only necessary if the following private feature
> +   * is not implemented in the driver */
> +  CoglPrivateFeatureFlags feature_replacement;
>  } BuiltinUniformData;
>
>  static BuiltinUniformData builtin_uniforms[] =
>    {
>      { "cogl_point_size_in",
>        cogl_pipeline_get_point_size, update_float_uniform,
> -      COGL_PIPELINE_STATE_POINT_SIZE },
> +      COGL_PIPELINE_STATE_POINT_SIZE,
> +      COGL_PRIVATE_FEATURE_BUILTIN_POINT_SIZE_UNIFORM },
>      { "_cogl_alpha_test_ref",
>        cogl_pipeline_get_alpha_test_reference, update_float_uniform,
> -      COGL_PIPELINE_STATE_ALPHA_FUNC_REFERENCE }
> +      COGL_PIPELINE_STATE_ALPHA_FUNC_REFERENCE,
> +      COGL_PRIVATE_FEATURE_ALPHA_TEST }
>    };
>
> -#endif /* HAVE_COGL_GLES2 */
> -
>  const CoglPipelineProgend _cogl_pipeline_glsl_progend;
>
>  typedef struct _UnitState
> @@ -101,10 +104,8 @@ typedef struct
>
>    GLuint program;
>
> -#ifdef HAVE_COGL_GLES2
>    unsigned long dirty_builtin_uniforms;
>    GLint builtin_uniform_locations[G_N_ELEMENTS (builtin_uniforms)];
> -#endif
>
>    GLint modelview_uniform;
>    GLint projection_uniform;
> @@ -423,10 +424,9 @@ update_constants_cb (CoglPipeline *pipeline,
>    return TRUE;
>  }
>
> -#ifdef HAVE_COGL_GLES2
> -
>  static void
> -update_builtin_uniforms (CoglPipeline *pipeline,
> +update_builtin_uniforms (CoglContext *context,
> +                         CoglPipeline *pipeline,
>                           GLuint gl_program,
>                           CoglPipelineProgramState *program_state)
>  {
> @@ -436,7 +436,9 @@ update_builtin_uniforms (CoglPipeline *pipeline,
>      return;
>
>    for (i = 0; i < G_N_ELEMENTS (builtin_uniforms); i++)
> -    if ((program_state->dirty_builtin_uniforms & (1 << i)) &&
> +    if ((context->private_feature_flags &
> +         builtin_uniforms[i].feature_replacement) == 0 &&
> +        (program_state->dirty_builtin_uniforms & (1 << i)) &&
>          program_state->builtin_uniform_locations[i] != -1)
>        builtin_uniforms[i].update_func (pipeline,
>                                         program_state
> @@ -446,8 +448,6 @@ update_builtin_uniforms (CoglPipeline *pipeline,
>    program_state->dirty_builtin_uniforms = 0;
>  }
>
> -#endif /* HAVE_COGL_GLES2 */
> -
>  typedef struct
>  {
>    CoglPipelineProgramState *program_state;
> @@ -740,9 +740,11 @@ _cogl_pipeline_progend_glsl_end (CoglPipeline *pipeline,
>        clear_flushed_matrix_stacks (program_state);
>
>        for (i = 0; i < G_N_ELEMENTS (builtin_uniforms); i++)
> -        GE_RET( program_state->builtin_uniform_locations[i], ctx,
> -                glGetUniformLocation (gl_program,
> -                                      builtin_uniforms[i].uniform_name) );
> +        if ((ctx->private_feature_flags &
> +             builtin_uniforms[i].feature_replacement) == 0)
> +          GE_RET( program_state->builtin_uniform_locations[i], ctx,
> +                  glGetUniformLocation (gl_program,
> +                                        builtin_uniforms[i].uniform_name) );
>
>        GE_RET( program_state->modelview_uniform, ctx,
>                glGetUniformLocation (gl_program,
> @@ -757,13 +759,11 @@ _cogl_pipeline_progend_glsl_end (CoglPipeline *pipeline,
>                                      "cogl_modelview_projection_matrix") );
>      }
>
> -#ifdef HAVE_COGL_GLES2
>    if (program_changed ||
>        program_state->last_used_for_pipeline != pipeline)
>      program_state->dirty_builtin_uniforms = ~(unsigned long) 0;
>
> -  update_builtin_uniforms (pipeline, gl_program, program_state);
> -#endif
> +  update_builtin_uniforms (ctx, pipeline, gl_program, program_state);
>
>    _cogl_pipeline_progend_glsl_flush_uniforms (pipeline,
>                                                program_state,
> @@ -784,14 +784,14 @@ _cogl_pipeline_progend_glsl_pre_change_notify (CoglPipeline *pipeline,
>
>    if ((change & _cogl_pipeline_get_state_for_fragment_codegen (ctx)))
>      dirty_program_state (pipeline);
> -
> -#ifdef HAVE_COGL_GLES2
> -  else if (ctx->driver == COGL_DRIVER_GLES2)
> +  else
>      {
>        int i;
>
>        for (i = 0; i < G_N_ELEMENTS (builtin_uniforms); i++)
> -        if ((change & builtin_uniforms[i].change))
> +        if ((ctx->private_feature_flags &
> +             builtin_uniforms[i].feature_replacement) == 0 &&
> +            (change & builtin_uniforms[i].change))
>            {
>              CoglPipelineProgramState *program_state
>                = get_program_state (pipeline);
> @@ -800,7 +800,6 @@ _cogl_pipeline_progend_glsl_pre_change_notify (CoglPipeline *pipeline,
>              return;
>            }
>      }
> -#endif /* HAVE_COGL_GLES2 */
>  }
>
>  /* NB: layers are considered immutable once they have any dependants
> --
> 1.7.11.3.g3c3efa5
>
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl