Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd PortAudio support #14
Conversation
This comment has been minimized.
This comment has been minimized.
ryanleesipes
commented
Feb 26, 2016
This is pretty awesome. Any noticeable effect on performance? |
This comment has been minimized.
This comment has been minimized.
Great work, I have some comments but it's working fine on my Ubuntu machine. When running mimic with portaudio I do get some line noice, is this to be expected or have I got some incorrect settings? |
This comment has been minimized.
This comment has been minimized.
Great work! (And fast!) Let's see if this can be tested on Mac before we merge it. Is the output I get from my portaudio installation (had to build from source to get dev-files in ubuntu) or is it something that is expected? |
This comment has been minimized.
This comment has been minimized.
With respect to performance (@ryanleesipes was asking about), I haven't noticed any significant change. With respect to the annoying output, this 2012 page explains it great. Basically they claim this noise comes from ALSA configuration errors. Muting them may hide real errors and I would need to bypass PortAudio using libasound calls to do that. Not a clean fix available, as I see it. |
This comment has been minimized.
This comment has been minimized.
julianrichen
commented
Feb 26, 2016
@zeehio Just tested the patch on Debian Sid 64bit & portaudio19-dev 19+svn20140130-1, works. Thank you :) I'll be out most of the day with my Fedora 23 laptop so I'll try to compile and test that when I have free time. |
This comment has been minimized.
This comment has been minimized.
julianrichen
commented
Feb 26, 2016
On Fedora 23 with portaudio-19-22.fc23 it's reporting audio device is missing. Same message: $ ./bin/mimic -t "hello"
oss_audio: failed to open audio device /dev/dsp |
This comment has been minimized.
This comment has been minimized.
@julianrichen, do you have portaudio-devel installed? It is necessary to compile the portaudio module in mimic |
This comment has been minimized.
This comment has been minimized.
julianrichen
commented
Feb 26, 2016
@zeehio Oops, that was it. It works on Fedora 23 just fine. Edit, like someone else mentioned above I also see some notices:
|
This comment has been minimized.
This comment has been minimized.
ryanleesipes
commented
Mar 4, 2016
@julianrichen these notices are typical with PortAudio in my experience (we use it in Mycroft Core as well) |
This comment has been minimized.
This comment has been minimized.
The sound output is just fine, but the errors I get are a little different from the ones @julianrichen posted:
@zeehio (for clarification) is the |
zeehio
reviewed
Mar 5, 2016
} | ||
|
||
int audio_init_portaudio() { | ||
int err = Pa_Initialize(); |
This comment has been minimized.
This comment has been minimized.
zeehio
Mar 5, 2016
Author
Contributor
Before and after Pa_Initialize, add an error message to ignore harmless error messages from Pa_Initialize if audio works fine
zeehio
reviewed
Mar 5, 2016
/* Read audio from stream and play it to audio device, converting */ | ||
/* it to pcm if required */ | ||
if (CST_AUDIOBUFFSIZE <= 0) { | ||
cst_errmsg("play_wave_from_socket not supported with PortAudio\n"); |
This comment has been minimized.
This comment has been minimized.
zeehio
Mar 5, 2016
Author
Contributor
@LongBoolean This is the error message that would appear if play_wave_from socket was used.
The bin/mimic binary does not use it.
This comment has been minimized.
This comment has been minimized.
The printed messages depend on your audio devices, they are printed as portaudio lists the devices. On Sunday I will add an error message before and after messages are printed, so the user knows that if they hear the audio they dont have to worry about those errors. @LongBoolean, With respect to play_wave_from_socket, your errors are not related to that. I have added a couple of notes in the pull request so you can read what is the error that would be printed |
This comment has been minimized.
This comment has been minimized.
ryanleesipes
commented
Mar 16, 2016
Is this working properly? If so let's go ahead and merge it. |
This comment has been minimized.
This comment has been minimized.
@ryanleesipes I believe we still lack Mac OS testing... it boils down to
|
This comment has been minimized.
This comment has been minimized.
What platforms has it been tested on? @ryanleesipes Should we wait until we know it works on them or solve that problem when we come to it? |
This comment has been minimized.
This comment has been minimized.
I discussed this with @LongBoolean a while ago. We wanted to test it out on OS X and Windows before merge. |
This comment has been minimized.
This comment has been minimized.
ryanleesipes
commented
Mar 16, 2016
This is a really good idea. Let me get @communitywireless to test it on Windows. Then we need to find someone to test it on Mac. @LongBoolean should have a Mac show up at his house shortly. |
This comment has been minimized.
This comment has been minimized.
julianrichen
commented
Mar 17, 2016
If you need another Windows tester I can switch to my Windows partition. However, I have never compiled under Windows (only Linux) so I can't guarantee anything. I'll try and give it a go tomorrow later in the day. I assume all you need is MinGW & Automake installed? |
This comment has been minimized.
This comment has been minimized.
@julianrichen I'm not quite sure of all that a windows install will require, thats part of the fun!!! |
This comment has been minimized.
This comment has been minimized.
julianrichen
commented
Mar 18, 2016
I've used Windows for a long a time but never for development, this is very much different then an unix environment when it comes to setting-up a dev environment. Only had a bit of time trying things out but still not sure what I'm doing... but as you said, it's part of the fun! I'll try again on the weekend. Hopefully someone with more experience can also test. |
ryanleesipes
added
the
in progress
label
Mar 23, 2016
This comment has been minimized.
This comment has been minimized.
@zeehio
I have already installed portaudio with brew. Any ideas? |
This comment has been minimized.
This comment has been minimized.
can you run I did not consider that brew installs software out of the default paths, I have to use pkg-config inside the configure script to append the right build flags accordingly |
This comment has been minimized.
This comment has been minimized.
after installing pkg-config with
And the other command gives this error: |
This comment has been minimized.
This comment has been minimized.
Can you find the portaudio.h in your file system? |
This comment has been minimized.
This comment has been minimized.
portaudio.h is in |
This comment has been minimized.
This comment has been minimized.
I hope I have fixed it with the last commit.. Otherwise, you may want to It's hard to fix stuff without being able to properly test things. Thanks @LongBoolean (and everyone) for your patience. |
This comment has been minimized.
This comment has been minimized.
We have sound!!!!
I don't know how big of a deal this is but it is good to know. |
This comment has been minimized.
This comment has been minimized.
The warning is related to this issue Future PortAudio versions may deal with this. I will do a final review tomorrow, make sure instructions and documentation are clear and merge |
This comment has been minimized.
This comment has been minimized.
I have not tested it out with mycroft on the mac to see if they play nicely together. (I still need to get mycroft set up on the mac) It may not be a big deal, so if you really want to merge to development before I get that tested that is fine by me. |
This comment has been minimized.
This comment has been minimized.
Wooo! works on windows as well! I've added However, if you have a working osx and linux I say merge! Windows isn't compilable without some modifications (took me the weekend to figure out) and I can make the modifications needed for portaudio in my pull request. |
zeehio commentedFeb 25, 2016
This pull request aims to close issues #10 and #13.
What does it do?
Main changes:
play_wave
function needs to be edited to pass the whole buffer of samples at once.play_wave_sync
andplay_wave_from_socket
have not been adapted to work with PortAudio, mainly because I don't think we are using them.Testing
brew install portaudio
is required on Mac OS to install portaudio, as far as I know.