[vlc-devel] [RFC] custom module capabilities

[Alexandre Janniaux]

Hi,

On Thu, Sep 03, 2020 at 07:12:57PM +0100, Lyndon Brown wrote:
> This is all to-be-merged (in-tree) stuff though right? So could you not
> just easily incorporate addition of a new core enum entry and bump of
> the plugin API in the patch sets?

I hope yes, but it just moves the issue. Being able to
easily use the module system without needing to modify
the core is really good at the prototyping step. One
really big benefit of the current system is that you can
extend the initial set of features to introduce your own
capabilities and completely isolate what the modules
provides from what the core exposes.

It's also much simpler and much easier to maintain with
string than with enum, since:

 - with an enum, you need to maintain a map of what the
   module capability is to display it in the logs, so
   you actually have two locations to maintain where
   there were none before.

 - if you offset the index, you break compatibility with
   the currently compiled plugins, which can happen much
   easily than merging code with the wrong string
   capability since it will works on your machine and
   probably other dev machines, but might not work for code
   that has been shipped into an external plugin, and
   nothing really prevent your from adding a line or removing
   a line in the middle of an enum. It's also much harmful
   since you will open a module expecting a different
   capability than the one it provides, thus probably a
   different activate function, which means you'll likely
   wreck the stack (eg. write into filter->fmt_in while the
   module was opened like a demux).

> The current solution I implemented does, as I said, allow for custom
> string based capabilities to be used just as currently, alongside the
> main system using an enum. But ideally it would be neater if such
> support were dropped and only the enum was used. Is tweaking the enum
> in your work sufficiently problematic as to justify retaining this in
> an enum solution?

If you have both enum and string, it's just adding complexity
so I don't think this is a good idea either.

My stance on this is more "what does an enum concretely bring
here that would justify the additional complexity of the
indirection, maintenance cost and risk exposed above?"

> I do appreciate the agility of the existing string solution, though I'm
> not a fan of it :D
>
> Is there any desire to keep such custom string capability support for
> the benefit of 3rd-party out-of-tree modules? I.e. if one such module
> needs to create a new capability for other such out-of-tree modules to
> provide for it. Is supporting this a real concern?
>
> Regarding display in interfaces and such, a simple translation
> mechanism is provided in the solution for this.
>
> There's no particular problem resulting from the efficiency of the
> current design, I just saw opportunity to do better. As I said in the
> reply just sent to what Remi said, one efficiency improvement that the
> enum solution offers is the ability to fetch the set of modules for a
> given capability as quickly as indexing into an array, avoiding
> entirely binary/hash string based searches. It thus helps optimise all
> of the code paths setting up playback of media and such. (The plugin
> database already stores lists sorted by score and capability, but using
> the enum as a numeric array index we grab the right list directly,
> rather than searching for it).

I appreciate the work you've made on this, because I know
it's a very long and time-consuming task to update each and
every module in addition with the core.

But to be honest, my opinion is that if it doesn't sort out
a real performance issue, it's probably not worth it.

    git grep set_capability | cut -d\" -f2 | sort -u

The command above, while not listing all capabilities,
returns 66 results.

    log2(66) ~= 7

We usually load something like 10 or 20 modules during the
runtime of VLC so you're basically trying to optimize a
search with 140 steps into a searhc with 20 steps, and if
you take an amortized cost it's almost 140 -> 20 for the
whole execution of VLC.

I agree that we can benefit from whatever optimizes the code
but there's few optimization that has no cost. If the gain is
like 10% gain for a filter on images, it's probably worth it
to maintain an additional assembly code dedicated by platform.
But if it's 120 lookups on the whole program execution, I'm
not sure I really see the point.

I hope my opinion is clearer,

Regards,
--
Alexandre Janniaux
Videolabs

> On Thu, 2020-09-03 at 18:47 +0200, Alexandre Janniaux wrote:
> > In the current tree, "opengl interop" are not provided by the
> > core but by modules. In ML and private branch, we also have
> > "opengl filter" which doesn't exist in the core, or I also
> > had "wayland buffer backend" to extend the current wayland
> > module.
> >
> > In general, having string as module capabilities gives quite
> > some agility that I, personnally, would like to keep.
> >
> > In addition, it gives free debugging string for what is loaded
> > and what must be shown on the interface.
> >
> > I'd prefer that you create defines in the core that might be
> > used in modules instead for the catching mistake part.
> >
> > About efficiency, in which case is it a problem currently?
> > If there is, we could implement a quicker lookup with an
> > hash based function instead.
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel