Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-20 Thread Michael Niedermayer 
On Sun, Nov 19, 2017 at 03:23:47PM -0800, Nick Lewycky wrote:
> Sorry-- what should I do now? Wait for another patch to go in first then
> rebase on top of it? Attempt to migrate error_count to C11 atomics myself?
> If I'm migrating, is there a primer on how ffmpeg uses C11 atomics? For
> instance, given an atomic_int, should I assign to it with equality,
> atomic_store, or atomic_store_explicit and if picking atomic_store_explicit
> how clever should I be when picking memory orderings?

If you want the change done quicker than waiting then you can just
update my patch to use C11 atomics of course,

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Elect your leaders based on what they did after the last election, not
based on what they say before an election.



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

Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-20 Thread Michael Niedermayer 
On Sun, Nov 19, 2017 at 03:23:47PM -0800, Nick Lewycky wrote:
[...]
>  error_resilience.c |   22 --
>  error_resilience.h |3 ++-
>  h264_slice.c   |6 --
>  mpeg12dec.c|   11 ++-
>  mpegvideo_enc.c|5 -
>  5 files changed, 28 insertions(+), 19 deletions(-)
> 1978e414145ffde47b44859fe6c8e05b1459741d  
> 0001-libavcodec-error_resilience.h-Use-C11-atomics-for-ER.patch
> From 6a469111d07fb7ddc01f6a19d4bbfee3e20d738e Mon Sep 17 00:00:00 2001
> From: Nick Lewycky 
> Date: Thu, 16 Nov 2017 11:50:38 -0800
> Subject: [PATCH] libavcodec/error_resilience.h: Use C11 atomics for ERContext
>  error_count.
> 
> ---
>  libavcodec/error_resilience.c | 22 --
>  libavcodec/error_resilience.h |  3 ++-
>  libavcodec/h264_slice.c   |  6 --
>  libavcodec/mpeg12dec.c| 11 ++-
>  libavcodec/mpegvideo_enc.c|  5 -
>  5 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/libavcodec/error_resilience.c b/libavcodec/error_resilience.c
> index 0c7f29d171..0da5777b52 100644
> --- a/libavcodec/error_resilience.c
> +++ b/libavcodec/error_resilience.c
> @@ -25,6 +25,7 @@
>   * Error resilience / concealment.
>   */
>  
> +#include 
>  #include 
>  
>  #include "libavutil/atomic.h"
> @@ -807,7 +808,7 @@ void ff_er_frame_start(ERContext *s)
>  
>  memset(s->error_status_table, ER_MB_ERROR | VP_START | ER_MB_END,
> s->mb_stride * s->mb_height * sizeof(uint8_t));
> -s->error_count= 3 * s->mb_num;
> +atomic_store(>error_count, 3 * s->mb_num);

This cannot execute with anything in paralel so shouldnt need any
fancy way of storing i think


>  s->error_occurred = 0;
>  }
>  
> @@ -852,20 +853,20 @@ void ff_er_add_slice(ERContext *s, int startx, int 
> starty,
>  mask &= ~VP_START;
>  if (status & (ER_AC_ERROR | ER_AC_END)) {
>  mask   &= ~(ER_AC_ERROR | ER_AC_END);
> -avpriv_atomic_int_add_and_fetch(>error_count, start_i - end_i - 
> 1);
> +atomic_fetch_add(>error_count, start_i - end_i - 1);
>  }
>  if (status & (ER_DC_ERROR | ER_DC_END)) {
>  mask   &= ~(ER_DC_ERROR | ER_DC_END);
> -avpriv_atomic_int_add_and_fetch(>error_count, start_i - end_i - 
> 1);
> +atomic_fetch_add(>error_count, start_i - end_i - 1);
>  }
>  if (status & (ER_MV_ERROR | ER_MV_END)) {
>  mask   &= ~(ER_MV_ERROR | ER_MV_END);
> -avpriv_atomic_int_add_and_fetch(>error_count, start_i - end_i - 
> 1);
> +atomic_fetch_add(>error_count, start_i - end_i - 1);
>  }
>  
>  if (status & ER_MB_ERROR) {
>  s->error_occurred = 1;
> -avpriv_atomic_int_set(>error_count, INT_MAX);
> +atomic_store(>error_count, INT_MAX);
>  }
>  
>  if (mask == ~0x7F) {
> @@ -878,7 +879,7 @@ void ff_er_add_slice(ERContext *s, int startx, int starty,
>  }
>  
>  if (end_i == s->mb_num)
> -avpriv_atomic_int_set(>error_count, INT_MAX);
> +atomic_store(>error_count, INT_MAX);
>  else {
>  s->error_status_table[end_xy] &= mask;
>  s->error_status_table[end_xy] |= status;
> @@ -893,7 +894,7 @@ void ff_er_add_slice(ERContext *s, int startx, int starty,
>  prev_status &= ~ VP_START;
>  if (prev_status != (ER_MV_END | ER_DC_END | ER_AC_END)) {
>  s->error_occurred = 1;
> -avpriv_atomic_int_set(>error_count, INT_MAX);
> +atomic_store(>error_count, INT_MAX);
>  }
>  }
>  }

> @@ -910,10 +911,11 @@ void ff_er_frame_end(ERContext *s)
>  
>  /* We do not support ER of field pictures yet,
>   * though it should not crash if enabled. */
> -if (!s->avctx->error_concealment || s->error_count == 0||
> +if (!s->avctx->error_concealment   ||
> +atomic_load(>error_count) == 0  ||
>  s->avctx->lowres   ||
>  !er_supported(s)   ||
> -s->error_count == 3 * s->mb_width *
> +atomic_load(>error_count) == 3 * s->mb_width *
>(s->avctx->skip_top + s->avctx->skip_bottom)) {
>  return;
>  }
> @@ -927,7 +929,7 @@ void ff_er_frame_end(ERContext *s)
>  if (   mb_x == s->mb_width
>  && s->avctx->codec_id == AV_CODEC_ID_MPEG2VIDEO
>  && (FFALIGN(s->avctx->height, 16)&16)
> -&& s->error_count == 3 * s->mb_width * (s->avctx->skip_top + 
> s->avctx->skip_bottom + 1)
> +&& atomic_load(>error_count) == 3 * s->mb_width * 
> (s->avctx->skip_top + s->avctx->skip_bottom + 1)
>  ) {
>  av_log(s->avctx, AV_LOG_DEBUG, "ignoring last missing slice\n");
>  return;

like frame start, also frame end should not execute in paralel with
anything, so i think it doesnt need any fancy access functions

[...]

--

Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-19 Thread Nick Lewycky 
Sorry-- what should I do now? Wait for another patch to go in first then
rebase on top of it? Attempt to migrate error_count to C11 atomics myself?
If I'm migrating, is there a primer on how ffmpeg uses C11 atomics? For
instance, given an atomic_int, should I assign to it with equality,
atomic_store, or atomic_store_explicit and if picking atomic_store_explicit
how clever should I be when picking memory orderings?

Also, ERContext also has a non-volatile non-atomic int error_occurred.
Sometimes we update the error_count without adjusting error_occurred. Is
the idea that error_count is shared across threads and error_count is
checked at the end? Given that error_count could wrap around and equal
zero, should I make it atomic too, and set it to 1 every time we set
error_count to non-zero?

I've attached a patch with my initial attempt to use C11 atomics.

Nick

On 17 November 2017 at 12:49, Michael Niedermayer 
wrote:

> On Fri, Nov 17, 2017 at 05:10:17PM -0300, James Almer wrote:
> > On 11/17/2017 4:20 PM, James Almer wrote:
> > > On 11/17/2017 4:16 PM, Michael Niedermayer wrote:
> > >> On Thu, Nov 16, 2017 at 03:07:54PM -0800, Nick Lewycky wrote:
> > >>> Sorry! Let's try an attachment then.
> > >>>
> > >>> On 16 November 2017 at 14:36, Michael Niedermayer
> > >>>  wrote:
> >  On Thu, Nov 16, 2017 at 12:41:32PM -0800, Nick Lewycky wrote:
> > > I initially discovered a signed integer overflow on this line.
> Since
> > > this value is updated in multiple threads, I use an atomic update
> and
> > > as it happens atomic addition is defined to wrap around. However,
> > > there's still a potential bug in that the error_count may wrap
> around
> > > and equal zero again causing problems down the line.
> > >
> > > ---
> > >  libavcodec/mpeg12dec.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > > index d5bc5f21b2..b7c3b5106e 100644
> > > --- a/libavcodec/mpeg12dec.c
> > > +++ b/libavcodec/mpeg12dec.c
> > > @@ -28,6 +28,7 @@
> > >  #define UNCHECKED_BITSTREAM_READER 1
> > >  #include 
> > >
> > > +#include "libavutil/atomic.h"
> > >  #include "libavutil/attributes.h"
> > >  #include "libavutil/imgutils.h"
> > >  #include "libavutil/internal.h"
> > > @@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext
> *avctx,
> > > AVFrame *picture,
> > > >thread_context[0], NULL,
> > > s->slice_count, sizeof(void
> *));
> > >  for (i = 0; i < s->slice_count; i++)
> > > -s2->er.error_count +=
> > > s2->thread_context[i]->er.error_count;
> > > +
> > > avpriv_atomic_int_add_and_fetch(>er.error_count,
> > > s2->thread_context[i]->er.error_count);
> > >  }
> > 
> >  This patch is corrupted by newlines
> > 
> >  [...]
> > 
> >  --
> >  Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7
> 87040B0FAB
> > 
> >  Dictatorship naturally arises out of democracy, and the most
> aggravated
> >  form of tyranny and slavery out of the most extreme liberty. --
> Plato
> > 
> >  ___
> >  ffmpeg-devel mailing list
> >  ffmpeg-devel@ffmpeg.org
> >  http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> > >>
> > >>>  mpeg12dec.c |3 ++-
> > >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> > >>> eed6baa2f9ae5b6fcd227e4b7fa0e87e9ca55b64
> 0001-libavcodec-mpeg12dec-Use-atomic-addition-for-value-u.patch
> > >>> From cd8ed0ee35853ae089df3d904846879ce4f00c4a Mon Sep 17 00:00:00
> 2001
> > >>> From: Nick Lewycky 
> > >>> Date: Thu, 16 Nov 2017 11:50:38 -0800
> > >>> Subject: [PATCH] libavcodec/mpeg12dec: Use atomic addition for value
> updated
> > >>>  in multiple threads.
> > >>
> > >> LGTM, unless theres a new API for doing this, in which case the new
> > >> style should be used.
> > >
> > > Yes, he should use C11 atomics. It's been mentioned to everyone that
> > > submitted code using the old lavu wrappers, including you some days
> ago.
> >
> > To expand on this, I'm aware that the entire error resilience feature
> > needs to be ported to C11 atomics. My point is that, much like i said to
> > you in that patch some days ago, it should be ported before adding new
> > code that will ultimately make the port harder.
>
> Yes, i did not yet had time to update the patch
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Does the universe only have a finite lifespan? No, its going to go on
> forever, its just that you wont like living in it. -- Hiranya Peiri
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
>

Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-17 Thread Michael Niedermayer 
On Fri, Nov 17, 2017 at 05:10:17PM -0300, James Almer wrote:
> On 11/17/2017 4:20 PM, James Almer wrote:
> > On 11/17/2017 4:16 PM, Michael Niedermayer wrote:
> >> On Thu, Nov 16, 2017 at 03:07:54PM -0800, Nick Lewycky wrote:
> >>> Sorry! Let's try an attachment then.
> >>>
> >>> On 16 November 2017 at 14:36, Michael Niedermayer
> >>>  wrote:
>  On Thu, Nov 16, 2017 at 12:41:32PM -0800, Nick Lewycky wrote:
> > I initially discovered a signed integer overflow on this line. Since
> > this value is updated in multiple threads, I use an atomic update and
> > as it happens atomic addition is defined to wrap around. However,
> > there's still a potential bug in that the error_count may wrap around
> > and equal zero again causing problems down the line.
> >
> > ---
> >  libavcodec/mpeg12dec.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> > index d5bc5f21b2..b7c3b5106e 100644
> > --- a/libavcodec/mpeg12dec.c
> > +++ b/libavcodec/mpeg12dec.c
> > @@ -28,6 +28,7 @@
> >  #define UNCHECKED_BITSTREAM_READER 1
> >  #include 
> >
> > +#include "libavutil/atomic.h"
> >  #include "libavutil/attributes.h"
> >  #include "libavutil/imgutils.h"
> >  #include "libavutil/internal.h"
> > @@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx,
> > AVFrame *picture,
> > >thread_context[0], NULL,
> > s->slice_count, sizeof(void *));
> >  for (i = 0; i < s->slice_count; i++)
> > -s2->er.error_count +=
> > s2->thread_context[i]->er.error_count;
> > +
> > avpriv_atomic_int_add_and_fetch(>er.error_count,
> > s2->thread_context[i]->er.error_count);
> >  }
> 
>  This patch is corrupted by newlines
> 
>  [...]
> 
>  --
>  Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
>  Dictatorship naturally arises out of democracy, and the most aggravated
>  form of tyranny and slavery out of the most extreme liberty. -- Plato
> 
>  ___
>  ffmpeg-devel mailing list
>  ffmpeg-devel@ffmpeg.org
>  http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> >>
> >>>  mpeg12dec.c |3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>> eed6baa2f9ae5b6fcd227e4b7fa0e87e9ca55b64  
> >>> 0001-libavcodec-mpeg12dec-Use-atomic-addition-for-value-u.patch
> >>> From cd8ed0ee35853ae089df3d904846879ce4f00c4a Mon Sep 17 00:00:00 2001
> >>> From: Nick Lewycky 
> >>> Date: Thu, 16 Nov 2017 11:50:38 -0800
> >>> Subject: [PATCH] libavcodec/mpeg12dec: Use atomic addition for value 
> >>> updated
> >>>  in multiple threads.
> >>
> >> LGTM, unless theres a new API for doing this, in which case the new
> >> style should be used.
> > 
> > Yes, he should use C11 atomics. It's been mentioned to everyone that
> > submitted code using the old lavu wrappers, including you some days ago.
> 
> To expand on this, I'm aware that the entire error resilience feature
> needs to be ported to C11 atomics. My point is that, much like i said to
> you in that patch some days ago, it should be ported before adding new
> code that will ultimately make the port harder.

