| Message ID | CADbEz-iHr=OZOt4=V718cC=auO_B1_Qf_UM4ng5KO4e4JKDCrg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 > <michael@niedermayer.cc> 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 <inttypes.h> > >> > >> +#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, > >> &s2->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(&s2->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 <nlewycky@google.com> > 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. [...]
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 >> <michael@niedermayer.cc> 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 <inttypes.h> >>>> >>>> +#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, >>>> &s2->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(&s2->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 <nlewycky@google.com> >> 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.
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 >>> <michael@niedermayer.cc> 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 <inttypes.h> >>>>> >>>>> +#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, >>>>> &s2->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(&s2->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 <nlewycky@google.com> >>> 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.
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 > >>> <michael@niedermayer.cc> 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 <inttypes.h> > >>>>> > >>>>> +#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, > >>>>> &s2->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(&s2->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 <nlewycky@google.com> > >>> 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 [...]
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 <michael@niedermayer.cc> 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 > > >>> <michael@niedermayer.cc> 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 <inttypes.h> > > >>>>> > > >>>>> +#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, > > >>>>> &s2->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(&s2->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 <nlewycky@google.com> > > >>> 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 > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
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 <nlewycky@google.com> > 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 <stdatomic.h> > #include <limits.h> > > #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(&s->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(&s->error_count, start_i - end_i - 1); > + atomic_fetch_add(&s->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(&s->error_count, start_i - end_i - 1); > + atomic_fetch_add(&s->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(&s->error_count, start_i - end_i - 1); > + atomic_fetch_add(&s->error_count, start_i - end_i - 1); > } > > if (status & ER_MB_ERROR) { > s->error_occurred = 1; > - avpriv_atomic_int_set(&s->error_count, INT_MAX); > + atomic_store(&s->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(&s->error_count, INT_MAX); > + atomic_store(&s->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(&s->error_count, INT_MAX); > + atomic_store(&s->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(&s->error_count) == 0 || > s->avctx->lowres || > !er_supported(s) || > - s->error_count == 3 * s->mb_width * > + atomic_load(&s->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(&s->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 [...]
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, [...]
From cd8ed0ee35853ae089df3d904846879ce4f00c4a Mon Sep 17 00:00:00 2001 From: Nick Lewycky <nlewycky@google.com> 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 <inttypes.h> +#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, &s2->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(&s2->er.error_count, s2->thread_context[i]->er.error_count); } ret = slice_end(avctx, picture); -- 2.15.0.448.gf294e3d99a-goog