fix(core): harvest and fix nine stale upstream Netflix/vmaf reports (ADR-1166) - #1223
Conversation
Independent adversarial review — 20 findingsA multi-agent review attacked this PR's claims rather than confirming them, with a [HIGH]
|
Three HIGH findings fixedThe three HIGH items from the review above are fixed and pushed. Each was reproduced before being changed, and the new test fails on the pre-fix tree.
Evidence. Under New test
Still open: the twelve MEDIUM and LOW findings from the review — the MSVC ARM64 clz guard, the 🤖 Generated with Claude Code |
3fbc129 to
80910f1
Compare
80910f1 to
1c136d7
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /* maps that to -ENOMEM. Handing it a heap-allocated EMPTY dictionary */ | ||
| /* reaches that branch deterministically, with no malloc-fail */ | ||
| /* injection: pre-fix it `return`ed straight out and leaked the */ | ||
| /* caller's dictionary (the defect Netflix/vmaf#1242 reports); */ |
455d767 to
dc5fef5
Compare
37228e7 to
fa9ef2c
Compare
|
The two red legs here ( Root cause, reproduced exactly: It looked like this PR's fault because of a second gap: the CI impact planner selected no Intel LLVM job in any of the last eight master runs, so master was green on legs nobody ran, and the first PR broad enough to select them absorbed the blame. Fix is in #1266 (drops |
test_output::test_vmaf_version failed the Intel LLVM and macOS Clang legs of build.yml on PR #1223, a PR that does not touch test_output.c, does not touch any version surface, and whose only meson change is a CUDA include path. The test is byte-identical on master. The failure is not that PR's. Root cause. core/include/meson.build derives VMAF_VERSION from `git describe --tags --long --match 'v*.*.*' --always`. With --always, git exits 0 even when no matching tag is reachable and prints a bare abbreviated object name, which meson substitutes into vcs_version.h verbatim. build.yml checks out at the actions/checkout default fetch-depth of 1, which fetches no tags, so every build on that workflow has been stamping a commit abbreviation where a version belongs — `vmaf --version`, the JSON and XML `version` field, and vmaf_version() all reported it. That is silent until the abbreviation happens to contain no ASCII digit, which is what test_vmaf_version asserts against. #1223's merge commit was abafdfc, abbreviating to "abafdfc" — no digit. Roughly one commit in a thousand, (6/16)^7, so the defect sat in tree and then failed an unrelated PR. Reproduced exactly: a depth-1 tagless fetch of that commit run through mesonbuild's vcstagger yields VMAF_VERSION="abafdfc" (test fails) before the change and "3.2.1" after. Dropping --always makes git exit non-zero in precisely those cases, and vcstagger.py substitutes the vcs_tag fallback on any exception from the subprocess. The fallback is now spelled out rather than left implicit, since it is the entire tagless path. When a tag is reachable the --long form still embeds the commit, so no provenance is lost: v3.1.0-2417-g1ee6ebde42. build.yml also moves to fetch-depth 0, matching libvmaf-build-matrix.yml, so CI exercises the tagged path rather than the fallback. Two coverage gaps let this hide. The CI impact planner selected no Intel LLVM job in any of the last eight master runs, so master's own build.yml legs were not exercising the test; and the Windows job — the third leg — runs an explicit whitelist that omitted test_output. Windows now runs it (the test already carries a GetTempPathA/GetTempFileNameA path for that platform). Its `for` loop also gained `|| exit /b 1`: GitHub runs `shell: cmd` as `%ComSpec% /D /E:ON /V:OFF /S /C "CALL ..."`, so the step result was the errorlevel of the last executable alone and a failure in any earlier test was discarded. /V:OFF rules out !ERRORLEVEL!, so `||` is the portable form. scripts/ci/check-vcs-version-not-bare-sha.sh keeps --always out: it brackets the vcs_tag call, strips comments so prose may discuss the flag, and fails if --always returns, if the explicit fallback goes missing, or if --match is dropped. Negative-tested on all three; positive-tested on the restored file and on a comment mentioning the flag outside the call. Upstream Netflix carries the --always form, so this file will conflict on a sync — the gate turns a careless resolution into a build failure rather than a silently wrong version. Wired into `make lint-sh`. Verified: CPU build clean, fast suite 106 Ok / 0 Fail, test_output 13/13, `vmaf --version` prints v3.1.0-2417-g1ee6ebde42, pre-commit clean on all touched files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_output::test_vmaf_version failed the Intel LLVM and macOS Clang legs of build.yml on PR #1223, a PR that does not touch test_output.c, does not touch any version surface, and whose only meson change is a CUDA include path. The test is byte-identical on master. The failure is not that PR's. Root cause. core/include/meson.build derives VMAF_VERSION from `git describe --tags --long --match 'v*.*.*' --always`. With --always, git exits 0 even when no matching tag is reachable and prints a bare abbreviated object name, which meson substitutes into vcs_version.h verbatim. build.yml checks out at the actions/checkout default fetch-depth of 1, which fetches no tags, so every build on that workflow has been stamping a commit abbreviation where a version belongs — `vmaf --version`, the JSON and XML `version` attribute, and vmaf_version() all reported it. That is silent until the abbreviation happens to contain no ASCII digit, which is what test_vmaf_version asserts against. #1223's merge commit was abafdfc, abbreviating to "abafdfc" — no digit. Roughly one commit in a thousand, (6/16)^7. Reproduced exactly: a depth-1 tagless fetch of that commit run through mesonbuild's vcstagger yields VMAF_VERSION="abafdfc" before this change and "3.2.1" after. Master is not exempt, it has only been lucky. Its build.yml legs do run and are green, but all 20 most recent master commits abbreviate with a digit; the defect fires on whichever commit first abbreviates to all letters. PR merge commits simply roll the dice more often. Dropping --always makes git exit non-zero in precisely those cases, and vcstagger.py substitutes the vcs_tag fallback on any exception from the subprocess. The fallback is now spelled out rather than left implicit, since it is the entire tagless path. When a tag is reachable the --long form still embeds the commit, so no provenance is lost: v3.1.0-2417-g1ee6ebde42. build.yml also moves to fetch-depth 0, matching libvmaf-build-matrix.yml, so CI exercises the tagged path rather than the fallback. A real coverage gap kept the third leg quiet. The Windows job runs an explicit whitelist that omitted test_output; it now runs it, and the test already carries a GetTempPathA/GetTempFileNameA path for that platform. That loop also gained `|| exit /b 1`: GitHub runs `shell: cmd` as `%ComSpec% /D /E:ON /V:OFF /S /C "CALL ..."`, so the step result was the errorlevel of the last executable alone and a failure in any earlier test was discarded outright. /V:OFF rules out !ERRORLEVEL!, so `||` is the portable check. scripts/ci/check-vcs-version-not-bare-sha.sh keeps --always out: it brackets the vcs_tag call, strips comments so prose may discuss the flag, and fails if --always returns, if the explicit fallback goes missing, or if --match is dropped. Negative-tested on all three; positive-tested on the restored file and on a comment mentioning the flag outside the call. Upstream Netflix carries the --always form (verified against upstream/master:libvmaf/include/meson.build), so this file will conflict on a sync — the gate turns a careless resolution into a build failure rather than a silently wrong version. Wired into `make lint-sh`. Verified: CPU build clean, fast suite 106 Ok / 0 Fail, test_output 13/13, `vmaf --version` prints v3.1.0-2417-g1ee6ebde42, pre-commit clean on all touched files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correction to my comment above: the claim that the CI impact planner had skipped master's Intel LLVM legs was wrong — I conflated The root-cause finding is unchanged and, if anything, stronger: master passes only because all 20 most recent master commits abbreviate with a digit. The defect is latent on master, not absent from it — it fires on whichever commit first abbreviates to seven letters. A PR merge commit is a fresh object every time the PR or its base moves, so PRs simply roll the dice more often, which is why one surfaced it first. This PR is still not the cause, and #1266 is still the fix. |
…#1266) * fix(build): stop VMAF_VERSION degrading to a bare commit abbreviation test_output::test_vmaf_version failed the Intel LLVM and macOS Clang legs of build.yml on PR #1223, a PR that does not touch test_output.c, does not touch any version surface, and whose only meson change is a CUDA include path. The test is byte-identical on master. The failure is not that PR's. Root cause. core/include/meson.build derives VMAF_VERSION from `git describe --tags --long --match 'v*.*.*' --always`. With --always, git exits 0 even when no matching tag is reachable and prints a bare abbreviated object name, which meson substitutes into vcs_version.h verbatim. build.yml checks out at the actions/checkout default fetch-depth of 1, which fetches no tags, so every build on that workflow has been stamping a commit abbreviation where a version belongs — `vmaf --version`, the JSON and XML `version` attribute, and vmaf_version() all reported it. That is silent until the abbreviation happens to contain no ASCII digit, which is what test_vmaf_version asserts against. #1223's merge commit was abafdfc, abbreviating to "abafdfc" — no digit. Roughly one commit in a thousand, (6/16)^7. Reproduced exactly: a depth-1 tagless fetch of that commit run through mesonbuild's vcstagger yields VMAF_VERSION="abafdfc" before this change and "3.2.1" after. Master is not exempt, it has only been lucky. Its build.yml legs do run and are green, but all 20 most recent master commits abbreviate with a digit; the defect fires on whichever commit first abbreviates to all letters. PR merge commits simply roll the dice more often. Dropping --always makes git exit non-zero in precisely those cases, and vcstagger.py substitutes the vcs_tag fallback on any exception from the subprocess. The fallback is now spelled out rather than left implicit, since it is the entire tagless path. When a tag is reachable the --long form still embeds the commit, so no provenance is lost: v3.1.0-2417-g1ee6ebde42. build.yml also moves to fetch-depth 0, matching libvmaf-build-matrix.yml, so CI exercises the tagged path rather than the fallback. A real coverage gap kept the third leg quiet. The Windows job runs an explicit whitelist that omitted test_output; it now runs it, and the test already carries a GetTempPathA/GetTempFileNameA path for that platform. That loop also gained `|| exit /b 1`: GitHub runs `shell: cmd` as `%ComSpec% /D /E:ON /V:OFF /S /C "CALL ..."`, so the step result was the errorlevel of the last executable alone and a failure in any earlier test was discarded outright. /V:OFF rules out !ERRORLEVEL!, so `||` is the portable check. scripts/ci/check-vcs-version-not-bare-sha.sh keeps --always out: it brackets the vcs_tag call, strips comments so prose may discuss the flag, and fails if --always returns, if the explicit fallback goes missing, or if --match is dropped. Negative-tested on all three; positive-tested on the restored file and on a comment mentioning the flag outside the call. Upstream Netflix carries the --always form (verified against upstream/master:libvmaf/include/meson.build), so this file will conflict on a sync — the gate turns a careless resolution into a build failure rather than a silently wrong version. Wired into `make lint-sh`. Verified: CPU build clean, fast suite 106 Ok / 0 Fail, test_output 13/13, `vmaf --version` prints v3.1.0-2417-g1ee6ebde42, pre-commit clean on all touched files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(state): record T-VCS-VERSION-BARE-SHA and the AGENTS.md invariant Adds the docs/state.md row + Updated line for the version-string defect (ADR-0165 / CLAUDE.md §12 r13) and the scripts/ci/AGENTS.md section covering the three properties check-vcs-version-not-bare-sha.sh enforces and why build.yml's fetch-depth: 0 is load-bearing. The row records that master's build.yml legs do run and are green, and that this is luck rather than coverage: every recent master commit abbreviated with a digit, so the defect is latent on master rather than absent from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(changelog): render the Unreleased block from the new fragments scripts/release/concat-changelog-fragments.sh --check gates on drift between CHANGELOG.md and changelog.d/; the two fragments this PR adds have to be rendered in the same commit range. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ADR-1166) Verify a batch of long-open Netflix/vmaf issues against this tree — the fork diverged far enough (ADR-0700's `libvmaf/` -> `core/` rename, several C-to-C++ conversions, four fork-added GPU backends) that an upstream report is neither automatically applicable nor automatically stale — and fix the subset that still bites. The full triage table, including the ALREADY-FIXED and NOT-APPLICABLE verdicts, is in docs/research/1166-upstream-issue-harvest-2026-09-03.md. Memory safety, all reachable from the public C API today: * reported upstream as Netflix/vmaf#1582 (mirror half also Netflix/vmaf#1581): the reflect-101 fold in convolution_edge_s / _sq_s / _xy_s bounced an out-of-range tap exactly once, which only lands in range for size >= radius + 1; and convolution_x_c_s / convolution_y_c_s derived the trailing border bound as dim - (filter_width - radius), which goes negative for a plane narrower than the filter and starts the trailing loop at a negative index (heap underflow write). Two live paths reached those sizes: `--feature float_vif` on 9..15px frames (the guard admitted >= 9 but the four-scale ladder needs >= 16 — the binding constraint is scale 3), and `--feature float_motion` with motion_add_uv on a 4x4 YUV420P frame (the guard validated luma only while the blur runs at the 2x2 chroma dimensions). The fold is now iterative and bit-identical to the single bounce for every in-contract size; the borders are clamped; float_vif derives its floor from vif_get_min_dim(kernelscale); float_motion validates every plane it convolves. * reported upstream as Netflix/vmaf#1580: the three fork-added Metal motion extractors were written after the Research-0094 sweep and never got the min-dim guard, so a 1- or 2-pixel-tall frame read out of bounds on device. Correctness and contracts: * reported upstream as Netflix/vmaf#1242: vmaf_model_feature_overload() leaked the caller's dictionary on the -ENOMEM path, vmaf_model_collection_feature_overload() swallowed the copy error and dereferenced *model_collection unchecked, and feature.h / model.h documented opposite ownership rules — one of the two readings a latent double free. All three public headers now state the implemented contract identically. Supersedes ADR-0806. * reported upstream as Netflix/vmaf#1551, which retracts Netflix/vmaf#1422: the MSVC __builtin_clz shim used __lzcnt, which emits LZCNT with no runtime gate and silently retires as BSR on any x86-64 without ABM — wrong VIF and ADM log2 shifts, no fault, and invisible to CI because every hosted Windows runner has LZCNT. Now _BitScanReverse, with an architecture guard so an MSVC ARM64 leg compiles. User-visible surfaces: * reported upstream as Netflix/vmaf#743: the CLI wrote UTF-8 braille and a CSI erase to a Windows console it never configured, so the progress line was mojibake under every default code page. The console is switched to UTF-8 + VT for the run and restored on exit, with an ASCII fallback. * reported upstream as Netflix/vmaf#1178: libvmaf.pc omitted the C++ runtime, so `pkg-config --static --libs libvmaf` produced a link line that fails with hundreds of undefined references — the reason ADR-0198's static FFmpeg reproducer had to add -lstdc++ by hand. * reported upstream as Netflix/vmaf#1573: the nvcc fatbin include list used relative paths that stopped resolving at ADR-0700, and three shell-driven tool tests declared no `depends`, so a subset run built nothing and exited 127. Behaviour changes: float_vif now rejects frames below 16px in either dimension, and float_motion with motion_add_uv rejects sub-minimum chroma planes. Both convert previously undefined behaviour into a documented -EINVAL. Regression tests: core/test/test_convolution_edge_small.c (NaN-poisoned guard buffers; fails pre-fix), core/test/test_compat_clz.c, core/test/test_model_feature_overload_ownership.c, core/test/test_spinner.cpp, scripts/ci/check-msvc-clz-shim.sh (fails pre-fix), plus extended cases in test_motion_min_dim.c and test_float_vif_min_dim.c, and a real static link in the libvmaf-build-matrix pkg-config step. Netflix golden scores unchanged: 76.66744 / 35.070245 / 7.985956 (271 passed, 12 skipped). Confirmed but not batched, one docs/state.md row each: Netflix/vmaf#1564, #930, off-by-one found while triaging Netflix/vmaf#1580. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…holes Three HIGH findings from an independent adversarial review of this harvest. Each was reproduced before being fixed, and the regression test fails on the pre-fix tree. 1. The Netflix/vmaf#1582 border clamp landed only on the scalar path. convolution_f32_c_s dispatches straight into convolution_f32_avx_s whenever AVX2 is present — every CI runner and the dev workstation — so the clamp this PR added was dead code on x86. The AVX2 and AVX-512 twins derive the same `height - radius` split at three sites each and kept it unclamped. For a plane shorter than the radius that value is negative, so the trailing border loop starts at a negative row and the leading one runs past the end. Both are heap WRITES, not reads. All six sites now share the scalar clamp, which moved into convolution_internal.h as a static inline. 2. motion_filter_size=1 bypassed the minimum-dimension guard entirely. motion_check_min_dim gated its check on `effective_filter_size > 1`, but motion_blur_plane keeps filter_size = 5 for that value and only swaps in the FILTER_5_NO_OP_s coefficients, so the radius is still 2. A 1-row plane therefore reached the convolution in (1) through a documented public option with range 0..9. The guard now mirrors motion_blur_plane exactly: 3 taps only for motion_filter_size == 3, otherwise 5. 3. Odd-height 4:2:0 chroma planes were under-allocated by one row. motion_chroma_heights used `h / 2` while picture.c and the guard both use the ceiling `(h + 1) >> 1`, so motion_copy_and_blur overran ref, tmp and every MOTION_BLUR_RING blur buffer for both U and V. Even heights were unaffected, which is why neither golden fixture caught it. Also removes a stray `} // namespace` inside the _WIN32 block of core/tools/vmaf.cpp that closed a namespace never opened. It broke every Windows build and was invisible on Linux, where the preprocessor drops the block. This PR is a draft and drafts run no CI here, so nothing had compiled it. The file now has exactly one namespace opener and one closer, neither inside any conditional. New test core/test/test_motion_convolution_oob.c drives float_motion through the public vmaf_read_pictures entry point, because no existing test could reach the dispatched SIMD path: test_motion_min_dim only calls init(), and test_convolution_edge_small calls the scalar kernels directly. Verified both ways. The new test fails on the pre-fix tree and passes after. Under -Db_sanitize=address the pre-fix tree reports "heap-buffer-overflow ... WRITE of size 4" in convolution_f32_avx_s reached from vmaf_read_pictures, "0 bytes after 32-byte region" — the single-row buffer. Post-fix: zero sanitizer reports, the guard returns -EINVAL, and the odd-height case scores cleanly. meson test --suite=fast: 111 Ok, 0 Fail. Netflix golden gate: 271 passed, 12 skipped, 0 failed. The twelve MEDIUM and LOW findings from the same review are not addressed here and remain open on the PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… two leaks Fourth MEDIUM finding from the adversarial review of this harvest. The Netflix/vmaf#1242 contract was still stated three different ways, and one of them was a double free. <libvmaf/feature.h> and docs/api/index.md both claimed that an unknown feature_name never consumes the dictionary. That is true of vmaf_use_feature, which resolves the name against the global extractor registry and returns -EINVAL before touching it. It is NOT true of vmaf_model_feature_overload, which matches feature_name against the features of one particular model: a name that matches nothing there is not an error, it is a successful no-op returning 0, and the tail vmaf_dictionary_free consumes the dictionary anyway. A caller following the old wording would free it a second time. <libvmaf/libvmaf.h> already described vmaf_use_feature correctly. <libvmaf/model.h> described the overloads correctly but then claimed its rule "matches vmaf_use_feature", which is exactly the case where they differ. All four surfaces now state the asymmetry explicitly and say why it exists rather than papering over it. vmaf_use_feature also leaked the caller's dictionary on two failure paths: a failed vmaf_dictionary_copy returned without releasing the source, and a failed vmaf_feature_extractor_context_create returned without releasing the copy — that function frees only what it allocated itself. Both leaked precisely when the documented contract told the caller not to free, so nothing else could have released them. Two cases added to test_model_feature_overload_ownership.c pin the asymmetry from both sides: the model overload returning 0 and consuming on an unknown name, and vmaf_use_feature returning -EINVAL and handing the dictionary back. The suite is 8 tests and passes clean under -Db_sanitize=address, which is where a regression would surface as a double free rather than a silent contract violation. meson test --suite=fast: 111 Ok, 0 Fail. Netflix golden gate: 271 passed, 12 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MSVC shim's architecture test was `_M_X64 || _M_IX86`, justified in both the header comment and scripts/ci/check-msvc-clz-shim.sh by the claim that `_BitScanReverse` is x86-only. Per the MSVC intrinsics reference that is wrong: `_BitScanReverse` is available on x86, ARM, x64 and ARM64, and only `_BitScanReverse64` is restricted (to x64 and ARM64). `__lzcnt` is the x86-only one, and it is not used here. The header is the sole definition of `__builtin_clz` for integer_adm.c and integer_vif.h, which sit on the generic scalar path and are compiled for every target, so MSVC ARM64 matched no branch and failed to compile outright rather than falling back to anything. The fork runs no MSVC ARM64 CI leg, so the break was latent. The allowlist now enumerates every architecture MSVC targets, selects `_BitScanReverse64` on x64/ARM64 and keeps the two-step 32-bit reconstruction elsewhere. The gate now joins the guard's continuation lines before matching (the guard legitimately spans several lines) and asserts the ARM64 arm specifically, so the allowlist cannot be narrowed again; both that narrowing and an `__lzcnt` reintroduction were negative-tested against it. Header and gate comments corrected to the documented architecture matrix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round-4 review findings; each checked against the code before acting, and one did not hold up and is recorded as such. Metal motion kernels — the round-3 guard was insufficient and the real defect was in the kernel. integer_motion.metal, float_motion.metal and integer_motion_v2.metal load a TILE_W x TILE_H = 20x20 threadgroup tile at origin `bid * 16 - 2`, so the mirror helper receives indices up to `16*bid + 17`, far outside the 5-tap neighbourhood it appears to serve, and a single bounce only lands in range when `idx <= 2 * (sup - 1)`. Enumerated over the real tile span, the single-bounce form read out of bounds for every dimension in 1..9 AND for exactly 17 — at 17 the last workgroup reaches idx 33 while 2*(17-1) = 32, folding to -1. A 3x3 floor closes neither 4..9 nor 17, so all three kernels now fold iteratively, as the CPU scalar path already does in convolution_internal.h. Verified over dims 1..299 across the full tile span: always in range, always terminating, and bit-identical to the single bounce wherever one bounce sufficed, so no in-contract score moves. The host-side guard comments claimed the 3x3 floor was what kept the kernel in bounds, which was wrong; corrected. integer_motion_v2.metal was also the last backend still using the wrong reflection convention: `2 * sup - idx - 1` repeats the boundary row where reflect-101 skips it. CPU, CUDA (PR #120 / T7-15), SYCL and HIP all carry `- 2`, and the SYCL fix records the old form as a systematic ~2.6e-3 motion drift vs CPU on every frame after the first. Metal now matches (ADR-0214 places=4). The ADM kernels' `- 1` was checked and deliberately left alone — ADM legitimately uses whole-sample reflection, matching adm_tools.c::dwt2_src_indices_filt_s, CUDA's calculate_indices() and the SYCL twin. float_vif — all four GPU backends sat below the CPU floor. The CPU requires vif_get_min_dim() = 16 at the default kernelscale (the binding constraint is scale 3: max(9,10,12,16)). Metal checked only `scale_w[FVIF_SCALES-1] == 0`, i.e. `w >> 3 == 0`, an effective floor of 8; CUDA, HIP and SYCL had no dimension floor at all, halving to scale 3 unchecked. All four now derive the floor from vif_get_min_dim(), the CPU's own source of truth, so the 8..15px range that walks the reflect-101 mirror out of the plane at scale 3 is rejected uniformly. vif_tools.h gained an `extern "C"` guard — without it the C++ (SYCL) and Objective-C++ (Metal) callers demand mangled symbols against the C vif_tools.c. It was previously included only by C translation units. vmaf.cpp — `--help` and `--version` left the Windows console in UTF-8 + VT mode. WindowsConsoleGuard was an automatic local whose comment claimed it restored on every exit path. It did not: cli_parse terminates via usage_exit(), which is [[noreturn]] and calls exit(), and exit() does not destroy objects with automatic storage duration. Objects with static storage duration ARE destroyed by exit() ([basic.start.term]), so the guard is now static and the restore runs on the exit() paths, the `goto cleanup` spine and a normal return alike. POSIX is unaffected (the block is #ifdef _WIN32). check-msvc-clz-shim.sh was evadable by macro indirection: rules (1) and (4) keyed on the call syntax `__lzcnt(`, so `#define LZ __lzcnt` followed by `LZ(x)` reintroduced the instruction while still passing the gate that exists to prevent exactly that. Both rules now match the bare identifier, and rule (4) is scoped to source extensions because core/src/feature/AGENTS.md legitimately discusses __lzcnt in prose. Negative-tested: macro indirection, a narrowed architecture allowlist, and a direct __lzcnt reintroduction all fail the gate. libvmaf-build-matrix.yml — the static-link smoke test linked with bare `cc` while the matrix builds with `ccache gcc-14` / `ccache clang-22`, so it exercised a toolchain the archive was not produced with; now ${CC:-cc}. The accompanying LTO concern does not apply: b_lto is meson-default false here and explicitly false on the SYCL/CUDA legs, so the archive holds plain objects rather than LTO IR. NOT a defect — the Libs.private libc++ detection. The review held that keying on _LIBCPP_VERSION ignores an explicit -stdlib=libc++. Tested against the installed meson: a probe project reading cxx.get_define('FOO') under -Dcpp_args=-DFOO=42 reports 42, so compiler checks do observe the project's cpp_args and the _LIBCPP_VERSION probe therefore sees -stdlib=libc++ exactly as its comment claims. No change. Verified: CPU build + fast suite 111 Ok / 0 Fail; CUDA lane rc=0 with float_vif_cuda.c.o built; SYCL lane rc=0 under icpx with float_vif_sycl.o built and no undefined vif_get_min_dim, confirming the extern "C" linkage resolves. clang-tidy exit=0 on both files CI's changed-files job globs (core/tools/vmaf.cpp, core/src/feature/vif_tools.h); .mm and .metal are not in that glob and cuda/ hip/ sycl/ are excluded by path. The Metal kernels are not buildable on Linux — CI's macOS legs compile them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The whole-tree ratchet exited 2 on this branch — four files were ABOVE
their baseline, which ADR-1142 treats as the PR's own regression to fix
in code, never to baseline away:
core/src/feature/common/convolution_avx.c 0 -> 1
core/src/feature/common/convolution_avx512.c 0 -> 1
core/test/test_model_feature_overload_ownership.c 0 -> 1
core/test/test_motion_convolution_oob.c 0 -> 11
convolution_avx.c / convolution_avx512.c —
readability-function-size on convolution_f32_avx{,512}_xy_s: "62 lines
including whitespace and comments (threshold 60)". The threshold counts
comments, and the clamp this PR added carried a five-line rationale
block duplicated at all six call sites while the same explanation
already lives on convolution_clamp_borders() in
convolution_internal.h. Replaced with a three-line pointer to that
definition at every site: the explanation is not lost, it is no longer
copied six times, and both functions drop back under the threshold. No
code changed.
test_model_feature_overload_ownership.c — readability-function-size on
run_tests. Each mu_run_test expands to several statements, and the two
cases added for the ownership asymmetry took it to eight, crossing
StatementThreshold 120. Split into run_guard_tests() and
run_consumption_tests(), grouped the way core/test/test_iqa_helpers.c
and test_cli_parse.c already group theirs.
test_motion_convolution_oob.c — eleven modernize-use-nullptr. This is a
C translation unit, and ADR-1138 keeps NULL in C TUs because MSVC's
documented /std:clatest C23 feature set has no `nullptr` while the
required Windows build compiles it with cl.exe. Wrapped in
NOLINTBEGIN/NOLINTEND(modernize-use-nullptr) with the ADR-1138
citation inline, matching the pattern test_model.c and test_output.c
already use.
Verified: clang-tidy reports 0 warnings on all four files, the CPU fast
suite is 112 Ok / 0 Fail, and both new tests pass individually. The
three stale-high entries the same run reported (convolution.c 2 -> 0,
test_float_vif_min_dim.c 8 -> 0, test_motion_min_dim.c 15 -> 0) are
left for CI's next measurement to be committed as the tightened
baseline, since the previous measurement was taken with these
regressions still present and so is not a usable baseline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gate on Windows Two platform failures on this branch, neither reproducible on Linux x86-64. macOS arm64 — test_convolution_edge_small::test_large_plane_bit_identical failed with "iterative fold moved an in-contract result". The fold was NOT the cause. It is integer-only, so it cannot vary by platform; the failure was floating-point contraction. The test's reference_convolve() reads the file-scope `kFilter5`, which the compiler can constant-fold and vectorize, while the library kernel receives an opaque `const float *filter`. On any target with FMA in its baseline — every arm64 — clang contracts `accum += filter[k] * src[...]` to an fma in one and not necessarily the other, so the last bit legitimately differs. x86-64 agreed only because FMA is not in its baseline. Two separately-compiled float accumulations are not a portable bit-identity claim. Rather than loosen the invariant, this states it where it actually lives. test_fold_matches_single_bounce_exactly asserts the real claim directly and exhaustively: for every size 2..64 and every in-contract index, convolution_reflect101() must return exactly what a single bounce returns, and out of contract it must still land inside the plane. That is integer-only and platform-independent, and it is a stronger statement than the float comparison ever made. The end-to-end 24x24 cross-check is kept but compared within 8 ULP, with the contraction reasoning recorded on it; 8 ULP is far below anything score-visible while a genuine fold divergence changes which sample is read and moves results by O(1e-2). The now-unused bit-identity helpers are removed. Windows MinGW64 — check_msvc_clz_shim failed, and my first reading of it was wrong: it is unrelated to the rule changes in this PR. meson invokes the script through its shebang interpreter, and on the MinGW64 runner `bash` resolves to Windows' own WSL bash.exe, which has no installed distribution. The leg printed "Windows Subsystem for Linux has no installed distributions" and exited 1 before the script ever ran. The gate is a static source-content check, so it is now registered on non-Windows hosts only — Linux and macOS both run it in the fast suite (macOS passes it today) and the lint lane runs it as well, so no coverage is lost. Also replaced rule (4)'s `grep -vF "$HDR"` self-exclusion with grep's own --exclude on the basename. This is a robustness cleanup, not the Windows fix: comparing grep's walked path against a separately constructed absolute path is fragile, and the basename form has no path dependency. Negative-tested that macro indirection, a narrowed architecture allowlist, and an __lzcnt reintroduction in another file all still fail the gate, and that a clean tree passes. Verified: fast suite 112 Ok / 0 Fail, test_convolution_edge_small passes with the new exhaustive case, clang-tidy 0 warnings on the changed test, and the gate is still registered and passing on this Linux host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Windows MSVC + oneAPI SYCL leg failed to compile this file: test_motion_convolution_oob.c(70,22): error: call to undeclared function 'strtok_r'; ISO C99 and later do not support implicit function declarations test_motion_convolution_oob.c(70,16): error: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' strtok_r is POSIX; the MSVC runtime ships strtok_s instead, so the call went undeclared and its int return was then assigned to a char *. Plain strtok is on the fork's banned-function list (docs/principles.md S1.2 rule 30), so neither variant is available here. The option string this test parses is a fixed "k=v:k=v" form under its own control, so it now splits with strchr in a small loop: portable everywhere, no reentrancy question, and no banned call. Behaviour is identical for every input the test uses. Verified: test_motion_convolution_oob passes, the fast suite is green, and clang-tidy reports 0 warnings on the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fa9ef2c to
1a01ff4
Compare
…servability - Correct integer_motion_v2.metal header comment to document reflect-101 mirror padding matching CPU, CUDA, SYCL, and HIP twins - Set mu_skipped = 1 on -ENODEV in test_metal_motion_v2_parity.c to exit 77 on systems without Metal devices, and log stdout on active hardware - Configure test_metal_motion_v2_parity with protocol 'exitcode', should_fail false, and verbose true in core/test/meson.build - Add ADR-1176 closing out the Metal motion_v2 mirror fix landed in #1223, linking from ADR-1166 and docs/adr/README.md - Move T-METAL-MOTION-V2-MIRROR-OFF-BY-ONE-2026-09-03 to Recently closed in docs/state.md with HTML marker at line 337 - Add mv2_mirror reflect-101 invariant to core/src/feature/metal/AGENTS.md - Add changelog fragment and rebase notes entry
…servability - Correct integer_motion_v2.metal header comment to document reflect-101 mirror padding matching CPU, CUDA, SYCL, and HIP twins - Set mu_skipped = 1 on -ENODEV in test_metal_motion_v2_parity.c to exit 77 on systems without Metal devices, and log stdout on active hardware - Configure test_metal_motion_v2_parity with protocol 'exitcode', should_fail false, and verbose true in core/test/meson.build - Add ADR-1176 closing out the Metal motion_v2 mirror fix landed in #1223, linking from ADR-1166 and docs/adr/README.md - Move T-METAL-MOTION-V2-MIRROR-OFF-BY-ONE-2026-09-03 to Recently closed in docs/state.md with HTML marker at line 337 - Add mv2_mirror reflect-101 invariant to core/src/feature/metal/AGENTS.md - Add changelog fragment and rebase notes entry
…servability - Correct integer_motion_v2.metal header comment to document reflect-101 mirror padding matching CPU, CUDA, SYCL, and HIP twins - Set mu_skipped = 1 on -ENODEV in test_metal_motion_v2_parity.c to exit 77 on systems without Metal devices, and log stdout on active hardware - Configure test_metal_motion_v2_parity with protocol 'exitcode', should_fail false, and verbose true in core/test/meson.build - Add ADR-1176 closing out the Metal motion_v2 mirror fix landed in #1223, linking from ADR-1166 and docs/adr/README.md - Move T-METAL-MOTION-V2-MIRROR-OFF-BY-ONE-2026-09-03 to Recently closed in docs/state.md with HTML marker at line 337 - Add mv2_mirror reflect-101 invariant to core/src/feature/metal/AGENTS.md - Add changelog fragment and rebase notes entry
…servability - Correct integer_motion_v2.metal header comment to document reflect-101 mirror padding matching CPU, CUDA, SYCL, and HIP twins - Set mu_skipped = 1 on -ENODEV in test_metal_motion_v2_parity.c to exit 77 on systems without Metal devices, and log stdout on active hardware - Configure test_metal_motion_v2_parity with protocol 'exitcode', should_fail false, and verbose true in core/test/meson.build - Add ADR-1176 closing out the Metal motion_v2 mirror fix landed in #1223, linking from ADR-1166 and docs/adr/README.md - Move T-METAL-MOTION-V2-MIRROR-OFF-BY-ONE-2026-09-03 to Recently closed in docs/state.md with HTML marker at line 337 - Add mv2_mirror reflect-101 invariant to core/src/feature/metal/AGENTS.md - Add changelog fragment and rebase notes entry
…servability - Correct integer_motion_v2.metal header comment to document reflect-101 mirror padding matching CPU, CUDA, SYCL, and HIP twins - Set mu_skipped = 1 on -ENODEV in test_metal_motion_v2_parity.c to exit 77 on systems without Metal devices, and log stdout on active hardware - Configure test_metal_motion_v2_parity with protocol 'exitcode', should_fail false, and verbose true in core/test/meson.build - Add ADR-1176 closing out the Metal motion_v2 mirror fix landed in #1223, linking from ADR-1166 and docs/adr/README.md - Move T-METAL-MOTION-V2-MIRROR-OFF-BY-ONE-2026-09-03 to Recently closed in docs/state.md with HTML marker at line 337 - Add mv2_mirror reflect-101 invariant to core/src/feature/metal/AGENTS.md - Add changelog fragment and rebase notes entry
…servability - Correct integer_motion_v2.metal header comment to document reflect-101 mirror padding matching CPU, CUDA, SYCL, and HIP twins - Set mu_skipped = 1 on -ENODEV in test_metal_motion_v2_parity.c to exit 77 on systems without Metal devices, and log stdout on active hardware - Configure test_metal_motion_v2_parity with protocol 'exitcode', should_fail false, and verbose true in core/test/meson.build - Add ADR-1176 closing out the Metal motion_v2 mirror fix landed in #1223, linking from ADR-1166 and docs/adr/README.md - Move T-METAL-MOTION-V2-MIRROR-OFF-BY-ONE-2026-09-03 to Recently closed in docs/state.md with HTML marker at line 337 - Add mv2_mirror reflect-101 invariant to core/src/feature/metal/AGENTS.md - Add changelog fragment and rebase notes entry
…servability (#1294) - Correct integer_motion_v2.metal header comment to document reflect-101 mirror padding matching CPU, CUDA, SYCL, and HIP twins - Set mu_skipped = 1 on -ENODEV in test_metal_motion_v2_parity.c to exit 77 on systems without Metal devices, and log stdout on active hardware - Configure test_metal_motion_v2_parity with protocol 'exitcode', should_fail false, and verbose true in core/test/meson.build - Add ADR-1176 closing out the Metal motion_v2 mirror fix landed in #1223, linking from ADR-1166 and docs/adr/README.md - Move T-METAL-MOTION-V2-MIRROR-OFF-BY-ONE-2026-09-03 to Recently closed in docs/state.md with HTML marker at line 337 - Add mv2_mirror reflect-101 invariant to core/src/feature/metal/AGENTS.md - Add changelog fragment and rebase notes entry Co-authored-by: Lusoris <lusoris@pm.me>
2 participants
Summary
Harvests a batch of long-open Netflix/vmaf reports, verifies each one against this tree rather than trusting the report, and fixes the subset that still bites. The fork diverged far enough (ADR-0700's
libvmaf/→core/rename, several C-to-C++ conversions, four fork-added GPU backends) that an upstream issue is neither automatically applicable nor automatically stale — three of these were already fixed here, two were never applicable, and several are wider here than upstream because a fork-added backend or SIMD path copied the defective shape.Three of the fixes close memory-safety defects that are reachable today from the public C API with supported input. The complete triage table — including the ALREADY-FIXED and NOT-APPLICABLE verdicts, which are the expensive ones to re-derive — is in
docs/research/1166-upstream-issue-harvest-2026-09-03.md.Type
feat— new featurefix— bug fixperf— performance improvementrefactor— no behavior changedocs— documentation onlytest— test-onlybuild/ci— tooling / infraport— cherry-pick from upstream Netflix/vmafsycl/cuda/simd— backend-specificWhat is fixed, and the test that proves it
convolution_edge_s/_sq_s/_xy_sbounced an out-of-range reflect-101 tap once, which only lands in range forsize >= radius + 1; at size 2 a tap of −2 folds to +2 and +3 folds to −1 (heap-buffer-overflow READ). (b)convolution_x_c_s/_y_c_sderivedborders_right = dim - (filter_width - radius), negative for a plane narrower than the filter, so the trailing loop started at a negative index and wrotedst[i * dst_stride - 1](heap underflow WRITE). Reachable via--feature float_vifon any 9..15 px frame and via--feature float_motionwithmotion_add_uvon a 4x4 YUV420P frame.core/test/test_convolution_edge_small.c— planes embedded in NaN-poisoned buffers; an escaping tap taints the output, an escaping write replaces a poison NaN. Fails on the pre-fix tree, verified by stashing the two source files and rebuilding.test_large_plane_bit_identicaladditionally asserts bit equality against an explicit single-bounce reference at 24x24, so nothing in contract moved.motion_check_min_dim()validated luma only whilemotion_blur_plane()runs per plane atref_pic->w[c]/h[c].core/test/test_motion_min_dim.c::test_float_motion_add_uv_chroma_guard—motion_add_uvat 4x4 and 3x3 now-EINVAL, 5x5 and 576x324 still succeed.feature_extractor.cpp:229/232/234).core/test/test_motion_min_dim.c::test_metal_motion_min_dim— host-side rejection, so it needs no Apple GPU; degrades to a no-op withoutHAVE_METAL.vmaf_model_feature_overload()returned-ENOMEMpast the unconditional free (leaking the caller's dict); the collection wrapper discarded the copy error, leaked the partial copy, skipped remaining sub-models and could still return 0, and dereferenced*model_collectionunchecked.<libvmaf/feature.h>and<libvmaf/model.h>documented opposite ownership rules — one reading is a latent CWE-415.core/test/test_model_feature_overload_ownership.c— pins the guard paths, the success path, and drives the merge-failure branch deterministically (a heap-allocated empty dict makesvmaf_dictionary_merge()return NULL, which is exactly the branch that leaked; LSan reports it in the ASan lane).\033[Kto stderr with a byte-orientedfprintfwhile nothing in the tree ever set the console code page or enabled VT — mojibake under cp437/cp1252, replacement boxes under cp936, and a literal←[Kon legacy conhost, every frame of every run.core/test/test_spinner.cpp— drives the selectors with the code pages a real conhost reports (437/1252/936/0), asserts the ASCII fallback and the VT gate, and pins the braille table's first/last entries byte-for-byte plusstrlen == 6for all 56 so POSIX output cannot drift.__builtin_clzshim used__lzcnt, which emitsF3 0F BDwith no runtime gate; on x86-64 without ABM the prefix is ignored and it retires as BSR, returning the MSB index instead of the leading-zero count. Two of the four call sites are on the generic scalar path, so an MSVC build silently mis-normalised every VIF and ADM log2 — a 2048-LSB error, i.e. a factor of two in the VIF fixed point — with no fault and no CI signal (every hosted Windows runner has LZCNT).scripts/ci/check-msvc-clz-shim.sh(registered as afastmeson test) — fails on the pre-fix header with four findings, verified by stashing the file; also scans the rest ofcore/src. Pluscore/test/test_compat_clz.c, which unit-tests the31 - msbarithmetic the__lzcntform got wrong.libvmaf.pccarriedLibs.private: -pthread -lmand no C++ runtime, so linking the static archive failed with hundreds of undefined references tooperator new/std::ios_base::ios_base(). More exposed here than upstream: the C++ symbols come from the fork's own converted TUs, not just vendored libsvm. ADR-0198's static FFmpeg reproducer had to add-lstdc++by hand for exactly this reason.libvmaf-build-matrix"Verify static pkgconfig" step no longer greps the flag list — it compiles and links a C consumer with the C driver against exactly whatpkg-config --static --libs libvmafreports, which is the test that reproduces the downstream FFmpeg failure.core/; since ADR-0700 the documented layout put it elsewhere and every.cufailed withfatal error: cuda/integer_adm_cuda.h: No such file or directory. (c) Three shell-driven tool tests declared nodepends, so a subset run built nothing and died with exit 127. Hunk (a) is already fixed in-tree — do not re-port.make test-netflix-goldentriggers on this workstation.Checklist
make format && make lintis green locally.pre-commit run --files <48 changed files>exits 0. Every file this PR touches is at or below its clang-tidy baseline; the new files and both edited test files measure 0 warnings. See "Known follow-ups" for pre-existing ratchet drift on master that this PR does not cause.meson test -C build --suite=fast -j 4→ 110/110 OK./cross-backend-diffand the worst ULP is ≤ 2. — no cross-backend run needed: the only GPU-side change is the three Metalinit()dimension guards, which reject before any kernel dispatch and cannot move a score. No CUDA/SYCL/HIP kernel is touched. The CPU convolution change is proven bit-identical for every in-contract size bytest_large_plane_bit_identical..c/.cpp/.cu/.h/.hpp, it has the appropriate license header (seeCONTRIBUTING.md).scripts/ci/check-copyright.shexits 0.!orBREAKING CHANGE:and the migration path is documented below. — not a breaking change; see "Breaking changes / migration" for the two behaviour changes.docs/adr/_index_fragments/<NNNN-slug>.mdand the slug is appended todocs/adr/_index_fragments/_order.txt.scripts/docs/concat-adr-index.sh --checkexits 0.Bug-status hygiene (ADR-0165)
docs/state.mdupdated in this PR with a row in the appropriate section.Seven rows in Recently closed (one per fixed issue), nine in Open bugs (the confirmed-but-deferred set plus the Metal
motion_v2mirror off-by-one found while triaging Netflix/vmaf#1580), and six in Confirmed not-affected — including two claims from the reports themselves that are refuted here and should not be re-investigated (Netflix/vmaf#818's "pooling silently falls back to mean", and Netflix/vmaf#1494's nvd/adm_ref_display_heightpremise, which the existing guard atinteger_adm.c:3509already rejects).Netflix golden-data gate (ADR-0024)
assertAlmostEqual(...)score in the Netflix golden Python tests.CUDA_VISIBLE_DEVICES= make test-netflix-golden→ 271 passed, 12 skipped, and the three canonical CPU pairs re-scored on the built binary:Cross-backend numerical results
No cross-backend run applies. The only device-side change is three Metal
init()guards that return-EINVALbefore any pipeline is created; no kernel, no numeric path.Deep-dive deliverables (ADR-0108)
docs/research/1166-upstream-issue-harvest-2026-09-03.md: the full triage table, per-candidate evidence, and the deferred set with its reasoning.docs/adr/1166-upstream-issue-harvest.md§ Alternatives considered (five options: harvest-and-verify, ignore upstream, port blindly, report upstream only, one PR per issue).AGENTS.mdinvariant note —core/src/feature/AGENTS.md(the iterative fold, thesize <= 1termination guard, the border clamp, the deliberate single-bounce divergence in the motion extractors, and the never-__lzcntrule),core/tools/AGENTS.md(spinner table byte-stability and theWindowsConsoleGuarddeclaration-before-gotorequirement),core/test/AGENTS.md(the ADR-1138NULLcarve-out and therun_tests()branch budget).changelog.d/fixed/upstream-harvest.md;scripts/release/concat-changelog-fragments.sh --writerun,--checkexits 0.docs/rebase-notes.mdgained a section per touched upstream-mirror file, including the deliberate divergence from Keep motion filter mirror() in bounds for tiny frames Netflix/vmaf#1581 (upstream fixesmirror()so tiny frames can be scored; the fork errors out instead) and the DO-NOT-COPY marker against MSVC: Miscellaneous uncontroversial fixes Netflix/vmaf#1422.Reproducer
Known follow-ups
Confirmed, deliberately not batched — each has a
docs/state.mdOpen-bugs row with its evidence, and each needs its own PR because it either moves scores, changes CLI grammar, or needs hardware this workstation does not have:motion_v2mirror off-by-one (found while triaging mirror()-style boundary reflection reads out-of-bounds for width/height < 3 (motion feature, all backends) Netflix/vmaf#1580):integer_motion_v2.metal:54uses2 * sup - idx - 1where the CUDA twin and the CPU source use2 * size - idx - 2, and the file's own header comment claims it matches CUDA. Same class as the HIP fix in ADR-1106. Moves Metal scores; needs an Apple GPU parity run.adm_cmi == 0border reads rows {1,2,3} where the CPU reads {1,0,1}; and the>> shift_inner_accumrounding runs per warp (CUDA) / per thread (HIP) instead of once per image row. The x86half_w_modNthird defect is present but empirically score-neutral at 388x288 across three--cpumasksettings, because the corrupted column falls inside theADM_BORDER_FACTORcrop.angle_flagpredicates across CPU / CUDA s0 / CUDA s123 / SYCL / Metal; a 40M-sample sweep puts the disagreement at 0.0031–0.0046%, switching on exactly where the operands cross 2²⁴.libvmaffilter creates a mojibake filename for non-ASCIIlog_pathNetflix/vmaf#1568 — narrow_open()inoutput_file_open(), plus 12 further fork-added sites upstream does not have. Wants a newcompat/path_utf8surface and its own ADR.MINtruncates genuinely near-lossless frames: a one-byte flip on the 576x324 golden reference reports 60.0 dB where FFmpeg's own psnr filter (and the arithmetic) says 100.840479.strsepincli_parse.cppsilently truncates--model path=<dir>/dir=eq/m.jsonto a phantom path and makespath=C:\...unrepresentable; the Go surfaces synthesise the same string.perc*pool methods Netflix/vmaf#818 — the public pooling enum still has noMEDIAN/PERC*.drain_batch.c's fence batch is_Thread_local, not per-VmafContext, andvmaf_close()never closes it.adm_csf_mode=1overflowsuint16_t i_rfactorat the stock view distance and producesinteger_adm2_csf_1= 0.000614 against a float reference of 0.9396.Pre-existing clang-tidy ratchet drift on master, not caused by this PR.
python3 scripts/ci/tidy-ratchet.py --lane cpu --build-dir buildreports regressions indict.cpp(+1),ort_backend.c(+2),feature_collector.cpp(+3),feature_extractor.cpp(+1),fex_ctx_vector.cpp(+1),log.cpp(+2) andtest_ort_internals.c(+7), and stale-high slack inconvolution.c(−2),float_motion.c(−5),integer_vif.c(−14) andvif_tools.c(−26). None of these files' warning counts changed with this PR — verified by measuring the same files with the changes stashed — and the flagged diagnostics are unrelated to anything here (e.g.log.cpp's two areassert()-could-be-static_assertatlog.cpp:154-155). This PR additionally improvestest_motion_min_dim.c(15 → 0) andtest_float_vif_min_dim.c(8 → 0). I deliberately did not runtidy-ratchet.py --write, because--writeoverwrites the whole baseline with the local measurement and would silently raise the entries for the pre-existing regressions — the documented flow is to commit the measurement CI itself uploads. If the CI ratchet asks for a tighten, that artifact is the right source.Breaking changes / migration
Not a breaking change (no ABI change, no removed symbol, no renamed flag), but two input-validation surfaces became stricter. Both convert previously undefined behaviour into a documented
-EINVAL:float_vifnow requires 16x16, not 9x9. The four-scale ladder halves the dimension per scale and re-convolves with that scale's Gaussian, so the real floor ismaxover the ladder of((filter_width_s / 2) + 1) << s= 16 at the defaultvif_kernelscale. Input in 9..15 px previously passed the guard and read out of bounds at scale 3; those runs were reading uninitialised memory and their scores were not meaningful. The bound is derived fromvif_kernelscale, so a non-default kernel scale moves it. Documented indocs/metrics/vif.md§ Minimum frame size.float_motionwithmotion_add_uv=truenow validates the chroma planes. In 4:2:0 that means 5x5 minimum (chroma 3x3) rather than 3x3; 4:2:2 needs 5x3, 4:4:4 is unchanged at 3x3. The error message names the failing plane. Documented indocs/metrics/motion.md§ Input format constraints.Also user-visible, but not a restriction: on Windows the CLI now switches the console to UTF-8 + VT for the duration of the run and restores the previous state on exit, falling back to an ASCII spinner when the console refuses. POSIX output is byte-identical. Documented in
docs/usage/cli.md§ Windows console output.The
VmafFeatureDictionaryownership contract is now written the same way in all three public headers. It describes what the implementation has always done, so no working caller changes behaviour — but a caller that followed<libvmaf/model.h>'s previous wording ("ownership transfers even on a non-zero return") and therefore never freed after an-EINVALguard rejection was leaking, and one that followed<libvmaf/feature.h>'s wording literally was risking a double free on the non-guard paths. ADR-0806 is marked Superseded by ADR-1166.🤖 Generated with Claude Code
no ffmpeg-patches update needed: the three public headers this PR touches
(
core/include/libvmaf/feature.h,libvmaf.h,model.h) changedoc-comments only — the
VmafFeatureDictionaryownership contract wording,which previously stated opposite rules in two headers. Verified with
git diff -U0 origin/master..HEAD -- core/include/filtered to non-commentlines: zero changed lines outside comments, and zero
VMAF_EXPORT,signature,
struct,enumor#definechanges. No entry point is added,renamed or removed, no
LIBVMAFContextfield changes, and nocheck_pkg_configprobe symbol moves, so nothing inffmpeg-patches/consumes anything new. The
VmafContext/VmafModelmentions the gatematched are the unchanged signatures those comments sit above.