[FFmpeg-devel] [PATCH] libavfilter: constify filter list

[wm4]

On Thu, 1 Feb 2018 02:06:52 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Wed, Jan 31, 2018 at 12:24:43PM +0100, wm4 wrote:
> > On Wed, 31 Jan 2018 12:11:25 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Wed, Jan 31, 2018 at 10:03:58AM +0100, wm4 wrote:  
> > > > On Wed, 31 Jan 2018 09:08:23 +0100
> > > > Tobias Rapp <t.rapp at noa-archive.com> wrote:
> > > >     
> > > > > On 30.01.2018 20:37, Michael Niedermayer wrote:    
> > > > > > On Tue, Jan 30, 2018 at 08:27:27PM +0700, Muhammad Faiz wrote:      
> > > > > >> On Tue, Jan 30, 2018 at 7:50 PM, Michael Niedermayer
> > > > > >> <michael at niedermayer.cc> wrote:      
> > > > > >>> Its also a step away from supporting plugins.
> > > > > >>> Why plugins matter ? Because having plugin
> > > > > >>> support is a big advantage, it allows a much wider
> > > > > >>> community to work on, write and maintain filters.
> > > > > >>>
> > > > > >>> With plugins, people can write filters that are
> > > > > >>> written in languages other than C. Or filters which
> > > > > >>> some developer in FFmpeg doesnt want. Or they can be
> > > > > >>> maintained externally by people who just do not like us.
> > > > > >>> Or by people who perfer a FOSS license different from
> > > > > >>> LGPL/GPL/BSD. Iam sure others can come up with more
> > > > > >>> reasons ...
> > > > > >>>
> > > > > >>> Of course avfilter_register() isnt enough for plugins
> > > > > >>> but it or something equivalent is needed for plugins.
> > > > > >>>
> > > > > >>> So i would prefer if avfilter_register() stays supported
> > > > > >>> indefinitly or in case a different system is written for
> > > > > >>> plugins then until that system is in place.      
> > > > > >>
> > > > > >> It just returns error and logs error message that currently external
> > > > > >> filter is unsupported. If someone wants to implement support for
> > > > > >> external filter, it can be easily added later using separate
> > > > > >> list/table.      
> > > > > > 
> > > > > > Iam not sure "easily" is true
> > > > > > 
> > > > > > We started out with a fully public API that allowed external filters.
> > > > > > and little by little its removed or made private.
> > > > > > each individual change may be easy to undo but as a whole moving
> > > > > > libavfilter back to having a public API is not that easy anymore
> > > > > > 
> > > > > > IIRC the arguments to make things private where that people wanted to
> > > > > > improve the API. But from the idea and impression of a plan like:
> > > > > > 1. make it private
> > > > > > 2. design and implement better API
> > > > > > 3. make it public again
> > > > > > 
> > > > > > Somehow now people lost sight and interrest in 3. and even 2. is becoming
> > > > > > less interresting. But the problem is really without 2 + 3 actually happening
> > > > > > doing 1 seems like not a good idea at all.
> > > > > > 
> > > > > > To me it seems even mentioning external filters and public API makes some
> > > > > > people angry.
> > > > > > But really that has to be the goal at some point. To again have a public API      
> > > > > 
> > > > > I agree that having (again) a public filter API would be good. This 
> > > > > would give users the freedom to implement their own special-purpose 
> > > > > filters (see the "dumpwave" discussion).    
> > > > 
> > > > Someone needs to design and write it.
> > > > 
> > > > Whether we have external filters is completely orthogonal to this
> > > > change.     
> > >   
> > > > They were not possible before, because not enough API for
> > > > implementing them is public.     
> > > 
> > > This is correct if by "before" you mean today before this is applied.
> > > But longer ago, as in years, it was possible
> > > 
> > >   
> > > > They can be possible in the future, but
> > > > then registering them would need a different API anyway (one that
> > > > doesn't use global mutable state, but uses some sort of context
> > > > instead).    
> > > 
> > > i understand that you arent a native english speaker nor am i but
> > > what you write here is not true.
> > > 
> > > Several people desire to eliminate all "global mutable state"
> > > and thats fine, iam happy if that is achieved. But its not
> > > that its neccessary for a fully functional API register or otherwise.
> > > The registering code with "global mutable state" and some form
> > > of thread synchronization would work perfectly fine.
> > > 
> > > What exactly is in relation to registering the problem with 
> > > "global mutable state" ?
> > > 
> > > One application registering a filter with a common name and another 
> > > application unintentionally using that ?  
> > 
> > Yes. It's not library-safe.
> >   
> > > This is not even possible because each application is in its own process and
> > > will have its own independant copy of libavfilter in its address space only
> > > with "read only" pages shared or with pages with "copy on write".  
> > 
> > This is a library. There can be multiple users of a library in the same
> > process.
> >   
> > > The "one registers the other uses it by mistake" issue is as far as i
> > > understand only possible if an application uses libavfilter and that
> > > application uses also a lib that itself uses libavfilter too or
> > > similar cases. 
> > > And then both mess with registering custom filters and using custom
> > > filters.
> > > That looks like a very rare situation. If thats the only issue, i would
> > > really not say theres a "need" to avoid "global mutable state"  
> > 
> > I don't agree. It will cause problems you're not even thinking about
> > now. Look at any library that thought global mutable state is fine for
> > some thing, and what problems it caused. Besides someone will have the
> > idea to create some sort of plugin loader, and then all bets are off.  
> 
> to awnser just
>  "look at any library that thought global mutable state is fine for some thing,"
> Well we can look at libavcodec and libavformat and libavfilters register
> code. That existed since a long time and used global mutable state.
> searching trac for 
> "_register(" -> 1 match thats about the lock manager not the codec/format register
> "av_register_output_format" -> no matches
> "av_register_input_format" -> no matches
> 
> So as you requested, i looked at not just any library but at multiple
> and at exactly the very identical case of registering.
> Zero issues have occured there.
> 
> People hate "global mutable state" and i do not like it either but the
> thing that its causing problems in this case is exagerated.

