Message ID | 63cad17b-9ecb-fb65-f647-0c59e6952405@poczta.onet.pl |
---|---|
State | Superseded |
Headers | show |
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.
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; if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0;