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

Michael Niedermayer michaelni at gmx.at
Thu Oct 2 00:44:37 CEST 2014


On Wed, Oct 01, 2014 at 01:01:33PM -0700, Manfred Georg wrote:
> 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);
>      }

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


> 
> -    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 ?

if cb() was previously specified to return AVERROR* codes then they
are negative, otherwise if that wasnt specified than above wont
translate its return codes into AVERROR* ones


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141002/0379392a/attachment.asc>


More information about the ffmpeg-devel mailing list