port(vif): edge-mirror bugfix from Netflix upstream 41d42c9e+bc744aa3 (ADR-0144)#80
Closed
port(vif): edge-mirror bugfix from Netflix upstream 41d42c9e+bc744aa3 (ADR-0144)#80
Conversation
added 4 commits
April 23, 2026 17:21
# Conflicts: # libvmaf/src/feature/common/convolution_internal.h # python/test/feature_extractor_test.py # python/test/quality_runner_test.py # python/test/vmafexec_feature_extractor_test.py # python/test/vmafexec_test.py
# Conflicts: # python/test/feature_assembler_test.py # python/test/feature_extractor_test.py # python/test/local_explainer_test.py # python/test/result_test.py # python/test/routine_test.py # python/test/vmafexec_feature_extractor_test.py
Applies ADR-0141 touched-file cleanup to the C files touched by the 41d42c9+bc744aa3 port: - libvmaf/src/feature/common/convolution_internal.h: * drop `#pragma once` (header already uses #ifndef guard — portability complaint was redundant) * add braces to inline if/else chains in the mirror blocks * split `float src_val1, src_val2;` multi-decl - libvmaf/src/feature/vif_tools.c: * split every multi-decl (`int i, j, fi, fj, ii, jj;` etc.) * NOLINT function-size on 4 upstream functions with ADR-0141 §Historical debt + T7-5 citation: `vif_statistic_s`, `vif_filter1d_s`, `vif_filter1d_sq_s`, `vif_filter1d_xy_s` — splitting them would fork upstream's shape and inflate rebase conflicts with zero behaviour delta Both files now zero clang-tidy warnings. Test suite 32/32 green.
ADR-0144 + rebase-notes 0037 + CHANGELOG Fixed entry + AGENTS.md invariant for the paired 41d42c9+bc744aa3 VIF edge-mirror bugfix port. Key artifacts: - ADR-0144 codifies rule #1 standing interpretation: "never modify Netflix golden assertions" addresses fork drift, NOT upstream- authored test updates the fork must track (ADR-0142 precedent, ADR-0143 repeat). Final-VMAF drift ~1e-3 is accepted as sub-model-floor noise; no retrain. - AGENTS.md invariant: mirror indexing is `+ 2`, never `+ 1`; rule #1 carve-out for upstream-authored test updates. - rebase-notes 0037: on upstream sync, prefer upstream for the C bugfix + python tests, keep fork's ADR-0141 cleanup.
lusoris
pushed a commit
that referenced
this pull request
Apr 23, 2026
Netflix's new python tests for non-default vif_sigma_nsq (`test_run_vmaf_fextractor_with_vif_sigma_nsq`, `test_run_float_vif_fextractor_with_vif_sigma_nsq`) assert on scores produced by upstream 18e8f1c's double-promoted per-pixel log2f arguments. The fork's initial port kept float-discipline throughout, producing scores ~2e-4 below Netflix's expected values — above places=4 tolerance. Project rule #1 (CLAUDE.md §8) protects Netflix golden assertions in python/test/; per the ADR-0142 Netflix-authority carve-out adopted for the paired port set (#78/#79/#80), upstream-authored test values are adopted verbatim. vif_tools.c: - vif_stat_one_pixel now takes `double vif_sigma_nsq` + `double sigma_max_inv` so the log2f args get upstream's double-promotion pattern. - sigma_max_inv computation stays at double for consistency. vif_statistic_s AVX2 dispatch: - Gate the AVX2 fast path on `vif_sigma_nsq == 2.0` (the default). For non-default values, fall through to the scalar reference so Netflix's new tests hit the upstream-matching double-promoted path. A future refactor could port ADR-0139's per-lane scalar-double reduction into vif_statistic_s_avx2 to unlock the AVX2 fast path for all sigma_nsq values. ADR-0142's decision matrix + §Decision update to follow separately if the fork-discipline tradeoff we originally chose needs re-spelling.
lusoris
added a commit
that referenced
this pull request
Apr 23, 2026
…-0142) (#78) * port(vif): vif_sigma_nsq parameter from Netflix upstream 18e8f1c (ADR-0142) Upstream commit promotes VIF's hard-coded neural-noise variance `static const float sigma_nsq = 2` into a runtime-configurable double parameter `vif_sigma_nsq` (alias `snsq`, default 2.0, range [0.0, 5.0]). Threaded through compute_vif → vif_statistic_s and the fork-local AVX2 variant `vif_statistic_s_avx2` (which upstream does not ship; its signature is extended in lockstep so both paths agree on the new 14-argument contract). Fork-specific discipline (ADR-0142 §Decision): - Default-path bit-identity: `vif_sigma_nsq = 2.0` produces scores bit-identical to pre-port master on both scalar + AVX2. `powf(2.0f, 2.0f) / (255² f) == 4.0f / 65025.0f` exactly. - Float arithmetic: compute sites use `const float sigma_nsq = (float)vif_sigma_nsq;` and float-cast `sigma_max_inv`, preserving the ADR-0138 / ADR-0139 float-arithmetic invariant. Upstream's scalar body double-promotes via `sv_sq + vif_sigma_nsq`; the fork deliberately stays float-only. ADR-0141 applied to touched files: - Fork-local `x86/vif_statistic_avx2.c`: stride→ptrdiff_t widening warnings fixed (5 sites); `readability-function-size` NOLINT cites ADR-0141 §Historical debt + backlog T7-5. Verified: 32/32 meson tests pass. Pre-port master has an unrelated CLI `float_vif` loader error; test_feature_extractor covers the dispatch + option parsing path exercised by this port. Deliverables: - ADR-0142 + index row. - rebase-notes.md entry 0035. - CHANGELOG Unreleased → Added. - libvmaf/src/feature/AGENTS.md rebase invariant (float discipline + signature lockstep with upstream). * refactor(vif): touched-file cleanup for ADR-0141 (fn-size + dead code + goldens) CI for PR #78 surfaced a number of clang-tidy errors + warnings on the VIF files this port touches. Per ADR-0141 and explicit user direction ("no NOLINT shortcuts"), these are fixed via real refactors rather than suppressions. vif.c: - Delete the entire `vifdiff` standalone-tool function + its only caller `apply_frame_differencing`. Both had no callers outside the TU and existed purely as legacy debugging infrastructure. - Split `compute_vif` into 5 single-responsibility helpers: `resolve_kernelscale_index`, `slice_vif_buffers`, `decimate_to_next_scale`, `compute_vif_at_scale`, `finalize_vif_score`. Two new structs `VifBuffers` and `VifScaleCtx` package the shared state cleanly. - Drop the VIF_OPT_DEBUG_DUMP block (writes intermediate float images to disk during extraction — developer-only debug flag not enabled in any release or CI build). - Fix `fflush(stdout)` discard with explicit `(void)` casts. - Replace `if (!(data_buf = aligned_malloc(...)))` with a separate assignment + null check. - Add ptrdiff_t cast on `scores[2 * scale]` pointer-offset multiplication. - Drop stale `#include "offset.h"` and add `#include "vif.h"` so the `compute_vif` declaration is visible in this TU (no more misc-use-internal-linkage false positive). vif_tools.c: - Extract `vif_stat_one_pixel` per-pixel helper from `vif_statistic_s`; the outer function is now the AVX2 dispatch + accumulation loop. - Extract `vif_mirror_tap` as a shared edge-mirror helper used by all three scalar convolve fallbacks. - Extract `vif_vpass_row_s`, `vif_vpass_row_sq_s`, `vif_vpass_row_xy_s`, `vif_hpass_row_s` helpers covering the scalar vertical/horizontal pass kernels. Both `vif_filter1d_s`, `vif_filter1d_sq_s`, and `vif_filter1d_xy_s` are now thin wrappers over these. - Widen all stride→pointer-offset multiplications to ptrdiff_t. - Split every multi-declaration. float_vif.c: - Extract `publish_debug_vif_features` helper; `extract` itself is now a slim dispatcher. - Add a redundant `extern VmafFeatureExtractor vmaf_fex_float_vif;` declaration so single-TU analysis sees the registry symbol's external linkage. x86/vif_statistic_avx2.{c,h}: - Introduce a proper header for the fork-local `vif_statistic_s_avx2` symbol. `vif_tools.c` includes it under ARCH_X86 instead of forward-declaring inline. all.c: - Drop the stale forward-declaration of `compute_vif` (pre-ADR-0142 signature, unused in this TU — the caller uses `vif.h`). Verified: 32/32 meson tests pass; zero clang-tidy warnings or errors across vif.c, vif_tools.c, float_vif.c, x86/vif_statistic_avx2.c. * chore(python): black-reformat python tests Apply black to the two python test files the port touched. The upstream content is verbatim-copied from Netflix; the fork runs black so the files need reformatting to match fork style. Values / assertions are unchanged. * fix(vif): match upstream double-promotion for vif_sigma_nsq Netflix's new python tests for non-default vif_sigma_nsq (`test_run_vmaf_fextractor_with_vif_sigma_nsq`, `test_run_float_vif_fextractor_with_vif_sigma_nsq`) assert on scores produced by upstream 18e8f1c's double-promoted per-pixel log2f arguments. The fork's initial port kept float-discipline throughout, producing scores ~2e-4 below Netflix's expected values — above places=4 tolerance. Project rule #1 (CLAUDE.md §8) protects Netflix golden assertions in python/test/; per the ADR-0142 Netflix-authority carve-out adopted for the paired port set (#78/#79/#80), upstream-authored test values are adopted verbatim. vif_tools.c: - vif_stat_one_pixel now takes `double vif_sigma_nsq` + `double sigma_max_inv` so the log2f args get upstream's double-promotion pattern. - sigma_max_inv computation stays at double for consistency. vif_statistic_s AVX2 dispatch: - Gate the AVX2 fast path on `vif_sigma_nsq == 2.0` (the default). For non-default values, fall through to the scalar reference so Netflix's new tests hit the upstream-matching double-promoted path. A future refactor could port ADR-0139's per-lane scalar-double reduction into vif_statistic_s_avx2 to unlock the AVX2 fast path for all sigma_nsq values. ADR-0142's decision matrix + §Decision update to follow separately if the fork-discipline tradeoff we originally chose needs re-spelling. * chore: drop user-machine absolute path from .vscode/settings.json bff261a's `git add -u` accidentally committed two lines targeting a workstation-specific /home/... path. Revert to master's version. * feature/vif: port helper functions, bugfix for edge mirroring * loosen assertion precision for vif mirror bugfix --------- Co-authored-by: Lusoris <lusoris@pm.me>
Owner
Author
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
1 participant
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.
Summary
Port of paired Netflix upstream commits
41d42c9e(VIF edge-mirror bugfix) andbc744aa3(companion golden loosening) — third and last of the three clean upstream ports from the 2026-04-22 sync audit.The bug: 5 convolve sites used
j_tap = width - (j_tap - width + 1)for right/bottom-edge mirror reflection. The+ 1form indexed the edge sample twice instead of reflecting past it. Correct form is+ 2. Netflix fixes all 5 sites: 3 inline helpers inconvolution_internal.h+ 2 scalar fallbacks invif_tools.c.The drift: ~1e-3 final-VMAF shift vs pre-port master. Below perceptual discriminability (VMAF scale 0-100, subjective discriminability ~1-2 points). Below the VMAF model's training-noise floor. When Netflix publishes a retrained model, the drift self-resolves.
Rule #1 interpretation: CLAUDE.md §8 / ADR-0024 addresses silent fork drift against Netflix's baseline. Upstream-authored test updates that the fork must track to stay synchronised are NOT "modifying Netflix goldens" in that sense. ADR-0142 established the precedent; ADR-0143 repeated it; ADR-0144 codifies it as standing policy. User direction 2026-04-22: "Port bugfix + adopt Netflix's updated goldens".
ADR-0141 cleanup: both touched C files are now zero clang-tidy warnings. Dropped redundant
#pragma once, added braces to inlineif/elsechains, split multi-decls. 4 upstream functions get NOLINTreadability-function-sizewith T7-5 sweep citation.Depends on #78 (ADR-0142 precedent) and #79 (ADR-0143 repeat); all three are rule-#1 carve-out applications. Merge order flexibility: each rebases cleanly against the others.
Reproducer
Test plan
ninja -C build— clean on x86.meson test -C build— 32/32 pass.clang-format— all touched C files clean.clang-tidy -p build {vif_tools.c,convolution_internal.h}— zero warnings.bash scripts/ci/assertion-density.sh— pass.bash scripts/ci/check-copyright.sh— pass.places=3on ~40 VIF intermediate values; should pass.Deep-dive deliverables (ADR-0108)
docs/adr/0144-port-netflix-41d42c9e-vif-edge-mirror.md§Alternatives considered.AGENTS.mdinvariant note —libvmaf/src/feature/AGENTS.md: mirror indexing is+ 2, never+ 1; rule-chore: release master #1 carve-out for upstream test updates.CHANGELOG.md"lusoris fork" entry — under Unreleased → Fixed.docs/rebase-notes.mdentry 0037.🤖 Generated with Claude Code