Issue with Vestige + Helm

[wtpisaac]

I've noticed that when trying to use Helm as a VST under Windows, running on LMMS v1.2.0rc4, attempting to open a menu in Helm does not work.

Expected Behavior: Clicking on Distortion menu in the Helm synth opens the Distortion menu. Right clicking opens the right click menu.

Actual behavior: These menus open, but then close immediately, too quickly to interact with anything in it. Very rarely, it will work, which I notice causes Helm's animations to stop.

I believe this is an issue with LMMS (specifically vestige?) as the standalone Helm application does not have this issue on Windows, nor does this issue occur in other DAWs.

[tresf]

Tagging @DomClark -- our resident expert with VSTs. :)

[DomClark]

Successfully reproduced. I've found one other host where this problem occurs: Hermann Seib's VSTHost when the plugin is running with a different number of bits to that of the host, in which case it is loaded in a separate bridge process, like LMMS does. Presumably having the parent window in a different process messes something up somewhere. I'm quite busy with university work currently, but I'll take a look when I have time. In the meantime, the value can be changed by hovering over the menu and scrolling.

[DomClark]

The bug appears to be in JUCE.

Through some internal logic while opening a popup menu, execution reaches MouseSourceState::checkButtonState. From line 1039, the code is

if (! window.doesAnyJuceCompHaveFocus())
{
    if (timeNow > window.lastFocusedTime + 10)
    {
        PopupMenuSettings::menuWasHiddenBecauseOfAppChange = true;
        window.dismissMenu (nullptr);
        // Note: this object may have been deleted by the previous call..
    }
}

MenuWindow::doesAnyJuceCompHaveFocus is as follows:

bool doesAnyJuceCompHaveFocus()
{
    bool anyFocused = Process::isForegroundProcess();

    if (anyFocused && Component::getCurrentlyFocusedComponent() == nullptr)
    {
        // ...
    }

    return anyFocused;
}

And finally the Win32 version of Process::isForegroundProcess:

bool JUCE_CALLTYPE Process::isForegroundProcess()
{
    if (auto fg = GetForegroundWindow())
    {
        DWORD processID = 0;
        GetWindowThreadProcessId (fg, &processID);

        return processID == GetCurrentProcessId();
    }

    return true;
}

GetForegroundWindow() returns a handle to LMMS's window, then GetWindowThreadProcessId(fg, &processID) sets processID to LMMS's process ID. GetCurrentProcessId() returns RemoteVstPlugin's process ID, so isForegroundProcess() returns false. Then doesAnyJuceCompHaveFocus() returns false, and the code inside the initial if-statement executes. This immediately dismisses the menu, unless timeNow <= window.lastFocusedTime + 10, which is presumably when it "very rarely" works.

The problem here is the assumption in Process::isForegroundProcess that when interacting with the plugin, the foreground window belongs to the process in which the plugin is executing. In the case where a bridge process is used and the plugin is embedded inside the host window, this is not true.

I can't think of any obvious way we can work around this on our end; somebody should probably just file an issue with JUCE.

[DomClark]

Here is a pair of Helm builds with Process::isForegroundProcess patched to always return true, which should behave slightly better. Build is of master at time of writing, at commit 81a65d0. Standard "this may break stuff" disclaimers apply.

helmpatched.zip

[zonkmachine]

somebody should probably just file an issue with JUCE.

Maybe this is a job for the Helm devs?
JUCE issue tracker here: https://github.com/WeAreROLI/JUCE

[tresf]

@DomClark excellent investigation. Can you please open a bug at JUCE describing the issue?

[PhysSong]

Can be reproduced on Windows, but not on Linux. FYI one can work around this issue once #3786 is merged, by using separate VST window mode. Note that this isn't a complete fix...

[zonkmachine]

@DomClark Can you please open an issue with JUCE?
https://github.com/WeAreROLI/JUCE
I wouldn't know how to word it.

Also, bumping helm @mtytel

[DomClark]

Ah sorry, I completely forgot about this! I'll open an issue there soon.

[zonkmachine]

Seem to be a problem with Dexed as well (forum comment).

[zonkmachine]

Through some internal logic while opening a popup menu, execution reaches MouseSourceState::checkButtonState. From line 1039, the code is

if (! window.doesAnyJuceCompHaveFocus())
{
     if (timeNow > window.lastFocusedTime + 10)
     {

The lines just ahead of this code recently changed. Don't know if it applies to this case though.

[DomClark]

I've found a workaround here: if one clears the WS_CHILD style from the LVSL window, then GetForegroundWindow assumes it's a top-level window and will return its handle when opening the menu, so isForegroundProcess behaves as desired and everything works properly. This doesn't have any immediately obvious side effects, but I'm going to test it out for a while as this is somewhat abusing Win32 and may have consequences somewhere.
This also fixes Sylenth's menus as well as #4424.

[zonkmachine]

Fine, but please open an issue up-stream. I can't do this myself out of lack of competence on the matter.

[DomClark]

I've had a look at what some other DAWs do and it turns out FL also uses the WS_CHILD trick, so it should be safe to include.
JUCE issue opened here: juce-framework/JUCE#401.

[zonkmachine]

I've had a look at what some other DAWs do and it turns out FL also uses the WS_CHILD trick, so it should be safe to include.

As there seem to be no action in WeAreROLI/JUCE#401 maybe it's best to fix this on our side as you've suggested?

[DomClark]

Maybe it's best to fix this on our side as you've suggested?

Sure. Although I think this should go into master, not stable-1.2, since Lukas-W wants #2826 in master which seems a more minor change than this.

[zonkmachine]

Sure. Although I think this should go into master

I agree.

[DomClark]

Milestoned for 1.3. In the meantime please use "No embedding" for the "Plugin embedding" option as a workaround.

[zonkmachine]

This issue has been cleaned from off topic posts

[jpcima]

Hi I'm also impacted jpcima/ADLplug#53 and so is JuceOPLVSTi.
From what I observed, it's only LMMS of the hosts tried which exhibits this sort of problem.
The very latest JUCE from develop was not helpful.

What's the advice for a plugin developer to work around this?
a recommended patch that I can introduce currently in my release builds?

[DomClark]

@jpcima This bug should manifest itself in any host which bridges plugins and embeds plugin UIs in the host's main window, unless it specifically works around the issue (e.g. FL Studio). Another such host is VSTHost (when the plugin architecture differs from that of VSTHost), and there may be more that I'm not aware of. We intend to add a workaround, but it won't land in our next stable release.

For a workaround to use in your plugin, you can patch juce::Process::isForegroundProcess to always return true. It should suffice to only do this for the Win32 version (in modules/juce_gui_basics/native/juce_win32_Windowing.cpp), since LMMS (and VSTHost) only currently support Windows VST plugins. I haven't tested this thoroughly for any undesirable side effects, but it does fix the bug. Alternatively, you can instruct users to change the plugin embedding mode in LMMS to "no embedding". This option is available in our latest beta release, and will be in a stable release soon (hopefully).

[DomClark]

The following patch works on Windows, but I haven't tested it on Linux yet. PhysSong reports that the bug doesn't occur on Linux, so it may not need to be enabled there anyway. I haven't found any undesirable side effects yet, unlike the approach of not setting WS_CHILD on the wrapper.

diff --git a/plugins/vst_base/RemoteVstPlugin.cpp b/plugins/vst_base/RemoteVstPlugin.cpp
index 942c9e960..121f04b00 100644
--- a/plugins/vst_base/RemoteVstPlugin.cpp
+++ b/plugins/vst_base/RemoteVstPlugin.cpp
@@ -282,6 +282,8 @@ public:
 	// has to be called as soon as input- or output-count changes
 	int updateInOutCount();
 
+	HWND pluginHwnd() const { return m_window; }
+
 	inline void lockShm()
 	{
 		m_shmLock.lock();
@@ -435,6 +437,55 @@ private:
 } ;
 
 
+// Workaround for https://github.com/juce-framework/JUCE/issues/401.
+// JUCE tests if it has focus by comparing its process ID to that of the process
+// to which the currently active window belongs. This is incorrect for bridged,
+// embedded plugins; when the host's window is active, the bridge's process ID
+// won't the match the active window's process ID, i.e. that of the host.
+// We work around this by patching `GetWindowThreadProcessId` to return the
+// bridge's process ID when given a handle to the host's top level window.
+DWORD WINAPI GetWindowThreadProcessIdPatch(HWND hWnd, LPDWORD lpdwProcessId)
+{
+	// Is `hWnd` the top level window that contains the plugin?
+	if (GetAncestor(__plugin->pluginHwnd(), GA_ROOT) == hWnd)
+	{
+		// If so, pretend that it belongs to the current process.
+		if(lpdwProcessId) { *lpdwProcessId = GetCurrentProcessId(); }
+		return GetCurrentThreadId();
+	}
+	// Otherwise use the default behavior.
+	return GetWindowThreadProcessId(hWnd, lpdwProcessId);
+}
+
+// Walk the import table of the plugin and replace `GetWindowThreadProcessId`
+// with our own version.
+void injectJuceMenuWorkaround(HINSTANCE pluginLibrary)
+{
+	const auto base = reinterpret_cast<std::intptr_t>(pluginLibrary);
+	const auto dosHeader = reinterpret_cast<PIMAGE_DOS_HEADER>(base);
+	const auto ntHeaders = reinterpret_cast<PIMAGE_NT_HEADERS>(base + dosHeader->e_lfanew);
+	const auto& importDir = ntHeaders->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_IMPORT];
+	for(auto importDesc = reinterpret_cast<PIMAGE_IMPORT_DESCRIPTOR>(base + importDir.VirtualAddress);
+		importDesc->Name != 0; ++importDesc)
+	{
+		const auto libraryName = reinterpret_cast<LPCSTR>(base + importDesc->Name);
+		if(stricmp(libraryName, "user32.dll") != 0) { continue; }
+		for(auto originalFirstThunk = reinterpret_cast<PIMAGE_THUNK_DATA>(base + importDesc->OriginalFirstThunk),
+				firstThunk = reinterpret_cast<PIMAGE_THUNK_DATA>(base + importDesc->FirstThunk);
+			originalFirstThunk->u1.AddressOfData != 0; ++originalFirstThunk, ++firstThunk)
+		{
+			const auto functionName = reinterpret_cast<PIMAGE_IMPORT_BY_NAME>(
+				base + originalFirstThunk->u1.AddressOfData);
+			if(strcmp(functionName->Name, "GetWindowThreadProcessId") != 0) { continue; }
+			DWORD oldProtect = 0;
+			using FuncType = decltype(firstThunk->u1.Function);
+			VirtualProtect(&firstThunk->u1.Function, sizeof(FuncType), PAGE_READWRITE, &oldProtect);
+			firstThunk->u1.Function = reinterpret_cast<FuncType>(&GetWindowThreadProcessIdPatch);
+			VirtualProtect(&firstThunk->u1.Function, sizeof(FuncType), oldProtect, &oldProtect);
+			return;
+		}
+	}
+}
 
 
 #ifdef SYNC_WITH_SHM_FIFO
@@ -882,6 +933,8 @@ bool RemoteVstPlugin::load( const std::string & _plugin_file )
 		return false;
 	}
 
+	injectJuceMenuWorkaround(m_libInst);
+
 	m_plugin = mainEntry( hostCallback );
 	if( m_plugin == NULL )
 	{

[DomClark]

I tested Sylenth1 some more. I was able to determine that it uses JUCE, and that its menu bug is caused by the same buggy JUCE code. However, this new patch does not work with it due to the obfuscation used, which makes it impossible to overwrite its import address table (when unpacked in memory, I'm not sure it even meaningfully has one). There are other approaches, such as overwriting the code of GetWindowThreadProcessId, or patching the export table of user32.dll, but these are more invasive and more complex to implement.

[DomClark]

JUCE have fixed the bug on their end: juce-framework/JUCE#401 (comment). Do we still want to try and work around this, or should we assume most plugins will update and embedding can be turned off as a workaround for the few that are still broken?

[zonkmachine]

should we assume most plugins will update and embedding can be turned off as a workaround for the few that are still broken?

No. Helm will probably not update because it has been left by the developer for other projects. I don't know about other projects but if there is a possibility to have a workaround for lmms-1.3 I think it could still be worth it if it's not too much work involved. Even a simple fix that would not work with some plugins as the case with Sylenth1 above. I have unfortunately not had time to test your patch.

[Yutoob12]

I've noticed that when trying to use Helm as a VST under Windows, running on LMMS v1.2.0rc4, attempting to open a menu in Helm does not work.

Expected Behavior: Clicking on Distortion menu in the Helm synth opens the Distortion menu. Right clicking opens the right click menu.

Actual behavior: These menus open, but then close immediately, too quickly to interact with anything in it. Very rarely, it will work, which I notice causes Helm's animations to stop.

I believe this is an issue with LMMS (specifically vestige?) as the standalone Helm application does not have this issue on Windows, nor does this issue occur in other DAWs.

Turn off embedding of VST's in the settings
[Edit] > [Settings] > [General Settings] > "Plugin Embedding" > "No Embedding"
This should fix the problem. Now VST's will be kept in a separate window.

[zonkmachine]

Seem to be a problem with Dexed as well (forum comment).

This should be fixed in Dexed which used juce 7.0.1 (juce 6.1.0 introduced the fixe).

[zonkmachine]

I'm leaning toward closing this as wontfix and upstream.

Last Helm release was seven years ago and it's maybe better that someone fork and update it instead of us providing workarounds. The other projects we know the JUCE version they build against seem to have updated if they're still active.

[DomClark]

Seems reasonable to me. I hope to do more work on embedding bugs in the future, and it's possible this may be fixed as a side effect, but it doesn't need to be a target.