Skip to content

fix(adm): make the AVX2 and AVX-512 contrast masking wrap like scalar - #1474

Closed
lusoris wants to merge 3 commits into
port/upstream-2026-09from
fix/adm-cm-simd-bitexact
Closed

lusoris wants to merge 3 commits into
port/upstream-2026-09from
fix/adm-cm-simd-bitexact

Conversation

@lusoris

@lusoris lusoris commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Summary

The AVX2 and AVX-512 integer ADM paths gave slightly different scores from the scalar path on content with very large band coefficients, such as full-range noise. They now match scalar bit for bit. Stacked on #1473; review that first.

The scale-0 contrast-masking threshold adds a centre tap of (int16_t)(((ONE_BY_15 * abs(a)) + 2048) >> 12). For |a| above about 15360 the shifted value no longer fits int16, and the scalar reference (which is upstream Netflix/vmaf's C) wraps it. The vector threshold macros in adm_avx2.c and adm_avx512.c kept the 32-bit value; their own scalar edge macros did wrap. So the SIMD paths drifted from scalar exactly where the CSF-weighted band gets that large.

The fix sign-extends each vector tap from its low 16 bits (srai(slli(x, 16), 16)), which is what the (int16_t) conversion does.

Input AVX2 / AVX-512 vs scalar, before after
64x64 high-contrast ramp 7.2e-4 0
640x360 same ramp 2.7e-4 0
576x324 independent full-range noise 2.1e-4 0
Netflix golden pairs (src01, both checkerboards), akiyo multiply 0 0

Scalar scores do not change. SIMD scores on ordinary video don't either, because it never reaches the wrap; the golden pairs above are byte-identical before and after. Upstream's AVX2 and AVX-512 have the same macros.

This closes T-ADM-CM-SIMD-NOISE-NOT-BIT-EXACT-2026-09-18, which #1473 opened.

Type

  • feat — new feature
  • fix — bug fix
  • perf — performance improvement
  • refactor — no behavior change
  • docs — documentation only
  • test — test-only
  • build / ci — tooling / infra
  • port — cherry-pick from upstream Netflix/vmaf
  • sycl / cuda / simd — backend-specific

Checklist

  • Commits follow Conventional Commits (the commit-msg hook enforces this).
  • make format && make lint is green locally.
  • Unit tests pass: meson test -C build.
  • If I touched any SIMD/GPU code path, I ran /cross-backend-diff and the worst ULP is ≤ 2: integer path, now bit-exact with scalar on AVX2 and AVX-512 (table above); NEON unchanged and bit-exact under QEMU.
  • If I touched a feature extractor with SIMD/GPU twins, I either updated every twin or listed the gap under "Known follow-ups" below: NEON has no adm_cm kernel (scalar runs there). On the same noise the CUDA and HIP twins already match scalar (5e-8 and 0); the SYCL twin skips the same 16-bit truncations and drifts by 2.1e-4, which the stacked GPU PR (fix/gpu-adm-tiny-frames) fixes.
  • If I added a new .c / .cpp / .cu / .h / .hpp, it has the appropriate license header (see CONTRIBUTING.md).
  • If this is a breaking change, the commit message uses ! or BREAKING CHANGE: and the migration path is documented below: not breaking.
  • If this PR adds an ADR, the ADR row lives in docs/adr/_index_fragments/<NNNN-slug>.md and the slug is appended to docs/adr/_index_fragments/_order.txt: no ADR, a bug fix.

Bug-status hygiene (ADR-0165)

  • docs/state.md: T-ADM-CM-SIMD-NOISE-NOT-BIT-EXACT-2026-09-18 moves to Recently closed with the cause and the fix.

Netflix golden-data gate (ADR-0024)

  • I did not modify any assertAlmostEqual(...) score in the Netflix golden Python tests.
  • If I believe a golden value must change, I have explained why below AND pinged @lusoris for a CODEOWNERS exception: no golden value moves.

Deep-dive deliverables (ADR-0108)

  • Research digestdocs/research/2063-upstream-sync-2026-09-adm-vif-simd.md, open-questions entry updated with the cause.
  • Decision matrix — no alternatives: only-one-way fix (the scalar path is the reference; the SIMD path must reproduce it).
  • AGENTS.md invariant notecore/src/feature/x86/AGENTS.md.
  • Reproducer / smoke-test command — below.
  • CHANGELOG fragmentchangelog.d/fixed/adm-cm-simd-int16-wrap.md.
  • Rebase notedocs/rebase-notes.md: keep the wrap when a sync touches the vector threshold macros.

Reproducer

meson setup build core -Denable_cuda=false -Denable_sycl=false -Db_lto=false && ninja -C build
meson test -C build test_integer_adm_simd_noise test_integer_adm_simd test_integer_adm_tiny_frames
# end to end: default dispatch, AVX2 alone and scalar must print identical ADM scores
for m in 0 48 65535; do
  build/tools/vmaf -r ref.yuv -d dis.yuv -w 576 -h 324 -p 420 -b 8 --feature adm \
    --no_prediction --precision max --cpumask $m --json -o /tmp/adm_$m.json
done

ref.yuv / dis.yuv: any two independent full-range 8-bit noise clips.

Verified locally

Check Result
test_integer_adm_simd_noise passes; fails at 96x64 with the old macros
Same test on aarch64 (NEON) under QEMU passes
CPU fast suite 139 passed, 0 failed
clang-tidy on the touched C files 0 findings
scripts/dev/preflight.sh clang, MSVC-construct, tidy and cppcheck stages pass. Two stages fail for reasons unrelated to this change, both pre-existing tooling bugs being fixed separately: the 32-bit sweep compiles arm64/adm_neon.c and the x86 SIMD files, which the i686 lane never builds (-Denable_asm=false), and misreads gcc's missing-header message under LC_ALL=C; check_exported_symbols flags ASan's __start_asan_globals / __stop_asan_globals under GNU ld (CI links with lld)
pre-commit, every hook on the changed files passes

Known follow-ups

Lusoris added 3 commits September 19, 2026 23:31
The scale-0 masking threshold adds a centre tap of
(int16_t)(((ONE_BY_15 * abs(a)) + 2048) >> 12). For |a| above about 15360
the shifted value no longer fits int16, and the scalar reference, which is
upstream Netflix/vmaf's C, wraps it. The AVX2 and AVX-512 vector threshold
macros kept the 32-bit value (their scalar edge macros did wrap), so the SIMD
paths drifted from scalar wherever the CSF-weighted band gets that large:
576x324 full-range noise by 2.1e-4 in integer_adm_scale0, a 64x64
high-contrast ramp by 7.2e-4.

Each vector tap is now sign-extended from its low 16 bits, which is the
(int16_t) conversion. AVX2 and AVX-512 equal scalar on those inputs. Scalar
scores do not change, and neither do SIMD scores on the three Netflix golden
pairs or the akiyo multiply pair, which never reach the wrap.

New test_integer_adm_simd_noise scores independent full-range noise at three
sizes with the default dispatch and with AVX2 alone against scalar. It fails
at 96x64 with the old macros, and passes on NEON under QEMU.
@lusoris

lusoris commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Absorbed into the ADM stack train #1507, per your direction to fold this stack the way #1506 was folded.

This PR targeted the one below it in a five-deep stack, so none of the five could merge until every one below had merged and been restacked — five sequential rebase-plus-CI rounds. #1507 is one. Your work is in it unchanged; that PR's description lists the six defects the fold itself surfaced, none of which an individual PR could see, because each gate only looks at the files its own PR touches.

The branch stays on the remote.

@lusoris lusoris closed this Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant