On Thu, Oct 02, 2014 at 11:54:31AM -0700, Manfred Georg wrote: > On Wed, Oct 1, 2014 at 5:40 PM, Michael Niedermayer <[email protected]> > 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.
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
