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 <michae...@gmx.at>
> 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 ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
