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

Fix broken "npm install" and memory leaks #4

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nanokina
Copy link

@nanokina nanokina commented Feb 28, 2020

Hi @dmooney65
First of all thanks for making the nice addon we are using this on a bot
But I ended up facing several problems, so I had to fix them

  1. npm install node-libsamplerate no longer works properly, (supposedly) due to CMake-JS changed the manner of use of it.
  2. The addon reveals its src_ratio related problem when two or more input streams exist on an event loop and their input samplerates varies (so do the ratios, of course).
  3. There are bits of memory leaking in the code, I've fixed it as well. (Our bot ate up 8GB RAM in just three days!)

Anyways, hope you can take a look at it 😄

- for npm install to work properly
- use 'src_set_ratio' to prevent smooth transition and concurrent reset
- use private member 'src_ratio' to remember target ratio
  in case that more than two stream exist
- rebuild on npm install, to avoid errors
@nanokina nanokina changed the title Fix broken "npm install" Fix broken "npm install" and memory leaks Feb 28, 2020
@nanokina
Copy link
Author

Test passed! 🎉

@ThePedroo
Copy link

For anyone interested in using this library nowadays, I made a fork of nanokina's repository addressing issues in building and also removing cmake-js and bindings npm for a lighter project.

https://github.com/ThePedroo/node-libsamplerate

I don't plan to create a PR to fix those issues, but if the repository maintainer sees this, feel free to commit my fixes.

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