Skip to content

fix(libSceNgs2): clear the voice callback on sampler and mixer setup - #7428

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
mrfelek:fix/ngs2-setup-clears-callback
Oct 19, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
mrfelek:fix/ngs2-setup-clears-callback

Conversation

@mrfelek

@mrfelek mrfelek commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

What

A sampler or mixer setup (SCE_NGS2_SAMPLER_VOICE_PARAM_SETUP, mixer setup) now clears the voice callback that SCE_NGS2_VOICE_PARAM_CALLBACK registered. Ngs2Voice::ResetSetup() already resets the rest of the voice: waveform, blocks, ports, matrices, filters and the UserFx handler. Only the voice callback survived the setup, so a callback registered for one use of a voice ran for blocks queued after a later setup.

Metal Gear Solid: Peace Walker (PPSA18216) reuses one sampler voice for two jobs:

  1. It streams music on the voice and registers a block-end callback (flags 0x3). The callback reads a per-block object from the block's user_data.
  2. Later it sends a new sampler setup and queues one-shot ATRAC9 blocks on the same voice, with user_data 0.
  3. On main, the music callback still runs when the one-shot block ends. The game reads user_data + 0x20 (cmpb $1,0x20(%r15) with r15 0) and faults.

That's the crash at New Game in #5268.

The game never clears the callback itself, and after each setup it registers the callback again before it streams music. That only works if a setup clears it. The debt entry records this as unverified on console hardware.

TestSetupClearsCallback in ngs2_render/ngs2_core_render follows the same sequence with PCM blocks:

  1. Register the callback, then set the voice up again.
  2. Queue and play a block with user_data 0. The test expects no callback.
  3. Set the voice up, register the callback again, and play a block with user_data 0x55. The test expects exactly one block-end callback.

The existing OnBlock callback checks user_data == 0x55, so on main the test fails the same way the game does.

Tested

Release builds of this branch (main ccb0c09 plus this commit):

Linux Windows
System Ubuntu 26.10, GCC 15.3.0 Windows 11 Pro (build 26200), MinGW-w64 GCC 15.2.0 (winlibs, UCRT)
All 12 NGS2 tests (ctest -R ngs2) pass pass
ngs2_render, ngs2_core_render repeated (--repeat until-fail:20) 20 of 20 20 of 20
Negative control: main's Voice.cpp both fail with NGS2 check failed at line 25 (OnBlock got user_data 0) same
tools/check_conventions.py, git diff --check pass

PPSA18216, with this change on top of main c9e6c45 plus #6014, #6196, #6237, #6842, #7072 and #7200, and two local graphics workarounds (branch test/ppsa18216-ingame, described in #5268):

callback kept (as on main) callback cleared (this PR)
Music voice set up again while its callback is registered yes 3 times in an 8-minute automated run, 22 times in a manual session
Callback called with user_data 0 1 0
Result SIGSEGV at mgspw.elf + 0x4e8c, from Ngs2FinishBlock / AdvanceAtrac9 no crash; New Game reaches the beach tutorial, speech and music play

The title also reached the tutorial on Windows 11 (laptop RTX A2000) with the same branch.

tools/pr_overlap.py could not compare #1384, #5797, #6447, #6594 and #6680 because they conflict with main. I checked their diffs by hand, and none of them changes the voice callback or ResetSetup.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: PPSA18216 METAL GEAR SOLID: Peace Walker - Master Collection Version (PPSA18216) #5268 (crash at New Game)
  • GPUs tested: n/a
  • AI-assisted: yes

AI-assisted; the runs are from my machines.

@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 1384 core/libs/prx/libSceNgs2.native/src/Voice.cpp
PR 5797 core/libs/prx/libSceNgs2.native/src/Voice.cpp
docs/dev/debt/libSceNgs2.native.md

@wachi35676

Copy link
Copy Markdown
Contributor

Reproduced on Windows 11, WinLibs GCC 15.2.0 (MinGW), Release, this PR applied onto main 3e3e8b905: all 12 ngs2_* tests pass with this PR. With main's Voice.cpp and the new tests, ngs2_render and ngs2_core_render abort.

One side effect isn't covered by a test or by the debt line: a block-end callback that sets its own voice up again and queues the next block now unregisters itself. That is what SetupPcm in TestPcmCallbackSetup does once. In a probe that repeats it three times, main calls the callback 4 times over 8 grains and this PR calls it once. I don't know of a title that streams this way, but the debt entry should say that a setup made inside the callback also clears it. If PPSA18216 never sets the voice up from inside its callback, the entry could instead say this ordering is unverified.

AI-assisted: yes (Claude Code); results above were run on this machine.

@mrfelek

mrfelek commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

@wachi35676 thanks for reproducing it on Windows and for the probe. You're right about that side effect.

I checked whether Peace Walker relies on that ordering. I added a temporary probe that logs every setup made while a voice callback is running, then played from the launcher to the beach tutorial on Linux. The block-end callback ran 3 times, and the game never set a voice up from inside a callback. Every setup that cleared a callback came from outside one, which is the case this PR is about.

So I updated the debt entry in 5e0deae. It now says that a setup made from inside the voice's own callback clears that callback too, so such a callback runs once instead of for every block, and that this ordering is unverified, since PPSA18216 never does it.

AI-assisted; the runs are from my machine(s).

@github-actions github-actions Bot added the unblocks title Names a title ID on the Unblocks line label Oct 18, 2026
@adriadam10 adriadam10 self-assigned this Oct 19, 2026
@adriadam10
adriadam10 merged commit dfd22c1 into boykopovar:main Oct 19, 2026
9 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator
  • ResetSetup() is only called from the sampler and mixer setup paths, so clearing the callback there matches what the PR describes.
  • TestSetupClearsCallback fails on main (a stale callback sees user_data 0), and the debt entry now records the setup-inside-callback side effect that @wachi35676 found.

Checked the diff, the callers and the test; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs prx unblocks title Names a title ID on the Unblocks line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants