Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some more VST bugs #4061

Merged
merged 4 commits into from
Jan 17, 2018
Merged

Fix some more VST bugs #4061

merged 4 commits into from
Jan 17, 2018

Conversation

DomClark
Copy link
Member

@DomClark DomClark commented Dec 21, 2017

Remove the lock synchronising access to the VST plugin's methods. The only methods which are called outside of the UI thread are process, processReplacing and dispatcher(effProcessEvents, ...), all of which are intended to be called concurrently with other methods in order to facilitate real-time audio processing. This fixes a deadlock which occurs when a plugin waits on window messages sent to LMMS during an idle call.

I've also taken the opportunity to replace lockShm() with tryLockShm() in RemoteVstPlugin::process so as to remove all potentially blocking calls from that method.

The message sampleRateInformation now waits for a reply since, now we don't have a lock, audio processing shouldn't start before it's complete.

We now always tell the plugin the sample rate and buffer size on load as some plugins (e.g. Amplitube) crash if not given this information.

Fixes #4052.

The audioMasterPinConnected host callback returns 0 now to indicate that all pins are connected. This message is deprecated but some plugins (e.g. iZotope effects) still rely on it and won't produce sound unless told that they are connected.

Fixes #4095.

@tresf
Copy link
Member

tresf commented Dec 22, 2017

@DomClark is this ready for merge? I know you had a question on #devtalk about whether or not to combine a second fix.

Fixes AmpliTube crash
@DomClark
Copy link
Member Author

DomClark commented Dec 23, 2017

Added the second fix now, turned out to be easier than I expected.

In terms of being ready to merge, there is one potential problem I've spotted; it seems unlikely to occur but I'd like to make sure it won't. Most VST calls can be made concurrently with the audio processing calls, but updating sample rate and buffer size can't. Buffer size is set only upon init, but the sample rate is changed when exporting at a non-default sample rate, so if anyone's particularly unlucky with thread scheduling, the sample rate update message won't make it to the VST before processing begins. The result depends on the plugin; best case scenario nothing happens, but worse case scenario the plugin crashes and LMMS hangs. I'll probably address this by making LMMS wait for a reply to IdSampleRateInformation so processing won't begin until everything's ready.

@tresf
Copy link
Member

tresf commented Dec 27, 2017

@DomClark per request on Discord, here's a build that contains this fix (please pardon the versioning, it's based off of RC5, not RC4)

https://github.com/tresf/lmms/releases/download/v1.2.0-rc4/lmms-1.2.0-rc4.130-win64.exe

@DomClark DomClark changed the title Remove plugin lock from RemoteVstPlugin Fix some more VST bugs Jan 7, 2018
@PhysSong
Copy link
Member

@DomClark Are there anything left before merging this?

@DomClark
Copy link
Member Author

I'm not aware of anything else for now, so I'm happy for this to be merged. I can always open another PR if anything new appears.

@PhysSong PhysSong merged commit e2c71e3 into LMMS:stable-1.2 Jan 17, 2018
@DomClark DomClark deleted the nopluginlock branch March 12, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iZotope plugins not working in LMMS Vestige freezes and crash LMMS with plugins that need verification
3 participants