Bug 55280 - tunnel modules cannot figure out adress/auth automatically
Summary: tunnel modules cannot figure out adress/auth automatically
Status: RESOLVED FIXED
Alias: None
Product: PulseAudio
Classification: Unclassified
Component: modules (show other bugs)
Version: unspecified
Hardware: Other All
: medium enhancement
Assignee: pulseaudio-bugs
QA Contact: pulseaudio-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-24 10:14 UTC by Pierre Ossman
Modified: 2013-09-13 14:14 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
pulse-tunnel-auto.patch (7.17 KB, text/plain)
2012-09-24 10:14 UTC, Pierre Ossman
Details
pulse-servers.patch (3.01 KB, patch)
2012-09-27 11:14 UTC, Pierre Ossman
Details | Splinter Review
0001-module-tunnel-automatically-find-the-PulseAudio-serv.patch (10.18 KB, patch)
2013-07-11 12:01 UTC, Pierre Ossman
Details | Splinter Review
0001-module-tunnel-automatically-find-the-PulseAudio-serv.patch (10.87 KB, patch)
2013-07-16 14:48 UTC, Pierre Ossman
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Ossman 2012-09-24 10:14:25 UTC
Created attachment 67623 [details]
pulse-tunnel-auto.patch

This is a bit of a niche feature, but I figured we wrote the code so we might as well share. :)

The attached patch makes sure that module-tunnel-* can use the environment and X11 properties to figure out how to reach the PulseAudio daemon, the same way a client would.

We use this in our Linux Remote Desktop product ThinLinc, where the client launches its own PulseAudio daemon (so we can be sure of what goes over the network), but we want it to talk to the local PulseAudio daemon to actually output sound.
Comment 1 Pierre Ossman 2012-09-27 11:14:13 UTC
Created attachment 67765 [details] [review]
pulse-servers.patch

Second patch that also makes sure the modules can handle server specifications with multiple entries.
Comment 2 Tanu Kaskinen 2012-09-27 12:34:47 UTC
Thanks for the patches! I have trouble applying them, though. What version are they generated against? Also, would formatting them with "git format-patch" be possible?
Comment 3 Pierre Ossman 2012-09-27 13:20:56 UTC
Unfortunately I have yet to convince the powers at be that we should convert from subversion to git. :)

The patches are against a git snapshot taken Sept 20th. Unfortunately I can't get the exact commit id. Given the current log it should have been 2c8aa18b1d though.
Comment 4 Peter Åstrand 2013-03-28 14:47:25 UTC
If there's anything we can do to make it easier to integrate this work, please let us know. Would a git pull request help?
Comment 5 Tanu Kaskinen 2013-03-29 12:10:53 UTC
A patch generated by "git format-patch" would be preferable, otherwise please give instructions for how to apply the patches.
Comment 6 Pierre Ossman 2013-07-11 12:01:01 UTC
Created attachment 82334 [details] [review]
0001-module-tunnel-automatically-find-the-PulseAudio-serv.patch

git-format-patch against master branch.
Comment 7 Tanu Kaskinen 2013-07-14 12:29:19 UTC
The tunnel module should get rewritten so that it uses libpulse, and this bug should get resolved as a side effect. There is no 100% guarantee that the rewrite project will finish, so if you want this patch applied anyway, I'm not against that. Some review comments follow.

In pa__init(), the server variable is never freed, even if it should be:

#ifdef HAVE_X11
    if ((server == NULL) && (xcb != NULL)) {
        char t[1024];
        if (pa_x11_get_prop(xcb, 0, "PULSE_SERVER", t, sizeof(t)))
            server = pa_xstrdup(t);
    }
#endif

It doesn't seem like a good idea to connect to a local server by default. If the user loads module-tunnel-sink without any arguments, I think it should fail, because tunneling to a local daemon doesn't usually make sense. I think it would be better if connecting to a local server would require an explicit indication that the user really wants to do that.
Comment 8 Tanu Kaskinen 2013-07-14 12:30:42 UTC
(In reply to comment #7)
> The tunnel module should get rewritten so that it uses libpulse, and this
> bug should get resolved as a side effect.

This was a bit unclear. When I said "should get rewritten", I meant that there's an ongoing effort to do that.
Comment 9 Pierre Ossman 2013-07-14 13:37:10 UTC
(In reply to comment #7)
> The tunnel module should get rewritten so that it uses libpulse, and this
> bug should get resolved as a side effect. There is no 100% guarantee that
> the rewrite project will finish, so if you want this patch applied anyway,
> I'm not against that. Some review comments follow.

I'd appreciate that. The rewrite is very interesting to us, but it has a bit more to go before it can replace module-tunnel. So we'll probably be doing relevant fixes and improvements to module-tunnel until then.

> 
> In pa__init(), the server variable is never freed, even if it should be:
> 

Will have a look at that. Thanks.

> It doesn't seem like a good idea to connect to a local server by default. If
> the user loads module-tunnel-sink without any arguments, I think it should
> fail, because tunneling to a local daemon doesn't usually make sense. I
> think it would be better if connecting to a local server would require an
> explicit indication that the user really wants to do that.

I can sympathise with that. Would something like an "auto" argument suffice?
Comment 10 Tanu Kaskinen 2013-07-15 09:25:04 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > It doesn't seem like a good idea to connect to a local server by default. If
> > the user loads module-tunnel-sink without any arguments, I think it should
> > fail, because tunneling to a local daemon doesn't usually make sense. I
> > think it would be better if connecting to a local server would require an
> > explicit indication that the user really wants to do that.
> 
> I can sympathise with that. Would something like an "auto" argument suffice?

Yes.
Comment 11 Pierre Ossman 2013-07-16 14:48:08 UTC
Created attachment 82484 [details] [review]
0001-module-tunnel-automatically-find-the-PulseAudio-serv.patch

Updated patch. The automatic magic now requires an "auto=yes" argument. Without this it will behave like before, with one difference: the server name is interpreted as a list rather than a single server.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.