Skip to content

model, feature: clean up option dictionaries on failure - #1588

Open
lusoris wants to merge 2 commits into
Netflix:masterfrom
VMAFx:fix/model-overload-ownership
Open

model, feature: clean up option dictionaries on failure#1588
lusoris wants to merge 2 commits into
Netflix:masterfrom
VMAFx:fix/model-overload-ownership

Conversation

@lusoris

@lusoris lusoris commented Sep 8, 2026

Copy link
Copy Markdown

Feature-option overloads leak supplied dictionaries when merging fails; collection overloads can leak partial copies and return success. Feature registration has related failure paths: copied dictionaries survive copy/create errors, and an invalid option leaves the constructor's context, extractor descriptor and private storage allocated. Callers also bind backend/frame-sync state before checking whether creation succeeded.

This change releases owned allocations, propagates errors, and publishes a feature context only after successful construction. It extends the focused overload cleanup from VMAFx #1223. Related to #1242; it does not redesign or deprecate the API.

Ownership documentation preserves the existing distinctions:

  • Model overloads consume the supplied dictionary after argument validation, including merge failure and no matching feature. A NULL argument retains caller ownership.
  • vmaf_use_feature() retains its supplied dictionary on argument/name rejection or failure to copy it. Once copying succeeds, it consumes that dictionary even if subsequent validation/registration fails. The error code alone cannot distinguish these stages.
  • Model and worker registration borrow their source options and own only their private copies. Earlier successful registrations/overloads remain in place if a later operation fails.

Tests cover natural invalid-option rejection and retry, constructor publication and all three constructor allocation failures, explicit/model partial-copy cleanup, and explicit/model context-create failure. The existing test_context executable is now registered with Meson. Deterministic allocation tests use GNU link wrapping on Linux static builds; no production test hooks or C++ runtime helpers are added.

Validation against upstream f85a853692a8c730d0270cd733c8bb30b5b93b7c (libvmaf 3.2.0), x86-64 Linux, GCC 15.2, Meson 1.10.1, C11:

meson setup build libvmaf -Denable_cuda=false -Denable_docs=false -Denable_float=true --buildtype=release
ninja -C build
meson test -C build --print-errorlogs
# 25/25 passed

meson setup build-sanitize libvmaf -Denable_cuda=false -Denable_docs=false -Denable_float=true --buildtype=debug -Db_sanitize=address,undefined
ninja -C build-sanitize
ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 UBSAN_OPTIONS=halt_on_error=1 \
  meson test -C build-sanitize test_model test_context test_feature_extractor test_registration_failure --print-errorlogs
# 4 targets / 24 cases passed; ASan/UBSan/LSan clean

Negative controls preserve the original implementations. A real public model-registration probe leaks 554 bytes in seven allocations with the original shared library and is clean with the fixed library. The new constructor test fails because the original publishes a failed context; the partial-copy test fails because the original does not free the copy. Earlier overload controls also fail on the original merge/copy paths.

Worker failure paths were source-reviewed, not covered by injected runtime worker failures. Unrelated private pool defects are outside this change. The previously recorded full sanitizer baseline had existing leaks in test_predict and test_pic_preallocation; this update reruns the ownership-focused sanitizer set and does not claim a green full sanitizer suite. No Netflix golden assertion, score calculation or GPU runtime validation is changed or claimed.

@lusoris lusoris changed the title model: clean up dictionaries and report overload failures model, feature: clean up option dictionaries on failure Sep 8, 2026
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