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

compn tempn at twmi.rr.com
Mon Mar 18 17:36:37 CET 2013


On Mon, 18 Mar 2013 15:43:33 +0100, Michael Niedermayer wrote:
>On Mon, Mar 18, 2013 at 08:08:35AM +0100, Hendrik Leppkes wrote:
>> 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
>
>agree
>
>
>> 
>> 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?
>
>possibly ... iam no win32 person so this isnt my area but iam happy
>to apply a a patch making such change if thats what the people using
>win32 want

i'd rather people work on bugreports instead of warning messages no one
cares about. if there is feature missing or bug in mingw, there are a
few devs who use mingw who can work on it.

-compn


More information about the ffmpeg-devel mailing list