[FFmpeg-devel] [PATCH] libavcodec/mpegvideo_enc: Fix a chroma mb size error in sse_mb()

[Chen, Wenbin]

> On Fri, Jul 01, 2022 at 01:34:34PM +0800, Wenbin Chen wrote:
> > For 422 frames we should not use hard coded 8 to calculate mb size for
> > uv plane. Chroma shift should be taken into consideration to be
> > compatiple with different sampling format.
> >
> > The error is reported by fate test when av_cpu_max_align() return 64
> > on the platform supporting AVX512. This is a hidden error and it is
> > exposed after commit 17a59a634c39b00a680c6ebbaea58db95594d13d.
> >
> > mpeg2enc has a mechanism to reuse frames. When it computes SSE (sum
> of
> > squared error) on current mb, reconstructed mb will be wrote to the
> > previous mb space, so that the memory can be saved. However if the align
> > is 64, the frame is shared in somewhere else, so the frame cannot be
> > reused and a new frame to store reconstrued data is created. Because the
> > height of mb is wrong when compute sse on 422 frame, starting from the
> > second line of macro block, changed data is read when frame is reused
> > (we need to read row 16 rather than row 8 if frame is 422), and unchanged
> > data is read when frame is not reused (a new frame is created so the
> > original frame will not be changed).
> >
> > That is why commit 17a59a634c39b00a680c6ebbaea58db95594d13d
> exposes this
> > issue, because it add av_cpu_max_align() and this function return 64 on
> > platform supporting AVX512 which lead to creating a frame in mpeg2enc,
> > and this lead to the different outputs.
> >
> > Signed-off-by: Wenbin Chen <wenbin.chen at intel.com>
> > ---
> >  libavcodec/mpegvideo_enc.c             | 29 +++++++++++++------
> >  tests/ref/seek/vsynth_lena-mpeg2-422   | 40 +++++++++++++-------------
> >  tests/ref/vsynth/vsynth1-mpeg2-422     |  8 +++---
> >  tests/ref/vsynth/vsynth2-mpeg2-422     |  8 +++---
> >  tests/ref/vsynth/vsynth3-mpeg2-422     |  8 +++---
> >  tests/ref/vsynth/vsynth_lena-mpeg2-422 |  8 +++---
> >  6 files changed, 56 insertions(+), 45 deletions(-)
> >
> > diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
> > index d6a85a037a..c9d9e2a764 100644
> > --- a/libavcodec/mpegvideo_enc.c
> > +++ b/libavcodec/mpegvideo_enc.c
> > @@ -2558,24 +2558,35 @@ static int sse(MpegEncContext *s, uint8_t *src1,
> uint8_t *src2, int w, int h, in
> >  static int sse_mb(MpegEncContext *s){
> >      int w= 16;
> >      int h= 16;
> > +    int chroma_mb_w = w >> s->chroma_x_shift;
> > +    int chroma_mb_h = h >> s->chroma_y_shift;
> >
> >      if(s->mb_x*16 + 16 > s->width ) w= s->width - s->mb_x*16;
> >      if(s->mb_y*16 + 16 > s->height) h= s->height- s->mb_y*16;
> >
> >      if(w==16 && h==16)
> >        if(s->avctx->mb_cmp == FF_CMP_NSSE){
> 
> > -        return s->mecc.nsse[0](s, s->new_picture->data[0] + s->mb_x * 16 + s-
> >mb_y * s->linesize   * 16, s->dest[0], s->linesize,   16) +
> > +        return s->mecc.nsse[0](s, s->new_picture->data[0] + s->mb_x * 16 +
> s->mb_y * s->linesize * 16,
> > +                               s->dest[0], s->linesize, 16) +
> 
> This doesnt belong in this patch, its not a functional change and makes it
> harder to see what is changed
> 
> please seperate this in a seperate patch if you want to suggest this
> whitespace change
> 
> thx

Ok, I will separate it into two patches and resubmit them.

Thanks

> 
> 
> > -               s->mecc.nsse[1](s, s->new_picture->data[1] + s->mb_x *  8 + s-
> >mb_y * s->uvlinesize *  8, s->dest[1], s->uvlinesize,  8) +
> > -               s->mecc.nsse[1](s, s->new_picture->data[2] + s->mb_x *  8 + s-
> >mb_y * s->uvlinesize *  8, s->dest[2], s->uvlinesize,  8);
> > +               s->mecc.nsse[1](s, s->new_picture->data[1] + s->mb_x *
> chroma_mb_w + s->mb_y * s->uvlinesize * chroma_mb_h,
> > +                               s->dest[1], s->uvlinesize, chroma_mb_h) +
> > +               s->mecc.nsse[1](s, s->new_picture->data[2] + s->mb_x *
> chroma_mb_w + s->mb_y * s->uvlinesize * chroma_mb_h,
> > +                               s->dest[2], s->uvlinesize, chroma_mb_h);
> 
> [...]
> --
> Michael     GnuPG fingerprint:
> 9FF2128B147EF6730BADF133611EC787040B0FAB
> 
> If you fake or manipulate statistics in a paper in physics you will never
> get a job again.
> If you fake or manipulate statistics in a paper in medicin you will get
> a job for life at the pharma industry.