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: 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 <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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
