Fix problems when compiling for/on various platforms and OSes - #1551
Open
JensAc wants to merge 3 commits into
Open
Fix problems when compiling for/on various platforms and OSes#1551JensAc wants to merge 3 commits into
JensAc wants to merge 3 commits into
Conversation
Author
Collaborator
|
Hey, @JensAc thanks for the patience as I've been working through all these PRs. Can you tell me, is it mainly MSVC support you are looking for? So far we've only officially supported MINGW64 builds on Windows, but I'm trying understand how much value MSVC brings for Windows devs? |
Author
|
Hi @kylophone, In particular, with MSVC we ran into issues, and I think these are addressed completely by #1477 , that I did not recognize when opening this PR. I did not want to propose too many changes (e.g. introducing pthreads4win), as I thought the interest might be limited due to already working MINGW64 builds. For that reason, I concentrated on changes that do not alter the functional behavior, but make life easier in case you want to compile with MSVC. From my perspective, you could close this PR, if you don't want to apply the changes or prefer #1477. The other option would be to merge it, as it should not affect the functional behavior. |
Contributor
|
Hi @JensAc, I am the original author of the PR you referenced. First of all, thanks for the mention and for looking at my PR. As you can see there, the PR is not monolithic and depends on other little PRs which address more atomic items needed first (like avx). You are right onto mentioning pthreads4win, it can also be implemented in a way that it can be used externally while I bundled it for the new CI tests to pass and to help non experienced users. Moreover, vmaf's codebase changed a bit and it needs to be rebased to work with the latest changes. Though the changes are minimal I did not wanted to touch more the PR in order to wait for @kylophone to have time to look at it as said so he does not feel pressured, a thing that of course I will do to make the PR to work again. However, I would recommend to close this PR (and I will take the little changes for those atomic PRs too crediting you of course) because I also include doc and CI tests to ensure that MSVC build will never break with the future changes. Cheers, David. |
Author
|
Hi David, @StormBytePP , I fully agree with your comment and your recommendation is totally fair. Also, I like the effort you put into MSVC support very much. However, I will leave it up to @kylophone to decide how to proceed and not close this for the moment. Best regards |
Contributor
|
Hi Jens, @JensAc, Yes, I am also waiting for his review/instructions. I have just rebased and updated all the code to work with the latest master so the review can be done properly. Specially I've fixed the problems you had here too in the CI tests (the same ones that appeared with the latest master) by changing the intrinsics to properly support plain x86 code: Additionally, I have added a new option If you have the chance, it would be greatly appreciated if you could test the external pthreads mode ( Best regards, |
Merged
26 tasks
lusoris
pushed a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 3, 2026
…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>
lusoris
pushed a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 3, 2026
…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>
lusoris
pushed a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 3, 2026
…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>
lusoris
pushed a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 3, 2026
…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>
lusoris
pushed a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 3, 2026
…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>
lusoris
pushed a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 3, 2026
…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>
lusoris
pushed a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 3, 2026
…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>
lusoris
added a commit
to VMAFx/vmafx
that referenced
this pull request
Sep 4, 2026
…ADR-1166) (#1223) * fix(core): harvest and fix nine stale upstream Netflix/vmaf reports (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> * fix(feature): clamp the SIMD convolution borders and close two guard 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> * fix(api): state the dictionary ownership contract per function, close 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> * fix(compat): cover MSVC ARM64 in the __builtin_clz allowlist 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> * fix(metal): fold the motion mirror iteratively and align the VIF floor 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> * fix(test): bring this PR's own files back to zero clang-tidy warnings 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> * fix(test): make the fold invariant portable and unregister the shell 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> * fix(test): drop strtok_r from the motion OOB test for MSVC portability 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> --------- Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey there,
I just collected a few changes required when you want to compile for/on various platforms and OSes.
E.g. MSVC would not compile the avx accelerated code with plain casts and
[]access in__m128is.The changes do not change the behavior of the code itself, but make life simpler when (cross) compiling libvmaf.