sfztools / sfizz

SFZ parser and synth c++ library, providing a JACK standalone client
https://sfz.tools/sfizz/
BSD 2-Clause "Simplified" License
405 stars 58 forks source link

Crash if a midi note is recieved while the SFZ file is reloaded #1203

Closed Sylv1o closed 8 months ago

Sylv1o commented 11 months ago

Hi, I use SFIZZ in Synthedit, so unsure it also happen in SFIZZ VST or Standalone.

Here is the message I posted in the Synthedit group :

I worked a lot on my percussion sampler, things are very promising but...now that my project has became bigger (especially the SFZ files), I noticed a very problematique bug :

If the SFZ player module recieve a midi note while it is reloading the SFZ file, SE crash, same in exported plugin.

Basically in my big project, I need to wait one or two seconds after a change before playing a new note, or SE crashes. If the SFZ file is very small, It's harder to make it crash but I've been able to do it by playing many notes while adjusting some parameters. So of course, the bigger the file, the longer the wait...

Sylv1o commented 10 months ago

It's now fixed in Synthedit. I was able to reproduce the bug in the SFIZZ VST3.

essej commented 10 months ago

Ummm, did they make a fix in the libsfizz code or in how they used it? Sounds like we need a PR from them...

redtide commented 10 months ago

heh, I was about to ask it: what do you mean, that it was a bug in SE or still a bug in sfizz but fixed only there?

Sylv1o commented 10 months ago

I asked Jeff (Synthedit creator) to send the fix here

Sylv1o commented 10 months ago

He has just filter incoming midi notes while files are reloading

Sylv1o commented 10 months ago

Here the link to the changes (line 166)

paulfd commented 9 months ago

I was able to reproduce the bug in the SFIZZ VST3.

Just playing midi notes data while reloading made the plugin crash? There should be a process lock so that'd be a bug.

I could see to make the doc clearer but:

    /**
     * @brief Send a note on event to the synth.
     ...
     * @par Thread-safety constraints
     * - @b RT: the function must be invoked from the Real-time thread
     */
    void noteOn(int delay, int noteNumber, int velocity) noexcept;

and

    /**
     * @brief Empties the current regions and load a new SFZ file into the synth.
     ...
     * @par Thread-safety constraints
     * - @b CT: the function must be invoked from the Control thread
     * - @b OFF: the function cannot be invoked while a thread is calling @b RT functions
     */
    bool loadSfzFile(const std::string& path);

I guess a different wording could be that no @b RT functions must be called while the loadSfzFile function has not returned.

We removed locking within the library on purpose because it's usually much cleaner to do it at the "plugin interface" level where you can possibly leverage abilities offered by hosts.

Sylv1o commented 8 months ago

The solution implemented by Jeff in Synthedit seems perfect, I can't see anything wrong with it. One just want the midi to be filtered while the instrument is reloading...

paulfd commented 8 months ago

The same mechanism is implemented in the VST plug-in, but you mentioned in a previous comment you managed to make it crash. I was curious to know more about the context in how you made it crash (I.e. which daw, sfz instrument, ...). If you remember :)

Sylv1o commented 8 months ago

It seems the mechanism you mention don't work. I've just tested again in Cubase 12 with SFIZZ vst3, I load an instrument, and play some midi notes while the instrument is loading : no problem. Then I open the SFZ file in a text editor, make a useless change (just add a line), then I save the file and immediatly play some midi notes : Cubase crash.

Sylv1o commented 8 months ago

I can make it carsh with any SFZ file, but if the file is very small, it take too little time to load to make Cubase crash. Try with any big SFZ file, it should crash easily.

essej commented 8 months ago

Double check the version of the sfizz plugin you have installed, this sounds like a bug that was fixed last year.

Sylv1o commented 8 months ago

I'll check but I use sfizz for less than a year...

Sylv1o commented 8 months ago

1.2.1

essej commented 8 months ago

You need 1.2.2! :)

Sylv1o commented 8 months ago

Indeed not the last one, I'll check with 1.2.2

Sylv1o commented 8 months ago

I'm so sorry, indeed bug is fixed in 1.2.2

I don't even know why I had this old version...

My mistake, the fact that this bug was still there in the Synthedit version had made me wrong...

Appologies again.

redtide commented 8 months ago

No worries :) So can close this?

Sylv1o commented 8 months ago

I think so yes