Bug 445644 - Portaudio does not work with / through pulseaudio
Portaudio does not work with / through pulseaudio
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: portaudio (Show other bugs)
9
All Linux
low Severity low
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
: Patch, Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-05-08 05:13 EDT by Hans de Goede
Modified: 2009-05-15 19:29 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-07 17:25:09 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Fixed patch (14.24 KB, patch)
2008-12-08 13:38 EST, Kevin Kofler
no flags Details | Diff

  None (edit)
Description Hans de Goede 2008-05-08 05:13:03 EDT
Description of problem:
The summary says it all.

I've been looking into fixing this, and that won't be easy, the current alsa 
code in portaudio onl works with alsa devices which support mmap, and adding 
support for new output devices to portaudio seems for from easy.

I think the best solution is to add non mmap (regular read/write) support to 
portaudio's alsa support, if you look for MMAP and mmap you will find all the 
mmap specific code.

I was thinking about writing a fix for this myself, but I currently simply do 
not have the time for this.
Comment 1 Hans de Goede 2008-05-08 06:08:06 EDT
Ah,

Matthias, I see this is yours, I know how busy you are, so I'll reassign this 
to me. Shall we co-maintain this?

Comment 2 Matthias Saou 2008-05-08 07:31:48 EDT
(In reply to comment #1)
> Matthias, I see this is yours, I know how busy you are, so I'll reassign this 
> to me. Shall we co-maintain this?

Perfectly fine by me, go for it! Oh, and... thanks :-)
Comment 3 Bug Zapper 2008-05-14 06:49:27 EDT
Changing version to '9' as part of upcoming Fedora 9 GA.
More information and reason for this action is here:
http://fedoraproject.org/wiki/BugZappers/HouseKeeping
Comment 4 Anthony Symons 2008-10-16 21:52:14 EDT
Any progress on this? Im a software dev, trying to use wiresharks rtp player to listen to a captured rtp audio stream. Ive discovered that I cant because it uses portaudio. I found this bug report, and considered ditching pulseaudio from my system, but it seems too deeply integrated to risk trying.
Comment 5 Anthony Symons 2008-10-16 22:02:52 EDT
Ok, I found out all i needed was a "killall pulseaudio" to use wiresharks rtpplayer. Still would be nice if the extra compatibility was there, though.
Comment 6 Hans de Goede 2008-10-17 02:38:09 EDT
(In reply to comment #4)
> Any progress on this? Im a software dev, trying to use wiresharks rtp player to
> listen to a captured rtp audio stream. Ive discovered that I cant because it
> uses portaudio. I found this bug report, and considered ditching pulseaudio
> from my system, but it seems too deeply integrated to risk trying.

I'm afraid not portaudio is a mighty complex beast (esp for what it does). I've looked into how to handle this, but haven't gotten any further then that (-ENOTIME).
Comment 7 Kevin Kofler 2008-12-05 13:42:52 EST
FYI, I'm currently working on this (I'm trying to fix PortAudio to work with _any_ non-mmap ALSA device, including PulseAudio, with PulseAudio being the one I do my testing with).
Comment 8 Kevin Kofler 2008-12-06 21:34:36 EST
Here's a patch, can you please try this?
http://techweb.rfa.org/pipermail/portaudio/2008-December/009023.html
Tested with espeak (by me), Audacity (by David Henningsson) and the basic PortAudio test programs (by me).
It will also have to be applied to the audacity and audacity-freeworld packages because they ship private patched copies of PortAudio.
Comment 9 Kevin Kofler 2008-12-06 21:36:26 EST
See also http://techweb.rfa.org/pipermail/portaudio/2008-November/008990.html for some context (but that's an OLD version of the patch).
Comment 10 Hans de Goede 2008-12-07 05:07:17 EST
(In reply to comment #8)
> Here's a patch, can you please try this?
> http://techweb.rfa.org/pipermail/portaudio/2008-December/009023.html

Awesome! I'm very happy you found the time to this, thanks!

> Tested with espeak (by me), Audacity (by David Henningsson) and the basic
> PortAudio test programs (by me).

I've gotten involved of this because of a game using portaudio (wordwarvi) and I've just done a build of portaudio with this patch and testes it. wordwarvi crashes with the patched portaudio :( 

I'll make some time to look into this asap.
Comment 11 Hans de Goede 2008-12-08 07:35:41 EST
Kevin,

Good news after too much debugging I've found the issue with wordwarvi and it is wordwarvi's fault, with the bug in wordwarvi fixed things work fine with your patch,

However during debugging I noticed that snd_pcm_writei was being called with 8193 samples instead of 8192. This is also the reason why you need the + 1 to the buffer-size for the non-mmap buffers.

This is caused by this part of the patch:
@@ -1831,7 +1837,7 @@
     PA_UNLESS( framesPerHostBuffer != 0, paInternalError );
     self->maxFramesPerHostBuffer = framesPerHostBuffer;
 
-    if( !accurate )
+    if( !self->playback.canMmap || !accurate )
     {
         /* Don't know the exact size per host buffer */
         *hostBufferSizeMode = paUtilBoundedHostBufferSize;

And can (I believe) be fixed by simply removing this hunk. The accurate thing is to handle cards where periodsize is a not integer, for example 500.5 samples / period. I really see no reason to always tread things as if we have a non integer period size in the read / write case. There really is no difference with regards to periodsize handling between mmap / rw mode. Writing 8193 samples instead of 8192 (in my test case) really is quite in-efficient and I see no reason for this.

Even if we drop this chunk we still need the + 1 in the allocs / buffersize calculations for the nonMMAP buffer for the case where accurate is not true.

---

I've prepared a new portaudio release with your patch (minus the hunk) applied, and if you're ok with the removal of the hunk I'll build it for rawhide and F-10 and push it to F-10 updates-testing.
Comment 12 Kevin Kofler 2008-12-08 13:17:48 EST
We cannot drop this hunk entirely!

This hunk is the fix for the stuttering I had with the first version of the patch. The if condition contains 2 lines (plus comments):
        /* Don't know the exact size per host buffer */
        *hostBufferSizeMode = paUtilBoundedHostBufferSize;
        /* Raise upper bound */
        ++self->maxFramesPerHostBuffer;
The first one is what we need - we must use paUtilBoundedHostBufferSize instead of paUtilFixedHostBufferSize, because we must be prepared to request different sizes from the callback, we cannot wait until we have the fixed size because that's too late. The second line (and the associated +1s in the allocs) is the problem.

I'm preparing a fixed patch.
Comment 13 Kevin Kofler 2008-12-08 13:38:05 EST
Created attachment 326162 [details]
Fixed patch

Please try this one. The only change from my previous patch is this:
         /* Raise upper bound */
-        ++self->maxFramesPerHostBuffer;
+        if( !accurate )
+            ++self->maxFramesPerHostBuffer;

Compared to the patch you were planning to use, it adds this hunk:
@@ -1831,12 +1837,13 @@
     PA_UNLESS( framesPerHostBuffer != 0, paInternalError );
     self->maxFramesPerHostBuffer = framesPerHostBuffer;
 
-    if( !accurate )
+    if( !self->playback.canMmap || !accurate )
     {
         /* Don't know the exact size per host buffer */
         *hostBufferSizeMode = paUtilBoundedHostBufferSize;
         /* Raise upper bound */
-        ++self->maxFramesPerHostBuffer;
+        if( !accurate )
+            ++self->maxFramesPerHostBuffer;
     }
 
 error:

This should fix the bogus increase from 4092 to 4093 and still use the correct mode (paUtilBoundedHostBufferSize) for rw mode. Using the fixed host buffer size sounds horrible in all my tests, it just doesn't work. The reason is that a fixed host buffer size means requests from callbacks look like this:
                /* We've committed to a fixed host buffer size, stick to that */
                framesGot = framesGot >= stream->maxFramesPerHostBuffer ? stream->maxFramesPerHostBuffer : 0;
which means it will wait until it has all of stream->maxFramesPerHostBuffer available before calling the callback, and that's too late, it will invariably lead to an xrun.
Comment 14 Hans de Goede 2008-12-08 14:08:51 EST
(In reply to comment #13)
> Created an attachment (id=326162) [details]
> Fixed patch
> 

Looks good, I already thought we might end up with having to do things this way, but in my test case it worked fine without the hunk.

I've done a new build of portaudio with your latest patch in for rawhide:
http://koji.fedoraproject.org/koji/buildinfo?buildID=73915

I would like to also fix this in F-10, so I plan on also doing a build for F-10 and push it to updates-testing, is that ok with you or do you think that is a bad idea?
Comment 15 Kevin Kofler 2008-12-08 14:37:41 EST
I'm fine with it. I'd push it all the way down to F8, but that's just me. :-)
Comment 16 Fedora Update System 2008-12-10 08:21:45 EST
portaudio-19-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/portaudio-19-6.fc10
Comment 17 Fedora Update System 2008-12-11 03:01:39 EST
portaudio-19-6.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update portaudio'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-11164
Comment 18 Fedora Update System 2008-12-13 09:55:14 EST
portaudio-19-6.fc10, wordwarvi-0.23-2.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update portaudio wordwarvi'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2008-11164
Comment 19 Fedora Update System 2008-12-30 18:48:50 EST
portaudio-19-6.fc10, wordwarvi-0.23-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 20 Fedora Update System 2009-01-02 18:44:15 EST
audacity-1.3.5-0.9.beta.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/audacity-1.3.5-0.9.beta.fc10
Comment 21 Kevin Kofler 2009-01-21 18:40:43 EST
IMHO this should also get fixed in Fedora 9 (which is what this bug was filed against in the first place).
Comment 22 Hans de Goede 2009-01-25 13:28:28 EST
(In reply to comment #21)
> IMHO this should also get fixed in Fedora 9 (which is what this bug was filed
> against in the first place).

As you wish an update is on its way to f9-updates-testing.
Comment 23 Fedora Update System 2009-01-26 20:47:36 EST
portaudio-19-6.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing-newkey update portaudio'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2009-0967
Comment 24 Fedora Update System 2009-02-07 17:25:04 EST
portaudio-19-6.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 25 Kevin Kofler 2009-02-08 03:33:51 EST
Looks like the wordwarvi in F9 also suffers from the same bug as the one in F10 (despite being a much older version). :-( So an update for that is also needed.
Comment 26 Hans de Goede 2009-02-09 14:29:55 EST
(In reply to comment #25)
> Looks like the wordwarvi in F9 also suffers from the same bug as the one in F10
> (despite being a much older version). :-( So an update for that is also needed.

Aw, good point, I'm pushing a fixed version straight to updates (stable).
Comment 27 Fedora Update System 2009-02-28 12:08:48 EST
audacity-1.3.7-0.3.beta.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/audacity-1.3.7-0.3.beta.fc10
Comment 28 Kevin Kofler 2009-02-28 12:34:38 EST
For Audacity, this part of my patch should be removed now:

diff -Nur audacity-src-1.3.7-orig/lib-src/portaudio-v19/src/os/unix/pa_unix_hostapis.c audacity-src-1.3.7/lib-src/portaudio-v19/src/os/unix/pa_unix_hostapis.c
--- audacity-src-1.3.7-orig/lib-src/portaudio-v19/src/os/unix/pa_unix_hostapis.c        2009-01-27 21:51:40.000000000 +0100
+++ audacity-src-1.3.7/lib-src/portaudio-v19/src/os/unix/pa_unix_hostapis.c     2009-02-02 19:08:22.000000000 +0100
@@ -103,4 +103,8 @@
         0   /* NULL terminated array */
     };
 
+#if defined(PA_USE_OSS) && defined(PA_USE_ALSA)
+int paDefaultHostApiIndex = 1;
+#else
 int paDefaultHostApiIndex = 0;
+#endif

because ALSA now has index 0 on __linux__:

/** Note that ALSA is placed before OSS so that the latter is preferred over the
 * latter on Linux.
 */

PaUtilHostApiInitializer *paHostApiInitializers[] =
    {
#ifdef __linux__

#ifdef PA_USE_ALSA
        PaAlsa_Initialize,
#endif

#ifdef PA_USE_OSS
        PaOSS_Initialize,
#endif

#else

#ifdef PA_USE_OSS
        PaOSS_Initialize,
#endif

#ifdef PA_USE_ALSA
        PaAlsa_Initialize,
#endif

#endif  /* __linux__ */


I'm not convinced it was a good idea to change the indexes of the APIs (due to compatibility), but it's how upstream does it. So please remove that portion of my patch, otherwise OSS is the default (which is exactly what my patch was fixing)!

This will presumably also affect portaudio itself if updated to a newer snapshot.
Comment 29 Kevin Kofler 2009-02-28 12:56:48 EST
Actually, I took the liberty to just fix this myself. (Devel and F-10 builds now running.)

Note that this change only affects Audacity >= 1.3.7 and portaudio >= around December 2008, the current portaudio package should either keep the patch as is or change the hunk to look like the upstream solution instead.
Comment 30 Michael Schwendt 2009-02-28 13:59:49 EST
Hard-coding an array index like that is fragile. The default index could have been determined at run-time.
Comment 31 Kevin Kofler 2009-02-28 17:09:48 EST
Well, I suggest you take that up with PortAudio upstream. My paDefaultHostApiIndex change is no longer necessary because ALSA is the default in current snapshots (i.e. what Audacity >= 1.3.7 is using), so I think it's best we don't touch upstream's default device code, there's no immediate need. If you think it can or should be improved, upstream is probably the best place.

Unfortunately, upstream doesn't look very responsive to me, given that I got no feedback at all from upstream about the non-mmap ALSA patch. :-( (Several people from other distributions showed interest and some other distros now ship my patch as well, but upstream didn't even bother rejecting it.) That's really annoying, I'd really like that fix to go upstream!
Comment 32 Fedora Update System 2009-05-13 05:03:53 EDT
audacity-1.3.7-0.7.beta.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/audacity-1.3.7-0.7.beta.fc10
Comment 33 Fedora Update System 2009-05-15 19:29:44 EDT
audacity-1.3.7-0.7.beta.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.