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

Improve ConvolverNode #537

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

b-ma
Copy link
Collaborator

@b-ma b-ma commented Nov 29, 2024

Hey, it's been quite some time, hope you are well !

I just had this very rapid test with the ConvolverNode to use the fft-convolver crate, and it seems quite interesting in terms of performance (numbers are speedup vs. realtime in the benchmarks example)

main

Convolution reverb: 16.92

feat/improve-convolver

with let _ = convolver.init(RENDER_QUANTUM_SIZE, &samples);

Convolution reverb: 20.91

with let _ = convolver.init(RENDER_QUANTUM_SIZE * 2, &samples);

Convolution reverb: 43.12

with let _ = convolver.init(RENDER_QUANTUM_SIZE * 4, &samples);

Convolution reverb: 81.54

with let _ = convolver.init(RENDER_QUANTUM_SIZE * 8, &samples);

Convolution reverb: 134.55

Don't really know all the implications of this partition size parameter (probably some trade off between speed and quality given the layout of the numbers...) but all unit tests are passing on my side and I didn't ear anything really chocking

I just hacked it very rapidly (i.e. tail time needs to be re-implemented, etc.) and I won't have much time to work on it until at least January.

But wanted to have you feeling about that, do you you think it worth it to continue?

@orottier
Copy link
Owner

orottier commented Dec 1, 2024

Hello, it has been a while indeed. All is good here, I'm actually slowly working on a new release recently (just maintenance stuff).
The crate looks really promising. Real-time, partitioned and efficient is something I did not manage to find in pure rust two years ago.
Hence, let's continue exploring this solution. The tests in the rust code are quite light, so I wonder how it holds up to the wpt suite.
I'm also quite busy this month but I will try to take it a bit further. Would be really nice to have this working since the ConvolverNode may be the one true thing that still needs improvement in our lib.

@b-ma
Copy link
Collaborator Author

b-ma commented Dec 2, 2024

Ok cool

The tests in the rust code are quite light, so I wonder how it holds up to the wpt suite.

I checked the wpt, no difference, it even improved a bit for some reason

// - RENDER_QUANTUM_SIZE * 8 -> 134x
let partition_size = RENDER_QUANTUM_SIZE * 8;

[0..buffer.number_of_channels()].iter().for_each(|_| {
Copy link
Owner

Choose a reason for hiding this comment

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

As clippy suggests, iterating over [Range] does not do what you would expect. Perhaps better just for _ in 0..buffer.number_of_channels() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right (I did it but somehow lost it in my rebase...), I will fix that soon

(...this is not exactly the right strategy I think but this can fixed/improved later)

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.

2 participants