[vlc-devel] [PATCH 5/7] vout: add new vulkan/libplacebo vout

[Niklas Haas]

On Tue, 02 Oct 2018 16:37:09 +0200, Hugo Beauzée-Luyssen <hugo at beauzee.fr> wrote:
> Hi,
> 
> On Mon, Oct 1, 2018, at 4:51 PM, Niklas Haas wrote:
> > From: Niklas Haas <git at haasn.xyz>
> > 
> > +dnl
> > +dnl  libplacebo support
> > +dnl
> > +AC_ARG_ENABLE([libplacebo],
> > +  AS_HELP_STRING([--disable-libplacebo],
> > +      [disable libplacebo support (default auto)]))
> > +
> > +AS_IF([test "$enable_libplacebo" != "no"], [
> > +  PKG_CHECK_MODULES([LIBPLACEBO], [libplacebo >= 0.5], [
> > +    AC_DEFINE([HAVE_LIBPLACEBO], [1], [Define to 1 if libplacebo is 
> > enabled.])
> 
> Unrelated to the patch itself since this is just moving, but couldn't this be defined in the .am file instead of config.h? It would prevent a full rebuilt of all files when adding/removing libplacebo as config.h wouldn't change

The opengl vout checks for HAVE_LIBPLACEBO throughout its code.

> > +#define SIZE(n, bits, pad) (((n) * (bits) + (pad) + 7) / 8)
> > +#define COMPS(...) {__VA_ARGS__}
> 
> IIRC this is not valid ANSI C. Most compilers we use probably support it anyway, but you're only using this macro in 2 locations so it might be worth removing it plainly.

We need it because writing `PLANE(2, bits, {1, 2}, wd, hd, 0)` gets
interpreted as 7 macro arguments (rather than 6). I could make it ANSI C
by doing it like this though:

#define ARR2(x,y) {x,y}
#define ARR4(x,y,z,w) {x,y,z,w}

and then using those instead of COMPS() respectively in the two cases
it's required. Would that be preferable?

> > +#ifdef HAVE_CONFIG_H
> > +# include <config.h>
> > +#endif
> > +
> > +
> > +#ifdef HAVE_CONFIG_H
> > +# include <config.h>
> > +#endif
> > +
> 
> That's one too many :)

Fixed, and I've also removed all of the unused config.h imports in
general. We don't actually check for any of them in the code.

> > +error:
> > +    pl_renderer_destroy(&sys->renderer);
> > +    if (sys->vk != NULL)
> > +        vlc_vk_Release(sys->vk);
> > +    vlc_obj_free(obj, sys);
> 
> I believe this is done automatically

The vlc_obj_free(), you mean? So I just remove that line?

> > +static picture_pool_t *Pool(vout_display_t *vd, unsigned 
> > requested_count)
> > +{
> > +    assert(requested_count <= VLCVK_MAX_BUFFERS);
> > +    vout_display_sys_t *sys = vd->sys;
> > +    if (sys->pool)
> > +        return sys->pool;
> > +
> > +    unsigned count;
> > +    picture_t *pictures[requested_count];
> 
> I'm not sure we want more VLAs

That code (authored by Thomas) was copy/pasted from the opengl vout. I
can change it to use malloc if that would make you happier, but maybe we
should do that for both vouts then for consistency's sake?

> This is also missing some POTFILES.in inclusion

I've opted to just remove almost all of the translated messages instead.
The only ones I kept in are the name of the module itself ("Vulkan video
output") and the option to choose which surface extension to use.

I've addressed/fixed the rest of your concerns, thanks for the feedback.

> 
> Regards,
> 
> -- 
>   Hugo Beauzée-Luyssen
>   hugo at beauzee.fr
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel