diff mbox

[FFmpeg-devel,v3,1/3] lavc/utils.c: use C11 atomics for entangled thread handling

Message ID 20171125170157.32154-1-atomnuker@gmail.com
State Accepted
Commit 3154f4417ab56cfd1e978e6abbb68a9f9f7600ed
Headers show

Commit Message

Rostislav Pehlivanov Nov. 25, 2017, 5:01 p.m. UTC 
Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---
 libavcodec/utils.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

James Almer Nov. 25, 2017, 6:37 p.m. UTC | #1 
On 11/25/2017 2:01 PM, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  libavcodec/utils.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index e50de6e89b..3a0f3c11f5 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -56,6 +56,7 @@
>  #include "version.h"
>  #include <stdlib.h>
>  #include <stdarg.h>
> +#include <stdatomic.h>
>  #include <limits.h>
>  #include <float.h>
>  #if CONFIG_ICONV
> @@ -114,7 +115,7 @@ static int (*lockmgr_cb)(void **mutex, enum AVLockOp op) = NULL;
>  
>  
>  volatile int ff_avcodec_locked;
> -static int volatile entangled_thread_counter = 0;
> +static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0);
>  static void *codec_mutex;
>  static void *avformat_mutex;
>  
> @@ -1944,11 +1945,11 @@ int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
>              return -1;
>      }
>  
> -    if (avpriv_atomic_int_add_and_fetch(&entangled_thread_counter, 1) != 1) {
> +    if (atomic_fetch_add(&entangled_thread_counter, 1)) {
>          av_log(log_ctx, AV_LOG_ERROR,
>                 "Insufficient thread locking. At least %d threads are "
>                 "calling avcodec_open2() at the same time right now.\n",
> -               entangled_thread_counter);
> +               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;
> @@ -1967,7 +1968,7 @@ int ff_unlock_avcodec(const AVCodec *codec)
>  
>      av_assert0(ff_avcodec_locked);
>      ff_avcodec_locked = 0;
> -    avpriv_atomic_int_add_and_fetch(&entangled_thread_counter, -1);
> +    atomic_fetch_add(&entangled_thread_counter, -1);

Nit: You could use atomic_fetch_sub() instead of adding -1.

>      if (lockmgr_cb) {
>          if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE))
>              return -1;
>
Rostislav Pehlivanov Nov. 25, 2017, 6:44 p.m. UTC | #2 
On 25 November 2017 at 18:37, James Almer <jamrial@gmail.com> wrote:

> On 11/25/2017 2:01 PM, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> > ---
> >  libavcodec/utils.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> > index e50de6e89b..3a0f3c11f5 100644
> > --- a/libavcodec/utils.c
> > +++ b/libavcodec/utils.c
> > @@ -56,6 +56,7 @@
> >  #include "version.h"
> >  #include <stdlib.h>
> >  #include <stdarg.h>
> > +#include <stdatomic.h>
> >  #include <limits.h>
> >  #include <float.h>
> >  #if CONFIG_ICONV
> > @@ -114,7 +115,7 @@ static int (*lockmgr_cb)(void **mutex, enum AVLockOp
> op) = NULL;
> >
> >
> >  volatile int ff_avcodec_locked;
> > -static int volatile entangled_thread_counter = 0;
> > +static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0);
> >  static void *codec_mutex;
> >  static void *avformat_mutex;
> >
> > @@ -1944,11 +1945,11 @@ int ff_lock_avcodec(AVCodecContext *log_ctx,
> const AVCodec *codec)
> >              return -1;
> >      }
> >
> > -    if (avpriv_atomic_int_add_and_fetch(&entangled_thread_counter, 1)
> != 1) {
> > +    if (atomic_fetch_add(&entangled_thread_counter, 1)) {
> >          av_log(log_ctx, AV_LOG_ERROR,
> >                 "Insufficient thread locking. At least %d threads are "
> >                 "calling avcodec_open2() at the same time right now.\n",
> > -               entangled_thread_counter);
> > +               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;
> > @@ -1967,7 +1968,7 @@ int ff_unlock_avcodec(const AVCodec *codec)
> >
> >      av_assert0(ff_avcodec_locked);
> >      ff_avcodec_locked = 0;
> > -    avpriv_atomic_int_add_and_fetch(&entangled_thread_counter, -1);
> > +    atomic_fetch_add(&entangled_thread_counter, -1);
>
> Nit: You could use atomic_fetch_sub() instead of adding -1.
>
> >      if (lockmgr_cb) {
> >          if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE))
> >              return -1;
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Fixed locally
Michael Niedermayer Nov. 26, 2017, 1:56 a.m. UTC | #3 
On Sat, Nov 25, 2017 at 05:01:55PM +0000, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  libavcodec/utils.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

LGTM

The whole lock manager should possibly be simplified using some
more standard mutex stuff from C11 or use that as default.

thx

[...]
James Almer Nov. 26, 2017, 2:05 a.m. UTC | #4 
On 11/25/2017 10:56 PM, Michael Niedermayer wrote:
> On Sat, Nov 25, 2017 at 05:01:55PM +0000, Rostislav Pehlivanov wrote:
>> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
>> ---
>>  libavcodec/utils.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> LGTM
> 
> The whole lock manager should possibly be simplified using some
> more standard mutex stuff from C11 or use that as default.
> 
> thx

If you mean threads.h stuff, afaik it's optional and glibc doesn't
include it, so hardly an option today.
Rostislav Pehlivanov Nov. 26, 2017, 2:52 a.m. UTC | #5 
On 26 November 2017 at 01:56, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Sat, Nov 25, 2017 at 05:01:55PM +0000, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> > ---
> >  libavcodec/utils.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
>
> LGTM
>
> The whole lock manager should possibly be simplified using some
> more standard mutex stuff from C11 or use that as default.
>
> thx
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Let us carefully observe those good qualities wherein our enemies excel us
> and endeavor to excel them, by avoiding what is faulty, and imitating what
> is excellent in them. -- Plutarch
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
pushed, thanks
Michael Niedermayer Nov. 26, 2017, 3:14 a.m. UTC | #6 
On Sat, Nov 25, 2017 at 11:05:22PM -0300, James Almer wrote:
> On 11/25/2017 10:56 PM, Michael Niedermayer wrote:
> > On Sat, Nov 25, 2017 at 05:01:55PM +0000, Rostislav Pehlivanov wrote:
> >> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> >> ---
> >>  libavcodec/utils.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > LGTM
> > 
> > The whole lock manager should possibly be simplified using some
> > more standard mutex stuff from C11 or use that as default.
> > 
> > thx
> 
> If you mean threads.h stuff, afaik it's optional and glibc doesn't
> include it, so hardly an option today.

that is sad

[...]
diff mbox

Patch

diff --git a/libavcodec/utils.c b/libavcodec/utils.c
index e50de6e89b..3a0f3c11f5 100644
--- a/libavcodec/utils.c
+++ b/libavcodec/utils.c
@@ -56,6 +56,7 @@ 
 #include "version.h"
 #include <stdlib.h>
 #include <stdarg.h>
+#include <stdatomic.h>
 #include <limits.h>
 #include <float.h>
 #if CONFIG_ICONV
@@ -114,7 +115,7 @@  static int (*lockmgr_cb)(void **mutex, enum AVLockOp op) = NULL;
 
 
 volatile int ff_avcodec_locked;
-static int volatile entangled_thread_counter = 0;
+static atomic_int entangled_thread_counter = ATOMIC_VAR_INIT(0);
 static void *codec_mutex;
 static void *avformat_mutex;
 
@@ -1944,11 +1945,11 @@  int ff_lock_avcodec(AVCodecContext *log_ctx, const AVCodec *codec)
             return -1;
     }
 
-    if (avpriv_atomic_int_add_and_fetch(&entangled_thread_counter, 1) != 1) {
+    if (atomic_fetch_add(&entangled_thread_counter, 1)) {
         av_log(log_ctx, AV_LOG_ERROR,
                "Insufficient thread locking. At least %d threads are "
                "calling avcodec_open2() at the same time right now.\n",
-               entangled_thread_counter);
+               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;
@@ -1967,7 +1968,7 @@  int ff_unlock_avcodec(const AVCodec *codec)
 
     av_assert0(ff_avcodec_locked);
     ff_avcodec_locked = 0;
-    avpriv_atomic_int_add_and_fetch(&entangled_thread_counter, -1);
+    atomic_fetch_add(&entangled_thread_counter, -1);
     if (lockmgr_cb) {
         if ((*lockmgr_cb)(&codec_mutex, AV_LOCK_RELEASE))
             return -1;