[vlc-devel] [PATCH v2 13/13] opengl: add plane filtering to mock filter

Alexandre Janniaux ajanni at videolabs.io
Tue Apr 6 08:57:59 UTC 2021


On Tue, Apr 06, 2021 at 10:21:46AM +0200, Romain Vimont wrote:
> On Thu, Apr 01, 2021 at 08:22:30PM +0200, Alexandre Janniaux wrote:
> > Hi,
> >
> > On Thu, Apr 01, 2021 at 07:06:03PM +0200, Romain Vimont wrote:
> > > On Thu, Apr 01, 2021 at 06:37:42PM +0200, Alexandre Janniaux wrote:
> > > > Hi,
> > > >
> > > > If the mock{plane} is not able to use any other options, I´d
> > > > rather have it as a separate submodule mock_plane than an
> > > > option of the filter, if that´s ok with you.
> > >
> > > I'd prefer to keep only 1 module and 1 file for OpenGL mocks for
> > > simplicity.
> > >
> > > Moreover, in theory, the mock filter plane could use many of existing
> > > options (angle, speed, mask).
> > >
> > > Regards
> >
> > Well, there´s two reason between this actually:
> >
> >  - --MOCK_CFG_PREFIX-plane would set all mock filter into
> >    plane filtering by default. But plane filtering and
> >    picture filtering look like very different things at
> >    first so this is a bit unexpected imho. A different
> >    module name won´t have this issue.
>
> The only difference is that it applies on individual planes instead of
> the final picture. We could enable the filter_planes flag on the mask
> mock filter (but we don't handle it because it brings little benefit to
> test the OpenGL filters behavior).
>
> >  - You can have a different submodule which can still share
> >    the same option as the first, but only enable the one it
> >    supports, making it much clearer on what is supported and
> >    what is not.
> >
> > Besides, A submodule doesn´t really add complexity, it
> > removes complexity by scoping things and add verbosity
> > instead.
> >
> > What do you think?
>
> I think that using a submodule adds complexity compared to not using
> one: we need to have in mind what happens to VLC variables, how they are
> shared/inherited, how they must be named from the command-line (with the
> parent prefix or the submodule prefix?), etc.

There´s almost never a submodule prefix for variables, especially
since submodules (ie. all modules) only have shortcuts, not names
since they share the main module´s name. It also changes nothing
to variable inheritency. But...

> For a simple mock plugin, I personally prefer without submodules.
>
> Moreover, it would be inconsistent to make the mock plane filter a
> submodule, but not the others (mask VS blend, which share the same angle
> and speed variables). So they should be separate submodules too. IMO
> it's not worth it.

I agree that it would be inconsistent but that would be something
to fix here, instead of just keeping adding additional features.
However, I eventually agree with your point below, regarding
exposing raw features for tests, so this patchset LGTM.

