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) = 

-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)
          if (!lockmgr_cb)
              av_log(log_ctx, AV_LOG_ERROR, "No lock manager is set, please see 
-        atomic_store(&ff_avcodec_locked, 1);
          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))

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

Reply via email to