Skip to content

feat(ngs2): route custom submixer buffers through Gain - #10851

Merged
oneandonlydean merged 7 commits into
boykopovar:mainfrom
Felipebmaitan:feat/ngs2-audio-engine
Oct 26, 2026
Merged

oneandonlydean merged 7 commits into
boykopovar:mainfrom
Felipebmaitan:feat/ngs2-audio-engine

Conversation

@Felipebmaitan

@Felipebmaitan Felipebmaitan commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

What

Custom submixers currently use one sample buffer for inputs, output ports and Gain. Add 1-4 per-voice buffers for custom submixers with zero modules or one Gain stage: inputs feed matching buffers, each grain clears unwritten samples, ports select their configured source, and Gain overwrites its selected destination. Broader chains and module routing stay unsupported until their processing-order contract is established.

Correct Gain's 37 unclamped levels, previous-level i/N ramps, equal-gain fast path, steady zero clearing and steady unity copying. Invalid Gain positions return 0x804A835A and stop linked parameter processing. Contracts follow the PS5 12.70 module report and later findings; KytyPS5 corroborates port selection. No new console measurement is claimed.

Unverified Gain initialization/lifecycle, callback setup/route changes and nonfinite integer rendering retain their runtime refusals and debt entries. Sampler Gain, broader custom chains and channel conversion remain outside this milestone. Tests exercise supported operations rather than requiring unimplemented cases to keep throwing.

Dean's requested follow-up is implemented at c40ba98945e845b950ecca7819c472942757ad30. Merged main 9b520147fbeddff4357d6e8f92e5235a16342171 in 0c3f104067239abd5358b8d1992295f9da7b27ee; then removed the unsupported exception assertions. CustomSafety keeps ordinary custom/upstream callbacks, explicitly configured Gain, route changes before rendering, callback counts, port metadata and literal output checks in both libraries. Gain keeps finite integer controls and its float/numeric/ramp coverage. Ngs2Custom's supported coverage is retained.

The earlier proposed queue's #9926, #10127 and #10410 are now in main, including #10410's PCM-fixture lifetime repair. The merge preserves their PLAY/list-error propagation, PCM widths/accounting, mastering gain and all tests alongside this PR's Gain early errors and callback safeguards. No open prerequisite is required.

#11352 independently proposes the same Gain-ramp correction already included here; apply that correction once and preserve the combined coverage whichever PR lands first. Custom Reverb #10599, experimental PCM retention #5548, and newer custom-module/channel/codec work remain separate. Their later compositions must preserve each topic's scope, state ownership and test registrations and be validated at their actual heads. The refreshed overlap report and independent reviews record current pinned references; no sibling branch was edited or combined-runtime result claimed.

Tested

Windows 11 Pro 10.0.26200; WinLibs MinGW-w64 GCC 15.2.0 r7; Ninja Release, BUILD_TESTING=ON, SPIRV-Tools enabled. Latest exact head: c40ba98945e845b950ecca7819c472942757ad30, main ancestor 9b520147fbeddff4357d6e8f92e5235a16342171.

  • Reconfigured the existing isolated full-build tree and rebuilt both production NGS2 library targets plus all 22 focused test executables with cmake --build build --target <both libraries and NGS2 test targets> --parallel 2.
  • ctest --test-dir build -R '^ngs2_' --output-on-failure --parallel 2: 30/30 passed, zero skips, 2.95 seconds. This includes all ten supported CustomSafety cases, Gain/Routing against both libraries, native/core PCM and native voice events.
  • Retained literal/rational expectations for previous-level ramps, target replacement, channel/voice independence, pause/resume, grain changes, levels outside +/-32, float nonfinite classifications, finite integer output, invalid module positions/list termination, all supported channel layouts, 1-4 buffers, overwrite/aliasing, per-grain clearing and separate-buffer unity copying.
  • python -B tools/check_conventions.py --base origin/main and exact-base whitespace checks pass. python -B core/libs/tests/test_public_headers.py --audit-only: 368 libraries/12,231 exports verified; this is a declaration audit, not a compiled-header run.
  • Fresh independent GPT-6.1-Sol/xhigh macro and micro reviews at this exact head found no actionable candidate defect and confirmed this fulfills Dean's requested scope. They performed source/artifact checks; the exact-head runtime results above are parent-run.

The former unsupported-refusal tests and their historical mutation results are not retained-test evidence. No new title, GPU, original-module or PS5 run was performed for this follow-up. The earlier a43a610 production smoke reached a capped Dead Cells loading screen; it establishes no latest-head title result, audio fidelity, callback timing, GPU validation or performance. Full-suite/hosted runtime checks and unselected external compositions are not claimed.

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: none
  • GPUs tested: n/a
  • AI-assisted: yes

@Felipebmaitan

Copy link
Copy Markdown
Contributor Author

Owner coordination for this standalone audio-routing change, currently a43a610bbf8a8c5b9e7499dd2631247a8597a551 on main e7e3e9c1984c293f06bff08428b8c89b7b932ac2. It has no prerequisite PR and remains draft. I own the new buffers, Gain changes and callback safeguards here; I have not edited any sibling branch.

@JJenkx (#5548), @JAB-dev (#9926/#10410), @kyon-eth (#10599): the overlap scan identifies shared test registrations, event/control propagation, the integer-output guard and custom-module setup/processing. Could you confirm ownership and landing order? Maintainers can choose an order if the later branch preserves every topic's behavior and tests. I will validate whichever composition is selected; none has been built or claimed green yet.

Two independent adversarial reviews and my source inspection confirmed these issues at the pinned counterpart heads:

Could the respective owners take those repairs, or confirm who should own them, before we attempt shared edits? This is a static-source report, not a console measurement. The other audited heads were #5548 ddf93fbe14b4f036131e5c870a43874414414597, #9926 2375223a92690cfb798e4c9906dbc8eb5493e71b, #1384 70b4d2de4b4e9ed4c27259f4ef75f0e9f7b366e0, #10249 a4d01ff96ffaf96a9b7afa45f05cecaffbb0e4f9 and #10127 749f6cfa6586f6a515dcad24b60f20d2448423f2; none contains this branch's route/layout repair. That absence does not imply each standalone branch supports the new two-buffer reproducer.

On this branch, all four native/core route-mutation regressions fail at 2da1332bff0194443e91c9ab11a644cf6b4f58d8 and pass after the explicit refusal. The 27-test NGS2 suite passes. A further test-only tightening now distinguishes each safety refusal and adds nonfinite integer-output coverage; it will be pushed after validation and fresh review. No complete merged composition or PS5 fidelity is claimed.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the exact-head follow-up e50cbf349d4e0af8647627640df9c8c9ddf610b7 against a43a610bbf8a8c5b9e7499dd2631247a8597a551. This follow-up is test-only: callback safety cases now require the exact refusal and a single callback invocation, and the integer-output case checks finite positive/negative gain plus nonfinite rejection with output sentinels unchanged. I found no issue in these added assertions. I did not build or run them locally; the 27/27 NGS2 results in the description are author-reported. This is not a full review or approval of the composed change: the PR remains draft while overlap owners coordinate, and the body says no combined branch has been validated.

@Felipebmaitan

Copy link
Copy Markdown
Contributor Author

Recommended landing queue, after refreshing current PR metadata and inspecting read-only source merge conflicts on 2026-10-25:

#9926 → #10127 → #10851 → repaired #10410 → repaired #10599. Defer draft #5548 until its own evidence gaps are resolved.

This replaces the open-ended landing-order question in my earlier coordination comment. It is my recommendation, not an agreement from other authors or a claim that any counterpart is ready to merge. This PR has no prerequisite and is being marked ready for standalone review; if an earlier item is unavailable, it can land independently. I accept ownership of rebasing #10851 after earlier changes and preserving their behavior/tests. No sibling branch was edited.

Why this order:

Pinned inputs (main is still e7e3e9c1984c293f06bff08428b8c89b7b932ac2; no declared prerequisites):

PR Exact inspected head Source combination with #10851
#10851 e50cbf349d4e0af8647627640df9c8c9ddf610b7 Clean against current main; reviewed implementation unchanged
#9926 2375223a92690cfb798e4c9906dbc8eb5493e71b Voice.cpp and native test list conflict; retain both Gain return/list termination and Ngs2ReturnCode event propagation
#10127 749f6cfa6586f6a515dcad24b60f20d2448423f2 Source merge clean; still requires combined runtime tests if composed
#10410 182e2a227f47286536d9852340badc8e088e5581 Render.cpp integer-output guard conflict; preserve the stronger guard and distinct mastering behavior
#10599 b24bea85547585f4350f1b59fc5a5a817ecbd13d Custom.cpp, native test list and debt conflict; preserve both setup invalidation/revision and Reverb reset, dispatch and state
#5548 ddf93fbe14b4f036131e5c870a43874414414597 Both NGS2 test lists conflict; preserve every registration

I rechecked the two sibling defects at these unchanged heads: #10410 borrowed-PCM fixture, and #10599 position mask. @JAB-dev and @kyon-eth own those repairs unless they coordinate otherwise. @JJenkx: retention is not a prerequisite here. Owners/maintainers may choose a different queue; the later branch must preserve each topic and all registrations, then build both NGS2 libraries and run the union of their focused tests.

#1384 at 70b4d2de4b4e9ed4c27259f4ef75f0e9f7b366e0 remains a conflicting experimental draft, excluded from this queue; preserve current main's reference-based Reverb if revived. #10249 at a4d01ff96ffaf96a9b7afa45f05cecaffbb0e4f9 has no NGS2 source conflict with #10851 and no audio dependency; its merge check reports unrelated AGC/JobManager conflicts, and the maintainer has requested scope/regression work. Parser PRs #9148 (8accc665b121f428380c0545578bb8d2a94ebabc), #9149 (ea64374d3a35da14ee6259f7a8b9c6b7155fa43d) and #9324 (7644d6c2b4649f7f7c5b7b53e8faa71616632ab0) share only debt text with this patch and do not impose routing dependencies.

Validation for this documentation/readiness update: current heads/state/comments were refreshed; git merge-tree --write-tree --name-only --no-messages <headA> <headB> inspected the above source combinations without checking out or modifying another branch. No combined build/runtime result is claimed. No source, head SHA, test results, console evidence or prerequisite changed. No new tests were run because this update changes only PR metadata. AI-assisted: yes.

@Felipebmaitan
Felipebmaitan marked this pull request as ready for review October 25, 2026 05:31
@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 5548 core/libs/prx/libSceNgs2.native/CMakeLists.txt L5
core/libs/prx/libSceNgs2/CMakeLists.txt L5-13
PR 9149 docs/dev/debt/libSceNgs2.native.md L49-50
PR 10599 core/libs/prx/libSceNgs2.native/CMakeLists.txt L5
core/libs/prx/libSceNgs2.native/src/Custom.cpp L95-97, L166, L245-250
docs/dev/debt/libSceNgs2.native.md L32-33, L35
PR 11347 core/libs/prx/libSceNgs2.native/src/Custom.cpp L166
docs/dev/debt/libSceNgs2.native.md L28-29, L32-35
PR 11352 core/libs/prx/libSceNgs2.native/src/Custom.cpp L199-204
docs/dev/debt/libSceNgs2.native.md L32-33
PR 11354 core/libs/prx/libSceNgs2.native/src/Ngs2Internal.hpp L149
docs/dev/debt/libSceNgs2.native.md L32-33
PR 11388 core/libs/prx/libSceNgs2.native/CMakeLists.txt L5

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/CMakeLists.txt
core/libs/prx/libSceNgs2.native/include/Ngs2Types.hpp
core/libs/prx/libSceNgs2.native/src/Ngs2Internal.hpp
core/libs/prx/libSceNgs2.native/src/Render.cpp
core/libs/prx/libSceNgs2.native/src/Voice.cpp
core/libs/prx/libSceNgs2/CMakeLists.txt
PR 10249 core/libs/prx/libSceNgs2.native/src/Custom.cpp
PR 11315 core/libs/prx/libSceNgs2.native/CMakeLists.txt
core/libs/prx/libSceNgs2.native/include/Ngs2Types.hpp
core/libs/prx/libSceNgs2.native/src/Custom.cpp
core/libs/prx/libSceNgs2.native/src/Ngs2Internal.hpp
core/libs/prx/libSceNgs2.native/src/Render.cpp
core/libs/prx/libSceNgs2.native/src/Voice.cpp
core/libs/prx/libSceNgs2.native/tests/Ngs2Custom.cpp
docs/dev/debt/libSceNgs2.native.md

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@Felipebmaitan The custom submixer buffers and the corrected Gain stage read well. One test change:

Several cases the console accepts are left as NotImplemented in the code, which is a fine placeholder, but the tests assert that they throw, with the exact messages:

  • gain processing before the first parameter set, or after setup/stop;
  • patch changes during source rendering;
  • integer rendering of non-finite samples;
  • voice setup changes during custom processing.

Each of those assertions would fail as soon as the case is implemented. Could you remove them (keep the NotImplemented in the code) and keep the rest of the Ngs2Custom, Ngs2CustomSafety and Ngs2Gain coverage? The branch also needs a merge of main (it currently conflicts).

@Felipebmaitan

Copy link
Copy Markdown
Contributor Author

@oneandonlydean Addressed in c40ba98945e845b950ecca7819c472942757ad30.

  • Removed the assertions requiring unimplemented Gain lifecycle, callback setup/route mutation and nonfinite integer rendering to throw, including the exact-message matching. The runtime guards and debt entries remain.
  • Retained the rest of Ngs2Custom and Ngs2Gain, including finite integer output. Ngs2CustomSafety now checks supported custom/upstream signal callbacks, explicitly configured Gain, callback counts, port metadata and literal output. Its route changes happen before rendering; the block-end callback only counts invocations. All five supported cases run against each library.
  • Merged main 9b520147fbeddff4357d6e8f92e5235a16342171 in 0c3f104067239abd5358b8d1992295f9da7b27ee. The resolution preserves main's PCM formats/accounting, PLAY/list errors, empty-setup behavior, mastering-gain/PCM-fixture repair, pan changes and every test registration alongside this PR's Gain errors and safeguards.

Reconfigured and rebuilt both production NGS2 variants plus all 22 focused executables on Windows/WinLibs GCC 15.2.0, with build concurrency 2. At exact head c40ba98945:

ctest --test-dir build -R '^ngs2_' --output-on-failure --parallel 2
30/30 passed, zero skips, 2.95 seconds

Conventions and whitespace checks pass. The declaration audit passes for 368 libraries/12,231 exports. No new title, GPU, PS5 or original-module result is claimed. Historical refusal-mutation results are not evidence for the revised tests.

Both fresh independent macro and micro reviews found no actionable candidate defect and confirmed the follow-up fulfills your request. Their source/artifact audits are separate from my exact-head build and 30/30 runtime execution above. This PR remains ready for standalone review; no sibling-owner agreement is a prerequisite.

Independent review pins and integration notes

Both reviewers use GPT-6.1-Sol/xhigh with independent isolated worktrees. They are read-only; the parent owns all edits and the exact-head build/test result above. Reviewed base: 9b520147fbeddff4357d6e8f92e5235a16342171; head: c40ba98945e845b950ecca7819c472942757ad30; prerequisites: none. The binary patch artifact SHA256 is fe4f9cbd57f716ff035b473636636736b6af5cbcff8b9e4ff86f9f7d0c56bd0a.

  • Macro task: node:delegated-task:command%3Amcp%3Aaf0a8e9b-fd5a-4833-b3ae-e1558c384efd%3Adelegate-task%3Angs2-dean-macro-c40ba98945-r1
  • Micro task: node:delegated-task:command%3Amcp%3Aaf0a8e9b-fd5a-4833-b3ae-e1558c384efd%3Adelegate-task%3Angs2-dean-micro-c40ba98945-r1

The previous queue's #9926, #10127 and #10410 are now merged into main. Newer #11352 at 66bca2376f64b8846397063c646f4f1c57673730 duplicates the Gain-ramp correction already present here: @edubox, either order should apply that correction once and preserve coverage; it is not a prerequisite of this PR. The refreshed overlap scan also covers #5548, #10599, #11315, #11347, #11354 and #11388. Existing conflicted #1384/#10249 comparisons remain manual integration work. No sibling checkout/branch was changed and no external composition is claimed tested.

The micro reviewer reverified two separate sibling parameter-index issues: #10599 at 54ac175ee309d0a7a517d7bd3524413e6ad8383c still extracts five bits for Reverb, and #11315 at 316f48500a7193fb89aae6d3d9b2c260df55b409 does so for immediate/deferred PitchShift. In each, position 0x20 aliases module zero and its positive fixture relies on the alias. @kyon-eth owns those branches: validate the full position before mutation/enqueueing, return 0x804A835A when invalid, preserve state and stop the linked parameter list, and use valid positions in positive fixtures. These are defined error-contract checks, separate from the unimplemented-path assertions removed here. Neither is included in or blocks standalone #10851.

The latest observed main 7704178631f3cb40e9d9c0efa168209f53cd27c3 source-merges cleanly with this head and changes no NGS2 files from the merged base. The exact runtime result above remains for c40ba98945, not a claimed build of that newer composition. Console initialization/numerical equivalence, callback timing, blocking-callback progress limits and broader custom chains remain the documented gaps.

AI-assisted: yes.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 26, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

@Felipebmaitan Thanks, removing those assertions while keeping the guards is what was asked. We'll review the update and the main merge, then run CI on this head.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants