On Tue, 12 Dec 2017 08:50:01 +0100 Hendrik Leppkes <h.lepp...@gmail.com> wrote:
> On Tue, Dec 12, 2017 at 12:25 AM, Aaron Levinson > <alevinsn_...@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 > >>> <atomnu...@gmail.com> wrote: > >>>> > >>>> Its already done by lockmgr. > >>>> > >>>> Signed-off-by: Rostislav Pehlivanov <atomnu...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel