[FFmpeg-devel] Patch: Enable OpenCL with Win32 threads
Michael Niedermayer
michaelni at gmx.at
Thu May 1 20:27:36 CEST 2014
On Thu, May 01, 2014 at 07:23:15PM +1000, Matt Oliver wrote:
> 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
> opencl.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
> opencl.h | 1 +
> 2 files changed, 42 insertions(+), 9 deletions(-)
> 99edce8779dbdcc1ec884c54a0e96123bf576f10 opencl-add-support-for-non-pthread-locking.patch
> From db36cb0349527abcab76cb589008b19f53c7ea1f Mon Sep 17 00:00:00 2001
> From: Matt Oliver <protogonoi at gmail.com>
> Date: Thu, 1 May 2014 19:21:29 +1000
> Subject: [PATCH] opencl: add support for non-pthread locking
applied
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20140501/94ed0e21/attachment.asc>
More information about the ffmpeg-devel
mailing list