I took a crack at the more general change (it will be arriving shortly).
It's split into two patches, the first is a simplified version of this
change and the second patch more strongly defines the behavior on failure
of both the user supplied function and av_lockmgr_register. Please
consider this patch dead.
Manfred
On Tue, Sep 30, 2014 at 12:13 PM, Michael Niedermayer <michae...@gmx.at>
wrote:
> 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
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
