pulseaudio: fix inadvertent assignment in PaPulseAudio_StartStreamCb

[invertego]

This bug was introduced by a fairly late commit in #336 (https://github.com/PortAudio/portaudio/pull/336/commits/c454c37f738f1dff0f6a8330daa3131e841a3d36).

I don't know how to induce this failure mode, so the code after the if statement is untested.

[illuusio]

Thank you for contribution! Seems that I've introduced lot of small code mishaps but good that there is people who find the. Fix is good but your reference commit is not or I just don't spot the change but there where so much cleaning up at the end. That part of code is old. Failure will happen if Pulseaudio server won't let you in. Mostly people use local Pulseaudio and they'll never reach the fail state. If you use other user Pulseaudio for example it will reject you and it should fail.

[cuavas]

Thank you for contribution! Seems that I've introduced lot of small code mishaps but good that there is people who find the.

I’d suggest you enable more compiler warnings. This is easily caught by clang’s -Werror=parentheses and GCC has an option to detect these kinds of errors as well.

[kleinerm]

@illuusio We have a little problem here. I just tested this patch and on my machine it ends with no audio and a hang of my application.

This bug fix exposes a different bug in the handling of stream->isStopped in PaPulseAudio_StartStreamCb() and _PaPulseAudio_ProcessAudio.

Before this patch, the code will "fall-through" this check, and set stream->isStopped = 0; before _PaPulseAudio_ProcessAudio executes the first time. This way the check for

if( stream->isStopped )
{
    return paStreamIsStopped;
}

doesn't trigger and the first processing is done.

With this patch applied, the code will loop until pulseaudioState == PA_STREAM_READY before setting stream->isStopped = 0;. This allows _PaPulseAudio_ProcessAudio to execute while stream->isStopped == 1, so above check triggers, and _PaPulseAudio_ProcessAudio will abort prematurely with return paStreamIsStopped;. This can end in deadlocks and silence, and at least does for my application.

One way to fix this is to set stream->isStopped = 0; already at the beginning of PaPulseAudio_StartStreamCb(). That fixed it for me. I do wonder though if it would be better to remove the if( stream->isStopped ) check in _PaPulseAudio_ProcessAudio?

So this needs a fix in this area before merging it.

[illuusio]

One way to fix this is to set stream->isStopped = 0; already at the beginning of PaPulseAudio_StartStreamCb(). That fixed it for me. I do wonder though if it would be better to remove the if( stream->isStopped ) check in _PaPulseAudio_ProcessAudio?

This can't be fixed like this as streams in Pulseaudio start when they start (one can't really know when but they should be running after READY) and they can be sending stuff before start ends so that is why there is hack. It should be somehow nicer and it should work as described. Gotta take alook at this one more time as this has been pain from the start. I think this should be done in callback and hope for the best as then we can exit start soon as we have created stream.

[invertego]

Converted to a draft to indicate this is not mergeable. @illuusio, if you plan to address this with your own PR, I will close this one. I was reporting this upstream as a favor and am not planning to investigate further at this time.

@cuavas You may wish to back out the trivial fix from MAME in light of the above comments.

[cuavas]

At this point I'd be more inclined to just disable the PulseAudio backend. I'm not going to disable the warning - it's too useful.

[illuusio]

Ok I'll get #852 in and then this latency PR after that there is time to this. If want to make it quicker please check that #852 does not make things worser than they were as I'm about to merge it.

[kleinerm]

I think removing the following isStopped check in _PaPulseAudio_ProcessAudio

    /*
     * When stopped we should stop feeding or recording right away
     */
    if( stream->isStopped )
    {
        return paStreamIsStopped;
    }

would also work, as the immediately following check for if( !stream->pulseaudioIsActive ) will take care of stopping audio output by playing out silence until the whole playback machinery is stopped. So the above check not only causes this bug, but is also redundant.

[illuusio]

Please see #863 which should address this one and help iron bugs out from initialization and stream start code

[RossBencina]

@illuusio so should we close this PR now?

[invertego]

Closing in favor of #863

[illuusio]

@invertego could you test #863 is it fixing your problem?