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

[Manfred Georg]

[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 "=".


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



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

Correct.

Manfred