xmos / lib_xcore_math

XMOS optimised arithmetic and vector processing library
Other
6 stars 14 forks source link

filter_biquad_s32 does not have saturation limits #174

Closed Liu0345 closed 4 months ago

Liu0345 commented 6 months ago

In the lib_dsp/api/dsp_filters.h file, there are comments about the saturation limit,

“If overflow occurs in the final 64-bit result, it is saturated at the minimum/maximum value given the fixed-point format and finally shifted right by q_format bits.”

image

In the latest lib_xcore_math/api/xmath/filter.h file, the filter_biquad_s32 comment does not specify this saturation limit I would like the lib_xcore_math maintainers to help add the corresponding saturation limit for filter_biquad_s32, In actual use, when the input is greater than 0dBFS, the output waveform will approach normal, not inverting, and the spectrum will approach normal

Thank you!

mbanth commented 6 months ago

This issue is tracked by Jira AP-449.

mbanth commented 6 months ago

@Liu0345, thank you for pointing out this issue.

Liu0345 commented 5 months ago

@mbanth , I used #175 681fec1 to test and found the following problem(Maybe I tested it wrong). Since I don't know whether this problem has been found, I propose it.

In the picture, the first channel is the data processed by filter biquad s32, and the second channel is the original data.

EQ function is realized through filter biquad s32, and then Gain value is set, so that filter biquad s32 can process signals exceeding 0dB. Finally, the obtained data does not appear to exceed 0dB from the waveform, but the viewing spectrum is abnormal

image add-saturation-for-biquad-test.zip, It's the test audio I recorded

uvvpavel commented 5 months ago

Hi @Liu0345, the current filter_biquad_s32 does not saturate (because it's very costly in terms of cycles). PR #175 makes the current API saturate at int32_max/int32_min instead of overflow, so the behavior you're seeing is normal (when saturation occurs, it will add harmonics). I assume you're using q31? if so, there's no headroom for signals that are above 0dB, because the output data can not exceed 0dB as [int32_min, int32_max] is [-1, 1] in q31. If you want to process signal higher then 0dB you can use lower qxx formats. Going one bit down will give you 6dB of headroom. Hope this clears things out ✨

Liu0345 commented 5 months ago

@uvvpavel Thank you so much for responding to my question, clearing my mind and providing me with direction

uvvpavel commented 5 months ago

Hi @Liu0345, I've just merged #175, which adds a new filter_biquad_sat_s32 API, it works the same as filter_biquad_s32 but saturates at [int32_min + 1, int32_max], rather than overflowing. The old API is unchanged. If you know that your filter might overflow (i.e. if it has a gain), you can choose to use the saturating API at a cost of speed.

I hope this helps, thanks for raising the issue 😊