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.
- Hendrik