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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130318/7dbf612f/attachment.asc>