[FFmpeg-devel] [PATCH 2/4] lavfi/avfilter: simplify filter registration

Michael Niedermayer michael at niedermayer.cc
Thu Nov 30 16:01:10 EET 2017


On Thu, Nov 30, 2017 at 11:54:25AM +0000, Rostislav Pehlivanov wrote:
> On 29 November 2017 at 16:40, Michael Niedermayer <michael at 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 <michael at 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 <atomnuker at 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 at 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 at 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 <atomnuker at 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 <atomnuker at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171130/0a76902c/attachment.sig>


More information about the ffmpeg-devel mailing list