[FFmpeg-devel] [PATCH 2/2] av_lockmgr_register defines behavior on failure.

Manfred Georg mgeorg at google.com
Wed Oct 1 22:01:33 CEST 2014


On Tue, Sep 30, 2014 at 8:16 PM, Michael Niedermayer <michaelni at gmx.at>
wrote:

> On Tue, Sep 30, 2014 at 07:34:28PM -0700, Manfred Georg wrote:
> > Yeah, that seemed a bit odd to me....I guess I get to go correct some
> > calling code.
> >
> > Here is yet another update with a comment which tells you not to use a
> > static mutex.
> >
> > 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 ffmpeg.  The register
> > function is also now explicit about its behavior on failure (it now
> > unregisters the previous callback and destroys all mutex).
> > ---
> >  libavcodec/avcodec.h | 25 ++++++++++++++++---------
> >  libavcodec/utils.c   | 31 +++++++++++++++++++++----------
> >  2 files changed, 37 insertions(+), 19 deletions(-)
> >
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 94e82f7..dae3612 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -5120,16 +5120,23 @@ 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.
> > + * 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.  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 0, if it fails it will
> > + * return non-zero and destroy all mutex and unregister all callbacks.
>
> we have no positve error codes, the positive values could be used
> as success like 0 giving us the ability to extend/use them for
> something in the future
>

Changed comment.  I also changed the code to propagate the error from the
user function.  However, to maintain backwards compatibility, I negate any
positive values that the user provides.

>
>
> > + *
> > + * @param cb User defined callback.  FFmpeg invokes calls to this
> > + *           callback and the previously registered callback during the
> > + *           call to av_lockmgr_register().  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.
> > - *           Also note that during unregistration the previously
> registered
> > - *           lockmgr callback may also be invoked.
> >   */
> >  int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op));
> >
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 778bdc6..717c5b1 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -3457,22 +3457,33 @@ 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;
> > -        avformat_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 = cb;
> > +    lockmgr_cb = NULL;
>
> > +    codec_mutex = NULL;
> > +    avformat_mutex = NULL;
>
> why is this moved outside "if (lockmgr_cb)" ?
>

Because I didn't want to think about it too hard. :)  I moved all three
assignments inside the if statement (after thinking about it for a bit).


>
>
> >
> > -    if (lockmgr_cb) {
> > -        if (lockmgr_cb(&codec_mutex, AV_LOCK_CREATE))
> > +    if (cb) {
> > +        void *new_codec_mutex = NULL;
> > +        void *new_avformat_mutex = NULL;
> > +        if (cb(&new_codec_mutex, AV_LOCK_CREATE)) {
> >              return -1;
> > -        if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE))
> > +        }
> > +        if (cb(&new_avformat_mutex, AV_LOCK_CREATE))
> > +        {
> > +            // Ignore failures to destroy the newly created mutex.
> > +            cb(&new_codec_mutex, AV_LOCK_DESTROY);
> >              return -1;
> > +        }
>
> > +        lockmgr_cb = cb;
> > +        codec_mutex = new_codec_mutex;
> > +        avformat_mutex = new_avformat_mutex;
>
> these probably should use avpriv_atomic_ptr_cas()
> to ensure the values where NULL that are replaced.
> this could provide usefull debug information if someone
> misuses av_lockmgr_register() like calling it from 2 threads at the
> same time
>

Changed assignments to use atomic operation and added some sanity checking
that the operations actually happened.  This is probably overkill,
considering the disaster which would happen if av_lockmgr_register is
called concurrently with any function which makes use of locking.
Personally, I kind of liked it better beforehand, but I don't have much
experience with threading issues.

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 ffmpeg.  The register
function is also now explicit about its behavior on failure (it now
unregisters the previous callback and destroys all mutex).
Furthermore, the register function is now a bit more careful about
synchronization issues.
---
 libavcodec/avcodec.h | 28 ++++++++++++++++++---------
 libavcodec/utils.c   | 53
+++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 94e82f7..f7ad115 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.
+ * 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.  FFmpeg invokes calls to this
+ *           callback and the previously registered callback during the
+ *           call to av_lockmgr_register().  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.
- *           Also note that during unregistration the previously registered
- *           lockmgr callback may also be invoked.
  */
 int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op));

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 778bdc6..0f1c8ff 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -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);
     }

-    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;
+        }
+        avpriv_atomic_ptr_cas((void * volatile *)&lockmgr_cb, NULL, cb);
+        avpriv_atomic_ptr_cas((void * volatile *)&codec_mutex,
+                              NULL, new_codec_mutex);
+        avpriv_atomic_ptr_cas((void * volatile *)&avformat_mutex,
+                              NULL, new_avformat_mutex);
+        if (lockmgr_cb != cb || codec_mutex != new_codec_mutex ||
+            avformat_mutex != new_avformat_mutex) {
+            // Some synchronization error occurred.
+            lockmgr_cb = NULL;
+            codec_mutex = NULL;
+            avformat_mutex = NULL;
+            return AVERROR(EDEADLK);
+        }
     }
+
     return 0;
 }

-- 
2.1.0.rc2.206.gedb03e5


More information about the ffmpeg-devel mailing list