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

[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
>> <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.