On Thu, Nov 30, 2017 at 11:54:25AM +0000, Rostislav Pehlivanov wrote: > On 29 November 2017 at 16:40, Michael Niedermayer <mich...@niedermayer.cc> > wrote: > > > On Wed, Nov 29, 2017 at 03:51:34AM +0000, Rostislav Pehlivanov wrote: > > > On 28 November 2017 at 20:23, Michael Niedermayer <mich...@niedermayer.cc > > > > > > wrote: > > > > > > > On Mon, Nov 27, 2017 at 04:30:19AM +0000, Rostislav Pehlivanov wrote: > > > > > Atomics were entirely pointless and did nothing but slow and > > complicate > > > > > the process down. This could be improved further still but the main > > > > > objective of this commit is to simplify. > > > > > > > > > > Signed-off-by: Rostislav Pehlivanov <atomnu...@gmail.com> > > > > > --- > > > > > libavfilter/avfilter.c | 8 +++++--- > > > > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > > > > > *_register() can be called by the user app in a unsyncronized way > > > > for example from a module like vlc used AFAIK. > > > > or from libs loaded by an application which use libavfilter or other > > > > internally. > > > > > > > > These registration functions should stay thread safe > > > > > > > > > > > > [...] > > > > > > > > -- > > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7 > > 87040B0FAB > > > > > > > > Those who would give up essential Liberty, to purchase a little > > > > temporary Safety, deserve neither Liberty nor Safety -- Benjamin > > Franklin > > > > > > > > _______________________________________________ > > > > ffmpeg-devel mailing list > > > > ffmpeg-devel@ffmpeg.org > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > > > What variable actually needs to be protected? > > > > The linked list of registered "entities" > > That is if the addition is not synchronized by some means, an atomic > > exchange a mutex or something then a race can generally occur > > and only one addition might succeed or something else might get > > entangled > > > > This is in practical terms a very unlikely event to occur of course > > > > [...] > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > You can kill me, but you cannot change the truth. > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > Could you test the attached patch? Pretty much everything is synchronized.
> avfilter.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
> 8ab986fa3b0f735e025e5c0a3235ca4c7e227f1b
> v2-0002-lavfi-avfilter-simplify-filter-registration.patch
> From fb9789206c76c876d6a79e81df68c6b8041f03c3 Mon Sep 17 00:00:00 2001
> From: Rostislav Pehlivanov <atomnu...@gmail.com>
> Date: Mon, 27 Nov 2017 04:12:26 +0000
> Subject: [PATCH v2 2/2] lavfi/avfilter: simplify filter registration
>
> Atomics were entirely pointless and did nothing but slow and complicate
> the process down. This could be improved further still but the main
> objective of this commit is to simplify.
>
> Signed-off-by: Rostislav Pehlivanov <atomnu...@gmail.com>
> ---
> libavfilter/avfilter.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index b98b32bacb..76c9f12be9 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -19,7 +19,6 @@
> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> */
>
> -#include "libavutil/atomic.h"
> #include "libavutil/avassert.h"
> #include "libavutil/avstring.h"
> #include "libavutil/buffer.h"
> @@ -37,6 +36,7 @@
> #define FF_INTERNAL_FIELDS 1
> #include "framequeue.h"
>
> +#include <stdatomic.h>
> #include "audio.h"
> #include "avfilter.h"
> #include "filters.h"
> @@ -574,7 +574,7 @@ int avfilter_process_command(AVFilterContext *filter,
> const char *cmd, const cha
> }
>
> static AVFilter *first_filter;
> -static AVFilter **last_filter = &first_filter;
> +static _Atomic (AVFilter **) last_filter = ATOMIC_VAR_INIT(&first_filter);
>
> const AVFilter *avfilter_get_by_name(const char *name)
> {
> @@ -592,16 +592,16 @@ const AVFilter *avfilter_get_by_name(const char *name)
>
> int avfilter_register(AVFilter *filter)
> {
> /* the filter must select generic or internal exclusively */
> av_assert0((filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE) !=
> AVFILTER_FLAG_SUPPORT_TIMELINE);
>
> filter->next = NULL;
>
> + /* Iterate through the list until the last entry has been reached */
> + do {
> + *atomic_load(&last_filter) = filter;
> + atomic_store(&last_filter, &filter->next);
> + } while (*atomic_load(&last_filter));
assume 2 register run at the same time
both execute atomic_load(&last_filter)
and obtain a pointer to a pointer to first_filter, which they both
store their (different) filter arguments in (non atomically)
Thats a race & undefined behavior
and even if this store was atomic, the later would overwrite the
earlier store before the earlier code had finished the rest of its
steps
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
