[FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4

[Ronald S. Bultje]

Hi,

On Mon, Jul 10, 2017 at 5:32 AM, wm4 <nfxjfg at googlemail.com> wrote:

> On Sun, 9 Jul 2017 22:37:46 -0400
> "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
>
> > Hi,
> >
> > On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayer
> <michael at niedermayer.cc>
> > wrote:
> >
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >  libavcodec/mpegvideo.c | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/mpegvideo.c b/libavcodec/mpegvideo.c
> > > index e29558b3a2..574b3854e0 100644
> > > --- a/libavcodec/mpegvideo.c
> > > +++ b/libavcodec/mpegvideo.c
> > > @@ -495,7 +495,20 @@ int ff_mpeg_update_thread_context(AVCodecContext
> > > *dst,
> > >      // in that case dst may need a reinit
> > >      if (!s->context_initialized) {
> > >          int err;
> > > -        memcpy(s, s1, sizeof(MpegEncContext));
> > > +#define COPY(x) s->x = s1->x;
> > >
> >
> > Unused?
> >
> >
> > > +#define COPYR(a, b) memcpy(&s->a, &s1->a, ((uint8_t*)&s->b) -
> > > ((uint8_t*)&s->a))
> > > +        COPYR(h263_aic,                         qscale);
> > > +        COPYR(lambda,                           mv_dir);
> > > +        COPYR(no_rounding,                      dest);
> > > +        COPYR(mb_index2xy,                      resync_mb_x);
> > > +        COPYR(next_p_frame_damaged,             h263_aic_dir);
> > > +        COPYR(h263_slice_structured,            mcsel);
> > > +        COPYR(quant_precision,                  first_slice_line);
> > > +        COPYR(flipflop_rounding,                gb);
> > > +        COPYR(gop_picture_number,               picture_structure);
> > > +        COPYR(picture_structure,                pblocks);
> > > +        COPYR(decode_mb,                        er);
> > > +        COPYR(error_rate,                       noise_reduction);
> >
> >
> > This is very obscure... The obscure part is what happens when fields get
> > (through refactoring) reordered or new fields get inserted...
> >
> > I also appear to remember that we used to use this same pattern for h264
> > also, but we don't anymore. Does anyone remember why?
>
> Do you mean 0ba471d7d864c712f45d7ac6aca4829aba025adc?
>
>     h264: eliminate copy_fields
>
>     It is very fragile against fields being moved and hides what is
> actually
>     being copied. Copy all the fields explicitly instead.
>
> Seems justification enough for me.


Ah, yes, I tried to look for it but couldn't find it, thanks!

Ronald