PR #25011 opened by kierank URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25011 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/25011.patch
The second pass of the centre (hv) 6-tap filter computes ((a-b)/4-b+c)/4+c from the first pass's 16-bit sums a, b, c, each in [-5100, 21420] for 8-bit input (plus a bias that cancels here). The partial sum (a-b)/4-b+c spans [-33150, 33150], so it can wrap in a 16-bit lane. The MMX qpel4 and SSE2 hv2 functions already add c with paddsw; the SSSE3 qpel8/qpel16 hv2 used paddw, and they are the versions every CPU with SSSE3 runs. With alternating 0/255 stripes of period 3 in both directions, mc21/mc12/mc22/mc23/mc32 return wrong pixels: a decoder running this asm reconstructs a different picture from the spec and from --disable-asm. Saturating is exact. Saturation happens only when c >= 21038, where the exact result is at least (8192+21038)/64 = 457 and clips to 255, as does the saturated path; or when c <= -4719, where both results are negative and clip to 0. The following add stays inside 16 bits. The assembled objects differ only in the six opcode bytes (three sites, put and avg). checkasm now also runs every qpel function on that stripe pattern, in all six column phases and both polarities. # Summary of changes Briefly describe what this PR does and why. <!-- If this PR requires new FATE test samples, attach them to the PR and list their target paths below (relative to the fate-suite root). Attached filenames must match the sample's filename: ```fate-samples # e.g. vorbis/new-sample.ogg ``` --> >From b7c38e6a62fbf6cc60f260a0d422ca20fdd6edbb Mon Sep 17 00:00:00 2001 From: Kieran Kunhya <[email protected]> Date: Sun, 11 Oct 2026 14:51:24 +0100 Subject: [PATCH] avcodec/x86/h264_qpel_8bit: saturate the SSSE3 hv2 intermediate The second pass of the centre (hv) 6-tap filter computes ((a-b)/4-b+c)/4+c from the first pass's 16-bit sums a, b, c, each in [-5100, 21420] for 8-bit input (plus a bias that cancels here). The partial sum (a-b)/4-b+c spans [-33150, 33150], so it can wrap in a 16-bit lane. The MMX qpel4 and SSE2 hv2 functions already add c with paddsw; the SSSE3 qpel8/qpel16 hv2 used paddw, and they are the versions every CPU with SSSE3 runs. With alternating 0/255 stripes of period 3 in both directions, mc21/mc12/mc22/mc23/mc32 return wrong pixels: a decoder running this asm reconstructs a different picture from the spec and from --disable-asm. Saturating is exact. Saturation happens only when c >= 21038, where the exact result is at least (8192+21038)/64 = 457 and clips to 255, as does the saturated path; or when c <= -4719, where both results are negative and clip to 0. The following add stays inside 16 bits. The assembled objects differ only in the six opcode bytes (three sites, put and avg). checkasm now also runs every qpel function on that stripe pattern, in all six column phases and both polarities. --- libavcodec/x86/h264_qpel_8bit.asm | 6 +++--- tests/checkasm/h264qpel.c | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/libavcodec/x86/h264_qpel_8bit.asm b/libavcodec/x86/h264_qpel_8bit.asm index 3aa1f233a0..2bfe6042a1 100644 --- a/libavcodec/x86/h264_qpel_8bit.asm +++ b/libavcodec/x86/h264_qpel_8bit.asm @@ -682,7 +682,7 @@ cglobal %1_h264_qpel8_hv2_lowpass, 3,4,6 ; dst, tmp, dstStride psubw m0, m1 psraw m0, 2 psubw m0, m1 - paddw m0, m2 + paddsw m0, m2 psraw m0, 2 paddw m0, m2 psraw m0, 6 @@ -730,8 +730,8 @@ cglobal %1_h264_qpel16_hv2_lowpass, 3,4,8 ; dst, tmp, dstStride psraw m3, 2 psubw m0, m1 psubw m3, m4 - paddw m0, m2 - paddw m3, m5 + paddsw m0, m2 + paddsw m3, m5 psraw m0, 2 psraw m3, 2 paddw m0, m2 diff --git a/tests/checkasm/h264qpel.c b/tests/checkasm/h264qpel.c index 6f4a021faf..96e781f78e 100644 --- a/tests/checkasm/h264qpel.c +++ b/tests/checkasm/h264qpel.c @@ -45,6 +45,24 @@ static const uint32_t pixel_mask[3] = { 0xffffffff, 0x01ff01ff, 0x03ff03ff }; } \ } while (0) +/* 0/max stripes with period 3 in both directions take the centre filter's + * intermediate sums to their extremes; phase covers every column offset + * and both polarities */ +static void fill_stripes(uint8_t *buf0, uint8_t *buf1, int width, int bit_depth, int phase) +{ + int max = (1 << bit_depth) - 1; + for (int k = 0; k < BUF_SIZE / SIZEOF_PIXEL; k++) { + int x = k % width, y = k / width; + int v = ((y % 3 == 1) ^ ("010010"[(x + phase) % 6] - '0') ^ (phase >= 6)) ? max : 0; + if (bit_depth == 8) { + buf0[k] = buf1[k] = v; + } else { + AV_WN16A(buf0 + 2 * k, v); + AV_WN16A(buf1 + 2 * k, v); + } + } +} + #define src0 (buf0 + 3 * 2 * 16) /* h264qpel functions read data from negative src pointer offsets */ #define src1 (buf1 + 3 * 2 * 16) @@ -73,6 +91,15 @@ void checkasm_check_h264qpel(void) call_new(dst1, src1, size * SIZEOF_PIXEL); if (memcmp(buf0, buf1, BUF_SIZE) || memcmp(dst0, dst1, BUF_SIZE)) fail(); + for (int phase = 0; phase < 12; phase++) { + fill_stripes(buf0, buf1, size, bit_depth, phase); + call_ref(dst0, src0, size * SIZEOF_PIXEL); + call_new(dst1, src1, size * SIZEOF_PIXEL); + if (memcmp(buf0, buf1, BUF_SIZE) || memcmp(dst0, dst1, BUF_SIZE)) { + fail(); + break; + } + } bench_new(dst1, src1, size * SIZEOF_PIXEL); } } -- 2.52.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
