Bug 116

Summary:	Crash when removing sampler channel with active streams/voices
Product:	LinuxSampler	Reporter:	Grigor Iliev <gr.iliev>
Component:	gig::Engine	Assignee:	Andreas Persson <andreas56>
Status:	CLOSED FIXED
Severity:	normal	CC:	cuse
Priority:	P2
Version:	SVN Trunk
Hardware:	PC
OS:	Linux
Attachments:	116.patch.bz2 116-2.patch

Description Grigor Iliev 2009-01-25 23:44:01 CET

When removing sampler channel with active streams/voices the DiskThread is
trying to access already deleted streams.
Christian, please take a look at the proposed fix, because I'm not much familiar
with that part of LS and I may miss something.

Comment 1 Grigor Iliev 2009-01-25 23:45:28 CET

Created attachment 44 [details]
116.patch.bz2

Comment 2 Christian Schoenebeck 2009-01-31 13:10:37 CET

I'm not sure if it's ok just to remove the

Engine::instruments.HandBackDimReg(itVoice->pDimRgn);

loop. I talked with Andreas, he added it due to some problem when he 
implemented the host plugins, he said to be able to load instruments while not 
being connected to an audio output device.

Andreas, ... I leave it to you to decide. ;-)

Comment 3 Andreas Persson 2009-02-22 12:08:07 CET

Created attachment 49 [details]
116-2.patch

I had a look again at this bug (sorry for taking so long).

The first thing I got stuck on in your patch was the change from
HandBackInstrument to HandBack, where you could remove the whole HandBackDimReg
loop. I thought that there must have been a good reason that I wrote that code.
But, now I fail to find such a reason. So, I think it's a good change.

As I said in a mail, a problem with KillImmediatelyAllVoices is that it doesn't
wait for the DiskThread to actually delete the streams.

Then I noticed that DisconnectAudioOutputDevice is calling ResetInternal, which
calls Engine::ResetInternal, which calls DiskThread::Reset, which kills all
streams.

So, I think a simpler solution than your patch is to move the ResetInternal
call first in DisconnectAduioOutputDevice.

I attach my version of the patch. What do you think? Am I missing something?

Comment 4 Grigor Iliev 2009-02-22 15:03:44 CET

(In reply to comment #3)
> Then I noticed that DisconnectAudioOutputDevice is calling ResetInternal, which
> calls Engine::ResetInternal, which calls DiskThread::Reset, which kills all
> streams.

I missed that.

> So, I think a simpler solution than your patch is to move the ResetInternal
> call first in DisconnectAduioOutputDevice.
> 
> I attach my version of the patch. What do you think? Am I missing something?

The only disadvantage I see is that, if we have more sampler channels connected
to the same audio device, Engine::ResetInternal call in
DisconnectAduioOutputDevice will kill all active voices in the other sampler
channels too. But I guess we can live with that :)
The patch looks fine to me.

Comment 5 Andreas Persson 2009-02-23 19:30:24 CET

Patch committed