[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