[FFmpeg-devel] Patch: Enable OpenCL with Win32 threads
Matt Oliver
protogonoi at gmail.com
Thu May 1 11:23:15 CEST 2014
On 18 April 2014 11:25, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, Apr 17, 2014 at 01:46:36PM +1000, Matt Oliver wrote:
> > >
> > > Could this just be integrated in the pthread_mutex_lock win32 wrapper
> > > code to emulate PTHREAD_MUTEX_INITIALIZER?
> >
> >
> > It probably shouldn't as it adds extra checks that are not necessary if
> you
> > did the appropriate _init calls anyway (also I did it wrong ;) ). Win32
> > already does an internal check on the -1 waiters value which is what I
> used
> > in the first patch that added a static initialisation for win32.
> >
> >
> > > this is not thread safe at all
> > >
> > > thread #1 avpriv_atomic_cas
> > > thread #2 avpriv_atomic_cas (if skiped) LOCK_OPENCL
> use
> > > of uninitialized mutex
> > >
> > > please see the existing code, that sets up global mutexes
> > > in the lock manager for example
> > >
> >
> > This is one of those things were I knew that and knew better but for some
> > reason did it anyway (good reason not to submit patches late at night).
> It
> > needed a second atomic and wait to work properly. Anyway attached is a
> > patch that initialises in the same way the existing lock manager does
> which
> > should be more acceptable and consistent with existing code. Its a little
> > more complicated than static initialisation but if the first static patch
> > with the win32 static initializer isnt suitable then this seems the most
> > consistent way to support all native thread implementations.
>
> > opencl.c | 63
> ++++++++++++++++++++++++++++++++++++++++++++++++++-------------
> > opencl.h | 1 +
> > 2 files changed, 51 insertions(+), 13 deletions(-)
> > 1b908cff3181f07226e68c0ed2fa647517dcfae7
> opencl-add-support-for-non-pthread-locking.patch
> > From 7644090cf14982a700859b6ae23724f3a1800ace Mon Sep 17 00:00:00 2001
> > From: Matt Oliver <protogonoi at gmail.com>
> > Date: Thu, 17 Apr 2014 13:21:48 +1000
> > Subject: [PATCH] opencl: add support for non-pthread locking
> >
> > ---
> > libavutil/opencl.c | 63
> +++++++++++++++++++++++++++++++++++++++++++-----------
> > libavutil/opencl.h | 1 +
> > 2 files changed, 51 insertions(+), 13 deletions(-)
> >
> > diff --git a/libavutil/opencl.c b/libavutil/opencl.c
> > index 142c6b0..5a3b85f 100644
> > --- a/libavutil/opencl.c
> > +++ b/libavutil/opencl.c
> > @@ -27,15 +27,21 @@
> > #include "avassert.h"
> > #include "opt.h"
> >
> > +#if HAVE_THREADS
> > #if HAVE_PTHREADS
> > -
> > #include <pthread.h>
> > -static pthread_mutex_t atomic_opencl_lock = PTHREAD_MUTEX_INITIALIZER;
> > -
> > -#define LOCK_OPENCL pthread_mutex_lock(&atomic_opencl_lock)
> > -#define UNLOCK_OPENCL pthread_mutex_unlock(&atomic_opencl_lock)
> > +#elif HAVE_W32THREADS
> > +#include "compat/w32pthreads.h"
> > +#elif HAVE_OS2THREADS
> > +#include "compat/os2threads.h"
> > +#endif
> > +#include "atomic.h"
> >
> > -#elif !HAVE_THREADS
> > +#define MUTEX_OPENCL pthread_mutex_t *atomic_opencl_lock;
> > +#define LOCK_OPENCL pthread_mutex_lock(opencl_ctx.atomic_opencl_lock)
> > +#define UNLOCK_OPENCL
> pthread_mutex_unlock(opencl_ctx.atomic_opencl_lock)
> > +#else
> > +#define MUTEX_OPENCL
> > #define LOCK_OPENCL
> > #define UNLOCK_OPENCL
> > #endif
> > @@ -74,6 +80,7 @@ typedef struct {
> > int kernel_code_count;
> > KernelCode kernel_code[MAX_KERNEL_CODE_NUM];
> > AVOpenCLDeviceList device_list;
> > + MUTEX_OPENCL
> > } OpenclContext;
> >
> > #define OFFSET(x) offsetof(OpenclContext, x)
> > @@ -321,14 +328,44 @@ void av_opencl_free_device_list(AVOpenCLDeviceList
> **device_list)
> > av_freep(device_list);
> > }
> >
> > -int av_opencl_set_option(const char *key, const char *val)
> > +inline int init_opencl_ctx(void)
> > {
> > - int ret = 0;
> > - LOCK_OPENCL;
> > +#if HAVE_THREADS
>
> > + if (!opencl_ctx.atomic_opencl_lock) {
>
> Thread #1 if(1), Thread #2 if(1)
>
> > + pthread_mutex_t *tmp = av_malloc(sizeof(pthread_mutex_t));
> > + int err;
> > + if (!tmp)
> > + return AVERROR(ENOMEM);
> > + if ((err = pthread_mutex_init(tmp, NULL))) {
> > + av_free(tmp);
> > + return AVERROR(err);
> > + }
>
> > + if (!avpriv_atomic_ptr_cas(opencl_ctx.atomic_opencl_lock, NULL,
> tmp)) {
>
> Thread #1 if(1), Thread #2 if(0)
> Thread #1 stops here, by choice of the scheduler
>
> > + LOCK_OPENCL;
> > + av_opt_set_defaults(&opencl_ctx);
> > + opencl_ctx.opt_init_flag = 1;
> > + UNLOCK_OPENCL;
> > + }
> > + else {
> > + pthread_mutex_destroy(tmp);
> > + av_free(tmp);
> > + }
>
> Thread #2 continues on without av_opt_set_defaults() having been
> executed
>
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I have never wished to cater to the crowd; for what I know they do not
> approve, and what they approve I do not know. -- Epicurus
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Well this would be pretty rare as any code that calls the init_ctx function
immediately follows it with a mutex lock. So the scheduler would have to
pause thread #1 exactly after the atomic function and before the following
Lock statement for there to be a problem. But you are correct in that it is
possible so attached is a patch that rectifies this. As a result the
changes are far less and this patch just adds the function to detect that
the mutex has been initialised. Constantly checking is a little ugly but
short of static init or a shared global function for all mutex
initialisation then this is the only way. The checks are only done when
getting set options or when actually initialising opencl which is only
really done once so its not actually that much of an issue.
Matt
-------------- next part --------------
A non-text attachment was scrubbed...
Name: opencl-add-support-for-non-pthread-locking.patch
Type: application/octet-stream
Size: 3152 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140501/425f29f6/attachment.obj>
More information about the ffmpeg-devel
mailing list