Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PulseAudio: time is spent perhaps due to insufficient checking of state in PaPulseAudio_CloseStreamCb function #895

Open
tatsuki-makino opened this issue Apr 16, 2024 · 8 comments
Assignees
Labels
P3 Priority: Normal src-pulseaudio PulseAudio Host API in src/hostapi/pulseaudio

Comments

@tatsuki-makino
Copy link

Describe the bug
It uses about 50 seconds (usleep(10000) * 5000) per stream, probably due to insufficient checking of state in PaPulseAudio_CloseStreamCb function.

To Reproduce

  1. Set up PulseAudio as appropriate.
  2. Build PortAudio with PulseAudio enabled.
  3. Launch Audacity 3.4.2.
  4. Wait until the window appears.

Expected behavior
Audacity works smoothly.

Actual behavior
The user has to wait about 10 minutes before the window appears.
When touching a device associated with PulseAudio, we get the feeling that a glitch is accumulating.

Desktop (please complete the following information):

  • OS: FreeBSD
  • OS Version 12.4-STABLE (It has been tested. All versions are affected.)
  • PortAudio version: 88ab584

Additional context
A patch of the experiment is attached.
Instead of just one state, have the inline function PA_STREAM_IS_GOOD check the condition.
However, I do not know if this condition is correct :)
It needs to be verified by someone who is familiar with PulseAudio.
switch and PA_DEBUG are included to know the state.
patch.txt

@illuusio
Copy link
Collaborator

illuusio commented Apr 16, 2024

Thank you for bringing this up. I have to test this. Could you please open PR for this as it's easier to follow than just having patch..

illuusio added a commit to illuusio/portaudio that referenced this issue Apr 18, 2024
…pen (PortAudio#895)

Fixing situation when Portaudio pulseaudio stream is created but never opened
but just closed. application like Audacity does it like that with stream
and that caused long delay before closing as stream could not be in correct state
@illuusio
Copy link
Collaborator

illuusio commented Apr 18, 2024

Now there is fix mostly based on your patch: #897. Could you please test is doesn't cause any problems.

RossBencina pushed a commit that referenced this issue Apr 19, 2024
…pen (#895) (#897)

Fixing situation when Portaudio pulseaudio stream is created but never opened
but just closed. application like Audacity does it like that with stream
and that caused long delay before closing as stream could not be in correct state
@RossBencina
Copy link
Collaborator

#897 has been merged. @tatsuki-makino please confirm that it fixes this issue

@tatsuki-makino
Copy link
Author

Thank you for accepting the fix.
The code at the commit of db778cf can ostensibly use the input and output from Audacity to PulseAudio without any problems.
But, I don't know which states the pa_stream_disconnect and pa_stream_unref can use for streams and which states it can switch to, so I can't say that this is entirely correct :)
Well, this issue would have reached a state where it could be closed.

One thing that still bothers me is that if we leave the condition for exiting this loop after
5000 times, even though it no longer occurs, it is likely to cause some kind of resource leak, so it may be better to make it an application error as soon as it reaches 5000 times.

@RossBencina RossBencina added the src-pulseaudio PulseAudio Host API in src/hostapi/pulseaudio label Apr 26, 2024
@RossBencina RossBencina added the P3 Priority: Normal label Apr 26, 2024
@RossBencina
Copy link
Collaborator

it is likely to cause some kind of resource leak

what kind of resource leak, how?

@tatsuki-makino
Copy link
Author

If the input and output streams are NULL under this condition, it means that pa_stream_unref has been executed before that.
If it repeats 5000 times and escapes the loop, there will be a reference count left.
If that happens, we will have to implement it so that it will retry, and I feel that we will not be able to make the decision to retry because we have no way of knowing that it has happened in the first place.

@illuusio
Copy link
Collaborator

I've been making it less than 5000 as I don't know does any TCP connection will be available after 10 minutes if it's terminating. It should be like 30 secs max.
Probably they should be unref after that loop to make sure that there is no leakage.

@illuusio
Copy link
Collaborator

Could you take a look at #914 which should fix rest of this bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 Priority: Normal src-pulseaudio PulseAudio Host API in src/hostapi/pulseaudio
Projects
None yet
Development

No branches or pull requests

3 participants