audiojs / audio-speaker

Output audio stream to speaker, browser/node-wise
103 stars 14 forks source link

Release 2.0 #27

Closed vectrixdevelops closed 7 years ago

vectrixdevelops commented 7 years ago

Adds in a native node binding to mpg123 with various improvements to the JavaScript implementation a long with the binding implementation (and a more up to date mpg123). These changes have been added to tests and docs. A lot has been changed so it seems fitting for this major version to be incremented.

Thanks to Dustin and Jamen for their contributions to supporting Linux and MacOS.

jamen commented 7 years ago

As far as the testing goes. Could we use some sort of small lib (maybe tape?). Just for logging I guess, and if you need to use any assertions.

Also a test for the browser code? I think @dfcreative uses this thing called budo, which I used a little bit, and is pretty cool. Forgot browser code will be in another repo!

jamen commented 7 years ago

The _validate had me confused for a bit. I think defaults should be set another way.

Maybe like this?:

  options = objectAssign({
    // defaults here
  }, options)

Validate makes me think it checks the options not does defaults and stuff.

vectrixdevelops commented 7 years ago

Is there anything I may be missing @dfcreative and @jamen (except for browser tests)?

dy commented 7 years ago

I gotta give it a look, hold on. Sorry for delay.

vectrixdevelops commented 7 years ago

Those can use this new update by npm install audio-speaker@next.

dy commented 7 years ago

In node works like a charm, awesome work! But browser tests seem to be broken now.

dy commented 7 years ago

@connorhartley I’ve made little changes if you don’t mind

vectrixdevelops commented 7 years ago

Awesome thanks! They look good. I never got around to testing browser yet, because @jamen wanted to do something first, but I think he changed his mind. I need to fix the build for unix systems due to the new prebuilt binary needs a separate build for each of the OS' supported. Then we can be looking at a merge.

dy commented 7 years ago

@connorhartley ideally we ought to pass all classic audio-speaker test cases (I’ve resurrected them). Because they pretty much test regression from audio-streams practice.

vectrixdevelops commented 7 years ago

I maybe went a bit excessive with the releases of audio-mpg123, sorry everyone. Finally I can stop doing that now since everything is now working fine (releasing was the only way to test the pre-built binaries). This will work for node versions 4 to 7 for Windows x64 and Linux x64 - MacOS and Windows ia32 builds coming soon. Other platforms have a fallback to be built manually instead of using the pre-built binary.

dy commented 7 years ago

@connorhartley audio-mpg123 is reasonable package, that allows for managing version independent of audio-speaker. Maybe just the name a bit misleading, because conceptually it is like node-speaker, from audio-mpg123 it is not clear, is it a encoder/decoder, player or etc. Also I would elaborate readme for that, because now it is not clear how to use that.

vectrixdevelops commented 7 years ago

@dfcreative I added some information into audio-mpg123. Let me know if you think I should add more.

dy commented 7 years ago

@connorhartley yeah, I meant is it possible to describe how we use it from js-land? Like a simple stream, writer function, should we instantiate it or whatever?

jamen commented 7 years ago

I originally called for audio-mpg123 not really for people to use on their own, but so we could have it be a optional dependency, so browser users don't have to download it (using --no-optional).

It's basically the binding for node that was here, plopped into another package. Kind of like Leveldown, just the binding. And speaker would be like Levelup.

I'm all for documenting it though, but probably low priority in my mind. Certainly people can use it on their own if they want to.

dy commented 7 years ago

@jamen ok, I see, but still even for inner purposes for the future/other contribs it is good practice to explain minimally what is that and how it can be used in simplest form. Or you think it is not of any use except for audio-speaker?

vectrixdevelops commented 7 years ago

@dfcreative it would be fit for other purposes, but it is a little hard to document. Probably at a later date I could do that. I did make a big note that this is mainly for audio-speaker and only those who understand how bindings work should use audio-mpg123 directly. Otherwise this provides a very easy layer for them to work with.

jamen commented 7 years ago

@dfcreative Okay, I agree docs would be helpful for understanding use here in audio-speaker.

I can't really say if it is useful outside of audio-speaker. People could use it if they want really minimal speaker interaction inside Node. But audio-speaker is probably more appealing in most cases because it is just a higher level wrapper that supports browser too.

vectrixdevelops commented 7 years ago

Is that cool if we merge this and work on the rest from master? @dfcreative @jamen

Just browser tests to go and a clean up.

jamen commented 7 years ago

I think merging on master now before releasing would be confusing for people coming from npm. Kind of trivial in my mind though so I'll let you guys decide. I don't mind working on this branch until we release.

Speaking of browser: I tried to test it yesterday and couldn't get it to work. I'm not sure if it is web-audio-stream or me, and there is no errors at all, so I'm not really sure where to go from there. I took a peak at web-audio-stream/writer and it seems like it should work.

dy commented 7 years ago

Yeah, let's make sure all classic tests pass before merging, also we might be willing to compare code in master with the current branch for fixing bugs, so it is a bit too early to merge. That would not be too nice to have not working code in master.

vectrixdevelops commented 7 years ago

This PR will be re-established on the release-2.0-browser branch.