-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Chorus audio effect #10044
base: main
Are you sure you want to change the base?
Chorus audio effect #10044
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like your approach of adding multiple voices. It'd be nice to also have support for pitch shifting with a synthio.LFO
on delay_ms
to achieve the full "warble" effect, but I need to do some hw testing before making that call. As for testing, it may be better to run an audiocore.WaveFile
through it (ie: StreetChicken.wav) instead of a constant note.
} | ||
word = word / voices; | ||
|
||
word = synthio_mix_down_sample(word, SYNTHIO_MIX_DOWN_SCALE(2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't synthio_mix_down_sample(word, SYNTHIO_MIX_DOWN_SCALE(voices))
have a similar effect rather than calculating the average and then performing the mix down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SYNTHIO_MIX_DOWN_SCALE
value may also need to be pre-calculated in common_hal_audiodelays_chorus_set_voices
to avoid unnecessary float calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synthio_mix_down_sample
works to remove peaks that would push close to/beyond the upper range of the int16. It won't actually perform and average at all, especially if you are dealing with quieter sounds.
E.G. say a range of -20 to +20 and you have 4 voice samples -5, 0, +5, +5 which results in +5/4 or 1.25. The mix down call would just pass +5 on.
The mix down call is required cause you could have 4 voice samples of +15, +15, +15, +15 giving you +60 in a +20 upper range.
That said I should change the 2 to voices and whether we pre-calculate that is based on the next comment I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my experimentation, I've found that averaging reduces the perceived volume of the effect, and that the mix down call is appropriate enough to prevent peaking. This is the same paradigm used in other audio effects. For example, audiodelays.Echo
adds echo and sample together without dividing by 2 before mixing down.
circuitpython/shared-module/audiodelays/Echo.c
Lines 402 to 403 in e19ff43
word = (int32_t)(echo_buffer[j % echo_buf_len] * decay + sample_word); | |
word = synthio_mix_down_sample(word, SYNTHIO_MIX_DOWN_SCALE(2)); |
Another example is synthio which adds each note buffer into the output buffer via sum_with_loudness
then applies mix down.
circuitpython/shared-module/synthio/__init__.c
Lines 342 to 346 in 255eea9
// mix down audio | |
for (size_t i = 0; i < dur * synth->base.channel_count; i++) { | |
int32_t sample = out_buffer32[i]; | |
out_buffer16[i] = synthio_mix_down_sample(sample, SYNTHIO_MIX_DOWN_SCALE(CIRCUITPY_SYNTHIO_MAX_CHANNELS)); | |
} |
if (voices == MP_OBJ_NULL) { | ||
voices = mp_obj_new_float(MICROPY_FLOAT_CONST(1.0)); | ||
} | ||
synthio_block_assign_slot(voices, &self->voices, MP_QSTR_voices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if BlockInput
support is necessary for voices
, especially since changes in this property are integer-based. I think it'd only need to be an integer >= 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it was BlockInput
so the number of voices could be changed slowly from an LFO. It rounds at the moment. It may never be used but I'm not sure if there is a downside?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me, but I think it needs testing to ensure that the output isn't too distorted during the voice changes.
shared-bindings/audiodelays/Chorus.c
Outdated
mp_arg_val_t args[MP_ARRAY_SIZE(allowed_args)]; | ||
mp_arg_parse_all_kw_array(n_args, n_kw, all_args, MP_ARRAY_SIZE(allowed_args), allowed_args, args); | ||
|
||
mp_int_t max_delay_ms = mp_arg_validate_int_range(args[ARG_max_delay_ms].u_int, 1, 100, MP_QSTR_max_delay_ms); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it doesn't make too much sense from a "chorus" perspective to have a delay length greater than 100ms, should we limit the user to that range? I think we should encourage the user to use certain settings to achieve the typically desired effect in the documentation but not rule out other possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point. I'll up the limit to match Echo. Who knows maybe something cool comes from it.
What if you always loop through the full size of the buffer, |
word = sample_word; | ||
} else { | ||
int32_t step = chorus_buf_len / (voices - 1) - 1; | ||
int32_t c_pos = self->chorus_buffer_pos - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If chorus_buffer_pos == 0
, c_pos could be -1. If the if (c_pos < 0) { ...
check is moved to the top of the for loop below, it would avoid going out of the range of the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch!
@gamblor21 I've recently implemented your chorus algorithm into another project I'm working on, but modified it to use an independent size and delay offset. When modifying the offset with an LFO, I found that it would generate significant pops because of the rapid jumps to different buffer indexes. In order to help prevent this problem, I used a copy of the offset which incrementally follows the desired offset as well as an index bit shift (similar to https://github.com/relic-se/zero-stomp-arduino/blob/main/src/effects/Chorus.cpp This results in a more significant "chorus-ing" effect. I've recorded a demonstration of this using a live guitar input: dry -> basic chorus (5 voices, 50ms) -> lfo chorus (5 voices, 50ms). https://soundcloud.com/re-li-c/chorus-demo (the hardware & firmware of this project is not complete, so audio quality isn't perfect) |
Marking as draft pending #10036 being confirmed to work.
Add a chorus effect to the delay library. Chorus will have a small (100ms or less) buffer of what has played and add the current sample to the number of voices desired from within the delay buffer.
Example given a 50ms buffer:
I am a bit mixed on the usefulness/sound of this effect. On a sine wave it does not sound great, but a triangle better. But since I have wrote it may as well see what others think.