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

[Hendrik Leppkes]

On Mon, Mar 18, 2013 at 5:36 PM, compn <tempn at twmi.rr.com> wrote:
> 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.
>

If people don't know that mingw32 is discouraged to be used, it could
result in more bugreports of things that are known to be broken with
it, instead of the user realizing this and upgrading his toolchain.
I am using mingw, and i would not waste my time trying to support a
outdated and abandoned toolchain, is all. :)