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

[Romain Vimont]

On 1/28/20 4:40 PM, Alexandre Janniaux wrote:
> 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)

It was already like that before :)

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

I agree. (But this existing code will be reworked in further refactors 
anyway)

> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>