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.c...@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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
