audio: the plain reverb, the delay, the biquad filter on a mix and the splitter destinations' filters #1063
Open
blushOFF wants to merge 4 commits from
blushOFF/pr/audio-effects into main
pull from: blushOFF/pr/audio-effects
merge into: MagicBeanDev:main
MagicBeanDev:main
MagicBeanDev:github/pr/2893
MagicBeanDev:github/pr/2890
MagicBeanDev:github/pr/2887
MagicBeanDev:github/pr/2878
MagicBeanDev:github/pr/2875
MagicBeanDev:github/pr/2874
MagicBeanDev:github/pr/2873
MagicBeanDev:github/pr/2871
MagicBeanDev:github/pr/2756
MagicBeanDev:github/pr/2897
MagicBeanDev:github/pr/2896
MagicBeanDev:github/pr/2718
MagicBeanDev:github/pr/2842
MagicBeanDev:github/pr/2691
MagicBeanDev:github/pr/2895
MagicBeanDev:github/pr/2743
MagicBeanDev:github/pr/2765
MagicBeanDev:github/pr/2708
MagicBeanDev:github/pr/2732
MagicBeanDev:github/pr/2755
MagicBeanDev:github/pr/2817
MagicBeanDev:github/pr/2876
MagicBeanDev:release-please--branches--main
MagicBeanDev:release-please--branches--main--release-notes
MagicBeanDev:github/pr/2778
MagicBeanDev:github/pr/2888
MagicBeanDev:github/pr/2866
MagicBeanDev:github/pr/2877
MagicBeanDev:github/pr/2800
MagicBeanDev:github/pr/2829
MagicBeanDev:github/pr/2850
MagicBeanDev:github/pr/2889
MagicBeanDev:github/pr/2882
MagicBeanDev:github/pr/2860
MagicBeanDev:github/pr/2849
MagicBeanDev:github/pr/2886
MagicBeanDev:github/pr/2794
MagicBeanDev:github/pr/2840
MagicBeanDev:github/pr/2830
MagicBeanDev:github/pr/2834
MagicBeanDev:github/pr/2719
MagicBeanDev:github/pr/2742
MagicBeanDev:github/pr/2741
MagicBeanDev:github/pr/2820
MagicBeanDev:github/pr/2771
MagicBeanDev:github/pr/2859
MagicBeanDev:github/pr/2858
MagicBeanDev:github/pr/2823
MagicBeanDev:github/pr/2757
MagicBeanDev:github/pr/2769
MagicBeanDev:github/pr/2844
MagicBeanDev:github/pr/2843
MagicBeanDev:github/pr/2673
MagicBeanDev:github/pr/2855
MagicBeanDev:github/pr/2739
MagicBeanDev:github/pr/2807
MagicBeanDev:github/pr/2851
MagicBeanDev:github/pr/2789
MagicBeanDev:github/pr/2814
MagicBeanDev:github/pr/2818
MagicBeanDev:github/pr/2774
MagicBeanDev:github/pr/2772
MagicBeanDev:github/pr/2750
MagicBeanDev:github/pr/2786
MagicBeanDev:github/pr/2785
MagicBeanDev:github/pr/2782
MagicBeanDev:github/pr/2779
MagicBeanDev:github/pr/2775
MagicBeanDev:github/pr/2706
MagicBeanDev:github/pr/2766
MagicBeanDev:github/pr/2747
MagicBeanDev:github/pr/2759
MagicBeanDev:github/pr/2745
MagicBeanDev:github/pr/2740
MagicBeanDev:github/pr/2733
MagicBeanDev:github/pr/2725
MagicBeanDev:github/pr/2727
MagicBeanDev:github/pr/2728
MagicBeanDev:github/pr/2729
MagicBeanDev:github/pr/2721
MagicBeanDev:github/pr/2722
MagicBeanDev:github/pr/2715
MagicBeanDev:github/pr/2714
MagicBeanDev:github/pr/2707
MagicBeanDev:github/pr/2703
MagicBeanDev:github/pr/2705
MagicBeanDev:github/pr/2697
MagicBeanDev:github/pr/2679
MagicBeanDev:github/pr/2676
MagicBeanDev:github/pr/2669
MagicBeanDev:docs/what-hookshot-connects-to
MagicBeanDev:fix/hdr-encode-test
MagicBeanDev:fix/main-tests
MagicBeanDev:fix/play-cache-files
MagicBeanDev:perf/game-barrier-order
MagicBeanDev:feat/heap-lazy
MagicBeanDev:feat/texture-detail
MagicBeanDev:perf/astc-opaque-bc1
MagicBeanDev:fix/fs-dir-lock
MagicBeanDev:fix/framegen-native-kept
MagicBeanDev:fix/update-no-downgrade
MagicBeanDev:port/k10-gcn-f16-mirrors
MagicBeanDev:fix/framegen-mult-cap
MagicBeanDev:chore/release-as-2.1.1
MagicBeanDev:perf/pcache-load-switch
MagicBeanDev:fix/prepare-progress-log
MagicBeanDev:fix/update-closes-stale-game
MagicBeanDev:fix/hdr-scaled-sampled
MagicBeanDev:perf/first-start-preset
MagicBeanDev:perf/zero-upload
MagicBeanDev:fix/vcore-escape-2s
MagicBeanDev:test/fake-gh-atomic
MagicBeanDev:perf/batch-idle-flush
MagicBeanDev:test/finish-stub-a
MagicBeanDev:chore/record-2.1.0
MagicBeanDev:fix/main-red-tests
MagicBeanDev:fix/test-display-startup-driver-cache
MagicBeanDev:feat/mesh-shader-mode
MagicBeanDev:fix/menu-probes-at-startup
MagicBeanDev:fix/fps-counter-classic
MagicBeanDev:fix/test-log-batch-flake
MagicBeanDev:fix/narrow-screen-hud
MagicBeanDev:ci/prereleases
MagicBeanDev:release/v2.0.15
MagicBeanDev:hotfix/v2.0.15
MagicBeanDev:fix/present-ahead-replay
MagicBeanDev:release/v2.0.13
MagicBeanDev:fix/bracket-paths-2013
MagicBeanDev:fix/hdr-overlays-2013
MagicBeanDev:fix/fps-counter-scale-2013
MagicBeanDev:fix/stick-seen-at-rest
MagicBeanDev:hotfix/menu-fits-screen
MagicBeanDev:fix/f1-always-opens-menu
MagicBeanDev:fix/ini-keep-lines
MagicBeanDev:claude/cranky-pike-7cf247
MagicBeanDev:perf/texture-answer-flat
MagicBeanDev:fix/stamp-test-modules
MagicBeanDev:fix/appimage-fonts
MagicBeanDev:fix/display-awake
MagicBeanDev:release/v2.0.12
MagicBeanDev:fix/discord-windows
MagicBeanDev:rc/v2.0.12-tests
MagicBeanDev:fix/dlss-keep-feature
MagicBeanDev:rc/v2.0.12
MagicBeanDev:button-icons
No reviewers
Labels
Clear labels
accessibility
Barrier affecting people with disabilities
autorelease: pending
autorelease: tagged
bug
Something isn't working
dependencies
Pull requests that update a dependency file
documentation
Improvements or additions to documentation
duplicate
This issue or pull request already exists
enhancement
New feature or request
github_actions
Pull requests that update GitHub Actions code
good first issue
Good for newcomers
help wanted
Extra attention is needed
invalid
This doesn't seem right
question
Further information is requested
testing wanted
wontfix
This will not be worked on
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
2 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1063
Loading…
Reference in a new issue
No description provided.
Delete branch "blushOFF/pr/audio-effects"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
OoT calls
nn::audioeffects that the import handler took until now, so the mixes they sit on came out dry: noreverb on the sub mixes the game gives one, no delay, and no filter on a mix. A gameplay log listed them as
unimplemented import(43nn::audiosymbols, withlr, so called). This implements them inhost/nn_audio.c.What it adds
AddReverb,Set/IsReverb*,Set/GetReverbInputOutput): run on the existing I3DL2 network, from theplain reverb's parameters: linear early, late, reverb and out gains; the early mode as the size of the early-reflection
taps (small room to cavern; none); the late mode as the late delay (room, hall, plate, cavern, maximum); pre-delay,
decay time and high-frequency decay ratio. This is an approximation of the console's reverb, not a copy of it.
AddDelay,Set/GetDelay*,Set/IsDelayEnabled,SetDelayInputOutput): a delay line per channel; whatcomes back is low-passed (
LowPassAmount), mixed with the neighbour channel's (ChannelSpread) and fed back(
FeedbackGain); out =DryGainx input + the delayed signal.HOS_AUDIO_NO_DELAY=1leaves it out (read withenv_on, asHOS_AUDIO_NO_REVERB).AddBiquadFilter,SetBiquadFilterParameter,SetBiquadFilterEnabled,SetBiquadFilterInputOutput): the floatTBiquadFilterParameter(enable @0, numerator @4, additive denominator @16,as the voices' float parameter), transposed direct form II as the voices' filters, with the same blow-up guard. It
runs on
SetBiquadFilterEnabled; the parameter's enable @0 is not read, because OoT leaves it 0 on mix 2's low passwhile it turns the filter on.
SetSplitterBiquadFilterParameter, sub mix and final mix): two filters perdestination, applied to that destination's copy of the voice. The filter state is kept per splitter (OoT gives each
voice a splitter of its own).
SetAuxEnabled,SetAuxInputOutput(aux stays pass-through),AcquireAudioOutputDeviceNotification, the device output volume and auto-tune calls,ListVirtualInputDevices, thevoice-drop parameter.
(
audio: reverb 0 on mix 7: wet rms ...), as a check that it does something.Measured
audio: biquad filter 0 on mix 2,audio: delay 0 on mix 4, 6 channels, up to 500 ms,audio: reverb 0..2 on mix 7/8/9; the low pass the game keeps on mix 2(b 0.416 0.833 0.416, a -0.461 -0.205) now filters. 142 s of output dumped (
HOS_AUDIO_DUMP) without clipping.tests/test_audio_effects.py(RUN_ALL_ARGS = ["--cc", "gcc"]; passes with gcc and clang under-Wall -Wextra -Werror) compiles the real delay and mix-filtersection with synthetic signals: echoes at the delay time and at the feedback, dry gain, channel spread, a NaN input,
a one-pole low pass (50 Hz kept, 12 kHz down by more than 20 dB) and unstable coefficients (silence, no burst).
Not done
reverb_frameis where they go.Hi! This PR has merge conflicts with
mainnow (a lot has landed today). Could you mergemainin or rebase and resolve them? Happy to give it a proper review once it's up to date.Rebased on current
main(7dbdcdc0), conflicts resolved. The patch itself is unchanged:effects_framestill runs on each mix before the final mix's channel map, so it sits before the new 5.1 path.tests/test_audio_effects.pypasses on the rebased tree (6 cases). I have not replayed it in game since the rebase yet; I'll report here when that run is done.Thanks for the rebase. I read through the whole diff; the effect code looks careful (handle ranges don't collide between reverbs/delays/mix filters, the NaN and blow-up guards are there, the delay reads all channels before writing so in-place in/out is fine, and the ARG() indices match the AddReverb/AddDelay/SetSplitterBiquadFilterParameter signatures). One thing to fix, plus a few smaller notes.
Test doesn't build with -Werror (verified).
tests/test_audio_effects.pyextracts the section from host/nn_audio.c intoaudio_effects.incand compiles with-Wall -Wextra -Werror. The section containsstatic Delay delays[MAX_DELAYS];andstatic BqFx bqfx[MAX_BQFX];, and the test never references either, so gcc stops with'delays' defined but not used [-Werror=unused-variable](same forbqfx). I reproduced it by running the same extraction on the head file and compilingtests/audio_effects_test.cwith the same flags. Since the test is registered inrun_all.py, it will fail there. Easiest fix is to reference them in the test (e.g.(void)delays; (void)bqfx;), or to start the extraction after those two declarations.Smaller notes (nits unless you disagree):
bq_z) is per splitter, not per voice. The comment says OoT gives each voice its own splitter, which is fine, but if two voices ever share one splitter their samples will run through the same filter state. Worth keeping in mind.SetBiquadFilterParameteron a mix ignores the parameter's enable flag at @0 (onlySetBiquadFilterEnabledcounts), while the splitter path honours it. If that's deliberate, a short comment would help.@ -375,0 +477,4 @@f->z[c][0] = z0, f->z[c][1] = z1;}}Unused in the test's extracted section, so
-Wall -Wextra -Werrorin tests/test_audio_effects.py fails with'delays' defined but not used(same forbqfxbelow). See the review body.Thanks for the ListVirtualInputDevices change, that one's fine. The blocker from my last review is still open, though: nothing changed in the test or the section it extracts, and CI is still red on both Python source checks jobs for exactly this (the only failing test in those runs is tests/test_audio_effects.py, which fails with
'bqfx' defined but not usedand'delays' defined but not usedunder -Wall -Wextra -Werror). The test extracts the delay/filter section but the functions that usedelays[]/bqfx[]aren't in it, so the statics are unused. Easiest fix is to reference them in the fixture (e.g.(void)delays; (void)bqfx;in audio_effects_test.c), or use them from the test.Still open from before, all nits: splitter filter state is per splitter rather than per voice; SetBiquadFilterParameter on a mix ignores the enable flag at @0 unlike the splitter path; the description still says the voices' float biquad is left to #669 (it's merged now); only the SubMix overloads of AddBiquadFilter/AddReverb/AddDelay exist.
Fix the test build and this is good to go.
@ -375,0 +477,4 @@f->z[c][0] = z0, f->z[c][1] = z1;}}This is the
delaysthat -Werror flags as defined but not used in the extracted test section (same forbqfxat the BqFx declaration).Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks!
The blocker from both earlier reviews is still there. I ran
python -I -B tests/test_audio_effects.pyon this head and gcc stops with'bqfx' defined but not usedand'delays' defined but not usedunder-Werror. That's the only failure in the red Python source checks, so it comes from this PR. Adding(void)delays; (void)bqfx;in tests/audio_effects_test.c is enough to fix it.Beyond that:
RUN_ALL_ARGS, so the ARGS line should move into test_audio_effects.py.HOS_AUDIO_NO_DELAYwithgetenv(...) != NULL.HOS_AUDIO_NO_REVERBbeside it uses env_on since #2042, so this one should too.The earlier nits are still open: splitter filter state is per splitter, the mix biquad ignores the enable flag at @0, and the description still mentions #669, which has merged.
Thanks for the reviews, and sorry the blocker sat there for so long. Rebased on current
main(d3353393), and the new commit (7c83fd2e) addresses the review:-Werrorblocker:tests/audio_effects_test.cnow references the section'sdelays[]andbqfx[]tables ((void)delays, (void)bqfx;).python -I -B tests/test_audio_effects.py --cc gccpasses (7 cases), and so does clang, both under-Wall -Wextra -Werror. The test file includeshookshot_env.hfor the extracted section.host/nn_audio.csits on main's 5.1 output, the reverb delay bounds and #2190's device recovery.RUN_ALL_ARGS = ["--cc", "gcc"]lives intests/test_audio_effects.py, andtests/run_all.pyis main's.HOS_AUDIO_NO_DELAYis read withenv_on, likeHOS_AUDIO_NO_REVERB.tests/test_env_on.pyfailed on the old head and passes now.SetBiquadFilterEnabledand the parameter's enable are both on. The state is reset when the parameter turns it back on. A new test case covers it: with the parameter off, the mix goes through untouched.On this tree,
tests/run_all.pyon Windows gives 372 passed and 5 failed. The 5 failures (test_fpx_sticky,test_pipeline_play_cache,test_setup_tar_diagnostics,test_portable_setup_paths,test_prepare_stall) also fail here without this PR, so they come from my local setup.In game (Windows 11, RTX 3070, a copy of a Kokiri save), a teleport tour of Kokiri Forest, Hyrule Field and Kakariko with the audio dumped: no crash or device lost, and 0
unimplemented importfornn::audio. The log shows the biquad filter on mix 2 (b 0.416 0.833 0.416, a -0.461 -0.205), the delay on mix 4 and the reverbs on mixes 7 to 9. (That biquad line is printed fromSetBiquadFilterEnabledalone, so this run does not show whether the game sets the parameter's enable too; I have not checked that yet.) 204 s of output were dumped with 0 clipped samples.Follow-up on the mix biquad's enable @0: I checked it in game, and gating on it was wrong, so
487e2e82takes it back.With a log line that shows the parameter's enable, the Kokiri tour prints
audio: biquad filter 0 on mix 2 (off by its parameter): b 0.416 0.833 0.416 a -0.461 -0.205. OoT turns that low pass on withSetBiquadFilterEnabledand gives it real coefficients, but leaves byte @0 of the float parameter at 0. With the gate,7c83fd2esilently turned off the filter this PR is mostly about.The filter runs on
SetBiquadFilterEnabledalone again, as before. The code comment now says why @0 is not read, and the test case for it is gone.tests/test_audio_effects.pypasses (6 cases, gcc and clang,-Werror), andhost/nn_audio.cpasses a clang-Wall -fsyntax-only. The full rebuild and in-game run of this exact head are still to come; I'll post here if they show anything.The -Werror blocker is fixed. I ran tests/test_audio_effects.py on
487e2e8and it passes (6 cases), and HOS_AUDIO_NO_DELAY now goes through env_on (nn_audio.c:422). The red Python jobs on this head were main's own breakage at the time (test_env_on on HOS_FPS_LOG, plus test_pcache_header_read and test_psychov35 on Windows), not this PR. The comment explaining why @0 isn't read on the mix biquad settles that point.What blocks it now is #1544, which merged this morning. It processes Aux buffers, keeps biquad history per voice and per splitter destination, and orders effects across types with its own effect_order (nn_audio.c:315 and 470). The branch conflicts in host/nn_audio.c again, and it's more than a text conflict:
The title "audio: ..." fails the Conventional Commit check. "feat(audio): ..." would pass.