Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4
Hi, On Mon, Jul 10, 2017 at 5:32 AM, wm4wrote: > On Sun, 9 Jul 2017 22:37:46 -0400 > "Ronald S. Bultje" wrote: > > > Hi, > > > > On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayer > > > wrote: > > > > > Signed-off-by: Michael Niedermayer > > > --- > > > 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(>a, >a, ((uint8_t*)>b) - > > > ((uint8_t*)>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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4
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> wrote: > > > Signed-off-by: Michael Niedermayer > > --- > > 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(>a, >a, ((uint8_t*)>b) - > > ((uint8_t*)>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 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 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. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4
On Sun, 9 Jul 2017 22:37:46 -0400 "Ronald S. Bultje"wrote: > Hi, > > On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayer > wrote: > > > Signed-off-by: Michael Niedermayer > > --- > > 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(>a, >a, ((uint8_t*)>b) - > > ((uint8_t*)>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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4
On Mon, 10 Jul 2017 03:19:51 +0200 Michael Niedermayerwrote: > Signed-off-by: Michael Niedermayer > --- > 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(>a, >a, ((uint8_t*)>b) - > ((uint8_t*)>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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4
Hi, On Sun, Jul 9, 2017 at 9:19 PM, Michael Niedermayerwrote: > Signed-off-by: Michael Niedermayer > --- > 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(>a, >a, ((uint8_t*)>b) - > ((uint8_t*)>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 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4
On Mon, Jul 10, 2017 at 8:19 AM, Michael Niedermayerwrote: > Signed-off-by: Michael Niedermayer > --- > 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(>a, >a, ((uint8_t*)>b) - > ((uint8_t*)>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? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/mpegvideo: Fix all but one tsan warning in fate-vsynth1-mpeg4
Signed-off-by: Michael Niedermayer--- 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(>a, >a, ((uint8_t*)>b) - ((uint8_t*)>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; -- 2.13.0 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel