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

[Romain Vimont]

On Mon, Jun 22, 2020 at 12:04:23PM +0200, Alexandre Janniaux wrote:
> Hi,
> 
> Sorry to be annoying, but I find it very important to explain
> why this feature has been removed instead of being fixed

But there is no "feature" removed, nor something to be fixed. It's just
trivial code simplification: use a value directly instead of assigning
it to a field then read it.

> be it by saying that it was not used yet and it simplify the next
> patches.

This patch indeed simplifies the following patches (by not using
additional unnecessary fields), but it's not its main purpose. It is
quite independent and would make sense even alone (as it was proposed by
Marvin).

> If I look at commit message and see this commit as-is, I'd
> have to look-up discussions to understand why

IMO, all these discussions are about extrapolations of future possible
features, but are not about why the patch in the first place (which is
no more than just code simplification).

> > 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.
> 
> Then just write it in the commit message. ;)

Most of what I wrote here seems quite off-topic for the patch itself,
and would be quite confusing for just understanding the change.

But let's try a mix of the previous commit messages:

    Since the GLSL version and precision header are defined statically,
    just insert them directly in the templates.

    This simplifies the code, and improves consistency with the
    sub_renderer (and other filters, in the future), without preventing
    to make some parts more dynamic later if necessary.

What do you think?

> 
> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> On Mon, Jun 22, 2020 at 09:30:32AM +0200, Romain Vimont wrote:
> > 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
> > _______________________________________________
> > 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