[vlc-devel] [PATCH] opengl: simplify shader templates

[Romain Vimont]

Hi,

I agree with the change.

However, I currently have a local branch with OpenGL refactors and 
filters. On this branch I have this commit which does almost the same: 
https://code.videolan.org/rom1v/vlc/-/commit/6213689bcf19c120cfd93f952437017109f33983

I try to submit patchsets step by step on the ML (this commit is 
currently at position 42). Would you agree to wait for it to avoid 
unnecessary conflicts?

Thank you :)

On 3/31/20 11:29 AM, Marvin Scholz wrote:
> Instead of globally storing the glsl version and prevision header,
> just conditionally insert them directly in the templates, which
> makes the shaders easier to understand as one has not to jump around
> in the code to find the different variable that are replaced into
> the shader for the different cases.
> 
> This is the same way that it is already done for the sub_renderer
> shaders.
> ---
>   modules/video_output/opengl/renderer.c | 29 +++++++++++++-------------
>   modules/video_output/opengl/renderer.h |  6 ------
>   2 files changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/modules/video_output/opengl/renderer.c b/modules/video_output/opengl/renderer.c
> index ae4179be4a..f796485004 100644
> --- a/modules/video_output/opengl/renderer.c
> +++ b/modules/video_output/opengl/renderer.c
> @@ -249,7 +249,11 @@ BuildVertexShader(const struct vlc_gl_renderer *renderer)
>   {
>       /* Basic vertex shader */
>       static const char *template =
> -        "#version %u\n"
> +#if defined(USE_OPENGL_ES2)
> +        "#version 100\n"
> +#else
> +        "#version 120\n"
> +#endif
>           "attribute vec2 PicCoordsIn;\n"
>           "varying vec2 PicCoords;\n"
>           "attribute vec3 VertexPosition;\n"
> @@ -263,8 +267,8 @@ BuildVertexShader(const struct vlc_gl_renderer *renderer)
>           "               * vec4(VertexPosition, 1.0);\n"
>           "}";
>   
> -    char *code;
> -    if (asprintf(&code, template, renderer->glsl_version) < 0)
> +    char *code = strdup(template);
> +    if (code == NULL)
>           return NULL;
>   
>       if (renderer->b_dump_shaders)
> @@ -285,9 +289,14 @@ BuildFragmentShader(struct vlc_gl_renderer *renderer)
>           return NULL;
>   
>       static const char *template =
> -        "#version %u\n"
> +#if defined(USE_OPENGL_ES2)
> +        "#version 100\n"
>           "%s" /* extensions */
> -        "%s" /* precision header */
> +        "precision highp float;\n"
> +#else
> +        "#version 120\n"
> +        "%s" /* extensions */
> +#endif
>           "%s" /* vlc_texture definition */
>           "varying vec2 PicCoords;\n"
>           "void main() {\n"
> @@ -300,8 +309,7 @@ BuildFragmentShader(struct vlc_gl_renderer *renderer)
>                              : "";
>   
>       char *code;
> -    int ret = asprintf(&code, template, renderer->glsl_version, extensions,
> -                       renderer->glsl_precision_header, vlc_texture);
> +    int ret = asprintf(&code, template, extensions, vlc_texture);
>       free(vlc_texture);
>       if (ret < 0)
>           return NULL;
> @@ -451,13 +459,6 @@ vlc_gl_renderer_New(vlc_gl_t *gl, const struct vlc_gl_api *api,
>       renderer->api = api;
>       renderer->vt = vt;
>       renderer->b_dump_shaders = b_dump_shaders;
> -#if defined(USE_OPENGL_ES2)
> -    renderer->glsl_version = 100;
> -    renderer->glsl_precision_header = "precision highp float;\n";
> -#else
> -    renderer->glsl_version = 120;
> -    renderer->glsl_precision_header = "";
> -#endif
>   
>   #ifdef HAVE_LIBPLACEBO
>       // Create the main libplacebo context
> diff --git a/modules/video_output/opengl/renderer.h b/modules/video_output/opengl/renderer.h
> index 8151045eb4..a4144e61f9 100644
> --- a/modules/video_output/opengl/renderer.h
> +++ b/modules/video_output/opengl/renderer.h
> @@ -52,12 +52,6 @@ struct vlc_gl_renderer
>       /* True to dump shaders, set by the caller */
>       bool b_dump_shaders;
>   
> -    /* GLSL version, set by the caller. 100 for GLSL ES, 120 for desktop GLSL */
> -    unsigned glsl_version;
> -    /* Precision header, set by the caller. In OpenGLES, the fragment language
> -     * has no default precision qualifier for floating point types. */
> -    const char *glsl_precision_header;
> -
>       GLuint program_id;
>   
>       struct {
>