Repository navigation
docs(api): libvmaf public API error-path consistency audit (ADR-0787) - #125
Merged
Merged
Conversation
lusoris
marked this pull request as draft
May 29, 2026 11:50
lusoris
force-pushed
the
worktree-agent-a19de4c291fe6bfe1
branch
from
May 29, 2026 12:08
eb68f07 to
8b93460
Compare
lusoris
added a commit
that referenced
this pull request
May 29, 2026
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>
lusoris
added a commit
that referenced
this pull request
May 29, 2026
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>
lusoris
marked this pull request as ready for review
May 31, 2026 13:49
Contributor
Author
|
Superseded by master merge marathon 2026-05-31. |
lusoris
marked this pull request as draft
May 31, 2026 18:50
lusoris
added a commit
that referenced
this pull request
Jun 2, 2026
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>
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>
Audit of all VMAF_EXPORT functions across core/include/libvmaf/*.h and implementations. Convention confirmed: 0 on success, negative errno on failure, no VMAF-specific error namespace. Six defects identified (no implementation yet — fixes in follow-up PR): 1. vmaf_write_output_with_format() returns -EINVAL on file-open failure (should be -errno) 2. vmaf_cuda_state_init() returns -EINVAL for driver-not-found / no-GPU (should be -ENOSYS / -ENODEV) 3. vmaf_close() silently drops int return values from vmaf_framesync_destroy() and vmaf_thread_pool_wait() 4. vmaf_cuda_state_free() has single-pointer/int-return vs. double-pointer/void for all other backends (ABI break, deferred) 5. vmaf_cuda_preallocate_pictures() lacks -EBUSY guard present in SYCL/Vulkan 6. vmaf_init() funnels all sub-init errors to -ENOMEM (should propagate err) Deliverables: Research-0787, ADR-0787, changelog fragment, rebase-notes entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lusoris
force-pushed
the
worktree-agent-a19de4c291fe6bfe1
branch
from
June 3, 2026 15:17
8b93460 to
fb80f35
Compare
lusoris
marked this pull request as ready for review
June 3, 2026 15:17
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
VMAF_EXPORTfunctions incore/include/libvmaf/*.hand their implementations for error-path consistency0on success,< 0(negative errno) on failure, no VMAF-specific error namespaceFindings
libvmaf.c:2948-2964vmaf_write_output_with_format()returns-EINVALonopen(2)failure-errnocuda/common.c:154-183vmaf_cuda_state_init()returns-EINVALfor driver-not-found (should be-ENOSYS) and no-GPU (should be-ENODEV)libvmaf.c:1433-1437vmaf_close()dropsintreturn values fromvmaf_framesync_destroy()andvmaf_thread_pool_wait()(void)-castlibvmaf_cuda.h:61vmaf_cuda_state_free()isint f(T*)vs.void f(T**)for all other backendslibvmaf.c:354-383vmaf_cuda_preallocate_pictures()lacks-EBUSYguard on double-calllibvmaf.c:307vmaf_init()funnels all sub-init errors to-ENOMEMerrTest plan
meson test -C build --suite=fastpasses (doc-only change, no test impact)make lintpasses (no C changes)Deliverables checklist
docs/research/research-0787-libvmaf-api-error-path-audit.md## Alternatives consideredchangelog.d/fixed/0787-libvmaf-api-error-path-audit.mddocs/rebase-notes.md🤖 Generated with Claude Code