| Message ID | 20170306194739.1870-1-mfcc64@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Mar 07, 2017 at 02:47:36AM +0700, Muhammad Faiz wrote: > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > 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.... [...]
On Tue, 7 Mar 2017 02:47:36 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > Signed-off-by: Muhammad Faiz <mfcc64@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? 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.
On Tue, Mar 7, 2017 at 1:44 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Tue, 7 Mar 2017 02:47:36 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> Signed-off-by: Muhammad Faiz <mfcc64@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? Does anyone know why avcodec_register() is > even still public API? The same affects some other things. I don't know about these. > > 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. Ok, thank's for the suggestion
On Tue, Mar 07, 2017 at 07:44:43AM +0100, wm4 wrote: > On Tue, 7 Mar 2017 02:47:36 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > > > Signed-off-by: Muhammad Faiz <mfcc64@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 > 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 [...]
On 3/7/17, Michael Niedermayer <michael@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@gmail.com> wrote: >> >> > Signed-off-by: Muhammad Faiz <mfcc64@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 > > >> 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.
On Tue, 7 Mar 2017 14:37:27 +0100 Michael Niedermayer <michael@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@gmail.com> wrote: > > > > > Signed-off-by: Muhammad Faiz <mfcc64@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.
On Tue, Mar 07, 2017 at 03:04:14PM +0100, wm4 wrote: > On Tue, 7 Mar 2017 14:37:27 +0100 > Michael Niedermayer <michael@niedermayer.cc> 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. [...]
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); }
Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> --- libavcodec/allcodecs.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)