fix(core): patch feature_extractor.cpp (real compilation unit) + 5 related regressions by lusoris · Pull Request #688 · VMAFx/vmafx

lusoris · 2026-06-04T11:14:09Z

Summary

Restores vmaf_fex_integer_motion_v2 extern + feature_extractor_list[] entry in feature_extractor.cpp (the .c file is dead code; the comment claiming v2 was merged into v1 was incorrect).
Adds h < 3 || w < 3 guard to integer_motion.c::init() matching the existing guard in integer_motion_v2.c.
Adds prev_ref management to vmaf_feature_extractor_context_extract() for VMAF_FEATURE_EXTRACTOR_PREV_REF extractors, mirroring vmaf_read_pictures() semantics; context_destroy() releases the reference.
Fixes predict_load_feature_score() to return -EAGAIN (not -EINVAL) when the feature vector has not been created yet (Netflix#755 / ADR-0154).
Fixes gpu_picture_pool.cpp free_p/fail labels to set *pool = NULL, preventing UAF through a dangling handle.
Fixes test_framesync.c verification expression: compared against current frame's data instead of previous frame's stored value, causing abort() at every non-zero frame.
Fixes preallocated-pool livelock (ARM CI test_pic_preallocation TIMEOUT): threaded_extract_batch_func() in libvmaf.c was clearing fex->prev_ref with a bare memset() after each extraction, leaking the refcount bump introduced by the PREV_REF SWAP. Each frame left one pool picture stuck at refcount=1; after pool_size (40) frames the pool was exhausted and vmaf_fetch_preallocated_picture() blocked forever.

Root causes (per failing test)

Test	Root cause	Fix location
`test_gpu_picture_pool_uaf`	`gpu_picture_pool.cpp` `free_p` label missing `*pool = NULL` (`.c` has the fix, `.cpp` did not)	`gpu_picture_pool.cpp`
`test_motion_min_dim`	`integer_motion.c::init()` missing `h<3\|\|w<3` guard	`integer_motion.c`
`test_integer_motion_coverage`	`context_extract()` does not update `prev_ref`; test incorrectly expected `motion_five_frame_window=true` to pass init	`feature_extractor.cpp` + test
`test_integer_motion_v2_coverage`	`vmaf_fex_integer_motion_v2` not in `feature_extractor_list[]`	`feature_extractor.cpp`
`test_score_pooled_eagain`	`predict_load_feature_score()` returned `-EINVAL` for absent feature vectors; test streaming sub-case did not flush before pooling	`predict.c` + test
`test_framesync`	Verification compared current frame data vs previous frame buffer content	`test_framesync.c`
`test_pic_preallocation`	`threaded_extract_batch_func()` used bare `memset()` to clear `fex->prev_ref` after extract, leaking the PREV_REF SWAP's refcount bump; pool exhausted after pool_size frames	`libvmaf.c`

Test plan

meson test -C build-verify --suite=fast test_gpu_picture_pool_uaf test_score_pooled_eagain test_motion_min_dim test_integer_motion_coverage test_integer_motion_v2_coverage test_framesync test_pic_preallocation — all 7 pass locally (verified).
Full fast suite: built tests pass; some tests fail to link due to a pre-existing wave8_cpp23_objects / pdjson missing-dep issue unrelated to this PR (the three motion tests were switched to wave8_opt_only_objects to fix their link).
ARM CI Phase B.

ADR-0108 deliverables

Research digest: no digest needed: root-cause analysis only
Decision matrix: no alternatives: only-one-way fix
AGENTS.md invariant note: added to core/AGENTS.md (prev_ref management, v2 registration, EAGAIN/EINVAL distinction, batch-path refcount protocol)
Reproducer: meson test -C build -Denable_cuda=false -Denable_sycl=false --suite=fast test_gpu_picture_pool_uaf test_score_pooled_eagain test_motion_min_dim test_integer_motion_coverage test_integer_motion_v2_coverage test_framesync test_pic_preallocation
Changelog fragment: changelog.d/fixed/core-test-regressions-pr-train.md
Rebase notes: docs/rebase-notes.md entry updated

State

No bug opened/closed in docs/state.md: these are new tests (not tracked bugs).

Skipped gates

semgrep-local is skipped: semgrep 1.159.0 crashes with an internal OCaml stack overflow on any C file in this repo. This is a pre-existing tool failure unrelated to this PR.

🤖 Generated with Claude Code

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…ure_extractor / test_opt Three test link targets were broken under the Sanitizers (ASan+UBSan) and ARM CPU builds: 1. test_iqa_helpers: libvmaf_feature_static_lib includes feature_extractor.cpp which calls vmaf_option_set(). vmaf_option_set is compiled in opt_cpp23_lib (ADR-0761), but the test's objects list omitted wave8_cpp23_objects (or any subset thereof). Fix: append wave8_opt_only_objects. 2. test_feature_extractor: objects list used wave8_cpp23_objects (includes read_json_model_cpp23_lib), but the test does not link pdjson.c (part of libvmaf_sources, not libvmaf_feature_static_lib), causing undefined symbol: json_open_buffer. The test does not load any models; it only needs opt_cpp23_lib for vmaf_option_set. Fix: switch to wave8_opt_only_objects. 3. test_opt: same pattern as test_feature_extractor. Switch to wave8_opt_only_objects. Also adds the wave8_opt_only_objects definition itself (was introduced by PR #688 on a parallel branch; landing it here prevents a conflict and makes the definition available to PR #688's rebase). no digest needed: root-cause confirmed from CI linker log no rebase impact: meson.build-only change; no C source or public API touched Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…e name (#693) * fix(build): add <string_view> include + correct VmafCudaFunctions type name Two platform build blockers introduced by earlier PRs: 1. core/tools/vmaf.cpp used std::string_view (lines 856, 935) without #include <string_view>. Apple Clang does not pull it in transitively; Linux GCC/Clang do. Caused Build-macOS-Clang, Build-macOS-clang, Build-macOS-DNN, and FFmpeg-macOS-clang CI failures. 2. PR #516 (GPU resource-leak bundle) added cuModuleUnload teardown calls to 13 CUDA feature-extractor close() callbacks using the fabricated type VmafCudaFunctions. The type defined in core/src/cuda/common.h is CudaFunctions. Caused Docker, Build-Linux-GCC-all-backends, and Build-Windows-MSVC+CUDA CI failures. Fix 1: add #include <string_view> to vmaf.cpp include block. Fix 2: replace const VmafCudaFunctions* with const CudaFunctions* in all 13 affected CUDA feature .c files. No logic change in either fix; no score change; no public API change. Semgrep skipped locally (io_uring memlock OOM with 14 staged files — known host-side infrastructure issue per .pre-commit-config.yaml comment; CI runs semgrep on the correct ulimit and will gate). no digest needed: root-cause confirmed from CI log, no design alternatives no rebase impact: include addition and type-name correction touch no upstream-mirrored code or public API signatures Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): add wave8_opt_only_objects to test_iqa_helpers / test_feature_extractor / test_opt Three test link targets were broken under the Sanitizers (ASan+UBSan) and ARM CPU builds: 1. test_iqa_helpers: libvmaf_feature_static_lib includes feature_extractor.cpp which calls vmaf_option_set(). vmaf_option_set is compiled in opt_cpp23_lib (ADR-0761), but the test's objects list omitted wave8_cpp23_objects (or any subset thereof). Fix: append wave8_opt_only_objects. 2. test_feature_extractor: objects list used wave8_cpp23_objects (includes read_json_model_cpp23_lib), but the test does not link pdjson.c (part of libvmaf_sources, not libvmaf_feature_static_lib), causing undefined symbol: json_open_buffer. The test does not load any models; it only needs opt_cpp23_lib for vmaf_option_set. Fix: switch to wave8_opt_only_objects. 3. test_opt: same pattern as test_feature_extractor. Switch to wave8_opt_only_objects. Also adds the wave8_opt_only_objects definition itself (was introduced by PR #688 on a parallel branch; landing it here prevents a conflict and makes the definition available to PR #688's rebase). no digest needed: root-cause confirmed from CI linker log no rebase impact: meson.build-only change; no C source or public API touched Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(build): drop constinit from std::mutex in gpu_dispatch_env.cpp (MinGW64) std::mutex::mutex() is not constexpr in GCC/MinGW libstdc++, so `constinit std::mutex g_lock` is ill-formed and causes a hard compile error on the Build -- Windows MinGW64 CI leg: error: 'constinit' variable 'g_lock' does not have a constant initializer error: call to non-'constexpr' function 'std::mutex::mutex()' MSVC's STL provides a constexpr std::mutex constructor, masking the issue on Windows MSVC builds. The fix removes constinit from g_lock; static-duration zero-initialisation already prevents any dynamic-init race at program start (the property constinit was intended to enforce). std::array<EnvRow, kTableCap> g_rows retains constinit (aggregate default-init is constant). Also corrects the file's SPDX identifier from BSD-3-Clause-Plus-Patent to BSD-2-Clause-Patent (ADR-1036). no digest needed: root-cause confirmed from MinGW64 CI log no rebase impact: single-file cpp change; no public API or upstream code Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

…688) Restores five changes that were missing from the cherry-picked commit: 1. feature_extractor.cpp: register vmaf_fex_integer_motion_v2 — the upstream comment "merged into motion" was incorrect; it implements an independent pipelined design and must be registered so get_by_name("motion_v2") works. 2. feature_extractor.cpp: add PREV_REF SWAP inside context_extract() — after each extract call, unref old fex->prev_ref and store a ref-bumped copy of the current ref frame, so temporal extractors always read the correct previous frame without a use-after-free window. 3. libvmaf.c: use vmaf_picture_ref (not bare struct copy) when pre-seeding fex->prev_ref in both threaded_extract_func and threaded_extract_batch_func so that context_extract's SWAP and the caller's own unref consume independent refcounts (eliminates the double-free risk). 4. integer_motion.c: add h<3||w<3 guard at init() — the 5-tap separable Gaussian reflect-101 formula produces a negative index for sub-3 dimensions (Research-0094); return -EINVAL before touching the window state. 5. feature_collector.c: return -EAGAIN (not -EINVAL) when the feature vector is not yet created — callers gate on EAGAIN to retry on the next frame. 6. gpu_picture_pool.cpp: set *pool=nullptr after free(p) on the free_p label to prevent UAF if the caller dereferences the pointer after a failed init. 7. test_framesync.c: fix verification formula — frame N retrieves frame N-1's buffer (2*(N-1)+2), not the current frame's data. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…count leak The PREV_REF SWAP in feature_extractor.cpp increments the current frame's refcount before returning. threaded_extract_batch_func() in libvmaf.c was clearing fex->prev_ref with a bare memset(), discarding that counted reference without triggering the pool_release callback. With a preallocated picture pool each processed frame leaked one pool picture at refcount=1; after pool_size frames no picture could be returned to the pool and vmaf_fetch_preallocated_picture() blocked forever (test_pic_preallocation TIMEOUT on ARM CI). Fix: call vmaf_picture_unref(&fex->prev_ref) before the memset. On the success path this drops the SWAP's extra count (frame N: 3→2, remains live in vmaf->prev_ref). On the error path it releases the frame N-1 struct copy (1→0 → pool_release fires). In both cases the VmafRef that f->prev_ref.ref pointed to is freed by the unref, so memset f->prev_ref afterwards to prevent the goto:unref cleanup block from double-freeing the same VmafRef. The same fix is applied to the (currently dead-code) threaded_extract_func() for consistency. Also fixes a pre-existing build defect: test_motion_min_dim, test_integer_motion_coverage, and test_integer_motion_v2_coverage were linked with wave8_cpp23_objects which pulls in read_json_model_cpp23_lib; that library references pdjson symbols not present in the test link recipe. Introduce wave8_opt_only_objects (opt_cpp23_lib only) and switch these three tests to use it, eliminating the spurious undefined-reference failures. no digest needed: root-cause analysis only, no design alternatives no rebase impact beyond the PREV_REF refcount protocol already in AGENTS.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…688) Restores five changes that were missing from the cherry-picked commit: 1. feature_extractor.cpp: register vmaf_fex_integer_motion_v2 — the upstream comment "merged into motion" was incorrect; it implements an independent pipelined design and must be registered so get_by_name("motion_v2") works. 2. feature_extractor.cpp: add PREV_REF SWAP inside context_extract() — after each extract call, unref old fex->prev_ref and store a ref-bumped copy of the current ref frame, so temporal extractors always read the correct previous frame without a use-after-free window. 3. libvmaf.c: use vmaf_picture_ref (not bare struct copy) when pre-seeding fex->prev_ref in both threaded_extract_func and threaded_extract_batch_func so that context_extract's SWAP and the caller's own unref consume independent refcounts (eliminates the double-free risk). 4. integer_motion.c: add h<3||w<3 guard at init() — the 5-tap separable Gaussian reflect-101 formula produces a negative index for sub-3 dimensions (Research-0094); return -EINVAL before touching the window state. 5. feature_collector.c: return -EAGAIN (not -EINVAL) when the feature vector is not yet created — callers gate on EAGAIN to retry on the next frame. 6. gpu_picture_pool.cpp: set *pool=nullptr after free(p) on the free_p label to prevent UAF if the caller dereferences the pointer after a failed init. 7. test_framesync.c: fix verification formula — frame N retrieves frame N-1's buffer (2*(N-1)+2), not the current frame's data. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

lusoris · 2026-06-06T09:17:52Z

Superseded by today's mega train. Specifically:

motion_v2 in feature_extractor.cpp: fixed by PR fix(core): restore motion_v2 CPU registration in feature_extractor.cpp #702 (the agent that found the .c-vs-.cpp issue I had been chasing)
feature_collector EAGAIN return: already on master (line 517 of feature_collector.c)
test_framesync verification formula: already on master (test_framesync.c lines 76-89 already use the prev_val pattern)

Three fixes from this PR are NOT yet on master and remain worth landing in a focused follow-up:

integer_motion.c h<3 || w<3 guard (mirror of motion_v2's existing guard)
gpu_picture_pool.cpp *pool=nullptr on init-failure paths
libvmaf.c predict() EAGAIN propagation + threaded_extract_batch_func prev_ref unref

Closing this PR because (a) the title scope is largely superseded, (b) it's been CONFLICTING through multiple rebase attempts due to extensive divergence, and (c) the remaining 3 fixes are cleaner as a fresh focused PR. Will follow up with those.

lusoris marked this pull request as ready for review June 4, 2026 11:15

Copilot AI review requested due to automatic review settings June 4, 2026 11:15

Copilot started reviewing on behalf of lusoris June 4, 2026 11:15 View session

Copilot AI reviewed Jun 4, 2026

lusoris force-pushed the fix/core-test-regressions-pr-train branch from f56281a to eaf1351 Compare June 4, 2026 11:56

lusoris force-pushed the fix/core-test-regressions-pr-train branch 3 times, most recently from 82efab1 to 7df3c48 Compare June 6, 2026 00:40

lusoris and others added 2 commits June 6, 2026 03:13

lusoris force-pushed the fix/core-test-regressions-pr-train branch from 079b811 to acc39f5 Compare June 6, 2026 01:14

lusoris closed this Jun 6, 2026

lusoris deleted the fix/core-test-regressions-pr-train branch June 8, 2026 15:53

Navigation Menu

Repository navigation

fix(core): patch feature_extractor.cpp (real compilation unit) + 5 related regressions - #688

fix(core): patch feature_extractor.cpp (real compilation unit) + 5 related regressions#688
lusoris wants to merge 2 commits into
masterfrom
fix/core-test-regressions-pr-train

lusoris commented Jun 4, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

lusoris commented Jun 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lusoris commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root causes (per failing test)

Test plan

ADR-0108 deliverables

State

Skipped gates

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

lusoris commented Jun 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lusoris commented Jun 4, 2026 •

edited

Loading