LiquidSFZ remaining integration issues

[tim-janik]

List of issues that need fixing to complete successful integration of LiquidSFZ:

• [x] The SFZ instrument field in the UI needs serialization, turned out to be a data race
• [x] A file browser should popup at the UI for the instrument field, ideally not allowing direct text edits
• [ ] To make the file browser somewhat usable, Anklang should provide a sample library search facility the user can choose from, this might be Milestone 0.4 material though.
• [ ] Once an SFZ file is loaded, properties need to be rescanned, to show e.g. Panning and Volume props
• [ ] We need to get rid of the printerr() calls in there and move to a DEBUG() macro

Updated 2024-06-12.

[tim-janik]

In current trunk, loading of LiquidSFZ instruments fails. This is due to load_.idle conditional processing of input events in devices/liquidsfz/liquidsfz.cc:render(). Initially, the module is reset, which causes the loader to be busy, so the first time render() is called with a PARAM_VALUE event that assigns the INSTRUMENT property, render() simply doesn't process the input event queue at all, so it misses the INSTRUMENT value. In short, input events always have to be processed, Anklang couldn't possibly know when to resend events. Something like the following patch forces event processing, @swesterfeld please give some input for the rest of the synth/loader logic:

diff --git a/devices/liquidsfz/liquidsfz.cc b/devices/liquidsfz/liquidsfz.cc
index 00abb7d8..e1b372f2 100644
--- a/devices/liquidsfz/liquidsfz.cc
+++ b/devices/liquidsfz/liquidsfz.cc
@@ -172,8 +172,6 @@ class LiquidSFZ : public AudioProcessor {
   }
   void
   render (uint n_frames) override
-  {
-    if (loader_.idle())
   {
     if (synth_need_reset_)
       {
@@ -190,6 +188,7 @@ class LiquidSFZ : public AudioProcessor {
             synth_.add_event_note_off (ev.frame, ev.channel, ev.key);
             break;
           case MidiMessage::NOTE_ON:
+            if (loader_.idle())
               synth_.add_event_note_on (ev.frame, ev.channel, ev.key, std::clamp (irintf (ev.velocity * 127), 0, 127));
             break;
           case MidiMessage::ALL_NOTES_OFF:
@@ -206,6 +205,8 @@ class LiquidSFZ : public AudioProcessor {
           }
       }

+    if (loader_.idle())
+      {
         float *output[2] = {
           oblock (stereo_out_, 0),
           oblock (stereo_out_, 1)

[swesterfeld]

Is there a way to trigger the problem?

[tim-janik]

Is there a way to trigger the problem?

Save a LiqudSFZ device with an instrument assigned. Try loading it, the input event for loading the INSTRUMENT is never processed. Regardless, all input events must always be processed in render, otherwise random events will be missed.

[swesterfeld]

all input events must always be processed in render

I agree. At least the filename changes, midi events can safely be dropped while a file is being loaded.

Your patch is not good, because it will introduce race conditions of both threads accessing the same data. Since we cannot use locks due to RT constraints, I guess the real fix will be to use a lock-free queue to send events to the loader thread. I'll look into it and come up with a real fix and submit a PR. However, here is a quick fix that just avoids the initial load problem and is still safe as in it doesn't introduce race conditions:

diff --git a/devices/liquidsfz/liquidsfz.cc b/devices/liquidsfz/liquidsfz.cc
index 00abb7d8..ea97bf9b 100644
--- a/devices/liquidsfz/liquidsfz.cc
+++ b/devices/liquidsfz/liquidsfz.cc
@@ -73,7 +73,7 @@ public:
   {
     if (state_.load() == STATE_IDLE)
       {
-        if (want_sfz_ == have_sfz_ && want_sample_rate_ == have_sample_rate_)
+        if (want_sfz_ == have_sfz_ && (want_sample_rate_ == have_sample_rate_ || want_sfz_ == ""))
           return true;
       }
     state_.store (STATE_LOAD);

You can use this until we have a better solution.

[tim-janik]

all input events must always be processed in render

I agree. At least the filename changes, midi events can safely be dropped while a file is being loaded.

Your patch is not good, because it will introduce race conditions of both threads accessing the same data.

Yes, that's why I pinged you ;-)

Since we cannot use locks due to RT constraints, I guess the real fix will be to use a lock-free queue to send events to the loader thread. I'll look into it and come up with a real fix and submit a PR.

We have lock free Queue like structures already. In atomics.hh you will find AtomicStack, suitable for MpMc scenarios or AtomicIntrusiveStack.pop_reversed() which can be used as an MpSc queue.

However, here is a quick fix that just avoids the initial load problem and is still safe as in it doesn't introduce race conditions:

diff --git a/devices/liquidsfz/liquidsfz.cc b/devices/liquidsfz/liquidsfz.cc
index 00abb7d8..ea97bf9b 100644
--- a/devices/liquidsfz/liquidsfz.cc
+++ b/devices/liquidsfz/liquidsfz.cc
@@ -73,7 +73,7 @@ public:
   {
     if (state_.load() == STATE_IDLE)
       {
-        if (want_sfz_ == have_sfz_ && want_sample_rate_ == have_sample_rate_)
+        if (want_sfz_ == have_sfz_ && (want_sample_rate_ == have_sample_rate_ || want_sfz_ == ""))
           return true;
       }
     state_.store (STATE_LOAD);

You can use this until we have a better solution.

I suppose you mean this to be applied on top of my chage?

[swesterfeld]

We have lock free Queue like structures already

I know.

I suppose you mean this to be applied on top of my chage?

No. Do not apply your change. It breaks stuff. Just my change and Anklang will load projects properly. I tested that.

[tim-janik]

We have lock free Queue like structures already

I know.

I suppose you mean this to be applied on top of my chage?

No. Do not apply your change. It breaks stuff. Just my change and Anklang will load projects properly. I tested that.

Ok, I see now what you did. I was hoping for a quick fix that involves unconditional input event processing. Having a proper PR is definitely appreciated, correctly attributing GH issue patches is kind of a pain.

I've left a TODO comment in there, as I'd rather not have someone copy conditional input event processing by accident.