[MPlayer-dev-eng] [PATCH] PulseAudio audio output

[Lennart Poettering]

On Sun, 16.07.06 21:24, Reimar Döffinger (Reimar.Doeffinger at stud.uni-karlsruhe.de) wrote:

> Hello,

Hi!

> On Sun, Jul 16, 2006 at 03:18:59PM +0200, Lennart Poettering wrote:
> > As some of you probably noticed the Polypaudio project was recently
> > renamed to PulseAudio. The following patch renames MPlayer's ao_polyp
> > driver to ao_pulse and updates it for the recent PulseAudio API changes:
> 
> I am not at all a fan of senseless renames I must say. And what kind of
> API changes? Will it no longer work with older versions? And how old are
> the versions it would break?

Neither do I like "senseless" renames. But this one isn't senseless. 
See this blog story for an explanation:

http://0pointer.de/blog/projects/pulse.html

Polypaudio never got widespread acceptance, thus we decided to do a
clean cut and not support the older versions any more. The new name
was something like a requirement to get the blessing of the big
distributions to be part of the core desktops. 

The API changes are usually cleanups. But this shouldn't matter to
MPlayer. 

> > http://0pointer.de/public/mplayer-pulse.patch
> 
> That's impossible to review, it doesn't show what exactly you changed.
> You should have used e.g. svn move to create libao2/ao_pulse.c.
> Also attaching patches is very much preferred.

The problem with "svn mv" in this case is that the patch generated
with "make diff" is no longer directly applicable to the source tree
with "patch" because it doesn't contain the information that
ao_pulse.c is actually based on ao_polyp.c.

Anyway, as you requested it: Here is the same patch, this time done
with "svn mv":

http://0pointer.de/public/mplayer-pulse-with-mv.patch

> > Some other changes have been made as well:
> 
> Cosmetics (such as renaming) and other stuff must be a separate
> patch.

Ahem. What's the point of that? The intermediate version would not
compile and would be strangely named because the file would be called
"ao_polyp.c" and the internals are actually called "pulse". It makes
much more sense to do that change atomically. 

> > * The PulseAudio client runs its event loop in a thread of its
> >   own. This should make PulseAudio's latency interpolation over the
> >   network much more reliable.
> 
> Judging by the number of pa_threaded_mainloop* stuff this was not
> exactly introduced in a transparent way. Isn't there an API that gets
> away with a bit less locking and provides a synchronous API?

The answer is no. There must be some communication between the
playback thread and the main thread. In modern kernels (which futexes
and stuff) locking doesn't hurt much and this stuff is not really time
critical anyway. 

PulseAudio offers a simplified synchronous API. But this API is
mostly just a wrapper around the async API whith exactly the same
pa_threaded_mainloop* stuff used internally. 

> > * Proper surround sound support

> Should be a separate patch as well (unless it is directly connected to
> the new API).

We get that actually nearly for free with the new API. Not activating
this is a matter of a single function argument. I don't see a point in
removing that argument for a seperate patch and than adding it again. 

Lennart

-- 
Lennart Poettering; lennart [at] poettering [dot] net
ICQ# 11060553; GPG 0x1A015CC4; http://0pointer.net/lennart/