audiodelays, audiofilters, audiofreeverb: buffer lengths and silence … - #11367
Open
peterbay wants to merge 1 commit into
Open
audiodelays, audiofilters, audiofreeverb: buffer lengths and silence …#11367peterbay wants to merge 1 commit into
peterbay wants to merge 1 commit into
Conversation
…fills Echo and MultiTapDelay applied their length floor after their ceiling, so a short delay could be raised above the maximum and the memset that follows took "max - len" as an unsigned length. Chorus never clamped its delay to the buffer it allocated. PitchShift accepted a window too small to hold one sample. The unsigned 16-bit silence fill used memset with 32768, which repeats a single byte and so wrote zeros -- full negative deflection rather than the midpoint -- in PitchShift, GranularPitchShift and Distortion. Distortion's hard clip had an upper bound of 32768, which becomes -32768 on the cast to int16_t below it. Freeverb cleared only half of each delay line, counting uint16_t entries as bytes, and declared its channel offsets inside the per-sample loop, so both channels went through the left bank of filters. The biquad chain counted a size_t length with a uint8_t in four places.
Collaborator
|
Looks reasonable at first glance but I’ve not tested. This does address an issue I believe I’ve experienced with Echo and Chorus. |
Author
|
Testing and diagnostic script. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Code written by Claude Code, guided and corrected by @peterbay.
The problem
Eleven defects in the audio effect modules, most of them in how a delay line's length is worked out or how a buffer is filled with silence. Three of them fault the board outright.
The changes
EchoandMultiTapDelayapplied their floor after their ceiling. The two limits were arms of oneelse if, so raising a short delay to the audio buffer size could put it back above the maximum, and thememsetthat follows tookmax - lenas an unsigned length. Changed to apply the floor first and the ceiling always after it.Chorusnever clamped its delay to the buffer it allocated. The buffer is sized once frommax_delay_ms, and nothing stopped a longerdelay_msbeing used against it;EchoandMultiTapDelayboth limit this. Added the same clamp.PitchShiftaccepted a window too small to hold a sample. The window is given in bytes but holds oneint16_tper channel, sowindow=1allocated a single byte that the playback loop then wrote a 16-bit sample into. Addedmp_arg_validate_int_min.The silence fill wrote the minimum sample value, not the midpoint.
memset(word_buffer, 32768, ...)repeats a single byte, and 32768 truncates to 0, so the unsigned 16-bit path filled with full negative deflection where silence was intended. Replaced with a loop writing0x8000, inPitchShift,GranularPitchShiftandDistortion. The 8-bit branch beside it is correct, because 128 does fit in a byte.Distortion's hard clip had an upper bound of 32768. That becomes -32768 on the cast toint16_tbelow it, so a positive overdrive came out as a full negative spike. Changed to 32767.Freeverbcleared half of each delay line. The buffers holduint16_tand are allocated assize * sizeof(uint16_t), but onlysizebytes were cleared, so the reverb started from whatever was on the heap.Freeverbwas not a stereo reverb. The channel offsets were declared inside the per-sample loop, so they were zero again at the top of every sample and the switch at the bottom of it had no effect; both channels went through the left bank of comb and allpass filters.The biquad chain counted a
size_tlength with auint8_t. In the reset, tick and process helpers inaudiofilters/__init__.cand the per-block loop inFilter.c. Past 256Biquadobjects — 128 in the reset helper, which countsobjs_len * channel_count— the counter wrapped and the loop did not terminate.Testing
Seeed XIAO nRF52840 Sense with an Adafruit Audio BFF, running two builds of this branch that differ only by these changes. Effects were rendered into a WAV in RAM with
audiofilewriter.AudioFileWriterover anio.BytesIO, so the output could be compared numerically rather than by ear.Echo(max_delay_ms=10, delay_ms=5, freq_shift=False)MultiTapDelay(max_delay_ms=10, delay_ms=5)Chorus(max_delay_ms=10, delay_ms=50), playingPitchShift(window=1)ValueErrorDistortion, square wave driven into the clipFreeverb, stereo input with the channels at 3:1GranularPitchShift, fill after the sample ends0x00000x8000The three faults were recorded through
microcontroller.nvm, set immediately before the call under test and immediately after it, so the result does not depend on catching the USB serial before the board goes down.Two of these are not covered by that table. Clearing the whole of Freeverb's delay lines makes no observable difference on this board, because the heap read back as zero either way; it is correct by inspection, since the allocation is
size * sizeof(uint16_t)and thememsetwassize. The biquad counter needs more than 256Biquadobjects to show, which does not fit in this part's RAM.audiodelays,audiofiltersandaudiofreeverbare off by default on this port, so the builds setCIRCUITPY_AUDIODELAYS,CIRCUITPY_AUDIOFILTERSandCIRCUITPY_AUDIOFREEVERBexplicitly.