[FFmpeg-devel] [PATCH 1/4] avcodec/allcodecs: make avcodec_register_all thread safe

wm4 nfxjfg at googlemail.com
Tue Mar 7 16:04:14 EET 2017


On Tue, 7 Mar 2017 14:37:27 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Tue, Mar 07, 2017 at 07:44:43AM +0100, wm4 wrote:
> > On Tue,  7 Mar 2017 02:47:36 +0700
> > Muhammad Faiz <mfcc64 at gmail.com> wrote:
> >   
> > > Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
> > > ---
> > >  libavcodec/allcodecs.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> > > index eee322b..1d05fc1 100644
> > > --- a/libavcodec/allcodecs.c
> > > +++ b/libavcodec/allcodecs.c
> > > @@ -24,6 +24,8 @@
> > >   * Provide registration of all codecs, parsers and bitstream filters for libavcodec.
> > >   */
> > >  
> > > +#include <stdatomic.h>
> > > +
> > >  #include "config.h"
> > >  #include "avcodec.h"
> > >  #include "version.h"
> > > @@ -60,11 +62,14 @@
> > >  
> > >  void avcodec_register_all(void)
> > >  {
> > > -    static int initialized;
> > > +    static atomic_int initialized;
> > > +    static atomic_int ready;
> > >  
> > > -    if (initialized)
> > > +    if (atomic_exchange_explicit(&initialized, 1, memory_order_relaxed)) {
> > > +        while (!atomic_load_explicit(&ready, memory_order_relaxed))
> > > +            ;
> > >          return;
> > > -    initialized = 1;
> > > +    }
> > >  
> > >      /* hardware accelerators */
> > >      REGISTER_HWACCEL(H263_VAAPI,        h263_vaapi);
> > > @@ -716,4 +721,6 @@ void avcodec_register_all(void)
> > >      REGISTER_PARSER(VP8,                vp8);
> > >      REGISTER_PARSER(VP9,                vp9);
> > >      REGISTER_PARSER(XMA,                xma);
> > > +
> > > +    atomic_store_explicit(&ready, 1, memory_order_relaxed);
> > >  }  
> >   
> 
> > avcodec_register() is already "safe" by attempting to do lock-free and
> > wait-free list insertion (not very convincing IMHO, but it's there).
> > Should that code be removed?  
> 
> that code is needed, otherwise avcodec_register() would not be thread
> safe

Sure, also list access wouldn't be synchronized in the first place.

> 
> > Does anyone know why avcodec_register() is
> > even still public API? The same affects some other things.  
> 
> It is public so that user written codecs can be registered

That's not possible. It's all private API, multitudes of complex,
unexposable private API.

> Ideally we would have a real plugin system and for example rune could
> maintain his cinepack decoder as a plugin in his github repo.

He can do that anyway, as a patch?

> Then he would likely not have left FFmpeg today.

> Removing avcodec_register() IMO is a step in the wrong direction as
> after that theres no way to register user written codecs and then the
> API also would no longer need to be unchanged and once the API changes
> frequently adding a real plugin interface becomes MUCH harder.

Wrong. It most likely would exist in a completely different form, and
API to support plugin registration could be added as needed. The
current avcodec_register() call is completely misleading, and implies
codecs can be added from the outside. But in fact, merely using this
function is an API usage error. Why is it public? It's an oxymoron.

> The real problem behind all this is of course more complex and a
> combination of many of our developers being rather aggressive and
> having strong oppinions on code outside the area they actively maintain.

So devs who don't maintain a specific part of the code don't get to
have a "strong" opinion? I guess only a weak opinion is wanted, at most?

If you argue this way, we'd have to put maintainership of central code
under multiple developers or so.

> That drives some contributors away

Which ones? The Cinepak thing was clearly rejected, and the patch
author couldn't deal with it. If that's all it takes to run away,
he probably wasn't meant to contribute to a big project with many
developers anyway. (Funny how the Cinepak maintainer never even showed
up - tells a lot about our MAINTAINERS file?)

> and now the lack of a plugin
> interface means they are driven over the ledge and a lost as
> contributors to FFmpeg.

FFmpeg never had a plugin interface.

I'm not even sure why you combine multiple completely separate issues in
your argumentation.

> 
> I would very much like to see some effort to add a real plugin
> interface instead of the opposite direction

Go ahead and propose one? But if it forces too much private API to be
exposed, then it'd make FFmpeg development harder, because we'd have to
guarantee stability of those internal APIs until the next major bump,
and it'd be near-useless for outside developers because they had to fix
their plugins every major bump.

I think the thing that's wrong with FFmpeg API is that its APIs are not
designed for long-time API and ABI stability. That would be a good
place to start.

A realistic approach to supporting plugins would probably be to create
a separate API, which translates the required calls to the internal
API, but which is designed in a way that can be supported for a long
time. Rather low-level (and low-feature) examples would include the
ladspa and frei0r libavfilter wrappers.


More information about the ffmpeg-devel mailing list