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

Michael Niedermayer michaelni at gmx.at
Thu Oct 2 02:40:41 CEST 2014


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


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


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

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- 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/45fad074/attachment.asc>


More information about the ffmpeg-devel mailing list