Skip to content

fix(ngs2): return documented Delay option errors - #12107

Open
Th3EvilGod wants to merge 4 commits into
boykopovar:mainfrom
Th3EvilGod:codex/ngs2-delay-option-errors
Open

Th3EvilGod wants to merge 4 commits into
boykopovar:mainfrom
Th3EvilGod:codex/ngs2-delay-option-errors

Conversation

@Th3EvilGod

@Th3EvilGod Th3EvilGod commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

What

For malformed custom Delay options, return the PS5 errors through both rack-creation APIs: 0x804A8013 for option size, 0x804A8095 for type, 0x804A8096 for tap count, and 0x804A8097 for an out-of-range finite maximum length. The source is fma-rc's reading of PS5 12.70 libSceNgs2.native.sprx, Delay setup at 0x30b80, not disassembly or console measurements I performed. The constant names are local descriptive names.

Keep the existing Delay creation/audio compatibility path and unsupported-path throws. Null-option processor initialization, nonfinite/reserved values, unsupported rack/state shapes and the precedence of multiple invalid payload fields remain unresolved; Delay DSP and controls remain unimplemented. The added regression checks the documented creation errors rather than asserting unknown query or failure semantics. The existing custom-rack suite now runs against both NGS2 libraries.

This update preserves the published test-cleanup commit and its authorship, including removal of the new invalid-query, negative-infinity and multiply-invalid throw assertions requested by oneandonlydean. Rebase conflicts retain main's PS4 voice-state test registration and all its PitchShift, sampler, peak-meter and debt changes; only the Delay debt entry is changed here.

Integration: no code prerequisite. #11315 is merged. The original conflicts with #10599 and #13491 are absent after this rebase: direct Git merge previews with #10599 bc07113988e1a8098d8c44cfc3f70d0fa8b40923 and #13491 f4b5b8040bd4629ea675379388fbe66580af161c both exit 0. Those are merge checks, not combined runtime validation. #12107 before #13491 is agreed in Kyon's proposal and my acceptance. I also requested #12107 before #10599 in that reply; Kyon's confirmation remains pending, and no agreement or review approval is claimed for it.

The refreshed overlap tool fails closed on stale branches, so it is not reported as a clean scan. I manually inspected every listed external branch: #1384 changes Reverb declarations/registration; #5548 PCM decoding notes/registration; #9149 PCM-loop debt; #10249 the Reverb/PitchShift option-word helper; #11104 pending-export debt; #11388 VAG declarations/debt; #12673 multi-buffer UserFx and Reverb. None implements these Delay errors. The #12107 row is our old published head being replaced. These unresolved automatic comparisons remain disclosed; the stale branches still need their own current-main reconciliation.

Tested

Windows 11 build 26300, AMD Ryzen 7 9800X3D, WinLibs MinGW-w64 GCC 15.2.0 r7, Release full-runtime configuration, BUILD_TESTING=ON, pinned bundled source dependencies. Current tested head 7f6f7870d22172e13e3d0ca39faae164bd54da1c, based on main 3b0523ab913e8d974fdb755189634c31c60a50ce.

  • Runtime libs and all 32 currently registered NGS2 test executables build successfully; both NGS2 modules are NID-patched.
  • ctest --test-dir build/windows-cnd -R '^ngs2_' -V: 46/46 pass, 4.89 seconds, including ngs2_custom, ngs2_core_custom and the newer sampler, PitchShift, worker/lifecycle and PS4 voice-state tests inherited from main.
  • The independent 20-byte fixture uses literal module ID 0x1d and literal expected error codes. It covers 26 malformed options at module positions 0, 1 and 23 in 1-, 2- and 24-module racks. Both creation APIs return the expected errors, preserve the output handle and make no allocator calls. Cases include unsigned extremes and immediately adjacent floats outside both length endpoints. A valid rack still creates after those failures.
  • Existing varying-PCM, silence, valid option endpoint and all-256-Delay-control rejection coverage remains enabled. The newly introduced assertions on unknown console results are removed.
  • Post-rebase conventions and whitespace checks pass. The live overlap tool's unresolved comparisons and manual review are described above; it did not return a clean result.

Historical regression evidence: the original new ngs2_custom fixture on unchanged main 771d8b5bf7 aborted with NGS2: this custom Delay module option size is not implemented. B-Howell independently reported Linux/WSL2 GCC 11.4.0 runs: both custom suites passed ten repetitions at earlier head 6bb9fe3ac7 merged onto 6e9b9134f6, and both failed when only production was restored to main. Those are historical, attributed results, not a Linux run of this rebased head.

No current-head Linux/macOS, PS5 hardware or commercial-title run, no complete repository test-suite run, and no performance claim. The PCM regression preserves host compatibility; it does not establish console-equivalent Delay processing.

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

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

The current branch merges cleanly with main, and I independently confirmed that it conflicts in docs/dev/debt/libSceNgs2.native.md with each open PR #10599 and #11315. Your integration section says the proposed landing order is not yet agreed, while the checklist says Depends on: none. CONTRIBUTING.md requires the authors to agree on which overlapping PR lands first and to record the ordering in Depends on. Please document that agreement with both authors and update the checklist before merge. This is an integration-rule blocker; the PRs can all remain open.

@github-actions

github-actions Bot commented Oct 27, 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/include/Ngs2Types.hpp
core/libs/prx/libSceNgs2/CMakeLists.txt
PR 5548 core/libs/prx/libSceNgs2/CMakeLists.txt
docs/dev/debt/libSceNgs2.native.md
PR 10249 core/libs/prx/libSceNgs2.native/src/Custom.cpp
PR 13491 docs/dev/debt/libSceNgs2.native.md
PR 13703 docs/dev/debt/libSceNgs2.native.md
PR 14119 core/libs/prx/libSceNgs2.native/include/Ngs2Types.hpp
docs/dev/debt/libSceNgs2.native.md
PR 14212 docs/dev/debt/libSceNgs2.native.md
PR 15341 docs/dev/debt/libSceNgs2.native.md
PR 15538 docs/dev/debt/libSceNgs2.native.md
PR 15732 core/libs/prx/libSceNgs2.native/include/Ngs2Types.hpp
docs/dev/debt/libSceNgs2.native.md
PR 15946 core/libs/prx/libSceNgs2.native/src/Custom.cpp
docs/dev/debt/libSceNgs2.native.md
PR 16015 docs/dev/debt/libSceNgs2.native.md
PR 16248 core/libs/prx/libSceNgs2.native/include/Ngs2Types.hpp
docs/dev/debt/libSceNgs2.native.md

@oneandonlydean

Copy link
Copy Markdown
Collaborator

@Th3EvilGod thanks for this one, and for saying plainly that the codes come from fma-rc's reading of the 12.70 module. The codes and ranges match that comment, and no option that created a rack before is rejected now. We keep tests to real behaviour and argument validation, not asserting that a case with an unknown console result throws, since that kind of test breaks the moment someone settles it. In core/libs/prx/libSceNgs2.native/tests/Ngs2Custom.cpp, please drop (1) the assertion in TestCustomDelayOptionErrors that sceNgs2RackQueryBufferSize throws std::invalid_argument and leaves its output untouched (lines 694-703 on this head), since the description says query's contract is unresolved, and (2) the new entries in the CreateThrows list: the -infinity length and the four options with more than one invalid field ({20, 2, 0}, {20, 2, 4, 0.0f}, {20, 0, 0, 0.0f}, {20, 2, 0, 0.0f}, lines 786-787), because their console result is not known. The production code can keep throwing for them, and the error-code checks can stay.

@B-Howell

Copy link
Copy Markdown
Contributor

Linux result (the description has Windows only).

Setup: Ubuntu 22.04 under WSL2, GCC 11.4.0, Ninja Release, BUILD_TESTING=ON, AMD Ryzen 7 7800X3D. Head 6bb9fe3ac7499647b5142f92471d91384b8d9e51 merged onto main 6e9b9134f6ed14e659d870a33118d164096551df (clean).

Tests: ctest -R '^ngs2_custom$|^ngs2_core_custom$' --repeat until-fail:10

  • PR merged onto main: 2/2 pass on all 10 runs (both the libSceNgs2.native and the libSceNgs2 builds of Ngs2Custom.cpp).
  • Control: libSceNgs2.native/src/Custom.cpp and include/Ngs2Types.hpp restored from main, tests kept: both abort with what(): NGS2: this custom Delay module option size is not implemented, the same failure as your Windows control.

Not tested: the other ngs2_* tests, Windows (already in the description), macOS, any title. This does not address the ordering against #10599 and #11315 raised in the review.

AI-assisted (Claude Code).

@kyon-eth

Copy link
Copy Markdown
Contributor

@Th3EvilGod My draft #13491 declares the separate sceNgs2SystemSetLoudThreshold export and adds a debt entry. A direct merge preview with this PR reports a conflict in the shared NGS2 debt file; I found no common export. Would you be happy for #12107 to land first, with my draft reconciled afterward? I will hold the draft while the order is discussed.

AI-assisted: yes.

@oneandonlydean oneandonlydean self-assigned this Oct 30, 2026
@Th3EvilGod

