swesterfeld / liquidsfz

SFZ Sampler
Mozilla Public License 2.0
79 stars 12 forks source link

Please document API thread restrictions #3

Closed falkTX closed 3 years ago

falkTX commented 4 years ago

I was looking into the documentation of liquidsfz, to see if it was possible to use in Carla (instead of sfzero). Seems like yes, the fact that you provide pkg-config files is perfect! Thanks a lot for that.

But, the use of different threads and restrictions on it is not specified in the documentation. Take a look at the LV2 docs for a good example, at http://lv2plug.in/ns/lv2core/lv2core.html We have "Discovery Class", "Instantiation Class" and "Audio Class", then written how we are supposed to use them. Would be great if liquidsfz Synth class had a similar text/documentation.

I can think of a few questions already, to give you a few points to write about:

  1. are there restrictions on which thread we need to call set_sample_rate and load? mostly ask if they need to be called on the same thread as the one that created the Synth object
  2. which of the functions in the Synth class are RT safe? (I am mostly interested on set_gain and set_max_voices)
  3. can we call load while process is running in a separate thread? meaning, does liquidsfz take care of pointer swaps, try-locks and other techniques for us or do we need to do that ourselves?
  4. load is a blocking operation, correct? so when we use set_progress_function, the callback function is called inside the load function scope, or does that use a separate thead?
  5. regarding logs, are they sometimes called in a RT context? because if so, developers using this API will need a ringbuffer to display the messages later on the GUI
swesterfeld commented 4 years ago

It would be great if you could use liquidsfz in Carla, this was one of the applications I had in mind for starting this library. Certainly if you need anything, let me know.

So since I just tried to write all answers to your questions about threading as a consistent piece of documentation here: http://space.twc.de/~stefan/liquidsfz/api-0.2.1/

If the documentation is not clear enough, let me know, and I'll try to fix it there. There are two remarks to make

swesterfeld commented 4 years ago
  • typical errors from the RT thread are really developer errors

Ok, I updated this now. Basically we now have two classes of log messages:

  1. Errors, warnings and info messages can never occur from RT thread. These are the messages you typically care about.
  2. Debug messages can occur from any thread at any time, but typically you ignore these anyway, so you need no RT-safe callback. I changed the severity of the example above and other developer relevant messages to debug().

Updated text: http://space.twc.de/~stefan/liquidsfz/api-0.2.1/

be1 commented 4 years ago

In QLiquidSFZ i use Synth::set_gain(float) from the graphical thread. Is a wrong approach ? It runs flawlessly in my environment, but it could be wrong ?

swesterfeld commented 4 years ago

In QLiquidSFZ i use Synth::set_gain(float) from the graphical thread. Is a wrong approach ?

The code for Synth::set_gain(float) was not written for being run parallel to Synth::process(). So currently there is no guarantee that it really works (even if it does in tests).

However it may be a good idea to change the code in the liquidsfz library to support using set_gain() from any thread by using std::atomic internally rather than fixing the problem in QLiquidSFZ.

be1 commented 4 years ago

@swesterfeld Ok I read http://space.twc.de/~stefan/liquidsfz/api-0.2.1/ and I will manage to avoid concurrent calls.

swesterfeld commented 4 years ago

@swesterfeld Ok I read http://space.twc.de/~stefan/liquidsfz/api-0.2.1/ and I will manage to avoid concurrent calls.

Ok, I've looked at your code, this should work. I would have used std::atomic for _gain, as it is accessed by both threads. In any way it seems like it is not a big deal, so supporting concurrent set_gain calls in liquidsfz is not very high priority.

swesterfeld commented 3 years ago

Since liquidsfz-0.2.1 is out now, the new API documentation is officially visible. Threading constraints are better documented now. Also some fine tuning of the logging API ensures that you need not do anything to make that thread safe (ring buffers or so), as no info/warning/error messages are produced in RT / audio thread.

So I'm closing this issue now.