cuda: don't flush CUDA feature extractors twice when a thread pool is present - #1553
Open
BardieJoensen wants to merge 1 commit into
Open
cuda: don't flush CUDA feature extractors twice when a thread pool is present#1553BardieJoensen wants to merge 1 commit into
BardieJoensen wants to merge 1 commit into
Conversation
motion_cuda is declared TEMPORAL | CUDA. With a thread pool present, flush_context_threaded() flushes every TEMPORAL extractor and the HAVE_CUDA block in flush_context() then flushes every CUDA extractor, so motion_cuda gets flushed twice. The second flush re-appends the final motion2 score at the same picture index, which feature_vector_append() rejects, and the resulting -EINVAL is folded into the same err as the cuCtxSynchronize() calls, so any CUDA run with --threads N aborts with a misleading "context could not be synchronized" even though no CUDA call failed. Skip CUDA extractors in the temporal flush loop and leave them to the HAVE_CUDA block, which is the only thing that flushes them on the non-threaded path already. Verified across --threads 0/1/4 x default/--gpumask 0: every combination now completes, where before any --threads >= 1 run aborted with exit 234. On 8-bit input all six combinations produce identical scores; 10-bit needs the 16bpc motion stride fix (submitted separately) on top for full score equality, since that bug's out-of-bounds reads vary with allocation layout.
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
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.
Any CUDA-enabled run with
--threads Ncurrently aborts at flush time:The error message points at CUDA, but no CUDA call fails (
CUDA_LAUNCH_BLOCKING=1surfaces nothing, compute-sanitizer is clean). The actual problem is in the flush logic.motion_cudais the only extractor declared with both flags:When a thread pool exists,
flush_context()flushes it twice: once inflush_context_threaded(), which walks every TEMPORAL extractor, and again in the#ifdef HAVE_CUDAblock, which walks every CUDA extractor. The second flush re-appends the final motion2 score at the same picture index,feature_vector_append()rejects it (that's the warning), and the-EINVALis OR-ed into the sameerras thecuCtxSynchronize()calls, producing the misleading context error.Instrumenting
flush_fex_cuda()with an fprintf on entry and return shows it directly (12-frame clip, indexes 0..11):The fix skips CUDA extractors in the temporal flush loop and leaves them to the HAVE_CUDA block, which matches what the non-threaded path already does (its non-CUDA loop excludes them the same way).
Verified on an RTX 5060 Ti (driver 595.71.05, CUDA 12.9 and 13.3 builds, master @ 0f9912e), 1080p y4m pairs: all six combinations of
--threads 0/1/4x default/--gpumask 0now complete, where before every--threads >= 1run aborted with exit 234. On the 8-bit pair all six produce identical scores (mean 63.9282). On a 10-bit pair they only fully converge with the 16bpc motion stride fix (#1552) applied on top — that bug reads out of bounds, so its effect on the score varies with allocation layout; with both fixes, all six combinations give the same mean (63.5439).#1538 addresses the same symptom by destroying the thread pool in
vmaf_cuda_import_state(). That also resolves the abort, but it silently disables threading whenever CUDA state is imported — including--gpumask 0runs, where feature extraction happens on the CPU and does benefit from the pool. With the double flush fixed the pool can stay; threaded and single-threaded runs produce identical scores in all the combinations above.A possible follow-up (not in this PR): separating the extractor-flush
errfrom the CUDA syncerrinflush_context()would stop feature-collector errors from being reported as "context could not be synchronized" — that conflation is what made this bug (and apparently #1538) hard to triage in the first place.