Skip to content

Audio Effect Reverb #10196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Apr 21, 2025
Merged

Audio Effect Reverb #10196

merged 25 commits into from
Apr 21, 2025

Conversation

gamblor21
Copy link
Member

@gamblor21 gamblor21 commented Mar 28, 2025

Adding a reverb effect for audio effects, based on FreeVerb.

Question:

  1. Should this be the final location for the effect (audiodelays)? Early on we debated more modules but I'm not sure if there is value to make a Reverb only one.

Time permitting I may still add in a way to control dry and wet mix independently. So mix could either be mix=0.5 or mix=(0.3,0.3)

@gamblor21 gamblor21 marked this pull request as ready for review April 5, 2025 19:28
@gamblor21
Copy link
Member Author

@relic-se If you have a moment this is ready for testing. No rush.

Copy link

@relic-se relic-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work so far! I haven't dug into everything quite yet, but the biggest issue at the moment is definitely the mix implementation. <0.5 doesn't seem to work for me.

All my testing was on an RP2040 with mono 22050hz sample audio (StreetChicken.wav).

@relic-se
Copy link

relic-se commented Apr 7, 2025

As for comments on your questions, I think it is fine to keep reverb in audiodelays. Although it is filter-based, it is mostly associated with delay, and it is unlikely at the moment for us to add multiple reverb-based effects in to warrant a dedicated audioreverbs module (but I am a fan of spring reverb...). It can always be moved around in a future release if need be.

Okay, I may have changed my mind. What if we did create a new audioreverbs module, renamed this effect to "RoomReverb" or "Freeverb". Then, we could aim for "SpringReverb" and "PlateReverb" additions in the future? I'm not certain exactly how those would work or if they're feasible for the target platforms, but it would definitely give us sufficient variety. Other potential reverb types could be "ReverseReverb" and "Flerb" (has a "flanging" sound).

Unless you want to take a swing at allowing independent dry/wet mix control globally, I vote we keep it standardized for now and then expand all effects in the future to support this functionality. This question might be better poised on a separate issue/PR, but how would you handle BlockInput support in that scenario?

@gamblor21
Copy link
Member Author

Okay, I may have changed my mind. What if we did create a new audioreverbs module, renamed this effect to "RoomReverb" or "Freeverb". Then, we could aim for "SpringReverb" and "PlateReverb" additions in the future? I'm not certain exactly how those would work or if they're feasible for the target platforms, but it would definitely give us sufficient variety. Other potential reverb types could be "ReverseReverb" and "Flerb" (has a "flanging" sound).

@tannewt @dhalbert Any preference on keeping the reverb in audiodelays vs making a new audioreverbs? We had talked about a new module for them previously. Or we could always create these effect for now and move it later?

@dhalbert
Copy link
Collaborator

dhalbert commented Apr 9, 2025

The advantage of a separate module is that we can turn it off if there are space problems. So I don't think it's bad. Didn't we have audioeffects for a while but it became too big?

As for further reverbs, how different are they in terms of code? Could you make a parameterized reverb that handles all of those in a single class with parameters, or is there something different in the calcuations?

@relic-se
Copy link

relic-se commented Apr 9, 2025

The advantage of a separate module is that we can turn it off if there are space problems. So I don't think it's bad. Didn't we have audioeffects for a while but it became too big?

There was never a singular audioeffects module to my knowledge since we moved to separate modules in initial talks. Nonetheless, I am in agreement.

As for further reverbs, how different are they in terms of code? Could you make a parameterized reverb that handles all of those in a single class with parameters, or is there something different in the calculations?

Unfortunately, I think other reverb types will operate very differently. The reverb algorithm implemented here, "Freeverb", is finely-tuned for its particular operation (a general "room" sound). Further research may dispute this.

@tannewt
Copy link
Member

tannewt commented Apr 10, 2025

I'd go further and put this in a freeverb module (check pypi to make sure it doesn't exist). That way each reverb implementation can have its own module and inclusion.

@gamblor21
Copy link
Member Author

Unless you want to take a swing at allowing independent dry/wet mix control globally, I vote we keep it standardized for now and then expand all effects in the future to support this functionality. This question might be better poised on a separate issue/PR, but how would you handle BlockInput support in that scenario?

I think I may leave it and get this PR in and we can look at how to handle mix better across the board later. I think for BlockInput on separate wet/dry I'd just let them vary and and maybe with a caveat if you got 1.0 and 1.0 then yes your audio volume could double. Direct people to the mix=0.5 but have an advance mode that basically we take the guard rails off.

@gamblor21
Copy link
Member Author

I think the CI errors may be build errors?

@relic-se
Copy link

@gamblor21 I've tested your updates, and can confirm that the mix property is working as intended now. I've also tested BlockInput on all 3 properties, mix, roomsize, and damp. Everything works great!

@gamblor21 gamblor21 requested a review from tannewt April 14, 2025 22:48
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small things and a CI to fix. Thanks for the new module!

@tannewt
Copy link
Member

tannewt commented Apr 15, 2025

@gamblor21
Copy link
Member Author

This script will fix the zephyr build for you: https://github.com/adafruit/circuitpython/blob/main/ports/zephyr-cp/cptools/update_board_info.py

Thanks, took a bit to realize how it all worked. Just a note for the future, if someone (me) has stale/old folders in the shared-bindings directory it can cause problems. Wasn't hard to fix after but a small "gotcha".

I think the other issues were mostly the fact I submitted this PR before zephyr, and that should not happen again for others.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@tannewt tannewt merged commit b2c338b into adafruit:main Apr 21, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants