[FFmpeg-devel] fix MSVC compilation errors

After some tests:
1) #undef far
after #include <windows.h> is wrong -- in oleauto.h is declaration
WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
and 'FAR' is defined as 'far' which is define as empty.

2) #undef near
after #include <windows.h> works in ffmpeg but is danger -- see 1)

3) after
git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
git revert 590136e78da3d091ea99ab5432543d47a559a461
and patch

Mateusz



W dniu 04.12.2017 o 20:27, Mateusz pisze:
> W dniu 04.12.2017 o 15:02, Derek Buitenhuis pisze:
>> On 12/4/2017 8:03 AM, Mateusz wrote:
>>> After commit 3701d49 'error_resilience: remove avpriv_atomic usage'
>>> we have included windows.h in much more files and we should
>>> avoid conflicts with defines/function declarations.
>>>
>>> Signed-off-by: Mateusz Brzostek <mateuszb@poczta.onet.pl>
>>> ---
>>>  libavcodec/jpegls.h  | 4 ++++
>>>  libavcodec/mss2.c    | 6 +++---
>>>  libavformat/mxfenc.c | 2 +-
>>>  3 files changed, 8 insertions(+), 4 deletions(-)
>>
>> Sprinkling these weird ifdefs and renames around is pretty ugly. Is there
>> some sort of canonical list on MSDN or something we can use globally-ish?
>>
>> - Derek
> 
> There is a list of "Predefined Macros" in MSVC -- IMO there are OK
> https://msdn.microsoft.com/en-us/library/b0084kay(v=vs.140).aspx
> 
> More danger are macros from windows.h -- there is a list of macros
> to exclude some parts (from MSDN and windows.h):
> Define WIN32_LEAN_AND_MEAN to exclude APIs such as
> Cryptography, DDE, RPC, Shell, and Windows Sockets.
> 
> /*  If defined, the following flags inhibit definition
>  *     of the indicated items.
>  *
>  *  NOGDICAPMASKS     - CC_*, LC_*, PC_*, CP_*, TC_*, RC_
>  *  NOVIRTUALKEYCODES - VK_*
>  *  NOWINMESSAGES     - WM_*, EM_*, LB_*, CB_*
>  *  NOWINSTYLES       - WS_*, CS_*, ES_*, LBS_*, SBS_*, CBS_*
>  *  NOSYSMETRICS      - SM_*
>  *  NOMENUS           - MF_*
>  *  NOICONS           - IDI_*
>  *  NOKEYSTATES       - MK_*
>  *  NOSYSCOMMANDS     - SC_*
>  *  NORASTEROPS       - Binary and Tertiary raster ops
>  *  NOSHOWWINDOW      - SW_*
>  *  OEMRESOURCE       - OEM Resource values
>  *  NOATOM            - Atom Manager routines
>  *  NOCLIPBOARD       - Clipboard routines
>  *  NOCOLOR           - Screen colors
>  *  NOCTLMGR          - Control and Dialog routines
>  *  NODRAWTEXT        - DrawText() and DT_*
>  *  NOGDI             - All GDI defines and routines
>  *  NOKERNEL          - All KERNEL defines and routines
>  *  NOUSER            - All USER defines and routines
>  *  NONLS             - All NLS defines and routines
>  *  NOMB              - MB_* and MessageBox()
>  *  NOMEMMGR          - GMEM_*, LMEM_*, GHND, LHND, associated routines
>  *  NOMETAFILE        - typedef METAFILEPICT
>  *  NOMINMAX          - Macros min(a,b) and max(a,b)
>  *  NOMSG             - typedef MSG and associated routines
>  *  NOOPENFILE        - OpenFile(), OemToAnsi, AnsiToOem, and OF_*
>  *  NOSCROLL          - SB_* and scrolling routines
>  *  NOSERVICE         - All Service Controller routines, SERVICE_ equates, etc.
>  *  NOSOUND           - Sound driver routines
>  *  NOTEXTMETRIC      - typedef TEXTMETRIC and associated routines
>  *  NOWH              - SetWindowsHook and WH_*
>  *  NOWINOFFSETS      - GWL_*, GCL_*, associated routines
>  *  NOCOMM            - COMM driver routines
>  *  NOKANJI           - Kanji support stuff.
>  *  NOHELP            - Help engine interface.
>  *  NOPROFILER        - Profiler interface.
>  *  NODEFERWINDOWPOS  - DeferWindowPos routines
>  *  NOMCX             - Modem Configuration Extensions
>  */
> 
> The most danger are small caps macros defined as empty in minwindef.h:
> far
> near
> pascal
> cdecl
> 
> I think it is possible to make in ffmpeg's *.h files which include windows.h
> something like this:
> #define WIN32_LEAN_AND_MEAN
> #define NOMINMAX
> #define NOGDI
> #include <windows.h>
> #undef far
> #undef near
> #undef pascal
> #undef cdecl
> 
> I will make some test and write back.
> 
> Mateusz

