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 support for asoundrc #2124

Merged
merged 5 commits into from
Nov 30, 2024

Conversation

mcrosson
Copy link
Contributor

Some x86_64 mini pc's need an ~/.asoundrc file within the viewer container for sound output to work. This PR enables support for setups that need an ~/.asoundrd.

This has been tested with a MeLE PCG02 Mini PC 'Stick'. The ~/.asoundrc this device needs is:

pcm.!default {
    type hw
    card 0
    device 1
}

ctl.!default {
    type hw
    card 0
    device 1
}

@mcrosson mcrosson requested a review from a team as a code owner November 12, 2024 16:22
@nicomiguelino
Copy link
Contributor

@mcrosson, thanks for creating a pull request!
I'll go test it on my x86 NUC-like device and on my Pi devices as well (to ensure nothing breaks).

@nicomiguelino nicomiguelino removed the bug label Nov 12, 2024
Copy link
Contributor

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Card and device numbers will vary across devices, so .asoundrc needs to be dynamic too.
  • Have you checked if there's audio on your mini PC's 3.5mm jack? If there's no .asoundrc config (or the config is blank), if could be that it defaults to the audio jack.

@@ -37,6 +37,7 @@ services:
restart: always
volumes:
- resin-data:/data
- /home/${USER}/.asoundrc:/data/.asoundrc
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you've accidentally placed this in the anthias-server section. Please move this to anthias-viewer.

Copy link
Contributor Author

@mcrosson mcrosson Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that, I've updated the PR accordingly. If you'd like me to squash the commits just let me know.

Copy link
Contributor

@nicomiguelino nicomiguelino Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be fine. We can just squash and merge later if this pull request gets approved

@mcrosson
Copy link
Contributor Author

mcrosson commented Nov 13, 2024

Card and device numbers will vary across devices, so .asoundrc needs to be dynamic too.

I think for the moment an empty file (by default) that users can adjust is sufficient. Users can docker exec -it viewer bash and work inside the viewer container to find the sound device setup needed and then adjust .asoundrc file accordingly.

I tracked down what .asoundrc I needed using the viewer container and trying things from various forums and helps sites. Most of the troubleshooting steps I found relied on aplay, alsamixer and .asoundrc and it seemed like there isn't a great way to make .asoundrc dynamic unless you show users a full list of devices to choose from.

Thinking in a more 'long-term' way I could imagine updating the web gui Audio output drop down to show a list of all valid sound devices on the system. However, that is likely a much larger change and outside my capabilities.

Have you checked if there's audio on your mini PC's 3.5mm jack? If there's no .asoundrc
config (or the config is blank), if could be that it defaults to the audio jack.

I did not have the ability to test the 3.5mm jack when developing the patch. If I get creative, I can probably test it sometime in the next few days. Just let me know if you'd like me to give it a test.

Even if the 3.5mm jack does work, I still need to use HDMI audio for my production deployment(s) which will require me to have an .asoundrc (or equivalent functionality).

@mcrosson
Copy link
Contributor Author

I was able to carve out some time to do a quick test of this device with no .asoundrc and an empty .asoundrc. Both forms resulted in the following error. I have a feeling this device just has an 'odd' form of sound setup that'll require tuning the audio setup via .asoundrc or equivalent.

SDL_OpenAudio (2 channels, 44100 Hz): ALSA: Couldn't open audio device: No such file or directory
SDL_OpenAudio (1 channels, 44100 Hz): ALSA: Couldn't open audio device: No such file or directory
```

@nicomiguelino
Copy link
Contributor

@mcrosson, I;m still thinking abouit options. We can either let users do one of the following:

  • Manually change the .asoundrc file
    • This is fine for devices running Raspberry Pi OS or Debian.
    • If we'll add support for BalenaOS as well, this won't be an option.
  • Let the user edit the .asoundrc file via the web UI (for instance, using textarea), on the settings page.
    • This is okay if we'd like users to have more control, but might be overwhelming to some users.
  • Have the back-end fetch the card and devices info, but depending on the device, it's hard to know which device/cards are for HDMI, 3.5mm, etc.
    • If we are to go down this path, we might do a fair bit of Python programming.

@mcrosson
Copy link
Contributor Author

Understood re evaluating options.

For the web ui text field: could you set it up to have a url parm to enable editing of the file and/or have it setup behind an 'advanced sound options (you probably dont want this)' toggle or similar? Such an approach could be a good compromise for the time being.

I think the python approach may be 'best' long-term but I have a feeling that'll involve a ton of programming...

Copy link
Contributor

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcrosson, would you be able to make changes so that:

  • .asoundrc will be created from the viewer docker container instead?
    • Make sure to store it inside /data.
    • In the viewer container, $HOME is set to /data. That path is mounted as a volume, so it will be persisted.

If the viewer container will take care of creating an .asoundrc file (if it doesn't exist yet), changes to the Ansible files can be removed.

@mcrosson
Copy link
Contributor Author

@nicomiguelino

  • .asoundrc will be created from the viewer docker container instead?

The .asoundrc file cannot be instantiated by the container with the way the /data directory is setup currently. My understanding and experience with Docker is that a file must exist prior to the container launching otherwise Docker will auto-create a directory instead of a file at the source path. If there is a way to force Docker to create a file instead of a folder, I can make that update but I've never seen a way to do this.

The other option would be to bind mount the whole of /data but I don't think that's a desired behavior.

  • Make sure to store it inside /data.
  • In the viewer container, $HOME is set to /data. That path is mounted as a volume, so it will be persisted.

This is already addressed by the docker-compose.yml.tmpl changes in the PR.

@nicomiguelino
Copy link
Contributor

@mcrosson

The other option would be to bind mount the whole of /data but I don't think that's a desired behavior.

Although there's no bind mount for /data, there's a volume mount for that (line 67):

anthias-viewer:
image: screenly/anthias-viewer:${DOCKER_TAG}-${DEVICE_TYPE}
build:
context: .
dockerfile: docker/Dockerfile.viewer
mem_limit: ${VIEWER_MEMORY_LIMIT_KB}k
depends_on:
- anthias-server
environment:
- HOME=/data
- PORT=80
- NOREFRESH=1
- LISTEN=anthias-nginx
extra_hosts:
- "host.docker.internal:host-gateway"
privileged: true
restart: always
shm_size: ${SHM_SIZE_KB}kb
volumes:
- resin-data:/data
- /home/${USER}/.screenly:/data/.screenly
- /home/${USER}/screenly_assets:/data/screenly_assets
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
labels:
io.balena.features.supervisor-api: '1'

Here's the voluime declaration:

volumes:
resin-data:
redis-data:
screenly-data:

One caveat when creating an .asoundrc from the Dockerfile is that you have to copy .asoundrc from the container to host, edit it, then copy it back. (Unless there's a way to modify it from the settings page.)

I'll keep you posted.

Copy link
Contributor

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcrosson, also make sure to test this with existing installation.

Here's a suggestion on how to test it:

  • Do a fresh installation (master).
  • Then try to install your changes.
    • You might want to temporarily modify the install script and use that modified script instead.

Let me know if you need details on the how-tos.

ansible/roles/screenly/tasks/main.yml Outdated Show resolved Hide resolved
ansible/roles/screenly/tasks/main.yml Outdated Show resolved Hide resolved
@mcrosson
Copy link
Contributor Author

I've tested the ansible updates and they are working as expected and will not override an existing .asoundrc file and will create an empty .asoundrc file if it's not present on the system. The update have been pushed and should be good to go.

Also: given the web ui code for hdmi/3.5mm audio toggle should work with rpi devices: would it be good to apply the .asoundrc only when the system is not a raspberry pi?

I'd prefer leave the bind mount for .asoundrc for the moment, it's far less fiddly than trying to pull & push a file to a docker volume. Is this ok to leave as-is?

bin/install.sh Outdated Show resolved Hide resolved
bin/install.sh Outdated Show resolved Hide resolved
@mcrosson mcrosson force-pushed the asound-local-override-support branch from 2f6da0d to cbd6a9b Compare November 29, 2024 16:15
@mcrosson
Copy link
Contributor Author

The changes to install.sh were not supposed to be included in my pushed code. I've removed these changes from my repo and the install.sh file is no longer touched by this PR.

@nicomiguelino
Copy link
Contributor

@mcrosson, let me re-test this thoroughly. I'll rebase and merge this when ready.

Copy link
Contributor

@nicomiguelino nicomiguelino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcrosson, here are some comments:

  • Make sure that your branch is up to date. Rebase if needed.
  • Resolve the typo.
  • Add the following in your bin/upgrade_containers.sh:
    sed -i '/\.asoundrc/d' \
        /home/${USER}/screenly/docker-compose.yml

Make sure to add it just below the block below:

if [ "$DEVICE_TYPE" = "x86" ]; then
    sed -i '/devices:/ {N; /\n.*\/dev\/vchiq:\/dev\/vchiq/d}' \
        /home/${USER}/screenly/docker-compose.yml

After that, your upgrade_containers.sh would look like the following:

if [ "$DEVICE_TYPE" = "x86" ]; then
    sed -i '/devices:/ {N; /\n.*\/dev\/vchiq:\/dev\/vchiq/d}' \
        /home/${USER}/screenly/docker-compose.yml
    sed -i '/\.asoundrc/d' \
        /home/${USER}/screenly/docker-compose.yml
fi

That change is needed so that your changes will only take effect on x86 devices.

ansible/roles/screenly/tasks/main.yml Outdated Show resolved Hide resolved
@mcrosson
Copy link
Contributor Author

The typo has been fixed.

@nicomiguelino nicomiguelino merged commit e390a31 into Screenly:master Nov 30, 2024
1 check passed
@nicomiguelino
Copy link
Contributor

@mcrosson, should you have some more changes to make, feel free to create another pull request since I just merged this one.

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.

2 participants