Merge pull request #2497 from Fastigium/preview-fix

Fix crashes/hangs when previewing instrument presets

include/Mixer.h

@@ -224,7 +224,7 @@ class EXPORT Mixer : public QObject
 		return m_playHandles;
 	}
 
-	void removePlayHandles( Track * _track, bool removeIPHs = true );
+	void removePlayHandlesOfTypes( Track * _track, const quint8 types );
 
 	bool hasNotePlayHandles();
 

include/PlayHandle.h

@@ -40,11 +40,10 @@ class PlayHandle : public ThreadableJob
 public:
 	enum Types
 	{
-		TypeNotePlayHandle,
-		TypeInstrumentPlayHandle,
-		TypeSamplePlayHandle,
-		TypePresetPreviewHandle,
-		TypeCount
+		TypeNotePlayHandle = 0x01,
+		TypeInstrumentPlayHandle = 0x02,
+		TypeSamplePlayHandle = 0x04,
+		TypePresetPreviewHandle = 0x08
 	} ;
 	typedef Types Type;
 

include/PresetPreviewPlayHandle.h

@@ -38,6 +38,11 @@ class EXPORT PresetPreviewPlayHandle : public PlayHandle
 	PresetPreviewPlayHandle( const QString& presetFile, bool loadByPlugin = false, DataFile *dataFile = 0 );
 	virtual ~PresetPreviewPlayHandle();
 
+	virtual inline bool affinityMatters() const
+	{
+		return true;
+	}
+
 	virtual void play( sampleFrame* buffer );
 	virtual bool isFinished() const;
 

plugins/GigPlayer/GigPlayer.cpp

@@ -101,7 +101,9 @@ GigInstrument::GigInstrument( InstrumentTrack * _instrument_track ) :
 
 GigInstrument::~GigInstrument()
 {
-	Engine::mixer()->removePlayHandles( instrumentTrack() );
+	Engine::mixer()->removePlayHandlesOfTypes( instrumentTrack(),
+				PlayHandle::TypeNotePlayHandle
+				| PlayHandle::TypeInstrumentPlayHandle );
 	freeInstance();
 }
 

plugins/carlabase/carla.cpp

@@ -188,7 +188,7 @@ CarlaInstrument::CarlaInstrument(InstrumentTrack* const instrumentTrack, const D
 
 CarlaInstrument::~CarlaInstrument()
 {
-    Engine::mixer()->removePlayHandles( instrumentTrack() );
+    Engine::mixer()->removePlayHandlesOfTypes(instrumentTrack(), PlayHandle::TypeNotePlayHandle | PlayHandle::TypeInstrumentPlayHandle);
 
     if (fHost.resourceDir != NULL)
     {

plugins/opl2/opl2instrument.cpp

@@ -217,7 +217,9 @@ opl2instrument::opl2instrument( InstrumentTrack * _instrument_track ) :
 
 opl2instrument::~opl2instrument() {
 	delete theEmulator;
-	Engine::mixer()->removePlayHandles( instrumentTrack() );
+	Engine::mixer()->removePlayHandlesOfTypes( instrumentTrack(),
+				PlayHandle::TypeNotePlayHandle
+				| PlayHandle::TypeInstrumentPlayHandle );
 	delete [] renderbuffer;
 }
 

plugins/sf2_player/sf2_player.cpp

@@ -157,7 +157,9 @@ sf2Instrument::sf2Instrument( InstrumentTrack * _instrument_track ) :
 
 sf2Instrument::~sf2Instrument()
 {
-	Engine::mixer()->removePlayHandles( instrumentTrack() );
+	Engine::mixer()->removePlayHandlesOfTypes( instrumentTrack(),
+				PlayHandle::TypeNotePlayHandle
+				| PlayHandle::TypeInstrumentPlayHandle );
 	freeFont();
 	delete_fluid_synth( m_synth );
 	delete_fluid_settings( m_settings );

plugins/vestige/vestige.cpp

@@ -102,7 +102,9 @@ vestigeInstrument::~vestigeInstrument()
 		knobFModel = NULL;
 	}
 
-	Engine::mixer()->removePlayHandles( instrumentTrack() );
+	Engine::mixer()->removePlayHandlesOfTypes( instrumentTrack(),
+				PlayHandle::TypeNotePlayHandle
+				| PlayHandle::TypeInstrumentPlayHandle );
 	closePlugin();
 }
 

plugins/zynaddsubfx/ZynAddSubFx.cpp

@@ -144,7 +144,9 @@ ZynAddSubFxInstrument::ZynAddSubFxInstrument(
 
 ZynAddSubFxInstrument::~ZynAddSubFxInstrument()
 {
-	Engine::mixer()->removePlayHandles( instrumentTrack() );
+	Engine::mixer()->removePlayHandlesOfTypes( instrumentTrack(),
+				PlayHandle::TypeNotePlayHandle
+				| PlayHandle::TypeInstrumentPlayHandle );
 
 	m_pluginMutex.lock();
 	delete m_plugin;

src/core/EffectChain.cpp

@@ -79,6 +79,8 @@ void EffectChain::loadSettings( const QDomElement & _this )
 {
 	clear();
 
+	// TODO This method should probably also lock the mixer
+
 	m_enabledModel.setValue( _this.attribute( "enabled" ).toInt() );
 
 	const int plugin_cnt = _this.attribute( "numofeffects" ).toInt();
@@ -264,10 +266,14 @@ void EffectChain::clear()
 {
 	emit aboutToClear();
 
+	Engine::mixer()->lock();
+
 	m_enabledModel.setValue( false );
 	for( int i = 0; i < m_effects.count(); ++i )
 	{
 		delete m_effects[i];
 	}
 	m_effects.clear();
+
+	Engine::mixer()->unlock();
 }

src/core/Mixer.cpp

@@ -642,12 +642,32 @@ void Mixer::removePlayHandle( PlayHandle * _ph )
 	{
 		lockPlayHandleRemoval();
 		_ph->audioPort()->removePlayHandle( _ph );
+		bool removedFromList = false;
+		// Check m_newPlayHandles first because doing it the other way around
+		// creates a race condition
+		m_playHandleMutex.lock();
 		PlayHandleList::Iterator it =
-				qFind( m_playHandles.begin(),
-						m_playHandles.end(), _ph );
+				qFind( m_newPlayHandles.begin(),
+						m_newPlayHandles.end(), _ph );
+		if( it != m_newPlayHandles.end() )
+		{
+			m_newPlayHandles.erase( it );
+			removedFromList = true;
+		}
+		m_playHandleMutex.unlock();
+		// Now check m_playHandles
+		it = qFind( m_playHandles.begin(),
+					m_playHandles.end(), _ph );
 		if( it != m_playHandles.end() )
 		{
 			m_playHandles.erase( it );
+			removedFromList = true;
+		}
+		// Only deleting PlayHandles that were actually found in the list
+		// "fixes crash when previewing a preset under high load"
+		// (See tobydox's 2008 commit 4583e48)
+		if ( removedFromList )
+		{
 			if( _ph->type() == PlayHandle::TypeNotePlayHandle )
 			{
 				NotePlayHandleManager::release( (NotePlayHandle*) _ph );
@@ -665,13 +685,13 @@ void Mixer::removePlayHandle( PlayHandle * _ph )
 
 
 
-void Mixer::removePlayHandles( Track * _track, bool removeIPHs )
+void Mixer::removePlayHandlesOfTypes( Track * _track, const quint8 types )
 {
 	lockPlayHandleRemoval();
 	PlayHandleList::Iterator it = m_playHandles.begin();
 	while( it != m_playHandles.end() )
 	{
-		if( ( *it )->isFromTrack( _track ) && ( removeIPHs || ( *it )->type() != PlayHandle::TypeInstrumentPlayHandle ) )
+		if( ( *it )->isFromTrack( _track ) && ( ( *it )->type() & types ) )
 		{
 			( *it )->audioPort()->removePlayHandle( ( *it ) );
 			if( ( *it )->type() == PlayHandle::TypeNotePlayHandle )

src/tracks/BBTrack.cpp

@@ -387,7 +387,9 @@ BBTrack::BBTrack( TrackContainer* tc ) :
 
 BBTrack::~BBTrack()
 {
-	Engine::mixer()->removePlayHandles( this );
+	Engine::mixer()->removePlayHandlesOfTypes( this,
+				PlayHandle::TypeNotePlayHandle
+				| PlayHandle::TypeInstrumentPlayHandle );
 
 	const int bb = s_infoMap[this];
 	Engine::getBBTrackContainer()->removeBB( bb );

src/tracks/InstrumentTrack.cpp

@@ -430,7 +430,10 @@ void InstrumentTrack::silenceAllNotes( bool removeIPH )
 	lock();
 	// invalidate all NotePlayHandles linked to this track
 	m_processHandles.clear();
-	Engine::mixer()->removePlayHandles( this, removeIPH );
+	Engine::mixer()->removePlayHandlesOfTypes( this, removeIPH
+				? PlayHandle::TypeNotePlayHandle
+					| PlayHandle::TypeInstrumentPlayHandle
+				: PlayHandle::TypeNotePlayHandle );
 	unlock();
 }
 

src/tracks/SampleTrack.cpp

@@ -446,7 +446,7 @@ SampleTrack::SampleTrack( TrackContainer* tc ) :
 
 SampleTrack::~SampleTrack()
 {
-	Engine::mixer()->removePlayHandles( this );
+	Engine::mixer()->removePlayHandlesOfTypes( this, PlayHandle::TypeSamplePlayHandle );
 }