[FFmpeg-devel] [PATCH] lavc/utils: remove unnecessary locking
Hendrik Leppkes
h.leppkes at gmail.com
Tue Dec 12 10:08:51 EET 2017
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.
- Hendrik
More information about the ffmpeg-devel
mailing list