[FFmpeg-devel] [PATCH] lavc/utils: remove unnecessary locking

[wm4]

On Tue, 12 Dec 2017 08:50:01 +0100
Hendrik Leppkes <h.leppkes at gmail.com> wrote:

> On Tue, Dec 12, 2017 at 12:25 AM, Aaron Levinson
> <alevinsn_dev at levland.net> wrote:
> > On 12/8/2017 2:27 AM, Michael Niedermayer wrote:  
> >>
> >> On Fri, Dec 08, 2017 at 09:49:25AM +0100, Hendrik Leppkes wrote:  
> >>>
> >>> On Fri, Dec 8, 2017 at 6:09 AM, Rostislav Pehlivanov
> >>> <atomnuker at gmail.com> wrote:  
> >>>>
> >>>> Its already done by lockmgr.
> >>>>
> >>>> Signed-off-by: Rostislav Pehlivanov <atomnuker at gmail.com>
> >>>> ---
> >>>>   libavcodec/utils.c | 6 ------
> >>>>   1 file changed, 6 deletions(-)
> >>>>
> >>>> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> >>>> index baf09119fe..796d24dcbb 100644
> >>>> --- a/libavcodec/utils.c
> >>>> +++ b/libavcodec/utils.c
> >>>> @@ -115,7 +115,6 @@ static int (*lockmgr_cb)(void **mutex, enum AVLockOp
> >>>> op) = NULL;
> >>>>   #endif
> >>>>
> >>>>
> >>>> -static atomic_bool ff_avcodec_locked;
> >>>>   static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0);
> >>>>   static void *codec_mutex;
> >>>>   static void *avformat_mutex;
> >>>> @@ -1943,7 +1942,6 @@ int av_lockmgr_register(int (*cb)(void **mutex,
> >>>> enum AVLockOp op))
> >>>>
> >>>>   int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
> >>>>   {
> >>>> -    _Bool exp = 0;
> >>>>       if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE ||
> >>>> !codec->init)
> >>>>           return 0;
> >>>>
> >>>> @@ -1959,21 +1957,17 @@ int ff_lock_avcodec(AVCodecContext *log_ctx,
> >>>> const AVCodec *codec)
> >>>>                  atomic_load(&entangled_thread_counter));
> >>>>           if (!lockmgr_cb)
> >>>>               av_log(log_ctx, AV_LOG_ERROR, "No lock manager is set,
> >>>> please see av_lockmgr_register()\n");
> >>>> -        atomic_store(&ff_avcodec_locked, 1);
> >>>>           ff_unlock_avcodec(codec);
> >>>>           return AVERROR(EINVAL);
> >>>>       }
> >>>> -    av_assert0(atomic_compare_exchange_strong(&ff_avcodec_locked, &exp,
> >>>> 1));
> >>>>       return 0;
> >>>>   }
> >>>>
> >>>>   int ff_unlock_avcodec(const AVCodec *codec)
> >>>>   {
> >>>> -    _Bool exp = 1;
> >>>>       if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE ||
> >>>> !codec->init)
> >>>>           return 0;
> >>>>
> >>>> -    av_assert0(atomic_compare_exchange_strong(&ff_avcodec_locked, &exp,
> >>>> 0));
> >>>>       atomic_fetch_add(&entangled_thread_counter, -1);
> >>>>       if (lockmgr_cb) {
> >>>>           if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE))
> >>>> --
> >>>> 2.15.1.424.g9478a66081
> >>>>  
> >>>
> >>> These variables never performed any locking, they only existed as a
> >>> sanity check that lock/unlock is always called in pairs. If that is
> >>> really required is up for discussion.  
> >>
> >>
> >> They shuld be usefull to detect some bugs related to locking
> >>
> >> [...]  
> >
> >
> > I don't have the most recent response to this e-mail, from Hendrik, in my
> > inbox anymore, but I spent some time reviewing the patch, and I also don't
> > see any point to making access to ff_avcodec_locked atomic.
> >
> > Prior to patch
> > https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99ab5432543d47a559a461
> > , ff_avcodec_locked was declared as volatile and also specified as an extern
> > in internal.h.  Neither the volatile declaration nor the global exposure of
> > ff_avcodec_locked are necessary.  The patch eliminated the global exposure,
> > but it replaced "volatile" with "atomic".
> >
> > Write access to ff_avcodec_locked is already protected via lockmgr_cb. If,
> > somehow, lockmgr_cb is NULL, which shouldn't happen in practice, the
> > entangled_thread_counter backup approach that immediately follows isn't
> > sufficient as currently implemented to protect writes to ff_avcodec_locked,
> > which makes me wonder why it is there in the first place.  It is possible to
> > use entangled_thread_counter in a way that protects access to
> > ff_avcodec_locked though, and I think that's the intention of the code.
> >
> > I think the correct approach is to mostly restore the code prior to patch
> > https://github.com/FFmpeg/FFmpeg/commit/590136e78da3d091ea99ab5432543d47a559a461
> > but to leave ff_avcodec_locked statically declared and eliminate the
> > volatile designation.  
> 
> I've reverted the commit last night already.
> 
> >
> > On a separate note, this whole approach of using
> > ff_lock_avcodec/ff_unlock_avcodec seems a little hokey to me.  It is
> > effectively using a global lock (via lockmgr_cb) to control access to avctx,
> > which should be local to the calling thread.  As implemented, it prevents
> > two concurrent calls to avcodec_open2() from proceeding simultaneously.  As
> > far as I can tell, the implementation of avcodec_open2() doesn't modify any
> > global structures and only modifies avctx.  I would think that a better
> > approach would be to use a lock that is specific to the avctx object,
> > perhaps one stored in avctx->internal. This approach would also eliminate
> > the codec_mutex memory leak.
> >  
> 
> The Lock doesn't lock access to avctx, it locks access to the AVCodec
> - specifically when opening it. The reason for that is that many
> codecs still have global data which they initialize on opening, so
> opening a codec is locked.
> We've been working on and off to remove any global codec state and
> replacing it with either hardcoded static data or context-state, or
> using  pthread_once, and any codec that is known to not do any unsafe
> global init is flagged with FF_CODEC_CAP_INIT_THREADSAFE, and the
> locking is skipped.
> 
> AVCodecs are const, so they can't contain a mutex (and sometimes data
> is also stored between multiple codecs), and multiple avctx can refer
> to the same AVCodec, so there is no place to store a mutex but
> globally. We would all be happy  to get rid of the lock manager, but
> it isn't quite as simple. Lots of work still remains to check and fix
> all codecs.

Incorrect. AVCodecs are mutable, because otherwise you couldn't make
them part of the codec linked list.