Revamp synchronization with the audio engine (#6881)

The revamp consists of one lock. When the audio thread needs to render audio or another thread wants to run a change, acquiring the lock grants mutual exclusion to do one of the two. The intention is that this will provide stronger guarantees that changes do not run concurrently with the audio thread, as well as having the synchronization mechanism itself be free of data races (verified with TSan).

include/AudioEngine.h

@@ -25,14 +25,13 @@
 #ifndef LMMS_AUDIO_ENGINE_H
 #define LMMS_AUDIO_ENGINE_H
 
-#include <QMutex>
-
-#if (QT_VERSION >= QT_VERSION_CHECK(5,14,0))
-	#include <QRecursiveMutex>
+#ifdef __MINGW32__
+#include <mingw.mutex.h>
+#else
+#include <mutex>
 #endif
 
 #include <QThread>
-#include <QWaitCondition>
 #include <samplerate.h>
 
 #include <vector>
@@ -420,10 +419,6 @@ class LMMS_EXPORT AudioEngine : public QObject
 
 	void clearInternal();
 
-	//! Called by the audio thread to give control to other threads,
-	//! such that they can do changes in the model (like e.g. removing effects)
-	void runChangesInModel();
-
 	bool m_renderOnly;
 
 	std::vector<AudioPort *> m_audioPorts;
@@ -453,8 +448,6 @@ class LMMS_EXPORT AudioEngine : public QObject
 	struct qualitySettings m_qualitySettings;
 	float m_masterGain;
 
-	bool m_isProcessing;
-
 	// audio device stuff
 	void doSetAudioDevice( AudioDevice *_dev );
 	AudioDevice * m_audioDev;
@@ -476,19 +469,7 @@ class LMMS_EXPORT AudioEngine : public QObject
 
 	bool m_clearSignal;
 
-	bool m_changesSignal;
-	unsigned int m_changes;
-	QMutex m_changesMutex;
-#if (QT_VERSION >= QT_VERSION_CHECK(5,14,0))
-	QRecursiveMutex m_doChangesMutex;
-#else
-	QMutex m_doChangesMutex;
-#endif
-	QMutex m_waitChangesMutex;
-	QWaitCondition m_changesAudioEngineCondition;
-	QWaitCondition m_changesRequestCondition;
-
-	bool m_waitingForWrite;
+	std::mutex m_changeMutex;
 
 	friend class Engine;
 	friend class AudioEngineWorkerThread;

src/core/AudioEngine.cpp

@@ -67,6 +67,7 @@ namespace lmms
 using LocklessListElement = LocklessList<PlayHandle*>::Element;
 
 static thread_local bool s_renderingThread;
+static thread_local bool s_runningChange;
 
 
 
@@ -83,19 +84,12 @@ AudioEngine::AudioEngine( bool renderOnly ) :
 	m_newPlayHandles( PlayHandle::MaxNumber ),
 	m_qualitySettings( qualitySettings::Mode::Draft ),
 	m_masterGain( 1.0f ),
-	m_isProcessing( false ),
 	m_audioDev( nullptr ),
 	m_oldAudioDev( nullptr ),
 	m_audioDevStartFailed( false ),
 	m_profiler(),
 	m_metronomeActive(false),
-	m_clearSignal( false ),
-	m_changesSignal( false ),
-	m_changes( 0 ),
-#if (QT_VERSION < QT_VERSION_CHECK(5,14,0))
-	m_doChangesMutex( QMutex::Recursive ),
-#endif
-	m_waitingForWrite( false )
+	m_clearSignal(false)
 {
 	for( int i = 0; i < 2; ++i )
 	{
@@ -165,8 +159,6 @@ AudioEngine::AudioEngine( bool renderOnly ) :
 
 AudioEngine::~AudioEngine()
 {
-	runChangesInModel();
-
 	for( int w = 0; w < m_numWorkers; ++w )
 	{
 		m_workers[w]->quit();
@@ -232,17 +224,13 @@ void AudioEngine::startProcessing(bool needsFifo)
 	}
 
 	m_audioDev->startProcessing();
-
-	m_isProcessing = true;
 }
 
 
 
 
 void AudioEngine::stopProcessing()
 {
-	m_isProcessing = false;
-
 	if( m_fifoWriter != nullptr )
 	{
 		m_fifoWriter->finish();
@@ -447,8 +435,6 @@ void AudioEngine::renderStageMix()
 
 	emit nextAudioBuffer(m_outputBufferRead);
 
-	runChangesInModel();
-
 	// and trigger LFOs
 	EnvelopeAndLfoParameters::instances()->trigger();
 	Controller::triggerFrameCounter();
@@ -459,6 +445,8 @@ void AudioEngine::renderStageMix()
 
 const surroundSampleFrame *AudioEngine::renderNextBuffer()
 {
+	const auto lock = std::lock_guard{m_changeMutex};
+
 	m_profiler.startPeriod();
 	s_renderingThread = true;
 
@@ -811,57 +799,16 @@ void AudioEngine::removePlayHandlesOfTypes(Track * track, PlayHandle::Types type
 
 void AudioEngine::requestChangeInModel()
 {
-	if( s_renderingThread )
-		return;
-
-	m_changesMutex.lock();
-	m_changes++;
-	m_changesMutex.unlock();
-
-	m_doChangesMutex.lock();
-	m_waitChangesMutex.lock();
-	if (m_isProcessing && !m_waitingForWrite && !m_changesSignal)
-	{
-		m_changesSignal = true;
-		m_changesRequestCondition.wait( &m_waitChangesMutex );
-	}
-	m_waitChangesMutex.unlock();
+	if (s_renderingThread || s_runningChange) { return; }
+	m_changeMutex.lock();
+	s_runningChange = true;
 }
 
-
-
-
 void AudioEngine::doneChangeInModel()
 {
-	if( s_renderingThread )
-		return;
-
-	m_changesMutex.lock();
-	bool moreChanges = --m_changes;
-	m_changesMutex.unlock();
-
-	if( !moreChanges )
-	{
-		m_changesSignal = false;
-		m_changesAudioEngineCondition.wakeOne();
-	}
-	m_doChangesMutex.unlock();
-}
-
-
-
-
-void AudioEngine::runChangesInModel()
-{
-	if( m_changesSignal )
-	{
-		m_waitChangesMutex.lock();
-		// allow changes in the model from other threads ...
-		m_changesRequestCondition.wakeOne();
-		// ... and wait until they are done
-		m_changesAudioEngineCondition.wait( &m_waitChangesMutex );
-		m_waitChangesMutex.unlock();
-	}
+	if (s_renderingThread || !s_runningChange) { return; }
+	m_changeMutex.unlock();
+	s_runningChange = false;
 }
 
 bool AudioEngine::isAudioDevNameValid(QString name)
@@ -1297,29 +1244,12 @@ void AudioEngine::fifoWriter::run()
 		auto buffer = new surroundSampleFrame[frames];
 		const surroundSampleFrame * b = m_audioEngine->renderNextBuffer();
 		memcpy( buffer, b, frames * sizeof( surroundSampleFrame ) );
-		write( buffer );
+		m_fifo->write(buffer);
 	}
 
 	// Let audio backend stop processing
-	write( nullptr );
+	m_fifo->write(nullptr);
 	m_fifo->waitUntilRead();
 }
 
-
-
-
-void AudioEngine::fifoWriter::write( surroundSampleFrame * buffer )
-{
-	m_audioEngine->m_waitChangesMutex.lock();
-	m_audioEngine->m_waitingForWrite = true;
-	m_audioEngine->m_waitChangesMutex.unlock();
-	m_audioEngine->runChangesInModel();
-
-	m_fifo->write( buffer );
-
-	m_audioEngine->m_doChangesMutex.lock();
-	m_audioEngine->m_waitingForWrite = false;
-	m_audioEngine->m_doChangesMutex.unlock();
-}
-
 } // namespace lmms