[FFmpeg-devel] [PATCH] lavc/utils: remove unnecessary locking
wm4
nfxjfg at googlemail.com
Tue Dec 12 10:10:39 EET 2017
On Tue, 12 Dec 2017 09:08:51 +0100
Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> On Tue, Dec 12, 2017 at 9:04 AM, wm4 <nfxjfg at googlemail.com> wrote:
> > 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.
>
> Well they are handed around as const everywhere else, anyway.
> avcodec_open2 takes a const AVCodec, avctx contains a const pointer
> only, and so on.
It's legal to cast that away.
Anyway, that probably doesn't even help, and I agree we should just
finish the thread safety changes to get rid of this nonsense.
More information about the ffmpeg-devel
mailing list