chore(lint): clang-tidy upstream cleanup rounds 2-4#2
Merged
Conversation
Previously `vmaf_use_tiny_model()` opened the ONNX session, validated the op allowlist, then closed it immediately without ever running inference. The CLI flag succeeded but published no feature, which contradicted the principles.md "no half-finished implementations" rule. Extend ort_backend with a `vmaf_ort_input_shape()` accessor so the caller can statically determine the expected tensor dims. Introduce an internal `dnn_ctx` bridge header that transfers ownership of the opened session, the sidecar metadata, and the strdup'd feature name into VmafContext. `vmaf_read_pictures()` now invokes the tiny model on the main thread after the extractor dispatch completes: reference luma is normalised into the pre-allocated tensor buffer via `vmaf_tensor_from_luma`, the scalar output is appended to the feature collector under the sidecar name (or `vmaf_tiny_model` when no sidecar is present). Current scope is NCHW [1, 1, H, W] 8-bit luma with matching picture dims. Non-matching shapes, bit-depths, or ranks return -ENOTSUP / -ERANGE at attach/first-frame time rather than silently producing wrong scores. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The five shipped manifests under ai/src/vmaf_train/data/manifests/ are empty by design — the repository cannot redistribute Netflix / KoNViD / LIVE-VQC / YouTube-UGC / BVI-DVC content or MOS scores under their respective licences. Previously there was no tooling to populate them, which left the files indistinguishable from scaffolds. Add a `vmaf-train manifest-scan --dataset <name> --root <path> [--mos-csv <csv>]` subcommand that walks a local cache, pins every YUV/Y4M/MP4/MKV/WebM file by SHA-256, optionally joins a MOS CSV (columns: key,mos), and overwrites the in-tree manifest. Each manifest file now carries a short comment that documents why it ships empty and points at the scan command. The manifests/ README is updated with the full workflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduces vmaf_dnn_session_open / vmaf_dnn_session_run_luma8 / vmaf_dnn_session_close — a session surface that does not need a VmafContext. Intended for consumers that want luma-in / luma-out learned pre-processing without running the scoring pipeline (e.g. the new FFmpeg vmaf_pre filter in ffmpeg-patches/0002). The session owns its own ORT handle, sidecar metadata, and paired input/output float buffers. Shape is validated against NCHW [1,1,H,W] f32; normalization follows the model sidecar when present. The -ENOSYS stubs are preserved for builds without -Denable_dnn.
…elector Rewrites the patch series so it targets the current FFmpeg release (n8.1) instead of scaffolded drafts: - 0001 adds tiny_model/tiny_device/tiny_threads/tiny_fp16 options to the existing libvmaf filter and calls vmaf_use_tiny_model(). - 0002 adds a new vmaf_pre filter wired to vmaf_dnn_session_* (the new session API introduced in the previous commit) instead of the previous filter-run TODO; chroma planes pass through. - 0003 wires the SYCL backend selector into vf_libvmaf: sycl_device (-1 disables, 0+ selects), sycl_profile, CONFIG_LIBVMAF_SYCL-guarded state init/import/free. All three patches use FFmpeg n8.1's FFFilter struct layout, updated configure check_pkg_config anchors, and insert at correct line ranges verified against upstream source. Updates series.txt (drops the scaffold comments, lists all three), build-and-run.sh (FFMPEG_SHA: n7.1 → n8.1, git apply --3way for tolerance to small upstream drift), and README.md (replaces "scaffolding only" with an accurate per-patch summary).
- ai/pyproject.toml: force-include pointed at src/vmaf_train/configs which does not exist; the real training configs live at ai/configs/. Fix the mapping so `hatchling build` resolves the path. - libvmaf/src/dnn: the -Denable_dnn=false stub paths necessarily match the real-ORT function signatures declared in the public header, so clang-tidy's readability-non-const-parameter is a false positive on those out-params. Annotate with NOLINT instead of changing signatures.
Bring .clang-tidy's readability-function-size thresholds in line with docs/principles.md §1.1 Power-of-10 rule 4 (LineThreshold 60 / StatementThreshold 120 / BranchThreshold 15 / NestingThreshold 4). Oversized upstream functions get NOLINTNEXTLINE markers with a refactor ticket tracked in .workingdir2/OPEN.md. libvmaf.c: suppress reserved-identifier on `__libc_single_threaded` (ABI-mandated), isolate aggregate declarations, add missing braces, (void)-cast unchecked snprintf/fprintf calls (cert-err33-c), and propagate fclose errors in vmaf_write_output_with_format. read_json_model.c: include its own header to silence misc-use-internal -linkage false positives, add missing braces, fix key/m leak paths, replace banned sprintf with snprintf (JPL rule 30), check fclose returns, and NOLINT the cross-parameter ownership transfer that the analyzer can't track. model.c: replace banned strcpy with memcpy (JPL rule 30 / CERT clang-analyzer-security.insecureAPI.strcpy). docs/principles.md: factual note in §1.3 that the noisy Microsoft- only insecureAPI.DeprecatedOrUnsafeBufferHandling subset is disabled (it has no portable POSIX equivalent). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace atoi() with strtol + errno/ERANGE/INT_MIN/INT_MAX validation (JPL rule 30 / CERT ERR30-C), replace rewind() with fseek + error check, (void)-cast all fclose calls on error paths (cert-err33-c), split long-typed file-size into sz_raw/sz for cleaner size_t arithmetic, and add an assert + NOLINT justification for the buf[sz] = '\\0' null-termination the analyzer loses track of across the fread path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the stale "download stubs" framing with a concrete pointer to the manifest-scan CLI command that populates manifests from a local VMAF_DATA_ROOT cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Systematic sweep of libvmaf/src under stricter clang-tidy budget. Fixes
both real bugs and policy violations in upstream Netflix code.
Real bugs fixed:
- framesync.c: use-after-free in vmaf_framesync_destroy. Loop walked
linked list by buf_cnt count but freed tail early, causing next
iteration to read freed memory. Rewrote as standard list-walk.
- feature_collector.c: vmaf_feature_collector_append had two-part
allocation bug — multiplied sizeof twice (bytes squared) and used
the wrong struct's capacity field. Also leaked feature_vector on
realloc failure.
- feature_extractor.c: vmaf_fex_ctx_pool_aquire leaked a dict on
copy/create failure and leaked the newly-created fex_ctx if the
slot was already in_use. Now assigns slot unconditionally after
creation and frees the dict on error paths.
- log.c: missing va_end after va_start.
- dict.c: vmaf_dictionary_set used existing_entry after realloc
could invalidate it; restructured so overwrite runs before realloc.
Also leaked buf on realloc-fail goto.
Policy/style fixes:
- opt.c: replaced banned atoi/atof with strtol/strtod with full
validation (errno, end-pointer, range).
- feature_collector.c, dict.c: replaced banned strcpy/strncpy with
memcpy + explicit NUL terminator.
- Added explicit (void *) / (T **) casts for multilevel pointer
conversions (model.c, predict.c, fex_ctx_vector.c,
feature_collector.c, opt.c).
- Added assert(capacity > 0) before realloc doublings to satisfy
clang-analyzer-optin.portability.UnixAPI realloc-0 check.
- Converted strcmp-as-bool usage to explicit != 0 comparisons.
- Bracketed stateful cert-err33-c floods with NOLINTBEGIN/END plus
single ferror() check at end of output writers.
- Added default cases to all fpclassify switches (output.c).
- Added missing own-header includes (log.c, output.c) to fix
misc-use-internal-linkage false positives.
- Suppressed legitimate uintptr_t tagged-pointer idiom
(picture.c) with scoped NOLINT + justification comment.
- Added readability-function-size NOLINT where decomposition would
obscure flow (vmaf_picture_alloc, vmaf_dictionary_set,
vmaf_predict_score_at_index, output writers,
vmaf_fex_ctx_pool_aquire, vmaf_feature_collector_append,
vmaf_picture_pool_init).
- mem.c: explicit error-path braces; annotated _POSIX_C_SOURCE as a
POSIX feature-test macro (not a user identifier).
- pdjson.c: full-file NOLINT bracket (vendored 3rd-party).
All 21 meson tests pass. No numerical changes — behavior-preserving
correctness and lint-conformance work only.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Covers libvmaf/tools under stricter clang-tidy budget.
Real bugs fixed:
- vmaf.c: file stream leaks on early-error paths. file_ref/file_dist
were never closed when the subsequent open/validate steps failed.
Also added video_input_close on reference-video cleanup when the
distorted side fails.
Policy fixes:
- vmaf_bench.c: replaced banned atoi/sscanf in CLI argument parsing
with strtol + explicit end-pointer, range, and terminator checks.
- vmaf_vpl.c: same treatment for --frames / --device.
- y4m_input.c: scoped NOLINTBEGIN/END bracket (vendored Daala Y4M
parser; sscanf return-code checks already guard against malformed
headers and per-check suppression would clutter the file).
Remaining warning in vmaf.c is a main()-exit model_collection "leak"
that the OS reclaims on process termination — not material.
All 21 meson tests pass.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Silence two clang-analyzer-optin.portability.UnixAPI warnings about calloc(n_features, ...) potentially requesting 0 bytes. Models are validated upstream to have at least one feature; this precondition is now explicit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
main() had nine early-exit `return -1` paths that leaked heap state: the malloc'd `model[]` / `model_collection[]` arrays, open VmafContext, SYCL state, and (in the validate-videos case) the second video_input. clang-analyzer-unix.Malloc flagged this as a Potential leak of memory pointed to by 'model_collection'. Converts main() to the standard Linux-kernel cleanup-ladder pattern: - zero-init all owning locals up front - every early exit becomes `ret = -1; goto cleanup;` - track vid_ref_open / vid_dist_open + sycl_active so cleanup is safe after any partial-init point - model_collection_label VLA replaced with malloc so goto can cross its declaration scope - ownership-transfer on FILE*: once video_input_open(vid, file) succeeds, NULL the FILE* so cleanup doesn't double-close - fix benign sizeof mismatch: malloc(model_sz) for model_collection - add NULL checks on the three mallocs - add explicit casts on malloc/free/memset to silence bugprone-multi-level-implicit-pointer-conversion - rename inner shadowed `int err` in unref paths to err_unref All 21 meson tests pass. clang-analyzer-unix.Malloc warning gone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lusoris
force-pushed
the
chore/clang-tidy-round-2
branch
from
77d54b1 to
2307dd9
Compare
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
Aggressive clang-tidy cleanup sweep across
libvmaf/src/andlibvmaf/tools/, bringing analyzer-level (real-bug) warnings to zero for fork-owned surface and upstream Netflix code alike.Commits
chore(lint): clang-tidy upstream cleanup round 2— bugs fixed:vmaf_framesync_destroycounter-walk freed tail then iterated reading freed memory; fixed with next-pointer walk.initial_size * 2whereinitial_sizewas alreadysizeof*cap(bytes^2); also wrong capacity field and leak on realloc failure.vmaf_fex_ctx_pool_aquire.strcpy/strncpy→memcpy;atoi/atof→strtol/strtodwith full validation inopt.c.assert(cap > 0)) in vector growth sites.bugprone-multi-level-implicit-pointer-conversion.chore(lint): clang-tidy upstream cleanup round 3 (tools):atoi/sscanfinvmaf_bench.c,vmaf_vpl.c→ validatedstrtol.y4m_input.c(vendored Daala parser) NOLINT-bracketed.chore(lint): add n_features>0 assertions in predict.c— guards twocalloc(n_features, ...)sites.fix(tools): plug main() leak via goto-cleanup ladder in vmaf.c— eliminates the lastclang-analyzer-unix.Mallocwarning (model_collection leak). Converts 9 early-exit paths to a single reverse-order cleanup. Also fixes a benignmalloc(model_sz)→malloc(model_collection_sz)sizeof mismatch.Test plan
meson test -C build— 21/21 pass after each commitclang-tidy -p build libvmaf/{src,tools}/*.c— zeroclang-analyzer-*,bugprone-use-after-move,bugprone-dangling,cert-err34-c, orperformance-no-int-to-ptrwarnings remainOut of scope (tracked in `.workingdir2/OPEN.md`)
🤖 Generated with Claude Code