Add support for tuple of Biquad objects within audiofilters.Filter.

[relic-se]

Support the processing of multiple synthio.Biquad objects within audiofilters.Filter by renaming filter to filters and requiring it be a List[synthio.Biquad].

import board
import audiobusio
import audiofilters
import audiocore
import synthio

audio = audiobusio.I2SOut(bit_clock=board.GP0, word_select=board.GP1, data=board.GP2)

wave_file = open("StreetChicken.wav", "rb")
wave = audiocore.WaveFile(wave_file)

synth = synthio.Synthesizer(channel_count=wave.channel_count, sample_rate=wave.sample_rate)

effect = audiofilters.Filter(
    filters=[
        synth.high_pass_filter(200),
        synth.low_pass_filter(2000),
    ],
    buffer_size=1024,
    channel_count=wave.channel_count,
    sample_rate=wave.sample_rate,
    mix=1.0
)
effect.play(wave, loop=True)
audio.play(effect)

Comments:

• Direct assignment of objects in list does not call synthio_biquad_filter_assign (ie: effect.filters[0] = ...)
• effect.filters.append(...) and effect.filters.remove(...) are detected by testing length in audio buffer loop. There may be a better method of detecting these list manipulations.

[relic-se]

I would like some insight into a better method of updating the biquad_filter_state objects when the list of Biquad objects is updated, but otherwise this PR is functional. If there isn't a better method, I'll need to update the docstring for audiofilters.Filter.filters to demonstrate proper usage.

[jepler]

I don't think there's any more clever way to detect a changed list length. You could use realloc and only initialize any new slots.

But of course not all changes to the filters list would necessarily change the list length. For instance, code could use filters[0] = new_filter_zero. Is the code handling this case?

Another option is to say that the contents of the list are snapshotted on assignment or require the value supplied to be a read-only sequence (tuple).

Is this code enough to implement equalizer-type functionality? as it stands, the common biquad constructors (and this includes the new block biquads). In my reading I haven't seen it spelled out exactly how an equalizer would be constructed from biquads, but I feel like that seems to be the most obvious use of a bank of filters. An example which does this would be very illustrative & helpful for me.

[jepler]

I'm relatively sure that m_malloc already throws in case of failed allocation, so the call to m_malloc_fail is not needed; but also, the call to common_hal_audiofilters_filter_deinit is ineffective. I see this idiom has been used across the audiofilters so it may need to be revised elsewhere as well.

[relic-se]

I'll look into this issue and apply changes as necessary throughout. I'm still learning about how best to program within the MicroPython/CircuitPython ecosystem so bear with me! :)

[relic-se]

I began removing the ..._deinit and m_malloc_fail statements, but I'm finding that this idiom is fairly common within multiple modules of CircuitPython. As you certain that this isn't the proper handling of m_malloc?

Here are a few examples:

circuitpython/shared-module/audiomixer/Mixer.c

Lines 28 to 38 in ddfa519

	self->first_buffer = m_malloc(self->len);
	if (self->first_buffer == NULL) {
	common_hal_audiomixer_mixer_deinit(self);
	m_malloc_fail(self->len);
	}
	self->second_buffer = m_malloc(self->len);
	if (self->second_buffer == NULL) {
	common_hal_audiomixer_mixer_deinit(self);
	m_malloc_fail(self->len);
	}

circuitpython/shared-module/audiomp3/MP3Decoder.c

Lines 306 to 327 in ddfa519

	self->inbuf.buf = m_malloc(DEFAULT_INPUT_BUFFER_SIZE);
	if (self->inbuf.buf == NULL) {
	common_hal_audiomp3_mp3file_deinit(self);
	m_malloc_fail(DEFAULT_INPUT_BUFFER_SIZE);
	}
	if (buffer_size >= 2 * MAX_BUFFER_LEN) {
	self->pcm_buffer[0] = (int16_t *)(void *)buffer;
	self->pcm_buffer[1] = (int16_t *)(void *)(buffer + MAX_BUFFER_LEN);
	} else {
	self->pcm_buffer[0] = m_malloc(MAX_BUFFER_LEN);
	if (self->pcm_buffer[0] == NULL) {
	common_hal_audiomp3_mp3file_deinit(self);
	m_malloc_fail(MAX_BUFFER_LEN);
	}
	self->pcm_buffer[1] = m_malloc(MAX_BUFFER_LEN);
	if (self->pcm_buffer[1] == NULL) {
	common_hal_audiomp3_mp3file_deinit(self);
	m_malloc_fail(MAX_BUFFER_LEN);
	}
	}

