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

[Reimar Doeffinger]

Hello,
On Sun, Jul 16, 2006 at 10:04:46PM +0200, Lennart Poettering wrote:
> 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

If that's the same as I read it quite fits my definition of senseless.
But that's actually mostly irrelevant.

> Polypaudio never got widespread acceptance, thus we decided to do a
> clean cut and not support the older versions any more.

I don't think it's acceptable to rip out functionality where the
replacement is really young and untested.
If there's no way to merge the two so older versions are supported as
well IMHO ao_polyp must stay.

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

If it means the same code (with maybe a few #ifdefs) can't be used for
both API it matters a lot.

> 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.

"make diff"? I think svn diff contains that information, but only in a
human-readable form, not something that patch can use. Also svn
copy/move should be used when applying, so this patch will avoid making
a mistake here.

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

I'll have a look at it if ever DNS is going to start working here again.

> 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. 

Well, you were talking about a rename, I didn't realize that you
actually (as far as the API is concerned) you meant a complete rewrite.

> > 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. 

I don't care about speed in this case, and if it requires a 10GHz CPU to
play a MP3. The point I was trying to make is that it is a lot of code
just to emulate a synchronous API with a lot of ugly whiles.

> 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. 

Well, if it saves 1/5 of the code, I'd say that's a very good reason to
use it. I didn't read the code well, but my impression was that the
asynchronous, locking based API has no advantage due to the way it is
used.

> > > * 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. 

So this means surround support is not possible or at least not possible
in the same way in the old API? That's why I said "unless it is directly
connected to the new API".
I just find it unjustified to call this a "rename" when it actually
seems to break any program using the old API. Then it's actually a new
thing and should be treated as such. And the news entry on the homepage
isn't clear about how massive the changes seem to be.

Greetings,
Reimar Doeffinger