search

audiojs / audio-speaker

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

Own speaker implementation #2

Open dy opened 8 years ago

dy commented 8 years ago

npm install audio-speaker gives 

C:\Users\dmitry\Dropbox\Projects\audio-through\node_modules\nan\nan.h(41): fatal error C1189: #error :  This version of
 node/NAN/v8 requires a C++11 compiler [C:\Users\dmitry\Dropbox\Projects\audio-through\node_modules\speaker\build\bindi
ng.vcxproj]

This happens only when local version of node-gyp is used. Probably some config needed or something. Because global node-gyp rebuilds speaker just fine.

dy commented 8 years ago

Or maybe that is a perfect reason to study prebuild. Because it’s annoying to wait for TooTallNate to merge speaker as well as the whole situation with absence of sound in node is a sort of crap - it’s 2016, we’ve sent satellites to Mars.

dy commented 8 years ago

speaker also really needs .npmignore. Guess we need own implementation.

vectrixdevelops commented 8 years ago

I can work on this if you'd like? @dfcreative

dy commented 8 years ago

Wow, that would be awesome

vectrixdevelops commented 8 years ago

I'm just creating a mirror of the mpg123 svn repo to git, so we can easily keep the library up to date, give credit to those who made it and let people modify it through git. Then we can add it here as a submodule and make the binding.

I'm not the best with C++ and C, but we'll put it on a separate branch in case someone else can contribute to it if I can't make it.

dy commented 8 years ago

Yeah, that is good to have mpg123 updated automatically. The point as I get it is not C/C++ but node bindings, that takes good knowledge of v8. Speaking practice, running build on npm install is always a headache for npm users. For example, for now to use node-speaker for windows users we need installing microsoft SDKs, which are 7Gb of extra space, just for tiny module. I tried to research node prebuild to avoid that, but that was too difficult for me..

vectrixdevelops commented 8 years ago

Alright, I will look into prebuild and yeah the bindings will be difficult, but it's worth giving it a shot. For windows building maybe this is a temporary fix https://www.npmjs.com/package/windows-build-tools although I have found it doesn't set the python path very well, although they are working on a fix I believe. It also requires cmd or powershell to run in admin.

vectrixdevelops commented 8 years ago

I have got a mirror setup now. I will start working on the binding. https://github.com/connorhartley/mpg123

vectrixdevelops commented 8 years ago

Just some information on what I've done. Basically I have forked mpg123-mirror from my profile to audiojs to make modifications specifically for audio-speaker. Currently binding-implementation compiles on Windows fine. I am going to focus on finishing the binding for Windows then we will worry about porting this to other operating systems and arch types. I will need help with testing this on the other systems afterwards.

dy commented 8 years ago

Yeah, I am also on windows, cc @jamen

jamen commented 8 years ago

Linux Mint here.

vectrixdevelops commented 8 years ago

The C++ binding stuff is done, now all we'll need is the JS implementation for this. I'm open to suggestions on implementation. @dfcreative @jamen @ahdinosaur @mmckegg

dy commented 8 years ago

@connorhartley The branch seems to be an orphan, it has no master’s history. For pull-request we will have to create a new branch I guess.

Also now it’s a bit tricky to figure out how to install mpg123. That would be nice to add that to readme.

Also that is must-have package.json with build script.

Also I do

node-gyp clean;
node-gyp configure;
node-gyp build;

...

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.Platform.targets(55,5): error MSB8020: The build tools for Visual Studio 2010 (Platform Toolset = 'v100') cannot be found. To build using the v100 build tools, please install Visual Studio 2010 build tools.  Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\Users\dmitry\Dropbox\projects\audio-\speaker\build\src\module.vcxproj]

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.Platform.targets(55,5): error MSB8020: The build tools for Visual Studio 2010 (Platform Toolset = 'v100') cannot be found. To build using the v100 build tools, please install Visual Studio 2010 build tools.  Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\Users\dmitry\Dropbox\projects\audio-\speaker\build\src\compat.vcxproj]

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.Platform.targets(55,5): error MSB8020: The build tools for Visual Studio 2010 (Platform Toolset = 'v100') cannot be found. To build using the v100 build tools, please install Visual Studio 2010 build tools.  Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\Users\dmitry\Dropbox\projects\audio-\speaker\build\src\mpg123.vcxproj]

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.Platform.targets(55,5): error MSB8020: The build tools for Visual Studio 2010 (Platform Toolset = 'v100') cannot be found. To build using the v100 build tools, please install Visual Studio 2010 build tools.  Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\Users\dmitry\Dropbox\projects\audio-\speaker\build\src\out123.vcxproj]

Not sure what’s up.

vectrixdevelops commented 8 years ago

@dfcreative Yeah I will rebase before I merge to master.

I will add the readme very soon, along with instructions for configuring mpg123 a long with build instructions.

What version of Visual Studio are you using?

EDIT

The error can be caused by multiple reasons, try the following.

npm install --msvs_version=2015

Alternatively to save time in the future for building, you can set an environment variable.

Variable: GYP_MSVS_VERSION Value: 2015

If that doesn't work, I would take a look at the other options in this SO answer I found. http://stackoverflow.com/questions/32556295/npm-install-error-the-build-tools-for-v120-platform-toolset-v120-cannot

If that doesn't work, let me know. :smile:

dy commented 8 years ago

With vs2015 flag:

C:\Users\dmitry\Dropbox\projects\audio-\speaker\src\mpg123\src\libout123\out123_int.h(12): fatal error C1083: Cannot op
en include file: 'config.h': No such file or directory [C:\Users\dmitry\Dropbox\projects\audio-\speaker\build\src\modul
e.vcxproj]
  compat.c
  compat_str.c
..\..\src\mpg123\src\compat\compat.c(12): fatal error C1083: Cannot open include file: 'config.h': No such file or dire
ctory [C:\Users\dmitry\Dropbox\projects\audio-\speaker\build\src\compat.vcxproj]
c:\users\dmitry\dropbox\projects\audio-\speaker\src\mpg123\src\compat\compat.h(17): fatal error C1083: Cannot open incl
ude file: 'config.h': No such file or directory (compiling source file ..\..\src\mpg123\src\compat\compat_str.c) [C:\Us
ers\dmitry\Dropbox\projects\audio-\speaker\build\src\compat.vcxproj]
  parse.c
...

Basically config.h is not found, feels like mpg123 needs to be configured or something.

vectrixdevelops commented 8 years ago

@dfcreative Could you paste the whole error (specifically gyps info) as I can't seem to reproduce the error?

EDIT

The latest commit may fix your problem, as the target arch default was ia32 (maybe your arch type is also ia32) and I hadn't made a configuration for it. If you are running x64 use --target-arch=x64 when building and configuring. Note: I will put this information in the readme after.

dy commented 8 years ago

@connorhartley btw considering the latest changes in @audiojs, for own implementation it would be enough to have write(buffer, cb) function, not necessary to implement stream wrapper

vectrixdevelops commented 8 years ago

@dfcreative did that error get fixed?

dy commented 8 years ago

Naah, still get the same error - config.h not found. I just copied the code of audiojs/mpg123 into src/mpg123 folder, I guess it has to be configured or something... When I tried to do the same thing before with updated mpg123 I got the same error and just gave up

vectrixdevelops commented 8 years ago

@dfcreative have you tried setting the --target-arch flag in the build and configure command?

e.g node-gyp configure --target-arch=x64 node-gyp build --target-arch=x64

If the problem persists please let me know. :smile:

dy commented 8 years ago

Btw weird that --target-arch (dash) but --msvs_version (lodash). But the problem is still the same:

cannot find 'config.h' : No such file or directory (compiling source file ..\..\src\mpg123\src\libout123\legacy_module.c) [C:\Users\dmitry\Dropbox\projects\audio-\speaker\build\src\out123.vcxproj]
dy commented 8 years ago

Possible instruction to get started:

Clone this repo: git clone git@github.com:audiojs/audio-speaker.git Switch to the speaker directory: cd audio-speaker Initialize the mpg123 submodule: git submodule init Update the mpg123 submodule: git submodule update Install npm dependencies: npm install Run node-gyp to generate build scripts: npm run build

vectrixdevelops commented 8 years ago

Ah my bad, they're both low dash. You may want to try that again. Sorry :tired_face:

dy commented 8 years ago

I tried all of the possible options, the error persists. I did exactly what described in steps.

vectrixdevelops commented 8 years ago

Alright, I will try mess around with the build configuration. Thanks for the help! :+1:

vectrixdevelops commented 8 years ago

@dfcreative I should've fixed the missing config.h issue now. Let me know if the problem persists.

vectrixdevelops commented 8 years ago

@dfcreative Feel free to add your name to the credits in readme. 😄

vectrixdevelops commented 8 years ago

@dfcreative These latest changes should have the audio-lena test working good with the command npm test let me know if it works for you.

EDIT: With the additional debug messages type set DEBUG=speaker in the same terminal instance as the following commands.

dy commented 8 years ago

Hi @connorhartley! Thank you a lot for the work, I really appreciate your activity. I will try to look at it soon

vectrixdevelops commented 8 years ago

No problem! @jamen could you also test this, please. I will get a configuration for MacOS soon. Browser implementation still needs to be added and documentation.

jamen commented 8 years ago

It works. :+1:

vectrixdevelops commented 8 years ago

I added node stream and pull stream implementation as well as made some improvements to tests and the write function. I think that the browser implementation should be a separate module because mpg123 is a large library that isn't required for browsers and can take up unnecessary room. @dfcreative and @jamen what're your thoughts?

jamen commented 8 years ago

Sounds good to me. :)

jamen commented 8 years ago

What if instead: I (or someone) separated the mpg123 binding into a package (i.e. audio-mpg123)? Then we could have that be an optionalDependency, so browser users could do:

npm install audio-speaker --no-optional

Then we could use is-browser to check and give a WAA one instead.

This would feel more graceful than a separate web-audio-speaker package. Because it would work on both platforms no matter what, with a simple way to exclude mpg123 on browser.

But then there is also the issue of this

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 300.0 DAI (300.0 USD @ $1.0/DAI) attached to it.

johnson73A commented 5 years ago

Good

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 1 week, 3 days from now. Please review their action plans below:

1) elemino has been approved to start work.

  1. Learn the complete history to gauge the scope of the issue and install the module to see how it behaves when it works.
  2. Reproduce the behavior in the environment in which the error occurs and understand to what extent the c++11 compile error does represent root cause.
  3. Understand addon bindings to see if the problem is caused due to node.js and or reconfigure the node-gyp build.
  4. Refactor module and add dependencies that are not currently present in the local install.

I would be absolutely thrilled to learn about and fix this issue.

Learn more on the Gitcoin Issue Details page.

2) anorakthagreat has applied to start work _(Funders only: approve worker | reject worker)_.

Believe I know how to fix this issue. Let me give it a go.

Learn more on the Gitcoin Issue Details page.

dy commented 5 years ago

cc @jamen @connorhartley ↑↑ :eyes: I've just asked gitcoin for grant.

Elemino commented 5 years ago

Please let me fix this. I can do it.

jamen commented 5 years ago

@dy @connorhartley I don't know how the bounty works, but I can approve something if needed.

@Elemino I should let you know that node-cubeb is what we want to use. See "Cubeb for audio-speaker" for more details. Cubeb is a better library than Mpg123 in about every aspect, put simply.

audio-mpg123 could be fixed, but it would be replaced by node-cubeb sooner than later, so bear that in mind. If you're more familiar with Mpg123 than Cubeb, and/or finishing node-cubeb becomes a larger task, then audio-mpg123 could let us see a functioning speaker module faster.

Edit: node-cubeb also has audio sources instead of just audio sinks, so its practically twice as useful to us.

Elemino commented 5 years ago

Okay, I will just start working on this I guess as I feel this burning desire. I've always dreamed of working with audio but I am not a trad C++ dev. I will not consider audio-mpg as part of the solution but it's written in JS so it's fun to go over the code. If node cubeb has audio sources then that must be good. I am going over the different types of streams.

anorakthagreat commented 5 years ago

Think I know how to fix this, it's just npm trying to be smart. I applied for work through Gitcoin, but it didn't post my comment on here (weird).

gitcoinbot commented 5 years ago

@elemino Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Elemino commented 5 years ago

This is what happened when I first cloned the repo and ran 'npm install' https://gist.github.com/Elemino/e96ea60c737f791a2f8b37ac587516a7

fatal error C1189 number seem to be exclusive to win32. Interesting.

gitcoinbot commented 5 years ago

@elemino Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 5 years ago

@elemino Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

Elemino commented 5 years ago

Ok, it took me a week to learn all of the components related to the error. I'll even bisect to earlier commits if need be. There are a lot of clues all over the place. My bet is that it's either a v8 problem or a release pipeline node gyp generation problem where npm triggers it wrong or the problem happens strictly on the gyp configure side where there are leftover assumptions by the build system regarding manipulating the raw vcprojs. I will try and write a doc on how all of the components work together and start playing with a local new node-gyp build. I will get to the bottom of it sooner or later. It will hopefully help the cubeb implementation in the future as well. Cubeb is an alternative to v8, am I right?

jamen commented 5 years ago

Here is an image to try and illustrate the role of different parts

image

In a nutshell, a native binding to an audio library plus Web Audio API to support audio output across any JavaScript context.

We're using Mpg123 in audio-mpg123 and Cubeb in node-cubeb, but only one of these is needed. They're also developed with NAN and N-API respectively. N-API being newer and easier to work with.

All errors are probably an issue with these packages (likely not V8 or Npm as suggested). There's points to be made about which will be easier to get stable and which is more useful.

Thank you for investigating! Let me know if there is more I can explain.

gitcoinbot commented 5 years ago

@elemino Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 5 years ago

@elemino Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days