Fix some VST deadlocks/hangs

[DomClark]

Fixes #1008, fixes #1183, fixes #3274.

All problems referred to here I've (re)produced on 64-bit Windows 10.

I've experienced #1183 (or something matching the description), and it is caused by a deadlock during autosave. During the autosave, LMMS sends a message to the plugin (generally IdSaveSettingsToFile) and waits for a response. This message is picked up by the processing thread in RemoteVstPlugin and posted to the main UI thread. Usually the UI thread then receives the message, saves a chunk to a temporary file and sends a response. However, in the case of the crash, the main thread is processing some other window message (from what I've found, it always seems to be WM_SETCURSOR). Messages such as this one, if not handled by a window's WindowProc, are instead sent to the parent window. Thus if the plugin doesn't handle WM_SETCURSOR, it is forwarded to LMMS and the OS waits for the message to be processed. This is the deadlock; LMMS's UI thread is blocking on RemoteVstPlugin's, and vice versa.

This PR fixes the issue by passing true as a second argument to RemotePluginBase::waitForMessage in any method which may be called from LMMS's UI thread, causing LMMS to continue to process events while waiting for the reply. Note that this does not need to be done for ZynAddSubFx, as it runs in a separate window and thus will not forward such window messages to LMMS.

This does, however, introduce a new issue, where the UI thread may attempt to communicate with RemoteVstPlugin in response to a new event while still waiting on a reply to an earlier message. This will cause the UI thread to attempt to acquire the plugin mutex twice and thus block on itself. This isn't an issue usually, as such events tend to be user-input driven and the message loop in RemotePluginBase::waitForMessage specifies QEventLoop::ExcludeUserInputEvents as an argument to QCoreApplication::processEvents. However, autosaving is carried out on the UI thread and is triggered by a timer, and so may cause an issue here. Thus this PR moves autosaving to a new thread, and in general any automatic VST communication should be carried out in a different thread from now on so as to avoid deadlock. Additionally, this also causes message-waiting code that is sometimes called from the UI thread, and sometimes from other threads, to always run a message loop in RemotePluginBase::waitForMessage. This is unnecessary when not on the UI thread and will waste CPU cycles, so this PR also modifies waitForMessage to ignore its second argument if not called from the UI thread.

#1008 is a similar issue, and has the same fix. Kontakt, when loading settings back in, attempts to display a modal dialog. This appears to involve dispatching a message to the VST's parent window, although I have been unable to discover which message this is as it is dispatched and remains undelivered entirely inside the Windows kernel. Nevertheless, the previous changes have fixed this and projects using Kontakt can now be loaded successfully.

This PR also fixes another VST-related hang. Currently, RemoteVstPlugin receives messages in its processing thread, and, unless these are messages designed to be processed in real time (i.e. MIDI events and sound processing), they are sent to be handled by the UI thread by wrapping them in a Windows user message and passing it to PostThreadMessage along with a handle to the UI thread. The UI thread then processes these messages in its message loop as it receives them. However, there is a problem: thread messages are eaten by modal loops. Such modal loops are how Windows implements modal dialogs; thus any message that is sent while a modal dialog is open will disappear, causing a hang if LMMS is waiting for a reply. This can be seen, for example, on 1.2.0-rc3 by opening a modal dialog on a VST plugin (e.g. by clicking Kontakt's 'Options' button), then leaving it open while the autosave kicks in. Autosaving has been moved to a new thread, but there are other messages from which LMMS expects a reply, so the problem should be addressed. This is fixed by using a standard WindowProc as a callback. The dummy window currently used for receiving timer events has been repurposed to receive all messages sent to the UI thread, and the PostThreadMessage calls have been replaced by PostMessage. There are a couple of new synchronization constructs introduced as it is now possible for the VST to re-call the WindowProc whilst handling a message.

Now fixes #2784, fixes #2157, fixes #1468 too.

[zonkmachine]

I've tested the auto-save function. It's running like before. 👍
I don't use a whole lot of vst stuff so someone else will have to test this.

@LMMS/developers Maybe bump this for RC4

[PhysSong]

Maybe bump this for RC4

Good Idea. We should let users test it before stable 1.2.0 release.

[tresf]

@DomClark thanks for this patch. I'm not sure if we have any developers familiar with this logic to be able to properly review so as @zonkmachine and @PhysSong have suggested this is going to get it's best approval if we merge and release with the next release candidate -- RC4.

@DomClark are there any items left todo or is this ready? The code quality and format are top notch and thank you for researching and fixing the winegcc issue via 5c1e48d.

I vote we merge this to stable-1.2 once @DomClark gives the nod.

[DomClark]

Latest commit fixes #2784, #2157. RemoteVstPlugin::updateInOutCount is synchronized with RemoteVstPlugin::process so shared memory is never freed during audio processing. A flag is cleared in RemoteVstPlugin::updateInOutCount to indicate shared memory is invalid, then is set again upon receiving the new shared memory key from LMMS. The handling of the IdChangeSharedMemoryKey message has been moved to the processing thread to ensure the pointer to shared memory is changed outside of any call to RemotePluginClient::doProcessing (I feel this is clearly better than the alternative of continuing to have it sent to the UI thread and synchronizing on that anyway). Also, RemotePluginClient::setInputOutputCount has been added as an alternative to setInputCount and setOutputCount so the shared memory is only reallocated once, stopping the validity flag being set prematurely. (As a side effect, this fixes #1468, which was due to trying to attach the shared memory reallocated by setInputCount when setOutputCount had already started reallocating a second time.)

[zonkmachine]

Fixes #1008, #1183, #3274.

If you want to automatically close multiple issues on merge they each need their own keyword.

[DomClark]

Ah, fixed it, thank you. I'm happy with the code now, you can merge when you're ready.

[Umcaruje]

I don't think I've ever seen so many issues closed by one PR. Amazing job @DomClark