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

[Michael Niedermayer]

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


> + *
> + * @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)" ?


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20141001/5ca3c774/attachment.asc>