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

[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
> >>> <michael at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171117/6913ebb0/attachment.sig>