Repository navigation
fix(libSceNgs2): buffer sampler filter params sent during processing - #14242
Conversation
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — conflicts with current main; needs a rebase before merge
This PR buffers sampler filter parameters that the game sends while a voice is processing. The stack check reports a merge conflict against the current base, because main has changed the same libSceNgs2.native files since this PR's base. The code was read in part, and no hosted test has run on this head. The conflict has to be resolved first.
What I ran
- Head
c31a358b1c0221205f7eba41a9aa73b32eb71ea7; baseb5705afa47849dd56ef268929edab06352e4f078. 2 files, +106/-28:libSceNgs2.native/src/Voice.cpp(the filter parameter checks and the PS4 and PS5 parameter layouts) andlibSceNgs2.native/tests/Ngs2JobBuffer.cpp(the test). - Read the production diff in part: the new
CheckFilterandSamplerFilterParamfunctions, and the branch that handlesSCE_NGS2_SAMPLER_VOICE_PARAM_FILTER. Did not read the buffering logic that sends the parameters during processing, or the test. - Stack check:
PR #14242: conflict against current base(current main4569fe654a). Main changed sevenlibSceNgs2.nativefiles (646 insertions) after this PR's base. Individual previews: conflict. No declared dependencies. - Hosted: the check runs for this head are conventions, intake and labels only. Windows and the relinkers did not run.
- Read-only checks from the trusted main checkout: diff check and conventions passed. The stack check exited 1 (the conflict). ABI audit passed with warnings: 22
public-declaration-not-foundfindings (for exampleCountBlockEnd,RaceComplete,RunPeerAction,SetupPeer), not compared with the base. - No MarcZX10 review at this head.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | conventions check from the main checkout |
| Mergeability | conflict | the stack check reports a conflict against current main; main has changed both of this PR's files (Voice.cpp and Ngs2JobBuffer.cpp) since the base |
| Filter parameter checks (read in part) | passed by reading | sizes, the filter index, the type, and the location checks follow the PS4 and PS5 layouts as written |
| Buffering during processing (not read) | not verified | the logic that holds parameters while a voice processes was not read |
The test Ngs2JobBuffer |
not run | no hosted run on this head |
| ABI audit (main-checkout tool) | passed with warnings | 22 public-declaration-not-found findings; whether they predate this PR was not checked |
| Hosted CI | not run | conventions, intake and labels only; Windows and the relinkers did not run |
Read-only check output
Precheck was not run: no PR-head code is run here. Diff check: passed. Conventions: passed. ABI audit: passed with warnings (22). Stack check: exit 1, individual preview in conflict. Hosted: gh api repos/boykopovar/AnyPS5/commits/c31a358b1c0221205f7eba41a9aa73b32eb71ea7/check-runs.
Negative control
Not run here. A negative control needs a local build, and code from this head is not run here.
Findings
Blocking
- The PR conflicts with the current base. Rebase onto current main, resolve the conflicts in
Voice.cppandNgs2JobBuffer.cpp(both changed on main since this PR's base), and push.
Non-blocking
- The buffering logic and the test were not read in this pass, so there is no verdict on them.
- The ABI audit's 22 warnings need a look against the base.
Sources
- Production: https://github.com/boykopovar/AnyPS5/blob/c31a358b1c0221205f7eba41a9aa73b32eb71ea7/core/libs/prx/libSceNgs2.native/src/Voice.cpp
- Test: https://github.com/boykopovar/AnyPS5/blob/c31a358b1c0221205f7eba41a9aa73b32eb71ea7/core/libs/prx/libSceNgs2.native/tests/Ngs2JobBuffer.cpp
- Stack report (main-checkout tool):
/dev/shm/reviewer-run/precheck/14242ro/stack/stack-report.md
Not verified
- The buffering logic during processing and the test.
- Whether the ABI warnings predate this PR.
- Behaviour after the rebase (the rebased head will need a fresh review).
To get this to a good state
- Rebase onto current main and push. Then request a fresh review at the new head.
A sampler voice param 0x1000000a (filter) sent while the system renders, or while earlier controls are still buffered, went to the buffered path, which did not list it and threw "this buffered voice parameter is not implemented". The filter is now validated as the immediate path does, with the same return codes, and buffered as a copy that ApplyParam applies with the other buffered controls. The filter checks move out of Ngs2SetFilter into CheckFilter, and the PS4 and PS5 layout handling into SamplerFilterParam, so both paths use the same code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Rebased onto current main ( |
c31a358 to
f0ed4cb
Compare
|
Tester run of Setup: Windows 11, WinLibs GCC 15.2.0 r7, and WSL2 Ubuntu 22.04, GCC 11.4. Both are Release builds with Tests:
The control gives the same result as the author's own on current Buffering path (read at PS4 library: the registered
A 1 kHz low-pass was accepted ( Overlap: the open NGS2 PRs #13491, #15039, #9149, #14119, #12673, #14379 and #13703 each merge cleanly on top of The AI-assisted: yes (Claude Code) |
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — approved
When a sampler filter param arrives during processing, the change now checks it and buffers it the same way the immediate path handles it. The new filter scenario passes on the merged tree. With Voice.cpp restored from main, it aborts with the reported error.
What I ran
Head f0ed4cb8171a9ebe2fd524e69529600198352dd8, main 9090972b7e91fd266819cbd77a8b058e177b5682, merged tree 26df95f13e28e746730e05e0907b5fe992da5197. Debian 13 VM (Xeon Gold 6136), GCC 14.2.0, Release, Ninja, BUILD_TESTING=ON.
git diff --check 9090972b7e91fd266819cbd77a8b058e177b5682...f0ed4cb8171a9ebe2fd524e69529600198352dd8
python3 tools/check_conventions.py --base 9090972b7e91fd266819cbd77a8b058e177b5682
ctest --test-dir build -j8 --test-load 32 -R ngs2 --output-on-failure
# negative control
git show 9090972b7e:core/libs/prx/libSceNgs2.native/src/Voice.cpp > core/libs/prx/libSceNgs2.native/src/Voice.cpp
ninja -C build -j32 -l32 ngs2_jobbuffer_tests
build/tests/ngs2_jobbuffer_tests
build/tests/ngs2_jobbuffer_tests filter
git show 26df95f13e:core/libs/prx/libSceNgs2.native/src/Voice.cpp > core/libs/prx/libSceNgs2.native/src/Voice.cpp
ninja -C build -j32 -l32 ngs2_jobbuffer_tests
build/tests/ngs2_jobbuffer_tests filter
git status --porcelain
Results
| Check | Result |
|---|---|
Merge into main |
clean |
git diff --check |
passed |
check_conventions.py |
passed |
Build (selected targets and libs) |
passed |
| Script's selected tests (750) | 748 passed, 1 expected skip, 1 known failure on main (unwrap_self, unrelated); no new failures or timeouts |
ctest -R ngs2 |
47/47 passed, 7.1 s in total |
| Negative control | ngs2_jobbuffer_tests aborts with NGS2: this buffered voice parameter is not implemented |
| Restored merged tree | filter scenario passes, git status --porcelain is empty |
Negative control output (Voice.cpp from main)
[libc] unhandled exception 'St13runtime_error' thrown from 0x7f5dd63e5167
terminate called after throwing an instance of 'std::runtime_error'
what(): NGS2: this buffered voice parameter is not implemented
Aborted
The filter scenario on its own gives the same output.
Negative control
I restored only core/libs/prx/libSceNgs2.native/src/Voice.cpp from main and rebuilt ngs2_jobbuffer_tests. Both the full run and the filter scenario abort with the error quoted in the description. After I restored the merged file, the scenario passes again and the worktree matches the merged tree. This agrees with the author's control and B-Howell's control on current main.
Findings
Blocking
None.
Non-blocking
- No registered test covers the PS4 buffered filter path. The
filterscenario builds only the PS5Ngs2SamplerVoiceFilterParam, andngs2_core_jobbufferruns onlyover. As a result, the PS4 branch ofSamplerFilterParam(core/libs/prx/libSceNgs2.native/src/Voice.cpp:559-572) is never tested on the buffered path. B-Howell already raised this and showed it with a local probe; I confirm the gap by reading the code only. A follow-up could register a PS4 variant offilteronngs2_core_jobbuffer_tests.
Code reading of the new branch (core/libs/prx/libSceNgs2.native/src/Voice.cpp:647-654):
- It rejects a custom sampler and another rack in the same order and with the same codes as
ApplyParam(Voice.cpp:1019-1022). It then runsSamplerFilterParamandCheckFilter, the same code the immediate path reaches throughNgs2SetFilter. - The capture-time checks cannot go stale before
ApplyPendingControlsreplays the copy:maxFiltersis a rack property.voice.portsis sized only at voice creation (Rack.cpp:207).- So
CheckFiltercannot newly throw when the copy is applied.
CopyParamcopiesparam.sizebytes, and that size was already checked against the layout, so the replay reads no more than was validated.- The refactor of
Ngs2SetFilterandApplySamplerParamonly moves code.
Not verified
- PS4-layout filter params sent during processing: no test exercises this path (see above). I checked it only by reading the code.
- Red Dead Redemption (PPSA30528): I did not run the title, by policy.
- Windows/MinGW build: not built here. B-Howell reports 47/47 on Windows.
- Vulkan validation comparison: skipped by the script, because no file under the AGC driver, shader recompiler or VideoOut paths changed.
BunnyFl
left a comment
There was a problem hiding this comment.
Reads right, and here's a Windows run next to XairRrR's Linux one. In core/libs/prx/libSceNgs2.native/src/Voice.cpp, the immediate path keeps its behaviour: the four filter checks move unchanged into CheckFilter, and the PS4/PS5 layout handling (with its size checks and codes) moves into SamplerFilterParam. The buffered path runs the same checks before CopyParam, rejects the custom sampler rack with the same codes, and treats another rack as an error, as for the other buffered params.
Tested on Windows 11, WinLibs GCC 15.2.0, with this head merged onto current main: ngs2_jobbuffer and ngs2_core_jobbuffer pass, nothing skipped. With main's Voice.cpp, ngs2_jobbuffer aborts with NGS2: this buffered voice parameter is not implemented, the error PPSA30528 stops on.
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 380 tests)Skipped (3)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
A sampler voice filter param (
0x1000000a) that a title sends while the NGS2 system renders, or while earlier controls on the voice are still buffered, ended the process:Controls sent during processing have been buffered since #12995 (
CaptureParamControl), but the PS5 sampler filter param, whichApplySamplerParamhas handled since 52be0d5, was not on the buffered list. Red Dead Redemption (PPSA30528) sends one about 20 s after start, during the opening, so on currentmainit stops there; the earlier reports in its thread predate #12995 and reached gameplay.CaptureParamControlnow accepts the filter param on a sampler voice: it runs the same checks as the immediate path, with the same return codes (custom sampler rack, another rack, size, filter index, type, location), and buffers a copy thatApplyPendingControlspasses toApplyParam, as setups and patches already are.Ngs2SetFilterintoCheckFilter, and the PS4/PS5 layout handling fromApplySamplerParamintoSamplerFilterParam, so both paths share them. The immediate path's behaviour is unchanged.ngs2_jobbuffergains afilterscenario: during processing, each rejected filter returns the code the immediate path returns for it, and a 1 kHz low-pass sent then is applied (a Nyquist-rate signal drops below 1/1000 of its energy). Without the change the scenario stops with the error above.Not in this PR: the custom-rack filter param (
0x400013xx), the sampler exit-loop and peak-meter params are not on the buffered list either and would throw the same way. No title is known to send them during processing.Tested
CachyOS Linux (kernel 7.3.0-4-cachyos), GCC 16.2.1, Release; AMD Ryzen 7 5800X; NVIDIA GeForce RTX 2060 (615.78.08).
ctest -R ngs2: 45 of 45 pass, including the newfilterscenario (182ngs2_jobbufferscenarios in all) and thengs2_core_*PS4 tests. Thefilterscenario fails onmainwith the error above.maindc9dc7eaae: onmainit stops ~20 s in with the error above (2 of 2 runs); with this change it runs through the opening into gameplay (3 runs, the longest 240 s), with noterminate,not implementedor driver failure in the log.tools/pr_overlap.pycould not compare seven open NGS2 PRs that conflict withmain. Checked by hand: none changes the filter handling; #13857, #13938 and #14119 edit other branches ofCaptureParamControl, and #13857 and #13938 also add scenarios toNgs2JobBuffer.cpp, so whichever lands second needs a small rebase there.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code