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

Nick Lewycky nlewycky at google.com
Mon Nov 20 01:23:47 EET 2017


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 at 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 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: 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 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
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libavcodec-error_resilience.h-Use-C11-atomics-for-ER.patch
Type: application/octet-stream
Size: 9746 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171119/747eedf0/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4847 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20171119/747eedf0/attachment.bin>


More information about the ffmpeg-devel mailing list