circuitpython/shared-module/audiocore/WaveFile.c

Lines 100 to 110 in ddfa519

	self->buffer = m_malloc(self->len);
	if (self->buffer == NULL) {
	common_hal_audioio_wavefile_deinit(self);
	m_malloc_fail(self->len);
	}
	self->second_buffer = m_malloc(self->len);
	if (self->second_buffer == NULL) {
	common_hal_audioio_wavefile_deinit(self);
	m_malloc_fail(self->len);
	}

circuitpython/ports/stm/common-hal/pulseio/PulseIn.c

Lines 87 to 91 in ddfa519

	self->buffer = (uint16_t *)m_malloc(maxlen * sizeof(uint16_t));
	if (self->buffer == NULL) {
	// TODO: free the EXTI here?
	m_malloc_fail(maxlen * sizeof(uint16_t));
	}

(also in other ports with pulseio)

There are other cases where instead of m_malloc(...) there are calls to port_malloc(...), malloc(...), realloc(...), malloc_with_finaliser(...), ringbuf_alloc(...), and gc_alloc(...) before m_malloc_fail(...). Maybe someone down the line copied that over but with m_malloc(...) and it stuck within audio and pulseio modules?

Rather than experimenting and going outside of the accepted formatting, I'm copying a lot of my homework from other portions of CircuitPython. This is something I borrowed from audiodelays without fully understanding the underlying principles. I plan on keeping it as is for now until we develop a plan for all other instances of this methodology (likely another PR).

[relic-se]

@jepler Using a read-only tuple is the perfect solution here! The filter states would only need assignment within the setter. Thanks for the tip!

As for "equalizer" usage, the only applicable use at this time is to combine LPF and HPF to filter out unwanted signal. The example code in the first comment demonstrates that. Future additions of peakingEQ, lowShelf, and highShelf (from the Audio EQ Cookbook) to BlockBiquad would allow you to build a equalizer effect more akin to an analog audio mixer.

effect = audiofilters.Filter(
    filters=(
        synthio.BlockBiquad(
            mode=synthio.FilterMode.LOW_SHELF,
            frequency=250,
            A=0.5, # potential future parameter
            Q=1.0,
        ),
        synthio.BlockBiquad(
            mode=synthio.FilterMode.PEAKING_EQ,
            frequency=2000,
            A=2.0,
            Q=0.8,
        ),
        synthio.BlockBiquad(
            mode=synthio.FilterMode.HIGH_SHELF,
            frequency=6000,
            A=1.5,
            Q=1.2,
        ),
    ),
)

[jepler]

For this kind of equalizer does it work to apply the filters sequentially (e.g., output_samples = high_shelf(peaking_eq1(peaking_eq2(low_shelf(input_samples)))) using function call notation? I had an impression from somewhere that the filters needed to be parallel rather than sequential, but maybe this is mistaken on my part.

[relic-se]

@jepler I think that cascading the biquad filters as you've demonstrated will perform well enough for our needs. The primary use of parallel eq is within graphic equalizers where you have a large bank of band pass filters at frequency intervals and mix them together into the output. I could see this as a separate class with a predesignated filter design, but I don't think it's a high priority right now. Here's a research paper which covers this topic: https://mycourses.aalto.fi/pluginfile.php/666520/course/section/128564/Aleksi%20Peussa_1929990_assignsubmission_file_AAT_Seminar_Paper_724441.pdf. Here's a snippet from the conclusion which I think nails it on the head: "While both methods have their benefits, each also has their downsides. Simplicity of design and speed benefit the cascade, while simultaneous computation and accuracy benefit the parallel method."

[jepler]

thank you, I took a quick glance at the paper you linked and I think it will be enlightening when I get a chance to read it through. In CircuitPython we're not likely to be taking advantage of parallel methods of computation anytime soon, so simplicity and adequate single thread performance are great features to enjoy.

[jepler]

Hi! I mostly have just nitpicky things, plus one concern about backwards compatibility. Given that in practice there's not much audiofilters code deployed so far I'm inclined not to worry but I'd like Dan to sign off on that as well.

[jepler]

@dhalbert can we ignore this backward-incompatible change? audiofilters "filter" argument was in 9.2.0 but would not be in 9.2.1.

[relic-se]

It may take some work to implement, but would the ideal scenario be filter: Optional[synthio.Biquad|Tuple[synthio.Biquad]]? It would retain backwards compatibility and reduce confusion if the user only plans on using a single biquad.

[jepler]

call common_hal_audiofilters_filter_set_filters here instead of duplicating code

[jepler]

also you don't need to create an empty list, you can require that the empty tuple (that can also be the default constructor argument) is used if zero filters are actually intended.

[relic-se]

call common_hal_audiofilters_filter_set_filters here instead of duplicating code

I'm good with this. I haven't seen to many calls to common_hal_... functions in other implementations of common_hal_..._construct(..), so I wasn't sure if that was a bad practice.

also you don't need to create an empty list, you can require that the empty tuple (that can also be the default constructor argument) is used if zero filters are actually intended.

I think I'm going to switch it out to default to mp_const_none instead. I like the idea of turning off the filter without rearranging the audio signal path, by setting filters = None. In that case, common_hal_audiofilters_filter_set_filters will set filter_states = NULL and audiofilters_filter_get_buffer will simply pass the sample through.

[jepler]

.u_obj = mp_const_empty_tuple

[relic-se]

I plan on changing the default to None rather than an empty tuple. The only problem I potentially see here is if a user decides to iterate through effect.filters without testing it (ie: if effect.filters: .... It will throw "TypeError: 'NoneType' object is not iterable" if that is the case, whereas an empty tuple wouldn't.

If you would prefer the empty tuple to avoid that situation, let me know.

[jepler]

seems like if you're going for compatibility, then assigning None or a single Biquad would have to retain the current behavior when the property is fetched. On the other hand, you quickly identified that the ergonomics are not great for generic code when trying to be compatible. That's why I thought it was worth considering going ahead and breaking compatibility with this change.

[relic-se]

@jepler I may have become heavy-handed with my most recent changes, but hopefully it's for the best. I've done the following:

• Rename filters back to filter
• Support the following values:
  • individual Biquad object
  • tuple of Biquad objects
  • list of Biquad objects (which is converted to a tuple)
  • None

This retains backwards compatibility while still allowing the use of multiple filters.

The only issue I am having now with the property is that if you set it to a tuple/list of invalid objects, it generates a TypeError during synthio_biquad_filter_assign, but the value of self->filter has already been updated in common_hal_audiofilters_filter_set_filter.

import audiofilters
import synthio

synth = synthio.Synthesizer()
effect = audiofilters.Filter()

try:
    effect.filter = ["test"]
except TypeError:
    pass

print(effect.filter)
# ('test',)

The solution here would be to iterate through items within common_hal_audiofilters_filter_set_filter and test their type before calling reset_filter_states. Do you think it's worth addressing this problem?

[jepler]

We have a helper for checking items in a sequence for the correct type. Here's an example of its use:

    for (size_t i = 0; i < nmatch; i++) {
        matches[i] = mp_arg_validate_type_in(match_objects[i], &canio_match_type, MP_QSTR_matches);
    }

so you'd iterate over the collection and check the type of each object. If you don't do this first, it seems like the object ends up in an inconsistent state.

[relic-se]

That did the trick! Thank you, @jepler