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 <[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
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"
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
