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

Add resample support via ssrc (libresample) #9

Closed
wants to merge 3 commits into from

Conversation

gnif
Copy link

@gnif gnif commented Mar 11, 2019

This patch set rewrites the common processing code to add resample support if it is required. Things of note with this patch set:

  • Run-time memory allocation/deallocation has been replaced with ringbuffers
  • RnNoise has been altered slightly to support a ringbuffer avoiding additional memory copies
  • Resample should be bypassed for 48KHz, but this is untested.

Copy link
Owner

@werman werman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good, I didn't look deep into it just glanced at it, I'll look better later. Also could you squash the commits because I don't think all this history is needed

src/common/src/RnNoiseCommonPlugin.cpp Outdated Show resolved Hide resolved
src/common/src/RnNoiseCommonPlugin.cpp Show resolved Hide resolved
src/common/src/RnNoiseCommonPlugin.cpp Outdated Show resolved Hide resolved
While this on the surface may not make much sense to do, according to Xiph the
RnNoise neural net is computed specifically for 48KHz and as such would require
a re-train simply to operate at a different sample rate.

This patch also implements a ring buffer to avoid excessive memory operations.
@gnif
Copy link
Author

gnif commented Mar 12, 2019

That's it, relevant commits squashed together. I kept RnNoise alterations separate. It might be an idea to suggest these upstream, but I will leave that up to you if you wish to pursue that.

@werman
Copy link
Owner

werman commented Mar 12, 2019

Thanks! Could you allow me to change the branch? I would like to push my changes. Or here are they: feature/resample, you can change the branch yourself.

Also there is one issue - the performance. I don't have exact numbers but I can see in htop that pulseaudio starts using around 30% of core while before the changes it used 5%. Since I have 44100Hz sample rate resampling is working but seems costly. So we need to think what to do about it because without resampling rnnoise can cope with slight sampling difference from my observations.

@gnif
Copy link
Author

gnif commented Mar 12, 2019

I will pull that patch into my repo, I have also noted that on larger buffer sizes it's doing strange things, ie. as an Audacity plugin. As for CPU usage, the resampler has quality settings, where I selected the best mode possible, however as this is for speech is is likely way overkill and can be dropped down. I will do some experimentation and see.

Edit: Actually I will use your branch as my upstream and rebase on your code.

@gnif
Copy link
Author

gnif commented Mar 13, 2019

I have spend considerable time on this today and as it currently stands I can not see how this will ever function correctly for programs that supply large buffers like Audacity,

At the very least the LADSPA interface expects the same number of samples out as were sent in, and since Audacity operates in 512KB blocks, this number is not a multiple of the ReNoise frame size of 480 ((512*1024)/480 = 1092.26666667), which makes this filter practically useless for such applications.

I am not sure how to proceed and my time allotment for this task is up I am sorry

@theglobe
Copy link
Contributor

theglobe commented Mar 13, 2019 via email

@werman
Copy link
Owner

werman commented Mar 13, 2019

At the very least the LADSPA interface expects the same number of samples out as were sent in, and since Audacity operates in 512KB blocks, this number is not a multiple of the ReNoise frame size of 480 ((512*1024)/480 = 1092.26666667), which makes this filter practically useless for such applications.

I am not sure how to proceed and my time allotment for this task is up I am sorry

That doesn't sound good, just a wild idea - either pad the last frame with silence or pad it on the left with data from previous frame. I probably should try to use ladspa plugin with Audacity.

I guess 480 was chosen because of the sample rate 48000 kHz, or?

Yes.

If we want to use the rnn properly for other sample rates I think we must adjust and train it accordingly for each sample rate.

I do believe that it shouldn't be necessary to retrain the network because what is the REAL difference for neural network between sample rates - I don't think there is much.

@gnif
Copy link
Author

gnif commented Mar 13, 2019

I do believe that it shouldn't be necessary to retrain the network because what is the REAL difference for neural network between sample rates - I don't think there is much.

I think you will find that it actually does matter, there is a fork of RnNoise called RnNoise-nu which attempts to implement variable sample rates, both the author of RnNoise and RnNoise-nu have agreed that a retrain is required.

See: xiph/rnnoise#39

@werman
Copy link
Owner

werman commented Mar 13, 2019

I do believe that it shouldn't be necessary to retrain the network because what is the REAL difference for neural network between sample rates - I don't think there is much.

I think you will find that it actually does matter, there is a fork of RnNoise called RnNoise-nu which attempts to implement variable sample rates, both the author of RnNoise and RnNoise-nu have agreed that a retrain is required.

See: xiph/rnnoise#39

That's unfortunate.

@MabaKalox MabaKalox mentioned this pull request Oct 11, 2021
@werman
Copy link
Owner

werman commented Jul 23, 2022

I took stab at resampling, but nothing good came out. I see no point in implementing resampling. At the end I ended adding a stern warning to use only 48KHz. It's a already a standard sampling rate on all systems.

@werman werman closed this Jul 23, 2022
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

Successfully merging this pull request may close these issues.

3 participants