[vlc-devel] [PATCH 02/24] opengl: make fragment shader local

[Alexandre Janniaux]

Hi,

Looks good overall, comments inline:

On Mon, Jan 27, 2020 at 09:19:52PM +0100, Romain Vimont wrote:
> Since vlc_gl_interop has been introduced, the fragment shader is built
> by vout_helper instead of the converter module.
>
> Therefore, there is no need to keep it exposed in the
> opengl_tex_converter_t instance.
> ---
>  modules/video_output/opengl/converter.h   |  4 ---
>  modules/video_output/opengl/vout_helper.c | 43 +++++++++++------------
>  2 files changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/modules/video_output/opengl/converter.h b/modules/video_output/opengl/converter.h
> index 2d3ec4287b..11a2ab61c2 100644
> --- a/modules/video_output/opengl/converter.h
> +++ b/modules/video_output/opengl/converter.h
> @@ -59,10 +59,6 @@ struct opengl_tex_converter_t
>       * has no default precision qualifier for floating point types. */
>      const char *glsl_precision_header;
>
> -    /* Fragment shader, must be set from the module open function. It will be
> -     * deleted by the caller. */
> -    GLuint fshader;
> -
>      /* The following is used and filled by the opengl_fragment_shader_init
>       * function. */
>      struct {
> diff --git a/modules/video_output/opengl/vout_helper.c b/modules/video_output/opengl/vout_helper.c
> index a8c73074fb..dec3be50c3 100644
> --- a/modules/video_output/opengl/vout_helper.c
> +++ b/modules/video_output/opengl/vout_helper.c
> @@ -394,7 +394,24 @@ opengl_link_program(struct prgm *prgm)
>      struct vlc_gl_interop *interop = tc->interop;
>
>      GLuint vertex_shader = BuildVertexShader(tc, interop->tex_count);
> -    GLuint shaders[] = { tc->fshader, vertex_shader };
> +    if (!vertex_shader)
> +        return VLC_EGENERIC;
> +
> +    GLuint fragment_shader =
> +        opengl_fragment_shader_init(tc, interop->tex_target,
> +                                    interop->sw_fmt.i_chroma,
> +                                    interop->sw_fmt.space);
> +    if (!fragment_shader)
> +        return VLC_EGENERIC;
> +
> +    assert(fragment_shader != 0 &&
> +           interop->tex_target != 0 &&
> +           interop->tex_count > 0 &&
> +           interop->ops->update_textures != NULL &&
> +           tc->pf_fetch_locations != NULL &&
> +           tc->pf_prepare_shader != NULL);

(disclamer: I haven't checked whether it's changed after the patch)

The fragment_shader != 0 test is useless here, as is it already
checked just above. ;)

Also nit, but it might be better to split the assertions to
ease the feedback? It's not important for me though.

> +
> +    GLuint shaders[] = { fragment_shader, vertex_shader };
>
>      /* Check shaders messages */
>      for (unsigned i = 0; i < 2; i++) {
> @@ -415,12 +432,12 @@ opengl_link_program(struct prgm *prgm)
>      }
>
>      prgm->id = tc->vt->CreateProgram();
> -    tc->vt->AttachShader(prgm->id, tc->fshader);
> +    tc->vt->AttachShader(prgm->id, fragment_shader);
>      tc->vt->AttachShader(prgm->id, vertex_shader);
>      tc->vt->LinkProgram(prgm->id);
>
>      tc->vt->DeleteShader(vertex_shader);
> -    tc->vt->DeleteShader(tc->fshader);
> +    tc->vt->DeleteShader(fragment_shader);
>
>      /* Check program messages */
>      int infoLength = 0;
> @@ -623,26 +640,6 @@ opengl_init_program(vout_display_opengl_t *vgl, vlc_video_context *context,
>          return VLC_EGENERIC;
>      }
>
> -    GLuint fragment_shader =
> -        opengl_fragment_shader_init(tc, interop->tex_target,
> -                                    interop->sw_fmt.i_chroma,
> -                                    interop->sw_fmt.space);
> -    if (!fragment_shader)
> -    {
> -        vlc_object_delete(interop);
> -        free(tc);
> -        return VLC_EGENERIC;
> -    }
> -
> -    tc->fshader = fragment_shader;
> -
> -    assert(tc->fshader != 0 &&
> -           interop->tex_target != 0 &&
> -           interop->tex_count > 0 &&
> -           interop->ops->update_textures != NULL &&
> -           tc->pf_fetch_locations != NULL &&
> -           tc->pf_prepare_shader != NULL);
> -
>      prgm->tc = tc;
>
>      ret = opengl_link_program(prgm);
> --
> 2.25.0
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel