Repository navigation
feat(core): add percentile pooling methods (median, perc5, perc10, perc20) (ADR-1181) - #1392
Conversation
a3f3b00 to
10d8914
Compare
|
Blocked on Tidy Ratchet, and the two tidy gates disagree with each other on the same file in the same CI run. The
The file is not in Tidy Changed's exclusion list, so that pass is not vacuous — it genuinely linted it and found nothing. What I ruled out locally, replicating the ratchet's exact invocation (
CI builds the ratchet lane with Why I am not just baselining it: the ratchet explicitly refuses ("fix the code, never raise the baseline"), and it is right to. What would settle it: the ratchet's report artifact records per-file counts only, not the diagnostics. Having Holding in the merge train meanwhile; it has held the window ~25 minutes. |
Pull request was converted to draft
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fa92170 to
46f4c84
Compare
…GPU filters #1340 landed the percentile pooling C API (ADR-1188). This branch had implemented the same feature independently under ADR-1181 -- byte-identical enum names, order and semantics -- so its core half is now dead code that can never merge: it collides with master on libvmaf.h, libvmaf.c, output.cpp, predict.c and core/test/meson.build. Reduced to the consumer surfaces #1340 did not ship, which are genuinely additive: - pkg/libvmaf: a typed `PoolMethod` with a String() mapping to the on-the-wire names, so Go callers pick a pooling method without touching cgo enums. - ffmpeg-patches 0005 / 0006 / 0013: the libvmaf_sycl, libvmaf_vulkan and libvmaf_metal filters accept the four new `pool` values. The CPU libvmaf filter already did. - docs/usage/cli.md: documents them on the CLI. - python/test/command_line_test.py: end-to-end coverage through the CLI. Dropped: the duplicate core implementation, core/src/pooling_percentile.h, core/test/test_pooling_percentile.c, ADR-1181 and its index rows, and the docs/api, docs/state and rebase-notes edits master already carries from #1340. ADR-1181 is not superseded because it never landed -- ADR-1188 is the record. The changelog fragment now describes only the consumer wiring; master already carries `percentile-pooling-methods.md` for the API itself. Verified against master's implementation rather than its own: `go build` and `go vet ./pkg/libvmaf/` are clean and `gofmt -l` is empty, which also confirms the enum values agree. `go test ./pkg/libvmaf/` cannot link here for an unrelated reason -- the workstation has no DNN-enabled libvmaf installed, so cgo fails on vmaf_dnn_session_run; plain master fails identically, and CI's Go lane builds the library first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
46f4c84 to
dac17be
Compare
…DR-1188 output ADR-1188 intentionally keeps pool_report_order limited to the historical four methods (min, max, mean, harmonic_mean) so the default XML output does not include median/perc5/perc10/perc20. Revert the speculative additions in command_line_test.py so vmafexec tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The clang-tidy lanes install three toolchain components and treated them inconsistently. clang comes from apt.llvm.org at 22 because ubuntu-24.04 ships 18, which cannot parse the tree's C++26. meson comes from PyPI because the distro's 1.3.2 predates `c23` in c_std. gcc was left at the image's gcc-14, with no rationale in the workflow and no ADR pinning it -- the only mention of gcc-14 in docs/adr/ is an incidental line in a VVenC changelog summary. That stopped being cosmetic on PR #1392. The ratchet reported test_pooling_percentile.c going 0 -> 1 while Tidy Changed -- which fails on ANY warning and does not exclude that file -- called the same file clean, on the same runner and commit. It could not be reproduced on gcc-15 or gcc-16. The cause is structural: clang-tidy parses each TU against the system headers the C compiler provides, so a ratchet count depends on gcc's version as much as on clang-tidy's, and the baseline recorded only clang_tidy_version. Pinning gcc a major behind every developer's box while recording nothing about it makes a disagreement unexplainable. Three changes: - the lanes install gcc-15/g++-15 from ppa:ubuntu-toolchain-r/test; - the ratchet records cc_version, read from the build directory's meson-info/intro-compilers.json so it is the compiler that actually produced compile_commands.json, and annotates a mismatch like it already does for clang-tidy; - the --report artifact keeps every diagnostic as `path:line:col: [check]`. parse_diagnostics() already produced them and the script discarded all but the count. The baseline stays counts-only so it remains reviewable and does not churn on line-number shifts. Verified locally: the report now carries `cc (GCC) 16.2.1 20260810` and 29 diagnostics for output.cpp, naming check and location. This requires a CI-side regeneration of tidy-baseline-cpu.json -- counts under gcc-15's headers will not equal gcc-14's, and a local --write would resolve enable_dnn=auto differently and rebaseline unrelated files against the wrong build. libvmaf-build-matrix.yml still pins gcc-14; moving it surfaces its own crop of warnings across ten-plus legs and belongs in its own PR. Refs: ADR-1230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
Implements upstream Netflix/vmaf#818:
VMAF_POOL_METHOD_MEDIAN,PERC5,PERC10and
PERC20, evaluated by sort plus linear rank interpolation (matching NumPy'sdefault
percentile).This is the pooling half of #1311, split out. That PR bundled ADR-1181
(percentile pooling) with ADR-1182 (Windows UTF-8 path contract). The two are
independent, and bundling them dragged
model_loader.c,vmaf_roi.candvmaf_per_shot.cinto the touched-file set purely to route theirfopencallsthrough the shim — three files carrying ~71 pre-existing clang-tidy findings that
rule 12 then made this PR's problem. The UTF-8 shim follows separately.
Both writers loop
for (j = 1; j < VMAF_POOL_METHOD_NB; j++), so extending theenum adds an attribute per method to every
<metric>element and every JSONpool object:
There is no flag to select a subset. The four pre-existing values do not move —
verified byte-identical on the 576x324 pair with and without frame skipping — so a
reader that looks an attribute up by name is unaffected; one that matches a
<metric>line exactly, or rejects unknown attribute names, must be updated.python/test/command_line_test.pycarried three such exact-match assertions and isupdated here. The scores in them are unchanged — only the four new attributes
are appended. That file is not one of the five Netflix golden-assertion files in
CLAUDE.md §8, and no
assertAlmostEqualvalue is touched anywhere in this PR.Also in here
output.cppis now clang-tidy clean (29 findings → 0). Most were style, but8 were
bugprone-suspicious-stringview-data-usage:fmt_or_default()returned astd::string_viewwhose.data()went straight tostd::fprintfas a formatstring.
string_view::data()carries no null-termination guarantee, so that isUB the moment the view is built from a non-terminated buffer. It returns
const char *now — latent rather than live, since today's callers all passterminated data, but worth removing while the file is open.
test_pooling_percentileskips instead of failing without its fixtures. TheNetflix golden YUVs are untracked (
scripts/test/fetch-test-yuvs.shfetchesthem), and only the golden-harness job restores them — so the sanitizer, coverage
and ARM legs had no file to open. It now probes and sets
mu_skipped(exit 77).It also no longer leaks its model and context on early returns, which under
LeakSanitizer had been turning one failed assertion into 96912 bytes of unrelated-
looking LSan output.
Reproducer / smoke-test command
Measured:
meson test --suite=fast117 Ok / 0 Fail;clang-tidyonoutput.cpp29 → 0.
Deep-dive deliverables (ADR-0108)
## Alternatives considered.AGENTS.mdinvariant note — no rebase-sensitive invariants beyond the rebase note below, which carries all four.changelog.d/added/pooling-percentile-methods.md, with the breaking schema change called out;CHANGELOG.mdregenerated.Docs (rule 10)
docs/usage/cli.mddocuments the new pooled attributes andtells readers to tolerate new attribute names;
docs/api/index.mdcovers the enum additions.State (rule 13)
docs/state.md:T-UPSTREAM-818-POOLING-ENUM-NO-PERCENTILES-2026-09-03moves fromOpen bugs to Recently closed, citing this PR.
🤖 Generated with Claude Code