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

2017-07-10 Thread Ronald S. Bultje
Hi,

On Mon, Jul 10, 2017 at 5:32 AM, wm4  wrote:

> 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

2017-07-10 Thread 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 
> 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

2017-07-10 Thread wm4
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

2017-07-10 Thread wm4
On Mon, 10 Jul 2017 03:19:51 +0200
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;
> +#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

2017-07-09 Thread Ronald S. Bultje
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?

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

2017-07-09 Thread Muhammad Faiz
On Mon, Jul 10, 2017 at 8:19 AM, 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;
> +#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

2017-07-09 Thread Michael Niedermayer
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