On Tue, Dec 5, 2017 at 12:31 AM, Mateusz <mateuszb@poczta.onet.pl> wrote:
> After some tests:
> 1) #undef far
> after #include <windows.h> is wrong -- in oleauto.h is declaration
> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
> and 'FAR' is defined as 'far' which is define as empty.

Yeah generally undefing all of them might result in errors in other
windows headers, so thats bad.

>
> 2) #undef near
> after #include <windows.h> works in ffmpeg but is danger -- see 1)

Doing it rather late,  ie. right before its used in the file should be
relatively safe. If not, we can always rename the variable.
But it seems to work so far. And from the nature of this bug, it
should just error out instead of resulting in other breakage - if it
happens.

>
> 3) after
> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
> git revert 590136e78da3d091ea99ab5432543d47a559a461
> and patch
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> -    MXFPackage packages[2] = {};
> +    MXFPackage packages[2] = {{NULL}};
> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
> (without reverting 590136e hangs at api-flac-test.exe)
>
> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
> we can apply this patch and

We can't really revert those, so fixing them is better.

> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index baf09119fe..b34a3803b8 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>
>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>  {
> -    _Bool exp = 0;
> +    atomic_bool exp = 0;
>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>          return 0;
>
> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>
>  int ff_unlock_avcodec(const AVCodec *codec)
>  {
> -    _Bool exp = 1;
> +    atomic_bool exp = 1;
>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>          return 0;
>
>

atomic_compare_exchange* usage is a bit iffy because it uses pointers,
and the compat wrappers don't fully represent all types properly (for
simplicity, implementing them in a generic way would be madness).
So this work-around seems fine to me, if it builds with GCC as well.

- Hendrik

On 12/4/2017 8:31 PM, Mateusz wrote:
> After some tests:
> 1) #undef far
> after #include <windows.h> is wrong -- in oleauto.h is declaration
> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
> and 'FAR' is defined as 'far' which is define as empty.
> 
> 2) #undef near
> after #include <windows.h> works in ffmpeg but is danger -- see 1)
> 
> 3) after
> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
> git revert 590136e78da3d091ea99ab5432543d47a559a461
> and patch
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> -    MXFPackage packages[2] = {};
> +    MXFPackage packages[2] = {{NULL}};
> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
> (without reverting 590136e hangs at api-flac-test.exe)
> 
> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
> we can apply this patch and
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index baf09119fe..b34a3803b8 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
> 
>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>  {
> -    _Bool exp = 0;
> +    atomic_bool exp = 0;
>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>          return 0;
> 
> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
> 
>  int ff_unlock_avcodec(const AVCodec *codec)
>  {
> -    _Bool exp = 1;
> +    atomic_bool exp = 1;

No, these are not meant to be atomic.

_Bool is c99, so maybe we could replace its usage with int in these lock
functions. The result will be the exact same.

>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>          return 0;
> 
> 
> Mateusz

On Tue, Dec 5, 2017 at 3:20 PM, James Almer <jamrial@gmail.com> wrote:
> On 12/4/2017 8:31 PM, Mateusz wrote:
>> After some tests:
>> 1) #undef far
>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>> and 'FAR' is defined as 'far' which is define as empty.
>>
>> 2) #undef near
>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
>>
>> 3) after
>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>> and patch
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> -    MXFPackage packages[2] = {};
>> +    MXFPackage packages[2] = {{NULL}};
>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>> (without reverting 590136e hangs at api-flac-test.exe)
>>
>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>> we can apply this patch and
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index baf09119fe..b34a3803b8 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>
>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>  {
>> -    _Bool exp = 0;
>> +    atomic_bool exp = 0;
>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>          return 0;
>>
>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>
>>  int ff_unlock_avcodec(const AVCodec *codec)
>>  {
>> -    _Bool exp = 1;
>> +    atomic_bool exp = 1;
>
> No, these are not meant to be atomic.
>

int will also not work, since you would pass a pointer to an int to
something that expects a pointer to intptr_t.

- Hendrik

On 12/5/2017 1:13 PM, Hendrik Leppkes wrote:
> On Tue, Dec 5, 2017 at 3:20 PM, James Almer <jamrial@gmail.com> wrote:
>> On 12/4/2017 8:31 PM, Mateusz wrote:
>>> After some tests:
>>> 1) #undef far
>>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>>> and 'FAR' is defined as 'far' which is define as empty.
>>>
>>> 2) #undef near
>>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
>>>
>>> 3) after
>>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>>> and patch
>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>> -    MXFPackage packages[2] = {};
>>> +    MXFPackage packages[2] = {{NULL}};
>>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>>> (without reverting 590136e hangs at api-flac-test.exe)
>>>
>>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>>> we can apply this patch and
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index baf09119fe..b34a3803b8 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>>
>>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>  {
>>> -    _Bool exp = 0;
>>> +    atomic_bool exp = 0;
>>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>>          return 0;
>>>
>>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>
>>>  int ff_unlock_avcodec(const AVCodec *codec)
>>>  {
>>> -    _Bool exp = 1;
>>> +    atomic_bool exp = 1;
>>
>> No, these are not meant to be atomic.
>>
> 
> int will also not work, since you would pass a pointer to an int to
> something that expects a pointer to intptr_t.
> 
> - Hendrik

So it's not a problem of _Bool being undefined in msvc? Sounds like the
win32 stdatomic.h wrapper are not correct, then. This was not an issue
with the old lavu wrapper.

Why does it use intptr_t at all for that matter? Why not the types the
gcc wrapper uses?

W dniu 05.12.2017 o 15:20, James Almer pisze:
> On 12/4/2017 8:31 PM, Mateusz wrote:
>> After some tests:
>> 1) #undef far
>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>> and 'FAR' is defined as 'far' which is define as empty.
>>
>> 2) #undef near
>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
>>
>> 3) after
>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>> and patch
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> -    MXFPackage packages[2] = {};
>> +    MXFPackage packages[2] = {{NULL}};
>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>> (without reverting 590136e hangs at api-flac-test.exe)
>>
>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>> we can apply this patch and
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index baf09119fe..b34a3803b8 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>
>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>  {
>> -    _Bool exp = 0;
>> +    atomic_bool exp = 0;
>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>          return 0;
>>
>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>
>>  int ff_unlock_avcodec(const AVCodec *codec)
>>  {
>> -    _Bool exp = 1;
>> +    atomic_bool exp = 1;
> 
> No, these are not meant to be atomic.
> 
> _Bool is c99, so maybe we could replace its usage with int in these lock
> functions. The result will be the exact same.

In atomic types the main thing is width (sizeof).
In atomic operations destination memory MUST BE sufficient
to copy all bytes from source, in
atomic_compare_exchange_strong(&ff_avcodec_locked, &exp, 1)
sizeof(exp) MUST BE >= sizeof(ff_avcodec_locked) == sizeof(atomic_bool)

In ffmpeg implementation of atomic code for MSVC sizeof(atomic_bool) == 8
and sizeof(_Bool) < 8 -- it is the reason of hangs.

And all volatile objects MUST BE explicitly volatile -- instead of
static atomic_bool ff_avcodec_locked;
should be
static volatile atomic_bool ff_avcodec_locked;

I will prepare a patch (but not before Friday -- lots of work).

Mateusz

On 5 December 2017 at 16:40, Mateusz <mateuszb@poczta.onet.pl> wrote:

> W dniu 05.12.2017 o 15:20, James Almer pisze:
> > On 12/4/2017 8:31 PM, Mateusz wrote:
> >> After some tests:
> >> 1) #undef far
> >> after #include <windows.h> is wrong -- in oleauto.h is declaration
> >> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
> >> and 'FAR' is defined as 'far' which is define as empty.
> >>
> >> 2) #undef near
> >> after #include <windows.h> works in ffmpeg but is danger -- see 1)
> >>
> >> 3) after
> >> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
> >> git revert 590136e78da3d091ea99ab5432543d47a559a461
> >> and patch
> >> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> >> -    MXFPackage packages[2] = {};
> >> +    MXFPackage packages[2] = {{NULL}};
> >> VS 2017 can compile ffmpeg and fate stops at
> audiomatch-nero-16000-mono-he-m4a
> >> (without reverting 590136e hangs at api-flac-test.exe)
> >>
> >> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
> >> we can apply this patch and
> >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> >> index baf09119fe..b34a3803b8 100644
> >> --- a/libavcodec/utils.c
> >> +++ b/libavcodec/utils.c
> >> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex,
> enum AVLockOp op))
> >>
> >>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
> >>  {
> >> -    _Bool exp = 0;
> >> +    atomic_bool exp = 0;
> >>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE ||
> !codec->init)
> >>          return 0;
> >>
> >> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx,
> const AVCodec *codec)
> >>
> >>  int ff_unlock_avcodec(const AVCodec *codec)
> >>  {
> >> -    _Bool exp = 1;
> >> +    atomic_bool exp = 1;
> >
> > No, these are not meant to be atomic.
> >
> > _Bool is c99, so maybe we could replace its usage with int in these lock
> > functions. The result will be the exact same.
>
> In atomic types the main thing is width (sizeof).
> In atomic operations destination memory MUST BE sufficient
> to copy all bytes from source, in
> atomic_compare_exchange_strong(&ff_avcodec_locked, &exp, 1)
> sizeof(exp) MUST BE >= sizeof(ff_avcodec_locked) == sizeof(atomic_bool)
>
> In ffmpeg implementation of atomic code for MSVC sizeof(atomic_bool) == 8
> and sizeof(_Bool) < 8 -- it is the reason of hangs.
>
> And all volatile objects MUST BE explicitly volatile -- instead of
> static atomic_bool ff_avcodec_locked;
> should be
> static volatile atomic_bool ff_avcodec_locked;
>
> I will prepare a patch (but not before Friday -- lots of work).
>
> Mateusz
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

That last thing belongs in the compat header rather than globablly in the
code.

On 12/5/2017 1:40 PM, Mateusz wrote:
> W dniu 05.12.2017 o 15:20, James Almer pisze:
>> On 12/4/2017 8:31 PM, Mateusz wrote:
>>> After some tests:
>>> 1) #undef far
>>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>>> and 'FAR' is defined as 'far' which is define as empty.
>>>
>>> 2) #undef near
>>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
>>>
>>> 3) after
>>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>>> and patch
>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>> -    MXFPackage packages[2] = {};
>>> +    MXFPackage packages[2] = {{NULL}};
>>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>>> (without reverting 590136e hangs at api-flac-test.exe)
>>>
>>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>>> we can apply this patch and
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index baf09119fe..b34a3803b8 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>>
>>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>  {
>>> -    _Bool exp = 0;
>>> +    atomic_bool exp = 0;
>>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>>          return 0;
>>>
>>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>
>>>  int ff_unlock_avcodec(const AVCodec *codec)
>>>  {
>>> -    _Bool exp = 1;
>>> +    atomic_bool exp = 1;
>>
>> No, these are not meant to be atomic.
>>
>> _Bool is c99, so maybe we could replace its usage with int in these lock
>> functions. The result will be the exact same.
> 
> In atomic types the main thing is width (sizeof).
> In atomic operations destination memory MUST BE sufficient
> to copy all bytes from source, in
> atomic_compare_exchange_strong(&ff_avcodec_locked, &exp, 1)
> sizeof(exp) MUST BE >= sizeof(ff_avcodec_locked) == sizeof(atomic_bool)
> 
> In ffmpeg implementation of atomic code for MSVC sizeof(atomic_bool) == 8
> and sizeof(_Bool) < 8 -- it is the reason of hangs.
> 
> And all volatile objects MUST BE explicitly volatile -- instead of
> static atomic_bool ff_avcodec_locked;
> should be
> static volatile atomic_bool ff_avcodec_locked;

No, the win32 wrapper should add volatile to the atomic_* typedefs if
anything. No other implementation needs it, so it must not be used here.

> 
> I will prepare a patch (but not before Friday -- lots of work).
> 
> Mateusz
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On Tue, Dec 5, 2017 at 5:25 PM, James Almer <jamrial@gmail.com> wrote:
> On 12/5/2017 1:13 PM, Hendrik Leppkes wrote:
>> On Tue, Dec 5, 2017 at 3:20 PM, James Almer <jamrial@gmail.com> wrote:
>>> On 12/4/2017 8:31 PM, Mateusz wrote:
>>>> After some tests:
>>>> 1) #undef far
>>>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>>>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>>>> and 'FAR' is defined as 'far' which is define as empty.
>>>>
>>>> 2) #undef near
>>>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
>>>>
>>>> 3) after
>>>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>>>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>>>> and patch
>>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>>> -    MXFPackage packages[2] = {};
>>>> +    MXFPackage packages[2] = {{NULL}};
>>>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>>>> (without reverting 590136e hangs at api-flac-test.exe)
>>>>
>>>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>>>> we can apply this patch and
>>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>>> index baf09119fe..b34a3803b8 100644
>>>> --- a/libavcodec/utils.c
>>>> +++ b/libavcodec/utils.c
>>>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>>>
>>>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>>  {
>>>> -    _Bool exp = 0;
>>>> +    atomic_bool exp = 0;
>>>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>>>          return 0;
>>>>
>>>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>>
>>>>  int ff_unlock_avcodec(const AVCodec *codec)
>>>>  {
>>>> -    _Bool exp = 1;
>>>> +    atomic_bool exp = 1;
>>>
>>> No, these are not meant to be atomic.
>>>
>>
>> int will also not work, since you would pass a pointer to an int to
>> something that expects a pointer to intptr_t.
>>
>> - Hendrik
>
> So it's not a problem of _Bool being undefined in msvc? Sounds like the
> win32 stdatomic.h wrapper are not correct, then. This was not an issue
> with the old lavu wrapper.
>

I don't know why atomic_compare_exchange* was implemented as a static
function and all types defined to the same type, instead of a macro
(like the GCC emulation).
In C++ you would use templates to handle all types, in C .. not sure
such mechanics exist.

- Hendrik

W dniu 05.12.2017 o 17:44, James Almer pisze:
> On 12/5/2017 1:40 PM, Mateusz wrote:
>> W dniu 05.12.2017 o 15:20, James Almer pisze:
>>> On 12/4/2017 8:31 PM, Mateusz wrote:
>>>> After some tests:
>>>> 1) #undef far
>>>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>>>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>>>> and 'FAR' is defined as 'far' which is define as empty.
>>>>
>>>> 2) #undef near
>>>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
>>>>
>>>> 3) after
>>>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>>>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>>>> and patch
>>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>>> -    MXFPackage packages[2] = {};
>>>> +    MXFPackage packages[2] = {{NULL}};
>>>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>>>> (without reverting 590136e hangs at api-flac-test.exe)
>>>>
>>>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>>>> we can apply this patch and
>>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>>> index baf09119fe..b34a3803b8 100644
>>>> --- a/libavcodec/utils.c
>>>> +++ b/libavcodec/utils.c
>>>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>>>
>>>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>>  {
>>>> -    _Bool exp = 0;
>>>> +    atomic_bool exp = 0;
>>>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>>>          return 0;
>>>>
>>>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>>
>>>>  int ff_unlock_avcodec(const AVCodec *codec)
>>>>  {
>>>> -    _Bool exp = 1;
>>>> +    atomic_bool exp = 1;
>>>
>>> No, these are not meant to be atomic.
>>>
>>> _Bool is c99, so maybe we could replace its usage with int in these lock
>>> functions. The result will be the exact same.
>>
>> In atomic types the main thing is width (sizeof).
>> In atomic operations destination memory MUST BE sufficient
>> to copy all bytes from source, in
>> atomic_compare_exchange_strong(&ff_avcodec_locked, &exp, 1)
>> sizeof(exp) MUST BE >= sizeof(ff_avcodec_locked) == sizeof(atomic_bool)
>>
>> In ffmpeg implementation of atomic code for MSVC sizeof(atomic_bool) == 8
>> and sizeof(_Bool) < 8 -- it is the reason of hangs.
>>
>> And all volatile objects MUST BE explicitly volatile -- instead of
>> static atomic_bool ff_avcodec_locked;
>> should be
>> static volatile atomic_bool ff_avcodec_locked;
> 
> No, the win32 wrapper should add volatile to the atomic_* typedefs if
> anything. No other implementation needs it, so it must not be used here.

Yes, you are right -- it should work not only in Windows. It is a bit tricky...
In Linux stdatomic.h looks quite different than ffmpeg win32/gcc wrappers.

Now I have no opinion how it should be done right.

Mateusz

W dniu 05.12.2017 o 12:12, Hendrik Leppkes pisze:
> On Tue, Dec 5, 2017 at 12:31 AM, Mateusz <mateuszb@poczta.onet.pl> wrote:
>> After some tests:
>> 1) #undef far
>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>> and 'FAR' is defined as 'far' which is define as empty.
> 
> Yeah generally undefing all of them might result in errors in other
> windows headers, so thats bad.
> 
>>
>> 2) #undef near
>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
> 
> Doing it rather late,  ie. right before its used in the file should be
> relatively safe. If not, we can always rename the variable.
> But it seems to work so far. And from the nature of this bug, it
> should just error out instead of resulting in other breakage - if it
> happens.
> 
>>
>> 3) after
>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>> and patch
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> -    MXFPackage packages[2] = {};
>> +    MXFPackage packages[2] = {{NULL}};
>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>> (without reverting 590136e hangs at api-flac-test.exe)
>>
>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>> we can apply this patch and
> 
> We can't really revert those, so fixing them is better.
> 
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index baf09119fe..b34a3803b8 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>
>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>  {
>> -    _Bool exp = 0;
>> +    atomic_bool exp = 0;
>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>          return 0;
>>
>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>
>>  int ff_unlock_avcodec(const AVCodec *codec)
>>  {
>> -    _Bool exp = 1;
>> +    atomic_bool exp = 1;
>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>          return 0;
>>
>>
> 
> atomic_compare_exchange* usage is a bit iffy because it uses pointers,
> and the compat wrappers don't fully represent all types properly (for
> simplicity, implementing them in a generic way would be madness).
> So this work-around seems fine to me, if it builds with GCC as well.
> 
> - Hendrik

Thanks for review.

I will try to write patches that not break ffmpeg on Linux
and will work with MSVC -- now it looks to me more complicated 
than before but it should be possible.

Mateusz

On 12/5/2017 8:12 AM, Hendrik Leppkes wrote:
> On Tue, Dec 5, 2017 at 12:31 AM, Mateusz <mateuszb@poczta.onet.pl> wrote:
>> After some tests:
>> 1) #undef far
>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>> and 'FAR' is defined as 'far' which is define as empty.
> 
> Yeah generally undefing all of them might result in errors in other
> windows headers, so thats bad.
> 
>>
>> 2) #undef near
>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
> 
> Doing it rather late,  ie. right before its used in the file should be
> relatively safe. If not, we can always rename the variable.
> But it seems to work so far. And from the nature of this bug, it
> should just error out instead of resulting in other breakage - if it
> happens.
> 
>>
>> 3) after
>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>> and patch
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> -    MXFPackage packages[2] = {};
>> +    MXFPackage packages[2] = {{NULL}};
>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>> (without reverting 590136e hangs at api-flac-test.exe)
>>
>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>> we can apply this patch and
> 
> We can't really revert those, so fixing them is better.
> 
>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> index baf09119fe..b34a3803b8 100644
>> --- a/libavcodec/utils.c
>> +++ b/libavcodec/utils.c
>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>
>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>  {
>> -    _Bool exp = 0;
>> +    atomic_bool exp = 0;
>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>          return 0;
>>
>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>
>>  int ff_unlock_avcodec(const AVCodec *codec)
>>  {
>> -    _Bool exp = 1;
>> +    atomic_bool exp = 1;
>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>          return 0;
>>
>>
> 
> atomic_compare_exchange* usage is a bit iffy because it uses pointers,
> and the compat wrappers don't fully represent all types properly (for
> simplicity, implementing them in a generic way would be madness).
> So this work-around seems fine to me, if it builds with GCC as well.

GCC may accept it (No idea what code it would generate, though), but
chances are Clang will complain since it's stricter about correct type
usage in these functions, and making the variable to be passed as the
"expected" parameter atomic is not correct.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

On Tue, Dec 5, 2017 at 8:23 PM, James Almer <jamrial@gmail.com> wrote:
> On 12/5/2017 8:12 AM, Hendrik Leppkes wrote:
>> On Tue, Dec 5, 2017 at 12:31 AM, Mateusz <mateuszb@poczta.onet.pl> wrote:
>>> After some tests:
>>> 1) #undef far
>>> after #include <windows.h> is wrong -- in oleauto.h is declaration
>>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
>>> and 'FAR' is defined as 'far' which is define as empty.
>>
>> Yeah generally undefing all of them might result in errors in other
>> windows headers, so thats bad.
>>
>>>
>>> 2) #undef near
>>> after #include <windows.h> works in ffmpeg but is danger -- see 1)
>>
>> Doing it rather late,  ie. right before its used in the file should be
>> relatively safe. If not, we can always rename the variable.
>> But it seems to work so far. And from the nature of this bug, it
>> should just error out instead of resulting in other breakage - if it
>> happens.
>>
>>>
>>> 3) after
>>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
>>> git revert 590136e78da3d091ea99ab5432543d47a559a461
>>> and patch
>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>> -    MXFPackage packages[2] = {};
>>> +    MXFPackage packages[2] = {{NULL}};
>>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
>>> (without reverting 590136e hangs at api-flac-test.exe)
>>>
>>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
>>> we can apply this patch and
>>
>> We can't really revert those, so fixing them is better.
>>
>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>>> index baf09119fe..b34a3803b8 100644
>>> --- a/libavcodec/utils.c
>>> +++ b/libavcodec/utils.c
>>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
>>>
>>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>  {
>>> -    _Bool exp = 0;
>>> +    atomic_bool exp = 0;
>>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>>          return 0;
>>>
>>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>>>
>>>  int ff_unlock_avcodec(const AVCodec *codec)
>>>  {
>>> -    _Bool exp = 1;
>>> +    atomic_bool exp = 1;
>>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
>>>          return 0;
>>>
>>>
>>
>> atomic_compare_exchange* usage is a bit iffy because it uses pointers,
>> and the compat wrappers don't fully represent all types properly (for
>> simplicity, implementing them in a generic way would be madness).
>> So this work-around seems fine to me, if it builds with GCC as well.
>
> GCC may accept it (No idea what code it would generate, though), but
> chances are Clang will complain since it's stricter about correct type
> usage in these functions, and making the variable to be passed as the
> "expected" parameter atomic is not correct.
>

Why does this code use atomics anyway?
The actual locking is done using a lock manager, and it warns/errors
when the lock functions are used concurrently without locking.

So, perhaps we should just remove that?

- Hendrik

On Tue, 5 Dec 2017 22:38:11 +0100
Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Tue, Dec 5, 2017 at 8:23 PM, James Almer <jamrial@gmail.com> wrote:
> > On 12/5/2017 8:12 AM, Hendrik Leppkes wrote:  
> >> On Tue, Dec 5, 2017 at 12:31 AM, Mateusz <mateuszb@poczta.onet.pl> wrote:  
> >>> After some tests:
> >>> 1) #undef far
> >>> after #include <windows.h> is wrong -- in oleauto.h is declaration
> >>> WINOLEAUTAPI VarUI1FromI8(LONG64 i64In, _Out_ BYTE FAR* pbOut);
> >>> and 'FAR' is defined as 'far' which is define as empty.  
> >>
> >> Yeah generally undefing all of them might result in errors in other
> >> windows headers, so thats bad.
> >>  
> >>>
> >>> 2) #undef near
> >>> after #include <windows.h> works in ffmpeg but is danger -- see 1)  
> >>
> >> Doing it rather late,  ie. right before its used in the file should be
> >> relatively safe. If not, we can always rename the variable.
> >> But it seems to work so far. And from the nature of this bug, it
> >> should just error out instead of resulting in other breakage - if it
> >> happens.
> >>  
> >>>
> >>> 3) after
> >>> git revert 3701d499f8734ec5f3e7359de7311b15d92070b0
> >>> git revert 590136e78da3d091ea99ab5432543d47a559a461
> >>> and patch
> >>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> >>> -    MXFPackage packages[2] = {};
> >>> +    MXFPackage packages[2] = {{NULL}};
> >>> VS 2017 can compile ffmpeg and fate stops at audiomatch-nero-16000-mono-he-m4a
> >>> (without reverting 590136e hangs at api-flac-test.exe)
> >>>
> >>> 4) if for any reasons commits 3701d49 and 590136e shouldn't be reverted,
> >>> we can apply this patch and  
> >>
> >> We can't really revert those, so fixing them is better.
> >>  
> >>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> >>> index baf09119fe..b34a3803b8 100644
> >>> --- a/libavcodec/utils.c
> >>> +++ b/libavcodec/utils.c
> >>> @@ -1943,7 +1943,7 @@ int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
> >>>
> >>>  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
> >>>  {
> >>> -    _Bool exp = 0;
> >>> +    atomic_bool exp = 0;
> >>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
> >>>          return 0;
> >>>
> >>> @@ -1969,7 +1969,7 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
> >>>
> >>>  int ff_unlock_avcodec(const AVCodec *codec)
> >>>  {
> >>> -    _Bool exp = 1;
> >>> +    atomic_bool exp = 1;
> >>>      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init)
> >>>          return 0;
> >>>
> >>>  
> >>
> >> atomic_compare_exchange* usage is a bit iffy because it uses pointers,
> >> and the compat wrappers don't fully represent all types properly (for
> >> simplicity, implementing them in a generic way would be madness).
> >> So this work-around seems fine to me, if it builds with GCC as well.  
> >
> > GCC may accept it (No idea what code it would generate, though), but
> > chances are Clang will complain since it's stricter about correct type
> > usage in these functions, and making the variable to be passed as the
> > "expected" parameter atomic is not correct.
> >  
> 
> Why does this code use atomics anyway?
> The actual locking is done using a lock manager, and it warns/errors
> when the lock functions are used concurrently without locking.
> 
> So, perhaps we should just remove that?

Well, in the mid-range, we could just drop all lock manager and atomics
code by managing the codec list like the BSF list, and finishing the
codec static init cleanup.