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

WaitCondition in JACK backend mixes up two clocks #795

Closed
s09bQ5 opened this issue Mar 18, 2023 · 5 comments · Fixed by #877
Closed

WaitCondition in JACK backend mixes up two clocks #795

s09bQ5 opened this issue Mar 18, 2023 · 5 comments · Fixed by #877
Labels
P2 Priority: High src-jack JACK Audio Connection Kit Host API /src/hostapi/jack

Comments

@s09bQ5
Copy link

s09bQ5 commented Mar 18, 2023

Describe the bug
WaitCondition in src/hostapi/jack/pa_jack.c uses PaUtil_GetTime to calculate a timeout and then uses that timeout value with pthread_cond_timedwait. By default the timeouts passed to pthread_cond_timedwait must be based on CLOCK_REALTIME. PaUtil_GetTime on the other hand prefers to use CLOCK_MONOTONIC.

Usually CLOCK_MONOTONIC is much smaller than CLOCK_REALTIME so that pthread_cond_timedwait will timeout immediately.

To Reproduce

  1. Try to use the JACK backend.

Expected behavior
It should work

Actual behavior
It doesn't work

Desktop (please complete the following information):

  • OS: Linux
  • OS Version: 5.15
  • PortAudio version: master branch
  • If Windows or Linux, which Host API (e.g. WASAPI): JACK

Additional context
I see three solutions:

  1. The easiest fix is to use clock_gettime(CLOCK_REALTIME, &ts) instead of PaUtil_GetTime. It also simplifies the code because we don't have to convert from PaTime back to struct timespec. The drawback is the one that led to the introduction of CLOCK_MONOTONIC: It won't work if someone changes the time significantly while we are waiting.
  2. We could use pthread_condattr_setclock. This is available since IEEE 1003.1j-2000. A bit more code is needed (pthread_condattr_init/pthread_condattr_destroy).
  3. we could use the Glibc extension pthread_cond_clockwait. Available since Glibc 2.30 (released in 2019) but not every platform with JACK uses Glibc.
@RossBencina RossBencina added the src-jack JACK Audio Connection Kit Host API /src/hostapi/jack label Mar 21, 2023
@RossBencina
Copy link
Collaborator

Can you give references to the relevant source code functions and line numbers please?

Is this timeout time-critical? If it's some kind of high-precision scheduling timeout then that is different than if it is just a slow last-ditch timeout.

The easiest fix is to use clock_gettime(CLOCK_REALTIME, &ts)

Is clock_gettime available on all platforms with JACK?

@s09bQ5
Copy link
Author

s09bQ5 commented Mar 28, 2023

err = pthread_cond_timedwait( &hostApi->cond, &hostApi->mtx, &ts );

The timeout is a 10 minute last ditch timeout.

Is clock_gettime available on all platforms with JACK?

Since pa_win_hostapis.c contains an #if PA_USE_JACK line, I'd say no. But then the question is, why does Windows have pthread_cond_timedwait?

It is also not supported before macOS 10.12 (released in 2016). Linux gained support in 2003. Here they list a few other UNIXes: https://www.rubydoc.info/stdlib/core/Process.clock_gettime

If clock_gettime does not exist and we are not on windows, the following works:

struct timeval tv;
gettimeofday(&tv, NULL)
ts.tv_sec = tv.tv_sec;
ts.tv_nsec = tv.tv_usec * 1000;

gettimeofday has been around forever. POSIX.1-2008 marked it as deprecated in favor of clock_gettime, but everyone still supports it.

@RossBencina
Copy link
Collaborator

RossBencina commented Mar 30, 2023

Thanks. It sounds like CLOCK_MONOTONIC provides the more robust solution when it's available.

Is clock_gettime available on all platforms with JACK?

Since pa_win_hostapis.c contains an #if PA_USE_JACK line, I'd say no. But then the question is,
why does Windows have pthread_cond_timedwait?

I think you need to link with a Windows pthread library. I seem to remember that being discussed. I've never tried JACK on Windows myself. I guess that means that this same bug exists on Windows (?and Macintosh), since PaUtil_GetTime almost certainly does not use the same time base as the Windows pthread library.

FWIW here's a Windows implementation of gettimeofday:

https://stackoverflow.com/questions/10905892/equivalent-of-gettimeofday-for-windows

@RossBencina
Copy link
Collaborator

RossBencina commented Apr 3, 2023

On Linux, CLOCK_MONOTONIC does not count time in suspend. This would create its own distortions to the timeout (to me, probably more problematic than the distortions caused by using CLOCK_REALTIME). On Linux you can use CLOCK_BOOTTIME to get a monotonic clock that does advance with wall time. See e.g. https://stackoverflow.com/questions/3523442/difference-between-clock-realtime-and-clock-monotonic#:~:text=As%20Ignacio%20and%20MarkR%20say,fixed%20point%20in%20the%20past

https://pubs.opengroup.org/onlinepubs/9699919799/functions/clock_getres.html

Seems like there are three things here:

  1. PaUtil_GetTime needs to be taken out of the picture. Don't use it here.
  2. Right now the code is broken. Fixing it by directly using the clock that is consistent with pthread_cond_timedwait is the first priority.
  3. Choosing the best clock on each target platform (Windows, Mac, Linux) would be a better fix. On Linux it looks like CLOCK_BOOTTIME might be the best choice.

We should add a caveat to PaUtil_GetTime that it guarantees no specific clock domain. Indeed it should probably be using CLOCK_BOOTTIME on Linux as well.

@RossBencina RossBencina added the P2 Priority: High label Apr 3, 2023
RossBencina added a commit to RossBencina/portaudio that referenced this issue Apr 4, 2023
RossBencina added a commit that referenced this issue Apr 10, 2023
…k is required. (#806)

This is an attempt to stop #795 happening again
@RossBencina
Copy link
Collaborator

RossBencina commented Jan 20, 2024

@s09bQ5 I have made a PR that should resolve this. Would you be able to see if it works for you please?, and, if you have time, take a look at the code and see if it looks OK?

Here it is: #877

RossBencina added a commit that referenced this issue Feb 23, 2024
* Ensure that the clock that is configured on the cond var object is the one that is used to compute the wait deadline.

* Ensure that the most appropriate clock is selected on each platform based on availability CLOCK_BOOTTIME > CLOCK_MONOTONIC > CLOCK_REALTIME (on Mac use gettimeofday because other clock interfaces are either not available or broken).

NB: use GetSystemTimeAsFileTime on Windows. should be much faster than GetSystemTime+SystemTimeToFileTime according to http://www.windowstimestamp.com/description. thanks s09bQ5

Used directly or indirectly by pa_jack, pa_alsa, pa_asihpi

Resolves #795, #807.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Priority: High src-jack JACK Audio Connection Kit Host API /src/hostapi/jack
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants