On Mon, Sep 29, 2014 at 09:57:02PM -0700, Manfred Georg wrote: > Answers inline. > > On Mon, Sep 29, 2014 at 7:07 PM, Michael Niedermayer <[email protected]> > wrote: > > > On Mon, Sep 29, 2014 at 02:41:38PM -0700, Manfred Georg wrote: > > > A badly behaving user provided mutex manager (such as that in OpenCV) > > may not reset the mutex to NULL on destruction. This can cause a problem > > for a later mutex manager (which may assert that the mutex is NULL before > > creating). > > > --- > > > libavcodec/utils.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > > > index 9eb2b5b..a1f7cfc 100644 > > > --- a/libavcodec/utils.c > > > +++ b/libavcodec/utils.c > > > @@ -3457,18 +3457,21 @@ 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)) > > > + void *old_codec_mutex = codec_mutex; > > > + void *old_avformat_mutex = avformat_mutex; > > > + int failure; > > > + codec_mutex = NULL; > > > + avformat_mutex = NULL; > > > + failure = lockmgr_cb(&old_codec_mutex, AV_LOCK_DESTROY); > > > + if (lockmgr_cb(&old_avformat_mutex, AV_LOCK_DESTROY) || failure) > > > return -1; > > > > why do you use temporary variables ? > > wouldnt simply setting them to NULL afterwards achieve the same ? > > > > The behavior on failure wouldn't be the same. In the original code the
i meant this:
failure = lockmgr_cb(&codec_mutex, AV_LOCK_DESTROY);
failure |= lockmgr_cb(&avformat_mutex, AV_LOCK_DESTROY);
codec_mutex = NULL;
avformat_mutex = NULL;
if (failure)
return -1;
> second call is never made if the first one fails. I feel like this
> implementation is at least a bit more symmetric. Really, we'd want to
> define some sane semantics on failure (both for this function and the lock
> manager function provided by the user) and specify what happens in the
> function comment in the header (without that we could argue in circles for
> hours about which implementation is less incorrect), but that seemed out of
> scope for this change.
>
>
> >
> > > }
> > >
> > > lockmgr_cb = cb;
> > >
> > > if (lockmgr_cb) {
> > > - if (lockmgr_cb(&codec_mutex, AV_LOCK_CREATE))
> > > - return -1;
> > > - if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE))
> > > + int failure = lockmgr_cb(&codec_mutex, AV_LOCK_CREATE);
> > > + if (lockmgr_cb(&avformat_mutex, AV_LOCK_CREATE) || failure)
> > > return -1;
> >
> > why, when the creation of the first lock manager fails you try to
> > create the 2nd one ?
> >
> > isnt it more logic to leave the state as it was before the call on
> > failure, instead of trying to half initialize things ?
> > that would also require to first create the 2 new lock managers
> > and then when both succeeded destroy the old
> > though thats orthogonal to the stated intend of teh patch and
> > should, if done, probably be in a seperate patch
> >
> >
> As far as I know, it is never specified what state the lock manager
> function should leave things in on failure. You may assume that failure
> means a mutex wasn't created while I may assume that it may have been
> anyway. This implementation seemed less likely to leave things in a bad
> state given that we don't know what the lock manager function is actually
> doing. At least both mutex will have had the same thing done on
> them...hopefully...probably.
hopefully...probably ?
also before the patch failure means either both locks havnt been
created or the 2nd hasnt been
after the patch a failure means either both locks havnt been
created or the 1st or the 2nd hasnt been
maybe its just me but i think its better to have the locks in a
deterministic state on failure
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
