| Message ID | 20171221222224.18577-2-nfxjfg@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Dec 21, 2017 at 11:22:21PM +0100, wm4 wrote: > Use static mutexes instead of requiring a lock manager. The behavior > should be roughly the same before and after this change for API users > which did not set the lock manager at all (except that a minor memory > leak disappears). > --- > doc/APIchanges | 5 +++ > libavcodec/avcodec.h | 8 +++- > libavcodec/utils.c | 107 +++++---------------------------------------------- > libavcodec/version.h | 5 ++- > 4 files changed, 26 insertions(+), 99 deletions(-) This fails to build with: make distclean ; ./configure --disable-pthreads && make -j12 libavcodec/utils.c:72:38: error: ‘PTHREAD_MUTEX_INITIALIZER’ undeclared here (not in a function) static pthread_mutex_t codec_mutex = PTHREAD_MUTEX_INITIALIZER; [...]
On 12/22/2017 10:25 PM, Michael Niedermayer wrote: > On Thu, Dec 21, 2017 at 11:22:21PM +0100, wm4 wrote: >> Use static mutexes instead of requiring a lock manager. The behavior >> should be roughly the same before and after this change for API users >> which did not set the lock manager at all (except that a minor memory >> leak disappears). >> --- >> doc/APIchanges | 5 +++ >> libavcodec/avcodec.h | 8 +++- >> libavcodec/utils.c | 107 +++++---------------------------------------------- >> libavcodec/version.h | 5 ++- >> 4 files changed, 26 insertions(+), 99 deletions(-) > > This fails to build with: > make distclean ; ./configure --disable-pthreads && make -j12 > > libavcodec/utils.c:72:38: error: ‘PTHREAD_MUTEX_INITIALIZER’ undeclared here (not in a function) > static pthread_mutex_t codec_mutex = PTHREAD_MUTEX_INITIALIZER; I guess the simplest way to fix this would be to wrap all this code in HAVE_THREADS checks (Much like it was pre patch), but ideally the libavutil/threads.h wrappers would be used instead, after being extended to also define PTHREAD_MUTEX_INITIALIZER for the no threads case.
On Sat, 23 Dec 2017 02:25:11 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Dec 21, 2017 at 11:22:21PM +0100, wm4 wrote: > > Use static mutexes instead of requiring a lock manager. The behavior > > should be roughly the same before and after this change for API users > > which did not set the lock manager at all (except that a minor memory > > leak disappears). > > --- > > doc/APIchanges | 5 +++ > > libavcodec/avcodec.h | 8 +++- > > libavcodec/utils.c | 107 +++++---------------------------------------------- > > libavcodec/version.h | 5 ++- > > 4 files changed, 26 insertions(+), 99 deletions(-) > > This fails to build with: > make distclean ; ./configure --disable-pthreads && make -j12 > > libavcodec/utils.c:72:38: error: ‘PTHREAD_MUTEX_INITIALIZER’ undeclared here (not in a function) > static pthread_mutex_t codec_mutex = PTHREAD_MUTEX_INITIALIZER; > > [...] Will fix locally by using our pthread aliases instead of pthread names directly (ff_mutex_lock() etc.), and I'll define PTHREAD_MUTEX_INITIALIZER as AV_MUTEX_INITIALIZER.
On Thu, Dec 21, 2017 at 11:22:21PM +0100, wm4 wrote: > Use static mutexes instead of requiring a lock manager. The behavior > should be roughly the same before and after this change for API users > which did not set the lock manager at all (except that a minor memory > leak disappears). > --- > doc/APIchanges | 5 +++ > libavcodec/avcodec.h | 8 +++- > libavcodec/utils.c | 107 +++++---------------------------------------------- > libavcodec/version.h | 5 ++- > 4 files changed, 26 insertions(+), 99 deletions(-) Are all thread APIs users used with our lock manager compatible with this replacement ? Someone, possibly reimar, but i may misremember who it was. Talked in the past about thread API incompatibilies I believe the concern was that the user APP used multiple threads, one for each decoder and one for the demuxer. But the locking used by the user app between its threads could be incompatible with the locking of our "pthread" locks. If this is not an issue, then iam happy about this patch and the removial of the lock manager complexity. thanks [...]
On Sat, 23 Dec 2017 22:32:36 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Dec 21, 2017 at 11:22:21PM +0100, wm4 wrote: > > Use static mutexes instead of requiring a lock manager. The behavior > > should be roughly the same before and after this change for API users > > which did not set the lock manager at all (except that a minor memory > > leak disappears). > > --- > > doc/APIchanges | 5 +++ > > libavcodec/avcodec.h | 8 +++- > > libavcodec/utils.c | 107 +++++---------------------------------------------- > > libavcodec/version.h | 5 ++- > > 4 files changed, 26 insertions(+), 99 deletions(-) > > Are all thread APIs users used with our lock manager compatible with this > replacement ? Yes, unless they explicitly disable threads in FFmpeg and used a lock manager to compensate for it. But that would lose safety in other places, where we already rely on pthread_once(), and would disable useful features like frame threading. Possibly we should add a note about this when the next release happens, or disallow disabling building without thread support. > Someone, possibly reimar, but i may misremember who it was. Talked in the past > about thread API incompatibilies > I believe the concern was that the user APP used multiple threads, one for each > decoder and one for the demuxer. But the locking used by the user app between > its threads could be incompatible with the locking of our "pthread" locks. > > If this is not an issue, then iam happy about this patch and the > removial of the lock manager complexity. I'm not worried about it, because FFmpeg uses the OS native threading lib. Thus it's compatible with whatever else builds on native threading. I don't even think it's possible to have threading that does not build on the OS primitives, unless you do extremely hacky stuff behind the libc (or ntdll on Windows).
diff --git a/doc/APIchanges b/doc/APIchanges index da444ffb7c..df1391d83a 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,11 @@ libavutil: 2017-10-21 API changes, most recent first: +2017-xx-xx - xxxxxxx - lavc 58.9.100 - avcodec.h + Deprecate av_lockmgr_register(). You need to build FFmpeg with threading + support enabled to get basic thread-safety (which is the default build + configuration). + 2017-xx-xx - xxxxxxx - lavc 58.8.100 - avcodec.h The MediaCodec decoders now support AVCodecContext.hw_device_ctx. diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index ce089b7c4a..a9182a9e3d 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -5930,10 +5930,11 @@ attribute_deprecated AVHWAccel *av_hwaccel_next(const AVHWAccel *hwaccel); #endif - +#if FF_API_LOCKMGR /** * Lock operation used by lockmgr */ +attribute_deprecated enum AVLockOp { AV_LOCK_CREATE, ///< Create a mutex AV_LOCK_OBTAIN, ///< Lock the mutex @@ -5963,8 +5964,13 @@ enum AVLockOp { * mechanism (i.e. do not use a single static object to * implement your lock manager). If cb is set to NULL the * lockmgr will be unregistered. + * + * @deprecated This function does nothing, and always returns 0. Be sure to + * build with thread support to get basic thread safety. */ +attribute_deprecated int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op)); +#endif /** * Get the type of the given codec. diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 873f39f9bd..c7f8cdf08f 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -67,58 +67,10 @@ #include "libavutil/ffversion.h" const char av_codec_ffversion[] = "FFmpeg version " FFMPEG_VERSION; -#if HAVE_PTHREADS || HAVE_W32THREADS || HAVE_OS2THREADS -static int default_lockmgr_cb(void **arg, enum AVLockOp op) -{ - void * volatile * mutex = arg; - int err; - - switch (op) { - case AV_LOCK_CREATE: - return 0; - case AV_LOCK_OBTAIN: - if (!*mutex) { - pthread_mutex_t *tmp = av_malloc(sizeof(pthread_mutex_t)); - if (!tmp) - return AVERROR(ENOMEM); - if ((err = pthread_mutex_init(tmp, NULL))) { - av_free(tmp); - return AVERROR(err); - } - if (avpriv_atomic_ptr_cas(mutex, NULL, tmp)) { - pthread_mutex_destroy(tmp); - av_free(tmp); - } - } - - if ((err = pthread_mutex_lock(*mutex))) - return AVERROR(err); - - return 0; - case AV_LOCK_RELEASE: - if ((err = pthread_mutex_unlock(*mutex))) - return AVERROR(err); - - return 0; - case AV_LOCK_DESTROY: - if (*mutex) - pthread_mutex_destroy(*mutex); - av_free(*mutex); - avpriv_atomic_ptr_cas(mutex, *mutex, NULL); - return 0; - } - return 1; -} -static int (*lockmgr_cb)(void **mutex, enum AVLockOp op) = default_lockmgr_cb; -#else -static int (*lockmgr_cb)(void **mutex, enum AVLockOp op) = NULL; -#endif - - volatile int ff_avcodec_locked; static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0); -static void *codec_mutex; -static void *avformat_mutex; +static pthread_mutex_t codec_mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_mutex_t avformat_mutex = PTHREAD_MUTEX_INITIALIZER; void av_fast_padded_malloc(void *ptr, unsigned int *size, size_t min_size) { @@ -1909,55 +1861,26 @@ void av_register_hwaccel(AVHWAccel *hwaccel) } #endif +#if FF_API_LOCKMGR int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op)) { - if (lockmgr_cb) { - // There is no good way to rollback a failure to destroy the - // mutex, so we ignore failures. - lockmgr_cb(&codec_mutex, AV_LOCK_DESTROY); - lockmgr_cb(&avformat_mutex, AV_LOCK_DESTROY); - lockmgr_cb = NULL; - codec_mutex = NULL; - avformat_mutex = NULL; - } - - if (cb) { - void *new_codec_mutex = NULL; - void *new_avformat_mutex = NULL; - int err; - if (err = cb(&new_codec_mutex, AV_LOCK_CREATE)) { - return err > 0 ? AVERROR_UNKNOWN : err; - } - if (err = cb(&new_avformat_mutex, AV_LOCK_CREATE)) { - // Ignore failures to destroy the newly created mutex. - cb(&new_codec_mutex, AV_LOCK_DESTROY); - return err > 0 ? AVERROR_UNKNOWN : err; - } - lockmgr_cb = cb; - codec_mutex = new_codec_mutex; - avformat_mutex = new_avformat_mutex; - } - return 0; } +#endif int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec) { if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE || !codec->init) return 0; - if (lockmgr_cb) { - if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_OBTAIN)) - return -1; - } + if (pthread_mutex_lock(&codec_mutex)) + return -1; if (atomic_fetch_add(&entangled_thread_counter, 1)) { av_log(log_ctx, AV_LOG_ERROR, "Insufficient thread locking. At least %d threads are " "calling avcodec_open2() at the same time right now.\n", atomic_load(&entangled_thread_counter)); - if (!lockmgr_cb) - av_log(log_ctx, AV_LOG_ERROR, "No lock manager is set, please see av_lockmgr_register()\n"); ff_avcodec_locked = 1; ff_unlock_avcodec(codec); return AVERROR(EINVAL); @@ -1975,30 +1898,20 @@ int ff_unlock_avcodec(const AVCodec *codec) av_assert0(ff_avcodec_locked); ff_avcodec_locked = 0; atomic_fetch_add(&entangled_thread_counter, -1); - if (lockmgr_cb) { - if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE)) - return -1; - } + if (pthread_mutex_unlock(&codec_mutex)) + return -1; return 0; } int avpriv_lock_avformat(void) { - if (lockmgr_cb) { - if ((*lockmgr_cb)(&avformat_mutex, AV_LOCK_OBTAIN)) - return -1; - } - return 0; + return pthread_mutex_lock(&avformat_mutex) ? -1 : 0; } int avpriv_unlock_avformat(void) { - if (lockmgr_cb) { - if ((*lockmgr_cb)(&avformat_mutex, AV_LOCK_RELEASE)) - return -1; - } - return 0; + return pthread_mutex_unlock(&avformat_mutex) ? -1 : 0; } unsigned int avpriv_toupper4(unsigned int x) diff --git a/libavcodec/version.h b/libavcodec/version.h index d55de89797..47a15d52b8 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -28,7 +28,7 @@ #include "libavutil/version.h" #define LIBAVCODEC_VERSION_MAJOR 58 -#define LIBAVCODEC_VERSION_MINOR 8 +#define LIBAVCODEC_VERSION_MINOR 9 #define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ @@ -126,6 +126,9 @@ #ifndef FF_API_USER_VISIBLE_AVHWACCEL #define FF_API_USER_VISIBLE_AVHWACCEL (LIBAVCODEC_VERSION_MAJOR < 59) #endif +#ifndef FF_API_LOCKMGR +#define FF_API_LOCKMGR (LIBAVCODEC_VERSION_MAJOR < 59) +#endif #endif /* AVCODEC_VERSION_H */