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

[Alexandre Janniaux]

Hi,

Sorry to be annoying, but I find it very important to explain
why this feature has been removed instead of being fixed, be
it by saying that it was not used yet and it simplify the next
patches.

If I look at commit message and see this commit as-is, I'd
have to look-up discussions to understand why, and this is
especially true for external or future committers that will
need to understand your full patchset.

> 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. ;)

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