[vlc-devel] [PATCH] opengl: insert shader headers at compile time

[Romain Vimont]

On Thu, Jun 18, 2020 at 11:59:33AM +0200, Alexandre Janniaux wrote:
> Hi,
> 
> On Thu, Jun 18, 2020 at 11:08:15AM +0200, Romain Vimont wrote:
> > On Thu, Jun 18, 2020 at 10:58:30AM +0200, Alexandre Janniaux wrote:
> > > Hi,
> > >
> > > We can reintroduce the field later when we add a correct
> > > support for this.
> >
> > This is just the renderer, the renderer itself does not (currently)
> > require GLSL > 120. If in the future it needs another version, maybe we
> > will use a field, maybe not… or maybe the content requiring GLSL > 120
> > will be in a separate filter… I don't know.
> >
> > For now, these fields are just an annoying useless indirection.
> >
> > > But that's not what the commit message is conveying.
> >
> > The commit message says that the fiels are not used anymore, and that
> > the version and precision header are inserted into the shader code
> > directly, which is what the commit does. :)
> 
> My points is that what this commit does is already written
> in the commit diff, while the rationale isn't clearly written
> in the commit message.

Yes, maybe it's not very clear. The rationale is just to simplify the
code.

I suggest to use the commit message from Marvin instead, which gives
more explanations:

    Instead of globally storing the glsl version and precision 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.

<https://mailman.videolan.org/pipermail/vlc-devel/2020-March/132784.html>

What do you think?

> > Insert them directly into the shader string literal, instead of
> > initializing renderer fields and inserting them at runtime.
> 
> This is factually what the commit does, which is quite easy
> to read in the diff, but it doesn't indicate neither that
> it's because we cannot currently support any higher version,

IMO, that's not relevant for this commit.

> nor that it will mean that the renderer code must be compiled
> twice, once for OpenGL and once for OpenGL ES.

Yes, we could add that we don't need to dynamically insert the shader
version because currently the version is not dynamic. I suggest this
change to the commit message above:

    Since the GLSL version and precision header are defined statically,
    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.

> The double compilation doesn't really matter right now because
> it's already the case for the two opengl/es modules we build
> on Linux, but it extends this situation in an annoying way.

If we want to compile once for both OpenGL and OpenGL ES, there will be
changes to do at different places, and we will probably use some
`is_opengles` boolean somewhere instead (so we will probably not keep
two fields `glsl_version` and `glsl_precision_header` in the renderer
anyway, which is only one specific filter). So IMO, this change just
simplifies the current code, and improves consistency with the
sub_renderer (and other filters, in the future), without preventing to
make some parts more dynamic in the future when necessary.

Regards