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

Sound is leaking across multiple xpra sessions attached to different machines. #1266

Closed
totaam opened this issue Jul 24, 2016 · 33 comments
Closed
Labels

Comments

@totaam
Copy link
Collaborator

totaam commented Jul 24, 2016

Issue migrated from trac ticket # 1266

component: sound | priority: major | resolution: fixed

2016-07-24 05:33:44: kundanvp created the issue


If we attach two xpra clients from two different machines, attached to two separate xpra server on one machine. (Both xpra server has xterm as it's child application)

And, if we start any sound playing application, like vlc player on any one of the xpra xterm session attached to one of the xpra server, we see sound coming from the other machine as well.

However, the expectation is that, since we have opened the vlc player from one xpra client machine only, it should not expose it's sound to the other xpra client machine.

This seems to be a regression from the behaviour of xpra v0.16 version.

Minimum configuration needed to reproduce: 3 Ubuntu 16.04 machines.

Steps to reproduce:-

  1. Start two xpra server on first machine, which is acting like server and don't have existing pulseaudio server running.
xpra start :100 --start-child=xterm --no-daemon --bind-tcp=192.168.56.1:10000
xpra start :101 --start-child=xterm --no-daemon --bind-tcp=192.168.56.1:10001
  1. Attach with 1st xpra server from second machine with command
xpra attach tcp:192.168.56.1:10000
  1. Attach with 2nd xpra server from second machine with command
xpra attach tcp:192.168.56.1:10001
  1. Open a sound playing application let's say vlc from 3rd machine xterm session which has got exposed through xpra attach.

  2. Play a sound file in vlc.

  3. We see sound coming from the 2nd machine as well.

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 10:07:17: antoine changed owner from antoine to kundanvp

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 10:07:17: antoine commented


It's very unlikely that this ever worked properly, even with previous versions unless each session is started from a different user: you cannot start more than one pulseaudio server per user without some major hacks.

So when you start your second xpra instance, the pulseaudio server will fail to start and the session will end up re-using the pulseaudio server started for the first session.

You can verify this by posting your server log file, which should contain the message: Warning: pulseaudio has terminated shortly after startup..

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 11:38:09: kundanvp commented


Hi Antoine,

But, it did work.

The reason, which I think, might be responsible for it would be that, we are not creating pulseaudio directly, but as a child of an extra shell. But, I am not sure.

I am attaching the output of creation of the two xpra servers, and
ps -eaf | grep pulseaudio output, to show that pulseaudio indeed started for both the two xpra sessions.

The log includes the output of following:-

xpra start :100 --start-child=xterm --no-daemon --bind-tcp=192.168.56.1:10000 > 1st_server.log

xpra start :101 --start-child=xterm --no-daemon --bind-tcp=192.168.56.1:10001 > 2nd_server.log

ps -eaf | grep pulseaudio > pulseaudio_ps_eaf.out

In the pulseaudio_ps_eaf.out, for each session two entries are seen. Since we have started two xpra servers we are able to see 4 entries there.

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 11:39:26: kundanvp uploaded file 1st_server.log (1.3 KiB)

1st server starting log

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 11:39:53: kundanvp uploaded file 2nd_server.log (1.3 KiB)

2nd server starting log.

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 11:40:30: kundanvp uploaded file pulseaudio_ps_eaf.out (1.9 KiB)

ps -eaf | grep pulseaudio output.

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 11:41:36: kundanvp changed owner from kundanvp to antoine

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 13:05:51: antoine changed owner from antoine to kundanvp

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 13:05:51: antoine commented


Oh. I didn't know that.
So Debian or Ubuntu must be patching pulseaudio to do that as my previous requests to pulseaudio developers were waved away.

If so, then the changes in #1141 may have caused this: it is possible that when we call pactl to set the default device it somehow affects both servers?

I don't have Ubuntu or Debian at hand to test.

@totaam
Copy link
Collaborator Author

totaam commented Jul 24, 2016

2016-07-24 19:24:09: kundanvp commented


It seems I got what is causing issue. And, it doesn't seem to be #1141.

I would explain it. But, first the pactl info from both the attached xpra xterm sessions.

  1. pactl info for the xpra xterm session on display :100.
Server String: /run/user/1000/pulse/native
Library Protocol Version: 30
Server Protocol Version: 30
Is Local: yes
Client Index: 46
Tile Size: 65472
User Name: kundan
Host Name: kundan-Vostro-3446
Server Name: pulseaudio
Server Version: 8.0
Default Sample Specification: s16le 2ch 44100Hz
Default Channel Map: front-left,front-right
Default Sink: Xpra-Speaker
Default Source: Xpra-Microphone.monitor
Cookie: 8960:1c15
  1. pactl info for the xpra xterm session on display :101.
Server String: /run/user/1000/pulse/native
Library Protocol Version: 30
Server Protocol Version: 30
Is Local: yes
Client Index: 48
Tile Size: 65472
User Name: kundan
Host Name: kundan-Vostro-3446
Server Name: pulseaudio
Server Version: 8.0
Default Sample Specification: s16le 2ch 44100Hz
Default Channel Map: front-left,front-right
Default Sink: Xpra-Speaker
Default Source: Xpra-Microphone.monitor
Cookie: 8960:1c15

In both the above output's. The server string is same i.e

/run/user/1000/pulse/native

However, two different pulseaudio server's has been started. But, their identity is same.

Earlier, this server string used to be, like /tmp/pulse-61/native, and which seemed to be coming from the following pulseaudio command.

/usr/bin/pulseaudio --start -vvvv --disable-shm=true --daemonize=false \
    --use-pid-file=false --system=false --exit-idle-time=-1 -n \
    --load=module-suspend-on-idle \
    --load=module-null-sink sink_name="xpra_output_private" sink_properties=device.description="Xpra\ Private\ Output\ Device" \
    --load=module-null-sink sink_name="xpra_output_device" sink_properties=device.description="Xpra\ Output\ Device" \
    --load=module-native-protocol-unix socket=/tmp/pulse-61/native

which I quoted in #1141#comment:14.

The last line,

--load=module-native-protocol-unix socket=/tmp/pulse-61/native

contains something which creates the pulseaudio server string and it contains the display number as one of the constituent of the server string which used to make it unique for each xpra server.

But, in /run/user/1000/pulse/native. I don't think we are giving any consideration to the display number. 1000 seems to be a randomly chosen number.
Therefore, even though we are creating different pulseaudio server's for each xpra start. There, isn't any differentiation between them. They all are referring to the same server string.

Hence, both the xpra _sound_record is getting confused and recording from the same pulseaudio server and sending it back to it's corresponding pulseaudio server on it's client.

kundan    5316  5043  5 22:42 pts/6    00:02:28 /usr/bin/python /usr/bin/xpra _sound_record - - pulsesrc device=Xpra-Speaker.monitor opus  1.0 -d
kundan    5345  5164  5 22:42 pts/20   00:02:23 /usr/bin/python /usr/bin/xpra _sound_record - - pulsesrc device=Xpra-Speaker.monitor opus  1.0 -d

It seems, this is causing the sound leak.

Both of the above xpra _sound_record commands should record from their corresponding pulseaudio servers. But, they can't identify which pulseaudio server belongs to them because we haven't given unique identification to them in form of display number. Therefore, they are always choosing the first pulseaudio server, leading to sound coming from both the client machines, which should be destined for only one of them.

I think, this should be a generic issue. Not, specific to Ubuntu 16.04. You may try on other Linux flavours as well like Fedora. And, could get a chance to see the issue. But, I haven't tested them. Therefore, can't say for sure that issue exist on those Linux flavours as well or not.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:38:01: kundanvp commented


I have got a workaround of the above problem, which can be fixed in the code as well.

If we start the xpra server's appending XDG_RUNTIME_DIR=/tmp/,
issue get's fixed.

Now, new xpra start commands:-

  1. Starting the first xpra server on display :100 with XDG_RUNTIME_DIR=/tmp/100
XDG_RUNTIME_DIR=/tmp/100 xpra start :100 --start-child=xterm --no-daemon --bind-tcp=192.168.56.1:10000 > 1st_server_with_xdg.out 2>&1
  1. Starting the second xpra server on display :101 with XDG_RUNTIME_DIR=/tmp/101
XDG_RUNTIME_DIR=/tmp/101 xpra start :101 --start-child=xterm --no-daemon --bind-tcp=192.168.56.1:10001 > 2nd_server_with_xdg.out 2>&1

The above fixes two problems:-

  1. We are able to start an xpra server even when existing pulseaudio server of Ubuntu is running.
  2. Sound leak doesn't appear as now each pulseaudio server created for xpra server has unique identification in terms of display number.

I am also uploading the ps -eaf output of pulseaudio before and after starting xpra server on the Server machine.

I am also uploading the pactl info on the now attached xpra xterm sessions, which is showing two distinct server strings for both the xpra sessions.

The commands whose list I am uploading:-

Before starting xpra sessions.

ps -eaf | grep pulseaudio > ps_eaf_pulse_without_xpra_start.out

After starting xpra sessions.

ps -eaf | grep pulseaudio > ps_eaf_pulse_with_xpra_start.out

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:39:17: kundanvp uploaded file 1st_server_with_xdg.out (3.6 KiB)

log of strating the first xpra server with XDG_RUNTIME_DIR environment variable.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:40:04: kundanvp uploaded file 2nd_server_with_xdg.out (3.6 KiB)

Log of starting 2nd xpra session with XDG_RUNTIME_DIR environment variable.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:40:44: kundanvp uploaded file ps_eaf_pulse_without_xpra_start.out (0.2 KiB)

ps -eaf | grep pulseaudio output before starting the xpra server.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:41:17: kundanvp uploaded file ps_eaf_pulse_with_xpra_start.out (2.0 KiB)

ps -eaf | grep pulseaudio output after starting the xpra server.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:43:12: kundanvp uploaded file 100_display_pactl_info_after_xdg.out (0.4 KiB)

pactl info from the xpra session attached to the first xpra server with display :100.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:44:16: kundanvp uploaded file 101_display_pactl_info_after_xdg.out (0.4 KiB)

pactl info from the xpra session attached to the second xpra server with display :101.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:50:43: kundanvp changed owner from kundanvp to antoine

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 04:50:43: kundanvp commented


Please look into the issue, if it could be fixed in code.
So that, the default behavior uses the display number to create the pulseaudio server string.

Thanks in advance.

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 11:32:53: antoine changed owner from antoine to kundanvp

@totaam
Copy link
Collaborator Author

totaam commented Jul 25, 2016

2016-07-25 11:32:53: antoine commented


The XDG_RUNTIME_DIR hack is what I was talking about in comment:1, that's the code I had written for winswitch and I'm not sure we want to use the same hack here: I would be very reluctant to modify this environment variable for all applications as it may have undesirable side effects (see #1129 #1105) when we only want pulseaudio to behave.

Can other applications still locate the pulseaudio server if XDG_RUNTIME_DIR is changed only for pulseaudio? I doubt it, how would they? X11 applications may be able to use the X11 display environment variables (see xprop -root | grep -i pulse, but command line applications would not.

@totaam
Copy link
Collaborator Author

totaam commented Jul 26, 2016

2016-07-26 20:09:10: kundanvp commented


Sorry, I didn't understand the first expectation completely.

Are we saying to prepend XDG_RUNTIME_DIR=/tmp<displayno> in DEFAULT_PULSEAUDIO_COMMAND before pulseaudio in file platform/features.py and check whether the server string of the pulseaudio changes or not?

For the second expectation, I ran, xprop -root | grep -i pulse in the xterm attached through xpra. But, it is showing nothing. However, if I run xprop -root only, it shows a lot of values containing _XPRA.
Would you be interested in knowing them, then I can upload the results?

Regarding fixing this issue,

I also don't think XDG_RUNTIME_DIR would be the only way to fix this issue. Earlier, also in xpra version v0.16. I think we didn't have this variable, but still things used to work.

One thing more which I would like to mention about the difference, is that earlier, I used to see PULSE_SERVER variable in the printenv output. But, now I don't see any such thing. Maybe XDG_RUNTIME_DIR might be playing it's role in some way.

But, my proposal is that can we bring back the socket field of module-native-protocol-unix --load=module-native-protocol-unix socket=/tmp/pulse-61/native by some way.

I understand that doing this in the DEFAULT_PULSEAUDIO_COMMAND itself might have some issues, i.e display number is not available at that place e.t.c.

But, One thing which I failed to understand from the current code is that, somehow this socket field of module-native-protocol-unix field is getting modified in code taking XDG_RUNTIME_DIR help. Therefore, the server string get's changed when I change the XDG_RUNTIME_DIR.
This means it is possible to change the socket field, even after the execution of the pulseaudio command. Correct me if I am wrong.

If we, do that and also pass the PULSEAUDIO_SERVER variable in xpra environment so that it could be seen in the printenv output of our attached xterm. Then I don't think we need to worry that, how applications would be able to locate pulseaudio server. Applications, would be able to know that information taking the help of PULSEAUDIO_SERVER variable. Correct me if I am wrong.

May be you might hit some better idea to fix this issue. As, I am not well versed with the things which is going here. What do you think?

@totaam
Copy link
Collaborator Author

totaam commented Jul 27, 2016

2016-07-27 14:59:22: antoine commented


On Fedora and centos, it is impossible to start another pulseaudio instance if one is running already, no matter what is set for the unix domain socket path or XDG_RUNTIME_DIR. This is what you get if you try:

  • with --load=module-native-protocol-unix socket=$HOME/tmp":
N: [pulseaudio] main.c: User-configured server at {0d37226ca0064aaab1db9e66016c45f5}unix:/run/user/1000/pulse/native, which appears to be local. Probing deeper.
E: [pulseaudio] pid.c: Daemon already running.
  • with XDG_RUNTIME_DIR=pwd/tmp/:
N: [pulseaudio] main.c: User-configured server at {0d37226ca0064aaab1db9e66016c45f5}unix:/run/user/1000/pulse/native, refusing to start/autospawn.

If a workaround is possible, it won't work on Fedora or centos, so I am not very interested in working on this, sorry. Patches welcome though.
The "pulseaudio-command" option should expand environment variables, if you want to use that. ie: $DISPLAY should be set by the time we start pulseaudio.

@totaam
Copy link
Collaborator Author

totaam commented Jul 14, 2017

2017-07-14 17:06:46: antoine commented


Another Ubuntu-only? sound issue (see also #1141 and #1219), I don't have time for Ubuntu - sorry. Re-scheduling.

@totaam
Copy link
Collaborator Author

totaam commented Jan 22, 2018

2018-01-22 09:20:59: antoine changed owner from kundanvp to Kundan

@totaam
Copy link
Collaborator Author

totaam commented Jan 22, 2018

2018-01-22 09:20:59: antoine commented


Done in r18103: we override XDG_RUNTIME_DIR only for the "pulseaudio" subprocess, using a temporary directory that we cleanup afterwards.
(see also #1638#comment:14)

@kundan: does that work for you?

@totaam
Copy link
Collaborator Author

totaam commented Jan 30, 2018

2018-01-30 16:24:53: antoine changed owner from Kundan to maxmylyn

@totaam
Copy link
Collaborator Author

totaam commented Jan 30, 2018

2018-01-30 16:24:53: antoine commented


@maxmylyn: if your setup only ever uses one instance per user account, you should disable this feature as it may cause problems in some corner cases.

@totaam
Copy link
Collaborator Author

totaam commented Feb 2, 2018

2018-02-02 17:08:35: maxmylyn changed status from new to closed

@totaam
Copy link
Collaborator Author

totaam commented Feb 2, 2018

2018-02-02 17:08:35: maxmylyn set resolution to fixed

@totaam
Copy link
Collaborator Author

totaam commented Feb 2, 2018

2018-02-02 17:08:35: maxmylyn commented


Noted - I'll pass this along.

@totaam totaam closed this as completed Feb 2, 2018
@totaam
Copy link
Collaborator Author

totaam commented Feb 18, 2018

2018-02-18 06:55:29: antoine commented


Related improvements (loop detection, notifications, etc): r18468

@totaam
Copy link
Collaborator Author

totaam commented Apr 24, 2018

2018-04-24 04:50:42: antoine commented


r19064+r19065 is a large-ish fixup for r18103 (see comment:12) because memfd is not available on Ubuntu 16.04.

Follow up: #2186

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant