Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FreeDV resampling filters #777

Open
drowe67 opened this issue Dec 1, 2024 · 5 comments
Open

FreeDV resampling filters #777

drowe67 opened this issue Dec 1, 2024 · 5 comments

Comments

@drowe67
Copy link
Owner

drowe67 commented Dec 1, 2024

In #776 there was some experimentation with resampling libraries in at attempt to mimimise distortion measured with the RADE loss.py tool. However - we can do (and perhaps should do) better than swapping out 3rd party libraries and hoping for the best. We have the knowledge to build our own, e.g. examples for fixed sample rate conversion ratios in fdmdv, and in radae/dsp.py have code for generating the filter coeffs at init time. That would give us testable (against RADE) resampling code we understand completely, and can be tuned to be fit for purpose. An alternative is to devise a suite of tests to evaluate third party libraries before adopting them.

As #776 & #761 (the PR around sample slips) has shown - the signal processing is very important and needs to be designed, engineered, and tested carefully. We need to be very careful with making ad-hoc design changes (e.g. optimisation, changes to the audio I/O) without checking the signal processing fundamentals. Recent changes freedv-gui to support automated testing of the signal processing path, and the RADE loss.py tool will help here.

This Issue is a placeholder for notes on the topic of resampling filters in freedv-gui. Before kicking off any work it should be discussed & approved at PLT.

@tmiw
Copy link
Collaborator

tmiw commented Dec 1, 2024

There was actually a resampling ctest prior to the RADE work (src/pipeline/tests/ResampleTest.cpp). Currently it just generates a sine wave for one second and makes sure that the number of output samples is +/- 10% of the theoretical number (for example, if you go from 8 kHz to 48 kHz, the output can be anywhere between 43,200 samples and 52,800 samples long). This test could be restructured to check other parameters instead, like RADE loss vs. reference.

@tmiw
Copy link
Collaborator

tmiw commented Dec 1, 2024

Another thing--we probably need to be careful how testing is done. Currently, if you pass a 16 kHz WAV file (expected by lpcnet_demo for instance) into the automated test framework, it'll upsample to the "analog" sound device's sample rate before downsampling back to 16 kHz. It may be more realistic to pass in audio files that are at the same sample rate as said sound device to match what happens when someone uses their microphone.

@drowe67
Copy link
Owner Author

drowe67 commented Dec 1, 2024

Another thing--we probably need to be careful how testing is done. ....

If we get the resampler right , e.g. a transfer function of 1 between a pre-defined mask of frequencies - we can resample as many times as we like without any distortion and all these test/configuration problems go away. This is why it's important to really think carefully about the resampler (and set requirements) before we do any more coding in this area.

@tmiw
Copy link
Collaborator

tmiw commented Dec 2, 2024

If we get the resampler right , e.g. a transfer function of 1 between a pre-defined mask of frequencies - we can resample as many times as we like without any distortion and all these test/configuration problems go away. This is why it's important to really think carefully about the resampler (and set requirements) before we do any more coding in this area.

Should we go ahead and merge what's already in #776, then? Or is there additional work that's needed there?

@drowe67
Copy link
Owner Author

drowe67 commented Dec 2, 2024

| Should we go ahead and merge what's already in #776, then? Or is there additional work that's needed there?

Oh this work is a bit more blue sky, so don't let it hold up any current PRs. Need to 🤔 about it some more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants