[FFmpeg-devel] [PATCH 1/4] lavc/parser: use C11 atomics

[Hendrik Leppkes]

On Thu, Nov 30, 2017 at 3:29 PM, Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> On Thu, Nov 30, 2017 at 3:17 PM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>> On Thu, Nov 30, 2017 at 11:55:03AM +0000, Rostislav Pehlivanov wrote:
>>> On 28 November 2017 at 01:26, Michael Niedermayer <michael at niedermayer.cc>
>>> wrote:
>>>
>>> > On Mon, Nov 27, 2017 at 04:30:18AM +0000, Rostislav Pehlivanov wrote:
>>> > > Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
>>> > > ---
>>> > >  libavcodec/parser.c | 14 ++++++++------
>>> > >  1 file changed, 8 insertions(+), 6 deletions(-)
>>> > >
>>> > > diff --git a/libavcodec/parser.c b/libavcodec/parser.c
>>> > > index 670680ea7c..baf1de4d88 100644
>>> > > --- a/libavcodec/parser.c
>>> > > +++ b/libavcodec/parser.c
>>> > > @@ -23,30 +23,32 @@
>>> > >  #include <inttypes.h>
>>> > >  #include <stdint.h>
>>> > >  #include <string.h>
>>> > > +#include <stdatomic.h>
>>> > >
>>> > >  #include "libavutil/avassert.h"
>>> > > -#include "libavutil/atomic.h"
>>> > >  #include "libavutil/internal.h"
>>> > >  #include "libavutil/mem.h"
>>> > >
>>> > >  #include "internal.h"
>>> > >  #include "parser.h"
>>> > >
>>> > > -static AVCodecParser *av_first_parser = NULL;
>>> > > +static _Atomic(AVCodecParser *)av_first_parser = NULL;
>>> >
>>> > This doesnt build here
>>> >
>>> > libavcodec/parser.c:35:8: warning: return type defaults to ‘int’ [enabled
>>> > by default]
>>> >  static _Atomic(AVCodecParser *)av_first_parser = NULL;
>>> >         ^
>>> > libavcodec/parser.c: In function ‘_Atomic’:
>>> > libavcodec/parser.c:35:32: error: expected declaration specifiers before
>>> > ‘av_first_parser’
>>> >  static _Atomic(AVCodecParser *)av_first_parser = NULL;
>>> >                                 ^
>>> > libavcodec/parser.c:38:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
>>> > ‘__attribute__’ before ‘{’ token
>>> >  {
>>> >  ^
>>> > libavcodec/parser.c:46:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
>>> > ‘__attribute__’ before ‘{’ token
>>> >  {
>>> >  ^
>>> > libavcodec/parser.c:55:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__a
>>> >
>>> >
>>> > [...]
>>> > --
>>> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>> >
>>> > Observe your enemies, for they first find out your faults. -- Antisthenes
>>> >
>>> > _______________________________________________
>>> > ffmpeg-devel mailing list
>>> > ffmpeg-devel at ffmpeg.org
>>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>> >
>>> >
>>>
>>> Does the attached patch work?
>>
>>>  parser.c |   14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>> 71f66a37fb46e29626f27da4658f34927e48033c  v2-0001-lavc-parser-use-C11-atomics.patch
>>> From b9ed59e9aff4a0fcc40f444f307c87acad55e139 Mon Sep 17 00:00:00 2001
>>> From: Rostislav Pehlivanov <atomnuker at gmail.com>
>>> Date: Mon, 27 Nov 2017 01:56:41 +0000
>>> Subject: [PATCH v2 1/2] lavc/parser: use C11 atomics
>>>
>>> Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
>>> ---
>>>  libavcodec/parser.c | 14 ++++++++------
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavcodec/parser.c b/libavcodec/parser.c
>>> index 670680ea7c..d6df62bf70 100644
>>> --- a/libavcodec/parser.c
>>> +++ b/libavcodec/parser.c
>>> @@ -23,30 +23,32 @@
>>>  #include <inttypes.h>
>>>  #include <stdint.h>
>>>  #include <string.h>
>>> +#include <stdatomic.h>
>>>
>>>  #include "libavutil/avassert.h"
>>> -#include "libavutil/atomic.h"
>>>  #include "libavutil/internal.h"
>>>  #include "libavutil/mem.h"
>>>
>>>  #include "internal.h"
>>>  #include "parser.h"
>>>
>>> -static AVCodecParser *av_first_parser = NULL;
>>> +static _Atomic(AVCodecParser *) av_first_parser = NULL;
>>
>> no
>>
>>
>> libavcodec/parser.c:35:8: warning: return type defaults to ‘int’ [enabled by default]
>>  static _Atomic(AVCodecParser *) av_first_parser = NULL;
>>         ^
>> libavcodec/parser.c: In function ‘_Atomic’:
>> libavcodec/parser.c:35:33: error: expected declaration specifiers before ‘av_first_parser’
>>  static _Atomic(AVCodecParser *) av_first_parser = NULL;
>>
>> also see
>> compat/atomics/gcc/stdatomic.h
>>
>> i think what you do is not supportd in that
>>
>>
>
> _Atomic is a keywork like volatile, so its syntax should be the same.
> Perhaps those emulations should just define it to "volatile" if its
> missing - the win32 emulation certainly would expect it to be
> volatile, and I would expect this to be true for the others as well.
>

On that note, it seems a bit odd that the emulations don't typedef the
atomic types to be volatile. We replace "volatile int" with
"atomic_int" for example, and with win32 or gcc emulation for
stdatomic.h, the volatile is basically just being dropped.
Sounds like that might be missing?

- Hendrik