-
Notifications
You must be signed in to change notification settings - Fork 148
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
Switch the backend from mpg123 to libao #111
base: master
Are you sure you want to change the base?
Conversation
@LinusU @TooTallNate @Mexxxo Bringing this to your attention. Any help testing is much appreciated. Sorry for the delay, I had to take care of merging proper |
This is pretty cool @ezequielgarcia, great work! |
8579a91
to
1eee5d9
Compare
This commit is quite invasive, but results in a much more simplified code, by using libao's simpler API. Moreover, mpg123 was having issues and not always playing correctly, requiring extra (and annoying) care. Reference: TooTallNate#101 Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
libao refuses to work with 0-bytes buffers. Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
1eee5d9
to
1e9e90f
Compare
|
||
## Installation | ||
|
||
Simply compile and install `node-speaker` using `npm`: | ||
You need `libao` installed on your system before installig `node-speaker`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possibly a bit of a drawback, I kind of liked the approach of bundling the source to the lib.
How do we feel about just adding the libao source code to this repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bundle of joy
Hum, well, I always thought bundling was a Bad Idea, but I see your point about ease of use. So, I've done some thinking and reviewing my stand on bundling dependencies.
Here's what I've come up with
The whole point of switching to libao is to avoid bundled sources. The tl;dr version of why is because once you bundle the sources, you kind of own the dependency and get all the maintenance burden (which we already have, as the issues show). On the other side, proper external dependencies allow us to report issues in the right place and let the proper owners deal with it.
Conclusion
It is comforting for users, but I'm not willing the pay the price.
References:
Haven't had time to look at it properly, but it seems awesome 👍 |
Hi, I will try it this weekend and provide some feedback |
@ezequielgarcia finally I have tested your PR. I integrated it into a working system that used the speaker with mpg123 and the only diffenrence is now that the sound sometimes has a minimal delay (100ms) <--- I guess this is related to the not available (internal) method to flush the speaker. I actually found this a lack of the current module that it is not possible to immediatly stop the speaker, is it possible to include a method in this module? Things I have found: the links for Bugs and Homepage in the package.json are not pointing to the destination repo ;) That needs to be changed for accepting the PR. Also I would suggest to add a contributors array of LinusU and you, as you both are taking care of this, you deserve some props 👍 I tested it with node 6 on a raspberry pi. The package gets not installed if libao has not been installed before. Is it possible to improve the error message? Currently the only meaningful in the 50 lines of error is one line that says "no package ao found". |
Also an entry in the history.md is missing |
So I have your libao PR now since a month in use on my raspi. Till now I have not observed any reproducible way for this issue. Once I know more I will let you know. |
Hi guys @LinusU @TooTallNate @Mexxxo ! Sorry for the huge delay: new job, lots to do. I have spent quite a bit of time re-considering this PR, and re-considering libao in particular. Leaving aside @Mexxxo's issues, I am now changing my mind regarding libao. My biggest concern is that it seems kind of dead, kind of unmaintained. Last official release was v1.2.0 - January 27, 2014. There is some activity on github, last commit was in January, 2018. I am under the impression that mpg123 is slightly more maintained (although since we are bundling here, it means manual syncing is required). In the end, it seems nor mpg123, neither libao is super maintained, and that both come with their own set of issues. |
As already discussed in RFC #101 , here is my proposal to ditch
mpg123
and uselibao
instead. Carryingmpg123
meant maintaining it as a dependency, tracking down fixes, and worrying about potential upstreaming of changes or API changes.On the other hand, with
libao
as backend we can keep it as an external binary dependency, and not worry about maintaining it.libao
is a good match for node-speaker given its multiplatform support. Also, the API is very simple, which also matchesnode-speaker
simple needs.This PR removes some module exported functions, which are no longer meaningful with
libao
as backend:Tested with pulseaudio plugin on Archlinux. All tests passes: