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

[Michael Niedermayer]

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

patch otherwise ok and applied

thanks

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct answer.
-------------- 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/9fd88a78/attachment.asc>