[FFmpeg-devel] [PATCH/RFC] Trac ticket 2363

Hendrik Leppkes h.leppkes at gmail.com
Mon Mar 18 08:08:35 CET 2013


On Mon, Mar 18, 2013 at 12:42 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Mar 17, 2013 at 06:27:28PM -0300, James Almer wrote:
>> lavu/atomic.c is not compiling with gcc when using -march=i386 and w32threads
>> or os2threads because __sync_val_compare_and_swap() is not defined and the
>> fallback implementation currently only supports pthreads.
>> Mingw-w64 has MemoryBarrier() defined even when using -march=i386, so this is
>> not as much of a problem as it is with mingw32.
>>
>> I was able to make it work using the pthreads wrappers from libavcodec, but
>> I'm not sure if this is a proper fix or not since threading is not my field of
>> expertise.
>>
>> I'm attaching two versions of the proposed patch. Both are essentially the
>> same, but one alters the pthreads implementation for the sake of cleaner code.
>> I didn't test with the pthreads wrapper for os2threads because i have no way
>> to do it, but in theory it should work as well.
>>
>> I'll send an actual patch once (and if) one of these is accepted.
>>
>> Regards.
>
>>  atomic.c |   27 +++++++++++++++++++--------
>>  1 file changed, 19 insertions(+), 8 deletions(-)
>> d2f851be5ec4d25c4b655b6a5cef60594edf989e  atomic.diff
>> diff --git a/libavutil/atomic.c b/libavutil/atomic.c
>> index 7a701e1..12537ad 100644
>> --- a/libavutil/atomic.c
>> +++ b/libavutil/atomic.c
>> @@ -22,38 +22,50 @@
>>
>>  #if !HAVE_MEMORYBARRIER && !HAVE_SYNC_VAL_COMPARE_AND_SWAP && !HAVE_MACHINE_RW_BARRIER
>>
>> -#if HAVE_PTHREADS
>> +#if HAVE_THREADS
>>
>> +#if HAVE_PTHREADS
>>  #include <pthread.h>
>> +#elif HAVE_W32THREADS
>> +#include "w32pthreads.h"
>> +#elif HAVE_OS2THREADS
>> +#include "os2threads.h"
>> +#endif
>>
>> -static pthread_mutex_t atomic_lock = PTHREAD_MUTEX_INITIALIZER;
>> +static pthread_mutex_t atomic_lock;
>>
>>  int avpriv_atomic_int_get(volatile int *ptr)
>>  {
>>      int res;
>>
>> +    pthread_mutex_init(&atomic_lock, NULL);
>>      pthread_mutex_lock(&atomic_lock);
>>      res = *ptr;
>>      pthread_mutex_unlock(&atomic_lock);
>> +    pthread_mutex_destroy(&atomic_lock);
>
> one possible solution would look something like:
>
> +static int volatile init;
>
>  int avpriv_atomic_int_get(volatile int *ptr)
>  {
>      int res;
>
> +    if(!init) {
> +        pthread_mutex_init(&atomic_lock, NULL);
> +        init = 1;
> +    }
>
>      pthread_mutex_lock(&atomic_lock);
>      res = *ptr;
>      pthread_mutex_unlock(&atomic_lock);
>  }
>
> this has 2 issues
> theres a unlikely race on allocation and we dont bother
> destroying the mutex ever
>
> also, a comment from some win32 expert if there is a better way would
> be welcome ...
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> When you are offended at any man's fault, turn to yourself and study your
> own failings. Then you will forget your anger. -- Epictetus
>

What i would do (besides calling mingw32 unsupported) is keep the
pthread implementation as-is, and add a second fallback using win32
mutexes directly.
This way you don't add any of the risk factors to the existing pthread
implementation, which may get used on more "fringe" systems without
atomics, and only use the new implementation for people with outdated
toolchains on windows.
Additionally, the pthread compat layers are in avcodec, and
technically not available to avutil.

Considering you only need 3 functions, it seems easier to just create
a second block and use
InitializeCriticalSection/EnterCriticalSection/LeaveCriticalSection
from win32 (which are required for w32threads to work, so you can
assume they are present under a #if HAVE_W32THREADS

A bit of a side-topic, but maybe configure should at least warn that
mingw32 is lacking features and builds may be slower or lack features?


More information about the ffmpeg-devel mailing list