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

[Michael Niedermayer]

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 +, Rostislav Pehlivanov wrote:
> >> Signed-off-by: Rostislav Pehlivanov 
> >> ---
> >>  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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 26 November 2017 at 01:56, Michael Niedermayer 
wrote:

> On Sat, Nov 25, 2017 at 05:01:55PM +, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov 
> > ---
> >  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 11/25/2017 10:56 PM, Michael Niedermayer wrote:
> On Sat, Nov 25, 2017 at 05:01:55PM +, Rostislav Pehlivanov wrote:
>> Signed-off-by: Rostislav Pehlivanov 
>> ---
>>  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Michael Niedermayer]

On Sat, Nov 25, 2017 at 05:01:55PM +, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Rostislav Pehlivanov]

On 25 November 2017 at 18:37, James Almer  wrote:

> On 11/25/2017 2:01 PM, Rostislav Pehlivanov wrote:
> > Signed-off-by: Rostislav Pehlivanov 
> > ---
> >  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 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #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(_thread_counter, 1)
> != 1) {
> > +if (atomic_fetch_add(_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(_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(_thread_counter, -1);
> > +atomic_fetch_add(_thread_counter, -1);
>
> Nit: You could use atomic_fetch_sub() instead of adding -1.
>
> >  if (lockmgr_cb) {
> >  if ((*lockmgr_cb)(_mutex, AV_LOCK_RELEASE))
> >  return -1;
> >
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Fixed locally
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 11/25/2017 2:01 PM, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov 
> ---
>  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 
>  #include 
> +#include 
>  #include 
>  #include 
>  #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(_thread_counter, 1) != 1) {
> +if (atomic_fetch_add(_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(_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(_thread_counter, -1);
> +atomic_fetch_add(_thread_counter, -1);

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

>  if (lockmgr_cb) {
>  if ((*lockmgr_cb)(_mutex, AV_LOCK_RELEASE))
>  return -1;
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel