Lmms: 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: https://github.com/WeAreROLI/JUCE/issues/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).