avoid data race on m_pStream

src/soundio/sounddeviceportaudio.cpp

@@ -94,7 +94,8 @@ SoundDevicePortAudio::SoundDevicePortAudio(UserSettingsPointer config,
           m_framesSinceAudioLatencyUsageUpdate(0),
           m_syncBuffers(2),
           m_invalidTimeInfoCount(0),
-          m_lastCallbackEntrytoDacSecs(0) {
+          m_lastCallbackEntrytoDacSecs(0),
+          m_callbackResult(paAbort) {
     // Setting parent class members:
     m_hostAPI = Pa_GetHostApiInfo(deviceInfo->hostApi)->name;
     m_sampleRate = mixxx::audio::SampleRate::fromDouble(deviceInfo->defaultSampleRate);
@@ -360,10 +361,16 @@ SoundDeviceStatus SoundDevicePortAudio::open(bool isClkRefDevice, int syncBuffer
 #endif
 
     // Start stream
+
+    // Tell the callback that we are starting but not ready yet.
+    // The callback will return paContinue but not access m_pStream
+    // yet.
+    m_callbackResult.store(paContinue, std::memory_order_release);
     err = Pa_StartStream(pStream);
     if (err != paNoError) {
         qWarning() << "PortAudio: Start stream error:" << Pa_GetErrorText(err);
         m_lastError = QString::fromUtf8(Pa_GetErrorText(err));
+        m_callbackResult.store(paAbort, std::memory_order_release);
         err = Pa_CloseStream(pStream);
         if (err != paNoError) {
             qWarning() << "PortAudio: Close stream error:"
@@ -391,18 +398,22 @@ SoundDeviceStatus SoundDevicePortAudio::open(bool isClkRefDevice, int syncBuffer
         m_invalidTimeInfoCount = 0;
         m_clkRefTimer.start();
     }
-    m_pStream = pStream;
+    m_pStream.store(pStream, std::memory_order_release);
+
     return SoundDeviceStatus::Ok;
 }
 
 bool SoundDevicePortAudio::isOpen() const {
-    return m_pStream != nullptr;
+    return m_pStream.load() != nullptr;
 }
 
 SoundDeviceStatus SoundDevicePortAudio::close() {
     //qDebug() << "SoundDevicePortAudio::close()" << m_deviceId;
-    PaStream* pStream = m_pStream;
-    m_pStream = nullptr;
+
+    // Tell the callback to return paAbort. This stops the stream as soon as possible.
+    // After stopping the stream using this technique, Pa_StopStream() still has to be called.
+    m_callbackResult.store(paAbort, std::memory_order_release);
+    PaStream* pStream = m_pStream.load(std::memory_order_relaxed);
     if (pStream) {
         // Make sure the stream is not stopped before we try stopping it.
         PaError err = Pa_IsStreamStopped(pStream);
@@ -415,18 +426,30 @@ SoundDeviceStatus SoundDevicePortAudio::close() {
         if (err < 0) {
             qWarning() << "PortAudio: Stream already stopped:"
                        << Pa_GetErrorText(err) << m_deviceId;
+            m_pStream.store(nullptr, std::memory_order_release);
             return SoundDeviceStatus::Error;
         }
 
-        //Stop the stream.
+        // Stop the stream. Since the start of this function we are returning
+        // paAbort from the callback. paAbort stops the stream as soon as possible.
+        // When stopping the stream using this technique, we still need to call
+        // Pa_StopStream() before starting the stream again.
+
         err = Pa_StopStream(pStream);
-        //PaError err = Pa_AbortStream(m_pStream); //Trying Pa_AbortStream instead, because StopStream seems to wait
-                                                   //until all the buffers have been flushed, which can take a
-                                                   //few (annoying) seconds when you're doing soundcard input.
-                                                   //(it flushes the input buffer, and then some, or something)
-                                                   //BIG FAT WARNING: Pa_AbortStream() will kill threads while they're
-                                                   //waiting on a mutex, which will leave the mutex in an screwy
-                                                   //state. Don't use it!
+        // We should now be able to safely set m_pStream to null
+        m_pStream.store(nullptr, std::memory_order_release);
+
+        // Note: With the use of return value paAbort as described above,
+        // the comment below is not relevant anymore, but I am leaving it
+        // because of the BIG FAT WARNING, just in case.
+        //
+        // Trying Pa_AbortStream instead, because StopStream seems to wait
+        // until all the buffers have been flushed, which can take a
+        // few (annoying) seconds when you're doing soundcard input.
+        // (it flushes the input buffer, and then some, or something)
+        // BIG FAT WARNING: Pa_AbortStream() will kill threads while they're
+        // waiting on a mutex, which will leave the mutex in an screwy
+        // state. Don't use it!
 
         if (err != paNoError) {
             qWarning() << "PortAudio: Stop stream error:"
@@ -455,8 +478,11 @@ QString SoundDevicePortAudio::getError() const {
 }
 
 void SoundDevicePortAudio::readProcess(SINT framesPerBuffer) {
-    PaStream* pStream = m_pStream;
-    if (pStream && m_inputParams.channelCount && m_inputFifo) {
+    PaStream* pStream = m_pStream.load(std::memory_order_acquire);
+    if (!pStream) {
+        return;
+    }
+    if (m_inputParams.channelCount && m_inputFifo) {
         int inChunkSize = framesPerBuffer * m_inputParams.channelCount;
         if (m_syncBuffers == 0) { // "Experimental (no delay)"
 
@@ -586,9 +612,11 @@ void SoundDevicePortAudio::readProcess(SINT framesPerBuffer) {
 }
 
 void SoundDevicePortAudio::writeProcess(SINT framesPerBuffer) {
-    PaStream* pStream = m_pStream;
-
-    if (pStream && m_outputParams.channelCount && m_outputFifo) {
+    PaStream* pStream = m_pStream.load(std::memory_order_acquire);
+    if (!pStream) {
+        return;
+    }
+    if (m_outputParams.channelCount && m_outputFifo) {
         int outChunkSize = framesPerBuffer * m_outputParams.channelCount;
         int writeAvailable = m_outputFifo->writeAvailable();
         int writeCount = outChunkSize;
@@ -820,7 +848,7 @@ int SoundDevicePortAudio::callbackProcessDrift(
             //qDebug() << "callbackProcess read:" << (float)readAvailable / outChunkSize << "Buffer empty";
         }
     }
-    return paContinue;
+    return m_callbackResult.load(std::memory_order_acquire);
 }
 
 int SoundDevicePortAudio::callbackProcess(const SINT framesPerBuffer,
@@ -872,7 +900,7 @@ int SoundDevicePortAudio::callbackProcess(const SINT framesPerBuffer,
             //qDebug() << "callbackProcess read:" << "Buffer empty";
         }
     }
-    return paContinue;
+    return m_callbackResult.load(std::memory_order_acquire);
 }
 
 int SoundDevicePortAudio::callbackProcessClkRef(
@@ -1007,7 +1035,7 @@ int SoundDevicePortAudio::callbackProcessClkRef(
                     << "SoundDevicePortAudio::callbackProcess m_outputParams channel count is zero or less:"
                     << m_outputParams.channelCount;
             // Bail out.
-            return paContinue;
+            m_callbackResult.store(paAbort, std::memory_order_relaxed);
         }
 
         composeOutputBuffer(out, framesPerBuffer, 0, m_outputParams.channelCount);
@@ -1017,7 +1045,7 @@ int SoundDevicePortAudio::callbackProcessClkRef(
 
     updateAudioLatencyUsage(framesPerBuffer);
 
-    return paContinue;
+    return m_callbackResult.load(std::memory_order_acquire);
 }
 
 void SoundDevicePortAudio::updateCallbackEntryToDacTime(

src/soundio/sounddeviceportaudio.h

@@ -59,7 +59,7 @@ class SoundDevicePortAudio : public SoundDevice {
     void updateAudioLatencyUsage(const SINT framesPerBuffer);
 
     // PortAudio stream for this device.
-    PaStream* volatile m_pStream;
+    std::atomic<PaStream*> m_pStream;
     // Struct containing information about this device. Don't free() it, it
     // belongs to PortAudio.
     const PaDeviceInfo* m_deviceInfo;
@@ -84,4 +84,5 @@ class SoundDevicePortAudio : public SoundDevice {
     int m_invalidTimeInfoCount;
     PerformanceTimer m_clkRefTimer;
     PaTime m_lastCallbackEntrytoDacSecs;
+    std::atomic<int> m_callbackResult;
 };