MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(vcores): no core held while a movie picture is decoded (#1929) #2322

Merged

mooki000 merged 1 commit from vcore-release-decoder into main

2026-11-11 16:36:29 +00:00

mooki000 commented

2026-11-11 15:00:37 +00:00

(Migrated from github.com)

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 holds
its 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_wait

In 8 samples it had held the core 100-270 ms (7 with dec_acquire in the stack, one in libswscale that could not be
named further); BaseProcExecutor5 escaped after 250.4 ms and AudioFrameworkThread a moment later.

What changed

host/nn_video.c dec_acquire: hos_vcore_away() before it takes d->lock; hos_vcore_back() after each of its
three ReleaseSRWLockExclusive(&d->lock), so a core is never waited for while the lock is held (as hos_cv_sleep
does). 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 actual dec_acquire, with the decoder's types cut from the
same 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
07b03596 (since then main has not changed nn_video.c or the scheduler's waits; it added the FPS counter's levels); the
main runs used #2127's commit, whose host code is 07b03596'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):

run	frames	mean fps	1% low fps	frames over 25 ms	over 33.4 ms	worst frame	load at start
main 1	3160	59.9	33.0	31	8	44 ms	5.7
this change 1	3160	60.0	39.7	12	0	33 ms	4.4
main 2	3219	59.9	33.1	40	6	43 ms	4.7
this change 2	3215	60.0	41.3	11	0	33 ms	3.8
main 3	3232	60.0	34.6	28	6	43 ms	3.1
this change 3	3223	60.0	42.5	6	0	31 ms	3.6

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):

hs-run ep-01 exit=124 wall=90.4s span=82.8s from=0s frames=4947(game) mean=59.8 low1=26.1 over16.7=2139 over20=239 over25=76 shots=41 1280x720 113.4MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 5.69 6.72 26.17 -> 4.40 6.06 24.04
hs-run ebd-01 exit=124 wall=90.3s span=82.7s from=0s frames=4946(game) mean=59.8 low1=32.8 over16.7=2200 over20=115 over25=23 shots=41 1280x720 113.4MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 4.40 6.06 24.04 -> 4.72 5.68 22.24
hs-run ep-02 exit=124 wall=90.3s span=83.8s from=0s frames=5004(game) mean=59.7 low1=26.5 over16.7=2232 over20=238 over25=80 shots=41 1280x720 113.4MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 4.72 5.68 22.24 -> 3.76 5.07 20.49
hs-run ebd-02 exit=124 wall=90.3s span=83.6s from=0s frames=5004(game) mean=59.8 low1=34.1 over16.7=2236 over20=119 over25=18 shots=42 1280x720 116.2MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 3.76 5.07 20.49 -> 3.14 4.53 18.87
hs-run ep-03 exit=124 wall=90.3s span=83.9s from=0s frames=4883(game) mean=58.2 low1=12.7 over16.7=2173 over20=261 over25=82 shots=40 1280x720 110.6MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 3.14 4.53 18.87 -> 3.57 4.42 17.51
hs-run ebd-03 exit=124 wall=90.3s span=83.7s from=0s frames=5010(game) mean=59.8 low1=34.4 over16.7=2178 over20=110 over25=16 shots=42 1280x720 116.2MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 3.57 4.42 17.51 -> 2.48 3.88 16.11

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:

	runs	escapes per run	behind the decoder	crashes
main's code	7	7, 8, 2, 2, 8, 3, 5	4, 0, 0, 0, 0, 0, 0	0
this change	6	7, 4, 3, 8, 5, 6	0 in all	0

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.py on this branch's head (8b4f6e60, on main 49f75dc8): 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-global under -Werror). The new test fails on main's nn_video.c (8 checks) and passes on the
branch, with g++ and clang++.

Risks

Guest memory is now written without a core (the sync handle, the copy into the pool's guest memory), as nn_fs.c's
file reads already do; nothing here runs guest code.
One more give-up and take of a core per acquired picture (about one a frame while a movie plays). The frame times above
include that cost.
A thread that holds a core and waits for d->lock (dec_submit, dec_flush) now waits for a thread that holds no
core; 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-tree against the open pull requests that touch host/nn_video.c: #2114 and #2042 merge cleanly; #1543 (a
local scene testing API) adds an acquire trace inside dec_acquire around the same lock lines and conflicts textually
there; 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_submit and dec_flush also take d->lock with the core held; they were never seen holding a core for long here
and are left as they are.

Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 [email protected]

**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 holds its 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_wait` In 8 samples it had held the core 100-270 ms (7 with `dec_acquire` in the stack, one in libswscale that could not be named further); BaseProcExecutor5 escaped after 250.4 ms and AudioFrameworkThread a moment later. ## What changed - `host/nn_video.c` `dec_acquire`: `hos_vcore_away()` before it takes `d->lock`; `hos_vcore_back()` after each of its three `ReleaseSRWLockExclusive(&d->lock)`, so a core is never waited for while the lock is held (as `hos_cv_sleep` does). 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 actual `dec_acquire`, with the decoder's types cut from the same 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 07b03596 (since then main has not changed `nn_video.c` or the scheduler's waits; it added the FPS counter's levels); the main runs used #2127's commit, whose host code is 07b03596'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): | run | frames | mean fps | 1% low fps | frames over 25 ms | over 33.4 ms | worst frame | load at start | |---|---:|---:|---:|---:|---:|---:|---:| | main 1 | 3160 | 59.9 | 33.0 | 31 | 8 | 44 ms | 5.7 | | this change 1 | 3160 | 60.0 | 39.7 | 12 | 0 | 33 ms | 4.4 | | main 2 | 3219 | 59.9 | 33.1 | 40 | 6 | 43 ms | 4.7 | | this change 2 | 3215 | 60.0 | 41.3 | 11 | 0 | 33 ms | 3.8 | | main 3 | 3232 | 60.0 | 34.6 | 28 | 6 | 43 ms | 3.1 | | this change 3 | 3223 | 60.0 | 42.5 | 6 | 0 | 31 ms | 3.6 | 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): ``` hs-run ep-01 exit=124 wall=90.4s span=82.8s from=0s frames=4947(game) mean=59.8 low1=26.1 over16.7=2139 over20=239 over25=76 shots=41 1280x720 113.4MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 5.69 6.72 26.17 -> 4.40 6.06 24.04 hs-run ebd-01 exit=124 wall=90.3s span=82.7s from=0s frames=4946(game) mean=59.8 low1=32.8 over16.7=2200 over20=115 over25=23 shots=41 1280x720 113.4MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 4.40 6.06 24.04 -> 4.72 5.68 22.24 hs-run ep-02 exit=124 wall=90.3s span=83.8s from=0s frames=5004(game) mean=59.7 low1=26.5 over16.7=2232 over20=238 over25=80 shots=41 1280x720 113.4MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 4.72 5.68 22.24 -> 3.76 5.07 20.49 hs-run ebd-02 exit=124 wall=90.3s span=83.6s from=0s frames=5004(game) mean=59.8 low1=34.1 over16.7=2236 over20=119 over25=18 shots=42 1280x720 116.2MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 3.76 5.07 20.49 -> 3.14 4.53 18.87 hs-run ep-03 exit=124 wall=90.3s span=83.9s from=0s frames=4883(game) mean=58.2 low1=12.7 over16.7=2173 over20=261 over25=82 shots=40 1280x720 110.6MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 3.14 4.53 18.87 -> 3.57 4.42 17.51 hs-run ebd-03 exit=124 wall=90.3s span=83.7s from=0s frames=5010(game) mean=59.8 low1=34.4 over16.7=2178 over20=110 over25=16 shots=42 1280x720 116.2MB fatal=0 crash=0 devlost=0 nocore=0 kernel=0 nvrm=1 dev=0:NVIDIA_GeForce_RTX_4090 load 3.57 4.42 17.51 -> 2.48 3.88 16.11 ``` 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: | | runs | escapes per run | behind the decoder | crashes | |---|---:|---|---|---:| | main's code | 7 | 7, 8, 2, 2, 8, 3, 5 | 4, 0, 0, 0, 0, 0, 0 | 0 | | this change | 6 | 7, 4, 3, 8, 5, 6 | 0 in all | 0 | 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.py` on this branch's head (8b4f6e60, on main 49f75dc8): `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-global` under `-Werror`). The new test fails on main's `nn_video.c` (8 checks) and passes on the branch, with g++ and clang++. ## Risks - Guest memory is now written without a core (the sync handle, the copy into the pool's guest memory), as `nn_fs.c`'s file reads already do; nothing here runs guest code. - One more give-up and take of a core per acquired picture (about one a frame while a movie plays). The frame times above include that cost. - A thread that holds a core and waits for `d->lock` (`dec_submit`, `dec_flush`) now waits for a thread that holds no core; 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-tree` against the open pull requests that touch `host/nn_video.c`: #2114 and #2042 merge cleanly; #1543 (a local scene testing API) adds an acquire trace inside `dec_acquire` around the same lock lines and conflicts textually there; 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_submit` and `dec_flush` also take `d->lock` with the core held; they were never seen holding a core for long here and are left as they are. Assisted-by: Claude Opus 5.5 Co-Authored-By: Claude Opus 5.5 <[email protected]>

CIB commented

2026-11-11 15:34:18 +00:00

(Migrated from github.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 vc_kick is never set; that one needs a scheduler condition, not a decoder away/back. But the movie decoder is exactly the “no checkpoint in host code” case this PR targets, and your 17→0 decoder escapes and 1% low 33→40 fps result are consistent with removing a 100–270 ms hold.

Placement: away before d->lock and back after each ReleaseSRWLockExclusive(&d->lock) is the right order — never wait for a core while holding d->lock. Please also confirm no guest callback runs inside the released window.

Scope: this does not cover the two remaining non-polling holders we still see: OpusDecoder (audio) and the nn.npln.Worker/nn.npln.Shared path. Those need the same pattern separately. We also found a guest-object lock held across a core in event_signal (e->lk), fixed locally with a try-lock; the other signal paths are worth auditing for the same shape.

Our local arms are 250 ms on an older main; your branch has #2127’s 2 s default, so the escape counts should be compared after re-baselining.

🤖 Generated with opencode using DeepSeek V4.1 Flash.

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 `vc_kick` is never set; that one needs a scheduler condition, not a decoder `away`/`back`. But the movie decoder is exactly the “no checkpoint in host code” case this PR targets, and your 17→0 decoder escapes and 1% low 33→40 fps result are consistent with removing a 100–270 ms hold. **Placement:** `away` before `d->lock` and `back` after each `ReleaseSRWLockExclusive(&d->lock)` is the right order — never wait for a core while holding `d->lock`. Please also confirm no guest callback runs inside the released window. **Scope:** this does not cover the two remaining non-polling holders we still see: `OpusDecoder` (audio) and the `nn.npln.Worker`/`nn.npln.Shared` path. Those need the same pattern separately. We also found a guest-object lock held across a core in `event_signal` (`e->lk`), fixed locally with a try-lock; the other signal paths are worth auditing for the same shape. Our local arms are 250 ms on an older `main`; your branch has #2127’s 2 s default, so the escape counts should be compared after re-baselining. --- 🤖 Generated with [opencode](https://opencode.ai) using DeepSeek V4.1 Flash.

mooki000 commented

2026-11-11 16:19:46 +00:00

(Migrated from github.com)

Thanks. The check you asked for, from the branch head (8b4f6e60), host/nn_video.c, dec_acquire (line 579):

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 ReleaseSRWLockExclusive(&d->lock). I read every call inside it:

decode_pending and decode_next (406): avcodec_send_packet / avcodec_receive_frame (dav1d), or on Windows mf_av1_decode with mf_picture as its callback. That callback is our own C (queue_frame, 341), not the game's. We set no libavcodec callback (only thread_count, 291).
the retries (592-597): the lock is released, hos_sleep_ns(2000000), the lock is taken, decode_pending again. hos_sleep_ns has its own away/back pair, which does nothing here because no core is held.
host_of (a table lookup under a host lock), the catch-up loop (decode_next, picture_free), the copies into the pool's host buffers, pool_rgba (swscale), hos_log.

None of these is handed the guest context: in this file only the entry points take it (dec_init, dec_submit, dec_acquire and the others), and none of them is called from here. The file signals guest events only through hos_system_event_signal, at line 511 (submitted, from the submit calls) and line 738 (ready_event); neither is on this path.

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 HOS_WRITTEN setting the dirty-page marks). Both are plain stores by host code into buffers the caller handed over. Main already does the same in read_at (nn_fs.c on 49f75dc8: the core is given up at line 392 and ReadFile writes the guest buffer at 399). The buffer index (703) and the result are written after the core is back.

Every way out after 589 takes the core back (603, 619, 702); the one return before it (582, no decoder) never gave it up. tests/test_vcore_video_acquire.py compiles the real function and checks a decoded picture, the end of the stream, no free buffer, a decode after three retries, and a thread that held no core, each for "decoded without the core" and "the core taken once, after d->lock is let go".

Scope: agreed, this is the decoder only. In our 6 runs with this change at 250 ms (33 escapes), none stood behind the decoder. nn.npln.Shared and nn.npln.Worker were the holders at 2 escapes of one run, OpusDecoder at none, ParallelDecompThread at 4. More about those three on #1929.

Comparing counts: our arms are at 250 ms too. The runs set HOS_VCORE_ESCAPE_US=250000 HOS_VCORE_ESCAPE_EQ_US=250000 in the environment, on builds of 880cf67f (main's code) and 07b03596 plus this patch. 880cf67f differs from the main before #2127 (364d1994) only in the two defaults that this switch overrides, so 250 ms arms on that older main compare with ours directly. The branch is the same patch on 49f75dc8 (I compared the hunks).

Thanks. The check you asked for, from the branch head (8b4f6e60), `host/nn_video.c`, `dec_acquire` (line 579): **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 `ReleaseSRWLockExclusive(&d->lock)`. I read every call inside it: - `decode_pending` and `decode_next` (406): `avcodec_send_packet` / `avcodec_receive_frame` (dav1d), or on Windows `mf_av1_decode` with `mf_picture` as its callback. That callback is our own C (`queue_frame`, 341), not the game's. We set no libavcodec callback (only `thread_count`, 291). - the retries (592-597): the lock is released, `hos_sleep_ns(2000000)`, the lock is taken, `decode_pending` again. `hos_sleep_ns` has its own away/back pair, which does nothing here because no core is held. - `host_of` (a table lookup under a host lock), the catch-up loop (`decode_next`, `picture_free`), the copies into the pool's host buffers, `pool_rgba` (swscale), `hos_log`. None of these is handed the guest context: in this file only the entry points take it (`dec_init`, `dec_submit`, `dec_acquire` and the others), and none of them is called from here. The file signals guest events only through `hos_system_event_signal`, at line 511 (`submitted`, from the submit calls) and line 738 (`ready_event`); neither is on this path. **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 `HOS_WRITTEN` setting the dirty-page marks). Both are plain stores by host code into buffers the caller handed over. Main already does the same in `read_at` (`nn_fs.c` on 49f75dc8: the core is given up at line 392 and `ReadFile` writes the guest buffer at 399). The buffer index (703) and the result are written after the core is back. Every way out after 589 takes the core back (603, 619, 702); the one return before it (582, no decoder) never gave it up. `tests/test_vcore_video_acquire.py` compiles the real function and checks a decoded picture, the end of the stream, no free buffer, a decode after three retries, and a thread that held no core, each for "decoded without the core" and "the core taken once, after `d->lock` is let go". **Scope: agreed, this is the decoder only.** In our 6 runs with this change at 250 ms (33 escapes), none stood behind the decoder. `nn.npln.Shared` and `nn.npln.Worker` were the holders at 2 escapes of one run, `OpusDecoder` at none, `ParallelDecompThread` at 4. More about those three on #1929. **Comparing counts:** our arms are at 250 ms too. The runs set `HOS_VCORE_ESCAPE_US=250000 HOS_VCORE_ESCAPE_EQ_US=250000` in the environment, on builds of 880cf67f (main's code) and 07b03596 plus this patch. 880cf67f differs from the main before #2127 (364d1994) only in the two defaults that this switch overrides, so 250 ms arms on that older main compare with ours directly. The branch is the same patch on 49f75dc8 (I compared the hunks).

hckrman101 (Migrated from github.com) approved these changes

2026-11-11 16:28:50 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good to me.

The placement is right: hos_vcore_away() before d->lock, and hos_vcore_back() only after each of the three ReleaseSRWLockExclusive, so a core is never waited for under the lock (same shape as hos_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_back are already declared in hos.h, so nothing new is needed for the Windows build.
The nested hos_sleep_ns in 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_of and hos_log, and no guest code. The two guest-memory stores (the idle sync value and the pool copy plus HOS_WRITTEN) go into buffers the caller handed over, the same as read_at in nn_fs.c on main.
dec_submit/dec_flush still wait for d->lock while 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 outer hos_vcore_away() here returns 1 and clears vc_escaped, so it takes a core after the decode, and the nested hos_sleep_ns away 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.

Looks good to me. - The placement is right: `hos_vcore_away()` before `d->lock`, and `hos_vcore_back()` only after each of the three `ReleaseSRWLockExclusive`, so a core is never waited for under the lock (same shape as `hos_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_back` are already declared in `hos.h`, so nothing new is needed for the Windows build. - The nested `hos_sleep_ns` in 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_of` and `hos_log`, and no guest code. The two guest-memory stores (the idle sync value and the pool copy plus `HOS_WRITTEN`) go into buffers the caller handed over, the same as `read_at` in `nn_fs.c` on main. - `dec_submit`/`dec_flush` still wait for `d->lock` while 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 outer `hos_vcore_away()` here returns 1 and clears `vc_escaped`, so it takes a core after the decode, and the nested `hos_sleep_ns` away 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.

mooki000 commented

2026-11-11 16:31:04 +00:00

(Migrated from github.com)

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.

mooki000 commented

2026-11-11 16:38:57 +00:00

(Migrated from github.com)

@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.

tests/test_vcore_video_acquire.py cuts dec_acquire out of host/nn_video.c and compiles it in a small harness. #2042 changed one line inside that function from getenv("HOS_VIDEO_NO_CATCHUP") != NULL to env_on("HOS_VIDEO_NO_CATCHUP"), and my harness has no env_on. On main at f9286c45, with clang:

fixture.cpp:159:28: error: use of undeclared identifier 'env_on'
  159 |         if (off < 0) off = env_on("HOS_VIDEO_NO_CATCHUP");

Each of the two passed alone; only the pair fails. It is the test's harness only: host/nn_video.c itself includes hookshot_env.h.

I'm preparing a one-file test: fix that pastes hookshot_env.h ahead of the cut-out code, the way #2042's own tests do, and will link it here as soon as it has passed with both compilers.

@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. `tests/test_vcore_video_acquire.py` cuts `dec_acquire` out of `host/nn_video.c` and compiles it in a small harness. #2042 changed one line inside that function from `getenv("HOS_VIDEO_NO_CATCHUP") != NULL` to `env_on("HOS_VIDEO_NO_CATCHUP")`, and my harness has no `env_on`. On main at `f9286c45`, with clang: ``` fixture.cpp:159:28: error: use of undeclared identifier 'env_on' 159 | if (off < 0) off = env_on("HOS_VIDEO_NO_CATCHUP"); ``` Each of the two passed alone; only the pair fails. It is the test's harness only: `host/nn_video.c` itself includes `hookshot_env.h`. I'm preparing a one-file `test:` fix that pastes `hookshot_env.h` ahead of the cut-out code, the way #2042's own tests do, and will link it here as soon as it has passed with both compilers.

mooki000 commented

2026-11-11 16:44:35 +00:00

(Migrated from github.com)

The fix is up: #2380 (issue #2379). It also covers #2301's tests/test_vcore_fs_open.py, which broke the same way (OpenFile's HOS_NO_MOVIES read became env_on too). Both tests pass with g++ and clang++ on the branch and fail to compile on main without it; each still fails against the source from before the fix it guards.

The fix is up: #2380 (issue #2379). It also covers #2301's `tests/test_vcore_fs_open.py`, which broke the same way (OpenFile's `HOS_NO_MOVIES` read became `env_on` too). Both tests pass with g++ and clang++ on the branch and fail to compile on main without it; each still fails against the source from before the fix it guards.