MagicBeanDev/Hookshot

Archived

Watch

Fork 0

fix(audio): process Aux buffers and isolate splitter filter history #1544

Merged

mikaba-eu merged 14 commits from pr/audio-effect-processing into main

2026-11-12 03:03:21 +00:00

mikaba-eu commented

2026-11-10 07:31:13 +00:00

(Migrated from github.com)

Exchange complete planar PCM32 frames through bounded Aux send/return rings, preserve effect insertion order, and keep biquad history per voice and splitter destination. Queued parameter updates, underrun silence, overflow handling and channel remapping have authored regression fixtures. Upstream audio output, volume, background muting and wave-buffer release behavior are retained.

Refreshed against upstream 0a28ff29. The diff contains audio implementation and regression fixtures, plus the existing one-line SDL ownership getter in the isolated Wayland fixture, needed for the current Clang warning checks. The new audio tests declare their compiler requirement to the repository runner. This PR is independent of the renderer and scene API changes.

Validation: full Linux source suite on b2885b46: 361 passed, 0 failed, 14 skipped. After merging 0a28ff29 and aligning compiler declarations, 24 affected checks passed, 0 failed, 0 skipped on final head 5bf7df2d. The skips in the full run cover platform/game/GPU requirements and unavailable optional tools. The Aux and biquad tests compile the actual production functions and exercise synthetic signals without an audio device or game assets. No game or audio output was started for this refresh. Existing silent gameplay checks do not establish audible effect quality, latency or surround output; focused listening on this PR head and Windows runtime validation remain unverified.

Final refresh: merged upstream ddafc1f9; 11 affected builder/update/fixture checks passed afterward.

The final source includes upstream 2e62a977; its release-only changes passed both affected release/update checks.

Aux audio currently bypasses the game's CPU effect workers, and splitter routes need independent filter histories to avoid contaminating other outputs. This fix was implemented with GPT-6 Astra medium & xhigh. Exchange complete planar PCM32 frames through bounded Aux send/return rings, preserve effect insertion order, and keep biquad history per voice and splitter destination. Queued parameter updates, underrun silence, overflow handling and channel remapping have authored regression fixtures. Upstream audio output, volume, background muting and wave-buffer release behavior are retained. Refreshed against upstream `0a28ff29`. The diff contains audio implementation and regression fixtures, plus the existing one-line SDL ownership getter in the isolated Wayland fixture, needed for the current Clang warning checks. The new audio tests declare their compiler requirement to the repository runner. This PR is independent of the renderer and scene API changes. Validation: full Linux source suite on `b2885b46`: **361 passed, 0 failed, 14 skipped**. After merging `0a28ff29` and aligning compiler declarations, **24 affected checks passed, 0 failed, 0 skipped** on final head `5bf7df2d`. The skips in the full run cover platform/game/GPU requirements and unavailable optional tools. The Aux and biquad tests compile the actual production functions and exercise synthetic signals without an audio device or game assets. No game or audio output was started for this refresh. Existing silent gameplay checks do not establish audible effect quality, latency or surround output; focused listening on this PR head and Windows runtime validation remain unverified. Final refresh: merged upstream `ddafc1f9`; 11 affected builder/update/fixture checks passed afterward. The final source includes upstream `2e62a977`; its release-only changes passed both affected release/update checks.

mikaba-eu commented

2026-11-10 19:14:34 +00:00

(Migrated from github.com)

Merged current main (1392ea3) and resolved the conflicts, preserving upstream replacements where they supersede our changes. Migrated local tests to the new per-test runner declarations. Independent source suites and the combined suite pass; fresh Linux gameplay results and limitations are recorded in the updated description. Hyrule Field and a warm Forest run completed the short movement tests, but camera hitches remain; earlier memory-guard stops and the incomplete Market event are not reported as passes. AI-assisted development: GPT-6-Astra (medium and xhigh).

Merged current main (`1392ea3`) and resolved the conflicts, preserving upstream replacements where they supersede our changes. Migrated local tests to the new per-test runner declarations. Independent source suites and the combined suite pass; fresh Linux gameplay results and limitations are recorded in the updated description. Hyrule Field and a warm Forest run completed the short movement tests, but camera hitches remain; earlier memory-guard stops and the incomplete Market event are not reported as passes. AI-assisted development: GPT-6-Astra (medium and xhigh).

mikaba-eu commented

2026-11-10 19:39:34 +00:00

(Migrated from github.com)

Refreshed again to main 117eae7 and resolved the new Anti-Lag source-fixture conflict. Complete Linux source suite re-run: 304 passed, 0 failed, 18 skipped. Existing gameplay evidence remains explicitly tied to its tested build.

Refreshed again to main `117eae7` and resolved the new Anti-Lag source-fixture conflict. Complete Linux source suite re-run: 304 passed, 0 failed, 18 skipped. Existing gameplay evidence remains explicitly tied to its tested build.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 18:26:07 +00:00

hckrman101 (Migrated from github.com) left a comment

What looks good

Splitter filter history is kept per voice and per destination (split_bq_z in Voice), and the epochs reset it when the route or the on/off state changes. That's the correct model: #1063 keeps one bq_z per splitter destination, shared by every voice that feeds it, which is exactly the contamination this PR avoids. Pulling biquad_frame out of render_voice leaves the voice path unchanged.
The Aux rings are bounded by the guest allocation. Capacity is rounded down to whole FRAME * n frames, so the send side never enqueues part of a frame, and a guest that writes whole frames can't misalign the return ring either. Every buffer index is checked against m->buffers before use.
Real-time safety: aux_frame and the splitter path add no allocation and no new lock. Everything runs under the existing au_lock. The new stack use (about 46 KB in aux_frame) is fine for the renderer thread. The static footprint grows by about 2.3 MB (split_bq_z x 512 voices), which is acceptable.
Ordering effects across types by insertion order (shared effect_order) is a good call.

What worries me

An enabled Aux now replaces its output buffers with the return ring, and underruns become silence. That matches hardware, but our renderer thread runs on its own 5 ms clock, and audio_mix_deadline makes it catch up back-to-back after any stall (up to 3 s). The game's worker gets no chance to run between those frames, so every frame of a burst after a hitch underruns and the Aux-routed buffers drop out. You mention camera hitches in your gameplay notes, so this is a real situation. Before, the same buffers passed through dry and kept playing. If the game routes a whole bus through an Aux, a hitch now cuts that bus out briefly. A small pre-roll (start returning only once one or two frames are queued) would hide most of this, at the cost of a frame of latency.
OP_AUX_ENABLE resets both rings every time it's applied, even when the state doesn't change (inline).
No listening test yet. The description says so itself: "No game or audio output was started for this refresh", "focused listening ... remain[s] unverified". Since this changes which signal reaches the speakers, please post an in-game before/after: which scenes use the Aux path (a HOS_AUDIO_DUMP of the same spot on main and on this branch would do), whether anything goes quiet or gets doubled, and whether hitches or loading cause dropouts. One Windows run would help too.

Smaller points

OP_SPLIT_BIQUAD is dropped unless dest_mix[d] already equals the mix. A game that sets the filter before the first SetSplitterMixVolume for that destination (which is where dest_mix gets set lazily) loses the filter without any sign. Probably fine, but worth a trace line.
Nit: the underruns and overruns counters are never reported. A rate-limited AUT line would make the in-game check above much easier.
Nit: aux_ring_read and aux_ring_write can memcpy from a NULL data with size 0 when capacity is 0. That's harmless in practice, but an early return when !r->capacity is cleaner.

Overlap

#1063 (blushOFF) adds the same SetSplitterBiquadFilterParameter HLEs and rewrites effects_frame. The two can't both land as they are (duplicate symbols, conflicting splitter state). I'd keep the per-voice history from this PR and have #1063 rebase its plain reverb, delay and mix biquad on top of the order scheme here. Worth agreeing between you which one goes first.
#2115 only touches the posix waveOut ring (win32_posix.c). There's no textual overlap. It doesn't change the renderer clock, so it neither helps nor hurts point 1.

CI: only the title check has run. The Windows builder, AppImage and game-free source regression workflows are waiting for approval (action_required), and the branch is 46 commits behind main. So the new test_audio_aux / test_audio_biquad tests haven't run in CI yet.

Thanks for this. I read the full diff against current main. The splitter half looks right to me. The Aux half is plausible, but it changes what is audible in a way no test here can check, so I'd like an in-game listen before this goes in. **What looks good** - Splitter filter history is kept per voice and per destination (`split_bq_z` in `Voice`), and the epochs reset it when the route or the on/off state changes. That's the correct model: #1063 keeps one `bq_z` per splitter destination, shared by every voice that feeds it, which is exactly the contamination this PR avoids. Pulling `biquad_frame` out of `render_voice` leaves the voice path unchanged. - The Aux rings are bounded by the guest allocation. Capacity is rounded down to whole `FRAME * n` frames, so the send side never enqueues part of a frame, and a guest that writes whole frames can't misalign the return ring either. Every buffer index is checked against `m->buffers` before use. - Real-time safety: `aux_frame` and the splitter path add no allocation and no new lock. Everything runs under the existing `au_lock`. The new stack use (about 46 KB in `aux_frame`) is fine for the renderer thread. The static footprint grows by about 2.3 MB (`split_bq_z` x 512 voices), which is acceptable. - Ordering effects across types by insertion order (shared `effect_order`) is a good call. **What worries me** 1. **An enabled Aux now *replaces* its output buffers with the return ring, and underruns become silence.** That matches hardware, but our renderer thread runs on its own 5 ms clock, and `audio_mix_deadline` makes it catch up back-to-back after any stall (up to 3 s). The game's worker gets no chance to run between those frames, so every frame of a burst after a hitch underruns and the Aux-routed buffers drop out. You mention camera hitches in your gameplay notes, so this is a real situation. Before, the same buffers passed through dry and kept playing. If the game routes a whole bus through an Aux, a hitch now cuts that bus out briefly. A small pre-roll (start returning only once one or two frames are queued) would hide most of this, at the cost of a frame of latency. 2. **`OP_AUX_ENABLE` resets both rings every time it's applied**, even when the state doesn't change (inline). 3. **No listening test yet.** The description says so itself: "No game or audio output was started for this refresh", "focused listening ... remain[s] unverified". Since this changes which signal reaches the speakers, please post an in-game before/after: which scenes use the Aux path (a `HOS_AUDIO_DUMP` of the same spot on main and on this branch would do), whether anything goes quiet or gets doubled, and whether hitches or loading cause dropouts. One Windows run would help too. **Smaller points** - `OP_SPLIT_BIQUAD` is dropped unless `dest_mix[d]` already equals the mix. A game that sets the filter before the first `SetSplitterMixVolume` for that destination (which is where `dest_mix` gets set lazily) loses the filter without any sign. Probably fine, but worth a trace line. - Nit: the `underruns` and `overruns` counters are never reported. A rate-limited `AUT` line would make the in-game check above much easier. - Nit: `aux_ring_read` and `aux_ring_write` can `memcpy` from a NULL `data` with size 0 when capacity is 0. That's harmless in practice, but an early return when `!r->capacity` is cleaner. **Overlap** - #1063 (blushOFF) adds the same `SetSplitterBiquadFilterParameter` HLEs and rewrites `effects_frame`. The two can't both land as they are (duplicate symbols, conflicting splitter state). I'd keep the per-voice history from this PR and have #1063 rebase its plain reverb, delay and mix biquad on top of the `order` scheme here. Worth agreeing between you which one goes first. - #2115 only touches the posix waveOut ring (`win32_posix.c`). There's no textual overlap. It doesn't change the renderer clock, so it neither helps nor hurts point 1. **CI:** only the title check has run. The Windows builder, AppImage and game-free source regression workflows are waiting for approval (`action_required`), and the branch is 46 commits behind main. So the new `test_audio_aux` / `test_audio_biquad` tests haven't run in CI yet.

host/nn_audio.c

On underrun this zero-fills the output buffers. That's right for hardware, but here the renderer catches up in back-to-back bursts after a stall (audio_mix_deadline), so a hitch now becomes a dropout of everything routed through this Aux, where main played it dry. A one- or two-frame pre-roll before the first read (and after a reset) would absorb host scheduling jitter. See point 1 in the review body.

On underrun this zero-fills the output buffers. That's right for hardware, but here the renderer catches up in back-to-back bursts after a stall (`audio_mix_deadline`), so a hitch now becomes a dropout of everything routed through this Aux, where main played it dry. A one- or two-frame pre-roll before the first read (and after a reset) would absorb host scheduling jitter. See point 1 in the review body.

host/nn_audio.c

This clears both rings on every SetAuxEnabled that reaches an update, including true -> true. If the game re-asserts enabled each update (common in effect-manager loops), the return ring is emptied every frame and the Aux output stays silent. Suggest resetting only when the state actually changes:

case OP_AUX_ENABLE:
    if (a->active_enabled != k->i0) aux_reset(a);
    a->active_enabled = k->i0;
    break;

This clears both rings on every `SetAuxEnabled` that reaches an update, including `true` -> `true`. If the game re-asserts enabled each update (common in effect-manager loops), the return ring is emptied every frame and the Aux output stays silent. Suggest resetting only when the state actually changes: ```c case OP_AUX_ENABLE: if (a->active_enabled != k->i0) aux_reset(a); a->active_enabled = k->i0; break; ```

Joey-Magnus (Migrated from github.com) requested changes

2026-11-11 21:45:59 +00:00

Joey-Magnus (Migrated from github.com) left a comment

GitHub shows this as mergeable, but it no longer builds its own tests on current main. I merged the head (5713010) into origin/main locally. tests/test_audio_aux.py and tests/test_audio_biquad.py both fail to compile with implicit declaration of function 'env_on'. Both pass at the PR head.

The cause is #2042, which landed after your last main merge (2e62a97). It changed host/nn_audio.c to read its switches with env_on (AUT at line 132, HOS_AUDIO_NO_REVERB, HOS_AUDIO_STATS and others). Your fixtures cut code out of that file but don't include gpu/vk/compat/hookshot_env.h. Do it the way tests/test_audio_voice_biquad.py does: an ENV_ON_H path passed with -include.

hckrman101's points at this same head are all still open:

Aux buffers drop to silence during the catch-up bursts after a hitch; a frame or two of pre-roll would hide this;
OP_AUX_ENABLE resets both rings even when the state doesn't change;
there's no in-game before/after yet (a HOS_AUDIO_DUMP of the same spot would do);
#1063 is still open and overlaps the splitter HLEs, so which goes first needs agreeing.

GitHub shows this as mergeable, but it no longer builds its own tests on current main. I merged the head (5713010) into origin/main locally. `tests/test_audio_aux.py` and `tests/test_audio_biquad.py` both fail to compile with `implicit declaration of function 'env_on'`. Both pass at the PR head. The cause is #2042, which landed after your last main merge (2e62a97). It changed `host/nn_audio.c` to read its switches with `env_on` (`AUT` at line 132, `HOS_AUDIO_NO_REVERB`, `HOS_AUDIO_STATS` and others). Your fixtures cut code out of that file but don't include `gpu/vk/compat/hookshot_env.h`. Do it the way `tests/test_audio_voice_biquad.py` does: an `ENV_ON_H` path passed with `-include`. hckrman101's points at this same head are all still open: - Aux buffers drop to silence during the catch-up bursts after a hitch; a frame or two of pre-roll would hide this; - `OP_AUX_ENABLE` resets both rings even when the state doesn't change; - there's no in-game before/after yet (a `HOS_AUDIO_DUMP` of the same spot would do); - #1063 is still open and overlaps the splitter HLEs, so which goes first needs agreeing.

hckrman101 (Migrated from github.com) reviewed

2026-11-11 22:17:39 +00:00

hckrman101 (Migrated from github.com) left a comment

Quick follow-up, since nothing has been pushed since my review (still 5713010).

I can confirm @Joey-Magnus's build break. host/nn_audio.c on main now calls env_on (AUT, HOS_AUDIO_NO_REVERB, HOS_AUDIO_STATS, HOS_AUDIO_IMMEDIATE), and neither tests/test_audio_aux.py nor tests/test_audio_biquad.py passes -include gpu/vk/compat/hookshot_env.h the way tests/test_audio_voice_biquad.py does (ENV_ON_H). So both new checks stop compiling once this meets main. The branch is now 117 commits behind main, so a merge of main plus that one-line fixture change should cover it.

My earlier points are unchanged and still open: the Aux underrun dropouts during catch-up bursts (pre-roll), OP_AUX_ENABLE resetting the rings when the state doesn't change, the in-game before/after listen, and agreeing the order with #1063.

CI: only the title check has run. The build and source-suite workflows are still waiting for approval, so the new tests haven't run in CI.

Quick follow-up, since nothing has been pushed since my review (still 5713010). I can confirm @Joey-Magnus's build break. `host/nn_audio.c` on main now calls `env_on` (`AUT`, `HOS_AUDIO_NO_REVERB`, `HOS_AUDIO_STATS`, `HOS_AUDIO_IMMEDIATE`), and neither `tests/test_audio_aux.py` nor `tests/test_audio_biquad.py` passes `-include gpu/vk/compat/hookshot_env.h` the way `tests/test_audio_voice_biquad.py` does (`ENV_ON_H`). So both new checks stop compiling once this meets main. The branch is now 117 commits behind main, so a merge of main plus that one-line fixture change should cover it. My earlier points are unchanged and still open: the Aux underrun dropouts during catch-up bursts (pre-roll), `OP_AUX_ENABLE` resetting the rings when the state doesn't change, the in-game before/after listen, and agreeing the order with #1063. CI: only the title check has run. The build and source-suite workflows are still waiting for approval, so the new tests haven't run in CI.