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

avoid data race on m_pStream #13899

Open
wants to merge 1 commit into
base: 2.5
Choose a base branch
from

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Nov 17, 2024

This fixes several tsan warnings related with stopping / closing portaudio streams, including #13870.

Apart from access to a non-atomic variable m_pStream, the real problem here was that
readProcess() writeProcess() would take a copy of m_pStream:

PaStream* pStream = m_pStream;

and close() would then call Pa_StopStream(m_pStream); Apparently stopping a stream and still accessing it in the callback leads to data races, at least on macOS.

Now, it the portaudio docs I read:

Pa_StopStream() is designed to make sure that the buffers you've processed in your callback are all played, which may cause some delay. Alternatively, you could call Pa_AbortStream(). On some platforms, aborting the stream is much faster and may cause some data processed by your callback not to be played.
Another way to stop the stream is to return either paComplete, or paAbort from your callback. paComplete ensures that the last buffer is played whereas paAbort stops the stream as soon as possible. If you stop the stream using this technique, you will need to call Pa_StopStream() before starting the stream again.

So what I do: in close() I set an atomic variable to STOP, to indicate that readProcess and writeProcess should not access the stream anymore, and to indicate that the callback function should return paAbort, which will stop the stream. Pa_StopStream can then safely be called and m_pStream set to null.

As a side effect this also addresses a comment in the code about a failed intent to use Pa_AbortStream instead of PaStopStream.

This works well on macOS and I have not seen any portaudio related tsan warnings anymore. But since the platform specific aspects of Portaudio it is important to ensure this works as expected on Windows and Linux.

@daschuer
Copy link
Member

m_pStream is meant to be an atomic. I think we can use the null state of this pointer instead of the additional atomic

PaStream* volatile m_pStream;

@@ -820,7 +856,9 @@ int SoundDevicePortAudio::callbackProcessDrift(
//qDebug() << "callbackProcess read:" << (float)readAvailable / outChunkSize << "Buffer empty";
}
}
return paContinue;
return m_streamState.load(std::memory_order_acquire) == StreamState::STOP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not used for syncing. Does relax also work? We may avoid the condition to store the return value directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants