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

[Michael Niedermayer]

On Sun, Jul 09, 2017 at 10:37:46PM -0400, Ronald S. Bultje 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?

yes


> 
> 
> > +#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...

yes, its just what you asked me about on IRC

<BBB> michaelni: I don’t think we’re asking you to actually fix mpeg4-frame-mt (there’s no issues anyway), more to figure out which fields to sync so the memcpy is unneeded
<BBB> the way I would do that is to copy all fields and remove them until frame-mt+fate breaks or tsan stops complaining

Thats a list copying all fields minus what either broke tsan or was
next to one and on first sight looked like it shouldnt be copied.

From this you can make a list of all ~200 fields that are still copied
if that is desired. (seems like a bad idea unless alot of fields can be
omited and only few remain)

or IMHO better, organize the struct members better so that no correct
addition or change can break it.
That is either have section(s) that are copied and documented as such
or have sub structure(s) that are copied.

My plan was to copy the ranges minus what breaks tsan for 
fate-vsynth1-mpeg4

then we can go over the other tests one by one and adjust the copied
ranges (aka remove areas that trigger tsan warnings)
and go over all fields one by one (there are alot) and adjust the
copying (here also the unused COPY macro likely would come in handy)
(easy for all developers to colaborate on if its in git master)

once we think its all ok we wait and see if any bug reorts come in
from the different set of what is copied (costs us 0 work)

and last once we are certain what fields need to be copied we factor
the code to make it clean and maintainable be that through sub
structure or through reordering of fields
(easy for all developers to colaborate on if its in git master)

It seems my first step/patch is not popular as its ugly which i am the
last to deny.

If its preferred to do all the steps outside git master (minus free
user testing as thats not possible), we can do this too. But i wont do
that alone by myself in some private repo, as i never intended to do
the whole work on my own.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship: All citizens are under surveillance, all their steps and
actions recorded, for the politicians to enforce control.
Democracy: All politicians are under surveillance, all their steps and
actions recorded, for the citizens to enforce control.
-------------- 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/20170710/a90b4c74/attachment.sig>