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

[Aaron Levinson]

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.

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.

Aaron Levinson