| Message ID | 20171208050910.19173-1-atomnuker@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 8, 2017 at 6:09 AM, Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > Its already done by lockmgr. > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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. - Hendrik
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@gmail.com> wrote: > > Its already done by lockmgr. > > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@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 [...]
On Fri, Dec 8, 2017 at 11:27 AM, Michael Niedermayer <michael@niedermayer.cc> 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@gmail.com> wrote: >> > Its already done by lockmgr. >> > >> > Signed-off-by: Rostislav Pehlivanov <atomnuker@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 > Either this patch or a simple revert of the original commit which introduced the atomics (the code didn't even use any atomics before, and probably doesn't need to because of the lock manager) should be pushed soon. MSVC builds remain broken otherwise. Since Michael sees some use for these checks, I'll push a revert in a day or two if noone objects. - Hendrik
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@gmail.com> wrote: >>> Its already done by lockmgr. >>> >>> Signed-off-by: Rostislav Pehlivanov <atomnuker@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
On Tue, Dec 12, 2017 at 12:25 AM, Aaron Levinson <alevinsn_dev@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@gmail.com> wrote: >>>> >>>> Its already done by lockmgr. >>>> >>>> Signed-off-by: Rostislav Pehlivanov <atomnuker@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
On Tue, 12 Dec 2017 08:50:01 +0100 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Tue, Dec 12, 2017 at 12:25 AM, Aaron Levinson > <alevinsn_dev@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@gmail.com> wrote: > >>>> > >>>> Its already done by lockmgr. > >>>> > >>>> Signed-off-by: Rostislav Pehlivanov <atomnuker@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.
On Tue, Dec 12, 2017 at 9:04 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Tue, 12 Dec 2017 08:50:01 +0100 > Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> On Tue, Dec 12, 2017 at 12:25 AM, Aaron Levinson >> <alevinsn_dev@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@gmail.com> wrote: >> >>>> >> >>>> Its already done by lockmgr. >> >>>> >> >>>> Signed-off-by: Rostislav Pehlivanov <atomnuker@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
On Tue, 12 Dec 2017 09:08:51 +0100 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Tue, Dec 12, 2017 at 9:04 AM, wm4 <nfxjfg@googlemail.com> wrote: > > On Tue, 12 Dec 2017 08:50:01 +0100 > > Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > >> On Tue, Dec 12, 2017 at 12:25 AM, Aaron Levinson > >> <alevinsn_dev@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@gmail.com> wrote: > >> >>>> > >> >>>> Its already done by lockmgr. > >> >>>> > >> >>>> Signed-off-by: Rostislav Pehlivanov <atomnuker@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.
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))
Its already done by lockmgr. Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com> --- libavcodec/utils.c | 6 ------ 1 file changed, 6 deletions(-)