-
Notifications
You must be signed in to change notification settings - Fork 134
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
Docker image, pyalsaaudio, audio device etc. #25
base: master
Are you sure you want to change the base?
Conversation
Whoa horsie. That's a big un. The problem with this PR as is, is that you've now gone and changed defaults that others hitherto rely on - so you risk confusing or breaking this for others. So if you want to include the new functionality for ALSA, those should be explicitly specified at startup, and the existing pyaudio left as default, until a time that there's consensus that ALSA is a good thing to have (as default). 😄 Isn't there a new kid on the Linux audio manager block? So e.g.
should be
I'm refining my PR for audio functionality - and this change will break how my PR determines latency. Does ALSA support latency determination? Other than that, without testing this myself, my comments remain abstract. But it looks like a solid effort. |
I think, at a minimum, this commit should be split up into its components (README, ALSA, Docker, etc) and not all squashed into one. |
I think it's worth investigating why portaudio wasn't working (before what?) Portaudio is cross-platform, while ALSA is a bunch of technical debt (for other platforms) since there is some code exclusive to it in your PR.
|
@systemcrash: Time to merge? |
Apologies for being inactive on this PR, it can be closed if need be :) |
Some portions of the PR have use - although we should not change the default from portaudio. If @charlesomer undertakes this change, and it does not affect how things currently work after testing, we can consider adding it. |
I've made some very quick changes which I haven't tested but should change the default back to PulseAudio :) It will almost definitely need tidying up at some point! |
Just a head’s up: I don’t know if the situation on macOS has changed, but Docker networking in macOS was such a different beast that it actually meant that some docker containers did not work properly. VMs are probably less hassle for anyone who wants multiple endpoints on the same silicon. |
@Joshfindit Would you care to give this PR a try? Wondering whether you can verify the docker portion of it. |
@charlesomer: Can you rebase your PR? After, who can test it? |
b88bcc6
to
57e4a3b
Compare
52f827e
to
017e620
Compare
Let me know what you think of this PR. Obviously things like the README and GitHub actions would need to be modified but I wasn't sure what they would need to be changed to at the moment.
Now supports
pyalsaaudio
for devices such as Raspberry Pi instead of PortAudio which wasn't working before. This means volume control now works as well. PortAudio still exists and can be enabled.Automated docker builds, it takes about 2 hours to build upon a push to master but requires docker hub credentials etc.