pulseaudio: Rework some initialization code and have better error reporting

[illuusio]

Rework Pulseaudio stream starting and initialization code little bit more readable form. Commit also reworks stream starting to work more reliable.

PR reworks also error reporting as it should be first class citizen if there is better stream handling and initialization.

This should overcome problems from PR #857

[RossBencina]

Please review the documentation for PaUtil_SetLastHostErrorInfo in pa_util.h and let me know if anything is unclear.

[kleinerm]

Just gave this pull request a test and it doesn't work at all, at least on Ubuntu 20.04.6-LTS.

Playback in callback mode just hangs, with the paCallback not ever getting called.

Capture dies with "Assertion 's' failed at pulse/stream.c:335, function pa_stream_get_state(). Aborting."

Backtrace in the capture abort:

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff5408859 in __GI_abort () at abort.c:79
#2  0x00007ffff5174973 in pa_stream_get_state () at /lib/x86_64-linux-gnu/libpulse.so.0
#3  0x00007ffff7fb1bae in _PaPulseAudio_WaitStreamState () at ./libportaudio.so.19.8
#4  0x00007ffff7fb1eef in PaPulseAudio_StartStreamCb () at ./libportaudio.so.19.8
#5  0x00007ffff7f97643 in Pa_StartStream () at ./libportaudio.so.19.8
#6  0x00007fffc62ab45c in PSYCHPORTAUDIOStartAudioDevice () at Common/PsychPortAudio/PsychPortAudio.c:4928
#7  0x00007fffc629f19a in mexFunction (nlhs=<optimized out>, plhs=<optimized out>, nrhs=5, prhs=0x7fffbcb83120) at Common/Base/PsychScriptingGlueMatlab.c:368
#8  0x00007ffff7798524 in call_mex(octave_mex_function&, octave_value_list const&, int) () at /lib/x86_64-linux-gnu/liboctinterp.so.7
#9  0x00007ffff72686a5 in octave_mex_function::call(octave::tree_evaluator&, int, octave_value_list const&) () at /lib/x86_64-linux-gnu/liboctinterp.so.7

It kind of looks as if the same problem as in PR #857 exists, but I don't have time to debug your new changes. See my proposal in that PR though for something that I think might solve that problem in a simple manner.

Sorry for not bringing more pleasant feedback as a christmas present :/.

I still hope that a Portaudio release with the pulseaudio backend could make it into the next big Ubuntu 24.04-LTS release. That would be splendid, but would require new portaudio packages to land in Debian unstable before February 29 2024, which is when Ubuntu stops importing packages from Debian.

Have nice christmas and happy new year!

[dmitrykos]

@illuusio please integrate changes from the master to make CI work/succeed.

[illuusio]

@illuusio please integrate changes from the master to make CI work/succeed.

Thank you for fixing some not working stuff on master. I've updated to this PR to current HEAD.

[illuusio]

Just gave this pull request a test and it doesn't work at all, at least on Ubuntu 20.04.6-LTS.

Playback in callback mode just hangs, with the paCallback not ever getting called.

Capture dies with "Assertion 's' failed at pulse/stream.c:335, function pa_stream_get_state(). Aborting."

Backtrace in the capture abort:

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff5408859 in __GI_abort () at abort.c:79
#2  0x00007ffff5174973 in pa_stream_get_state () at /lib/x86_64-linux-gnu/libpulse.so.0
#3  0x00007ffff7fb1bae in _PaPulseAudio_WaitStreamState () at ./libportaudio.so.19.8
#4  0x00007ffff7fb1eef in PaPulseAudio_StartStreamCb () at ./libportaudio.so.19.8
#5  0x00007ffff7f97643 in Pa_StartStream () at ./libportaudio.so.19.8
#6  0x00007fffc62ab45c in PSYCHPORTAUDIOStartAudioDevice () at Common/PsychPortAudio/PsychPortAudio.c:4928
#7  0x00007fffc629f19a in mexFunction (nlhs=<optimized out>, plhs=<optimized out>, nrhs=5, prhs=0x7fffbcb83120) at Common/Base/PsychScriptingGlueMatlab.c:368
#8  0x00007ffff7798524 in call_mex(octave_mex_function&, octave_value_list const&, int) () at /lib/x86_64-linux-gnu/liboctinterp.so.7
#9  0x00007ffff72686a5 in octave_mex_function::call(octave::tree_evaluator&, int, octave_value_list const&) () at /lib/x86_64-linux-gnu/liboctinterp.so.7

It kind of looks as if the same problem as in PR #857 exists, but I don't have time to debug your new changes. See my proposal in that PR though for something that I think might solve that problem in a simple manner.

Sorry for not bringing more pleasant feedback as a christmas present :/.

I still hope that a Portaudio release with the pulseaudio backend could make it into the next big Ubuntu 24.04-LTS release. That would be splendid, but would require new portaudio packages to land in Debian unstable before February 29 2024, which is when Ubuntu stops importing packages from Debian.

Have nice christmas and happy new year!

Thank you for testing. I've little bit reworked code and hopefully now it works.

[RossBencina]

@kleinerm is it working for you now?

[RossBencina]

If the two changes I've suggested are accepted, and @kleinerm says it works (in either order) then I'm ready to approve this.

[kleinerm]

Apart from my review comments, the problem persists, with exactly the same symptoms as mentioned before:

Hangs when trying to start playback, callback never called.
Dies with assertion when trying to start capture - updated backtrace now:

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff5406859 in __GI_abort () at abort.c:79
#2  0x00007ffff5172973 in pa_stream_get_state () at /lib/x86_64-linux-gnu/libpulse.so.0
#3  0x00007ffff7fb0d45 in _PaPulseAudio_WaitStreamState () at ./libportaudio.so.19.8
#4  0x00007ffff7fb1086 in PaPulseAudio_StartStreamCb () at ./libportaudio.so.19.8
#5  0x00007ffff7f96643 in Pa_StartStream () at ./libportaudio.so.19.8
#6  0x00007fffc62ab3fc in PSYCHPORTAUDIOStartAudioDevice ()
    at /home/kleinerm/projects/OpenGLPsychtoolbox/Psychtoolbox-3/Psychtoolbox/PsychBasic/Octave5LinuxFiles64/PsychPortAudio.mex
#7  0x00007fffc629f13a in mexFunction ()
    at /home/kleinerm/projects/OpenGLPsychtoolbox/Psychtoolbox-3/Psychtoolbox/PsychBasic/Octave5LinuxFiles64/PsychPortAudio.mex
#8  0x00007ffff7796524 in call_mex(octave_mex_function&, octave_value_list const&, int) () at /lib/x86_64-linux-gnu/liboctinterp.so.7
#9  0x00007ffff72666a5 in octave_mex_function::call(octave::tree_evaluator&, int, octave_value_list const&) () at /lib/x86_64-linux-gnu/liboctinterp.so.7

[kleinerm]

The additional needed fix on top of the suggestions i made is to change lines 681 - 682 in pa_linux_pulseaudio_cb.c

portaudio/src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c

Line 681 in 5b9ce72

stream->isActive = 0;

as follows:

-    stream->isActive = 0;
-    stream->isStopped = 1;
+    stream->isActive = 1;
+    stream->isStopped = 0;

The stream isActive and isStopped must start marked as active and not stopped at the beginning of that function. It must only be turned into a failure (ie. not isActive but isStopped) if an error happens, ie. as part of the startstreamcb_error: path.

Otherwise various checks that are called as part of the Pulseaudio processing thread will trigger and cause an abort of _PaPulseAudio_ProcessAudio(), e.g., as part of the PA_PULSEAUDIO_IS_ERROR macro, which always errors out if stream->isStopped or !stream->isActive.

All this is a race condition between the Pulseaudio processing thread and the main Portaudio client thread where PaPulseAudio_StartStreamCb() is executed. Therefore if the problem happens depends somewhat on the relative execution timing and speed of the test machine.

My description of the original problem can be found here #857 (comment).

With all my proposed changes applied, it works again.

[illuusio]

@RossBencina and @kleinerm thank you for reviewing this. I'll update the suggestion as I have some time to what's them thru. They seem to all to good stuff..

[RossBencina]

Please fix the whitespace issues flagged by CI:

error: src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c(770) bad indent: [19](https://github.com/PortAudio/portaudio/actions/runs/7728303666/job/21068771570?pr=863#step:4:20)
b'                   PaPulseAudio_UnLock( pulseaudioHostApi->mainloop );'
error: src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c(861) bad indent: 23
b'                       PaPulseAudio_UnLock( pulseaudioHostApi->mainloop );'
error: src/hostapi/pulseaudio/pa_linux_pulseaudio.c(104) trailing whitespace:
b'        '

[illuusio]

Please fix the whitespace issues flagged by CI:

error: src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c(770) bad indent: [19](https://github.com/PortAudio/portaudio/actions/runs/7728303666/job/21068771570?pr=863#step:4:20)
b'                   PaPulseAudio_UnLock( pulseaudioHostApi->mainloop );'
error: src/hostapi/pulseaudio/pa_linux_pulseaudio_cb.c(861) bad indent: 23
b'                       PaPulseAudio_UnLock( pulseaudioHostApi->mainloop );'
error: src/hostapi/pulseaudio/pa_linux_pulseaudio.c(104) trailing whitespace:
b'        '

Yes I've noted that and didn't got time to fix. It had unfinished things anyway so was not so biggie anyway. I've little bit cleaned up things as I by myself could not remember why I added isActive and isStopped as they are same thing. Long story short logic behind starting playback and record should now work as it should.. I've tested limited and if @kleinerm could once again see it my changes makes any difference.

[RossBencina]

could not remember why I added isActive and isStopped as they are same thing

They are not necessarily the same thing, see: https://www.portaudio.com/docs/proposals/010-ClarifyStreamStateMachine.html

[kleinerm]

Ok, I retested and also reviewed your latest chances. The changes you made to starting are a variation of what I proposed and therefore should work. And indeed, my testing with Psychtoolbox-3 on my Ubuntu 20.04.6-LTS laptop now works again for all tests with half-duplex playback, capture, and full-duplex modes 👍.

As far as I'm concerned, this would be ready for pulling "as is".

I did leave one comment for a cosmetic code fix - removing a now redundant if-statement. I don't care if it is left or not.

Wrt. the change in suggestedLatency handling, I'll leave this to @RossBencina and @philburk , as it doesn't affect my software in any way - I don't ever pass in negative or zero suggestedLatency values. But treating a zero suggestedLatency as DEFAULT_MIN_LATENCY is not strictly what the developers docs for Portaudio recommend - See https://github.com/PortAudio/portaudio/wiki/BufferingLatencyAndTimingImplementationGuidelines#user-model-latency-fields-and-parameters and the new issue #856 as a result of previous discussions we had here a few months ago.

So I'm not sure if working around application bugs by mapping a requested latency of zero to 15 msecs is really the proper thing to do. But I personally don't care.

It is a bit curious why in the old code, a suggestedLatency of zero which maps to a requested latency of 1 microsecond would cause the behaviour you observed though. pa_usec_to_bytes() will map that to 0 Bytes in tlength or fragsize, so maybe that is a value Pulseaudio doesn't like here, and it would make more sense to make sure that tlength and fragsize in PaPulseAudio_StartStreamCb() gets always clamped to be at least 1 Bytes or some other small reasonable value?
Supposedly Pulseaudio will pick higher latency values if one passes in too small value.

Anyhow, I'm ok with this.

[illuusio]

Wrt. the change in suggestedLatency handling, I'll leave this to @RossBencina and @philburk , as it doesn't affect my software in any way - I don't ever pass in negative or zero suggestedLatency values. But treating a zero suggestedLatency as DEFAULT_MIN_LATENCY is not strictly what the developers docs for Portaudio recommend - See https://github.com/PortAudio/portaudio/wiki/BufferingLatencyAndTimingImplementationGuidelines#user-model-latency-fields-and-parameters and the new issue #856 as a result of previous discussions we had here a few months ago.

So I'm not sure if working around application bugs by mapping a requested latency of zero to 15 msecs is really the proper thing to do. But I personally don't care.

Yes this one thing that I've though when implemented. It's not proper thing to do but as 0 latency works incorrect this lesser bad than un working audio but if there is some other advice it would help a lot..

[philburk]

Thanks for persevering with this host implementation. It sounds like you are close.

Clipping zero requested latency to a minimum value is a good thing.
It is best if the application stays between the recommended min and max values.
If an app tries to push outside that range then results are not guaranteed.
A value of zero is, of course, impossible and should be considered a request for the lowest possible latency.
If the latency is pushed extremely low then an app will have to expect an occasional glitch.

[RossBencina]

Suggested latency handling is covered in #99

[RossBencina]

I don't think the zero suggested latency handling is correct but we plan to merge it next week anyhow since the main issue is resolved and the suggested latency handling can be addressed in a separate PR.

[illuusio]

Ok I'll revert my changes and let latency stuff be merged. Then I'll can figure this out if needed or fix this straight to pure data as latency they determine in pd-gui is 25 ms but it's set somehow to 0 when starting portaudio stream which is not correct.

[illuusio]

As now it's working again I'm starting to hope this will be merged soon but let's see if there's something to fix still..

[kleinerm]

Retested on Ubuntu 20.04.6-LTS to be still fine.

Fwiw, I looked at some pulseaudio server code, and lines like these ...
https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/master/src/pulsecore/memblockq.c?ref_type=heads#L863
... suggest that a tlength == 0 is treated like tlength == -1, which is highest latency possible ~2 seconds.

Other code fragments, e.g., https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/master/src/pulsecore/memblockq.c?ref_type=heads#L852 in combination with https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/master/src/pulsecore/memblockq.c?ref_type=heads#L81 suggest that the lowest implementable value may be one audio frame size, ie. pa_frame_size(sample_spec) in that code. Similar, e.g., https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/master/src/pulsecore/protocol-native.c#L412

So what could be worth trying is making sure that
stream->inputBufferAttr.fragsize and stream->outputBufferAttr.tlength
never get lower than pa_frame_size(stream->inputSampleSpec) and pa_frame_size(stream->outputSampleSpec).
Ie. add a check and override assignment in PaPulseAudio_StartStreamCb().

This way 0 usecs or too small usecs would result in an assignment of at least pa_frame_size() bytes.

[RossBencina]

So what could be worth trying is making sure that
stream->inputBufferAttr.fragsize and stream->outputBufferAttr.tlength
never get lower than pa_frame_size(stream->inputSampleSpec) and pa_frame_size(stream->outputSampleSpec).
Ie. add a check and override assignment in PaPulseAudio_StartStreamCb().

This way 0 usecs or too small usecs would result in an assignment of at least pa_frame_size() bytes.

Sounds good to me.

Is there anything remaining for this PR before we merge it? Are we ok to merge this in 7 days?

[kleinerm]

@RossBencina From my side and testing, merging is fine.

[illuusio]

Same to me. I'll address these latency problems in other PR.

[RossBencina]

Seems good to me we've done enough hopefully we didn't miss anything.

[RossBencina]

Thank you everyone :)