[FFmpeg-devel] [PATCH 2/2] av_lockmgr_register defines behavior on failure.
Manfred Georg
mgeorg at google.com
Thu Oct 2 20:54:31 CEST 2014
On Wed, Oct 1, 2014 at 5:40 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:
> On Wed, Oct 01, 2014 at 04:37:21PM -0700, Manfred Georg wrote:
> > [snip]
> >
> > > @@ -3457,22 +3457,53 @@ AVHWAccel *av_hwaccel_next(const AVHWAccel
> > > *hwaccel)
> > > > int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
> > > > {
> > > > if (lockmgr_cb) {
> > > > - if (lockmgr_cb(&codec_mutex, AV_LOCK_DESTROY))
> > > > - return -1;
> > > > - if (lockmgr_cb(&avformat_mutex, AV_LOCK_DESTROY))
> > > > - return -1;
> > > > + // 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);
> > > > + avpriv_atomic_ptr_cas((void * volatile *)&lockmgr_cb,
> > > > + lockmgr_cb, NULL);
> > > > + avpriv_atomic_ptr_cas((void * volatile *)&codec_mutex,
> > > > + codec_mutex, NULL);
> > > > + avpriv_atomic_ptr_cas((void * volatile *)&avformat_mutex,
> > > > + avformat_mutex, NULL);
> > > > + }
> > > > +
> > >
> > > > + if (lockmgr_cb || codec_mutex || avformat_mutex) {
> > > > + // Some synchronization error occurred.
> > > > + lockmgr_cb = NULL;
> > > > codec_mutex = NULL;
> > > > avformat_mutex = NULL;
> > > > + return AVERROR(EDEADLK);
> > > > }
> > >
> > > this should be av_assert0()
> > > we dont want to continue after we know that the variables have
> > > been corrupted
> > > also it could be a seperate patch
> > >
> > >
> > I feel that if we use the atomic operation then this should be included
> in
> > this patch (since otherwise what's the point in having atomic operations
> > which we never check whether they succeed. I'd be happy to replace the
> > atomic operations with "=" again. Please advise whether I should use
> > av_assert0() or return to "=".
>
> the point of the 2nd set of atomic operations is to ensure we
> dont overwrite a non null pointer
>
> the set above could check that the overwritten pointer equals what
> was set before destroy
> as they are iam not sure if they provide an advantage over a normal
> a=b, iam also not sure the first set is really that usefull
>
I reverted to using =. We can switch to atomic operations in a later patch
if that is desired.
>
>
> >
> >
> > >
> > > >
> > > > - lockmgr_cb = cb;
> > > > -
> > > > - if (lockmgr_cb) {
> > > > - if (lockmgr_cb(&codec_mutex, AV_LOCK_CREATE))
> > > > - return -1;
> > > > - if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE))
> > > > - return -1;
> > > > + 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 ? -err : 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 ? -err : err;
> > >
> > > how does this work ?
> > >
> >
> > It ensures that the returned value is negative on failure. It also
> passes
> > through error codes if they are used. Note that we have to do something
> > with positive values, since even the lock manager in ffplay.c uses a
> > positive value to denote failure (it uses 1).
> > return err > 0 ? AVERROR(SOMETHING) : err;
> > would also work (or -1).
> > Please advise.
>
> AVERROR_EXTERNAL seems like an option
>
Done.
New patch:
Subject: [PATCH] av_lockmgr_register defines behavior on failure.
The register function now specifies that the user callback should
leave things in the same state that it found them on failure but
that failure to destroy is ignored by the library. The register
function is now explicit about its behavior on failure
(it unregisters the previous callback and destroys all mutex).
---
libavcodec/avcodec.h | 30 ++++++++++++++++++++----------
libavcodec/utils.c | 34 ++++++++++++++++++++++------------
2 files changed, 42 insertions(+), 22 deletions(-)
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 94e82f7..7fb97da 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -5120,16 +5120,26 @@ enum AVLockOp {
/**
* Register a user provided lock manager supporting the operations
- * specified by AVLockOp. mutex points to a (void *) where the
- * lockmgr should store/get a pointer to a user allocated mutex. It's
- * NULL upon AV_LOCK_CREATE and != NULL for all other ops.
- *
- * @param cb User defined callback. Note: FFmpeg may invoke calls to this
- * callback during the call to av_lockmgr_register().
- * Thus, the application must be prepared to handle that.
- * If cb is set to NULL the lockmgr will be unregistered.
- * Also note that during unregistration the previously registered
- * lockmgr callback may also be invoked.
+ * specified by AVLockOp. The "mutex" argument to the function points
+ * to a (void *) where the lockmgr should store/get a pointer to a user
+ * allocated mutex. It is NULL upon AV_LOCK_CREATE and equal to the
+ * value left by the last call for all other ops. If the lock manager is
+ * unable to perform the op then it should leave the mutex in the same
+ * state as when it was called and return a non-zero value. However,
+ * when called with AV_LOCK_DESTROY the mutex will always be assumed to
+ * have been successfully destroyed. If av_lockmgr_register succeeds
+ * it will return a non-negative value, if it fails it will return a
+ * negative value and destroy all mutex and unregister all callbacks.
+ * av_lockmgr_register is not thread-safe, it must be called from a
+ * single thread before any calls which make use of locking are used.
+ *
+ * @param cb User defined callback. av_lockmgr_register invokes calls
+ * to this callback and the previously registered callback.
+ * The callback will be used to create more than one mutex
+ * each of which must be backed by its own underlying locking
+ * 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.
*/
int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op));
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 778bdc6..b0ec09a 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -3457,22 +3457,32 @@ AVHWAccel *av_hwaccel_next(const AVHWAccel *hwaccel)
int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op))
{
if (lockmgr_cb) {
- if (lockmgr_cb(&codec_mutex, AV_LOCK_DESTROY))
- return -1;
- if (lockmgr_cb(&avformat_mutex, AV_LOCK_DESTROY))
- return -1;
- codec_mutex = NULL;
+ // 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;
}
- lockmgr_cb = cb;
-
- if (lockmgr_cb) {
- if (lockmgr_cb(&codec_mutex, AV_LOCK_CREATE))
- return -1;
- if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE))
- return -1;
+ 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_EXTERNAL : 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_EXTERNAL : err;
+ }
+ lockmgr_cb = cb;
+ codec_mutex = new_codec_mutex;
+ avformat_mutex = new_avformat_mutex;
}
+
return 0;
}
--
2.1.0.rc2.206.gedb03e5
More information about the ffmpeg-devel
mailing list