FST (VST 2.4) plugin: program changes and state (bank) loading sometimes switches to the blank patch

[attilammagyar]

There's a race between FstPlugin::handle_program_change() (audio thread) and FstPlugin::set_chunk() (GUI thread), since both can be writing the Bank object at the same time.

(Related: #11 and #21.)

[ZonderP]

Would 'FstPlugin::handle_program_change()' actually write the bank object, if there was no preceding call to 'set_program' with a different index? I think 'set_pogram' shouldn't be called at all in this scenario?

[attilammagyar]

SAVIHost calls set_program() right after set_chunk() from the GUI thread, without suspending the audio thread. While this is happening, the audio thread may render any number of blocks (including zero).

One scenario where this can cause a problem (assuming that the optimizer doesn't reorder save_current_patch_before_changing_program = false; - if it gets reordered, then the race gets much worse):

1 .set_chunk() sets save_current_patch_before_changing_program to false, then reaches the bank.import(buffer); line

2. bank.import() sets bank.current_program_index to 0, then proceeds to overwrite each program.
3. handle_program_change() in the audio thread sees that bank.get_current_program_index() returned 0 which is different from this->next_program, so it proceeds to the condition in line 559; since save_current_patch_before_changing_program is false, it gets set to true.
4. Meanwhile, the GUI thread has finished overwriting the bank, and proceeds to query the current program from it, which is 0.
5. Then the audio thread calls bank.set_current_program_index(next_program);, and finally, imports this program from the bank.
6. Then the GUI thread proceeds to import program 0 (which is the blank, initial preset), and then set the program-change parameter's value to 0.
7. When handle_program_change() gets called for the next block, it will mistakenly think that the program has changed again, and it will proceed to overwrite the current program in the bank with the synth's current state - which is the empty preset.

Long story short, the current implementation fails to satisfy multiple assumptions about thread safety, e.g. the synth's queue is supposed to be filled by at most one thread, and it's supposed to be consumed by at most one thread; it's a problem if multiple threads can call process_messages() parallelly.

The current implementation relies on the assumption that the host will suspend the plugin while set_chunk() is working, but apparently, not all hosts do that.

So instead, I'm planning to make sure that the bank is only ever written by the GUI thread, and the synth's message queue is only ever consumed by the audio thread.

[ZonderP]

Thanks Attila for the detailed explanation. Does more or less the same apply, if set_chunk is only called for a single preset? I've the feeling that your description is more related to bank loading (as also the issue title suggests). But most probably also the issue I described (loading of a single preset) is caused at least by something similar to this. One more question: Is there a specific reason, why you set the current bank index to 0 if a bank gets loaded? Shouldn't it just stay the same? I'm not at my home computer right now, so cannot check how other VSTs behave, but I would have thought that loading of a bank shouldn't (necessarily at least) reset/change the current bank index.

Looking forward to the new version!

[attilammagyar]

Does more or less the same apply, if set_chunk is only called for a single preset?

I'm not sure about the exact sequence of events that can trigger the bug with single preset set_chunk() calls, but invoking non-thread-safe code from multiple threads certainly doesn't help.

Is there a specific reason, why you set the current bank index to 0 if a bank gets loaded?

No, there's not. I just thought it would be nice to go with a clean state after a bank load, but recently I changed my mind about it. It's already on the dev branch: b6e92f738f0b3505c2816dee1b425271393bddcb (Note that I occasionally might force-push this branch.)

[attilammagyar]

FYI: I think the dev branch is now clean of thread crossings and program change races. Since almost all methods in FstPlugin have been rewritten or changed significantly, I want to do a lot of testing before releasing a new version though. (And there's one more thing that I want to do before the next version.)

I hope I can get to it by the end of the weekend.

[attilammagyar]

It should be fixed by v2.0.1.

[ZonderP]

Thanks Attila, that‘s great! Will only be able to test after my vacations in ~ 10 days.

Am 10.09.2023 um 18:59 schrieb Attila Magyar @.***>:



It should be fixed by v2.0.1https://github.com/attilammagyar/js80p/releases/tag/v2.0.1.

— Reply to this email directly, view it on GitHubhttps://github.com/attilammagyar/js80p/issues/24#issuecomment-1712875469, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABW37MZJJ67C6I5XXKPBJFTXZXWX7ANCNFSM6AAAAAA32PI77A. You are receiving this because you commented.Message ID: @.***>

[attilammagyar]

Thanks, and have fun! 🌴

[ZonderP]

Hi Attila!

With some rather quick testing, I have the feeling, that the 'switching to the blank patch' seems to be solved. But for me a major issue is, that the handling of the preset names doesn't work.

E.g.: I start with the default internal bank which has 'Blank' at pos 0, 'Bright Organ' at pos 1 and so on. I then go to pos 0 and load the sound 'tremolo_lead.js80p' from the js80p GUI to this position. Neither in Savihost nor in EXT64 that new name gets displayed, but instead still the old name 'Blank'. Then I switch to pos 1 and load the sound 'tech_noir_lead_3.js80p' from the js80p GUI to this position. The same happens - the name of the preset on pos 1 doesn't change to 'Tech Noir Lead 3' (from 'Bright Organ'). And so on... When I then save that changed bank as fxb bank file, then the names are not the ones they should be.

When I first save all the internal presets as fxp preset files and trying to build up a new bank by loading those fxp preset files to the positions, then it also doesn't work when I do that with Savihost, but it works when I do so with my VeeSTeeEx wrapper.

I didn't look at the code yet, because I got quite sick immediately after I returned from my vacations and have not recovered yet to be able to concentrate enough. Maybe 'effGetProgramName' (or what it is called) doesn't work properly, after loading the internal preset format?

[attilammagyar]

I'm not sure I understand the problem: JS80P's own GUI never handled the concept of banks and program names, and those files in the preset folder don't even contain the names, just the settings. Bank management and program naming is delegated entirely to the host (and the plugin interface).

However, when handling a single program, then indeed FstPlugin::get_chunk() and FstPlugin::set_chunk() forget to export and import the current program's name properly. I'm going to fix them soon, thanks for letting me know.

I hope you get well soon.

[ZonderP]

Already feeling much better, thanks!

Since js80P is offered as VST plugin, it should support the VST specifications and report back a preset name when 'effGetProgramName' is called (resp. also support 'effGetProgramNameIndexed'). Even if it would not support banks, but only a single program slot, it should be able to report back a name (the correct one) for that preset at that program slot. And somehow js80p is already able to do so - initially. When I start up js80p via Savihost, then it reports back its internal preset names at the according program slots. I think you defer their names from the file names in the presets folder (or you have them already in memory). I might be totally wrong, but I would assume it shouldn't be a big problem that when the loading of a patch via the load button on the js80p GUI occurs to the current program slot, then for that program slot also that name (derived from the file name) could be used to be reported back to a call of 'effGetProgramName' - instead the name which was initially set on that program position on startup.

Hope that makes sense.

P.S.: All halfway properly implemented/serious VST's (I'm aware of) which also have their internal patch handling, support this somehow. For the (mostly older) ones, which do not at all have their own preset handling, this special case it not so much a scenario, although even they need to ensure that - e.g. after a 'effSetChunk' for banks - they report back the proper names for the programs which they got from reading in the chunk.

[ZonderP]

I just looked at your code in commit a36da9fd72c1eea4799f6573fe3cfd634f4b857c: I wouldn't think you need the new additional code in 'get_chunk' for the preset?

[attilammagyar]

Other than the sloppy single-patch name handling that I mentioned above (already fixed on dev), I think the plugin handles eff[GS]etProgramName and eff[GS]etChunk correctly. (At least, as far as I can know without being able to legally access the official SDK.)

But when you use JS80P's own GUI for importing or exporting, then that has nothing to do with those opcodes, since the GUI doesn't talk to the synth via the VST interface. The .js80p files are only concerned with the synth's state, but not the plugin's state.

The built-in presets do come from the presets folder, but that happens in compile-time, and the presets are bound to the plugin interface implementation (not the synth or the GUI), because VST 2 and 3 do preset and bank management completely differently. And I don't plan to implement a custom preset management GUI (separate one for each plugin type), because the host should already have one. The import / export icons are really only intended to load and save the current state of each knob, nothing more.

I wouldn't think you need the new additional code in 'get_chunk' for the preset?

It is needed, because when current_patch was last written by the synth (unaware of any program names and banks and whatnot, only caring about the parameters), then it won't contain a program name.

Edit: in other words, the GUI is concerned with the synth's state, not the plugin's state. The latter is left for the host.

[ZonderP]

Strange... I just tried with your current dev version: Now when I load fxp preset file (e.g. 'Derezzed.fxp' - with the preset name 'Derezzed' correctly in its fxp header) to any current program position, then the name now changes (good), but not to what it should be, but instead to 'Prog[xxx]' where 'xxx' is the current position (bad)....

[ZonderP]

But when you use JS80P's own GUI for importing or exporting, then that has nothing to do with those opcodes, since the GUI doesn't talk to the synth via the VST interface. The .js80p files are only concerned with the synth's state, but not the plugin's state.

The built-in presets do come from the presets folder, but that happens in compile-time, and the presets are bound to the plugin interface implementation (not the synth or the GUI), because VST 2 and 3 do preset and bank management completely differently. And I don't plan to implement a custom preset management GUI (separate one for each plugin type), because the host should already have one. The import / export icons are really only intended to load and save the current state of each knob, nothing more.

Yes, I think that I somehow understand - it's all nicely decoupled. But in my opinion, there should be a way to let the respective implementation (VST, VST3, AU or whatever) somehow know, that the preset name at the current position changed. Without having a deeper look at the code, I think if a user selects a preset via the js80p GUI, then the GUI somehow let's the synth know, that it's state changed (all knob's states - more or less everything except the name). Probably it does so by messaging. Couldn't it also by a similar mechanism let the plugin (VST, VST3, etc.) inform, that the name changed?

It's a bit confusing, that apparently with its initial state (when loading the plugin) the names are correct, but not so after someone loads an internal preset.

Couldn't there be something like: to_plugin_string_messages.push( Message(MessageType::RENAME_PROGRAM, 0, file_name_of_just_loaded_preset) ); which - when consumed - would execute program_names[current_program_index].set_name(file_name_of_just_loaded_preset);

[attilammagyar]

Now when I load fxp preset file (e.g. 'Derezzed.fxp' - with the preset name 'Derezzed' correctly in its fxp header) to any current program position, then the name now changes (good), but not to what it should be, but instead to 'Prog[xxx]' where 'xxx' is the current position (bad)....

Did you export the preset file after a36da9fd72c1eea4799f6573fe3cfd634f4b857c? If the preset file was created before that commit, then the name of the program might not have been included in the serialized data, and so it won't be loaded either.

Couldn't there be something like: to_plugin_string_messages.push( Message(MessageType::RENAME_PROGRAM, 0, file_name_of_just_loaded_preset) ); which - when consumed - would execute program_names[current_program_index].set_name(file_name_of_just_loaded_preset);

The GUI pushes messages into the synth's message queue, not the plugin's message queues. The synth doesn't care about banks and programs, but the plugins (VST3 and FST) do.

I could of course:

1. sanitize the user's arbitrary file names to fit into kVstMaxProgNameLen bytes (and deal with locales and encodings, potentially variable length encodings like UTF-8 in Linux, in order to avoid cutting multibyte characters in half),
2. add a hook to the GUI which would notify the plugin about the program name change,
3. have the hook's implementation in the plugin send a message to the audio thread in order to get the new name merged into the bank,
4. have the audio thread signal back that it is done, and now audioMasterUpdateDisplay needs to be called in order to let the host know that we have a new program name,
5. and finally, call audioMasterUpdateDisplay in the GUI thread.

But IMHO the value added by this is not on par with the complexity that it brings (especially the first step).

(Besides, even big name plugins like Surge XT and Analog Lab V don't seem to bother with notifying the host about program name changes that were triggered on their UIs.)

[ZonderP]

Did you export the preset file after a36da9fd72c1eea4799f6573fe3cfd634f4b857c? If the preset file was created before that commit, then the name of the program might not have been included in the serialized data, and so it won't be loaded either.

No, and indeed this was the reason, it didn't work. I reexported with the dev version now and all is working fine.

I could of course: ...

Yes, that sounds like what I imagined and would make js80p a perfect VST2 compliant plugin. I admit, it's quite some steps to be programmed, but to be honest I wouldn't care too much about the first step. It's not like that exact names are that important, important is that a preset sound can be identified by a name. And VST users usually don't name presets with e.g. Chinese characters (because VST spec never defined the character set to be used - but initially it was assumed it should be plain ASCII) because they know that this wouldn't really work in hosts. And therefore they also won't name the files in such a way as soon as they find out that this will not work in FXP files or in host displays. I've a similar problem in VeeSTeeEx, since it deals with all kind of VSTs with thousands and thousands of presets and their names and usually only a few of them contain special characters like é, Ő etc. and usually it doesn't matter much, if instead of those some symbol like '[]' gets displayed. Even hosts have that problem, because also they cannot know what the plugin gives them, if it is UTF8, just plain ASCII or some special locale. From what I have experienced, I think most just try to interpret it as UTF8. If you don't have resp. don't want to use any UTF8 multi-platform string utility or so, it might be a bit of hassle, but I'd like to try to help in that case.

(Besides, even big name plugins like Surge XT and Analog Lab V don't seem to bother with notifying the host about program name changes that were triggered on their UIs.) Regarding Surge XT - See here: https://github.com/surge-synthesizer/surge/issues/5881 ;-)