FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Switch the backend from mpg123 to libao by ezequielgarcia · Pull Request #111 · TooTallNate/node-speaker · GitHub

/ node-speaker Public

Switch the backend from mpg123 to libao - #111

Open
ezequielgarcia wants to merge 3 commits into
TooTallNate:masterfrom
ezequielgarcia:master
ConversationCommits (3)ChecksFiles changed
Open

Switch the backend from mpg123 to libao#111
ezequielgarcia wants to merge 3 commits into
TooTallNate:masterfrom
ezequielgarcia:master

Conversation

ezequielgarcia commented Feb 1, 2018

Copy link
Copy Markdown

As already discussed in RFC #101 , here is my proposal to ditch mpg123 and use libao instead. Carrying mpg123 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 matches node-speaker simple needs.

This PR removes some module exported functions, which are no longer meaningful with libao as backend:

Speaker.api_version
Speaker.description
Speaker.module_name
Speaker.getFormat
Speaker.isSupported

Tested with pulseaudio plugin on Archlinux. All tests passes:

$ npm test
  exports
    ✓ should export a Function

  Speaker
    ✓ should return a Speaker instance
    ✓ should be a writable stream
    ✓ should emit an "open" event after the first write()
    ✓ should emit a "close" event after end()
    ✓ should only emit one "close" event


  6 passing (34ms)

ezequielgarcia commented Feb 1, 2018

Copy link
Copy Markdown
Author

@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 dlopen support for libao plugins to work everywhere. It happened in nodejs/node#12794, and got included in node 9.

TooTallNate commented Feb 1, 2018

Copy link
Copy Markdown
Owner

This is pretty cool @ezequielgarcia, great work!

ezequielgarcia force-pushed the master branch 5 times, most recently from 8579a91 to 1eee5d9 Compare February 1, 2018 21:03
Switch the backend from mpg123 to libao
f942ef3 
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>
Fix test and example style as per standardjs
7b53eb9 
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
test: Use 1k size buffers
1e9e90f 
libao refuses to work with 0-bytes buffers.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
LinusU reviewed Feb 3, 2018
View reviewed changes
Comment thread README.md
## Installation

Simply compile and install `node-speaker` using `npm`:
You need `libao` installed on your system before installig `node-speaker`.

LinusU Feb 3, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

ezequielgarcia Feb 3, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

LinusU commented Feb 3, 2018

Copy link
Copy Markdown
Collaborator

Haven't had time to look at it properly, but it seems awesome 👍

Flowr-es commented Feb 8, 2018

Copy link
Copy Markdown

Hi, I will try it this weekend and provide some feedback

Flowr-es commented Mar 13, 2018

Copy link
Copy Markdown

@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".

Flowr-es commented Mar 13, 2018

Copy link
Copy Markdown

Also an entry in the history.md is missing

Flowr-es commented Apr 16, 2018

Copy link
Copy Markdown

So I have your libao PR now since a month in use on my raspi.
What I could observe that there is sometimes (not very often) a short loud noise (like 1 second).
Maybe it is related to some internal methods I'm using of the speaker module or that some audio chunks are somehow misinterpretet. Nevertheless with the same code using the mpg123 speaker module I had not such issues.

Till now I have not observed any reproducible way for this issue. Once I know more I will let you know.

ezequielgarcia commented May 31, 2018

Copy link
Copy Markdown
Author

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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

LinusU LinusU left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL