Skip to content

fix(cuda): push context in cambi submit and gate twin selection on model options (ADR-1183) - #1312

Merged
lusoris merged 5 commits into
masterfrom
fix/cambi-cuda-context
Sep 5, 2026
Merged

lusoris merged 5 commits into
masterfrom
fix/cambi-cuda-context

Conversation

@lusoris

@lusoris lusoris commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the 100% crash on vmaf --backend cuda with the default model (vmaf_v1.0.16_3d0h, ADR-1169). The root cause was twofold: (1) submit_fex_cuda() in core/src/feature/cuda/integer_cambi_cuda.c performed the GPU pipeline and synchronous cuMemcpyDtoH without pushing fex->cu_state->ctx onto the calling worker/batch thread, causing CUDA_ERROR_INVALID_CONTEXT (201); (2) vmaf_fex_ctx_parse_options() silently dropped unrecognized model options, causing integer_adm_cuda (which lacks adm_csf_mode) to run with mode 0 while the model specified mode 2, producing mismatched feature names. This PR pushes/pops the CUDA context around CAMBI submit, adds strict option validation returning -EINVAL for unknown extractor options, implements option-aware GPU twin dispatch (ADR-1183) falling back to the CPU twin when model options are unsupported, routes integer_motion_cuda scores via feature_name_dict to support both motion2 and motion3, and adds unit and integration tests.

2026-09-05 13:20 — history rewritten: the branch was squashed to one commit and force-pushed because gitleaks scans every branch of the repo and the test's adm3_key = "integer_adm3_csf_2_…" / cambi_key = "cambi_hrs_…" lines tripped generic-api-key on every PR (#1284's scan went red on a commit from this branch). The variables are renamed and .gitleaks.toml allowlists VMAF pooled-metric names; the earlier per-commit references above therefore no longer resolve.

Type

  • fix — bug fix
  • cuda — backend-specific

Checklist

  • Commits follow Conventional Commits.
  • 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.
  • 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.txtdo not edit docs/adr/README.md directly.

Bug-status hygiene (ADR-0165)

  • docs/state.md updated: T-CAMBI-CUDA-INVALID-CONTEXT-2026-09-04 and T-GPU-TWINS-IGNORE-MODEL-OPTIONS-2026-09-05 moved to Recently closed; T-GPU-ADM-CSF-MODE-NOT-PORTED-2026-09-05 added to Open.

Netflix golden-data gate (ADR-0024)

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

Cross-backend numerical results

Ran CPU vs CUDA comparison on 576x324 Netflix pair (src01_hrc00 vs src01_hrc01) with default model vmaf_v1.0.16_3d0h:

  • vmaf: CPU = 82.816059, CUDA = 82.814059 (abs delta: 0.002000)
  • cambi: CPU = 11.233516, CUDA = 11.230853 (abs delta: 0.002663)
  • adm3: CPU = 0.902996, CUDA = 0.902996 (bit-exact match via CPU fallback per ADR-1183)
  • motion3: CPU = 8.163351, CUDA = 8.163352 (abs delta: 0.000001)
  • speed_chroma: CPU = 0.000000, CUDA = 0.000001 (abs delta: 0.000001)

Ran CPU vs CUDA comparison on 1080p Tennis pair (Tennis_24fps.yuv vs Tennis_20_288_375.yuv, 10 frames):

  • vmaf: CPU = 41.459893, CUDA = 41.445641 (abs delta: 0.014252)
  • cambi: CPU = 16.518621, CUDA = 16.500987 (abs delta: 0.017634)
  • adm3: CPU = 0.771923, CUDA = 0.771923 (bit-exact match)
  • motion3: CPU = 1.636750, CUDA = 1.636762 (abs delta: 0.000012)
  • speed_chroma: CPU = 0.000000, CUDA = 0.000000 (abs delta: 0.000000)

Deep-dive deliverables (ADR-0108)

  • Research digest — no digest needed: bug fix and twin option negotiation pattern (ADR-1183).
  • Decision matrix — captured in docs/adr/1183-model-options-gate-gpu-twin-selection.md § Alternatives considered.
  • AGENTS.md invariant note — added to core/src/feature/AGENTS.md.
  • Reproducer / smoke-test command — included below under Reproducer.
  • CHANGELOG fragmentchangelog.d/fixed/cambi-cuda-context.md, changelog.d/fixed/model-options-unknown-key-error.md.
  • Rebase note — entry added to docs/rebase-notes.md.

Reproducer

# 1. Reproducer: run default model on CUDA (previously failed with CUDA_ERROR_INVALID_CONTEXT in integer_cambi_cuda.c:907)
core/build/tools/vmaf \
  -r python/test/resource/yuv/src01_hrc00_576x324.yuv \
  -d python/test/resource/yuv/src01_hrc01_576x324.yuv \
  -w 576 -h 324 -p 420 -b 8 \
  --backend cuda --model version=vmaf_v1.0.16_3d0h --json

# 2. Pytest suite for CUDA default model
pytest python/test/cuda_default_model_test.py -v

# 3. Unit test for option validation error
meson test -C core/build test_feature_extractor

Known follow-ups

  • T-GPU-ADM-CSF-MODE-NOT-PORTED-2026-09-05: Implement CSF mode 2 in integer_adm_cuda and integer_adm_sycl so that the default model can run ADM on GPU silicon instead of falling back to CPU.
  • T-FEATURE-FLAG-IGNORES-BACKEND-2026-09-04: Upstream Netflix behavior is preserved where --feature <name> uses vmaf_get_feature_extractor_by_name() returning CPU extractors; documented in docs/usage/cli.md and docs/backends/cuda/overview.md.

🤖 Generated with Claude Code

@lusoris lusoris added this to the 1.0.0 — First release milestone Sep 5, 2026
@lusoris
lusoris force-pushed the fix/cambi-cuda-context branch 2 times, most recently from 635643f to 6ed95b4 Compare September 5, 2026 16:05
@lusoris
lusoris marked this pull request as ready for review September 5, 2026 16:20
@lusoris
lusoris marked this pull request as draft September 5, 2026 20:32
lusoris pushed a commit that referenced this pull request Sep 5, 2026
Two `Rules` gates caught the previous commit:

- `docs/state.md Gate` (ADR-0334) rejects a placeholder PR reference in an
  inserted row. The `T-TIDY-CHANGED-LTO-FLAG-2026-09-05` row said "This PR
  was the first C/C++ PR after #1290"; it now names PR #1312.
- `Release Script Contract` requires `CHANGELOG.md`'s Unreleased block to
  match `changelog.d/`. Re-rendered with
  `scripts/release/concat-changelog-fragments.sh --write`, which picks up
  `changelog.d/fixed/tidy-changed-lto-flag.md`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris marked this pull request as ready for review September 5, 2026 20:45
@lusoris
lusoris marked this pull request as draft September 5, 2026 21:01
@lusoris
lusoris force-pushed the fix/cambi-cuda-context branch from 0f59ed9 to a8d69e8 Compare September 5, 2026 21:15
@lusoris
lusoris marked this pull request as ready for review September 5, 2026 21:16
@lusoris
lusoris marked this pull request as draft September 5, 2026 21:39
Lusoris and others added 4 commits September 6, 2026 00:13
…ption validation, CPU-twin dispatch (ADR-1183)

- integer_cambi_cuda.c: push/pop the CUDA context around the submit path (the DtoH
  readback ran with no current context: CUDA_ERROR_INVALID_CONTEXT), mirror the CPU
  cambi option table (cambi_high_res_speedup/hrs, vlt, cmxv) and the TVI table init
  from cambi.c so the emitted feature name matches the model's.
- integer_motion_cuda.c: route scores through the feature_name_dict so motion3 emits
  integer_motion3_mmxv_18 like the CPU twin.
- feature_extractor.cpp: an opts_dict key the chosen extractor does not declare is an
  error (-EINVAL, logged) instead of being silently dropped.
- libvmaf.c: when a GPU twin lacks an option the model sets, resolve the CPU twin for
  that feature and log it (ADR-1183) — adm3 of vmaf_v1.0.16_3d0h runs on the CPU under
  CUDA/SYCL/HIP until the csf_mode port lands (docs/state.md
  T-GPU-ADM-CSF-MODE-NOT-PORTED-2026-09-05).
- python/test/cuda_default_model_test.py (new, skips without CUDA); .gitleaks.toml
  allowlists VMAF pooled-metric names (the test's `<name>_key = "<metric>"` lines
  tripped generic-api-key on every PR because gitleaks scans all branches).
Verified on the RTX 4090: 576x324 and 1080p reproducers exit 0; adm3 bit-exact vs CPU,
cambi delta 2.66e-3 / 1.76e-2 (tracked), v0.6.1 unchanged, typo option rejected;
golden gate 271 passed / 12 skipped / 0 failed.
The ADR-1183 GPU-twin fallback pushed vmaf_use_features_from_model past the
readability-function-size threshold and the two new test cases in
test_feature_extractor.c added eight uncited modernize-use-nullptr findings, so
the whole-tree ratchet (ADR-1142) refused the PR.

- The fallback moves into fex_honouring_model_options(), a helper that returns
  the extractor able to honour every option the model sets for one feature (the
  GPU twin, or the CPU twin with an INFO line, or NULL when nothing provides the
  feature). vmaf_use_features_from_model is back under the threshold and reads
  as one loop again.
- The new tests' NULL arguments and returns carry the ADR-1138 citation the
  other C TUs use (MSVC /std:clatest has no C nullptr).
- feature_extractor.cpp lost four findings to the option-validation refactor, so
  its baseline entry is tightened 54 -> 50 with the count CI measured (the local
  clang is a different version and is not the authority).

Behaviour is unchanged: the CUDA default-model run still exits 0 with the CPU
dispatch line for adm3, test_feature_extractor is 13/13.
The touched-file cleanup rule (ADR-0141) makes this PR inherit every
pre-existing clang-tidy finding in the test TUs it edits.  Discharge all
59 in `core/test/test_feature_extractor.c`:

