Fix VST effect bugs #4294

DomClark · 2018-04-14T00:33:01Z

Fixes #4110, and a couple of other VST effect bugs.

Summary of my changes, since I neglected to put any with my commits (Lukas-W's are with his commits):

Fix X11 embedding on Qt4: see Strange problems when loading a project containing VST effects. #4110 (comment).
Fix VST effect load crash on non-primary monitor: LMMS would crash when on a non-primary monitor if the first embedded VST opened was an effect. Upon embedding a VST, the chain of parent windows needs to be made native, and if the main window is not on the primary monitor, this triggers a repaint/relayout before the VstEffectControlDialog is added to the SubWindow. SubWindow then causes a segfault trying to access properties of its widget(), which is still null. I've just added null checks where appropriate.
Fix toggling UI for non-embedded VST effects: Fixes a bug from earlier on this branch, behaviour is the same as stable-1.2.

Fix RemoteVstPlugin not exiting when effect removed: this was a regression on Windows only, caused by a8311a7. On Windows, effect control dialogs are kept around even when the view that opened them is deleted:

lmms/src/gui/widgets/EffectView.cpp

Lines 162 to 176 in e554a4c

EffectView::~EffectView()

{

#ifdef LMMS_BUILD_LINUX

delete m_subWindow;

#else

if( m_subWindow )

{

// otherwise on win32 build VST GUI can get lost

m_subWindow->hide();

}

#endif

}

As a result, the QSharedPointer in VstEffectControlDialog keeps the VstPlugin instance alive even when the effect has been removed. I've changed this to a plain QPointer, so it maintains the nullify-on-delete behaviour but lets the plugin exit properly. Now cbf8cbd replaces it.

Edited by @PhysSong: Original fix eb560a7 have been dropped and cbf8cbd have been added instead. Another new commit d6ae46e fixes #1727.

zonkmachine · 2018-04-15T13:56:17Z

It tests fine like before but I'm not heavily into VSTs and wouldn't know what to look for.

lukas-w · 2018-04-15T14:35:32Z

plugins/VstEffect/VstEffectControlDialog.cpp

@@ -47,7 +47,8 @@ VstEffectControlDialog::VstEffectControlDialog( VstEffectControls * _ctl ) :
 	m_pluginWidget( NULL ),

 	m_plugin( NULL ),
-	tbLabel( NULL )
+	tbLabel( NULL ),
+	m_needsEmbed( false )


This appears to be equivalent to m_plugin && m_plugin->embetMethod() != "none". If so, can we remove the variable and make it a method instead in order to reduce redundancy?

We can't, because VstEffectControlDialog::showEvent may modify the value? I guess it would be safe to replace embed_vst with this variable if showEvent is never called while construction.

m_needsEmbed is a flag to indicate that the VST GUI needs to be embedded but hasn't been yet. The VST GUI seems to be deleted on Qt4 with X11 embedding if the first thing it's added into isn't currently part of the main window hierarchy, so I deferred the embedding to the first time showEvent is called, when the VstEffectControlDialog will be safely inside its subwindow. This could be replaced by m_plugin && m_plugin->embetMethod() != "none", but the GUI would be re-added to the dialog every time showEvent is called.

lukas-w · 2018-04-15T14:42:07Z

plugins/VstEffect/VstEffectControlDialog.cpp

 					SLOT( togglePluginUI( bool ) ) );
+		} else {
+			connect( btn, SIGNAL( clicked() ),
+					m_plugin.data(), SLOT( toggleUI() ) );


Why was this change needed? Can we fix it within togglePluginUI or toggleUI instead?

The toggled(bool) signal isn't emitted if the button isn't checkable, so when the plugin isn't embedded we have to connect to clicked instead. The trouble with connecting clicked(bool) to togglePluginUI(bool) is that when the button isn't checkable, the parameter is always false. This could be worked around by doing something like

if( m_plugin->embedMethod() == "none" ) { m_plugin->toggleUI(); return; }

inside togglePluginUI, but togglePluginUI is called from VstEffectControls::createView to set the visibility of the UI to what's in the project file, which this would break. Thus we can't sensibly use togglePluginUI, so I connected straight to VstPlugin::toggleUI instead.

The toggled(bool) signal isn't emitted if the button isn't checkable

If we can uncheck the button when VST window is closed, we can always make the button checkable. AFAIK we do something similar for ZynAddSubFX.

lukas-w · 2018-04-15T14:47:08Z

plugins/VstEffect/VstEffectControlDialog.h

@@ -67,7 +67,7 @@ class VstEffectControlDialog : public EffectControlDialog
 	PixmapButton * m_managePluginButton;
 	PixmapButton * m_savePresetButton;

-	QSharedPointer<VstPlugin> m_plugin;
+	QPointer<VstPlugin> m_plugin;


Can we get rid of the Windows-specific behaviour instead? Maybe the bug this workaround was made for has been fixed.

The workaround is very old(more than 5 years ago): 1c9c76f#diff-e3cda83bb57677b7ecd60d7e5e1fe92cR156
There have been so many changes after that(Qt version change, VST bug fixes, etc.) I think we should test whether the bug still exists.

Removing the workaround, I can still reproduce the issue:

Enable "One instrument track window mode"

Create a project with at least two instruments

Add a VST effect to one of them

Open the other instrument's window

Open the first instrument's window again and show the VST effect dialog

The VST GUI will have disappeared.

I'm sure there's a better way to work around this, but the X11 embedding on Qt4 seems to be especially fragile and I'm not going to have access to a Linux machine again for a couple of months, so I didn't want to touch this any more. If you can produce something better, it would be very welcome.

Removing the workaround, I can still reproduce the issue:

Could you tell me ~~your OS and Qt version~~ the embedding method you used? If I can reproduce that, I'll try to figure out what's going on.

Windows 10 64-bit, Qt 5.4.2.

I can reproduce the bug on GNU/Linux, too. (Edit: Qt5 only)
I found that the plugin widget is destroyed when VstEffectControlDialog is destroyed because of parenting. Simple unparenting seems to fix this.

@DomClark Is it fine to remove eb560a7 from this PR and add new fix which uses unparenting?

Edit: I'll remove Windows-specific behavior first and then fix the original bug. This procedure will remove the problematic behavior without re-introducing the GUI bug.

Sure, go ahead 😃

Unparenting approach dosen't work well for XEmbed + Qt4. So I think I need to wrap that with QT version check macro if I do that.

It seems like destroying parent window also destroys child windows on Windows. I guess it was the reason for the workaround.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms632682(v=vs.85).aspx

Might it be better to use a separate helper class to handle win32 embedding?

PhysSong · 2018-04-16T07:25:53Z

Maybe out of scope... I have a fix for #1727. Is it fine to include the fix here?

Umcaruje · 2018-04-16T12:31:11Z

I have a fix for #1727. Is it fine to include the fix here?

Depends on the size of the patch, though this PR already fixes a lot of things, your fix probably could be more suited as a separate PR, unless the fix is dependent on the changes made in this PR.

PhysSong · 2018-04-16T12:56:50Z

The fix changes a few lines and not directly related to this PR.
FYI, there will be minor merge conflicts when I open a separate pull request.

Umcaruje · 2018-04-16T14:54:49Z

The fix changes a few lines and not directly related to this PR.
FYI, there will be minor merge conflicts when I open a separate pull request.

Then I guess you could push here so we avoid the conflicts

PhysSong · 2018-04-17T14:51:48Z

src/gui/widgets/EffectView.cpp

-			m_subWindow->setFixedSize( m_subWindow->size() );
+			if (m_subWindow->layout()) {
+				m_subWindow->layout()->setSizeConstraint(QLayout::SetFixedSize);
+			}


@lukas-w I don't know if it's needed, but isn't it safe to keep previous behavior(probably with else)?

@PhysSong I don't think it's needed, setSizePolicy( QSizePolicy::Fixed, QSizePolicy::Fixed ) already does that in the else case.

DomClark · 2018-04-20T23:51:23Z

I've ~~mostly~~ implemented Umcaruje's suggestions from #4110 (comment), apart from VST windows being always-on-top on Linux. Can anyone help with this? On Windows, if a window is given an owner it will always be displayed on top of it. However, this requires the HWND of the parent, so in order to do this using Win32 code in RemoteVstPlugin, one would have to convert LMMS's native window ID into an HWND for Wine, which I don't know how to do (or if it's possible). Alternatively, this could be done natively, but again I have no idea how.
Update: all implemented via c4d610e and 4d27863.

zonkmachine · 2018-05-15T16:04:28Z

@DomClark Is there anything left to do here? I'm testing this and the few vst effects I have open up fine and gui show/hide is remembered.

lukas-w · 2018-05-15T16:28:33Z

@zonkmachine: As far as I know, @PhysSong still wanted to make some adjustments.

PhysSong · 2018-05-16T22:33:35Z

I want to make two changes(one is fixing RemoteVstPlugin exiting issue in new way, the other is fixing #1727), but they're optional.

qnebra · 2018-05-17T10:20:16Z

I got small issue, minor one with this PR.

Ubuntu 18.04, wine 3.0, latest master with only this PR merged.
Add new VST instrument track to project.
Add plugin, anything but instrument. In my case sforzando and arminator.
Work with them.
"Clone this track"

Expected behaviour:
Vestige track is cloned and vst plugin window shows.

Now:
Vestige track is cloned, but vst window is missing. Blank Vestige window.

PhysSong · 2018-05-18T07:19:33Z

plugins/vestige/vestige.cpp

@@ -127,6 +177,13 @@ void vestigeInstrument::loadSettings( const QDomElement & _this )
 	{
 		m_plugin->loadSettings( _this );

+		m_plugin->createUI( nullptr );


~~Why~~ Do we need this call? At this moment loadFile already have created UI, so this call will cause the bug reported by @qnebra.

Hmmm, interesing. Also I will check, if I can of course with my noob skills.

@PhysSong this call make a issue. I commented this out, compile lmms and check. Cloning works correctly.

PhysSong · 2018-05-19T08:18:27Z

Should we do a rebase and squash fixup commits? Squashing everything doesn't look good, but ordinary merge might mess up commit history.

zonkmachine · 2018-05-19T09:37:55Z

There is also the 'Rebase and merge' option.
Edit: In reference to keeping the commit history clean

PhysSong · 2018-05-20T06:55:57Z

I've done #4294 (comment) on top of rebased branch. Would it be fine to (force-)push that?

zonkmachine · 2018-05-20T11:01:24Z

Yes. I think so.

Fixes bugs where during project loading (observed with VST effects), empty widgets and sub-windows would be left floating around. These were caused by inconsistencies between the way VST UIs were created when loading a project and when adding an effect in an existing project. In some situations, this caused createUI to be called twice, leaving over multiple empty widgets. This commit refactors some code in order to avoid creating unnecessary sub- windows, which aren't needed with VST effects, but were still created, usually being invisible. All sub-window related code was moved out of VstPlugin into vestige.cpp, which is the only place where sub-window VSTs are actually used. A new sub-class of VstPlugin, VstInstrumentPlugin, now handles VST sub-windows and is used by vestigeInstrument. "guivisible" attribute loading was moved out of VstPlugin as well and is now done in VstEffectControls' and vestigeInstrument's loadSettings method respectively. This causes some minor code duplication unfortunately. Closes #4110

QMdiSubWindow::setSizePolicy doesn't have any effect because QMdiSubWindow uses a layout. This patch uses QMdiSubWindow::layout()->setSizeConstraint instead. This may cause effects that don't have a layout and don't implement sizeHint() to now be resizable. For effects that do though, it fixes the size constraint.

From MSDN: "In WM_SYSCOMMAND messages, the four low-order bits of the wParam parameter are used internally by the system. To obtain the correct result when testing the value of wParam, an application must combine the value 0xFFF0 with the wParam value by using the bitwise AND operator." Also calculate the required window size using AdjustWindowRect, rather than hard-coding some constants.

PhysSong · 2018-05-21T07:36:07Z

I've done a interactive rebase and cleaned up the commit history. A fix for #4294 (comment) is included in 9449c07(the original commit 102345b was the cause of this bug).
cbf8cbd now replaces eb560a7. Since the fix is not needed for Qt4 + XEmbed(it even breaks embedding for situation), I've wrapped it with a pair of macro.
d6ae46e is a simple fix for #1727. It was added here per #4294 (comment). As of #3786, it's possible to use VST effect dialogs without VST's GUI. So fixing #1727 doesn't require many changes. Also, it will be easy to revert this if it causes any issues.

zonkmachine · 2018-05-21T20:08:49Z

d6ae46e is a simple fix for #1727

I get a crash when testing this with mda Detune (as used in #1727). Crash occurs in the controls window when opening the control button (control VST-plugin from LMMS host) after pressing the Show/Hide button a bunch of times.

VstPlugin::showUI called before VstPlugin::createUI 
VstPlugin::showUI called before VstPlugin::createUI 
VstPlugin::showUI called before VstPlugin::createUI 
VstPlugin::showUI called before VstPlugin::createUI 
Segmentation fault (core dumped)

...

Core was generated by `./lmms'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  QMdiSubWindow::widget (this=0x34ec450) at widgets/qmdisubwindow.cpp:2361
2361	widgets/qmdisubwindow.cpp: No such file or directory.
(gdb) bt
#0  QMdiSubWindow::widget (this=0x34ec450) at widgets/qmdisubwindow.cpp:2361
#1  0x00007f56f4529bc0 in VstEffectControls::managePlugin (this=0x7f57157b0608)
    at /home/zonkmachine/builds/lmms/lmms/plugins/VstEffect/VstEffectControls.cpp:165
#2  0x00007f56f4530282 in VstEffectControls::qt_static_metacall (_o=0x7f57157b0628, _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x7ffe50d73980)
    at /home/zonkmachine/builds/lmms/lmms/build/plugins/VstEffect/moc_VstEffectControls.cxx:59
#3  0x00007f57141bb87a in QMetaObject::activate (sender=0x33a0a90, m=m@entry=0x7f57153f72a0 <QAbstractButton::staticMetaObject>, 
    local_signal_index=local_signal_index@entry=2, argv=argv@entry=0x7ffe50d73980) at kernel/qobject.cpp:3539
#4  0x00007f5714f72172 in QAbstractButton::clicked (this=<optimized out>, _t1=false) at .moc/release-shared/moc_qabstractbutton.cpp:219
#5  0x00000000005f85fb in AutomatableButton::mouseReleaseEvent (this=0x33a0a90, _me=0x7ffe50d73df0)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/AutomatableButton.cpp:151
#6  0x0000000000619c03 in PixmapButton::mouseReleaseEvent (this=0x33a0a90, _me=0x7ffe50d73df0)
    at /home/zonkmachine/builds/lmms/lmms/src/gui/widgets/PixmapButton.cpp:94

PhysSong · 2018-05-22T03:37:29Z

I get a crash when testing this with mda Detune

It looks like an old bug. To reproduce:

Load any VST effects(an effect dialog will be show up)
Click control button in the effect dialog
Close the the control dialog With 'X' button
Click the button in step 2, LMMS will crash

I'll look into this bug.

PhysSong · 2018-05-22T03:43:26Z

pressing the Show/Hide button

I think we should disable this button when the effect has no GUI. Is it worth to disable it for VSTi too?

PhysSong · 2018-05-22T05:42:06Z

#4294 (comment) seems to be introduced in 908591b. The problem is Qt::WA_DeleteOnClose attribute set by MainWindow::addWindowedWidget, so removing that will be sufficient.
I also found that manageVestigeInstrumentView and manageVSTEffectView are doing almost identical things. I'd like to merge them into one class(something like VSTPluginManageView), but it looks like out of scope of this pull request.

zonkmachine · 2018-05-22T16:53:32Z

I think we should disable this button when the effect has no GUI.

Yes.

Is it worth to disable it for VSTi too?

For VSTi's without a gui? Yes to this also.

Unset Qt::WA_DeleteOnClose for the dialog to avoid deletion when closed

PhysSong · 2018-05-23T11:30:12Z

Fixed a crash reported by @zonkmachine. I think Show/Hide button should be disabled for VSTs without GUI, but I think that's an UI/UX enhancement rather than a bug fix. So I think this one could be merged after enough reviews/tests.

zonkmachine · 2018-05-23T15:39:05Z

Fixed a crash reported by @zonkmachine

Fix confirmed.

I think this one could be merged after enough reviews/tests.

👍

zonkmachine · 2018-05-24T16:02:13Z

Testing with mda Detune (with setting 'Sync VST plugins to host playback' ticked). I can adjust the settings of it but I need to save/reload the project in order for the changes to take effect. Changes done via automation will not take effect. This works fine for Nyquist EQ so it may be an issue with mda Detune.

Edit: mda Degrade simlilarly don't have a gui and works fine.

zonkmachine · 2018-05-26T10:58:32Z

I've tested this some more and find nothing else to comment on. The mda Detune issue above I ascribe to mda Detune.

PhysSong · 2018-05-29T08:54:09Z

Should we wait for more testings, or merge this soon?

zonkmachine · 2018-05-29T16:57:22Z

Merge. It's gone through plenty of testing. 🚜

PhysSong requested a review from lukas-w April 15, 2018 03:17

lukas-w reviewed Apr 15, 2018

View reviewed changes

Umcaruje added this to the 1.2.0 milestone Apr 16, 2018

PhysSong reviewed Apr 17, 2018

View reviewed changes

PhysSong reviewed May 18, 2018

View reviewed changes

lukas-w and others added 10 commits May 21, 2018 16:09

Fix X11 embedding on Qt4

3eb5815

Fix VST effect load crash on non-primary monitor

c5f80d4

Fix toggling UI for non-embedded VST effects

2a35d5e

Preserve VST GUI positions and keep them on top

61ac18a

VST: Add workaround for small effect window on project load

cee327e

Fix RemoteVstPlugin not exiting when effect removed

cbf8cbd

Allow controlling VST effects without own GUI

d6ae46e

PhysSong force-pushed the fix/vst-effect-load branch 2 times, most recently from 4f526b1 to d6ae46e Compare May 21, 2018 07:19

Fix crash when re-opening VST effect manage dialog

211241f

Unset Qt::WA_DeleteOnClose for the dialog to avoid deletion when closed

PhysSong merged commit 24ae559 into stable-1.2 May 30, 2018

PhysSong deleted the fix/vst-effect-load branch May 30, 2018 00:02

	EffectView::~EffectView()
	{

	#ifdef LMMS_BUILD_LINUX

	delete m_subWindow;
	#else
	if( m_subWindow )
	{
	// otherwise on win32 build VST GUI can get lost
	m_subWindow->hide();
	}
	#endif

	}

Fix VST effect bugs #4294

Fix VST effect bugs #4294

Conversation

DomClark commented Apr 14, 2018 • edited by PhysSong Loading

zonkmachine commented Apr 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

PhysSong Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong commented Apr 16, 2018

Umcaruje commented Apr 16, 2018

PhysSong commented Apr 16, 2018

Umcaruje commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DomClark commented Apr 20, 2018 • edited Loading

zonkmachine commented May 15, 2018

lukas-w commented May 15, 2018

PhysSong commented May 16, 2018

qnebra commented May 17, 2018

PhysSong May 18, 2018 • edited Loading

Choose a reason for hiding this comment

qnebra May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhysSong commented May 19, 2018

zonkmachine commented May 19, 2018 • edited Loading

PhysSong commented May 20, 2018

zonkmachine commented May 20, 2018

PhysSong commented May 21, 2018

zonkmachine commented May 21, 2018

PhysSong commented May 22, 2018 • edited Loading

PhysSong commented May 22, 2018

PhysSong commented May 22, 2018

zonkmachine commented May 22, 2018 • edited Loading

PhysSong commented May 23, 2018

zonkmachine commented May 23, 2018

zonkmachine commented May 24, 2018 • edited Loading

zonkmachine commented May 26, 2018

PhysSong commented May 29, 2018

zonkmachine commented May 29, 2018

DomClark commented Apr 14, 2018 •

edited by PhysSong

Loading

PhysSong Apr 17, 2018 •

edited

Loading

PhysSong Apr 17, 2018 •

edited

Loading

PhysSong Apr 19, 2018 •

edited

Loading

DomClark commented Apr 20, 2018 •

edited

Loading

PhysSong May 18, 2018 •

edited

Loading

qnebra May 18, 2018 •

edited

Loading

zonkmachine commented May 19, 2018 •

edited

Loading

PhysSong commented May 22, 2018 •

edited

Loading

zonkmachine commented May 22, 2018 •

edited

Loading

zonkmachine commented May 24, 2018 •

edited

Loading