[vlc-devel] [PATCH] opengl: insert shader headers at compile time
Alexandre Janniaux
ajanni at videolabs.io
Thu Jun 18 11:59:33 CEST 2020
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. To be a little clearer:
> The version and precision header strings are different for OpenGL and
> OpenGL ES.
^ this part of the commit message doesn't really explain
something for the current patch as it has always been the
case and is not referenced again in what's next in the commit
message or in the patch.
> 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,
nor that it will mean that the renderer code must be compiled
twice, once for OpenGL and once for OpenGL ES.
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.
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > On Thu, Jun 18, 2020 at 10:52:51AM +0200, Romain Vimont wrote:
> > > On Thu, Jun 18, 2020 at 10:23:21AM +0200, Alexandre Janniaux wrote:
> > > > Hi,
> > > >
> > > > It doesn't feel like a good idea, and goes against Haasn
> > > > patches a few week ago.
> > >
> > > I think this change is independant of his patches.
> > >
> > > (We already discussed about this for commit
> > > 2fdbd23f2806cdad15ca03e0b83de46e543f6b6c.)
> > >
> > > > Are there reasons to do so?
> > >
> > > Currently, the renderer initializes two fields (glsl_version and
> > > glsl_precision_header) from constants, and inject their value into the
> > > shader code (via asprintf).
> > >
> > > Using these struct fields is useless: it's simpler to just put the
> > > constants in the string.
> > >
> > > Btw, the sub_renderer already does that. This change had also been
> > > independently proposed by Marvin:
> > > https://mailman.videolan.org/pipermail/vlc-devel/2020-March/132784.html
> > >
> > > >
> > > > Regards,
> > > > --
> > > > Alexandre Janniaux
> > > > Videolabs
> > > >
> > > > On Wed, Jun 17, 2020 at 09:33:11PM +0200, Romain Vimont wrote:
> > > > > The version and precision header strings are different for OpenGL and
> > > > > OpenGL ES.
> > > > >
> > > > > Insert them directly into the shader string literal, instead of
> > > > > initializing renderer fields and inserting them at runtime.
> > > > > ---
> > > > > modules/video_output/opengl/renderer.c | 31 ++++++++++++++------------
> > > > > modules/video_output/opengl/renderer.h | 6 -----
> > > > > 2 files changed, 17 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/modules/video_output/opengl/renderer.c b/modules/video_output/opengl/renderer.c
> > > > > index 335ca27e4841..6f7d42ee7ab9 100644
> > > > > --- a/modules/video_output/opengl/renderer.c
> > > > > +++ b/modules/video_output/opengl/renderer.c
> > > > > @@ -169,12 +169,23 @@ InitStereoMatrix(GLfloat matrix_out[static 3*3],
> > > > > #undef ROW
> > > > > }
> > > > >
> > > > > +/* https://en.wikipedia.org/wiki/OpenGL_Shading_Language#Versions */
> > > > > +#ifdef USE_OPENGL_ES2
> > > > > +# define SHADER_VERSION "#version 100\n"
> > > > > + /* In OpenGL ES, the fragment language has no default precision qualifier for
> > > > > + * floating point types. */
> > > > > +# define FRAGMENT_SHADER_PRECISION "precision highp float;\n"
> > > > > +#else
> > > > > +# define SHADER_VERSION "#version 120\n"
> > > > > +# define FRAGMENT_SHADER_PRECISION
> > > > > +#endif
> > > > > +
> > > > > static char *
> > > > > BuildVertexShader(const struct vlc_gl_renderer *renderer)
> > > > > {
> > > > > /* Basic vertex shader */
> > > > > static const char *template =
> > > > > - "#version %u\n"
> > > > > + SHADER_VERSION
> > > > > "attribute vec2 PicCoordsIn;\n"
> > > > > "varying vec2 PicCoords;\n"
> > > > > "attribute vec3 VertexPosition;\n"
> > > > > @@ -188,8 +199,8 @@ BuildVertexShader(const struct vlc_gl_renderer *renderer)
> > > > > " * vec4(VertexPosition, 1.0);\n"
> > > > > "}";
> > > > >
> > > > > - char *code;
> > > > > - if (asprintf(&code, template, renderer->glsl_version) < 0)
> > > > > + char *code = strdup(template);
> > > > > + if (!code)
> > > > > return NULL;
> > > > >
> > > > > if (renderer->dump_shaders)
> > > > > @@ -204,9 +215,9 @@ BuildFragmentShader(struct vlc_gl_renderer *renderer)
> > > > > struct vlc_gl_sampler *sampler = renderer->sampler;
> > > > >
> > > > > static const char *template =
> > > > > - "#version %u\n"
> > > > > + SHADER_VERSION
> > > > > "%s" /* extensions */
> > > > > - "%s" /* precision header */
> > > > > + FRAGMENT_SHADER_PRECISION
> > > > > "%s" /* vlc_texture definition */
> > > > > "varying vec2 PicCoords;\n"
> > > > > "void main() {\n"
> > > > > @@ -217,8 +228,7 @@ BuildFragmentShader(struct vlc_gl_renderer *renderer)
> > > > > ? sampler->shader.extensions : "";
> > > > >
> > > > > char *code;
> > > > > - int ret = asprintf(&code, template, renderer->glsl_version, extensions,
> > > > > - renderer->glsl_precision_header, sampler->shader.body);
> > > > > + int ret = asprintf(&code, template, extensions, sampler->shader.body);
> > > > > if (ret < 0)
> > > > > return NULL;
> > > > >
> > > > > @@ -328,13 +338,6 @@ vlc_gl_renderer_New(vlc_gl_t *gl, const struct vlc_gl_api *api,
> > > > > renderer->api = api;
> > > > > renderer->vt = vt;
> > > > > renderer->dump_shaders = var_InheritInteger(gl, "verbose") >= 4;
> > > > > -#if defined(USE_OPENGL_ES2)
> > > > > - renderer->glsl_version = 100;
> > > > > - renderer->glsl_precision_header = "precision highp float;\n";
> > > > > -#else
> > > > > - renderer->glsl_version = 120;
> > > > > - renderer->glsl_precision_header = "";
> > > > > -#endif
> > > > >
> > > > > int ret = opengl_link_program(renderer);
> > > > > if (ret != VLC_SUCCESS)
> > > > > diff --git a/modules/video_output/opengl/renderer.h b/modules/video_output/opengl/renderer.h
> > > > > index 6f4029504fb2..7130cfb26263 100644
> > > > > --- a/modules/video_output/opengl/renderer.h
> > > > > +++ b/modules/video_output/opengl/renderer.h
> > > > > @@ -50,12 +50,6 @@ struct vlc_gl_renderer
> > > > > /* True to dump shaders */
> > > > > bool dump_shaders;
> > > > >
> > > > > - /* GLSL version, set by the caller. 100 for GLSL ES, 120 for desktop GLSL */
> > > > > - unsigned glsl_version;
> > > > > - /* Precision header, set by the caller. In OpenGLES, the fragment language
> > > > > - * has no default precision qualifier for floating point types. */
> > > > > - const char *glsl_precision_header;
> > > > > -
> > > > > GLuint program_id;
> > > > >
> > > > > struct {
> > > > > --
> > > > > 2.27.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
> > > _______________________________________________
> > > 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list