| Message ID | 20170710011951.1435-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 10, 2017 at 8:19 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > Signed-off-by: Michael Niedermayer <michael@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; > +#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); Of course this is ugly. What about conditional memcpy, e.g if dst_byte != src_byte then dst_byte = src_byte?
Hi, On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > Signed-off-by: Michael Niedermayer <michael@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? Ronald
On Mon, 10 Jul 2017 03:19:51 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > Signed-off-by: Michael Niedermayer <michael@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; > +#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); > > s->avctx = dst; > s->bitstream_buffer = NULL; Please copy every member on its own, instead of doing a strange range copy, that will break in subtle and unobvious ways if someone reorders or adds fields. I thought we were in a state where we wouldn't add such tricky and easily broken code anymore.
On Sun, 9 Jul 2017 22:37:46 -0400 "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > Hi, > > On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > Signed-off-by: Michael Niedermayer <michael@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.
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@niedermayer.cc> > wrote: > > > Signed-off-by: Michael Niedermayer <michael@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 [...]
Hi, On Mon, Jul 10, 2017 at 5:32 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Sun, 9 Jul 2017 22:37:46 -0400 > "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > > > Hi, > > > > On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayer > <michael@niedermayer.cc> > > wrote: > > > > > Signed-off-by: Michael Niedermayer <michael@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
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; +#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); s->avctx = dst; s->bitstream_buffer = NULL;
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/mpegvideo.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)