[Portaudio] pa_linux_alsa.c:3636 Assertion failed (with hack fix)

[Chameleon]

Hello all,

I came across this bug/issue when converting my program from the blocking
interface to the callback interface, every so often (up to hours later) my
program would crash with SIGABRT.

I found this https://app.assembla.com/spaces/portaudio/support/tickets/268
from a year ago, but it didn't seem to have a resolution to it. I spent a
few days hacking around with it and eventually came up with a fix, and
posted it to the ticket. I'm writing here to potentially be notified if
anything comes of it.

To sum up the problem: (alsa_)snd_pcm_poll_descriptors can return -32
(Broken Pipe). Currently the portaudio library crashes the program when
alsa does so. Treating it as an xrun seems to work perfectly.

[Alan Horstmann]

Post by Chameleon
Hello all,
I came across this bug/issue when converting my program from the blocking
interface to the callback interface, every so often (up to hours later) my
program would crash with SIGABRT.
I found this https://app.assembla.com/spaces/portaudio/support/tickets/268
from a year ago, but it didn't seem to have a resolution to it. I spent a
few days hacking around with it and eventually came up with a fix, and
posted it to the ticket. I'm writing here to potentially be notified if
anything comes of it.
To sum up the problem: (alsa_)snd_pcm_poll_descriptors can return -32
(Broken Pipe). Currently the portaudio library crashes the program when
alsa does so. Treating it as an xrun seems to work perfectly.

Just to let you know, I have had a quick look at the ticket, and as a result
asked about the function on Alsa-dev list.

BTW when I try to download the file attachment assembla says 'file not ready'.
Could you generate a diff patch please of the 'fix' and put it on this list
thread?

Regards

Alan

[Chameleon]

Sure.

Index: src/hostapi/alsa/pa_linux_alsa.c
===================================================================
--- src/hostapi/alsa/pa_linux_alsa.c (revision 1968)
+++ src/hostapi/alsa/pa_linux_alsa.c (working copy)
@@ -3633,8 +3633,15 @@
PaError result = paNoError;
int ret = alsa_snd_pcm_poll_descriptors( self->pcm, pfds, self->nfds );
(void)ret; /* Prevent unused variable warning if asserts are turned
off */
- assert( ret == self->nfds );
+ /* Chameleon edit: No longer assert here */
+ //assert( ret == self->nfds );

+ /* Chameleon edit: If alsa returns anything else, like -32 for broken
pipe, return -1 so it can be caught */
+ if(ret != self->nfds)
+ {
+ return -1;
+ }
+ /* End Chameleon edit */
self->ready = 0;

return result;
@@ -3801,7 +3808,15 @@
{
/* self->pfds is in effect an array of fds; if necessary,
index past the capture fds */
playbackPfds = self->pfds + (pollCapture ? self->capture.nfds
: 0);
- PA_ENSURE( PaAlsaStreamComponent_BeginPolling(
&self->playback, playbackPfds ) );
+ /* Chameleon edit: No longer PA_ENSURE, catch the broken pipe
error from BeginPolling instead */
+ //PA_ENSURE( PaAlsaStreamComponent_BeginPolling(
&self->playback, playbackPfds ) );
+ int errRes = PaAlsaStreamComponent_BeginPolling(
&self->playback, playbackPfds );
+ if(errRes == -1)
+ {
+ xrun = 1;
+ goto end;
+ }
+ /* End Chameleon edit */
totalFds += self->playback.nfds;
}

Post by Chameleon

Post by Chameleon
Hello all,
I came across this bug/issue when converting my program from the blocking
interface to the callback interface, every so often (up to hours later)

my

Post by Chameleon
program would crash with SIGABRT.
I found this

https://app.assembla.com/spaces/portaudio/support/tickets/268

Post by Chameleon
from a year ago, but it didn't seem to have a resolution to it. I spent a
few days hacking around with it and eventually came up with a fix, and
posted it to the ticket. I'm writing here to potentially be notified if
anything comes of it.
To sum up the problem: (alsa_)snd_pcm_poll_descriptors can return -32
(Broken Pipe). Currently the portaudio library crashes the program when
alsa does so. Treating it as an xrun seems to work perfectly.

Just to let you know, I have had a quick look at the ticket, and as a result
asked about the function on Alsa-dev list.
BTW when I try to download the file attachment assembla says 'file not ready'.
Could you generate a diff patch please of the 'fix' and put it on this list
thread?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Chameleon]

And the diff for modifying one of the examples to trigger the bug:

Index: examples/paex_sine_c++.cpp
===================================================================
--- examples/paex_sine_c++.cpp (revision 1968)
+++ examples/paex_sine_c++.cpp (working copy)
@@ -43,6 +43,8 @@
*/
#include <stdio.h>
#include <math.h>
+#include <chrono>
+#include <thread>
#include "portaudio.h"

#define NUM_SECONDS (5)
@@ -158,6 +160,7 @@
const PaStreamCallbackTimeInfo* timeInfo,
PaStreamCallbackFlags statusFlags)
{
+ std::this_thread::sleep_for(std::chrono::milliseconds(1000));
float *out = (float*)outputBuffer;
unsigned long i;

Post by Chameleon
Sure.
Index: src/hostapi/alsa/pa_linux_alsa.c
===================================================================
--- src/hostapi/alsa/pa_linux_alsa.c (revision 1968)
+++ src/hostapi/alsa/pa_linux_alsa.c (working copy)
@@ -3633,8 +3633,15 @@
PaError result = paNoError;
int ret = alsa_snd_pcm_poll_descriptors( self->pcm, pfds, self->nfds );
(void)ret; /* Prevent unused variable warning if asserts are turned
off */
- assert( ret == self->nfds );
+ /* Chameleon edit: No longer assert here */
+ //assert( ret == self->nfds );
+ /* Chameleon edit: If alsa returns anything else, like -32 for broken
pipe, return -1 so it can be caught */
+ if(ret != self->nfds)
+ {
+ return -1;
+ }
+ /* End Chameleon edit */
self->ready = 0;
return result;
@@ -3801,7 +3808,15 @@
{
/* self->pfds is in effect an array of fds; if necessary,
index past the capture fds */
playbackPfds = self->pfds + (pollCapture ? self->capture.nfds
: 0);
- PA_ENSURE( PaAlsaStreamComponent_BeginPolling(
&self->playback, playbackPfds ) );
+ /* Chameleon edit: No longer PA_ENSURE, catch the broken pipe
error from BeginPolling instead */
+ //PA_ENSURE( PaAlsaStreamComponent_BeginPolling(
&self->playback, playbackPfds ) );
+ int errRes = PaAlsaStreamComponent_BeginPolling(
&self->playback, playbackPfds );
+ if(errRes == -1)
+ {
+ xrun = 1;
+ goto end;
+ }
+ /* End Chameleon edit */
totalFds += self->playback.nfds;
}

Post by Chameleon

Post by Chameleon
Hello all,
I came across this bug/issue when converting my program from the

blocking

Post by Chameleon
interface to the callback interface, every so often (up to hours later)

my

Post by Chameleon
program would crash with SIGABRT.
I found this

https://app.assembla.com/spaces/portaudio/support/tickets/268

Post by Chameleon
from a year ago, but it didn't seem to have a resolution to it. I spent

a

Post by Chameleon
few days hacking around with it and eventually came up with a fix, and
posted it to the ticket. I'm writing here to potentially be notified if
anything comes of it.
To sum up the problem: (alsa_)snd_pcm_poll_descriptors can return -32
(Broken Pipe). Currently the portaudio library crashes the program when
alsa does so. Treating it as an xrun seems to work perfectly.

Just to let you know, I have had a quick look at the ticket, and as a result
asked about the function on Alsa-dev list.
BTW when I try to download the file attachment assembla says 'file not ready'.
Could you generate a diff patch please of the 'fix' and put it on this list
thread?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Alan Horstmann]

Post by Alan Horstmann

Post by Chameleon
Hello all,
I came across this bug/issue when converting my program from the blocking
interface to the callback interface, every so often (up to hours later)
my program would crash with SIGABRT.
I found this
https://app.assembla.com/spaces/portaudio/support/tickets/268 from a year
ago, but it didn't seem to have a resolution to it. I spent a few days
hacking around with it and eventually came up with a fix, and posted it
to the ticket. I'm writing here to potentially be notified if anything
comes of it.
To sum up the problem: (alsa_)snd_pcm_poll_descriptors can return -32
(Broken Pipe). Currently the portaudio library crashes the program when
alsa does so. Treating it as an xrun seems to work perfectly.

Just to let you know, I have had a quick look at the ticket, and as a
result asked about the function on Alsa-dev list.
BTW when I try to download the file attachment assembla says 'file not
ready'. Could you generate a diff patch please of the 'fix' and put it on
this list thread?

The followup on this is that the Alsa list guys confirm that
'snd_pcm_poll_descriptors()' can return negative error codes, which depend
upon which particular plugin it is wrapping. So Portaudio should be checking
for this. Within one of the common plugins EPIPE can be returned as an
indication of XRUN.

I think treating all these errors as XRUN is indeed probably the best option.
However, I would prefer to check and return the actual error code, and keep
the assert, as per the attached patch (quickly coded and only roughly
tested!).

Could you try this out please and report if it is effective (I may easily have
messed up). Comments from anyone also welcome.

Regards

Alan

[Chameleon]

I can confirm that it works (by deliberately causing an xrun by taking too
long in the audiocallback on my system) and from reading the code it looks
correct. I can also confirm that I'm using alsa's dmix plugin.

The only small thing is that PaAlsaStreamComponent_BeginPolling(..) has a
return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on the
alsa/system error code. A (potential) cosmetic bug on a bug fix that stops
a library crash, so I offer it entirely as an observation and not as an
issue that requires attention.

Post by Chameleon

Post by Alan Horstmann

Post by Chameleon
Hello all,
I came across this bug/issue when converting my program from the

blocking

Post by Alan Horstmann

Post by Chameleon
interface to the callback interface, every so often (up to hours later)
my program would crash with SIGABRT.
I found this
https://app.assembla.com/spaces/portaudio/support/tickets/268 from a

year

Post by Alan Horstmann

Post by Chameleon
ago, but it didn't seem to have a resolution to it. I spent a few days
hacking around with it and eventually came up with a fix, and posted it
to the ticket. I'm writing here to potentially be notified if anything
comes of it.
To sum up the problem: (alsa_)snd_pcm_poll_descriptors can return -32
(Broken Pipe). Currently the portaudio library crashes the program when
alsa does so. Treating it as an xrun seems to work perfectly.

Just to let you know, I have had a quick look at the ticket, and as a
result asked about the function on Alsa-dev list.
BTW when I try to download the file attachment assembla says 'file not
ready'. Could you generate a diff patch please of the 'fix' and put it on
this list thread?

The followup on this is that the Alsa list guys confirm that
'snd_pcm_poll_descriptors()' can return negative error codes, which depend
upon which particular plugin it is wrapping. So Portaudio should be checking
for this. Within one of the common plugins EPIPE can be returned as an
indication of XRUN.
I think treating all these errors as XRUN is indeed probably the best option.
However, I would prefer to check and return the actual error code, and keep
the assert, as per the attached patch (quickly coded and only roughly
tested!).
Could you try this out please and report if it is effective (I may easily have
messed up). Comments from anyone also welcome.
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Alan Horstmann]

Post by Chameleon
I can confirm that it works (by deliberately causing an xrun by taking too
long in the audiocallback on my system) and from reading the code it looks
correct. I can also confirm that I'm using alsa's dmix plugin.
The only small thing is that PaAlsaStreamComponent_BeginPolling(..) has a
return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on the
alsa/system error code. A (potential) cosmetic bug on a bug fix that stops
a library crash, so I offer it entirely as an observation and not as an
issue that requires attention.

Thanks for the confirmation. Indeed, 'dmix' was mentioned as one plugin that
would return EPIPE to indicate XRUN.

As far as the return type is concerned, that was something I had also seen as
slightly untidy, but left it in haste to minimise changes. But, since the
return would not be checked by PA_ENSURE(.), it would really be better to
clean it up to an int, and remove 'result' as per this version-2 patch,
attached. Could you check this one instead, please?

Phil, Ross, are you happy with this change?

Regards

Alan

[Chameleon]

Confirmed still working with the version-2 patch.

Post by Alan Horstmann

Post by Chameleon
I can confirm that it works (by deliberately causing an xrun by taking

too

Post by Chameleon
long in the audiocallback on my system) and from reading the code it

looks

Post by Chameleon
correct. I can also confirm that I'm using alsa's dmix plugin.
The only small thing is that PaAlsaStreamComponent_BeginPolling(..) has a
return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on the
alsa/system error code. A (potential) cosmetic bug on a bug fix that

stops

Post by Chameleon
a library crash, so I offer it entirely as an observation and not as an
issue that requires attention.

Thanks for the confirmation. Indeed, 'dmix' was mentioned as one plugin that
would return EPIPE to indicate XRUN.
As far as the return type is concerned, that was something I had also seen as
slightly untidy, but left it in haste to minimise changes. But, since the
return would not be checked by PA_ENSURE(.), it would really be better to
clean it up to an int, and remove 'result' as per this version-2 patch,
attached. Could you check this one instead, please?
Phil, Ross, are you happy with this change?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Alan Horstmann]

Post by Chameleon
Confirmed still working with the version-2 patch.

Great, thanks for the confirmation. Hopefully this can go in.

Regards

Alan

[Tuukka Pasanen]

Hello,

Just asking without testing. Why not use paNoError as return (and return
some portaudio #define error on error)? It's just more portaudio style
way to handle things?

Tuukka

Post by Alan Horstmann

Post by Chameleon
Confirmed still working with the version-2 patch.

Great, thanks for the confirmation. Hopefully this can go in.
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Ross Bencina]

Post by Tuukka Pasanen
Just asking without testing. Why not use paNoError as return (and return
some portaudio #define error on error)? It's just more portaudio style
way to handle things?

It would be ok to convert the host error to a PA error in the caller if
that made sense for some reason. However right now (as if patch 2) the
error is not being converted at all, which is I think a bigger problem
(already noted in my previous message). At a minimum there should be a
comment indicating why the error was dropped.

Ross.

[Ross Bencina]

Hi Alan,

I am no fan of those PA_ENSURE et al macros, however the PA_ENSURE macro
usage that you removed sets the `result` local variable prior to jumping
to the error: label. I think you should do the same and set `result` to
something valid prior to `goto error`.

As a side note: it looks to me as though PaAlsaStream_WaitForFrames has
the potential to go into an infinite loop if the PA_ENSURE in the error:
section fails (since it will just `goto error;` again). But don't let me
distract you.

Ross.

Post by Alan Horstmann

Post by Chameleon
I can confirm that it works (by deliberately causing an xrun by taking too
long in the audiocallback on my system) and from reading the code it looks
correct. I can also confirm that I'm using alsa's dmix plugin.
The only small thing is that PaAlsaStreamComponent_BeginPolling(..) has a
return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on the
alsa/system error code. A (potential) cosmetic bug on a bug fix that stops
a library crash, so I offer it entirely as an observation and not as an
issue that requires attention.

Thanks for the confirmation. Indeed, 'dmix' was mentioned as one plugin that
would return EPIPE to indicate XRUN.
As far as the return type is concerned, that was something I had also seen as
slightly untidy, but left it in haste to minimise changes. But, since the
return would not be checked by PA_ENSURE(.), it would really be better to
clean it up to an int, and remove 'result' as per this version-2 patch,
attached. Could you check this one instead, please?
Phil, Ross, are you happy with this change?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Alan Horstmann]

Hi Ross & others,

Thanks for casting an eye over this, and you are right the handling is not
quite correct.

Having looked over at it again, where lower level errors are considered to be
due to an Xrun, this is not treated as paUnanticipatedHostError at the next
level up, but rather as a recoverable situation. So I have amended the patch
along these lines, not setting an error in the higher function, but still
exiting early and triggering recovery.

As for earlier discussion about return types, it is somewhat a matter of
style, but the PaAlsaStreamComponent_BeginPolling() function is very short
(probably inlined by the compiler) and PaError is not really appropriate. By
returning the actual Alsa error code there is the potential if needed to
respond differently to different Alsa errors (although my understanding ATM
is that recovery action is the appropriate response for all).

So, here is a version-3 patch - a retest would be appreciated!

Regards

Alan

Post by Ross Bencina
Hi Alan,
I am no fan of those PA_ENSURE et al macros, however the PA_ENSURE macro
usage that you removed sets the `result` local variable prior to jumping
to the error: label. I think you should do the same and set `result` to
something valid prior to `goto error`.
As a side note: it looks to me as though PaAlsaStream_WaitForFrames has
section fails (since it will just `goto error;` again). But don't let me
distract you.
Ross.

Post by Alan Horstmann

Post by Chameleon
I can confirm that it works (by deliberately causing an xrun by taking
too long in the audiocallback on my system) and from reading the code it
looks correct. I can also confirm that I'm using alsa's dmix plugin.
The only small thing is that PaAlsaStreamComponent_BeginPolling(..) has
a return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on the
alsa/system error code. A (potential) cosmetic bug on a bug fix that
stops a library crash, so I offer it entirely as an observation and not
as an issue that requires attention.

Thanks for the confirmation. Indeed, 'dmix' was mentioned as one plugin
that would return EPIPE to indicate XRUN.
As far as the return type is concerned, that was something I had also
seen as slightly untidy, but left it in haste to minimise changes. But,
since the return would not be checked by PA_ENSURE(.), it would really be
better to clean it up to an int, and remove 'result' as per this
version-2 patch, attached. Could you check this one instead, please?
Phil, Ross, are you happy with this change?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Phil Burk]

Thanks for working on this.
If all negative error codes are treated as xruns, then might important
errors be ignored?
What happens if a USB device is unplugged when running?
Maybe we should check for specific codes, eg. EPIPE.

Phil Burk

Post by Alan Horstmann
Hi Ross & others,
Thanks for casting an eye over this, and you are right the handling is not
quite correct.
Having looked over at it again, where lower level errors are considered to be
due to an Xrun, this is not treated as paUnanticipatedHostError at the next
level up, but rather as a recoverable situation. So I have amended the patch
along these lines, not setting an error in the higher function, but still
exiting early and triggering recovery.
As for earlier discussion about return types, it is somewhat a matter of
style, but the PaAlsaStreamComponent_BeginPolling() function is very short
(probably inlined by the compiler) and PaError is not really appropriate.
By
returning the actual Alsa error code there is the potential if needed to
respond differently to different Alsa errors (although my understanding ATM
is that recovery action is the appropriate response for all).
So, here is a version-3 patch - a retest would be appreciated!
Regards
Alan

Post by Ross Bencina
Hi Alan,
I am no fan of those PA_ENSURE et al macros, however the PA_ENSURE macro
usage that you removed sets the `result` local variable prior to jumping
to the error: label. I think you should do the same and set `result` to
something valid prior to `goto error`.
As a side note: it looks to me as though PaAlsaStream_WaitForFrames has
section fails (since it will just `goto error;` again). But don't let me
distract you.
Ross.

Post by Alan Horstmann

Post by Chameleon
I can confirm that it works (by deliberately causing an xrun by taking
too long in the audiocallback on my system) and from reading the code

it

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
looks correct. I can also confirm that I'm using alsa's dmix plugin.
The only small thing is that PaAlsaStreamComponent_BeginPolling(..)

has

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
a return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on

the

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
alsa/system error code. A (potential) cosmetic bug on a bug fix that
stops a library crash, so I offer it entirely as an observation and

not

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
as an issue that requires attention.

Thanks for the confirmation. Indeed, 'dmix' was mentioned as one

plugin

Post by Ross Bencina

Post by Alan Horstmann
that would return EPIPE to indicate XRUN.
As far as the return type is concerned, that was something I had also
seen as slightly untidy, but left it in haste to minimise changes.

But,

Post by Ross Bencina

Post by Alan Horstmann
since the return would not be checked by PA_ENSURE(.), it would really

be

Post by Ross Bencina

Post by Alan Horstmann
better to clean it up to an int, and remove 'result' as per this
version-2 patch, attached. Could you check this one instead, please?
Phil, Ross, are you happy with this change?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Alan Horstmann]

Hi Phil,

Bear in mind, without this change any negative return from
snd_pcm_poll_descriptors() will fail the assert; for Portaudio to have worked
with asserts enabled those error returns must be extremely rare! When I
asked on the Alsa-devel list it seemed various plugins could potentially give
different error return values, but there was no suggestion that they needed
handling differently. So I judged at this stage a catch-all approach should
at least be an improvement on the existing code which does not handle the
negative returns at all! Unplugging of USB devices does create tricky
situations indeed.

Alan

Post by Phil Burk
Thanks for working on this.
If all negative error codes are treated as xruns, then might important
errors be ignored?
What happens if a USB device is unplugged when running?
Maybe we should check for specific codes, eg. EPIPE.
Phil Burk

Post by Alan Horstmann
Hi Ross & others,
Thanks for casting an eye over this, and you are right the handling is
not quite correct.
Having looked over at it again, where lower level errors are considered to be
due to an Xrun, this is not treated as paUnanticipatedHostError at the next
level up, but rather as a recoverable situation. So I have amended the patch
along these lines, not setting an error in the higher function, but still
exiting early and triggering recovery.
As for earlier discussion about return types, it is somewhat a matter of
style, but the PaAlsaStreamComponent_BeginPolling() function is very
short (probably inlined by the compiler) and PaError is not really
appropriate. By
returning the actual Alsa error code there is the potential if needed to
respond differently to different Alsa errors (although my understanding ATM
is that recovery action is the appropriate response for all).
So, here is a version-3 patch - a retest would be appreciated!
Regards
Alan

Post by Ross Bencina
Hi Alan,
I am no fan of those PA_ENSURE et al macros, however the PA_ENSURE
macro usage that you removed sets the `result` local variable prior to
jumping to the error: label. I think you should do the same and set
`result` to something valid prior to `goto error`.
As a side note: it looks to me as though PaAlsaStream_WaitForFrames has
the potential to go into an infinite loop if the PA_ENSURE in the
error: section fails (since it will just `goto error;` again). But
don't let me distract you.
Ross.

Post by Alan Horstmann

Post by Chameleon
I can confirm that it works (by deliberately causing an xrun by
taking too long in the audiocallback on my system) and from reading
the code

it

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
looks correct. I can also confirm that I'm using alsa's dmix plugin.
The only small thing is that PaAlsaStreamComponent_BeginPolling(..)

has

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
a return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on

the

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
alsa/system error code. A (potential) cosmetic bug on a bug fix that
stops a library crash, so I offer it entirely as an observation and

not

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
as an issue that requires attention.

Thanks for the confirmation. Indeed, 'dmix' was mentioned as one

plugin

Post by Ross Bencina

Post by Alan Horstmann
that would return EPIPE to indicate XRUN.
As far as the return type is concerned, that was something I had also
seen as slightly untidy, but left it in haste to minimise changes.

But,

Post by Ross Bencina

Post by Alan Horstmann
since the return would not be checked by PA_ENSURE(.), it would really

be

Post by Ross Bencina

Post by Alan Horstmann
better to clean it up to an int, and remove 'result' as per this
version-2 patch, attached. Could you check this one instead, please?
Phil, Ross, are you happy with this change?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

[Ross Bencina]

Hi Alan, Phil,

I'm OK with the new patch. It's a step forward.

A possible improvement: Check, and if the error is not EPIPE (or
whatever else is expected) log a warning, something like "warning:
unexpected error (-42) -- recovering as if XRUN". That would at least
alert us to other conditions that need to be handled (and make it easier
to test unplugging USB devices etc).

Ross.

Post by Alan Horstmann
Hi Phil,
Bear in mind, without this change any negative return from
snd_pcm_poll_descriptors() will fail the assert; for Portaudio to have worked
with asserts enabled those error returns must be extremely rare! When I
asked on the Alsa-devel list it seemed various plugins could potentially give
different error return values, but there was no suggestion that they needed
handling differently. So I judged at this stage a catch-all approach should
at least be an improvement on the existing code which does not handle the
negative returns at all! Unplugging of USB devices does create tricky
situations indeed.
Alan

Post by Phil Burk
Thanks for working on this.
If all negative error codes are treated as xruns, then might important
errors be ignored?
What happens if a USB device is unplugged when running?
Maybe we should check for specific codes, eg. EPIPE.
Phil Burk

Post by Alan Horstmann
Hi Ross & others,
Thanks for casting an eye over this, and you are right the handling is
not quite correct.
Having looked over at it again, where lower level errors are considered to be
due to an Xrun, this is not treated as paUnanticipatedHostError at the next
level up, but rather as a recoverable situation. So I have amended the patch
along these lines, not setting an error in the higher function, but still
exiting early and triggering recovery.
As for earlier discussion about return types, it is somewhat a matter of
style, but the PaAlsaStreamComponent_BeginPolling() function is very
short (probably inlined by the compiler) and PaError is not really
appropriate. By
returning the actual Alsa error code there is the potential if needed to
respond differently to different Alsa errors (although my understanding ATM
is that recovery action is the appropriate response for all).
So, here is a version-3 patch - a retest would be appreciated!
Regards
Alan

Post by Ross Bencina
Hi Alan,
I am no fan of those PA_ENSURE et al macros, however the PA_ENSURE
macro usage that you removed sets the `result` local variable prior to
jumping to the error: label. I think you should do the same and set
`result` to something valid prior to `goto error`.
As a side note: it looks to me as though PaAlsaStream_WaitForFrames has
the potential to go into an infinite loop if the PA_ENSURE in the
error: section fails (since it will just `goto error;` again). But
don't let me distract you.
Ross.

Post by Alan Horstmann

Post by Chameleon
I can confirm that it works (by deliberately causing an xrun by
taking too long in the audiocallback on my system) and from reading
the code

it

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
looks correct. I can also confirm that I'm using alsa's dmix plugin.
The only small thing is that PaAlsaStreamComponent_BeginPolling(..)

has

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
a return type of PaError, which may at some point be used with
Pa_GetErrorText(..) which would then return "Invalid error code" on

the

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
alsa/system error code. A (potential) cosmetic bug on a bug fix that
stops a library crash, so I offer it entirely as an observation and

not

Post by Ross Bencina

Post by Alan Horstmann

Post by Chameleon
as an issue that requires attention.

Thanks for the confirmation. Indeed, 'dmix' was mentioned as one

plugin

Post by Ross Bencina

Post by Alan Horstmann
that would return EPIPE to indicate XRUN.
As far as the return type is concerned, that was something I had also
seen as slightly untidy, but left it in haste to minimise changes.

But,

Post by Ross Bencina

Post by Alan Horstmann
since the return would not be checked by PA_ENSURE(.), it would really

be

Post by Ross Bencina

Post by Alan Horstmann
better to clean it up to an int, and remove 'result' as per this
version-2 patch, attached. Could you check this one instead, please?
Phil, Ross, are you happy with this change?
Regards
Alan
_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio

_______________________________________________
Portaudio mailing list
https://lists.columbia.edu/mailman/listinfo/portaudio