Repository navigation
fix(vcores): no core held while a movie picture is decoded (#1929) - #2322
Conversation
…1929) AcquireXDecoderOutputBuffer decodes the pending submissions with libavcodec, and dav1d waits there for its own frame threads: the movie thread kept its virtual core 100-270 ms on Linux, and a thread waiting for that core ran without one (the escape). dec_acquire now gives the core up for the whole call and takes it again only after d->lock is let go, as hos_cv_sleep does. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Corroboration, plus a boundary for this PR. The decoder hold is real and fits the class we see: a non-polling host wait that keeps a vcore. Our host-stack capture at the 250 ms escape also shows a different holder class — the decompressor polls at every back-edge and is better priority than the waiter, so Placement: Scope: this does not cover the two remaining non-polling holders we still see: Our local arms are 250 ms on an older 🤖 Generated with opencode using DeepSeek V4.1 Flash. |
|
Thanks. The check you asked for, from the branch head (8b4f6e6), No guest code and no guest callback runs in the released window. The window opens at line 589 and closes at 603, 619 or 702, each right after
None of these is handed the guest context: in this file only the entry points take it ( Two stores into guest memory do happen in the window, as the description's Risks section says in short: the idle sync handle's value into the caller's out-parameter (613), and the picture copied into the pool's guest memory when the game does not read it through the API (668-672, with Every way out after 589 takes the core back (603, 619, 702); the one return before it (582, no decoder) never gave it up. Scope: agreed, this is the decoder only. In our 6 runs with this change at 250 ms (33 escapes), none stood behind the decoder. Comparing counts: our arms are at 250 ms too. The runs set |
ghost
left a comment
There was a problem hiding this comment.
Looks good to me.
- The placement is right:
hos_vcore_away()befored->lock, andhos_vcore_back()only after each of the threeReleaseSRWLockExclusive, so a core is never waited for under the lock (same shape ashos_cv_sleep). All three ways out after line 589 take it back, and the early return at 582 never gave it up.hos_vcore_away/hos_vcore_backare already declared inhos.h, so nothing new is needed for the Windows build. - The nested
hos_sleep_nsin the retry loop ends up as a no-op away/back pair because no core is held, so there's exactly one take per call. The test checks this too (sleeps_with_core == 0,takes == 1). - CIB asked whether a guest callback runs inside the released window, and your reply answers it. I went through the same calls and agree:
decode_next/decode_pending,mf_picture->queue_frame(our own C),pool_rgba,host_ofandhos_log, and no guest code. The two guest-memory stores (the idle sync value and the pool copy plusHOS_WRITTEN) go into buffers the caller handed over, the same asread_atinnn_fs.con main. dec_submit/dec_flushstill wait ford->lockwhile holding their core. That isn't new: before, the decoding thread held both a core and the lock, and now only the waiter holds a core. I'm fine leaving them as they are, as the description says.- The siblings: #2301 (
nn_fs.c) and #2302 (hos.h,hos_thread.c) touch no file this PR does, so any merge order works. They also fit together. With #2302, an escaped movie thread's outerhos_vcore_away()here returns 1 and clearsvc_escaped, so it takes a core after the decode, and the nestedhos_sleep_nsaway returns 0. This PR's test uses its own stand-in for away/back, so it passes either way.
The only thing left is CI. The three workflows (Game-free source regressions, Linux AppImage, Windows builder) are action_required and need a maintainer to approve the run. Only the PR-title check has run so far. It should be good to merge once they're green, the new test especially.
|
Thanks for the review, and for going through the released window yourself. Understood on CI: I'll watch the three workflows once they are approved and fix anything that comes up red, the new test on Windows first. |
|
@hckrman101 a heads-up, because this one is mine: this pull request's test and #2042 met on main, and the game-free tests don't build there now.
Each of the two passed alone; only the pair fails. It is the test's harness only: I'm preparing a one-file |
|
The fix is up: #2380 (issue #2379). It also covers #2301's |
What this is for: fewer slow frames while a movie plays (the title movie and the intro), and one cause less of threads running without a virtual core (#1929). What is measured: on Linux (RTX 4090, NVIDIA 615.78.08), six runs in turn on a quiet machine: 1% low 33.0 to 34.6 fps on main against 39.7 to 42.5 with the change, frames over 25 ms 28 to 40 against 6 to 12, none over 33.4 ms with it; escapes behind the decoder 17 in 16 runs without the change and none in 6 with it; a test that fails on main. Not measured: Windows (Media Foundation decodes there), AMD, the Steam Deck, the story movies of #2007 and #1919.
The problem
dec_acquire(host/nn_video.c) decodes the pending submissions with libavcodec while the calling guest thread holdsits virtual core; only the 2 ms sleeps between its retries gave the core up. A local diagnostic build that logs every
holder's host stack at each escape (two runs, 16 escapes) found the movie input thread (ムービー入力読込, priority 17)
holding core 5 there, asleep and asked to give the core up:
(game) > dec_acquire > decode_next > avcodec_receive_frame_flags > (libavcodec) > dav1d_get_picture > dav1d_parse_obus > dav1d_submit_frame > pthread_cond_waitIn 8 samples it had held the core 100-270 ms (7 with
dec_acquirein the stack, one in libswscale that could not benamed further); BaseProcExecutor5 escaped after 250.4 ms and AudioFrameworkThread a moment later.
What changed
host/nn_video.cdec_acquire:hos_vcore_away()before it takesd->lock;hos_vcore_back()after each of itsthree
ReleaseSRWLockExclusive(&d->lock), so a core is never waited for while the lock is held (ashos_cv_sleepdoes). In between the call runs host code only: the decoding (libavcodec/dav1d, or Media Foundation on Windows), the
2 ms retries (
hos_sleep_ns, which now has no core to give up), the colour conversion and the copy into the pool.tests/test_vcore_video_acquire.py(new): compiles the actualdec_acquire, with the decoder's types cut from thesame file, with stand-ins that record whether the thread holds its core while it decodes and whether it takes a core
while it holds
d->lock. It checks every way out (a picture handed out, nothing decoded after the end, no free buffer,a decode after three retries) and a thread that holds no core.
How it was tested
Linux (Arch), RTX 4090, NVIDIA 615.78.08, headless, a fresh profile each run. The game was built with this patch on main
07b0359 (since then main has not changed
nn_video.cor the scheduler's waits; it added the FPS counter's levels); themain runs used #2127's commit, whose host code is 07b0359's.
Frame times on a quiet machine (an exclusive hold of the card: no build and no other run beside it; load average at
each start 3.1-5.7), defaults (the 2 s escape), no input: the title and its movie, 90 s per run, A B A B A B. Frames from
the game's own presents (frame limiter included) from 30 s to 90 s after the first present (the presents end about 83 s
in):
The harness's own line for each run, as written (whole run from the first present, start-up included;
low1= 1% low,overN= frames over N ms,nocore= escapes,kernel= GPU driver faults; load before -> after):The slow frames are fewer in every pair, and with the change none is longer than two frames' time. Nothing escaped in
these runs, so the difference is the core itself: by my reading (not measured) the game's threads that share core 5
with the movie input thread no longer wait behind its decoding.
Escapes (the issue's steps: Start about 95 s into the title, 165 s or 115 s per run; escape set to 250 ms so that
waits over 250 ms show as escapes; one sitting, not a quiet machine). "Behind the decoder" = main's own escape line names
the movie input thread, asked, as the holder:
Over all 16 runs of builds without this change (with #2301's and #2302's changes and the diagnostic build), 17 escapes
behind the decoder in 5 runs; with it none in 6. The other escapes are the same kinds in both (a thread waiting for
sead::MainThread at the start; low-priority threads in the scene load after Start). The movie plays the same: the
pictures of one run each (one every 16 s) show the same title movie and the intro after Start.
Checks:
tests/run_all.pyon this branch's head (8b4f6e6, on main 49f75dc):363 passed, 2 failed, 10 skipped in 159 s (linux, 32 at once). The two failures are the ones this machine has on every branch:test_updater_asset_digest.py(only the .NET 10 SDK here, the fixture needs .NET 8) and
test_wayland_libdecor_hint.py(clang 23's-Wunused-but-set-globalunder-Werror). The new test fails on main'snn_video.c(8 checks) and passes on thebranch, with g++ and clang++.
Risks
nn_fs.c'sfile reads already do; nothing here runs guest code.
include that cost.
d->lock(dec_submit,dec_flush) now waits for a thread that holds nocore; the decoding thread needs none to finish and takes one only after letting the lock go, so this cannot deadlock.
Before, the decoding thread held both.
Overlaps
git merge-treeagainst the open pull requests that touchhost/nn_video.c: #2114 and #2042 merge cleanly; #1543 (alocal scene testing API) adds an acquire trace inside
dec_acquirearound the same lock lines and conflicts textuallythere; both sets of lines can stay. #2301 and #2302 (#1929's other two changes) touch other files.
Not tested
Windows (Media Foundation decodes there instead of dav1d: the change covers that path, but its wait was not measured),
AMD, the Steam Deck. Only the title movie and the intro after Start were run, not the story movies of #2007 / #1919.
dec_submitanddec_flushalso taked->lockwith the core held; they were never seen holding a core for long hereand are left as they are.
Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com