Crashes when pushing through MIDI input while cloning a Vestige track

[BluuDreamz]

Step 1: Place a TCO (3rd party VST instrument through Vestige) within the Song Editor and link it to your MIDI
Step 2: Send MIDI key input while cloning the TCO (Vestige linked VST instrument)

LMMS 1.2.0 RC8
Windows 10
MIDI: AKM320

Edit: Specifically occurs only with 3rd party VST instruments loaded through Vestige.
Built-in LMMS instruments are not affected by this step process.

[DomClark]

When you say "TCO", I think you mean "track". "TCO" refers to a block of notes (or sample, etc.) in the song editor. I can't reproduce the issue when cloning the track, but I do get a crash when sending MIDI while deleting the track. I suspect the issues are related, so I'll take a look at this. In the meantime, if you have any other information that might be useful, please let us know.

[DomClark]

@BluuDreamz mentioned on Discord that this bug could also be triggered when loading an VST through the file dialog in VeSTige. I was able to reproduce using this method. The crash when sending MIDI while deleting a track is unrelated - I will open a separate issue for this.

[DomClark]

Here are the relevant parts of a backtrace from the hung thread:

#0  0x00007ffc55c1aa24 in ntdll!ZwWaitForSingleObject () from /c/WINDOWS/SYSTEM32/ntdll.dll
#1  0x00007ffc52359252 in WaitForSingleObjectEx () from /c/WINDOWS/System32/KERNELBASE.dll
#2  0x000000006675659d in QBasicMutex::lockInternal(int) () from /home/Dominic/LMMS/vagrant-build/2019-06-11-165416/Qt5Core.dll
#3  0x0000000069a02f34 in vestigeInstrument::handleMidiEvent (this=0x6ee0020, event=..., time=..., offset=0) at /home/vagrant/lmms/plugins/vestige/vestige.cpp:399
#4  0x00000000004fc006 in InstrumentTrack::processOutEvent (this=0x6e5a7a0, event=..., time=..., offset=0) at /home/vagrant/lmms/src/tracks/InstrumentTrack.cpp:421
#5  0x000000000043bda5 in NotePlayHandle::noteOff (this=0x6e30508, _s=0) at /home/vagrant/lmms/src/core/NotePlayHandle.cpp:376
#6  0x00000000004fb6a0 in InstrumentTrack::processInEvent (this=0x6e5a7a0, event=..., time=..., offset=0) at /home/vagrant/lmms/src/tracks/InstrumentTrack.cpp:285
#7  0x000000000048facc in PianoView::focusOutEvent (this=0x1023f600) at /home/vagrant/lmms/src/gui/PianoView.cpp:686
...
#28 0x0000000004c5b4a8 in QWidget::setVisible(bool) () from /home/Dominic/LMMS/vagrant-build/2019-06-11-165416/Qt5Widgets.dll
#29 0x0000000066c84537 in VstPlugin::toggleEditorVisibility (this=0x100bada0, visible=1) at /home/vagrant/lmms/plugins/vst_base/VstPlugin.cpp:621
#30 0x0000000066c84012 in VstPlugin::showUI (this=0x100bada0) at /home/vagrant/lmms/plugins/vst_base/VstPlugin.cpp:544
#31 0x0000000069a02d83 in vestigeInstrument::loadFile (this=0x6ee0020, _file=...) at /home/vagrant/lmms/plugins/vestige/vestige.cpp:358
#32 0x0000000069a0540d in VestigeInstrumentView::openPlugin (this=0x101ee260) at /home/vagrant/lmms/plugins/vestige/vestige.cpp:721
...

A lock is taken in VeSTige before loading the new plugin. When the plugin is loaded, the GUI is opened, which takes focus away from the instrument track window so LMMS turns off any held notes. In doing so, it tries to send a MIDI message to the plugin, which tries to take the same lock again and hangs.

I guess we ought to do one or both of the following:

• Release the lock before opening the GUI.
• Kill any existing notes for the plugin without sending a note-off event.

[JohannesLorenz]

When we make LMMS realtime safe, incoming MIDI events from the UI must be queued anyways. In the best case, with a realtime safe, lockless ringbuffer. The audio thread (vestigeInstrument::play()) would handle them later (e.g. before it does what it does right now). (I think that should work)

For now, a mutexed ringbuffer or even a mutexed vector might suffice. In the scenario of the issue, the plugin mutex would be locked by the loading thread, and then the ringbuffer would be locked to append the note off events. So we have 2 mutexes instead of one. After that, both will be freed.

@DomClark does that make sense to you? If yes, I'd be happy to play around with a PR.

[DomClark]

That does make sense, and I agree that MIDI events from the UI should be passed to the processing thread for handling there, but I feel this is too big a change for stable-1.2. Unlocking the mutex earlier as I suggested above did indeed fix the hang, but the reason I haven't opened a PR with it yet is that I then hit a different crash later on in the code, which I also want to address.

[JohannesLorenz]

It sounds dangerous to me to release the mutex earlier. E.g., from the code, can you be sure that m_plugin->showUI() must not be mutexed?

Adding a simple ringbuffer doesn't seem that much to me, and who knows how many other issues that would fix?

[JohannesLorenz]

I can't reproduce the cloning bug with a9640c8 (stable-1.2). I connected a virtual keyboard from jack-MIDI that plays a note. In LMMS, this plays a note in a ZynAddSubFX VST.

Now, while the note is playing, I clone the track using the gear wheel. There's a short beep while the new instrument is loading, but no crash.

[JohannesLorenz]

@BluuDreamz Can you still reproduce this with the current stable-1.2 from git? I ask because I can't, maybe it has been solved meanwhile?

[tresf]

with the current stable-1.2 from git?

He's running Windows, is there an expectation that he's going to build the branch from scratch?

I can test on Windows 10 and stable-1.2, but the bug fails to mentions exactly which VST is being used.

Edit: The bug also fails to mention which architecture LMMS (32-bit versus 64-bit) is being tested as well as which architecture VST is being used (32-bit versus 64-bit).