> In the API, blend/mask and plane/non-plane are exposed as flags. Some
> combinations are also forbidden (a plane filter may not be blend,
> because we don't support it, since it is not useful in practice). I
> think it's better to just expose the same flags in the mock filter as in
> the API it tests.
>
> Regards
>
> >
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > > >
> > > > Regards,
> > > > --
> > > > Alexandre Janniaux
> > > > Videolabs
> > > >
> > > > On Tue, Mar 30, 2021 at 01:14:11PM +0200, Romain Vimont wrote:
> > > > > This helps for testing plane filtering.
> > > > > ---
> > > > >  modules/video_output/opengl/filter_mock.c | 129 +++++++++++++++++++++-
> > > > >  1 file changed, 125 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/modules/video_output/opengl/filter_mock.c b/modules/video_output/opengl/filter_mock.c
> > > > > index 258c102f92..41851fe35d 100644
> > > > > --- a/modules/video_output/opengl/filter_mock.c
> > > > > +++ b/modules/video_output/opengl/filter_mock.c
> > > > > @@ -49,8 +49,12 @@
> > > > >   * Several instances may be combined:
> > > > >   *
> > > > >   *     ./vlc file.mkv --video-filter='opengl{filter="mock{mask,speed=1}:mock{angle=180,speed=-1}"}'
> > > > > + *
> > > > > + * It can also be used to filter planes separately, drawing each one with a
> > > > > + * small offset:
> > > > > + *
> > > > > + *     ./vlc file.mkv --video-filter='opengl{filter=mock{plane}}'
> > > > >   */
> > > > > -
> > > > >  #ifdef HAVE_CONFIG_H
> > > > >  # include "config.h"
> > > > >  #endif
> > > > > @@ -80,7 +84,7 @@
> > > > >  #define MOCK_CFG_PREFIX "mock-"
> > > > >
> > > > >  static const char *const filter_options[] = {
> > > > > -    "angle", "mask", "msaa", "speed", NULL
> > > > > +    "angle", "mask", "msaa", "plane", "speed", NULL
> > > > >  };
> > > > >
> > > > >  struct sys {
> > > > > @@ -92,6 +96,7 @@ struct sys {
> > > > >          GLint vertex_pos;
> > > > >          GLint rotation_matrix;
> > > > >          GLint vertex_color; // blend (non-mask) only
> > > > > +        GLint plane;
> > > > >      } loc;
> > > > >
> > > > >      float theta0;
> > > > > @@ -193,6 +198,31 @@ DrawMask(struct vlc_gl_filter *filter, const struct vlc_gl_input_meta *meta)
> > > > >      return VLC_SUCCESS;
> > > > >  }
> > > > >
> > > > > +static int
> > > > > +DrawPlane(struct vlc_gl_filter *filter, const struct vlc_gl_input_meta *meta)
> > > > > +{
> > > > > +    struct sys *sys = filter->sys;
> > > > > +
> > > > > +    const opengl_vtable_t *vt = &filter->api->vt;
> > > > > +
> > > > > +    vt->UseProgram(sys->program_id);
> > > > > +
> > > > > +    struct vlc_gl_sampler *sampler = vlc_gl_filter_GetSampler(filter);
> > > > > +    vlc_gl_sampler_Load(sampler);
> > > > > +
> > > > > +    vt->Uniform1i(sys->loc.plane, (int) meta->plane);
> > > > > +
> > > > > +    vt->BindBuffer(GL_ARRAY_BUFFER, sys->vbo);
> > > > > +    vt->EnableVertexAttribArray(sys->loc.vertex_pos);
> > > > > +    vt->VertexAttribPointer(sys->loc.vertex_pos, 2, GL_FLOAT, GL_FALSE, 0,
> > > > > +                            (const void *) 0);
> > > > > +
> > > > > +    vt->Clear(GL_COLOR_BUFFER_BIT);
> > > > > +    vt->DrawArrays(GL_TRIANGLE_STRIP, 0, 4);
> > > > > +
> > > > > +    return VLC_SUCCESS;
> > > > > +}
> > > > > +
> > > > >  static void
> > > > >  Close(struct vlc_gl_filter *filter)
> > > > >  {
> > > > > @@ -250,6 +280,10 @@ InitBlend(struct vlc_gl_filter *filter)
> > > > >      sys->loc.vertex_color = vt->GetAttribLocation(program_id, "vertex_color");
> > > > >      assert(sys->loc.vertex_color != -1);
> > > > >
> > > > > +    sys->loc.rotation_matrix = vt->GetUniformLocation(sys->program_id,
> > > > > +                                                      "rotation_matrix");
> > > > > +    assert(sys->loc.rotation_matrix != -1);
> > > > > +
> > > > >      vt->GenBuffers(1, &sys->vbo);
> > > > >
> > > > >      static const GLfloat data[] = {
> > > > > @@ -356,6 +390,88 @@ InitMask(struct vlc_gl_filter *filter)
> > > > >      return VLC_SUCCESS;
> > > > >  }
> > > > >
> > > > > +static int
> > > > > +InitPlane(struct vlc_gl_filter *filter)
> > > > > +{
> > > > > +    struct sys *sys = filter->sys;
> > > > > +    const opengl_vtable_t *vt = &filter->api->vt;
> > > > > +
> > > > > +    /* Must be initialized before calling vlc_gl_filter_GetSampler() */
> > > > > +    filter->config.filter_planes = true;
> > > > > +
> > > > > +    struct vlc_gl_sampler *sampler = vlc_gl_filter_GetSampler(filter);
> > > > > +
> > > > > +    static const char *const VERTEX_SHADER =
> > > > > +        SHADER_VERSION
> > > > > +        "attribute vec2 vertex_pos;\n"
> > > > > +        "varying vec2 tex_coords;\n"
> > > > > +        "void main() {\n"
> > > > > +        "  gl_Position = vec4(vertex_pos, 0.0, 1.0);\n"
> > > > > +        "  tex_coords = vec2((vertex_pos.x + 1.0) / 2.0,\n"
> > > > > +        "                    (vertex_pos.y + 1.0) / 2.0);\n"
> > > > > +        "}\n";
> > > > > +
> > > > > +    static const char *const FRAGMENT_SHADER_TEMPLATE =
> > > > > +        SHADER_VERSION
> > > > > +        "%s\n" /* extensions */
> > > > > +        FRAGMENT_SHADER_PRECISION
> > > > > +        "%s\n" /* vlc_texture definition */
> > > > > +        "varying vec2 tex_coords;\n"
> > > > > +        "uniform int plane;\n"
> > > > > +        "void main() {\n"
> > > > > +        "  vec2 offset = vec2(float(plane) * 0.02);\n"
> > > > > +        "  gl_FragColor = vlc_texture(fract(tex_coords + offset));\n"
> > > > > +        "}\n";
> > > > > +
> > > > > +    const char *extensions = sampler->shader.extensions
> > > > > +                           ? sampler->shader.extensions : "";
> > > > > +
> > > > > +    char *fragment_shader;
> > > > > +    int ret = asprintf(&fragment_shader, FRAGMENT_SHADER_TEMPLATE, extensions,
> > > > > +                       sampler->shader.body);
> > > > > +    if (ret < 0)
> > > > > +        return VLC_EGENERIC;
> > > > > +
> > > > > +    GLuint program_id =
> > > > > +        vlc_gl_BuildProgram(VLC_OBJECT(filter), vt,
> > > > > +                            1, (const char **) &VERTEX_SHADER,
> > > > > +                            1, (const char **) &fragment_shader);
> > > > > +    free(fragment_shader);
> > > > > +    if (!program_id)
> > > > > +        return VLC_EGENERIC;
> > > > > +
> > > > > +    sys->program_id = program_id;
> > > > > +
> > > > > +    vlc_gl_sampler_FetchLocations(sampler, program_id);
> > > > > +
> > > > > +    sys->loc.vertex_pos = vt->GetAttribLocation(sys->program_id, "vertex_pos");
> > > > > +    assert(sys->loc.vertex_pos != -1);
> > > > > +
> > > > > +    sys->loc.plane = vt->GetUniformLocation(program_id, "plane");
> > > > > +    assert(sys->loc.plane != -1);
> > > > > +
> > > > > +    static const GLfloat vertex_pos[] = {
> > > > > +        -1,  1,
> > > > > +        -1, -1,
> > > > > +         1,  1,
> > > > > +         1, -1,
> > > > > +    };
> > > > > +
> > > > > +    vt->BindBuffer(GL_ARRAY_BUFFER, sys->vbo);
> > > > > +    vt->BufferData(GL_ARRAY_BUFFER, sizeof(vertex_pos), vertex_pos,
> > > > > +                   GL_STATIC_DRAW);
> > > > > +
> > > > > +    vt->BindBuffer(GL_ARRAY_BUFFER, 0);
> > > > > +
> > > > > +    static const struct vlc_gl_filter_ops ops = {
> > > > > +        .draw = DrawPlane,
> > > > > +        .close = Close,
> > > > > +    };
> > > > > +    filter->ops = &ops;
> > > > > +
> > > > > +    return VLC_SUCCESS;
> > > > > +}
> > > > > +
> > > > >  static vlc_gl_filter_open_fn Open;
> > > > >  static int
> > > > >  Open(struct vlc_gl_filter *filter, const config_chain_t *config,
> > > > > @@ -366,6 +482,7 @@ Open(struct vlc_gl_filter *filter, const config_chain_t *config,
> > > > >      config_ChainParse(filter, MOCK_CFG_PREFIX, filter_options, config);
> > > > >
> > > > >      bool mask = var_InheritBool(filter, MOCK_CFG_PREFIX "mask");
> > > > > +    bool plane = var_InheritBool(filter, MOCK_CFG_PREFIX "plane");
> > > > >      float angle = var_InheritFloat(filter, MOCK_CFG_PREFIX "angle");
> > > > >      float speed = var_InheritFloat(filter, MOCK_CFG_PREFIX "speed");
> > > > >      int msaa = var_InheritInteger(filter, MOCK_CFG_PREFIX "msaa");
> > > > > @@ -375,7 +492,9 @@ Open(struct vlc_gl_filter *filter, const config_chain_t *config,
> > > > >          return VLC_EGENERIC;
> > > > >
> > > > >      int ret;
> > > > > -    if (mask)
> > > > > +    if (plane)
> > > > > +        ret = InitPlane(filter);
> > > > > +    else if (mask)
> > > > >          ret = InitMask(filter);
> > > > >      else
> > > > >          ret = InitBlend(filter);
> > > > > @@ -387,7 +506,8 @@ Open(struct vlc_gl_filter *filter, const config_chain_t *config,
> > > > >      sys->theta0 = angle * M_PI / 180; /* angle in degrees, theta0 in radians */
> > > > >      sys->speed = speed;
> > > > >
> > > > > -    filter->config.msaa_level = msaa;
> > > > > +    /* MSAA is not supported for plane filters */
> > > > > +    filter->config.msaa_level = plane ? 0 : msaa;
> > > > >
> > > > >      return VLC_SUCCESS;
> > > > >
> > > > > @@ -407,5 +527,6 @@ vlc_module_begin()
> > > > >      add_float(MOCK_CFG_PREFIX "angle", 0.f, NULL, NULL, false) /* in degrees */
> > > > >      add_float(MOCK_CFG_PREFIX "speed", 0.f, NULL, NULL, false) /* in rotations per minute */
> > > > >      add_bool(MOCK_CFG_PREFIX "mask", false, NULL, NULL, false)
> > > > > +    add_bool(MOCK_CFG_PREFIX "plane", false, NULL, NULL, false)
> > > > >      add_integer(MOCK_CFG_PREFIX "msaa", 4, NULL, NULL, false);
> > > > >  vlc_module_end()
> > > > > --
> > > > > 2.31.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