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

[Hugo Beauzée-Luyssen]

On Fri, Oct 5, 2018, at 9:36 PM, Niklas Haas wrote:
> 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.
> 

I didn't mean to remove the define, but rather to pass it through the pre-project CFLAGS (most likely OPENGL_COMMONFLAGS) rather than config.h
That being said we will soon move out of autotools so we probably don't care about such details anymore.

> > > +#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:
> 

Oh my bad then, I wasn't aware of that problem!
Then sure, please use the COMPS macro :)

> #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.
> 

What do you mean by "we don't actually check for any of them" ?

> > > +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?

Yes

> 
> > > +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?
> 

I'm really not sure what should be done here. I was mostly hoping to start a discussion about VLAs :)
IMO it wouldn't be so bad to allocate an array of VLCVK_MAX_BUFFERS pointers on the stack.

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

That's also a very valid option :)

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

Thanks for your patches!

> > 
> > 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


-- 
  Hugo Beauzée-Luyssen
  hugo at beauzee.fr