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

Module load and unload support #59

Merged
merged 4 commits into from
Nov 9, 2021

Conversation

matt335672
Copy link
Member

See neutrinolabs/xrdp#1910

This PR makes the following changes to the pulseaudio modules:-

  • The first commit allows the modules to be configured via parameters instead of environment variables. This is to simplify using them on systems using systemd --user
  • The first commit makes sure file descriptors are closed before the modules are unloaded. Without this, pulseaudio leaks file descriptors, and chansrv does not re-listen on the audio sockets.
  • The third commit adds support for sink rewinds (needed for a buffer underrun), and the pulseaudio set_state_in_io_thread callback, for versions of pulseaudio that support it.

With these changes, the following script can be used to configure pulseaudio in an xrdp session using systemctl --user

#!/bin/sh

if [ -z "$XRDP_SOCKET_PATH" -o \
     -z "$XRDP_PULSE_SINK_SOCKET" -o \
     -z "$XRDP_PULSE_SOURCE_SOCKET" ]; then
    echo "** This does not appear to be an xrdp session" >&1
    false
else
    # Remove legacy environment variable if set
    systemctl --user unset-environment PA_SCRIPT || :

    # Unload modules
    pacmd unload-module module-xrdp-sink >/dev/null 2>&1
    pacmd unload-module module-xrdp-source >/dev/null 2>&1

    # Reload modules
    pacmd load-module module-xrdp-sink \
	xrdp_socket_path=$XRDP_SOCKET_PATH \
	xrdp_pulse_sink_socket=$XRDP_PULSE_SINK_SOCKET && \
    \
    pacmd load-module module-xrdp-source \
	xrdp_socket_path=$XRDP_SOCKET_PATH \
	xrdp_pulse_source_socket=$XRDP_PULSE_SOURCE_SOCKET
fi

exit $?

I've tested the module with these platforms:-

Platform pulseaudio version systemd --user ? Comments
Ubuntu 18.04 11.1 No Hard to configure as xrdp is old
Ubuntu 20.04 13.99.1 Yes
CentOS 7 10.0 No
CentOS 8 14.0 Yes Hard to build - wiki page added

Clearly we've got more work to do to support systemd --user properly, but this is a start.

@matt335672
Copy link
Member Author

@jsorg71 - I've just noticed that 6f18573 replaces a lot of the stuff you removed in #38, but it adds in the support for the set_state_in_io_thread callback which seemed to sort things out quite a bit on the platforms above. I'd be grateful if you could take a look at this, as you are doubtless more aware than I am how all this stuff hangs together.

@jsorg71
Copy link
Contributor

jsorg71 commented Oct 4, 2021

@matt335672 that should be ok. I was fixing a latency issue and I remember that the PA guys told me 'don't claim to support rewind if you don't implement it right'
As long as rewind works, it should be ok

@metalefty
Copy link
Member

This is a definitely useful improvement! I think the script should be included in the installation. The location may be $PREFIX/libexec/xrdp/pulse/pa-reload or somewhere like there. Then, users can run the script to reload pa modules.

Also, is the service file for user-mode systemd required?

@metalefty
Copy link
Member

Ah, I looked at #44.

The location of the reload script should be under libexec dir because it is a supplemental script/command. Actually, we should put startwm.sh/reconnectwm.sh scripts under libexec dir (Fedora does in local patch: https://src.fedoraproject.org/rpms/xrdp/tree/epel8 ). But let's put it aside.

Also, the desktop entry file should be included in installation
$PREFIX/share/xrdp/helpers/pulseaudio-xrdp.desktop or somewhere like there. Then, users can perform

cp /usr/share/xrdp/helpers/pulseaudio-xrdp.desktop /etc/xdg/autostart/

@matt335672
Copy link
Member Author

Thanks for that - it's useful.

I think I need to spit this into a couple of PRs. One will be the code changes, and the other will be the extra files for desktop integration. I'll need to do a bit more testing on various distros to see exactly what is required.

The script above needs some mods to be more generic. One thing I've found is that I should be using pactl to load the modules rather than pacmd, as this will start pulseaudio if it isn't already active. More info on this debian thread post

@metalefty
Copy link
Member

Okay, let's make a new release when desktop integration PR is finished (and README update).

@matt335672
Copy link
Member Author

Are you happy for the README to just point to the Wiki? It's going to be easier to add platform instructions there I think, on multiple pages.

@metalefty
Copy link
Member

Yes, how to build & install section can be a pointer to Wiki.

Copy link
Member

@metalefty metalefty left a comment

Choose a reason for hiding this comment

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

Let's go ahead.

@matt335672
Copy link
Member Author

OK - thanks.

I'm still hoping to hear from @davidcbcs on Gitter about CentOS 8, as I'm worried I might have missed something. I'll give him a couple of days to get back to me.

@matt335672 matt335672 merged commit ef3cc29 into neutrinolabs:devel Nov 9, 2021
@matt335672 matt335672 deleted the module_params branch November 9, 2021 10:44
@davidcbcs
Copy link

davidcbcs commented Nov 9, 2021 via email

@matt335672
Copy link
Member Author

Hi @davidcbcs

The wiki should be up to date for CentOS 8:-

https://github.com/neutrinolabs/pulseaudio-module-xrdp/wiki/Build-on-CentOS-8.x

Please tell me if there's anything missing.

@davidcbcs
Copy link

davidcbcs commented Nov 9, 2021 via email

@matt335672
Copy link
Member Author

Fair point. Each of the sections is referred to from the main page which has that on. I think I'll need to refer back to the main page from the sections for that.

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