Fix synthio oscillator wrapping one sample late - #11276
Merged
Merged
Conversation
synth_note_into_buffer() wraps the DDS accumulator on `accum > lim`, but `lim` is the exclusive end of the waveform loop, so the boundary sample (accum == lim) reads waveform[waveform_length] before wrapping on the next iteration. For a note looping the whole table this reads one sample past the buffer, so the same script can render different audio from run to run depending on what the allocator left there. The ring-modulator loop just below has the same bug. Fix both by wrapping on `>=` instead of `>`. Adds a regression test with a table whose loop end falls short of the buffer end, so the read stays in-bounds and the extra sample shows up deterministically as a marker value. Fixes adafruit#11266
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.
Fixes #11266.
synth_note_into_buffer()inshared-module/synthio/__init__.cwraps theDDS accumulator on
but
limis the exclusive end of the waveform loop -- the readablesamples are
[offset, lim). Wrapping on>rather than>=lets theaccumulator land exactly on
lim, and that iteration indexeswaveform[waveform_length]: one past the loop, and for the common case of anote looping the whole table, one past the buffer itself. The ring-modulator
loop just below has the identical bug with
ring_waveform.Any note whose
dds_ratedivideslimhits the boundary on a schedule --a table played at exactly one sample per output sample hits it every
waveform_lengthsamples. When the loop covers the whole buffer, the readis genuinely out of bounds, so the same script with the same events does
not render the same audio twice.
Repro
A table with a loop end short of the buffer end, so the extra read stays
in-bounds and lands on a marker value instead of undefined memory
(
waveform_loop_end=256of a 512-sample table,frequencyset so theoscillator advances exactly one table step per output sample):
Measured on a unix coverage build of
main, four blocks of 256 samples(1024 total), marker at index 256:
[255, 511, 767, 1023][]Includes a regression test
(
tests/circuitpython/synthio_oscillator_loop_end.py) built the same way;it fails on current
mainand passes with the fix. The neighboringbiquad/filter tests that pass on unmodified
mainin the same environmentstill pass.