Yes, i did not yet had time to update the patch

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

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri


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

Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-17 Thread James Almer 
On 11/17/2017 4:20 PM, James Almer wrote:
> On 11/17/2017 4:16 PM, Michael Niedermayer wrote:
>> On Thu, Nov 16, 2017 at 03:07:54PM -0800, Nick Lewycky wrote:
>>> Sorry! Let's try an attachment then.
>>>
>>> On 16 November 2017 at 14:36, Michael Niedermayer
>>>  wrote:
 On Thu, Nov 16, 2017 at 12:41:32PM -0800, Nick Lewycky wrote:
> I initially discovered a signed integer overflow on this line. Since
> this value is updated in multiple threads, I use an atomic update and
> as it happens atomic addition is defined to wrap around. However,
> there's still a potential bug in that the error_count may wrap around
> and equal zero again causing problems down the line.
>
> ---
>  libavcodec/mpeg12dec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index d5bc5f21b2..b7c3b5106e 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -28,6 +28,7 @@
>  #define UNCHECKED_BITSTREAM_READER 1
>  #include 
>
> +#include "libavutil/atomic.h"
>  #include "libavutil/attributes.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/internal.h"
> @@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx,
> AVFrame *picture,
> >thread_context[0], NULL,
> s->slice_count, sizeof(void *));
>  for (i = 0; i < s->slice_count; i++)
> -s2->er.error_count +=
> s2->thread_context[i]->er.error_count;
> +
> avpriv_atomic_int_add_and_fetch(>er.error_count,
> s2->thread_context[i]->er.error_count);
>  }

 This patch is corrupted by newlines

 [...]

 --
 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

 Dictatorship naturally arises out of democracy, and the most aggravated
 form of tyranny and slavery out of the most extreme liberty. -- Plato

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

>>
>>>  mpeg12dec.c |3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>> eed6baa2f9ae5b6fcd227e4b7fa0e87e9ca55b64  
>>> 0001-libavcodec-mpeg12dec-Use-atomic-addition-for-value-u.patch
>>> From cd8ed0ee35853ae089df3d904846879ce4f00c4a Mon Sep 17 00:00:00 2001
>>> From: Nick Lewycky 
>>> Date: Thu, 16 Nov 2017 11:50:38 -0800
>>> Subject: [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated
>>>  in multiple threads.
>>
>> LGTM, unless theres a new API for doing this, in which case the new
>> style should be used.
> 
> Yes, he should use C11 atomics. It's been mentioned to everyone that
> submitted code using the old lavu wrappers, including you some days ago.

To expand on this, I'm aware that the entire error resilience feature
needs to be ported to C11 atomics. My point is that, much like i said to
you in that patch some days ago, it should be ported before adding new
code that will ultimately make the port harder.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-17 Thread James Almer 
On 11/17/2017 4:16 PM, Michael Niedermayer wrote:
> On Thu, Nov 16, 2017 at 03:07:54PM -0800, Nick Lewycky wrote:
>> Sorry! Let's try an attachment then.
>>
>> On 16 November 2017 at 14:36, Michael Niedermayer
>>  wrote:
>>> On Thu, Nov 16, 2017 at 12:41:32PM -0800, Nick Lewycky wrote:
 I initially discovered a signed integer overflow on this line. Since
 this value is updated in multiple threads, I use an atomic update and
 as it happens atomic addition is defined to wrap around. However,
 there's still a potential bug in that the error_count may wrap around
 and equal zero again causing problems down the line.

 ---
  libavcodec/mpeg12dec.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
 index d5bc5f21b2..b7c3b5106e 100644
 --- a/libavcodec/mpeg12dec.c
 +++ b/libavcodec/mpeg12dec.c
 @@ -28,6 +28,7 @@
  #define UNCHECKED_BITSTREAM_READER 1
  #include 

 +#include "libavutil/atomic.h"
  #include "libavutil/attributes.h"
  #include "libavutil/imgutils.h"
  #include "libavutil/internal.h"
 @@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx,
 AVFrame *picture,
 >thread_context[0], NULL,
 s->slice_count, sizeof(void *));
  for (i = 0; i < s->slice_count; i++)
 -s2->er.error_count +=
 s2->thread_context[i]->er.error_count;
 +
 avpriv_atomic_int_add_and_fetch(>er.error_count,
 s2->thread_context[i]->er.error_count);
  }
>>>
>>> This patch is corrupted by newlines
>>>
>>> [...]
>>>
>>> --
>>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>>
>>> Dictatorship naturally arises out of democracy, and the most aggravated
>>> form of tyranny and slavery out of the most extreme liberty. -- Plato
>>>
>>> ___
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
> 
>>  mpeg12dec.c |3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> eed6baa2f9ae5b6fcd227e4b7fa0e87e9ca55b64  
>> 0001-libavcodec-mpeg12dec-Use-atomic-addition-for-value-u.patch
>> From cd8ed0ee35853ae089df3d904846879ce4f00c4a Mon Sep 17 00:00:00 2001
>> From: Nick Lewycky 
>> Date: Thu, 16 Nov 2017 11:50:38 -0800
>> Subject: [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated
>>  in multiple threads.
> 
> LGTM, unless theres a new API for doing this, in which case the new
> style should be used.

Yes, he should use C11 atomics. It's been mentioned to everyone that
submitted code using the old lavu wrappers, including you some days ago.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-17 Thread Michael Niedermayer 
On Thu, Nov 16, 2017 at 03:07:54PM -0800, Nick Lewycky wrote:
> Sorry! Let's try an attachment then.
> 
> On 16 November 2017 at 14:36, Michael Niedermayer
>  wrote:
> > On Thu, Nov 16, 2017 at 12:41:32PM -0800, Nick Lewycky wrote:
> >> I initially discovered a signed integer overflow on this line. Since
> >> this value is updated in multiple threads, I use an atomic update and
> >> as it happens atomic addition is defined to wrap around. However,
> >> there's still a potential bug in that the error_count may wrap around
> >> and equal zero again causing problems down the line.
> >>
> >> ---
> >>  libavcodec/mpeg12dec.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> >> index d5bc5f21b2..b7c3b5106e 100644
> >> --- a/libavcodec/mpeg12dec.c
> >> +++ b/libavcodec/mpeg12dec.c
> >> @@ -28,6 +28,7 @@
> >>  #define UNCHECKED_BITSTREAM_READER 1
> >>  #include 
> >>
> >> +#include "libavutil/atomic.h"
> >>  #include "libavutil/attributes.h"
> >>  #include "libavutil/imgutils.h"
> >>  #include "libavutil/internal.h"
> >> @@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx,
> >> AVFrame *picture,
> >> >thread_context[0], NULL,
> >> s->slice_count, sizeof(void *));
> >>  for (i = 0; i < s->slice_count; i++)
> >> -s2->er.error_count +=
> >> s2->thread_context[i]->er.error_count;
> >> +
> >> avpriv_atomic_int_add_and_fetch(>er.error_count,
> >> s2->thread_context[i]->er.error_count);
> >>  }
> >
> > This patch is corrupted by newlines
> >
> > [...]
> >
> > --
> > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > Dictatorship naturally arises out of democracy, and the most aggravated
> > form of tyranny and slavery out of the most extreme liberty. -- Plato
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >

>  mpeg12dec.c |3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> eed6baa2f9ae5b6fcd227e4b7fa0e87e9ca55b64  
> 0001-libavcodec-mpeg12dec-Use-atomic-addition-for-value-u.patch
> From cd8ed0ee35853ae089df3d904846879ce4f00c4a Mon Sep 17 00:00:00 2001
> From: Nick Lewycky 
> Date: Thu, 16 Nov 2017 11:50:38 -0800
> Subject: [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated
>  in multiple threads.

LGTM, unless theres a new API for doing this, in which case the new
style should be used.

[...]

-- 
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] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-16 Thread Nick Lewycky 
Sorry! Let's try an attachment then.

On 16 November 2017 at 14:36, Michael Niedermayer
 wrote:
> On Thu, Nov 16, 2017 at 12:41:32PM -0800, Nick Lewycky wrote:
>> I initially discovered a signed integer overflow on this line. Since
>> this value is updated in multiple threads, I use an atomic update and
>> as it happens atomic addition is defined to wrap around. However,
>> there's still a potential bug in that the error_count may wrap around
>> and equal zero again causing problems down the line.
>>
>> ---
>>  libavcodec/mpeg12dec.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
>> index d5bc5f21b2..b7c3b5106e 100644
>> --- a/libavcodec/mpeg12dec.c
>> +++ b/libavcodec/mpeg12dec.c
>> @@ -28,6 +28,7 @@
>>  #define UNCHECKED_BITSTREAM_READER 1
>>  #include 
>>
>> +#include "libavutil/atomic.h"
>>  #include "libavutil/attributes.h"
>>  #include "libavutil/imgutils.h"
>>  #include "libavutil/internal.h"
>> @@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx,
>> AVFrame *picture,
>> >thread_context[0], NULL,
>> s->slice_count, sizeof(void *));
>>  for (i = 0; i < s->slice_count; i++)
>> -s2->er.error_count +=
>> s2->thread_context[i]->er.error_count;
>> +
>> avpriv_atomic_int_add_and_fetch(>er.error_count,
>> s2->thread_context[i]->er.error_count);
>>  }
>
> This patch is corrupted by newlines
>
> [...]
>
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Dictatorship naturally arises out of democracy, and the most aggravated
> form of tyranny and slavery out of the most extreme liberty. -- Plato
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
From cd8ed0ee35853ae089df3d904846879ce4f00c4a Mon Sep 17 00:00:00 2001
From: Nick Lewycky 
Date: Thu, 16 Nov 2017 11:50:38 -0800
Subject: [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated
 in multiple threads.

---
 libavcodec/mpeg12dec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index d5bc5f21b2..b7c3b5106e 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -28,6 +28,7 @@
 #define UNCHECKED_BITSTREAM_READER 1
 #include 
 
+#include "libavutil/atomic.h"
 #include "libavutil/attributes.h"
 #include "libavutil/imgutils.h"
 #include "libavutil/internal.h"
@@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx, AVFrame *picture,
>thread_context[0], NULL,
s->slice_count, sizeof(void *));
 for (i = 0; i < s->slice_count; i++)
-s2->er.error_count += s2->thread_context[i]->er.error_count;
+avpriv_atomic_int_add_and_fetch(>er.error_count, s2->thread_context[i]->er.error_count);
 }
 
 ret = slice_end(avctx, picture);
-- 
2.15.0.448.gf294e3d99a-goog



smime.p7s
Description: S/MIME Cryptographic Signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavcodec/mpeg12dec: Use atomic addition for value updated in multiple threads.

2017-11-16 Thread Michael Niedermayer 
On Thu, Nov 16, 2017 at 12:41:32PM -0800, Nick Lewycky wrote:
> I initially discovered a signed integer overflow on this line. Since
> this value is updated in multiple threads, I use an atomic update and
> as it happens atomic addition is defined to wrap around. However,
> there's still a potential bug in that the error_count may wrap around
> and equal zero again causing problems down the line.
> 
> ---
>  libavcodec/mpeg12dec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
> index d5bc5f21b2..b7c3b5106e 100644
> --- a/libavcodec/mpeg12dec.c
> +++ b/libavcodec/mpeg12dec.c
> @@ -28,6 +28,7 @@
>  #define UNCHECKED_BITSTREAM_READER 1
>  #include 
> 
> +#include "libavutil/atomic.h"
>  #include "libavutil/attributes.h"
>  #include "libavutil/imgutils.h"
>  #include "libavutil/internal.h"
> @@ -2476,7 +2477,7 @@ static int decode_chunks(AVCodecContext *avctx,
> AVFrame *picture,
> >thread_context[0], NULL,
> s->slice_count, sizeof(void *));
>  for (i = 0; i < s->slice_count; i++)
> -s2->er.error_count +=
> s2->thread_context[i]->er.error_count;
> +
> avpriv_atomic_int_add_and_fetch(>er.error_count,
> s2->thread_context[i]->er.error_count);
>  }

This patch is corrupted by newlines

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato


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