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

Add Airplay 2 support - Upgrade shairport version #621

Conversation

fabienheureux
Copy link
Contributor

@fabienheureux fabienheureux commented Jan 22, 2023

Add AirPlay 2 support

Description

Following #607
This is an attempt to bring airplay 2 support to balena sound.

What's working ?

Current setup

  • a Raspberry pi 4 + hifiberry DAC, that is my current multiroom master
  • a raspberry pi zero w + hifiberry miniamp, that is a multiroom client

Features tested

  • Streaming to a single client
  • Streaming to multiple clients at once
  • Select volume on each client

I tested all this with SOUND_MODE=standalone and SOUND_MODE=MULTI_ROOM or SOUND_MODE=MULTI_ROOM_CLIENT

Issues

I noticed even before this upgrade that my raspberry pi zero had high CPU usage, and the sound was stuttering while streaming as a multiroom client.
I was not able to properly test the sound output for this device, the stuttering persisted after the shairport's version upgrade. I am pretty sure this is a different issue, but I would be glad if other users tested on their properly working Rpi zero to ensure this is not an issue caused by these changes.

How to test it locally

  1. Install Balena Cloud CLI
  2. Clone my repo git clone -b feature/bring-airplay-2-support https://github.com/fabienheureux/balena-sound.git
  3. Go to the repo directory cd balena-sound
  4. Publish it to your fleet using the cli balena push YOUR_FLEET_NAME
  5. Check on the dashboard that airplay is downloaded and properly restarted
  6. Enjoy 🎉

@fabienheureux
Copy link
Contributor Author

fabienheureux commented Jan 22, 2023

Note to self : first try, got warnings & errors below

airplay  warning: could not acquire a Shairport Sync native D-Bus interface "org.gnome.ShairportSync.i8" on the system bus.
airplay  warning: could not acquire an MPRIS interface named "org.mpris.MediaPlayer2.ShairportSync.i8" on the system bus.
airplay  fatal error: Can't access NQPTP! Is it installed and running?

Edit : This was fixed by starting NQPTP before shairport-sync

@fabienheureux
Copy link
Contributor Author

The current version works on my devices, I would be glad to have a review at that point 😇

@fabienheureux fabienheureux marked this pull request as ready for review January 22, 2023 12:11
@jellyfish-bot jellyfish-bot changed the title Add Airplay 2 support - Upgrade shairport version Add Airplay 2 support - Upgrade shairport version Jan 22, 2023
@fabienheureux fabienheureux mentioned this pull request Jan 23, 2023
@laytonhayes
Copy link
Contributor

Just tested this on a RasPi3 with HiFiBerry AMP2 and RasPi3 with USB DAC. Works great!

@maggie44
Copy link
Contributor

maggie44 commented Jan 24, 2023

Note to self : first try, got warnings & errors below

airplay  warning: could not acquire a Shairport Sync native D-Bus interface "org.gnome.ShairportSync.i8" on the system bus.
airplay  warning: could not acquire an MPRIS interface named "org.mpris.MediaPlayer2.ShairportSync.i8" on the system bus.
airplay  fatal error: Can't access NQPTP! Is it installed and running?

Edit : This was fixed by starting NQPTP before shairport-sync

Wow, thanks for looking in to this!

I had a read through, it seems that the NQPTP service is already scheduled to start in the container (https://github.com/mikebrady/shairport-sync/blob/master/docker/etc/s6-overlay/s6-rc.d/03-nqptp/run), but we override this in our script ENTRYPOINT []. That still makes sense, but now we have two processes running in one container. I can see that it would function, but it would be harder to debug because the logs of the first service (NQPTP) may not make it back up without running as PID1, and when it comes to shutdown it will probably end up getting killed. Hard to predict exactly what would happen in these circumstances, but it's never ideal.

I think the easiest way, without deviating too far from how it already is built by the maintainer, would be to use supervisor (apk add supervisor). We could use s6 like they do, but I would really like to try and keep it as simple and user friendly as possible, and that's probably using Superisor. It adds a bit of weight to the image sizes, but negligible.

Here is a good guide on implementation: https://towardsdatascience.com/run-multiple-services-in-single-docker-container-using-supervisor-b2ed53e3d1c0. Do you think this is something you would be able to add in to this PR? It would mean that both services are started, but that they are all connected to the initial running process and are managed properly by balena-engine.

@maggie44
Copy link
Contributor

I also noticed it is running with #!/bin/ash. I know this isn't your addition, but as you are refactoring could you switch that to sh for us?

#!/usr/bin/env sh

/bin/sh ...

@fabienheureux
Copy link
Contributor Author

I was not clear about what was started by the container actually, without the manual start of NQPTP.
I will look into this, I am not very familiar with this kind of development (being more a web developper usually), so no problem with doing these changes 👍

@fabienheureux
Copy link
Contributor Author

I made some changes @Maggie0002 but we might have an issue here.
It appears that shairport-sync can't be demonized with the default build.

See the logs below (sorry for the typo in shairpot 😅)

24.01.23 12:08:16 (+0100)  airplay  2023-01-24 11:08:16,860 INFO supervisord started with pid 7
24.01.23 12:08:17 (+0100)  airplay  2023-01-24 11:08:17,866 INFO spawned: 'nqptp' with pid 9
24.01.23 12:08:17 (+0100)  airplay  2023-01-24 11:08:17,878 INFO spawned: 'shairpot-sync' with pid 10
24.01.23 12:08:18 (+0100)  airplay  shairport-sync was built without libdaemon, so does not support daemonisation using the -d, --daemon, -j or --justDaemoniseNoPIDFile options
24.01.23 12:08:18 (+0100)  airplay  2023-01-24 11:08:18,025 INFO exited: shairpot-sync (exit status 1; not expected)
24.01.23 12:08:19 (+0100)  airplay  2023-01-24 11:08:19,030 INFO success: nqptp entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
24.01.23 12:08:19 (+0100)  airplay  2023-01-24 11:08:19,040 INFO spawned: 'shairpot-sync' with pid 11
24.01.23 12:08:19 (+0100)  airplay  shairport-sync was built without libdaemon, so does not support daemonisation using the -d, --daemon, -j or --justDaemoniseNoPIDFile options
24.01.23 12:08:19 (+0100)  airplay  2023-01-24 11:08:19,104 INFO exited: shairpot-sync (exit status 1; not expected)
24.01.23 12:08:21 (+0100)  airplay  2023-01-24 11:08:21,118 INFO spawned: 'shairpot-sync' with pid 12
24.01.23 12:08:21 (+0100)  airplay  shairport-sync was built without libdaemon, so does not support daemonisation using the -d, --daemon, -j or --justDaemoniseNoPIDFile options
24.01.23 12:08:21 (+0100)  airplay  2023-01-24 11:08:21,181 INFO exited: shairpot-sync (exit status 1; not expected)
24.01.23 12:08:24 (+0100)  airplay  2023-01-24 11:08:24,196 INFO spawned: 'shairpot-sync' with pid 13
24.01.23 12:08:24 (+0100)  airplay  shairport-sync was built without libdaemon, so does not support daemonisation using the -d, --daemon, -j or --justDaemoniseNoPIDFile options
24.01.23 12:08:24 (+0100)  airplay  2023-01-24 11:08:24,257 INFO exited: shairpot-sync (exit status 1; not expected)
24.01.23 12:08:25 (+0100)  airplay  2023-01-24 11:08:25,260 INFO gave up: shairpot-sync entered FATAL state, too many start retries too quickly

I am not clear on the best approach here.
I checked supervisor docs and it does not seem possible to run an application in the foreground, as supervisor will already be.

@fabienheureux
Copy link
Contributor Author

We could build a custom docker image based on the official one, but it would mean deviate even farer from how it is currently built by the maintainer...

@maggie44
Copy link
Contributor

Have you tried with the --with-libdaemon flag?

mikebrady/shairport-sync#909

@maggie44
Copy link
Contributor

maggie44 commented Jan 24, 2023

Or --with-dbus-interface? Although they may actually be build time flags rather than runtime flags. Hmm..

mikebrady/shairport-sync#1137

@maggie44
Copy link
Contributor

Hang on, I'm a bit confused. Looking again, I think you are going from the script to SupervisorD. But SupervisorD should be PID 1. So we could either add that in as the CMD in the Dockerfile directly and then use it to call the service. Or alternatively, have SupervisorD call the .sh script. Probably the latter is the closest to the original configuration. So it would be SupervisorD is the first thing called by the Dockerfile, not the .sh script. Then SupervisorD will call the .sh script.

@fabienheureux
Copy link
Contributor Author

Yes they are build time flags, hence the suggestion to use a custom dockerfile, but this is not an ideal setup in my opinion.
I will try to use supervisor instead of the .sh script 👍

@fabienheureux
Copy link
Contributor Author

fabienheureux commented Jan 24, 2023

I have tried your suggestion and it works !
I rebased and am ready for a new review


Logs below

24.01.23 12:56:34 (+0100)  airplay  2023-01-24 11:56:34,612 INFO supervisord started with pid 1
24.01.23 12:56:35 (+0100)  airplay  2023-01-24 11:56:35,619 INFO spawned: 'nqptp' with pid 8
24.01.23 12:56:35 (+0100)  airplay  2023-01-24 11:56:35,626 INFO spawned: 'shairport-sync' with pid 9
24.01.23 12:56:35 (+0100)  airplay  Starting AirPlay plugin...
24.01.23 12:56:35 (+0100)  airplay  Device name: Jean-Michel Mangemicro
24.01.23 12:56:35 (+0100)  airplay  Starting Shairport Sync
24.01.23 12:56:35 (+0100)  airplay  Shairport-sync started. Device is discoverable as Jean-Michel Mangemicro
24.01.23 12:56:35 (+0100)  airplay  Warning: the option -u is no longer needed and is deprecated. Debug and statistics output to STDERR is now the default. Use "--log-to-syslog" to revert.
24.01.23 12:56:35 (+0100)  airplay  warning: could not acquire a Shairport Sync native D-Bus interface "org.gnome.ShairportSync.i10" on the system bus.
24.01.23 12:56:35 (+0100)  airplay  warning: could not acquire an MPRIS interface named "org.mpris.MediaPlayer2.ShairportSync.i10" on the system bus.
24.01.23 12:56:36 (+0100)  airplay  2023-01-24 11:56:36,811 INFO success: nqptp entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
24.01.23 12:56:36 (+0100)  airplay  2023-01-24 11:56:36,812 INFO success: shairport-sync entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)

@Neustradamus
Copy link

To follow!

@maggie44
Copy link
Contributor

maggie44 commented Jan 24, 2023

Looking solid to me, great work!

@laytonhayes, are you able to test this one for us too?

@laytonhayes
Copy link
Contributor

@Maggie0002, tested out the update with both RasPi3s, works great! I'm getting something at startup about a CRIT Supervisor, not sure where that's coming from. Here's the startup log:

airplay  2023-01-24 21:53:58,319 CRIT Supervisor is running as root.  Privileges were not dropped because no user is specified in the config file.  If you intend to run as root, you can set user=root in the config file to avoid this message.
airplay  2023-01-24 21:53:58,354 INFO supervisord started with pid 1
airplay  2023-01-24 21:53:59,365 INFO spawned: 'nqptp' with pid 8
airplay  2023-01-24 21:53:59,379 INFO spawned: 'shairport-sync' with pid 9
airplay  Starting AirPlay plugin...
airplay  Device name: HiFi
airplay  Starting Shairport Sync
airplay  Shairport-sync started. Device is discoverable as HiFi
airplay  Warning: the option -u is no longer needed and is deprecated. Debug and statistics output to STDERR is now the default. Use "--log-to-syslog" to revert.
airplay  warning: could not acquire a Shairport Sync native D-Bus interface "org.gnome.ShairportSync.i10" on the system bus.
airplay  warning: could not acquire an MPRIS interface named "org.mpris.MediaPlayer2.ShairportSync.i10" on the system bus.
airplay  2023-01-24 21:54:00,514 INFO success: nqptp entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)
airplay  2023-01-24 21:54:00,515 INFO success: shairport-sync entered RUNNING state, process has stayed up for > than 1 seconds (startsecs)

@fabienheureux
Copy link
Contributor Author

fabienheureux commented Jan 25, 2023

This is because we added a program (supervisor) that allows to control several processes, that adds this warning when ran as root.
@Maggie0002 would you be happy with me adding the user=root in supervisor config ?

Do you know if docker in balenaOS is ran in rootless mode ? I am not clear of the implication (and risks) of running supervisor as root here.

@maggie44
Copy link
Contributor

@Maggie0002 would you be happy with me adding the user=root in supervisor config ?

Yep, good idea. It is running as root already, and should be, so we can add user=root to silence the message.

Other changes :
- Update entrypoint to start NQPTP before starting shairport sync
- Add logs when starting both scripts inside entrypoint
- Update shairport-sync config to remove default flag

Remove flag as it is now the default

update supervisor config
@fabienheureux
Copy link
Contributor Author

I have made the changes, as well in this change I removed the --use-stderr flag as it is now the default in shairport-sync https://github.com/mikebrady/shairport-sync/blob/6240fd83707b77fe2fb2250f8136e530e36010d2/shairport.c#LL327C21-L327C21

@maggie44
Copy link
Contributor

This looks tons better, and adds a new feature! @fabienheureux, @laytonhayes, there is no way we would have been able to do this without your help, huge thanks!

@fabienheureux, this looks good to me, if you are happy with the tests and ready for me to pull the trigger and merge then say the word. 👍

@fabienheureux
Copy link
Contributor Author

Always happy to contribute !
Let's merge then 🤘

@maggie44 maggie44 merged commit 2158f38 into balena-io-experimental:master Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants