feat(libvmaf): compile picture_pool unconditionally, drop VMAF_PICTURE_POOL gate#32
Merged
feat(libvmaf): compile picture_pool unconditionally, drop VMAF_PICTURE_POOL gate#32
Conversation
lusoris
force-pushed
the
feat/picture-pool-always-on
branch
from
a95e4d1 to
f3156a5
Compare
…E_POOL gate vmaf_preallocate_pictures and vmaf_fetch_preallocated_picture have always been declared in the public header libvmaf/libvmaf.h, but the definitions were guarded by -DVMAF_PICTURE_POOL — a flag the user had to opt into manually. Default builds shipped the declarations without the bodies, so any consumer who called either function got a linker error. Flip to always-on per issue recommendation B: - src/meson.build: picture_pool.c is now part of libvmaf_sources. - src/libvmaf.c: drop 5 #ifdef VMAF_PICTURE_POOL sites (include, struct field, prepare_picture_pool block, vmaf_close cleanup, check_picture_pool call in the frame path). - tools/vmaf.c: drop fallback branch — the CLI always uses the pool. - test/meson.build: test_pic_preallocation always builds and runs. All 26 meson tests pass. nm -D libvmaf.so now shows: T vmaf_fetch_preallocated_picture T vmaf_preallocate_pictures Closes #29. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The always-on picture pool (PR #32 parent commit) used pic_cnt = 2 when --threads 0 and (thread_cnt+1)*2 otherwise. Both undercount by one: vmaf->prev_ref holds the previous frame's ref picture across frame boundaries for motion features, so one pool slot is permanently live. Frame N+1 then cannot fetch both ref + dist and deadlocks in vmaf_picture_pool_fetch -> pthread_cond_wait. Reproduced on the Netflix golden pair (src01_hrc00 vs src01_hrc01): prior code hangs at frame 2; with pic_cnt = 2*(thread_cnt+1)+1 the CLI processes all frames and matches expected scores. Adds ADR-0104 capturing the sizing rationale + rejected alternatives. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
lusoris
force-pushed
the
feat/picture-pool-always-on
branch
from
f3156a5 to
b47ff26
Compare
The four early `return err;` sites (gpu collect, gpu submit, cpu extract, dnn run-frame) skipped the trailing vmaf_picture_unref(ref/dist) at the function tail. Pre-#32 this was a small malloc-backed leak the OS reclaimed at exit; with the always-on picture pool from #32 the leak holds a finite pool slot, and the next vmaf_picture_pool_fetch deadlocks in pthread_cond_wait once the pool drains. Reproducible deterministically by running the Netflix-golden pytest sequence: test_run_vmaf_runner_rdh540 invokes the adm extractor with adm_ref_display_height=540, the extractor errors on frame 0, and the single-threaded CLI hangs on frame 1's pool fetch. Standalone vmaf invocations exit cleanly because they bail out before any second-frame fetch is attempted. Restructured the four returns to `goto cleanup` so the existing ref/dist teardown runs on both success and error paths. CUDA branch unrefs are unchanged (also reachable via the new label). prev_ref update remains success-only. Verified: full quality_runner_test.py + feature_extractor_test.py sequence completes in 73s; previously hung indefinitely at rdh540. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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_preallocate_pictures/vmaf_fetch_preallocated_picturewere declared unconditionally in libvmaf/libvmaf.h:341,355, but the definitions lived under#ifdef VMAF_PICTURE_POOL— an opt-in C flag nobody passes. Default builds shipped the declarations without the bodies → any consumer got a linker error.Closes #29.
Changes
picture_pool.cnow part oflibvmaf_sources.#ifdef VMAF_PICTURE_POOLsites (include, struct field,prepare_picture_poolblock,vmaf_closecleanup,check_picture_poolcall).vmaf_picture_allocbranch — the CLI always uses the pool.test_pic_preallocationalways builds and runs.Verification
nm -D libvmaf.sonow shows both symbols on default build:meson test -C build), includingtest_pic_preallocation.🤖 Generated with Claude Code