Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

charlesomer
Copy link

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.

@systemcrash
Copy link
Member

systemcrash commented Apr 21, 2021

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.

parser.add_argument("-po", "--use-portaudio", required=False, help="Use port audio (useful for Windows and MacOS).", default=False)

should be

parser.add_argument("-po", "--use-portaudio", required=False, help="Use port audio (useful for Windows and MacOS).", default=True

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.

@systemcrash
Copy link
Member

I think, at a minimum, this commit should be split up into its components (README, ALSA, Docker, etc) and not all squashed into one.

@systemcrash
Copy link
Member

I think it's worth investigating why portaudio wasn't working (before what?)
e.g. here

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.

Now supports pyalsaaudio for devices such as Raspberry Pi instead of PortAudio which wasn't working before.

@Neustradamus
Copy link
Contributor

@systemcrash: Time to merge?

@charlesomer
Copy link
Author

Apologies for being inactive on this PR, it can be closed if need be :)

@systemcrash
Copy link
Member

@systemcrash: Time to merge?

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.

@charlesomer
Copy link
Author

charlesomer commented Jul 8, 2021

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!

@Joshfindit
Copy link

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.

@systemcrash
Copy link
Member

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.

@Joshfindit Would you care to give this PR a try? Wondering whether you can verify the docker portion of it.

@Neustradamus
Copy link
Contributor

@charlesomer: Can you rebase your PR?

After, who can test it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants