LMMS hangs when saving project while automating VSTi's pitch

[Skynse]

No description provided.

[PhysSong]

Please provide

• LMMS version + OS version
• The VSTi that you used
• Steps to reproduce

[Skynse]

I use lmms 1.2.0 rc6 beta and I used the Helm VSTi, when I automate the pitch of what I make in it, it works sometimes, but when I start saving the file rapidly, which is a habit of mine to prevent data loss, Lmms freezes and I have to force close it, it may also freeze

[DomClark]

Reproduced. Saving a VST processes events while waiting to get the settings back from the remote plugin, during which the value of the pitch knob is changed. This calls a slot on the knob, which is run on the main thread by the event loop and attempts to re-lock the plugin mutex to dispatch the pitch change event to the plugin.

[PhysSong]

IIUC the problem is trying to process VST changes while waiting for the response. How can we work around/avoid this?

[DomClark]

I think the pitch changes should be sent from the processing thread. I think this may also be related to the choppy VST automation, especially during export, that I've seen people mention on Discord - the automated value isn't updated until the main thread gets scheduled for sufficiently long, which happens less often during export.

[PhysSong]

the pitch changes should be sent from the processing thread

Well, won't it block the processing thread and the audio playback? Won't it make handling non-automated changes complicated?

the automated value isn't updated until the main thread gets scheduled for sufficiently long, which happens less often during export.

That sounds like a bug to me.

[DomClark]

Well, won't it block the processing thread and the audio playback?

The process call to the VST is on the processing thread and locks the same mutex already, so it's not going to make things worse; in the long run we probably want to get rid of the mutex or trylock it instead. It just seems bizarre to me to have a value, meant to control audio synthesised on the processing thread, being updated instead by the UI thread at some indeterminate time in the future.

Won't it make handling non-automated changes complicated?

I hadn't considered that; it may well do. I haven't looked at the code yet so I can't answer for sure, but if we choose to go down this route I guess we'll find out.

[DomClark]

Making a bunch of connections direct seems to solve this issue, #4552, and the choppy automation on export. I haven't checked whether it is thread-safe, or if any other connections should be changed, but here's an initial diff:

diff --git a/plugins/VstEffect/VstEffectControls.cpp b/plugins/VstEffect/VstEffectControls.cpp
index 21d940ddc..25f14e867 100644
--- a/plugins/VstEffect/VstEffectControls.cpp
+++ b/plugins/VstEffect/VstEffectControls.cpp
@@ -89,7 +89,7 @@ void VstEffectControls::loadSettings( const QDomElement & _this )
 				knobFModel[ i ]->setInitValue( (s_dumpValues.at( 2 ) ).toFloat() );
 			}
 
-			connect( knobFModel[i], SIGNAL( dataChanged() ), this, SLOT( setParameter() ) );
+			connect( knobFModel[i], SIGNAL( dataChanged() ), this, SLOT( setParameter() ), Qt::DirectConnection );
 		}
 
 	}
@@ -385,7 +385,7 @@ manageVSTEffectView::manageVSTEffectView( VstEffect * _eff, VstEffectControls *
 					0.0f, 1.0f, 0.01f, _eff, tr( paramStr ) );
 		}
 		connect( m_vi->knobFModel[ i ], SIGNAL( dataChanged() ), this, 
-								SLOT( setParameter() ) );
+								SLOT( setParameter() ), Qt::DirectConnection );
 		vstKnobs[ i ] ->setModel( m_vi->knobFModel[ i ] );
 	}
 
diff --git a/plugins/vestige/vestige.cpp b/plugins/vestige/vestige.cpp
index 540c8b5ce..89b2656e6 100644
--- a/plugins/vestige/vestige.cpp
+++ b/plugins/vestige/vestige.cpp
@@ -207,7 +207,7 @@ void vestigeInstrument::loadSettings( const QDomElement & _this )
 				knobFModel[ i ]->setInitValue( ( s_dumpValues.at( 2 )).toFloat() );
 			}
 
-			connect( knobFModel[i], SIGNAL( dataChanged() ), this, SLOT( setParameter() ) );
+			connect( knobFModel[i], SIGNAL( dataChanged() ), this, SLOT( setParameter() ), Qt::DirectConnection );
 		}
 	}
 	m_pluginMutex.unlock();
@@ -994,7 +994,7 @@ manageVestigeInstrumentView::manageVestigeInstrumentView( Instrument * _instrume
 			m_vi->knobFModel[ i ] = new FloatModel( (s_dumpValues.at( 2 )).toFloat(),
 				0.0f, 1.0f, 0.01f, castModel<vestigeInstrument>(), tr( paramStr ) );
 		}
-		connect( m_vi->knobFModel[i], SIGNAL( dataChanged() ), this, SLOT( setParameter() ) );
+		connect( m_vi->knobFModel[i], SIGNAL( dataChanged() ), this, SLOT( setParameter() ), Qt::DirectConnection );
 		vstKnobs[i] ->setModel( m_vi->knobFModel[i] );
 	}
 
diff --git a/plugins/zynaddsubfx/ZynAddSubFx.cpp b/plugins/zynaddsubfx/ZynAddSubFx.cpp
index adc337542..c8f0f4c2d 100644
--- a/plugins/zynaddsubfx/ZynAddSubFx.cpp
+++ b/plugins/zynaddsubfx/ZynAddSubFx.cpp
@@ -121,13 +121,13 @@ ZynAddSubFxInstrument::ZynAddSubFxInstrument(
 {
 	initPlugin();
 
-	connect( &m_portamentoModel, SIGNAL( dataChanged() ), this, SLOT( updatePortamento() ) );
-	connect( &m_filterFreqModel, SIGNAL( dataChanged() ), this, SLOT( updateFilterFreq() ) );
-	connect( &m_filterQModel, SIGNAL( dataChanged() ), this, SLOT( updateFilterQ() ) );
-	connect( &m_bandwidthModel, SIGNAL( dataChanged() ), this, SLOT( updateBandwidth() ) );
-	connect( &m_fmGainModel, SIGNAL( dataChanged() ), this, SLOT( updateFmGain() ) );
-	connect( &m_resCenterFreqModel, SIGNAL( dataChanged() ), this, SLOT( updateResCenterFreq() ) );
-	connect( &m_resBandwidthModel, SIGNAL( dataChanged() ), this, SLOT( updateResBandwidth() ) );
+	connect( &m_portamentoModel, SIGNAL( dataChanged() ), this, SLOT( updatePortamento() ), Qt::DirectConnection );
+	connect( &m_filterFreqModel, SIGNAL( dataChanged() ), this, SLOT( updateFilterFreq() ), Qt::DirectConnection );
+	connect( &m_filterQModel, SIGNAL( dataChanged() ), this, SLOT( updateFilterQ() ), Qt::DirectConnection );
+	connect( &m_bandwidthModel, SIGNAL( dataChanged() ), this, SLOT( updateBandwidth() ), Qt::DirectConnection );
+	connect( &m_fmGainModel, SIGNAL( dataChanged() ), this, SLOT( updateFmGain() ), Qt::DirectConnection );
+	connect( &m_resCenterFreqModel, SIGNAL( dataChanged() ), this, SLOT( updateResCenterFreq() ), Qt::DirectConnection );
+	connect( &m_resBandwidthModel, SIGNAL( dataChanged() ), this, SLOT( updateResBandwidth() ), Qt::DirectConnection );
 
 	// now we need a play-handle which cares for calling play()
 	InstrumentPlayHandle * iph = new InstrumentPlayHandle( this, _instrumentTrack );
@@ -137,7 +137,7 @@ ZynAddSubFxInstrument::ZynAddSubFxInstrument(
 			this, SLOT( reloadPlugin() ) );
 
 	connect( instrumentTrack()->pitchRangeModel(), SIGNAL( dataChanged() ),
-				this, SLOT( updatePitchRange() ) );
+				this, SLOT( updatePitchRange() ), Qt::DirectConnection );
 }
 
 
diff --git a/src/core/AutomatableModel.cpp b/src/core/AutomatableModel.cpp
index 0b2a1522b..9602e0b89 100644
--- a/src/core/AutomatableModel.cpp
+++ b/src/core/AutomatableModel.cpp
@@ -416,7 +416,7 @@ void AutomatableModel::linkModel( AutomatableModel* model )
 
 		if( !model->hasLinkedModels() )
 		{
-			QObject::connect( this, SIGNAL( dataChanged() ), model, SIGNAL( dataChanged() ) );
+			QObject::connect( this, SIGNAL( dataChanged() ), model, SIGNAL( dataChanged() ), Qt::DirectConnection );
 		}
 	}
 }
@@ -475,7 +475,7 @@ void AutomatableModel::setControllerConnection( ControllerConnection* c )
 	m_controllerConnection = c;
 	if( c )
 	{
-		QObject::connect( m_controllerConnection, SIGNAL( valueChanged() ), this, SIGNAL( dataChanged() ) );
+		QObject::connect( m_controllerConnection, SIGNAL( valueChanged() ), this, SIGNAL( dataChanged() ), Qt::DirectConnection );
 		QObject::connect( m_controllerConnection, SIGNAL( destroyed() ), this, SLOT( unlinkControllerConnection() ) );
 		m_valueChanged = true;
 		emit dataChanged();
diff --git a/src/core/ControllerConnection.cpp b/src/core/ControllerConnection.cpp
index af398d389..45e36e12f 100644
--- a/src/core/ControllerConnection.cpp
+++ b/src/core/ControllerConnection.cpp
@@ -117,7 +117,7 @@ void ControllerConnection::setController( Controller * _controller )
 	{
 		_controller->addConnection( this );
 		QObject::connect( _controller, SIGNAL( valueChanged() ),
-				this, SIGNAL( valueChanged() ) );
+				this, SIGNAL( valueChanged() ), Qt::DirectConnection );
 	}
 
 	m_ownsController =
diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp
index 90dbf11a6..1494d607f 100644
--- a/src/tracks/InstrumentTrack.cpp
+++ b/src/tracks/InstrumentTrack.cpp
@@ -127,8 +127,8 @@ InstrumentTrack::InstrumentTrack( TrackContainer* tc ) :
 	setName( tr( "Default preset" ) );
 
 	connect( &m_baseNoteModel, SIGNAL( dataChanged() ), this, SLOT( updateBaseNote() ) );
-	connect( &m_pitchModel, SIGNAL( dataChanged() ), this, SLOT( updatePitch() ) );
-	connect( &m_pitchRangeModel, SIGNAL( dataChanged() ), this, SLOT( updatePitchRange() ) );
+	connect( &m_pitchModel, SIGNAL( dataChanged() ), this, SLOT( updatePitch() ), Qt::DirectConnection );
+	connect( &m_pitchRangeModel, SIGNAL( dataChanged() ), this, SLOT( updatePitchRange() ), Qt::DirectConnection );
 	connect( &m_effectChannelModel, SIGNAL( dataChanged() ), this, SLOT( updateEffectChannel() ) );
 }

[PhysSong]

I haven't checked whether it is thread-safe, or if any other connections should be changed

I'll look into the threading issue when I have enough time.

[PhysSong]

@DomClark Can you create a PR with the fix so we can test it in RC7 or RC8?

[PhysSong]

I think updateBaseNote and updateEffectChannel may use direct connections as well. They are not related to VST deadlocks, but there's no point do them always on the UI thread.
Eventually, we may drop signal-slot connections and use direct function calls instead. They are potential causes of threading/performance issues.
Anyway, the suggested fix looks sufficient for 1.2 to me.

[DomClark]

It turns out this approach won't work without some additional tweaking: vestigeInstrument::setParameter uses QObject::sender to figure out which parameter has changed, but sender isn't valid outside of the receiver's thread. This is a problem since the vestigeInstrument lives on the main thread but the call is now from the processing thread. The same issue exists for VstEffectControls.

We could work around this on Qt5 by connecting to a lambda and passing the parameter id that way, e.g.

connect( knobFModel[i], SIGNAL( dataChanged() ), this,
	[this, i]() { setParameter(i); }, Qt::DirectConnection);

but this functionality is not available for Qt4. QSignalMapper isn't an option since it uses sender behind the scenes.

@PhysSong What do you think?

[PhysSong]

Creating dataChanged(Model*) signal may help finding the sender.
Anyway, I want to implement a lightweight replacement of the Qt::DirectConnection eventually.

[DomClark]

Creating dataChanged(Model*) signal may help finding the sender.

I can't see any other way, so I'll do this for Qt4 and use the lambda approach for Qt5. That should keep unnecessary workarounds out of master.

Anyway, I want to implement a lightweight replacement of the Qt::DirectConnection eventually.

Great! That should help avoid some of these headaches.