On Wed, Oct 01, 2014 at 01:01:33PM -0700, Manfred Georg wrote:
> On Tue, Sep 30, 2014 at 8:16 PM, Michael Niedermayer <michae...@gmx.at>
> wrote:
>
> > On Tue, Sep 30, 2014 at 07:34:28PM -0700, Manfred Georg wrote:
> > > Yeah, that seemed a bit odd to me....I guess I get to go correct some
> > > calling code.
> > >
> > > Here is yet another update with a comment which tells you not to use a
> > > static mutex.
> > >
> > > 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 ffmpeg. The register
> > > function is also now explicit about its behavior on failure (it now
> > > unregisters the previous callback and destroys all mutex).
> > > ---
> > > libavcodec/avcodec.h | 25 ++++++++++++++++---------
> > > libavcodec/utils.c | 31 +++++++++++++++++++++----------
> > > 2 files changed, 37 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > index 94e82f7..dae3612 100644
> > > --- a/libavcodec/avcodec.h
> > > +++ b/libavcodec/avcodec.h
> > > @@ -5120,16 +5120,23 @@ 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.
> > > + * 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 unable to perform the op then it should leave the mutex in the
> > same
> > > + * state as when it was called. However, when called with
> > AV_LOCK_DESTROY
> > > + * the mutex will always be assumed to have been successfully destroyed.
> >
> > > + * If av_lockmgr_register succeeds it will return 0, if it fails it will
> > > + * return non-zero and destroy all mutex and unregister all callbacks.
> >
> > we have no positve error codes, the positive values could be used
> > as success like 0 giving us the ability to extend/use them for
> > something in the future
> >
>
> Changed comment. I also changed the code to propagate the error from the
> user function. However, to maintain backwards compatibility, I negate any
> positive values that the user provides.
>
> >
> >
> > > + *
> > > + * @param cb User defined callback. FFmpeg invokes calls to this
> > > + * callback and the previously registered callback during the
> > > + * call to av_lockmgr_register(). The callback will be used
> > to
> > > + * create more than one mutex each of which must be backed
> > > + * by its own underlying locking mechanism (i.e. do not
> > > + * use a single static object to implement your lock manager).
> > > * 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.
> > > */
> > > int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op));
> > >
> > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > > index 778bdc6..717c5b1 100644
> > > --- a/libavcodec/utils.c
> > > +++ b/libavcodec/utils.c
> > > @@ -3457,22 +3457,33 @@ 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;
> > > - codec_mutex = NULL;
> > > - avformat_mutex = NULL;
> > > + // 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);
> > > }
> > >
> > > - lockmgr_cb = cb;
> > > + lockmgr_cb = NULL;
> >
> > > + codec_mutex = NULL;
> > > + avformat_mutex = NULL;
> >
> > why is this moved outside "if (lockmgr_cb)" ?
> >
>
> Because I didn't want to think about it too hard. :) I moved all three
> assignments inside the if statement (after thinking about it for a bit).
>
>
> >
> >
> > >
> > > - if (lockmgr_cb) {
> > > - if (lockmgr_cb(&codec_mutex, AV_LOCK_CREATE))
> > > + if (cb) {
> > > + void *new_codec_mutex = NULL;
> > > + void *new_avformat_mutex = NULL;
> > > + if (cb(&new_codec_mutex, AV_LOCK_CREATE)) {
> > > return -1;
> > > - if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE))
> > > + }
> > > + if (cb(&new_avformat_mutex, AV_LOCK_CREATE))
> > > + {
> > > + // Ignore failures to destroy the newly created mutex.
> > > + cb(&new_codec_mutex, AV_LOCK_DESTROY);
> > > return -1;
> > > + }
> >
> > > + lockmgr_cb = cb;
> > > + codec_mutex = new_codec_mutex;
> > > + avformat_mutex = new_avformat_mutex;
> >
> > these probably should use avpriv_atomic_ptr_cas()
> > to ensure the values where NULL that are replaced.
> > this could provide usefull debug information if someone
> > misuses av_lockmgr_register() like calling it from 2 threads at the
> > same time
> >
>
> Changed assignments to use atomic operation and added some sanity checking
> that the operations actually happened. This is probably overkill,
> considering the disaster which would happen if av_lockmgr_register is
> called concurrently with any function which makes use of locking.
> Personally, I kind of liked it better beforehand, but I don't have much
> experience with threading issues.
>
> 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 ffmpeg. The register
> function is also now explicit about its behavior on failure (it now
> unregisters the previous callback and destroys all mutex).
> Furthermore, the register function is now a bit more careful about
> synchronization issues.
> ---
> libavcodec/avcodec.h | 28 ++++++++++++++++++---------
> libavcodec/utils.c | 53
> +++++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 61 insertions(+), 20 deletions(-)
>
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 94e82f7..f7ad115 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.
> + * 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
> + * unable to perform the op then it should leave the mutex in the same
> + * state as when it was called and return a non-zero value. However,
> + * when called with AV_LOCK_DESTROY the mutex will always be assumed to
> + * have been successfully destroyed. If av_lockmgr_register succeeds
> + * it will return a non-negative value, if it fails it will return a
> + * negative value and destroy all mutex and unregister all callbacks.
> + * av_lockmgr_register is not thread-safe, it must be called from a
> + * single thread before any calls which make use of locking are used.
> + *
> + * @param cb User defined callback. FFmpeg invokes calls to this
> + * callback and the previously registered callback during the
> + * call to av_lockmgr_register(). The callback will be used to
> + * create more than one mutex each of which must be backed
> + * by its own underlying locking mechanism (i.e. do not
> + * use a single static object to implement your lock manager).
> * 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.
> */
> int av_lockmgr_register(int (*cb)(void **mutex, enum AVLockOp op));
>
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 778bdc6..0f1c8ff 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -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
>
> - 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 ?
if cb() was previously specified to return AVERROR* codes then they
are negative, otherwise if that wasnt specified than above wont
translate its return codes into AVERROR* ones
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
