feat(core): add percentile temporal pooling to the public C API by lusoris · Pull Request #1340 · VMAFx/vmafx

lusoris · 2026-09-05T23:56:35Z

Summary

enum VmafPoolingMethod exposed only {UNKNOWN, MIN, MAX, MEAN, HARMONIC_MEAN}, so a
C-API, Rust-binding or FFmpeg caller could not ask for the median / perc5 / perc10 /
perc20 summaries the Python harness has always offered — any discriminant past
HARMONIC_MEAN was rejected with -EINVAL. This appends the four order-statistic
enumerators (ABI-safe: existing values are unchanged), computes them by sorting the pooled
per-frame scores and interpolating linearly between ranks — the same
numpy.percentile(method="linear") rule the harness uses — and leaves the golden
accumulator path byte-identical. Closes the verified ledger row
T-UPSTREAM-818-POOLING-ENUM-NO-PERCENTILES-2026-09-03 (upstream Netflix/vmaf#818).

Type

feat — new feature

Checklist

Commits follow Conventional Commits (the commit-msg hook enforces this).
make format && make lint is green locally.
Unit tests pass: meson test -C build.
If I touched any SIMD/GPU code path, I ran /cross-backend-diff and the worst ULP is ≤ 2.
If I touched a feature extractor with SIMD/GPU twins, I either updated every twin or listed the gap under "Known follow-ups" below.
If I added a new .c / .cpp / .cu / .h / .hpp, it has the appropriate license header (see CONTRIBUTING.md).
If this is a breaking change, the commit message uses ! or BREAKING CHANGE: and the migration path is documented below.
If this PR adds an ADR, the ADR row lives in docs/adr/_index_fragments/<NNNN-slug>.md and the slug is appended to docs/adr/_index_fragments/_order.txt.

No SIMD/GPU code path is touched — pooling is backend-independent host code that reads the
feature collector, so there is no twin to update and no cross-backend ULP table to paste.
This is not a breaking change: the enum grows append-only and VMAF_POOL_METHOD_NB (already
documented as an unstable count sentinel) moves 5 → 9.

Bug-status hygiene (ADR-0165)

docs/state.md updated in this PR: the
T-UPSTREAM-818-POOLING-ENUM-NO-PERCENTILES-2026-09-03 row moves from Open bugs to
Recently closed.

Netflix golden-data gate (ADR-0024)

I did not modify any assertAlmostEqual(...) score in the Netflix golden Python tests.

The full gate was run locally on the CPU backend: 271 passed, 12 skipped across
quality_runner_test.py, feature_extractor_test.py, vmafexec_test.py,
vmafexec_feature_extractor_test.py and result_test.py. The accumulator methods keep
their exact upstream float expressions (ADR-1118 golden-gate isolation), and
vmaf_percentile() stays static inline so predict.c's golden-asserted bootstrap
ci_p95 arithmetic is still compiled in its own translation unit.

Cross-backend numerical results

n/a — host-side pooling only; no SIMD or GPU kernel is touched.

Performance (if `perf` or `feat`)

MIN / MAX / MEAN / HARMONIC_MEAN are unchanged: same expressions, O(1) space, zero
allocation. Only the new order-statistic methods allocate — 8 × n_frames bytes, grown
geometrically, plus one qsort.

Deep-dive deliverables (ADR-0108)

Research digest — docs/research/1188-percentile-pooling-methods.md.
Decision matrix — docs/adr/1188-percentile-pooling-methods.md ## Alternatives considered (five options).
AGENTS.md invariant note — core/src/AGENTS.md, "Pooling: accumulators stay O(1) and byte-identical; percentiles buffer (ADR-1188)".
Reproducer / smoke-test command — pasted below under "Reproducer".
CHANGELOG fragment — changelog.d/added/percentile-pooling-methods.md.
Rebase note — docs/rebase-notes.md, entry fix/t-upstream-818-pooling-enum-no-percentil.

Reproducer

# Before: every percentile discriminant is rejected. After: they pool.
meson setup build-cpu core -Denable_cuda=false -Denable_sycl=false
ninja -C build-cpu
./build-cpu/test/test_pool_percentile          # 6 tests run, 6 passed
meson test -C build-cpu --suite=fast           # 115/115 OK

# The C perc10 of the golden pair's frames vs the Python harness's golden constant:
CUDA_VISIBLE_DEVICES="" VMAF_FORCE_BACKEND=cpu PYTHONPATH=$PWD/python \
  python3 -m pytest python/test/quality_runner_test.py -k pool_perc10 -q

# ffmpeg patch series still replays onto pristine n9.0.1 (18/18):
git -C /path/to/ffmpeg checkout -B verify n9.0.1
for p in ffmpeg-patches/00*-*.patch; do git -C /path/to/ffmpeg am --3way "$p" || break; done

Pre-fix evidence (origin/master sources, same probe binary, discriminants 5–8 passed
through a cast because the enumerators did not exist yet):

MEDIAN(5)  rc=-22 score=-1.000000
PERC5(6)   rc=-22 score=-1.000000
PERC10(7)  rc=-22 score=-1.000000
PERC20(8)  rc=-22 score=-1.000000

Post-fix, same probe:

MEDIAN(5)  rc=0 score=2.500000
PERC5(6)   rc=0 score=1.150000
PERC10(7)  rc=0 score=1.300000
PERC20(8)  rc=0 score=1.600000

Known follow-ups

The vmaf CLI still has no --pool flag; its report carries min / max / mean /
harmonic_mean and its stdout line reports the mean. docs/reference/faq.md claimed such
a flag existed and has been corrected. Adding one — and/or widening the pooled_metrics
schema to carry percentiles — is a deliberate output-surface decision left out of this PR
(ADR-1188 "Neutral / follow-ups").

…-03 (PR #1340) Move the pooling-enum row from Open bugs to Recently closed now that the four order-statistic enumerators ship (ADR-1188), and point the Confirmed not-affected entry for Netflix/vmaf#818 at the closed row. Per ADR-0165 the ledger update lands in the same PR as the fix.

lusoris · 2026-09-06T09:28:21Z

Duplicate resolution: this PR is the keeper for percentile temporal pooling

#1311 implements the same feature (its Part A, ADR-1181). Comparing the two diffs, this
PR wins on every axis and #1311's percentile half should be dropped — full evidence in
#1311 (comment). Summary:

feat(core): percentile pooling and Windows UTF-8 path contract (ADR-1181/1182) #1311 sizes its score buffer malloc((index_high - index_low + 1) * sizeof(double)),
which wraps to malloc(0) on the 0, UINT_MAX idiom this repo documents at
docs/api/index.md:611 and then writes 8 * n_frames bytes past the end. This PR's
geometric pool_samples_push does not.
feat(core): percentile pooling and Windows UTF-8 path contract (ADR-1181/1182) #1311 leaves output.cpp looping [1, VMAF_POOL_METHOD_NB), so raising NB 5 → 9
silently adds four keys to every pooled_metrics JSON/XML block. This PR's
pool_report_order[] keeps the reported set — and the emitted bytes — unchanged.
6 fixture-free tests here (exact numpy.percentile agreement at 1e-12, n_subsample
parity, order-independence, invalid-discriminant rejection, ABI append-only pin) vs 2
in feat(core): percentile pooling and Windows UTF-8 path contract (ADR-1181/1182) #1311, one of which needs the YUV fixture and a loaded model and asserts at 1e-4.
Research digest present (ADR-0108 (1)); VMAF_HAVE_PERCENTILE_POOLING feature macro;
namespaced vmaf_percentile / vmaf_score_compare; ffmpeg mapping added as an
appended, macro-guarded patch 0018 rather than mutating 0005/0006/0013.

Both implementations produce the same numbers (#1311's 72.71734120 vs this PR's
72.717340155042217 for perc10 on the golden pair), so this is a completeness and
memory-safety decision, not a disagreement about the formula.

#1311 stays open — it also carries the unrelated Windows UTF-8 path contract
(ADR-1182 / Netflix#1568), which nothing else covers. It has been asked to de-scope to
that half.

Ledger: both PRs move T-UPSTREAM-818-POOLING-ENUM-NO-PERCENTILES-2026-09-03 to
Recently closed in docs/state.md. This PR keeps that row; #1311 drops it. Only one row
per id survives, or scripts/ci/check-state-md-rows.sh fails.

Follow-up to pick up here or in a separate PR: #1311 is the only place the Go binding
learns the new methods — PoolMethod / String() / ParsePoolMethod / toC() in
pkg/libvmaf/direct.go, the ScoreDirectRequest.PoolMethod and StreamConfig.PoolMethod
fields that replace the hardcoded C.VMAF_POOL_METHOD_MEAN in ScoreDirect and
StreamScorer.Finish, and the two table tests. This PR covers the Rust side
(bindings/rust/vmafx/src/score.rs) but not Go. That work should be re-targeted here
rather than lost when #1311 is de-scoped.

…-03 (PR #1340) Move the pooling-enum row from Open bugs to Recently closed now that the four order-statistic enumerators ship (ADR-1188), and point the Confirmed not-affected entry for Netflix/vmaf#818 at the closed row. Per ADR-0165 the ledger update lands in the same PR as the fix.

…t tidy-clean Three follow-ups found while rebasing this branch onto current master: - docs/state.md: the earlier "drop the duplicate rows a keep-both rebase created" commit removed the wrong copy — it deleted the branch's new Recently-closed row instead of the stale Open-bugs row, leaving the bug marked open while the moved-to comment claimed otherwise. Move the row for real (ADR-0165: exactly one row per id, citing PR #1340). - core/test/test_pool_percentile.c: the file is new, so its clang-tidy ratchet baseline is 0 and its 18 findings were a regression (ADR-0141 / ADR-1142). Drive the expectations off PoolCase tables through a shared helper so each test body stays inside readability-function-size's 15-branch budget, use nullptr, and drop the deprecated VMAF_POOL_METHOD_NB references. The one remaining finding is the deliberate out-of-range discriminant cast, which is the subject of the negative test; it carries an inline ADR-1188 citation. - docs/reference/faq.md: the list names eight pooling methods, not seven. Behaviour is unchanged: 6/6 in test_pool_percentile, 115/115 in --suite=fast.

lusoris · 2026-09-06T10:38:43Z

Queue audit (read-only verification pass, 2026-09-06). Two findings on this PR, both checked locally rather than inferred.

1. ADR-1138 violation — nullptr in a C translation unit.
core/test/test_pool_percentile.c uses the C23 nullptr keyword on 16 code lines (return nullptr;, VmafContext *vmaf = nullptr;, ...). ADR-1138 (Accepted) is explicit that C translation units keep NULL, because the documented MSVC /std:clatest C23 feature set does not include nullptr and the required Windows MSVC + CUDA leg builds core/ with ninja -v -C core\build install (which builds the test executables too). No other C TU in the tree uses the keyword: every nullptr hit under core/**/*.c on master sits inside an ADR-1138 NOLINT comment. PR #1338 had to make exactly this fix twice (fd101e6fd, f8fcde881). The fix is mechanical: nullptr -> NULL.

2. Duplicate feature with PR #1311.
#1311 Part A implements the same four enumerators (VMAF_POOL_METHOD_MEDIAN / _PERC5 / _PERC10 / _PERC20), the same numpy-linear interpolation rule, and closes the same ledger row T-UPSTREAM-818-POOLING-ENUM-NO-PERCENTILES-2026-09-03. Both PRs add an ADR with the identical slug (docs/adr/1181-percentile-pooling-methods.md vs docs/adr/1188-percentile-pooling-methods.md). git merge-tree between the two heads conflicts in 8 files, including core/include/libvmaf/libvmaf.h, core/src/predict.c, core/src/output.cpp and docs/state.md. Only one of the two can land; that is a maintainer call, not something a rebase resolves.

Otherwise clean: no merge conflict against current master, the six-deliverable gate passes locally, and the new test file measures 0 clang-tidy findings under the repo .clang-tidy at -std=c23.