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

Merged
merged 9 commits into from
Dec 10, 2024
Merged

Improve ConvolverNode #537

merged 9 commits into from
Dec 10, 2024

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.

@orottier orottier force-pushed the feat/improve-convolver branch from d8a39ad to 115f04a Compare December 1, 2024 13:08
@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

src/node/convolver.rs Outdated Show resolved Hide resolved
@orottier
Copy link
Owner

orottier commented Dec 4, 2024

We could also choose to implement the multi-channel features later. I think the PR as is is good enough to be included in a release this year

@b-ma
Copy link
Collaborator Author

b-ma commented Dec 5, 2024

We could also choose to implement the multi-channel features later. I think the PR as is is good enough to be included in a release this year

Sounds good indeed, I will try to clean it up today or tomorrow

@b-ma b-ma marked this pull request as ready for review December 5, 2024 10:01
src/node/convolver.rs Outdated Show resolved Hide resolved
Copy link
Owner

@orottier orottier left a comment

Choose a reason for hiding this comment

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

Mono processing looks good. Ready to merge right?

@b-ma
Copy link
Collaborator Author

b-ma commented Dec 10, 2024

Yup let's merge as is! We can continue later

@orottier orottier merged commit 0da4684 into orottier:main Dec 10, 2024
3 checks passed
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