Repository navigation
feat(ngs2): implement the sampler and submixer peak meter - #11354
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the implementation against the linked 12.70 analysis and issue #8039's follow-up. That analysis supports the detect modes, block range, ring calculation and a linear voicePeak, but its author explicitly says they have not determined which sampler state field receives that value. This PR writes it directly to Ngs2SamplerVoiceState::peak_height (and the submixer field). The field name makes that mapping plausible, but the current evidence does not establish the firmware mapping; the Trails report in #8039 still identifies it as unresolved. Please add evidence for the output field/offset and read semantics, or document that mapping as an unverified assumption rather than presenting it as confirmed behavior.
The new regression checks one rendered block for peak and RMS. It does not exercise the multi-block RMS average, ring eviction after the configured window, or the reset/retention behavior described in the implementation. Those cases would materially validate the main history logic.
Eligibility, conventions, intake and labels pass; hosted Build jobs are skipped by path policy. I attempted a local configure for an independent run, but it stopped because SDL2's CMake package is unavailable in this environment, so I did not run the reported 16-test suite. I’m leaving this as feedback rather than approval pending the evidence and focused coverage.
|
Additional integration check: current upstream Pairwise merge preview with #11352 is clean for code/tests but conflicts in |
|
Additional local validation on exact head |
|
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.
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.
|
|
I manually checked the new overlap warning against exact heads #11354 |
|
The previously reported NGS2 test binary built here, but this host still cannot execute it because its CPU preflight requires unsupported instructions; I have not claimed a local test pass. The sampler output mapping and multi-block RMS/ring/reset coverage questions in my earlier review remain unanswered. Current main is now |
91f72ac to
7d136db
Compare
|
Rebased onto current The debt entry now states that the output field is an assumption: Added
All 16 |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 7d136db8870a0cf2c6bc0e72aa18da5a231b45eb against current main 8100e480f7d55e87afb5376b8f784bf17df0f75c.
The new TestPeakMeterHistory addresses the earlier coverage gap: it exercises multi-block RMS, circular eviction, and reset behavior. The debt entry now accurately labels the peak_height mapping as an assumption, rather than established firmware behavior. That disclosure is useful, but the implementation still writes the result into that field and asserts reset retains the detect type even though the PR says neither behavior is verified on PS5. AGENTS.md requires unknown behavior to throw and be recorded in debt. Please provide firmware/console evidence for the field mapping and reset semantics, or leave those behaviors unsupported until evidence establishes them; the test currently verifies the PR's assumptions, not console equivalence.
The latest main has changed Render.cpp, Voice.cpp, Ngs2Render.cpp, and the NGS2 debt file since the PR's merge base c3db979cd33a65a0a651f1e7ba7958137a961413. The merge preview is currently clean, but those shared-path changes were not in the reported test base. Please refresh onto current main, preserve both sets of changes, and rerun the focused NGS2 suite on that exact head.
I also compared the current #11354 and #11315 heads with git merge-tree: they still conflict in Ngs2Internal.hpp, Rack.cpp, Render.cpp, Voice.cpp, and docs/dev/debt/libSceNgs2.native.md. These appear to be separate changes, so the conflict does not itself prove a functional dependency; please coordinate landing order and retain both implementations, test changes and debt entries when resolving it. The PR currently says Depends on: none.
The author reports all 16 ngs2_* tests passing on this head. I did not independently rerun them; hosted eligibility, conventions, labels and report checks pass, and product/build test jobs are skipped by path policy. My earlier local attempt could not execute this NGS2 test binary because the host CPU preflight requires unsupported instructions.
AI-assisted review.
7d136db to
05bb568
Compare
|
Rebased onto current I do not have PS5 firmware or console evidence for the |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 05bb568fe73b994b5a2f3b5dabe665d47a83914e against current main 4e43321bd6ceccdc1ae28568bbbc35d9a6446464. The branch is now current. The added history tests address the prior multi-block RMS, ring-eviction and reset-coverage gaps, and the author reports all 16 ngs2_* tests passing on this exact head. My python3 -I tools/check_conventions.py --base upstream/main --head upstream/pr/11354 and git diff --check upstream/main...upstream/pr/11354 also pass. I could not independently run the NGS2 binaries on this host: the earlier built test executable exits during its CPU-feature preflight because this Xeon lacks required instructions.
The remaining blocker is the behavior the author confirmed is unverified: the 12.70 module reading does not identify the peak_height output field or its read semantics, and no PS5 measurement establishes that mapping or whether reset retains the detect type. The implementation nevertheless writes voice.peakHeight to that field and the tests assert the assumed value and reset behavior. Recording the uncertainty in the debt file is transparent, but it does not make returning the assumed result conform to AGENTS.md's rule that unknown behavior remains unsupported. Please add firmware evidence for both behaviors, or leave the peak-meter output unsupported until they can be established.
The exact-head pairwise merge preview with #11315 still conflicts in Ngs2Internal.hpp, Rack.cpp, Render.cpp, Voice.cpp and the NGS2 debt file. The changes are distinct; coordinate landing order and preserve both sets when resolving those conflicts.
AI-assisted review.
|
On the two behaviours the review asks for firmware evidence on: I ran firmware 13.42's 1. The output field is the one at offset 12 (
In the first 28 bytes of each struct (what I printed), no other word changed with the detect type. The sampler's word at +24 grew by 0x40 per 64-sample grain. Detect types 3 and 2. A reset does not keep the detect type. With peak mode on and +12 reading 0.5, I sent the sampler's SETUP again (the reset that calls Notes on the setup, since the run needed two departures from the test helpers:
Both of those concern the shared NGS2 headers and helpers rather than this PR. The module's call to |
05bb568 to
4e9ab12
Compare
|
Applied the firmware 13.42 measurement on
Rebased onto current |
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 4e9ab12 against 035ac96. No actionable findings. The changed render paths maintain per-channel peak/RMS history, reset it when the detection mode changes, and validate sampler and submixer block counts. I built ngs2_render_tests, ngs2_custom_tests and ngs2_voiceindex_tests in Release with WinLibs GCC 15.2.0; focused CTest passed all three (3 passed, 0 skipped, 0 failed). Review-only checks also confirmed both rack types accept the 256-block upper boundary and reject 257. The exact-head Actions run passed eligibility only; product build/test jobs were skipped. This independently validates the affected Windows/MinGW paths, not the full 16-test NGS2 suite or console firmware behavior.
bda80ff to
c831bcc
Compare
|
Rebased onto current On the unverified behaviour: the |
c831bcc to
b452176
Compare
|
Rebased onto current |
b452176 to
e625f89
Compare
|
Rebased again onto current |
e625f89 to
c32e19c
Compare
|
Rebased onto current
Rebuilt and ran both focused suites on the rebased head: 21/21 in |
6ce47a5 to
687922b
Compare
|
Rebased automatically onto current |
687922b to
e57c454
Compare
|
Rebased automatically onto current |
e57c454 to
8218014
Compare
|
Rebased automatically onto current |
8218014 to
873d10b
Compare
|
Rebased automatically onto current |
873d10b to
137cfe3
Compare
|
Rebased automatically onto current |
137cfe3 to
f7ce4d2
Compare
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the open PRs #11945, #11899, #11859, #11772, #11087, #10789, #11627, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
|
Rebased automatically onto current |
f7ce4d2 to
41905ba
Compare
|
Rebased automatically onto current |
41905ba to
a2d5b03
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the diff against the 13.42 measurement in this thread ( |
What
Implement the sampler and submixer peak meter.
0x10000009and the submixer control0x20000005(16 bytes, detect type at offset 8).0x804A8093.num_peak_meter_blocksvalues per channel (1 to 256; otherwise0x804A8092). Peak mode stores the largest absolute sample of each grain, RMS mode the sum of squares.peak_heightthroughsceNgs2VoiceGetState: the largest channel peak, orsqrt(sum / (blocks * grain))for RMS, over the filled slots.Firmware 13.42 confirms the
peak_heightfield at offset 12 (peak 0.5 and RMS 0.353553 for alternating 16384/0 PCM) and that a sampler SETUP reset turns the meter off until it is enabled again. The submixer reset remains unmeasured and is recorded in the debt entry, together with the ring details that still follow fma-rc's 12.70 module reading.Tested
Linux, GCC 16.2.1. All 16
ngs2_*tests pass, includingTestPeakMeterandTestPeakMeterHistory:peak_heightin peak mode (0.5),0x804A8093,0x804A8092,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)