Repository navigation
Conversation
T-UPSTREAM-1305 (Netflix/vmaf#1305). The T-GPU-OPT-1 fence batch lived in a `static _Thread_local DrainBatchTls g_drain_batch`, but every entry in it is a `CUevent` and a `bool *` owned by a feature extractor bound to a particular `VmafCudaState` — thread scope is strictly wider than the handles the batch stores. `read_pictures_extractor_loop_cuda` deliberately returns with the batch open and n > 0 so the next frame's Phase-1 flush can wait on it, and `vmaf_close()` destroyed the extractor vector (freeing those events and flags) while `vmaf_cuda_drain_batch_thread_destroy` cleared only the drain stream. Any run that abandoned or errored out of a CUDA context before the terminal `vmaf_read_pictures(NULL, NULL)` therefore left destroyed CUevents and dangling `bool *`s in a batch the next VmafContext on that thread flushed. - `VmafCudaDrainBatch` moves into `VmafCudaState::drain_batch` (the public type is opaque, so no ABI break); every drain entry point takes the owning state; `vmaf_cuda_drain_batch_thread_destroy` becomes `..._destroy` and empties the batch instead of only dropping the stream (ADR-1187). - `vmaf_close()` runs a best-effort drain flush + close BEFORE `feature_extractor_vector_destroy()`, so in-flight GPU work has landed before the buffers it reads are freed. - `vmaf_score_at_index`, `vmaf_score_at_index_model_collection` and `vmaf_feature_score_at_index` return -EAGAIN for an index whose GPU work is still pending instead of predicting over a half-written feature row (ADR-1189). - `core/src/cuda/common.c`: the compute stream's priority clamp was `MAX(low, MIN(high, prio))`; CUDA's scale is inverted, so it collapsed to the LOWEST priority. Now `MIN(low, MAX(high, prio))`. Scheduling-only: the same events are waited on in the same order. The three pre-existing CUDA parity-test deltas are byte-identical before and after. New GPU-gated regression test `core/test/test_cuda_drain_batch_state_scope.c`; `core/src/cuda/drain_batch.c` tightens the cuda ratchet baseline 11 -> 9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
) ADR-0165: the row moves from '## Open bugs' to '## Recently closed' in the same PR that fixes it, citing PR #1341, ADR-1187 and ADR-1189. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Superseded by #1321, which merged first and makes the same fix.
void vmaf_cuda_drain_batch_open(const VmafCudaState *cu_state);
int vmaf_cuda_drain_batch_flush(VmafCudaState *cu_state);
void vmaf_cuda_drain_batch_thread_destroy(VmafCudaState *cu_state);plus the introspection helper backing the unit test that pins the rule a batch owned by another Worth noting for whoever picks up |
Summary
Fixes
T-UPSTREAM-1305-CUDA-DRAIN-BATCH-THREAD-GLOBAL-2026-09-03(Netflix/vmaf#1305), both halves plus the folded-in nit. The T-GPU-OPT-1 CUDA fence batch lived in astatic _Thread_local DrainBatchTls g_drain_batch, but every entry in it is aCUeventand abool *owned by a feature extractor bound to a particularVmafCudaState— thread scope is strictly wider than the handles it stores, and the frame loop deliberately returns with the batch open, so an abandoned or erroredVmafContexthanded destroyed events and freed flag pointers to the nextVmafContexton that thread. The batch now lives inVmafCudaState::drain_batch, teardown empties it, andvmaf_close()drains before destroying the extractors. Separately, the three public*_score_at_indexreaders now report an in-flight frame as-EAGAINinstead of predicting over a half-written feature row.Type
fix— bug fixcuda— backend-specificChecklist
make format && make lintis green locally —clang-formatclean;pre-commit run --files <every touched file>green (26 hooks, 0 failures);scripts/ci/assertion-density.shPASS;scripts/ci/check-copyright.sh,check-adr-numbering.sh,check-conflict-markers.shclean.meson test -C build-cuda --suite=fast→ 141 ok / 3 fail, and all 3 failures are pre-existing CPU-vs-CUDA parity deltas that reproduce byte-identically onorigin/mastersources (see "Cross-backend numerical results")./cross-backend-diffand the worst ULP is ≤ 2 — see below; the change is scheduling/ownership only and every measured score is unchanged.integer_adm_cuda.c,integer_vif_cuda.c,integer_ms_ssim_cuda.c) pluskernel_template.hwere all updated; HIP/SYCL/Metal have no drain batch (grep -rn drain_batch core/src/{hip,sycl,metal}is empty)..c/.cpp/.cu/.h/.hpp, it has the appropriate license header (seeCONTRIBUTING.md).!orBREAKING CHANGE:and the migration path is documented below — not breaking:VmafCudaStateis opaque in the public header, and the-EAGAINcontract only affects mid-stream reads of an index whose GPU work has not been collected (documented indocs/api/index.md+ the header).docs/adr/_index_fragments/<NNNN-slug>.mdand the slug is appended todocs/adr/_index_fragments/_order.txt.Bug-status hygiene (ADR-0165)
docs/state.mdupdated in this PR with a row in the appropriate section —T-UPSTREAM-1305-CUDA-DRAIN-BATCH-THREAD-GLOBAL-2026-09-03moves from "Open bugs" to "Recently closed".Netflix golden-data gate (ADR-0024)
assertAlmostEqual(...)score in the Netflix golden Python tests.-EAGAINguard is reachable from it.Cross-backend numerical results
The change is scheduling/ownership only — the same
finishedevents are waited on in the same order — so every score is expected to be unchanged. Verified by running the three CUDA parity tests that already fail on master, first with this branch's sources and then withgit show origin/master:<file>restored for all nine touched C/H files, in the same build dir:Byte-identical in both directions — these three are pre-existing CUDA parity debt, not introduced here, and this PR does not move them.
End-to-end on the Netflix 576x324 pair with
vmaf_v0.6.1(RTX 4090):(GPU is not bit-identical to CPU by design; the CPU number is the reference.)
Deep-dive deliverables (ADR-0108)
docs/research/2030-cuda-drain-batch-ownership-2026-09-06.md(scope candidates table, the by-valuevmaf_cuda_import_statecaveat, why a real fence in the getter was rejected).AGENTS.mdinvariant note — two entries added tocore/src/cuda/AGENTS.md: drain-batch storage scope, and the inverted CUDA stream-priority clamp.changelog.d/fixed/cuda-drain-batch-state-owned.md(CHANGELOG.mdre-rendered withbash scripts/release/concat-changelog-fragments.sh --write).docs/rebase-notes.mdunderfix/t-upstream-1305-cuda-drain-batch-thread-.Reproducer
# Build with CUDA and run the new GPU-gated regression test. meson setup build-cuda core -Denable_cuda=true -Denable_sycl=false -Db_lto=false ninja -C build-cuda test/test_cuda_drain_batch_state_scope ./build-cuda/test/test_cuda_drain_batch_state_scopePost-fix (RTX 4090):
Pre-fix — the same three cases, API-adapted to the thread-local signatures and run against
git show origin/master:core/src/cuda/drain_batch.{c,h}+common.{c,h}+kernel_template.h+libvmaf.c+ the three extractors, one case at a time because the harness stops at the first failure:Known follow-ups
ADR-0242citations scattered through the CUDA comment blocks are a stale mis-reference — ADR-0242 is the tiny-AI training corpus; the fence batch shipped in PR chore(changelog.d): prune 7 stale fragments + rewrite 2 misleading ones #312 with no ADR of its own. New/edited comments in this PR sayT-GPU-OPT-1, PR #312instead; the ~25 untouched sites tree-wide are left for a dedicated comment-citation sweep rather than churned here.scripts/ci/tidy-baseline-cuda.json: only thecore/src/cuda/drain_batch.ckey is tightened (11 → 9, measured withpython3 scripts/ci/tidy-ratchet.py --lane cuda --build-dir build-cuda --only core/src/cuda/drain_batch.c). The rest of that lane's baseline is already stale against the current tree — e.g.integer_ms_ssim_cuda.cmeasures 44 against a baseline of 43 on unmodifiedorigin/mastersources — and no workflow runs--lane cudatoday (lint-and-format.ymlandnightly.ymlboth run--lane cpu), so a wholesale regeneration is left to whoever enables that lane. The required cpu lane is unaffected:core/src/libvmaf.cmeasures 0 warnings / 1 uncited NOLINT, exactly its committed baseline, and the new test is CUDA-gated so it never enters the CPU build.