Add loop_start and loop_end properties to synthio.Note for waveshaping and sampling capabilities. #8629

dcooperdalrymple · 2023-11-17T23:57:17Z

Lately, I have been experimenting with utilizing synthio.Note as a sampling engine. This is done by loading audio data into the note.waveform property and manipulating the note.bend in such a way that the frequency of the note object reflects the actual frequency of the audio sample data (view code).

Currently, a limitation is that waveform data will always loop from 0 to buffer size - 1 (inclusive). By implementing simple loop_start and loop_end properties and some basic boolean logic, you can modify the looping start and end point of the waveform data when filling the audio buffer.

Additionally, when an envelope is triggered the accumulator will begin at 0 regardless of the loop settings and only start looping at loop_start once it's hit either loop_end or the end of waveform data.

Relevant Links

Simple Demonstration Code
Video Demonstration (includes sampling examples)

NOTE: This is my first attempt at a contribution to adafruit/circuitpython. Feel free to correct any improper formatting or language in the provided code updates.

…g and sampling capabilities.

jepler

Thank you! This looks nice and simple and adds good functionality. I have two small comments that may help result in better docstrings, please take a look.

jepler · 2023-11-18T03:45:11Z

shared-bindings/synthio/Note.c

+//|     loop_start: int
+//|     """The index of where to begin looping waveform data. Must be greater than 0 and less than the total size of the waveform data."""
+STATIC mp_obj_t synthio_note_get_loop_start(mp_obj_t self_in) {
+    synthio_note_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    return mp_obj_new_int(common_hal_synthio_note_get_loop_start(self));


Can you clarify this docstring? I think it "must be greater than or equal to zero and less than ..."

jepler · 2023-11-18T03:47:05Z

shared-bindings/synthio/Note.c

+//|     loop_end: int
+//|     """The index of where to end looping waveform data. Must be greater than 0 or ``loop_start`` and less than the total size of the waveform data."""


Can you clarify this docstring too? I think I'd add "If it's zero, or greater than or equal to the length of the waveform, the loop occurs at the end of the waveform", or something along those lines.

jepler · 2023-11-18T03:52:10Z

shared-module/synthio/Note.c

+}
+
+void common_hal_synthio_note_set_loop_start(synthio_note_obj_t *self, mp_int_t value_in) {
+    mp_int_t val = mp_arg_validate_int_range(value_in, 0, 32767, MP_QSTR_loop_start);


I used 16384 as the maximum length of a waveform (which maybe should have been 16383 since the upper bound is inclusive); this should probably match, which means it would be good if there was a #define for it in shared-module/synthio/__init__.h.

I don't have any notes that show why I used 16384 instead of a higher value like 32767. I may have been concerned about overflow in arithmetic.

Allowing a higher maximum size would definitely be a plus for sampling, but I vote we leave that for a separate PR. Creating the #define will help make the process of changing that value in the future easier, though.

jepler

Thank you! This looks like a nice bit of added functionality. I have a few small comments that I hope will lead to better docstrings, please take a look.

dcooperdalrymple · 2023-11-18T12:33:09Z

Hi @jepler! Thanks for chiming on this. I have no issue updating those docstrings (which were rushed initially) and creating that global #define, but after further thought, I want discuss some potential modifications first.

These properties may be simplified and more semantic if they were a single tuple called waveform_loop such as (start, end).
I do like needing to specify the index of the loop end, but it may be wiser to instead ask for the loop length. Currently, loop_end is completely ignored in the case where is it less than loop_start. Ie: note.loop_start = 256, note.loop_end = 128. With length instead, as long as it is greater than 2 (there must be at least 2 samples to my understanding), it should always work.
I haven't played around with ring modulation too much, but there shouldn't be any reason we can't also have a ring_waveform_loop property.

Notes on Testing:

I haven't run computation tests with all 12 notes running simultaneously, but the only operation that was added per-frame of the buffer is the + offset which I'm guessing won't make a significant difference to the CPU overhead.
The only device I've tested this on is an RP2040. I don't see any reason why other platforms would cause compatibility issues, but I do have some ESP32 devices laying around if that is needed.

jepler · 2023-11-18T23:55:24Z

I don't have a strong feeling about a pair of properties vs a single property that is a tuple. Here are some things to consider:

with a tuple, you can guarantee the assignment is atomic (both start and end updated at the same time)
you can check that 0 <= start < end and reject some nonsense
(but you can't check end < len(waveform) since waveform can be updated dynamically with a waveform of a different length

Creating a fresh tuple requires allocations, but so do many things in synthio (new envelope, new note being two obvious examples), so that's no reason to exclude it.

Something else to consider: A memoryview() can be efficiently sliced, in that it slicing't create a copy of the underlying memory. So, assigning note.waveform=memoryview(full_waveform)[start:end] is real close to what this PR does. However, it doesn't provide any way to run the [0:start] segment of the waveform exactly once at the start of a note.

I don't mind that the behavior with end <= start is nonsensical; the docs can simply say that the result is undefined.

I'm not particularly concerned about the performance; as you note, you didn't add anything to an inner loop.

…form_loop_...', added 'ring_waveform_loop_...` parameters, and updated docstrings.

dcooperdalrymple · 2023-11-20T15:58:59Z

I've decided to keep the current implementation mostly because I'm not a big fan of the current method of handling note envelopes.
I was not aware of the use of memoryview in this way. I see that as a great option for basic waveshaping, but it does conflict with the needs of sampling.
I've added in the define as SYNTHIO_WAVEFORM_SIZE and also made the slight adjustment of requiring that the loop_end parameter be at least 1. This makes sense from a semantic point of view, but make setting the property as 0 to quickly disable it not possible.
I've added parameters to control the looping of ring_waveform as well. By doing so, I've also renamed the properties to waveform_loop_... and ring_waveform_loop_.... I've updated the original example gist and added a new one to demonstrate this on the ring modulator: ring_waveform_loop_test.py & video demonstration.
I apologize for all of these updates being stashed into one commit. Pre-commit was fighting me on my attempts to separate them.

Let me know if you'd like to make any other adjustments, @jepler. Otherwise, I'm happy with the current iteration.

jepler · 2023-11-20T16:28:55Z

ping @todbot @jedgarpark in case you have feedback

jepler · 2023-11-20T16:37:17Z

I pushed some small tweaks to the documentation.

todbot · 2023-11-20T16:57:18Z

This is a wonderful addition, thanks Cooper! I'll be loading this onto some boards shortly.

dcooperdalrymple · 2023-11-20T19:34:54Z

Great job on the documentation, @jepler! I didn't want to get my hands too dirty on that one, but I think you nailed it. I'll try to put together a standalone gist soon to better demonstrate the use of synthio for sampling. Plus, I think the recent addition of synthio.Synthesizer.note_info could help make this even better.

dcooperdalrymple · 2023-11-20T21:44:52Z

Here's my quick sampler demonstration using the loop parameters of this build: code.py & video demonstration.

jepler

OK, let's ship it! Thanks again for your contribution.

jepler · 2023-11-20T16:02:59Z

shared-bindings/synthio/Note.c

@@ -53,13 +57,17 @@ static const mp_arg_t note_properties[] = {
 //|         frequency: float,
 //|         panning: BlockInput = 0.0,
 //|         waveform: Optional[ReadableBuffer] = None,
+//|         waveform_loop_start: int = 0,
+//|         waveform_loop_end: int = 0,


Now the default is not 0 anymore.

Add loop_start and loop_end properties to synthio.Note for waveshapin…

785ef5a

…g and sampling capabilities.

dhalbert requested a review from jepler November 18, 2023 01:25

jepler requested changes Nov 18, 2023

View reviewed changes

Added SYNTHIO_WAVEFORM_SIZE definition, renamed 'loop_...' to 'wave…

55db6b7

…form_loop_...', added 'ring_waveform_loop_...` parameters, and updated docstrings.

Documentation improvements

0fe7502

jepler approved these changes Nov 21, 2023

View reviewed changes

jepler merged commit 383f797 into adafruit:main Nov 21, 2023
325 checks passed

dcooperdalrymple deleted the synthio-note-loop branch November 21, 2023 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add loop_start and loop_end properties to synthio.Note for waveshaping and sampling capabilities. #8629

Add loop_start and loop_end properties to synthio.Note for waveshaping and sampling capabilities. #8629

dcooperdalrymple commented Nov 17, 2023

jepler left a comment

jepler Nov 18, 2023

jepler Nov 18, 2023

jepler Nov 18, 2023

dcooperdalrymple Nov 18, 2023

jepler left a comment

dcooperdalrymple commented Nov 18, 2023

jepler commented Nov 18, 2023

dcooperdalrymple commented Nov 20, 2023

jepler commented Nov 20, 2023

jepler commented Nov 20, 2023

todbot commented Nov 20, 2023

dcooperdalrymple commented Nov 20, 2023

dcooperdalrymple commented Nov 20, 2023

jepler left a comment

jepler Nov 20, 2023

		//\| loop_end: int
		//\| """The index of where to end looping waveform data. Must be greater than 0 or ``loop_start`` and less than the total size of the waveform data."""

Add loop_start and loop_end properties to synthio.Note for waveshaping and sampling capabilities. #8629

Add loop_start and loop_end properties to synthio.Note for waveshaping and sampling capabilities. #8629

Conversation

dcooperdalrymple commented Nov 17, 2023

Relevant Links

jepler left a comment

Choose a reason for hiding this comment

jepler Nov 18, 2023

Choose a reason for hiding this comment

jepler Nov 18, 2023

Choose a reason for hiding this comment

jepler Nov 18, 2023

Choose a reason for hiding this comment

dcooperdalrymple Nov 18, 2023

Choose a reason for hiding this comment

jepler left a comment

Choose a reason for hiding this comment

dcooperdalrymple commented Nov 18, 2023

jepler commented Nov 18, 2023

dcooperdalrymple commented Nov 20, 2023

jepler commented Nov 20, 2023

jepler commented Nov 20, 2023

todbot commented Nov 20, 2023

dcooperdalrymple commented Nov 20, 2023

dcooperdalrymple commented Nov 20, 2023

jepler left a comment

Choose a reason for hiding this comment

jepler Nov 20, 2023

Choose a reason for hiding this comment