Copy link
Copy Markdown
Contributor Author

@kyon-eth yes, agreed: #12107 first, then #13491 rebased with the debt entries reconciled. Please record #12107 in #13491's Depends on line while it is pending.

To settle MarcZX10's earlier ordering request as well, would you also agree to #12107 before your custom Reverb PR #10599, with #10599 rebased afterward retaining both debt changes? #11315 has now merged, so its earlier conflict no longer needs a pending dependency.

I am rebasing from the current published head c45a654, preserving the test cleanup requested by oneandonlydean, and rerunning the NGS2 runtime tests. I will refresh the overlap report and PR description and link the agreed ordering there.

@Th3EvilGod
Th3EvilGod force-pushed the codex/ngs2-delay-option-errors branch from c45a654 to 7f6f787 Compare October 30, 2026 22:57
@Th3EvilGod

Copy link
Copy Markdown
Contributor Author

Updated to 7f6f787 on main 3b0523a, retaining the published cleanup commit and its authorship. Both rebase conflicts are reconciled with main's newer registrations and debt intact. Runtime libs and all 32 NGS2 test executables build; all 46 ngs2_ CTests pass on Windows (4.89 seconds). Conventions and whitespace checks pass. The description now records this head, the removed unknown-result assertions, historical Linux evidence separately, and every unresolved automatic overlap comparison.

@kyon-eth the rebased head also changes the conflict premise: direct merge previews with #10599 at bc07113 and #13491 at f4b5b80 both exit 0, including the debt file. These are clean Git previews, not combined runtime tests.

Our agreement that #12107 precedes #13491 is linked in the description. My request for the preferred order with #10599 remains open; I have not represented silence as agreement. #11315 is already merged. The earlier integration review is not claimed approved or resolved on another reviewer's behalf.

Main's Custom.cpp rejects the PS5 filter struct in its PS4_LIBRARY branch,
and ngs2_core_custom builds against that branch, so the new PS5 filter cases
failed there. Build them only outside APS5_NGS2_PS4_LIBRARY and define it for
ngs2_core_custom_tests.
@TechLevelZero

Copy link
Copy Markdown
Contributor

✅ AMD GPU test: passed

PR head be9247c merged onto main 5b1d381 · tested 2026-11-04 14:08 UTC · 19 min

GPU AMD Radeon RX 6300 (RADV NAVI24), driver Mesa 25.2.8-0ubuntu0.24.04.4, Vulkan 1.4.318
Host Ubuntu 26.04.1 LTS, kernel 7.0.0-42-generic, container ubuntu:24.04
Logs build and test logs for this run
Build ✅ passed (Release, -DANYPS5_ENABLE_SPIRV_TOOLS=ON)
Tests ✅ 1,070 passed · 3 failed (0 new, 2 also fail on main, 1 unstable on main) · 5 skipped (ANYPS5_REQUIRE_VULKAN=1)
Test setup CPU-only tests without a GPU; GPU tests (and any CPU-only test that failed or skipped without one) once on the GPU with VK_LAYER_KHRONOS_validation and ANYPS5_NO_SHADER_CACHE=1; main ran the same way
Test selection 1,078 of 3,883 tests run: the ones this change can reach, chosen by tools/select_tests.py from main's build graph
Vulkan validation ✅ no new validation IDs vs main (547 tests run on the GPU)
Slow tests (>30 s, GPU tests timed with validation layers) ⚠️ agc_driver_pipeline_journal 120.5 s, ⚠️ public_headers 87.1 s, ⚠️ agc_driver_image_sample_unnormalized 48.4 s, ⚠️ agc_driver_lazy_resolved_abi 34.2 s, ⚠️ agc_driver_depth_clear_hle 31.6 s
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)
agc_driver_pipeline_journal: failed in 8 of the last 10 main runs
Failing on main too (2, not counted against this PR)
agc_driver_image_table
agc_driver_native_multisample_depth
Validated but not compared (55, not in the main baseline's validation pass)
public_headers
public_headers_compile_0
public_headers_compile_10
public_headers_compile_12
public_headers_compile_14
public_headers_compile_16
public_headers_compile_18
public_headers_compile_2
public_headers_compile_20
public_headers_compile_21
public_headers_compile_22
public_headers_compile_23
public_headers_compile_24
public_headers_compile_25
public_headers_compile_26
public_headers_compile_27
public_headers_compile_28
public_headers_compile_29
public_headers_compile_30
public_headers_compile_31
public_headers_compile_32
public_headers_compile_33
public_headers_compile_34
public_headers_compile_35
public_headers_compile_36
public_headers_compile_37
public_headers_compile_38
public_headers_compile_39
public_headers_compile_4
public_headers_compile_40
public_headers_compile_41
public_headers_compile_42
public_headers_compile_43
public_headers_compile_44
public_headers_compile_45
public_headers_compile_46
public_headers_compile_47
public_headers_compile_48
public_headers_compile_49
public_headers_compile_50
public_headers_compile_51
public_headers_compile_52
public_headers_compile_53
public_headers_compile_54
public_headers_compile_55
public_headers_compile_56
public_headers_compile_57
public_headers_compile_58
public_headers_compile_59
public_headers_compile_6
public_headers_compile_60
public_headers_compile_61
public_headers_compile_62
public_headers_compile_63
public_headers_compile_8
Validation IDs already present on main (8 IDs in 415 tests)
VUID-VkDeviceCreateInfo-pNext-pNext (415 tests): agc_driver_address_passes, agc_driver_alias_copy_label, agc_driver_alpha_to_coverage, agc_driver_array_layer_target, agc_driver_bc_block_copy, agc_driver_bda_anchor_reads, agc_driver_bda_span_reads, agc_driver_bindless_images, and 407 more
VUID-vkCmdCopyImage-srcImage-01548 (4 tests): agc_driver_depth_clear_hle, agc_driver_depth_storage_dispatch, agc_driver_depth_surface_from_storage, agc_driver_dispatch_move_depth
VUID-vkCmdCopyImage-srcImage-01551 (4 tests): agc_driver_depth_clear_hle, agc_driver_depth_storage_dispatch, agc_driver_depth_surface_from_storage, agc_driver_dispatch_move_depth
VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605 (1 test): agc_driver_prepared_exports
VUID-VkSamplerCreateInfo-addressModeU-01079 (1 test): agc_driver_image_gather_volume
VUID-VkSamplerCreateInfo-addressModeU-parameter (1 test): agc_driver_image_gather_volume
VUID-VkSamplerCreateInfo-addressModeV-parameter (1 test): agc_driver_image_gather_volume
VUID-VkSamplerCreateInfo-addressModeW-parameter (1 test): agc_driver_image_gather_volume
Skipped (5)
agc_driver_copied_writer_label_order: skipped, the device reports a minStorageBufferOffsetAlignment of 4, so the dispatch binds its record at +0x1004 in place and nothing is written back; the test needs an alignment above 4
agc_driver_float16_unused_nan_payloads: skipped, the device has no native f16 conversions with RTE rounding and kept f16 denormals
agc_driver_gcn_display_present: skipped, no display: Vulkan support is either not configured in SDL or not available in current SDL video driver (offscreen) or platform
agc_driver_linear_target_direct_write_completion: skipped, the device does not keep host imports write-watched, so no completion stamp over the target is tracked
agc_driver_ngg_vertex_wave_reduction: skipped, s3 describes a 32-lane wave only on 32-lane subgroups, and the device has 64 lanes
Reproduce
git fetch origin be9247ca5864767b0e4341ebd48988b773c0fff5 5b1d381d5ad5d9f923326b18479c3bd889cdd588
git checkout 5b1d381d5ad5d9f923326b18479c3bd889cdd588 && git merge --no-ff be9247ca5864767b0e4341ebd48988b773c0fff5
git submodule update --init --recursive
cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=ON -DANYPS5_ENABLE_SPIRV_TOOLS=ON
cmake --build build && cmake --build build --target libs
VK_DRIVER_FILES=/nonexistent/icd.json ctest --test-dir build -E '^(agc|shader|recompiler|videoout|video_out|guest_video_out|vulkan)|gpu' --timeout 120 --parallel 12 --output-on-failure
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation \
  ctest --test-dir build -R '^(agc|shader|recompiler|videoout|video_out|guest_video_out|vulkan)|gpu' --timeout 120 --parallel 16 --output-on-failure
# failures are then re-run alone: --timeout 360 --parallel 2; new validation IDs: the test alone, twice, --parallel 1

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

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

Copy link
Copy Markdown
Collaborator

Hi Th3EvilGod,

We merged current main into this branch. Main's Custom.cpp now rejects the PS5 filter struct in its PS4-library build, and ngs2_core_custom builds against that path, so the new PS5 filter cases now build only outside APS5_NGS2_PS4_LIBRARY. The rest of the PR is unchanged.

That leaves the PS4-library filter path without a test. A case that sends the PS4 layout there would be a welcome follow-up, but it's optional and not needed for this PR.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Nov 6, 2026

This branch has not been deployed

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

Labels

build docs needs rebase Conflicts with main, rebase needed prx

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants