Repository navigation
fix(registry): restore orphaned GPU SpEED registrations + delete dead feature_extractor.c - #1004
Merged
Merged
Conversation
lusoris
marked this pull request as ready for review
June 20, 2026 07:28
…feature_extractor.c PR #875 split feature_extractor.c into the compiled feature_extractor.cpp but left the six speed_{chroma,temporal}_{cuda,sycl,hip} externs + feature_extractor_list[] entries behind in the now-dead .c (meson compiles only the .cpp). The GPU SpEED kernels compiled but vmaf_get_feature_extractor_by_name("speed_chroma_cuda") returned NULL, so SpEED silently fell back to the CPU path — regressing the ADR-0964/0965/0852 GPU SpEED wiring. Port the six externs + array entries into the .cpp's #if HAVE_{CUDA,SYCL,HIP} blocks, delete the dead feature_extractor.c twin (implements the ADR-0545 dead-file policy), and add by-name resolution asserts to test_feature_extractor. Found by the RC independent registry audit (read-only worktree, master 97c147d). CPU Netflix golden gate unaffected (CPU speed_chroma/speed_temporal were always registered). Verified: CPU build + test green; feature_extractor.cpp compiles clean under -Denable_cuda=true. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lusoris
force-pushed
the
fix/speed-gpu-registry
branch
from
June 20, 2026 07:31
a49ab95 to
712eed4
Compare
lusoris
added a commit
that referenced
this pull request
Jun 20, 2026
…kends) + SYCL solve-barrier deadlock
Three real bugs in the SpEED extractors, found via container ASan + GPU-fault
debugging. The eigenvalue-scratch one is retrain-critical (corrupts the heap
during GPU speed extraction on the RTX 4090).
1. CPU speed.c heap-buffer-overflow (this was the CI all-backends SEGV).
speed_init_dimensions() returns -EINVAL for planes too small for the
NUM_SCALES pyramid (e.g. YUV420 chroma below ~160px), but speed_init() and
init_chroma() IGNORED the return -> submatrix_height = truncated_height -
block_size + 1 underflowed the size_t to ~2^64 -> compute_mean() walked off
the frame buffer. The SYCL twin already checked this return; the CPU path
did not. Fix: reorder the truncated==0 guard before the submatrix subtraction
and propagate the return through speed_init() + init_chroma(). ASan-clean.
2. GPU eigenvalue-scratch heap corruption (RETRAIN-CRITICAL; all 6 GPU speed
extractors: cuda/hip/sycl x chroma/temporal). speed_internal_compute_
eigenvalues() lays its scratch out as A[n*n] + d[n] + sd[n] + tmp[2*n] =
n*n + 4*n floats, but every GPU caller allocated n*n + 3*n -> si_tri_multiply
wrote n (=25) floats past the end -> 'free(): invalid next size'. On normal
heaps this is a silent 100-byte overwrite; under MALLOC_PERTURB/ASan it
crashes. The CPU path uses its own correctly-sized eig buffer. Fixed all 6.
3. SYCL solve-kernel divergent-barrier deadlock (DEVICE_LOST on strict-barrier
GPUs, e.g. Intel Arc). launch_solve early-returned idle lanes (lane >=
SP_ELEMENTS) and surplus warps before a work-GROUP group_barrier they were
required to reach -> the group deadlocked. Fix: gate the work with an
flag, keep all work-items in the barrier loop. Applied to
speed_chroma_sycl + speed_temporal_sycl.
Also enlarges test_sycl_speed_chroma_parity's fixture (256x144 -> 320x320): the
old fixture's chroma plane (128x72) is below the SpEED minimum, so the extractor
EINVAL'd and the test never actually ran speed_chroma; it was masked while
speed_chroma_sycl was unregistered (pre-#1004).
Verified in the dev container: ASan-clean on the SYCL CPU-OOB repro and on the
full Arc-GPU speed_chroma pipeline (means/cov/indterm/solve/score all pass,
no heap corruption, no DEVICE_LOST).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lusoris
added a commit
that referenced
this pull request
Jun 20, 2026
#1029) * fix(speed): heap OOB (CPU) + GPU eig-scratch heap corruption (all backends) + SYCL solve-barrier deadlock Three real bugs in the SpEED extractors, found via container ASan + GPU-fault debugging. The eigenvalue-scratch one is retrain-critical (corrupts the heap during GPU speed extraction on the RTX 4090). 1. CPU speed.c heap-buffer-overflow (this was the CI all-backends SEGV). speed_init_dimensions() returns -EINVAL for planes too small for the NUM_SCALES pyramid (e.g. YUV420 chroma below ~160px), but speed_init() and init_chroma() IGNORED the return -> submatrix_height = truncated_height - block_size + 1 underflowed the size_t to ~2^64 -> compute_mean() walked off the frame buffer. The SYCL twin already checked this return; the CPU path did not. Fix: reorder the truncated==0 guard before the submatrix subtraction and propagate the return through speed_init() + init_chroma(). ASan-clean. 2. GPU eigenvalue-scratch heap corruption (RETRAIN-CRITICAL; all 6 GPU speed extractors: cuda/hip/sycl x chroma/temporal). speed_internal_compute_ eigenvalues() lays its scratch out as A[n*n] + d[n] + sd[n] + tmp[2*n] = n*n + 4*n floats, but every GPU caller allocated n*n + 3*n -> si_tri_multiply wrote n (=25) floats past the end -> 'free(): invalid next size'. On normal heaps this is a silent 100-byte overwrite; under MALLOC_PERTURB/ASan it crashes. The CPU path uses its own correctly-sized eig buffer. Fixed all 6. 3. SYCL solve-kernel divergent-barrier deadlock (DEVICE_LOST on strict-barrier GPUs, e.g. Intel Arc). launch_solve early-returned idle lanes (lane >= SP_ELEMENTS) and surplus warps before a work-GROUP group_barrier they were required to reach -> the group deadlocked. Fix: gate the work with an flag, keep all work-items in the barrier loop. Applied to speed_chroma_sycl + speed_temporal_sycl. Also enlarges test_sycl_speed_chroma_parity's fixture (256x144 -> 320x320): the old fixture's chroma plane (128x72) is below the SpEED minimum, so the extractor EINVAL'd and the test never actually ran speed_chroma; it was masked while speed_chroma_sycl was unregistered (pre-#1004). Verified in the dev container: ASan-clean on the SYCL CPU-OOB repro and on the full Arc-GPU speed_chroma pipeline (means/cov/indterm/solve/score all pass, no heap corruption, no DEVICE_LOST). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed,cuda): download device chroma plane to host before host-side picture_copy speed_chroma_cuda extract_channel ran host-side picture_copy() on ref_pic->data[channel], which is a DEVICE pointer in the CUDA pipeline (like adm/vif_cuda) -> reading GPU memory on the host SEGVs every frame. Never caught (CUDA tests need a GPU; CI has none). Download via cuMemcpyDtoH into host staging, alias into a temp VmafPicture, then picture_copy. Verified on RTX 4090: SEGV gone, parity runs. A separate GPU-algorithm score discrepancy (~7x low vs CPU) remains, tracked separately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed,cuda): download device luma plane to host in speed_temporal_cuda Same device-pointer SEGV as speed_chroma_cuda: extract_fex_st ran host-side picture_copy() on ref_pic/dist_pic->data[0], which is a CUdeviceptr in the CUDA pipeline -> host read of GPU memory SEGVs every frame. Download via cuMemcpyDtoH (with a local cuCtxPushCurrent, since the GPU pipeline pushes the context later) before the host picture_copy. Verified on RTX 4090: SEGV gone, parity runs (a shared GPU-algorithm score discrepancy remains, tracked separately). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed,cuda): GPU SpEED means+cov kernels — global window, not per-tile speed_means_kernel + speed_cov_kernel computed PER-TILE block-local statistics (means[elem*num_blocks+tile], data sampled at tile_y*5+er) and summed num_blocks displaced block-local covariances, while the CPU reference computes ONE GLOBAL covariance per (x,y) element pair over a 5x5-phase-shifted full-plane submatrix (means[elem], data at er). The GPU port used the wrong sampling decomposition and was never validated (CUDA tests need a GPU; CI has none), giving ~7x-wrong SpEED scores on every GPU run. Rewrite both kernels to the CPU's global formulation: means kernel = 25 threads, each the global mean at (er,ec); cov kernel = global submatrix sweep with scalar global means, divide by N once, no tile loop. means[] stays over-allocated (25*num_blocks); only [0,25) used. Launch geometry unchanged. Verified on RTX 4090: test_cuda_speed_temporal_parity now PASSES bit-parity vs CPU (was wrong); test_cuda_speed_chroma_parity improved from cpu=19.84/cuda=2.89 (7x low) to within 2x — the residual is a SEPARATE chroma bug (the CUDA score kernel shares one eigenvalue/cov basis for ref+dis via save/restore, but the CPU uses separate ref/dis bases; matters only when ref!=dis, i.e. chroma not temporal). Tracked as a follow-up. SYCL/HIP twins carry the same kernel-geometry bug — to be ported next. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed): check temporal speed_init return + free CUDA buffers on init fail_pop - speed.c temporal init ignored speed_init()'s return; on -EINVAL (in-range-but- invalid speed_kernelscale / bad prescale string) float_stride stays 0 -> zero- size mallocs -> OOB writes in picture_copy. Capture + check the return. - speed_chroma_cuda.c / speed_temporal_cuda.c: the init fail_pop label popped the CUDA context and returned -EIO WITHOUT free_cuda_buffers(_st) -> device + pinned-host buffers leak on partial cuMemAlloc/cuMemHostAlloc failure. Free before returning (mirrors fail_after_pop / free_all). Found by the full-fork correctness audit (PR4 + PR5). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed,hip): free CPU scratch on init OOM (goto free_cpu) in hip speed extractors speed_chroma_hip.c / speed_temporal_hip.c init: a post-alloc NULL check did a bare 'return -ENOMEM' that bypassed the free_cpu cleanup label, leaking up to 10/12 CPU scratch buffers. Route to free_cpu; move the label below #endif HAVE_HIPCC so the goto compiles in non-HIPCC builds. (eig-scratch +4 fix preserved.) Audit PR-4 (HIP twin). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed,cuda): separate ref/dis covariance basis in GPU SpEED score The CUDA SpEED chroma/temporal extractors computed the reference path correctly but the DISTORTED path wrong (~2x-high chroma scores). extract_channel / extract_fex_st saved the reference covariance and restored it before the distorted CPU-linalg, so the distorted linear-system solution AND eigenvalues used the REFERENCE covariance; the shared score kernel then used one eigenvalue array for both ref and dis entropy. The CPU reference (est_params) computes SEPARATE ref and dis covariance + eigenvalues. Confirmed by RTX-4090 instrumentation: ref var/entropy matched bit-exactly, dis var/entropy diverged (5.11 vs 3.54 / 211 vs 190). Temporal masked it (consecutive frames -> ref≈dis). Fix: drop the cov save/restore (distorted path keeps the distorted covariance); add a d_eigenvalues_ref device buffer + cuMemcpyDtoD the ref eigenvalues aside after the reference linalg; speed_score_kernel now takes ref_eigenvalues AND dis_eigenvalues (ref entropy uses ref, dis entropy uses dis). Applied to speed_chroma_cuda.c, speed_temporal_cuda.c, speed_score.cu. Verified on RTX 4090: test_cuda_speed_chroma_parity AND test_cuda_speed_temporal_parity now PASS bit-parity (<1e-4) vs CPU. Completes the GPU SpEED correctness fix (kernel-geometry + basis). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed,hip): port verified GPU SpEED correctness fix (geometry + basis) to HIP Ports the RTX-4090-verified CUDA fix to the HIP twins (speed_score.hip kernels + speed_chroma_hip.c + speed_temporal_hip.c): means kernel = global window per element (not per-tile); cov kernel = global submatrix sweep, scalar global means, divide by N once (not per-tile block-local sum); separate ref/dis eigenvalue basis (d_eigenvalues_ref + hipMemcpyDtoD stash; score kernel takes ref+dis eigenvalue arrays; drop the cov save/restore so dis keeps dis covariance). Same algorithm as the CUDA fix that passes bit-parity on the RTX 4090. Not runnable on this host (no AMD GPU) — CI / ROCm hardware compile-verifies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(speed,sycl): port verified GPU SpEED correctness fix (geometry + basis) to SYCL Ports the RTX-4090-verified CUDA fix to the SYCL twins (speed_chroma_sycl.cpp + speed_temporal_sycl.cpp): means kernel = one work-item per element, global window (not per-tile); cov kernel = global submatrix sweep with scalar global means, divide by N once (not per-tile block-local sum); separate ref/dis eigenvalue basis (d_eigenvalues_ref USM buffer + q.memcpy DtoD stash; score kernel takes ref+dis eigenvalue arrays; drop the cov save/restore). Preserves the existing solve-barrier 'active'-flag deadlock fix and the eig-scratch +4 sizing. Same algorithm as the CUDA fix that passes bit-parity on the RTX 4090. Not runnable on the fp64-less Arc here — CI / fp64 hardware compile+run-verifies. Completes the GPU SpEED correctness fix across all three backends. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(speed): ADR-0108 deliverables for GPU SpEED correctness fix (#1029) Ship the deep-dive deliverables for the GPU SpEED correctness + safety PR: - changelog.d/fixed/gpu-speed-covariance-eigenbasis-correctness.md - docs/research/research-1120-gpu-speed-covariance-eigenbasis-correctness-2026-06-20.md (per-tile-vs-global covariance bug, shared-vs-separate ref/dis eigenbasis bug, co-located safety bugs, and the RTX-4090 bit-parity method) - docs/state.md: T-GPU-SPEED-COVARIANCE-EIGENBASIS-CORRECTNESS-2026-06-20 row under Recently closed - docs/rebase-notes.md: entry for fix/speed-extractor-oob-deadlock-heap-corruption - core/src/feature/cuda/AGENTS.md: rebase-sensitive invariant (global covariance + separate ref/dis eigenvalue bases) - docs/backends/cuda/overview.md: user-visible note that GPU SpEED scores now match CPU (numeric correction) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(speed): document GPU SpEED backend parity (CUDA/HIP/SYCL ↔ CPU) The doc-substance gate (ADR-0167) requires a docs/metrics or docs/backends/sycl edit when core/src/feature/sycl/*.cpp changes. Add a "GPU backend parity" section to the speed_qa metric doc covering the speed_chroma / speed_temporal CUDA/HIP/SYCL correctness fix: global covariance window and separate ref/dis eigenvalue bases, with the RTX-4090 bit-parity verification pointer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
3 of 7 tasks
lusoris
added a commit
that referenced
this pull request
Aug 30, 2026
…gistry parity) PR #875 split core/src/feature/feature_extractor.c into .cpp but carried over only 8 of the 17 *_metal externs; PR #1004 then deleted the dead .c twin where the remaining 9 lived. The .mm kernel TUs, the g_metal_features[] dispatch rows and the parity tests all stayed in place — only the registry entries were lost, so vmaf_get_feature_extractor_by_name() returned NULL and --feature <name> could not select them on macOS Metal builds. Restores the externs and feature_extractor_list[] entries for integer_ssim_metal, float_vif_metal, float_adm_metal, integer_vif_metal, integer_adm_metal, integer_ciede_metal, integer_psnr_hvs_metal, integer_cambi_metal and ssimulacra2_metal, bringing the registry back to the 17/17 contract asserted by test_metal_kernel_coverage_audit (ADR-0959). This was the single failing test on both macOS CI build legs (131 OK / 1 FAIL). All additions sit inside #if HAVE_METAL, so non-Metal builds are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lusoris
added a commit
that referenced
this pull request
Aug 30, 2026
…gistry parity) PR #875 split core/src/feature/feature_extractor.c into .cpp but carried over only 8 of the 17 *_metal externs; PR #1004 then deleted the dead .c twin where the remaining 9 lived. The .mm kernel TUs, the g_metal_features[] dispatch rows and the parity tests all stayed in place — only the registry entries were lost, so vmaf_get_feature_extractor_by_name() returned NULL and --feature <name> could not select them on macOS Metal builds. Restores the externs and feature_extractor_list[] entries for integer_ssim_metal, float_vif_metal, float_adm_metal, integer_vif_metal, integer_adm_metal, integer_ciede_metal, integer_psnr_hvs_metal, integer_cambi_metal and ssimulacra2_metal, bringing the registry back to the 17/17 contract asserted by test_metal_kernel_coverage_audit (ADR-0959). This was the single failing test on both macOS CI build legs (131 OK / 1 FAIL). All additions sit inside #if HAVE_METAL, so non-Metal builds are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 of 7 tasks
lusoris
added a commit
that referenced
this pull request
Aug 30, 2026
…+ MCP smoke path allowlist (#1136) Rebuilt on current master; the original branch (fix/master-ci-repair-aug) had an orphan-root history that made rebase report both-added conflicts on files it never touched. 1. Metal registry orphan. PR #875 split core/src/feature/feature_extractor.c into .cpp carrying only 8 of the 17 *_metal externs; PR #1004 then deleted the dead .c twin where the other 9 lived. The .mm kernel TUs, the g_metal_features[] dispatch rows and the parity tests all stayed in place, so nothing failed to build - only the registry entries vanished, and vmaf_get_feature_extractor_by_name() returned NULL for nine kernels that were compiled and dispatch-wired. Restores the 17/17 contract asserted by test_metal_kernel_coverage_audit (ADR-0959). All additions sit inside #if HAVE_METAL, so non-Metal builds are byte-unaffected. 2. MCP smoke 10-bit allowlist. The compute_vmaf 10-bit case writes its yuv420p10le fixtures to /tmp, but PR #1054's validate_path() admits only <repo>/testdata, <repo>/model, <repo>/python/test/resource, /workspace/python/test/resource and . score_yuv_pair() returned -EACCES and the assertion tripped. Fixed in the test rather than the allowlist: the case extends the allow-set through the documented VMAF_MCP_ALLOW escape hatch for its own duration and unsetenv()s it after. Allowlist enforcement is unchanged and stays covered by core/test/test_mcp_compute_vmaf_allowlist.c. Verified locally: 17 metal registry entries (was 8); meson test test_mcp_smoke 18/18 passing (was 17 run / 1 failed). Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
14 of 17 tasks
This was referenced Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RC independent-audit fix (HIGH — silent GPU regression). The SpEED GPU twins
(
speed_{chroma,temporal}_{cuda,sycl,hip}) were unreachable by name on theshipping build. PR #875 split
feature_extractor.cinto the compiledfeature_extractor.cpp(meson compiles only the.cpp) but left the six GPUSpEED
externs +feature_extractor_list[]entries behind in the now-dead.c.The kernels compiled, but
vmaf_get_feature_extractor_by_name("speed_chroma_cuda")(and SYCL/HIP variants) returned NULL — so SpEED silently fell back to the CPU
path, regressing the ADR-0964/0965/0852 GPU SpEED wiring.
core/src/feature/feature_extractor.cpp: port the 6externs + array entriesinto the existing
#if HAVE_{CUDA,SYCL,HIP}blocks; fix a pre-existing-Wunused-parameterin the same file ((void)fex;, thread-sensitive pool code).core/src/feature/feature_extractor.c— the dead split-brain twin(implements the ADR-0545 wire-or-delete policy).
core/test/test_feature_extractor.c: assert every GPU SpEED twin resolves byname under its backend (regression guard).
The other GPU twins (
adm/vif/cambi/…_cuda/_sycl/_hip) were already inthe
.cppregistry and are unaffected. The 9 Metal registrations that alsolive only in the old
.care intentionally NOT ported here — their.mmTUs arenot yet wired into meson (only
metal/stubs.cis), which is in-flight PR #986'slane; registering them now would be a link error. Tracked as a fast-follow on #986.
Reproducer / smoke test
Verified locally: CPU full build +
test_feature_extractorgreen;feature_extractor.cppcompiles clean under-Denable_cuda=true(host CUDA linkblocked by a pre-existing
cuda_helper.cuhinclude-path issue, §15 host-debt —CI GPU matrix verifies the link).
Deep-dive deliverables (ADR-0108)
.cis deleted per ADR-0545).core/src/feature/AGENTS.md— added the "GPU/Metal twins live in.cpp, registered-but-unresolvable is a silent bug" invariant + fixed the stale.cpath.changelog.d/fixed/speed-gpu-registry-orphan.md.docs/rebase-notes.md—fix/speed-gpu-registryentry.Other rules
T-SPEED-GPU-REGISTRY-ORPHAN-2026-06-19row (closed-on-merge).docs/page needed.🤖 Generated with Claude Code