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

fix: Check for PipeWire as well as PulseAudio before falling back to ALSA #1565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexhaydock
Copy link

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have performed a self-review of my code
  • I have tested my code in common scenarios and confirmed there are no regressions
  • I have added comments to my code, particularly in hard-to-understand sections

@alexhaydock alexhaydock changed the title Check for PipeWire as well as PulseAudio before falling back to ALSA fix: Check for PipeWire as well as PulseAudio before falling back to ALSA Jan 19, 2025
@szorfein
Copy link
Contributor

This will not work for pure Alsa system. Pipewire is always installed as dependencies for another app even if not used.

@szorfein
Copy link
Contributor

Maybe instead check if the pipewire service is running like

systemctl is-active pipewire

@alexhaydock
Copy link
Author

alexhaydock commented Jan 19, 2025 via email

@szorfein
Copy link
Contributor

szorfein commented Jan 19, 2025

if work for me if testing with pipewire-pulse if u can test

if ! command -v pacmd >/dev/null 2>&1 && ! command -v pipewire-pulse >/dev/null 2>&1; then

@alexhaydock
Copy link
Author

if work for me if testing with pipewire-pulse if u can test

if ! command -v pacmd >/dev/null 2>&1 && ! command -v pipewire-pulse >/dev/null 2>&1; then

Thanks! Have updated the PR to look for pipewire-pulse instead of just pipewire.

I think that's definitely more robust and I can confirm it works to solve the original bug still on Fedora 41. Should also cope with situations where the system is pure ALSA still, like you mentioned before.

@lj3954
Copy link
Member

lj3954 commented Jan 20, 2025

Could we not just check whether one of these servers is actively running?

if ! pidof pipewire 2>&1 /dev/null && ! pidof pulseaudio 2>&1 /dev/null; then

Copy link
Member

@lj3954 lj3954 left a comment

Choose a reason for hiding this comment

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

QEMU 8.1 added a pipewire audio backend. We should use it if possible. This should be a better solution in general.

Comment on lines +885 to 887
if ! command -v pacmd >/dev/null 2>&1 && ! command -v pipewire-pulse >/dev/null 2>&1; then
AUDIO_DRIVER="alsa"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ! command -v pacmd >/dev/null 2>&1 && ! command -v pipewire-pulse >/dev/null 2>&1; then
AUDIO_DRIVER="alsa"
fi
if pidof pipewire >/dev/null 2>&1; then
# QEMU's pipewire audio backend was added in version 8.1
if [ "${QEMU_VER_SHORT}" -ge 81 ]; then
AUDIO_DRIVER="pipewire"
fi
elif ! pidof pulseaudio >/dev/null 2>&1; then
AUDIO_DRIVER="alsa"
fi

Copy link
Contributor

Choose a reason for hiding this comment

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

good idea to test pipewire and pulseaudio with pidof and this include pipewire to the project :)

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.

bug: VMs crashing on Fedora 41 as a result of #1480
3 participants