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 <mich...@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 > > >>> <mich...@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 <nlewy...@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 > >
0001-libavcodec-error_resilience.h-Use-C11-atomics-for-ER.patch
Description: Binary data
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel