Skip to content

fix(cuda): own the drain batch in VmafCudaState, not the OS thread - #1341

Closed
lusoris wants to merge 2 commits into
masterfrom
fix/t-upstream-1305-cuda-drain-batch-thread-
Closed

lusoris wants to merge 2 commits into
masterfrom
fix/t-upstream-1305-cuda-drain-batch-thread-

Conversation

@lusoris

@lusoris lusoris commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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 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 it stores, and the frame loop deliberately returns with the batch open, so an abandoned or errored VmafContext handed destroyed events and freed flag pointers to the next VmafContext on that thread. The batch now lives in VmafCudaState::drain_batch, teardown empties it, and vmaf_close() drains before destroying the extractors. Separately, the three public *_score_at_index readers now report an in-flight frame as -EAGAIN instead of predicting over a half-written feature row.

Type

  • fix — bug fix
  • cuda — backend-specific

Checklist

  • Commits follow Conventional Commits (the commit-msg hook enforces this).
  • make format && make lint is green locally — clang-format clean; pre-commit run --files <every touched file> green (26 hooks, 0 failures); scripts/ci/assertion-density.sh PASS; scripts/ci/check-copyright.sh, check-adr-numbering.sh, check-conflict-markers.sh clean.
  • Unit tests pass: 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 on origin/master sources (see "Cross-backend numerical results").
  • If I touched any SIMD/GPU code path, I ran /cross-backend-diff and the worst ULP is ≤ 2 — see below; the change is scheduling/ownership only and every measured score is unchanged.
  • If I touched a feature extractor with SIMD/GPU twins, I either updated every twin or listed the gap under "Known follow-ups" below — the three CUDA registration sites (integer_adm_cuda.c, integer_vif_cuda.c, integer_ms_ssim_cuda.c) plus kernel_template.h were all updated; HIP/SYCL/Metal have no drain batch (grep -rn drain_batch core/src/{hip,sycl,metal} is empty).
  • 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 — not breaking: VmafCudaState is opaque in the public header, and the -EAGAIN contract only affects mid-stream reads of an index whose GPU work has not been collected (documented in docs/api/index.md + the 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.txt.

Bug-status hygiene (ADR-0165)

  • docs/state.md updated in this PR with a row in the appropriate section — T-UPSTREAM-1305-CUDA-DRAIN-BATCH-THREAD-GLOBAL-2026-09-03 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.
  • If I believe a golden value must change, I have explained why below AND pinged @lusoris for a CODEOWNERS exception — no golden value changes. The golden gate is CPU-only and reads after the terminal flush, so neither the drain-batch move nor the -EAGAIN guard is reachable from it.

Cross-backend numerical results

The change is scheduling/ownership only — the same finished events 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 with git show origin/master:<file> restored for all nine touched C/H files, in the same build dir:

                       this branch                                  origin/master sources
float_adm     cpu=0.45416954  cuda=0.45369203  d=4.78e-04   cpu=0.45416954  cuda=0.45369203  d=4.78e-04
ssimulacra2   cpu=-39.67294487 cuda=-39.67032687 d=2.62e-03  cpu=-39.67294487 cuda=-39.67032687 d=2.62e-03
psnr_hvs      cpu=9.99749385  cuda=9.60927651  d=3.88e-01   cpu=9.99749385  cuda=9.60927651  d=3.88e-01

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):

cuda pooled vmaf = 76.6678302560172
cpu  pooled vmaf = 76.66783086300072

(GPU is not bit-identical to CPU by design; the CPU number is the reference.)

Deep-dive deliverables (ADR-0108)

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_scope

Post-fix (RTX 4090):

test_drain_batch_is_state_scoped: pass
test_drain_batch_destroy_empties_open_batch: pass
test_cuda_stream_priority_is_greatest: pass
3 tests run, 3 passed

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:

test_drain_batch_is_state_scoped: fail
flush through state B must not drain state A's registration
1 tests run, 1 failed

test_drain_batch_destroy_empties_open_batch: fail
destroy must empty the batch, not just drop its stream
1 tests run, 1 failed

test_cuda_stream_priority_is_greatest: fail
compute stream must run at CUDA's greatest priority
1 tests run, 1 failed

Known follow-ups

  • The ADR-0242 citations 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 say T-GPU-OPT-1, PR #312 instead; 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 the core/src/cuda/drain_batch.c key is tightened (11 → 9, measured with python3 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.c measures 44 against a baseline of 43 on unmodified origin/master sources — and no workflow runs --lane cuda today (lint-and-format.yml and nightly.yml both run --lane cpu), so a wholesale regeneration is left to whoever enables that lane. The required cpu lane is unaffected: core/src/libvmaf.c measures 0 warnings / 1 uncited NOLINT, exactly its committed baseline, and the new test is CUDA-gated so it never enters the CPU build.

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>
@lusoris lusoris added this to the 1.0.0 — First release milestone Sep 6, 2026
)

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>
@lusoris

lusoris commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #1321, which merged first and makes the same fix.

git show origin/master:core/src/cuda/drain_batch.h already carries the owner-scoped API this PR proposes:

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 VmafCudaState is never waited on. Rebasing this would conflict in drain_batch.c, drain_batch.h and libvmaf.c against that merged work, with nothing left to add.

Worth noting for whoever picks up T-UPSTREAM-1305: I verified separately that this fix — in either form — does not resolve the libvmaf_cuda FFmpeg nondeterminism (still 2/80 wrong on current master), and does not resolve the --threads abort either, which turned out to be a different defect fixed in #1343 (ADR-1197). Closing as duplicate.

@lusoris lusoris closed this Sep 6, 2026
@lusoris
lusoris deleted the fix/t-upstream-1305-cuda-drain-batch-thread- branch September 18, 2026 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant