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

2017-03-07 Thread Michael Niedermayer
On Tue, Mar 07, 2017 at 03:04:14PM +0100, wm4 wrote:
> On Tue, 7 Mar 2017 14:37:27 +0100
> Michael Niedermayer  wrote:
> 
> > On Tue, Mar 07, 2017 at 07:44:43AM +0100, wm4 wrote:

[...]

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

Whats wanted is, what works. And with an increasing number of
developers with strong and different oppinions come increasing
conflicts. A possible solution would be strict rules on who has authority
over which part of the code or people being alot more tolerant than
they are. The end result either way would be that code like the
cinepack optimzations would be accepted but thats exactly what many
people dont want. And thats where the dilemma is and i have no awnser
or solution here. What i think though is
When people leave then "what was done" didnt work.
We can try to learn out of this or we can continue as we did previously.
And it all probably boils down to seeing other peoples needs and
accepting them vs. accepting to loose some of them.
But quite possibly there are solutions i fail to see ...


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

well, yes, but is that surprising?
Volunteers work in the environment where they are welcome and where
their contributions are welcome. If one community is rejecting a
sgnificant part or the work one cares about, moving to a different
community, field or area is the obvous option


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

would it help if he did ?
I think he couldnt have done much had he jumped into the discussion



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

Personally i would prefer internal codecs and plugins to use the same
API, but thats a detail. I am happy with any plugin API that works
and that people are happy with.

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-03-07 Thread wm4
On Tue, 7 Mar 2017 14:37:27 +0100
Michael Niedermayer  wrote:

> On Tue, Mar 07, 2017 at 07:44:43AM +0100, wm4 wrote:
> > On Tue,  7 Mar 2017 02:47:36 +0700
> > Muhammad Faiz  wrote:
> >   
> > > Signed-off-by: Muhammad Faiz 
> > > ---
> > >  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 
> > > +
> > >  #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(, 1, memory_order_relaxed)) 
> > > {
> > > +while (!atomic_load_explicit(, 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(, 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 

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

2017-03-07 Thread Paul B Mahol
On 3/7/17, Michael Niedermayer  wrote:
> On Tue, Mar 07, 2017 at 07:44:43AM +0100, wm4 wrote:
>> On Tue,  7 Mar 2017 02:47:36 +0700
>> Muhammad Faiz  wrote:
>>
>> > Signed-off-by: Muhammad Faiz 
>> > ---
>> >  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 
>> > +
>> >  #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(, 1,
>> > memory_order_relaxed)) {
>> > +while (!atomic_load_explicit(, 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(, 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
>
>
>> 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
>
> Ideally we would have a real plugin system and for example rune could
> maintain his cinepack decoder as a plugin in his github repo.
> 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.
>
> 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.
> That drives some contributors away and now the lack of a plugin
> interface means they are driven over the ledge and a lost as
> contributors to FFmpeg.
>
> I would very much like to see some effort to add a real plugin
> interface instead of the opposite direction

In that case I will fork FFmpeg as AGPL only.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-03-07 Thread Michael Niedermayer
On Tue, Mar 07, 2017 at 07:44:43AM +0100, wm4 wrote:
> On Tue,  7 Mar 2017 02:47:36 +0700
> Muhammad Faiz  wrote:
> 
> > Signed-off-by: Muhammad Faiz 
> > ---
> >  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 
> > +
> >  #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(, 1, memory_order_relaxed)) {
> > +while (!atomic_load_explicit(, 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(, 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


> 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

Ideally we would have a real plugin system and for example rune could
maintain his cinepack decoder as a plugin in his github repo.
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.

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.
That drives some contributors away and now the lack of a plugin
interface means they are driven over the ledge and a lost as
contributors to FFmpeg.

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


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-03-06 Thread wm4
On Tue,  7 Mar 2017 02:47:36 +0700
Muhammad Faiz  wrote:

> Signed-off-by: Muhammad Faiz 
> ---
>  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 
> +
>  #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(, 1, memory_order_relaxed)) {
> +while (!atomic_load_explicit(, 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(, 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? Does anyone know why avcodec_register() is
even still public API? The same affects some other things.

Also, if you want to do it right, please use ff_thread_once() to do it
without busy waiting. It works like pthread_once(), which was invented
to avoid such constructions.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-03-06 Thread James Almer
On 3/6/2017 11:55 PM, Michael Niedermayer wrote:
> On Tue, Mar 07, 2017 at 02:47:36AM +0700, Muhammad Faiz wrote:
>> Signed-off-by: Muhammad Faiz 
>> ---
>>  libavcodec/allcodecs.c | 13 ++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> fails to build
> 
> ibavcodec/allcodecs.c: In function ‘avcodec_register_all’:
> libavcodec/allcodecs.c:68:5: error: implicit declaration of function ‘typeof’ 
> [-Werror=implicit-function-declaration]
> libavcodec/allcodecs.c:68:33: error: expected ‘;’ before ‘_obj’
> libavcodec/allcodecs.c:68:78: error: expected ‘;’ before ‘_old’
> libavcodec/allcodecs.c:68:87: error: ‘_old’ undeclared (first use in this 
> function)
> libavcodec/allcodecs.c:68:87: note: each undeclared identifier is reported 
> only once for each function it appears in
> libavcodec/allcodecs.c:68:119: error: ‘_obj’ undeclared (first use in this 
> function)
> libavcodec/allcodecs.c:68:78: error: incompatible type for argument 1 of 
> ‘__sync_bool_compare_and_swap’
> cc1: some warnings being treated as errors
> make: *** [libavcodec/allcodecs.o] Error 1
> make: *** Waiting for unfinished jobs

This is from the compat header for c11 atomics on old GCC versions.
Looks like it should be using __typeof__ instead.

Try the attached patch.

>From 5af305a9180b691577e48731d9ca84a3b2342404 Mon Sep 17 00:00:00 2001
From: James Almer 
Date: Tue, 7 Mar 2017 00:04:46 -0300
Subject: [PATCH] compat/atomics/gcc: use __typeof__ instead of typeof

Signed-off-by: James Almer 
---
 compat/atomics/gcc/stdatomic.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/compat/atomics/gcc/stdatomic.h b/compat/atomics/gcc/stdatomic.h
index 41caddec5c..2b64687437 100644
--- a/compat/atomics/gcc/stdatomic.h
+++ b/compat/atomics/gcc/stdatomic.h
@@ -100,8 +100,8 @@ do {\
 
 #define atomic_exchange(object, desired)\
 ({  \
-typeof(object) _obj = (object); \
-typeof(*object) _old;   \
+__typeof__(object) _obj = (object); \
+__typeof__(*object) _old;   \
 do  \
 _old = atomic_load(_obj);   \
 while (!__sync_bool_compare_and_swap(_obj, _old, (desired)));   \
@@ -113,8 +113,8 @@ do {\
 
 #define atomic_compare_exchange_strong(object, expected, desired)   \
 ({  \
-typeof(object) _exp = (expected);   \
-typeof(*object) _old = *_exp;   \
+__typeof__(object) _exp = (expected);   \
+__typeof__(*object) _old = *_exp;   \
 *_exp = __sync_val_compare_and_swap((object), _old, (desired)); \
 *_exp == _old;  \
 })
-- 
2.12.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-03-06 Thread Michael Niedermayer
On Tue, Mar 07, 2017 at 02:47:36AM +0700, Muhammad Faiz wrote:
> Signed-off-by: Muhammad Faiz 
> ---
>  libavcodec/allcodecs.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

fails to build

ibavcodec/allcodecs.c: In function ‘avcodec_register_all’:
libavcodec/allcodecs.c:68:5: error: implicit declaration of function ‘typeof’ 
[-Werror=implicit-function-declaration]
libavcodec/allcodecs.c:68:33: error: expected ‘;’ before ‘_obj’
libavcodec/allcodecs.c:68:78: error: expected ‘;’ before ‘_old’
libavcodec/allcodecs.c:68:87: error: ‘_old’ undeclared (first use in this 
function)
libavcodec/allcodecs.c:68:87: note: each undeclared identifier is reported only 
once for each function it appears in
libavcodec/allcodecs.c:68:119: error: ‘_obj’ undeclared (first use in this 
function)
libavcodec/allcodecs.c:68:78: error: incompatible type for argument 1 of 
‘__sync_bool_compare_and_swap’
cc1: some warnings being treated as errors
make: *** [libavcodec/allcodecs.o] Error 1
make: *** Waiting for unfinished jobs


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


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

2017-03-06 Thread Muhammad Faiz
Signed-off-by: Muhammad Faiz 
---
 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 
+
 #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(, 1, memory_order_relaxed)) {
+while (!atomic_load_explicit(, 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(, 1, memory_order_relaxed);
 }
-- 
2.9.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel