Repository navigation
Conversation
…n backend OFF
Both `core/include/libvmaf/libvmaf_hip.h` and
`core/include/libvmaf/libvmaf_metal.h` document the contract that every
public entry point returns `-ENOSYS` when libvmaf is built without the
relevant backend. The real bodies in `core/src/hip/common.c`,
`core/src/metal/common.mm`, `core/src/metal/picture_import.mm`, and the
`vmaf_hip_import_state` / `vmaf_metal_import_state` /
`vmaf_metal_read_imported_pictures` definitions inside `core/src/libvmaf.c`
all sit behind `#ifdef HAVE_HIP` / `#ifdef HAVE_METAL`, so a default
`-Denable_hip=false -Denable_metal=disabled` build emitted none of those
symbols into `libvmaf.so` and any downstream link that referenced them
failed.
Fix: add `core/src/hip/stubs.c` + `core/src/metal/stubs.c` that mirror the
canonical `core/src/dnn/dnn_api.c` `VMAF_HAVE_DNN` stub pattern and wire
each TU into `libvmaf_feature_static_lib` via `hip_sources` /
`metal_sources` only when the backend is disabled. The stubs return
`-ENOSYS`, set out-params to NULL on the pointer-returning entry points,
and `vmaf_hip_available()` / `vmaf_metal_available()` correctly return 0.
Verified locally with:
meson setup build-cpu core -Denable_hip=false -Denable_metal=disabled \\
-Denable_cuda=false -Denable_sycl=false \\
-Denable_dnn=disabled
ninja -C build-cpu
nm -D build-cpu/src/libvmaf.so | grep -E "vmaf_(hip|metal)_"
all 14 documented public symbols are present (T-type) and a smoke main
linking against libvmaf.so observes -ENOSYS / NULL out-params on every
entry point and 0 from the availability probes.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lusoris
marked this pull request as draft
May 30, 2026 14:42
This was referenced May 30, 2026
lusoris
added a commit
that referenced
this pull request
May 31, 2026
) Round 3 of Metal kernel test coverage. PR #351 closed the registration audit (all 8 extractors discoverable) and PR #379 added parity tests for the 4 highest-priority kernels (motion_v2, integer_psnr, float_psnr, float_ssim). This commit fills the remaining 4 gaps so every registered Metal extractor now has a real per-kernel CPU-vs-Metal score gate. New tests (all in core/test/, all skip on -ENODEV): - test_metal_integer_motion_parity.c -> motion2_score (1e-4, ADR-0214) - test_metal_float_motion_parity.c -> motion2_score (1e-4, ADR-0214) - test_metal_float_moment_parity.c -> 4 moment keys (1e-4, ADR-0214) - test_metal_float_ms_ssim_parity.c -> ms_ssim (1e-3, ADR-0589) Pattern mirrors PR #379 / test_sycl_motion3_parity.c: synthetic 256x144 YUV420P fixture fed through both the CPU twin and the Metal extractor; assert places=4 (1e-4) parity per ADR-0214, except float_ms_ssim which inherits the 1e-3 SSIM-family bound from ADR-0589. Skip path: when vmaf_metal_state_init returns -ENODEV (Linux, Windows, Intel Mac), each test emits "[skip: no Metal device]" and passes cleanly. Runs the live kernel only on Apple-Family-7+ macOS CI lanes. Wiring: 4 new executable() + test() entries appended inside the existing enable_metal guard in core/test/meson.build (after the round-2 block, or after test_metal_install_header pre-merge), suite ['fast', 'gpu'] for consistency with the existing Metal smoke / round-2 parity tests. ADR-0108 deliverables: - Research digest: no digest needed: trivial test-only addition - Decision matrix: no alternatives: only-one-way fix (synthetic-fixture + skip-on-ENODEV is the established test_sycl_motion3_parity.c + PR #379 precedent) - AGENTS.md invariant note: no rebase-sensitive invariants in this PR; PR #379 already updates core/test/AGENTS.md for the Metal -ENODEV skip rule - Reproducer: see PR description - CHANGELOG fragment: changelog.d/added/metal-kernel-coverage-round3.md - Rebase note: docs/rebase-notes.md "Metal kernel parity tests round 3 (2026-05-31)" State.md updated with T-METAL-KERNEL-PARITY-ROUND3-2026-05-31 row under Recently closed (CLAUDE r13). Refs: PR #294, PR #308, PR #351 (registration audit), PR #379 (round 2) Cross-refs: ADR-0214, ADR-0361, ADR-0421, ADR-0589 Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
lusoris
marked this pull request as ready for review
May 31, 2026 13:24
lusoris
marked this pull request as draft
May 31, 2026 13:54
lusoris
marked this pull request as ready for review
May 31, 2026 14:00
Contributor
Author
|
Closing as part of marathon cleanup 2026-05-31 (150 PRs merged today). Content likely superseded by sibling merges. Reopen if specific finding still needs work; bigger PRs preferred going forward per session feedback. |
Contributor
Author
|
Superseded by #525 — bundled per 2026-06-01 triage. |
lusoris
added a commit
that referenced
this pull request
Jun 2, 2026
lusoris
added a commit
that referenced
this pull request
Jun 2, 2026
#308 ENOSYS stubs + #316 dispatch token-boundary) (#525) * fix(core): propagate correct errno codes — PR #125 defects Five actionable errno defects from the PR #125 code review: 1. vmaf_write_output_with_format: capture errno immediately after open(2) / fdopen(3) failure; return -errno instead of hardcoded -EINVAL so callers receive the OS-precise error code. 2. vmaf_cuda_state_init: map driver-library-missing → -ENOSYS and cuInit(0) failure → -ENODEV, mirroring the SYCL/HIP pattern already established in sycl/common.cpp and cuda/cuda_helper.cuh. 3. vmaf_close: propagate return values from vmaf_thread_pool_wait() and vmaf_framesync_destroy() (CERT ERR33-C / Power-of-10 #7). NULL-pool case (n_threads == 0) correctly returns 0. 4. vmaf_cuda_preallocate_pictures: return -EBUSY on double-call to prevent silent ring-buffer leak and in-flight picture corruption. 5. vmaf_init: return the actual sub-init error code rather than a hardcoded -ENOMEM for every error path. Fast test suite: 49/49. Pre-commit: all checks pass. ADR-0214 parity gate: unaffected (errno-only changes, no algorithm delta). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(tools): check fseek / vmaf_picture_alloc / vmaf_read_pictures returns in vmaf_bench Discharge three S9 (JPL Power-of-10 r7) unchecked-return findings in `core/tools/vmaf_bench.c` surfaced by the 2026-05-30 audit. Independent of PR #304 (the vmaf.c/vmaf_bench.c CUDA/SYCL state-leak fix); no overlapping hunks. * `yuv_pair_read_frame` — the two `fseek` calls were `(void)`-cast, silencing the lint but hiding the semantic bug: a failed `fseek` leaves the FILE position undefined, after which `fread` silently feeds the wrong bytes into the benchmark. Now checks each `fseek` and returns `-EIO` with a `perror` diagnostic on failure. * `run_sycl_gpu_profile` per-frame loop — `vmaf_picture_alloc` returns were discarded; on allocation failure the subsequent `yuv_pair_read_frame -> ref->data[0]` dereference would crash on a sentinel-zero `VmafPicture`. Now captures the return code, logs, unrefs any already-allocated sibling, and breaks the loop. * `run_sycl_gpu_profile` end-of-stream block — the final `vmaf_read_pictures(vmaf, NULL, NULL, 0)` flush surfaces pooling / aggregation errors via its int return that the previous code discarded; now captured and propagated. `printf` / `vmaf_close` returns explicitly `(void)`-cast to match the surrounding file convention. Adds `#include <errno.h>` for `EIO`. No behavior change on success paths. Builds clean (`meson setup build-cpu core -Denable_cuda=false -Denable_sycl=false && ninja -C build-cpu`); clang-tidy on the touched file produces no new warnings vs master (line numbers shift only). * fix(core): emit -ENOSYS stubs for libvmaf_hip.h / libvmaf_metal.h when backend OFF Both `core/include/libvmaf/libvmaf_hip.h` and `core/include/libvmaf/libvmaf_metal.h` document the contract that every public entry point returns `-ENOSYS` when libvmaf is built without the relevant backend. The real bodies in `core/src/hip/common.c`, `core/src/metal/common.mm`, `core/src/metal/picture_import.mm`, and the `vmaf_hip_import_state` / `vmaf_metal_import_state` / `vmaf_metal_read_imported_pictures` definitions inside `core/src/libvmaf.c` all sit behind `#ifdef HAVE_HIP` / `#ifdef HAVE_METAL`, so a default `-Denable_hip=false -Denable_metal=disabled` build emitted none of those symbols into `libvmaf.so` and any downstream link that referenced them failed. Fix: add `core/src/hip/stubs.c` + `core/src/metal/stubs.c` that mirror the canonical `core/src/dnn/dnn_api.c` `VMAF_HAVE_DNN` stub pattern and wire each TU into `libvmaf_feature_static_lib` via `hip_sources` / `metal_sources` only when the backend is disabled. The stubs return `-ENOSYS`, set out-params to NULL on the pointer-returning entry points, and `vmaf_hip_available()` / `vmaf_metal_available()` correctly return 0. Verified locally with: meson setup build-cpu core -Denable_hip=false -Denable_metal=disabled \\ -Denable_cuda=false -Denable_sycl=false \\ -Denable_dnn=disabled ninja -C build-cpu nm -D build-cpu/src/libvmaf.so | grep -E "vmaf_(hip|metal)_" all 14 documented public symbols are present (T-type) and a smoke main linking against libvmaf.so observes -ENOSYS / NULL out-params on every entry point and 0 from the availability probes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(gpu-dispatch): enforce token boundary on strategy-name match The shared VMAF_<BACKEND>_DISPATCH env-variable parser in core/src/gpu_dispatch_parse.h matched strategy names with strncmp(v, strategy_names[idx], slen) and accepted any string with the strategy name as a prefix. So VMAF_CUDA_DISPATCH=feature:directx silently routed to the valid "direct" strategy instead of being treated as unknown. A typo in any backend's dispatch env-var was indistinguishable from a valid override at the parse layer. Add a token-boundary check after the strncmp: the byte at v[slen] must be one of '\0', ',', '\n', ' ', or '\t' for a match to succeed. The terminator set mirrors the grammar documented in the header's leading doc comment plus newline (for env values read line-by-line in tests). Adds core/test/test_gpu_dispatch_parse.c (9 cases) wired into core/test/meson.build under the `fast` suite. The negative-control (prefix-without-boundary) cases fail against the previous code and pass against the fix, so any regression is caught locally and in CI. **Research digest** — no digest needed: trivial bug fix with a small, targeted reproducer. **Decision matrix** — no alternatives: only-one-way fix. The terminator set is mechanically derived from the doc-commented grammar plus '\n' for line-buffered tests. **AGENTS.md invariant note** — no rebase-sensitive invariants: file is fork-added (ADR-0483) and absent in upstream. **Reproducer / smoke-test command** — meson setup build-cpu core -Denable_cuda=false -Denable_sycl=false \ -Denable_hip=false -Denable_metal=disabled -Denable_dnn=disabled ninja -C build-cpu test/test_gpu_dispatch_parse -j4 meson test -C build-cpu --no-rebuild test_gpu_dispatch_parse **CHANGELOG fragment** — changelog.d/fixed/gpu-dispatch-parse-strict-match.md **Rebase note** — docs/rebase-notes.md entry gpu-dispatch-parse-strict-strategy-match (2026-05-30). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(changelog): add bundle-core-c fragment for PRs #148 #307 #308 #316 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Lusoris <lusoris@pm.me>
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
core/include/libvmaf/libvmaf_hip.handcore/include/libvmaf/libvmaf_metal.hdocument that every public entry point returns-ENOSYSwhen libvmaf is built without the relevant backend, but the real bodies all sit behind#ifdef HAVE_HIP/#ifdef HAVE_METALso on a default-Denable_hip=false -Denable_metal=disabledbuild the symbols were not emitted at all — any downstream link that referenced them failed.core/src/hip/stubs.c+core/src/metal/stubs.cmirroring the canonicalcore/src/dnn/dnn_api.cVMAF_HAVE_DNNstub pattern.libvmaf_feature_static_libviahip_sources/metal_sourcesfrom theelsebranch ofif is_hip_enabled/if is_metal_enabledincore/src/meson.buildso they compile only when the backend is OFF.The header contract is now honoured:
vmaf_hip_available()/vmaf_metal_available()return 0, every other entry point returns-ENOSYS, and pointer-returning entries set the out-param to NULL.Test plan
meson setup build-cpu core -Denable_hip=false -Denable_metal=disabled -Denable_cuda=false -Denable_sycl=false -Denable_dnn=disabled && ninja -C build-cpusucceeds.nm -D build-cpu/src/libvmaf.so | grep -E "vmaf_(hip|metal)_"lists all 14 public entry points asT(text/exported).libvmaf.soobserves-ENOSYS (-38)and NULL out-params on every entry point and0from the availability probes.clang-format --dry-run --Werrorclean on both new TUs.clang-tidy -p build-cpureports no warnings insidestubs.c(all 19+39 generated warnings are in non-user headers and are suppressed).pre-commit run --files <touched>passes (clang-format, copyright, worktree-drift, semgrep, ...).scripts/ci/assertion-density.sh+scripts/ci/check-copyright.shpass on the new files.ADR-0108 deliverables checklist
core/src/dnn/dnn_api.cVMAF_HAVE_DNNprecedent verbatim. The header contract is already documented inlibvmaf_hip.h/libvmaf_metal.h.-ENOSYScontract (a stub TU compiled in the negation of the backend'sHAVE_*macro). The DNN twin established the pattern.stubs.cline to the directory layout in bothcore/src/hip/AGENTS.mdandcore/src/metal/AGENTS.mdflagging that this file is wired in fromcore/src/meson.build'selsebranch (NOT viasubdir('hip')/subdir('metal')) and mirrors thednn_api.cVMAF_HAVE_DNNpattern.meson setup/ninja/nmtriple).changelog.d/fixed/hip-metal-enosys-stubs.md(new).docs/rebase-notes.md—no rebase impact: REASONsince every touched file is fork-local (the public headers, the meson gating, the stub TUs; upstream Netflix/vmaf has no HIP or Metal backend).Scope guard
meson_options.txtentry changed — themeson.buildchange is purely gating the new stub TUs on the inverse of existingis_hip_enabled/is_metal_enabledflags.ffmpeg-patches/not touched (no API surface change).docs/state.mdnot updated (no Open / Recently closed bug row associated with the audit-only S4 finding).🤖 Generated with Claude Code