Workaround for jack sample rate mismatch

[derselbst]

During the creation of a jack audio driver, it is checked whether the sample-rate of the settings object matches jack's rate. If not, it was adjusted previously via fluid_synth_set_sample_rate(). Due to the
deprecation of that function and removal of the real-time capability of the synth.sample-rate setting, a regression was introduced in 5fbddce causing the synth's sample-rate to be not updated. For such a case, a workaround has been added, which restores the previous behavior by causing the sample-rate to be corrected by using the now deprecated function fluid_synth_set_sample_rate() (to be improved in the future). Unfortunately, this only works when the driver was created with new_fluid_audio_driver(). In case of new_fluid_audio_driver2() we don't have a fluid_synth_t object, so we cannot change the sample-rate.

With this change, an annoying deprecation compiler warning will arise during compilation.
Any feedback, how to handle such a sample-rate mismatch is welcome. (@mawe42, @jjceresa??)

I see the following solutions:

1. If a sample-rate mismatch is detected and cannot be corrected (because we don't have the synth pointer), return with an error.
2. Inform the user about the mismatch and do nothing.
3. Add a new_fluid_audio_driver3 (fluid_synth_t *synth, fluid_audio_func_t func, void *data) and use the settings object that the synth was created with.

[mawe42]

How about
4. Inform the user about the mismatch and return an error from new_fluid_audio_driver2

Also, what about jack changing it's sample rate on the fly? We currently register a callback with jack_set_sample_rate_callback and have the callback simply return success without doing anything. Shouldn't we return an error to let jack know that we can't do that, or maybe not register this callback?

[mawe42]

Alternatively, solve the sample rate problem. Maybe by introducing a "rendering suspended" state into the synth. When going into suspend mode, the audio rendering APIs are still callable, but don't do any real work with the internal structures. They simply return silence. After suspending the rendering, we are free to do any non-realtime stuff and can even access structures in the rendering thread without risk of conflicts. No need for events to be passed to the rendering thread to do stuff, which gives us the ability to return proper error codes etc. As soon as the work is done, we unsuspend the rendering and audio resumes. We could even add a volume ramp up duing the unsuspend, to get rid of any artifacts introduced due to resetting of internal structures.

Quite a large job, though :-/

[derselbst]

Ok, so basically you're voting for 1. :)

Also, what about jack changing it's sample rate on the fly?

I have no clue how to trigger this callback.

Shouldn't we return an error to let jack know that we can't do that, or maybe not register this callback?

Jack's current implementation is not really interested in whether a callback is registered or not. And if we return an error from that callback, Jack only prints an error message:

https://github.com/jackaudio/jack2/blob/b54a09bf7ef760d81fdb8544ad10e45575394624/common/JackEngine.cpp#L309

But in any case, I agree.

Alternatively, solve the sample rate problem.

Just to clarify: Currently, the problem is not how to solve the sample-rate problem. The problem is how or where to get the synth instance from when new_fluid_audio_driver2() was called.

[...Although you're pointing in the right direction with your proposed solution. I would simplify things: I would not expose the sample-rate change via public API. Instead, when we get a sample-rate change callback, we know that no rendering takes place and we can change the sample-rate via an internal function call. If the synth is used outside the audio driver concurrently, the behavior is undefined. Ofc, that would need to be documented...]

The last resort I see without changing the API is to read the e.g. fluid_num_setting_t::data pointer passed to one of fluid_settings_callback_num() functions and hope that it's a synth. If that data pointer is NULL (i.e. no synth created), return with error.

[jjceresa]

During the creation of a jack audio driver, it is checked whether the sample-rate of the settings object matches jack's rate. If not, it was adjusted previously via fluid_synth_set_sample_rate().

If I understand well, your main fear is to ensure that jack driver will work as before for jack user.
On the other hand, I wonder if jack driver needs really this "sample rate adaptation" ?.
I mean , if such "adaptation" isn't implemented in other audio driver and given all fluidsynth drivers (jack and friends) rely on host driver, that means that this feature is probably useless as far a standard sample rate value is set in fluidsynth settings.
If my thoughts are correct, I vote for simple solution 2 because warning the user when audio driver initialization fails seems preferable.

[derselbst]

If I understand well, your main fear is to ensure that jack driver will work as before for jack user.

Yes.

On the other hand, I wonder if jack driver needs really this "sample rate adaptation" ?.

Not sure if an on-the-fly adaption is needed via the sample-rate change callback. But an adaption is definitely needed during the creation of the driver. This is because, for all other drivers, fluidsynth can dictate at which sample-rate audio is being written to the hardware. In case of jack however, jack dictates the sample-rate that we must use. If fluidsynth doesn't respect the sample-rate of jack, the audio is being played at a different pitch.

[jjceresa]

In case of jack however, jack dictates the sample-rate that we must use.

I wasn't aware of this unilateral behaviour (because I am not a jack user). Thanks for this useful information.

[mawe42]

Ok, so basically you're voting for 1. :)

Ah... yes, I think I am :-)

Jack's current implementation is not really interested in whether a callback is registered or not. And if we return an error from that callback, Jack only prints an error message:

Ok, that's interesting. Then ignore my comment about the sample rate callback returning an error. We could print a warning here, though. But that's probably off topic here.

The last resort I see without changing the API is to read the e.g. fluid_num_setting_t::data pointer passed to one of fluid_settings_callback_num() functions and hope that it's a synth. If that data pointer is NULL (i.e. no synth created), return with error.

And then document that the data pointer for callbacks registered for the sample rate change setting needs to be null or a synth instance? Would also be a solution, I think.

But thinking about the original question of this issue again... if I understand correctly then we have introduced a regression that cause both jack audio drivers not to adjust their sample rate to the jack server anymore. And the reason is the deprecation of a function and removal of code calling that function. One way to solve this could be to simply roll back that change completely and restore the previous behaviour. Then think about a proper solution so the sample rate change problem, possibly deprecating the function again.

[derselbst]

The last resort I see without changing the API is to read the e.g. fluid_num_setting_t::data pointer passed to one of fluid_settings_callback_num() functions and hope that it's a synth. If that data pointer is NULL (i.e. no synth created), return with error.

And then document that the data pointer for callbacks registered for the sample rate change setting needs to be null or a synth instance? Would also be a solution, I think.

Ok, I'll update the PR accordingly later this week.

we have introduced a regression that cause both jack audio drivers not to adjust their sample rate to the jack server anymore. And the reason is the deprecation of a function and removal of code calling that function. One way to solve this could be to simply roll back that change completely and restore the previous behaviour.

With the workaround described earlier, I think we have a quite good working solution for now. How we change the sample rate internally can be solved later, I'll file a bug for this.

This "internal" solution will be a simple, non-realtime solution, as I've described previously. I want leave the public API call deprecated because I see no way for a better implementation of a real-time sample-rate change. Doing it via rvoice_events the only way to avoid blocking rendering calls. And when looking through the net, fluid_synth_set_sample_rate() is not even used for real-time sample-rate changes (it doesn't make sense, IMO).

[derselbst]

Ofc, we could also export this "internal" solution via the API, but this would be a breaking change behavior for fluid_synth_set_sample_rate()... I need time to think about this. It's beyond the scope of this PR anyway. I will keep track of this in the bug ticket later. In any case, thanks so far!

[derselbst]

Updated. Now obtaining the synth pointer via the settings. Added a unit test to make sure that this works. And elaborated API doc of new_fluid_audio_driver*(), to explain that settings must be the same as synth->settings.