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

Michael Niedermayer michaelni at gmx.at
Thu Oct 2 21:58:41 CEST 2014


On Thu, Oct 02, 2014 at 09:50:12PM +0200, Michael Niedermayer wrote:
> On Thu, Oct 02, 2014 at 11:54:31AM -0700, Manfred Georg wrote:
> > On Wed, Oct 1, 2014 at 5:40 PM, Michael Niedermayer <michaelni at gmx.at>
> > wrote:
> > 
> > > 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
> > >
> > 
> > I reverted to using =.  We can switch to atomic operations in a later patch
> > if that is desired.
> > 
> > 
> > >
> > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > > -    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
> > >
> > 
> > Done.
> > 
> > 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 the library.  The register
> > function is now explicit about its behavior on failure
> > (it unregisters the previous callback and destroys all mutex).
> > ---
> >  libavcodec/avcodec.h | 30 ++++++++++++++++++++----------
> >  libavcodec/utils.c   | 34 ++++++++++++++++++++++------------
> >  2 files changed, 42 insertions(+), 22 deletions(-)
> > 
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 94e82f7..7fb97da 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.
> > - *           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.
> > + * 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
> 
> that for AV_LOCK_DESTROY would _require_ the function to leave a stale
> pointer in place, I dont think thats a good idea.
> ill send a patch to change that

i misread that, its actually ok as is

[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 3
"Rare item" - "Common item with rare defect or maybe just a lie"
"Professional" - "'Toy' made in china, not functional except as doorstop"
"Experts will know" - "The seller hopes you are not an expert"
-------------- 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/a7349c74/attachment.asc>


More information about the ffmpeg-devel mailing list