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

[Michael Niedermayer]

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


> -               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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220701/921b02d5/attachment.sig>