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.
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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