The problem so far has been easier, because you could NOT register user
defined codecs/filters. So a whole class of problems falls away. You
added all the atomics code to hack it into somewhat working yourself,
so I'm not sure why you deny that it had problems.

It's true that with proper synchronization, the current state is mostly
just an inconvenience of having to call completely useless functions
(the register_all ones), and more memory usage due to the register
funtions writing to data pages (i.e. it will make the process use
more memory even if the libs are not actually used).

> 
> > 
> > Why are you even stirring up an offtopic discussion again.   
> 
> How can discussing the removal and its reasons of a public API
> be off topic in a thread of a patch removing that API !?

For the hundredth time, having user filters has nothing to do with
having a global register function, and especially has nothing to do
with having a register_all function.

> 
> > I've also
> > mentioned multiple times how the main problem is creating public API
> > so filters can be implemented. This is not a trivial task, but adding
> > some sort of filter register API is absolutely trivial (even if it
> > readds the current API, which I hope it won't). So why bring it up
> > again and cause tons of fruitless discussions? Just why? Please explain
> > this.  
> 
> Yes, iam concerned and upset about the lack of activity on what you
> call the "main problem".
> If there was someone working on creating a new public API, then i would
> never complain about him removing existing API.
> "remove that code so we can put a better one in place" vs. "remove this,
> its trivial to put it or something back but that will never happen"
> Its the lack of a plan, the lack of intend to walk forward which makes me
> raise the issue of people taking a step backward.

For the hundredth time, this patch does not remove any API that would
actually enable you to have public API.

In fact, you probably merged the patch that deprecated/removed the
public filter API _yourself_.

I'm still expecting a proper explanation from you about your
misleading posts.