search

openairplay / airplay2-receiver

AirPlay 2 Receiver - Python implementation
2.13k stars 133 forks source link

Docker image, pyalsaaudio, audio device etc. #25

Open charlesomer opened 3 years ago

charlesomer commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@systemcrash: Time to merge?

charlesomer commented 3 years ago

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

systemcrash commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@charlesomer: Can you rebase your PR?

After, who can test it?