diff mbox

[FFmpeg-devel,v3,2/3] libavcodec/utils.c: simplify avcodec locking with atomics

Message ID CAE9qxYBXnNZ4GoYt26dugsq4heRoXh5MTA_2KF3GjqbJUgyjKA@mail.gmail.com
State New
Headers show

Commit Message

Rostislav Pehlivanov Nov. 25, 2017, 7:48 p.m. UTC 
On 25 November 2017 at 18:40, James Almer <jamrial@gmail.com> wrote:

> On 11/25/2017 2:01 PM, Rostislav Pehlivanov wrote:
> > Also makes it more robust than using volatiles.
> >
> > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> > ---
> >  libavcodec/internal.h |  1 -
> >  libavcodec/utils.c    | 12 ++++++------
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index d3310b6afe..1c54966f37 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -246,7 +246,6 @@ int ff_init_buffer_info(AVCodecContext *s, AVFrame
> *frame);
> >
> >  void ff_color_frame(AVFrame *frame, const int color[4]);
> >
> > -extern volatile int ff_avcodec_locked;
> >  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec);
> >  int ff_unlock_avcodec(const AVCodec *codec);
> >
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index 3a0f3c11f5..96bc9ff4a4 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -114,7 +114,7 @@ static int (*lockmgr_cb)(void **mutex, enum AVLockOp
> op) = NULL;
> >  #endif
> >
> >
> > -volatile int ff_avcodec_locked;
> > +static atomic_bool ff_avcodec_locked;
> >  static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0);
> >  static void *codec_mutex;
> >  static void *avformat_mutex;
> > @@ -1937,6 +1937,7 @@ int av_lockmgr_register(int (*cb)(void **mutex,
> enum AVLockOp op))
> >
> >  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
> >  {
> > +    _Bool exp = 1;
> >      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE ||
> !codec->init)
> >          return 0;
> >
> > @@ -1952,22 +1953,21 @@ 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");
> > -        ff_avcodec_locked = 1;
> > +        atomic_store(&ff_avcodec_locked, 1);
> >          ff_unlock_avcodec(codec);
> >          return AVERROR(EINVAL);
> >      }
> > -    av_assert0(!ff_avcodec_locked);
> > -    ff_avcodec_locked = 1;
> > +    av_assert0(atomic_compare_exchange_strong(&ff_avcodec_locked,
> &exp, 1));
>
> _Bool atomic_compare_exchange_strong( volatile A* obj,
>                                       C* expected, C desired );
>
> "Atomically compares the value pointed to by obj with the value pointed
> to by expected, and if those are equal, replaces the former with desired
> (performs read-modify-write operation).
> Return value: The result of the comparison: true if *obj was equal to
> *exp, false otherwise."
>
> exp should be 0. You need to assert that ff_avcodec_locked == 0, then
> set it to 1.
>

The whole experssion seems fine to me as-is and works as expected.


>
> >      return 0;
> >  }
> >
> >  int ff_unlock_avcodec(const AVCodec *codec)
> >  {
> > +    _Bool exp = 0;
> >      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE ||
> !codec->init)
> >          return 0;
> >
> > -    av_assert0(ff_avcodec_locked);
> > -    ff_avcodec_locked = 0;
> > +    av_assert0(atomic_compare_exchange_strong(&ff_avcodec_locked,
> &exp, 0));
>
> And here exp should be 1.
>
> >      atomic_fetch_add(&entangled_thread_counter, -1);
> >      if (lockmgr_cb) {
> >          if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE))
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Seems like I sent an older version of the patch (had to rebase to fix older
commit), attached patch fixed both.

Comments

Rostislav Pehlivanov Nov. 26, 2017, 2:56 a.m. UTC | #1 
On 25 November 2017 at 19:48, Rostislav Pehlivanov <atomnuker@gmail.com>
wrote:

>
>
> On 25 November 2017 at 18:40, James Almer <jamrial@gmail.com> wrote:
>
>> On 11/25/2017 2:01 PM, Rostislav Pehlivanov wrote:
>> > Also makes it more robust than using volatiles.
>> >
>> > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
>> > ---
>> >  libavcodec/internal.h |  1 -
>> >  libavcodec/utils.c    | 12 ++++++------
>> >  2 files changed, 6 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
>> > index d3310b6afe..1c54966f37 100644
>> > --- a/libavcodec/internal.h
>> > +++ b/libavcodec/internal.h
>> > @@ -246,7 +246,6 @@ int ff_init_buffer_info(AVCodecContext *s, AVFrame
>> *frame);
>> >
>> >  void ff_color_frame(AVFrame *frame, const int color[4]);
>> >
>> > -extern volatile int ff_avcodec_locked;
>> >  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec);
>> >  int ff_unlock_avcodec(const AVCodec *codec);
>> >
>> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
>> > index 3a0f3c11f5..96bc9ff4a4 100644
>> > --- a/libavcodec/utils.c
>> > +++ b/libavcodec/utils.c
>> > @@ -114,7 +114,7 @@ static int (*lockmgr_cb)(void **mutex, enum
>> AVLockOp op) = NULL;
>> >  #endif
>> >
>> >
>> > -volatile int ff_avcodec_locked;
>> > +static atomic_bool ff_avcodec_locked;
>> >  static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0);
>> >  static void *codec_mutex;
>> >  static void *avformat_mutex;
>> > @@ -1937,6 +1937,7 @@ int av_lockmgr_register(int (*cb)(void **mutex,
>> enum AVLockOp op))
>> >
>> >  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>> >  {
>> > +    _Bool exp = 1;
>> >      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE ||
>> !codec->init)
>> >          return 0;
>> >
>> > @@ -1952,22 +1953,21 @@ 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");
>> > -        ff_avcodec_locked = 1;
>> > +        atomic_store(&ff_avcodec_locked, 1);
>> >          ff_unlock_avcodec(codec);
>> >          return AVERROR(EINVAL);
>> >      }
>> > -    av_assert0(!ff_avcodec_locked);
>> > -    ff_avcodec_locked = 1;
>> > +    av_assert0(atomic_compare_exchange_strong(&ff_avcodec_locked,
>> &exp, 1));
>>
>> _Bool atomic_compare_exchange_strong( volatile A* obj,
>>                                       C* expected, C desired );
>>
>> "Atomically compares the value pointed to by obj with the value pointed
>> to by expected, and if those are equal, replaces the former with desired
>> (performs read-modify-write operation).
>> Return value: The result of the comparison: true if *obj was equal to
>> *exp, false otherwise."
>>
>> exp should be 0. You need to assert that ff_avcodec_locked == 0, then
>> set it to 1.
>>
>
> The whole experssion seems fine to me as-is and works as expected.
>
>
>>
>> >      return 0;
>> >  }
>> >
>> >  int ff_unlock_avcodec(const AVCodec *codec)
>> >  {
>> > +    _Bool exp = 0;
>> >      if (codec->caps_internal & FF_CODEC_CAP_INIT_THREADSAFE ||
>> !codec->init)
>> >          return 0;
>> >
>> > -    av_assert0(ff_avcodec_locked);
>> > -    ff_avcodec_locked = 0;
>> > +    av_assert0(atomic_compare_exchange_strong(&ff_avcodec_locked,
>> &exp, 0));
>>
>> And here exp should be 1.
>>
>> >      atomic_fetch_add(&entangled_thread_counter, -1);
>> >      if (lockmgr_cb) {
>> >          if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE))
>> >
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
> Seems like I sent an older version of the patch (had to rebase to fix
> older commit), attached patch fixed both.
>

Pushed, thanks
diff mbox

Patch

From e85ab6b0733e97e501dd24a02c921ed1b6394e3c Mon Sep 17 00:00:00 2001
From: Rostislav Pehlivanov <atomnuker@gmail.com>
Date: Sat, 25 Nov 2017 16:55:44 +0000
Subject: [PATCH] libavcodec/utils.c: simplify avcodec locking with atomics

Also makes it more robust than using volatiles.

Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---
 libavcodec/internal.h |  1 -
 libavcodec/utils.c    | 12 ++++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index d3310b6afe..1c54966f37 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -246,7 +246,6 @@  int ff_init_buffer_info(AVCodecContext *s, AVFrame *frame);
 
 void ff_color_frame(AVFrame *frame, const int color[4]);
 
-extern volatile int ff_avcodec_locked;
 int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec);
 int ff_unlock_avcodec(const AVCodec *codec);
 
diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index 3a0f3c11f5..c733709171 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -114,7 +114,7 @@  static int (*lockmgr_cb)(void **mutex, enum AVLockOp op) = NULL;
 #endif
 
 
-volatile int ff_avcodec_locked;
+static atomic_bool ff_avcodec_locked;
 static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0);
 static void *codec_mutex;
 static void *avformat_mutex;
@@ -1937,6 +1937,7 @@  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;
 
@@ -1952,22 +1953,21 @@  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");
-        ff_avcodec_locked = 1;
+        atomic_store(&ff_avcodec_locked, 1);
         ff_unlock_avcodec(codec);
         return AVERROR(EINVAL);
     }
-    av_assert0(!ff_avcodec_locked);
-    ff_avcodec_locked = 1;
+    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(ff_avcodec_locked);
-    ff_avcodec_locked = 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.0.417.g466bffb3ac