* modernize-use-nullptr (54): replace the 17 scattered, partly stale
  `NOLINTNEXTLINE` markers (several of which sat above comment lines and
  suppressed nothing) with the single file-scoped
  `NOLINTBEGIN(modernize-use-nullptr)` / `NOLINTEND` bracket that
  ADR-1138 prescribes and that `core/src/libvmaf.c`,
  `core/test/test_output.c` and the other C TUs already use.  The TU
  keeps spelling the null pointer constant `NULL`: the required MSVC
  C lane has no documented C `nullptr`, and upstream parity matters for
  the sync story.

* readability-function-size (5): split the oversized cases into one
  behaviour per test instead of suppressing them.
  - `test_feature_extractor_flush` and
    `test_feature_extractor_initialization_options` now share a
    `FexFixture` (context + ref/dist pictures + collector) with
    `fex_fixture_open` / `fex_fixture_close` /
    `fex_fixture_extract_two_frames_and_flush` helpers.
  - `test_fex_vector_dedup_by_provided_feature_name` gained a
    `fex_vector_create_and_append` helper.
  - `test_feature_extractor_supports_options_helper` split into
    `test_supports_options_empty_dict`, `_known_key`,
    `_unknown_key_reports_missing`, `_alias_key` and `_null_extractor`.
  - the SpEED GPU-twin registry guard moved out of
    `test_get_feature_extractor_by_name_and_feature_name` into
    `test_speed_gpu_twins_resolve_by_name`, which also keeps that case
    inside budget in an all-backends build.
  - `run_tests` now delegates to `run_registry_tests`,
    `run_context_tests` and `run_option_tests`, the grouping pattern
    `test_cli_parse.c` and `test_iqa_helpers.c` already use.

No assertion was dropped: 17 cases / 79 assertions become 18 cases / 88
assertions, and the conditions folded into the fixture helpers are still
checked there and reported through the fixture's status.  The Netflix
golden assertions are untouched, and the PR's CUDA CAMBI fix is
unaffected.

Verified: `clang-tidy -p core/build core/test/test_feature_extractor.c`
59 -> 0 warnings (clang-tidy 22.1.8); `core/build/test/test_feature_extractor`
18/18 pass; `meson test -C core/build --suite fast --no-suite gpu`
115/115 Ok; `pre-commit run --files` clean.  Baseline tightened in
`scripts/ci/tidy-baseline-cpu.json` (entry removed, total 3060 -> 3001)
per the ADR-1142 ratchet, and a rebase note recorded for the new group
runners and the NOLINT bracket.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The touched-file cleanup rule (ADR-0141) makes this PR inherit every
pre-existing clang-tidy finding in the TUs it edits.  Discharge all 51 in
`core/src/feature/feature_extractor.cpp` with no `NOLINT`:

* modernize-use-nullptr (29): this TU is C++, so ADR-1138's keep-`NULL`
  exemption (which exists for the required MSVC C lane and upstream C
  parity) does not apply — spell the null pointer constant `nullptr`.
  Prose in comments still says "NULL" where it refers to the C twin.

* misc-const-correctness (14): `const` on the error-code locals, on
  `ctx_array_sz`, and on the three read-only pointees (`ref_priv` in
  extract, `fex` in `vmaf_fex_ctx_pool_release` / `_flush`).

* misc-use-anonymous-namespace (5): move `feature_extractor_list[]`,
  `vmaf_fex_ctx_parse_options`, `get_fex_list_entry`,
  `ctx_pool_ensure_slot_ctx` and `ctx_pool_claim_slot` out of `static`
  and into anonymous namespaces.  The C twin keeps `static`.

* readability-function-size (2): split rather than suppress.
  - `vmaf_feature_extractor_context_extract` (85 lines / 16 branches):
    the CUDA and HIP picture buf_type validation moves verbatim into
    `check_pic_buf_type(fex, ref_priv)`, which returns 0 or -EINVAL.
    Same predicates, same log messages, same return value.
  - `get_fex_list_entry` (68 lines) splits into `find_fex_list_entry`
    (the existing-entry lookup), `grow_fex_list` (the geometric realloc
    plus placement-new of the fresh slots) and `init_fex_list_slot`
    (capacity/condvar/ctx_list/opts_dict init, with the same unwind on
    each failure).  The helpers return an errno; `get_fex_list_entry`
    still returns nullptr on any failure, which the caller still turns
    into -EINVAL, and `pool->cnt` is still bumped only on success.

* cert-dcl03-c,misc-static-assert (1): clang-tidy 22 flags every
  `assert()` whose condition contains no non-constexpr call, so
  `assert(pool->capacity > 0)` cannot stay.  Replaced with a hard
  `if (pool->capacity == 0) return -EINVAL;` in `grow_fex_list` — an
  always-on guard is strictly stronger than a check compiled out under
  NDEBUG, and the state is unreachable either way (`pool_create` seeds
  capacity = 8 and this is its only writer).  `<cassert>` is now unused
  and dropped.

Dropping `static` from the registry declaration broke the
`check-dispatch-registry` pre-commit / CI gate, whose `sed` range
hard-coded the `static VmafFeatureExtractor *feature_extractor_list[]`
spelling; it now accepts both spellings, with a new self-test case
(Test 4) covering the anonymous-namespace form.

No behaviour change: the extractor registry, the context pool and the
option validation this PR adds all keep their exact control flow, and
the CUDA CAMBI fix is untouched.

Verified: `clang-tidy -p core/build core/src/feature/feature_extractor.cpp`
51 -> 0 warnings (clang-tidy 22.1.8, CUDA-enabled build) and 0 in a
CPU-only build dir matching the CI lane; `core/build/test/test_feature_extractor`
18/18 pass; `meson test -C core/build --suite fast --no-suite gpu` 115/115 Ok;
`bash scripts/ci/tests/test-check-dispatch-registry.sh` 11/11 pass;
`pre-commit run --files` clean.  Baseline tightened in
`scripts/ci/tidy-baseline-cpu.json` (entry removed, total 3001 -> 2951)
per the ADR-1142 ratchet, and a rebase note recorded for the anonymous
namespaces and the two function splits.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the fix/cambi-cuda-context branch from a8d69e8 to ce6f1c7 Compare September 5, 2026 22:13
@lusoris
lusoris marked this pull request as ready for review September 5, 2026 22:13
`core/src/feature/feature_extractor.h` was the last file this branch touches
that still carried clang-tidy findings (8). clang-tidy has no compile command
for a header and falls back to the nearest matching TU's —
`feature_extractor.cpp` — so it parses this C header as C++ and proposes
rewrites that would break the roughly one hundred C translation units which
include it: `<cstdint>` / `<cstdlib>` for the C headers, `using` for the
`typedef struct`s, a C23 fixed underlying type on the two flag enums (which
MSVC does not document under `/std:clatest`, and the Windows MSVC build is a
required check), and a rename of the upstream `__VMAF_FEATURE_EXTRACTOR_H__`
include guard that every other header under `core/src/` also uses.

Discharged with one file-scoped `NOLINTBEGIN` / `NOLINTEND` bracket — the
shape ADR-1138 prescribes and `core/test/test_feature_extractor.c` already
uses — whose justification and its ADR-0141 / ADR-1138 / ADR-0148 / ADR-0150
citations live inside the marker's own block comment, where
`scripts/ci/tidy-ratchet.py::count_uncited_nolints` looks for them. No
declaration, macro, type or guard changed, so no includer is affected.

The `-Db_lto=false` half of the original commit is dropped: PR #1326 landed
the identical fix on master while this branch was in review.

Verified: clang-tidy 22.1.8 against a CPU-only build dir configured like the
CI lane reports 0 warnings, 0 errors, exit 0 for all four CI-linted files
(`feature_extractor.cpp`, `feature_extractor.h`, `libvmaf.c`,
`test_feature_extractor.c`); `count_uncited_nolints` on the header returns 0.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@lusoris
lusoris force-pushed the fix/cambi-cuda-context branch from ce6f1c7 to 97575f1 Compare September 5, 2026 22:15
@lusoris
lusoris merged commit fcae339 into master Sep 5, 2026
78 of 79 checks passed
@lusoris
lusoris deleted the fix/cambi-cuda-context branch September 5, 2026 22:35
lusoris pushed a commit that referenced this pull request Sep 6, 2026
…195)

CLAUDE.md rule 15 says to rebuild the container when "its image predates the
last master sync". That is stated in terms of time, and time cannot answer the
question. A build run against a checkout that is behind master produces an
image newer than every commit in the repository and missing exactly the work
it was rebuilt for.

That happened today. The container was rebuilt specifically to pick up the GPU
default-model fixes (#1307, #1312, #1324) so the epic #1246 GPU smoke could run
against them. The build succeeded, the image was the newest thing on disk, and
it contained none of the three: the build context was 28 commits behind
origin/master. It was caught only because a test file added by one of those PRs
was missing. Had the smoke run instead, it would have reported green numbers
for code that was not in the image, and those numbers would have been cited as
a retrain gate.

ADR-1102's marker answers "did a container build this?". It cannot answer
"which code was in that container?", and that second question is the one that
was wrong.

dev/Containerfile now records /etc/vmafx-dev-source (source_rev, source_ref,
source_repo) from a VMAFX_SOURCE_REV build argument supplied by
dev/docker-compose.yml. It is written in the LAST stage, deliberately away from
the ADR-1102 marker in the first: the first stage is reused by every rebuild,
so a marker there would report the revision of whichever build first populated
the layer cache -- authoritative and stale, which is worse than absent.

scripts/dev/check-container-source.sh answers it in both directions.
--pre-build refuses a checkout that is behind the reference and lists the
commits under baked-in paths the image would be missing; --image reads the
marker out of an existing image and reports current, stale (naming what is
missing), or unverifiable. A build that never received the argument records
`unknown`, and `unknown` is reported as "cannot verify", not as a pass: an
image that cannot say what it holds is not evidence.

dev/scripts/container-build.sh makes the correct path the easy one -- check,
build with the verified revision, re-verify the result. --allow-behind exists
for local experiments and says so loudly.

Verified by scripts/ci/tests/test-check-container-source.sh: 8 assertions,
hermetic apart from one Docker case that skips when no daemon is reachable.
The stale-context case reproduces today's shape; ahead-of-master is
deliberately allowed, since a feature branch legitimately leads master.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris pushed a commit that referenced this pull request Sep 6, 2026
… (ADR-1192)

Re-ran the Netflix benchmark suite on cd52f26 for epic #1245 items 1 and 5.
All three fixtures reproduce on CPU, CUDA and SYCL through the FFmpeg filter
path against a container-built current-master libvmaf, but every backend's
pooled score has drifted from testdata/netflix_benchmark_results.json (recorded
by PR #309 on 2026-05-02): CPU +2.83e-06, CUDA -1.07e-03, SYCL -1.40e-03 on the
576x324 pair. A rebuild of 5a08030 — the commit before the 2026-09-06 GPU
merges #1307/#1312/#1324 — shows the same drift, so none of it comes from
today's merges. The snapshot is deliberately NOT regenerated (ADR-1192) and no
throughput baseline is recorded, because the run also reproduced two
pre-existing GPU defects:

- vmaf --threads N aborts on every GPU backend (exit 234, "context could not be
  synchronized"); without --threads both CUDA and SYCL score correctly and are
  bit-stable over 10 runs. bench_all.sh hard-codes --threads 1.
- The libvmaf_cuda FFmpeg filter returns a wrong pooled score in 10 of 40 runs
  on master and 8 of 40 on 5a08030 — inside binomial noise of each other.

Harness fixes in the same change:

- bench_all.sh kept its stderr on /dev/null and relabelled every non-zero exit
  as "backend likely unavailable", which is how a hard abort passed for a
  missing device for months. It now captures stderr per row and prints FAIL
  with the exit code and the real last line. Its flag sets also drop
  --no_vulkan, unrecognized since ADR-0726 removed the Vulkan backend.
- benchmark_netflix.py hard-coded /home/kilian/dev/ffmpeg-8/ffmpeg (gone) and
  /dev/dri/renderD130 for the SYCL/QSV import (now the AMD iGPU on the bench
  host, so the SYCL rows failed outright). Both are environment overrides now,
  VMAF_FFMPEG and the new VMAF_SYCL_RENDER_NODE, per the ADR-0792 pattern.

No golden assertions touched; no snapshot regenerated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris pushed a commit that referenced this pull request Sep 6, 2026
…195)

CLAUDE.md rule 15 says to rebuild the container when "its image predates the
last master sync". That is stated in terms of time, and time cannot answer the
question. A build run against a checkout that is behind master produces an
image newer than every commit in the repository and missing exactly the work
it was rebuilt for.

That happened today. The container was rebuilt specifically to pick up the GPU
default-model fixes (#1307, #1312, #1324) so the epic #1246 GPU smoke could run
against them. The build succeeded, the image was the newest thing on disk, and
it contained none of the three: the build context was 28 commits behind
origin/master. It was caught only because a test file added by one of those PRs
was missing. Had the smoke run instead, it would have reported green numbers
for code that was not in the image, and those numbers would have been cited as
a retrain gate.

ADR-1102's marker answers "did a container build this?". It cannot answer
"which code was in that container?", and that second question is the one that
was wrong.

dev/Containerfile now records /etc/vmafx-dev-source (source_rev, source_ref,
source_repo) from a VMAFX_SOURCE_REV build argument supplied by
dev/docker-compose.yml. It is written in the LAST stage, deliberately away from
the ADR-1102 marker in the first: the first stage is reused by every rebuild,
so a marker there would report the revision of whichever build first populated
the layer cache -- authoritative and stale, which is worse than absent.

scripts/dev/check-container-source.sh answers it in both directions.
--pre-build refuses a checkout that is behind the reference and lists the
commits under baked-in paths the image would be missing; --image reads the
marker out of an existing image and reports current, stale (naming what is
missing), or unverifiable. A build that never received the argument records
`unknown`, and `unknown` is reported as "cannot verify", not as a pass: an
image that cannot say what it holds is not evidence.

dev/scripts/container-build.sh makes the correct path the easy one -- check,
build with the verified revision, re-verify the result. --allow-behind exists
for local experiments and says so loudly.

Verified by scripts/ci/tests/test-check-container-source.sh: 8 assertions,
hermetic apart from one Docker case that skips when no daemon is reachable.
The stale-context case reproduces today's shape; ahead-of-master is
deliberately allowed, since a feature branch legitimately leads master.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris pushed a commit that referenced this pull request Sep 6, 2026
…195)

CLAUDE.md rule 15 says to rebuild the container when "its image predates the
last master sync". That is stated in terms of time, and time cannot answer the
question. A build run against a checkout that is behind master produces an
image newer than every commit in the repository and missing exactly the work
it was rebuilt for.

That happened today. The container was rebuilt specifically to pick up the GPU
default-model fixes (#1307, #1312, #1324) so the epic #1246 GPU smoke could run
against them. The build succeeded, the image was the newest thing on disk, and
it contained none of the three: the build context was 28 commits behind
origin/master. It was caught only because a test file added by one of those PRs
was missing. Had the smoke run instead, it would have reported green numbers
for code that was not in the image, and those numbers would have been cited as
a retrain gate.

ADR-1102's marker answers "did a container build this?". It cannot answer
"which code was in that container?", and that second question is the one that
was wrong.

dev/Containerfile now records /etc/vmafx-dev-source (source_rev, source_ref,
source_repo) from a VMAFX_SOURCE_REV build argument supplied by
dev/docker-compose.yml. It is written in the LAST stage, deliberately away from
the ADR-1102 marker in the first: the first stage is reused by every rebuild,
so a marker there would report the revision of whichever build first populated
the layer cache -- authoritative and stale, which is worse than absent.

scripts/dev/check-container-source.sh answers it in both directions.
--pre-build refuses a checkout that is behind the reference and lists the
commits under baked-in paths the image would be missing; --image reads the
marker out of an existing image and reports current, stale (naming what is
missing), or unverifiable. A build that never received the argument records
`unknown`, and `unknown` is reported as "cannot verify", not as a pass: an
image that cannot say what it holds is not evidence.

dev/scripts/container-build.sh makes the correct path the easy one -- check,
build with the verified revision, re-verify the result. --allow-behind exists
for local experiments and says so loudly.

Verified by scripts/ci/tests/test-check-container-source.sh: 8 assertions,
hermetic apart from one Docker case that skips when no daemon is reachable.
The stale-context case reproduces today's shape; ahead-of-master is
deliberately allowed, since a feature branch legitimately leads master.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 6, 2026
…195) (#1337)

* ci(dev): make the container say which source it was built from (ADR-1195)

CLAUDE.md rule 15 says to rebuild the container when "its image predates the
last master sync". That is stated in terms of time, and time cannot answer the
question. A build run against a checkout that is behind master produces an
image newer than every commit in the repository and missing exactly the work
it was rebuilt for.

That happened today. The container was rebuilt specifically to pick up the GPU
default-model fixes (#1307, #1312, #1324) so the epic #1246 GPU smoke could run
against them. The build succeeded, the image was the newest thing on disk, and
it contained none of the three: the build context was 28 commits behind
origin/master. It was caught only because a test file added by one of those PRs
was missing. Had the smoke run instead, it would have reported green numbers
for code that was not in the image, and those numbers would have been cited as
a retrain gate.

ADR-1102's marker answers "did a container build this?". It cannot answer
"which code was in that container?", and that second question is the one that
was wrong.

dev/Containerfile now records /etc/vmafx-dev-source (source_rev, source_ref,
source_repo) from a VMAFX_SOURCE_REV build argument supplied by
dev/docker-compose.yml. It is written in the LAST stage, deliberately away from
the ADR-1102 marker in the first: the first stage is reused by every rebuild,
so a marker there would report the revision of whichever build first populated
the layer cache -- authoritative and stale, which is worse than absent.

scripts/dev/check-container-source.sh answers it in both directions.
--pre-build refuses a checkout that is behind the reference and lists the
commits under baked-in paths the image would be missing; --image reads the
marker out of an existing image and reports current, stale (naming what is
missing), or unverifiable. A build that never received the argument records
`unknown`, and `unknown` is reported as "cannot verify", not as a pass: an
image that cannot say what it holds is not evidence.

dev/scripts/container-build.sh makes the correct path the easy one -- check,
build with the verified revision, re-verify the result. --allow-behind exists
for local experiments and says so loudly.

Verified by scripts/ci/tests/test-check-container-source.sh: 8 assertions,
hermetic apart from one Docker case that skips when no daemon is reachable.
The stale-context case reproduces today's shape; ahead-of-master is
deliberately allowed, since a feature branch legitimately leads master.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(state): cite PR #1337 in the container-staleness row (ADR-0165 touch gate)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(state): drop the duplicate rows a keep-both rebase created

Each dropped row restates one origin/master already carries; master is the
authoritative record. Verified with scripts/ci/check-state-md-rows.sh.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
lusoris pushed a commit that referenced this pull request Sep 6, 2026
… (ADR-1192)

Re-ran the Netflix benchmark suite on cd52f26 for epic #1245 items 1 and 5.
All three fixtures reproduce on CPU, CUDA and SYCL through the FFmpeg filter
path against a container-built current-master libvmaf, but every backend's
pooled score has drifted from testdata/netflix_benchmark_results.json (recorded
by PR #309 on 2026-05-02): CPU +2.83e-06, CUDA -1.07e-03, SYCL -1.40e-03 on the
576x324 pair. A rebuild of 5a08030 — the commit before the 2026-09-06 GPU
merges #1307/#1312/#1324 — shows the same drift, so none of it comes from
today's merges. The snapshot is deliberately NOT regenerated (ADR-1192) and no
throughput baseline is recorded, because the run also reproduced two
pre-existing GPU defects:

- vmaf --threads N aborts on every GPU backend (exit 234, "context could not be
  synchronized"); without --threads both CUDA and SYCL score correctly and are
  bit-stable over 10 runs. bench_all.sh hard-codes --threads 1.
- The libvmaf_cuda FFmpeg filter returns a wrong pooled score in 10 of 40 runs
  on master and 8 of 40 on 5a08030 — inside binomial noise of each other.

Harness fixes in the same change:

- bench_all.sh kept its stderr on /dev/null and relabelled every non-zero exit
  as "backend likely unavailable", which is how a hard abort passed for a
  missing device for months. It now captures stderr per row and prints FAIL
  with the exit code and the real last line. Its flag sets also drop
  --no_vulkan, unrecognized since ADR-0726 removed the Vulkan backend.
- benchmark_netflix.py hard-coded /home/kilian/dev/ffmpeg-8/ffmpeg (gone) and
  /dev/dri/renderD130 for the SYCL/QSV import (now the AMD iGPU on the bench
  host, so the SYCL rows failed outright). Both are environment overrides now,
  VMAF_FFMPEG and the new VMAF_SYCL_RENDER_NODE, per the ADR-0792 pattern.

No golden assertions touched; no snapshot regenerated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 6, 2026
… (ADR-1192) (#1334)

* fix(testdata): make the Netflix benchmark harness honest and portable (ADR-1192)

Re-ran the Netflix benchmark suite on cd52f26 for epic #1245 items 1 and 5.
All three fixtures reproduce on CPU, CUDA and SYCL through the FFmpeg filter
path against a container-built current-master libvmaf, but every backend's
pooled score has drifted from testdata/netflix_benchmark_results.json (recorded
by PR #309 on 2026-05-02): CPU +2.83e-06, CUDA -1.07e-03, SYCL -1.40e-03 on the
576x324 pair. A rebuild of 5a08030 — the commit before the 2026-09-06 GPU
merges #1307/#1312/#1324 — shows the same drift, so none of it comes from
today's merges. The snapshot is deliberately NOT regenerated (ADR-1192) and no
throughput baseline is recorded, because the run also reproduced two
pre-existing GPU defects:

- vmaf --threads N aborts on every GPU backend (exit 234, "context could not be
  synchronized"); without --threads both CUDA and SYCL score correctly and are
  bit-stable over 10 runs. bench_all.sh hard-codes --threads 1.
- The libvmaf_cuda FFmpeg filter returns a wrong pooled score in 10 of 40 runs
  on master and 8 of 40 on 5a08030 — inside binomial noise of each other.

Harness fixes in the same change:

- bench_all.sh kept its stderr on /dev/null and relabelled every non-zero exit
  as "backend likely unavailable", which is how a hard abort passed for a
  missing device for months. It now captures stderr per row and prints FAIL
  with the exit code and the real last line. Its flag sets also drop
  --no_vulkan, unrecognized since ADR-0726 removed the Vulkan backend.
- benchmark_netflix.py hard-coded /home/kilian/dev/ffmpeg-8/ffmpeg (gone) and
  /dev/dri/renderD130 for the SYCL/QSV import (now the AMD iGPU on the bench
  host, so the SYCL rows failed outright). Both are environment overrides now,
  VMAF_FFMPEG and the new VMAF_SYCL_RENDER_NODE, per the ADR-0792 pattern.

No golden assertions touched; no snapshot regenerated.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(state): drop the duplicate rows a keep-both rebase created

Each dropped row restates one origin/master already carries; master is the
authoritative record. Verified with scripts/ci/check-state-md-rows.sh.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
lusoris pushed a commit that referenced this pull request Sep 6, 2026
The epic #1246 gate table listed G2 and G3 as FAIL against conditions that no
longer hold. G3 in particular blamed "PR #1307 & fix/cambi-cuda-context
unmerged" -- both merged on 2026-09-05/06.

Re-ran each gate rather than reasoning about it:

  G2  PASS  zero failing runs on master HEAD. The release-please failure the
            row was written for is gone; ADR-1171 made the missing release-bot
            credential warn-not-error on push and the workflow reports success.
  G3  PASS  #1307, #1312 and #1324 all on master; container rebuilt from
            cd52f26 and the default model verified on ALL FOUR backends --
            CPU 82.816062, CUDA 82.814062, SYCL 82.814061, HIP 82.816061, each
            exiting 0. The runbook only asked for CUDA; the others were checked
            because a model reaches the GPU twins through the model, not
            through --feature, so a CUDA-only check would not have covered them.
  G1  FAIL  12 epics open, listed by number, with the caveat that the epic
            bodies are snapshots and several of their items have already
            shipped -- the count overstates the work.
  G4  FAIL  still blocked on #1302, and now says why precisely: master's
            extract_k150k_features.py has no --vmaf-model flag (grep returns
            0), which is what §4.2's teacher_model assertion needs. #1302 has
            no failing check -- its only red mark is the aggregator's draft
            guard, and its ADR-0108 validator passes six of six. It needs
            promotion, not repair.

Adds a note that the table is a measurement and each row must be re-run rather
than carried forward, since stale-status drift is what it just corrected.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Sep 6, 2026
…tus (#1350)

* docs(ai): record measured retrain gate status, not authoring-time status

The epic #1246 gate table listed G2 and G3 as FAIL against conditions that no
longer hold. G3 in particular blamed "PR #1307 & fix/cambi-cuda-context
unmerged" -- both merged on 2026-09-05/06.

Re-ran each gate rather than reasoning about it:

  G2  PASS  zero failing runs on master HEAD. The release-please failure the
            row was written for is gone; ADR-1171 made the missing release-bot
            credential warn-not-error on push and the workflow reports success.
  G3  PASS  #1307, #1312 and #1324 all on master; container rebuilt from
            cd52f26 and the default model verified on ALL FOUR backends --
            CPU 82.816062, CUDA 82.814062, SYCL 82.814061, HIP 82.816061, each
            exiting 0. The runbook only asked for CUDA; the others were checked
            because a model reaches the GPU twins through the model, not
            through --feature, so a CUDA-only check would not have covered them.
  G1  FAIL  12 epics open, listed by number, with the caveat that the epic
            bodies are snapshots and several of their items have already
            shipped -- the count overstates the work.
  G4  FAIL  still blocked on #1302, and now says why precisely: master's
            extract_k150k_features.py has no --vmaf-model flag (grep returns
            0), which is what §4.2's teacher_model assertion needs. #1302 has
            no failing check -- its only red mark is the aggregator's draft
            guard, and its ADR-0108 validator passes six of six. It needs
            promotion, not repair.

Adds a note that the table is a measurement and each row must be re-run rather
than carried forward, since stale-status drift is what it just corrected.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(ai): fix the K150K scores path in both the smoke and the production run

The runbook pointed --scores at .corpus/konvid-150k/scores.csv. That file does
not exist. KoNViD-150k splits its scores by part, and the corpus on this
workstation holds k150ka_scores.csv and k150kb_scores.csv (plus the matching
*_votes.csv and a manifest.csv).

The wrong path appeared TWICE: in the section 4 five-clip smoke and in the
section 5.1 multi-day production extraction. extract_k150k_features.py
validates the file and exits with 'error: scores CSV not found', so each would
have aborted on its first line -- the smoke immediately, and the ~105-110 hour
K150K run at its very start.

Corrected to k150ka_scores.csv, which is also the script's own argparse default
and the path in its module docstring example.

--clips-dir is deliberately left as clips/: a real directory of 153,841 files
and a superset of the script's default k150ka_extracted/ (152,265). Lookup is by
video_name, so either resolves. Both paths are now listed in a verify-first
command so an operator checks them before committing to the long run.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(ai): correct four K150K command errors found by running the smoke

Ran the section 4 five-clip smoke end to end in the container. As written it
failed on its first line, then its second, then on every clip. Each fix below
comes from an actual run, not from reading.

1. --scores named scores.csv, which does not exist. KoNViD-150k splits its
   scores by part; the corpus holds k150ka_scores.csv (154,746 rows) and
   k150kb_scores.csv. k150ka_scores.csv is the script's own default.

2. --cpu-vmaf-bin was missing entirely. It is required, and its default
   /build/vmaf/core/build-cpu/tools/vmaf does not exist in the container, so the
   run aborts with 'error: cpu-vmaf-bin not found'. /usr/local/bin/vmaf serves
   both roles.

3. --clips-dir named clips/, which is unusable from inside the container. Its
   153,841 entries are symlinks to HOST absolute paths under
   /home/kilian/dev/vmaf/.workingdir2/konvid-150k/, which do not resolve in the
   container mount. Every clip failed with 'ffprobe ... returned non-zero exit
   status 1' -- which reads like corrupt media and is really a dangling link.
   The real files are in k150ka_extracted/ (152,265 files, ffprobe reports
   960x540), again the script's default.

All three appeared in BOTH the section 4 smoke and the section 5.1 multi-day
production extraction, so the ~105-110 hour K150K run would have aborted at its
very start.

With them fixed the pipeline runs clean: ok=5 fail=0 at 1.26 clip/s,
status complete, schema k150k-feature-extraction-manifest-v1, 5 parquet rows.

Of section 4.2's assertions, schema, status and stats.ok already pass on master.
Three do not, and all three come from #1302: teacher_model in the manifest, the
teacher_model parquet column, and adm3_mean (grep -c adm3 returns 0 on master's
extractor and 3 on #1302's). G4 is blocked on that PR alone -- corpus, binary
and pipeline are all verified working.

no digest needed: trivial

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@lusoris lusoris added the type:bug Something isn't working label Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant