Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solve the sample-rate-change problem #608

Closed
derselbst opened this issue Jan 17, 2020 · 6 comments · Fixed by #722
Closed

Solve the sample-rate-change problem #608

derselbst opened this issue Jan 17, 2020 · 6 comments · Fixed by #722
Labels
Milestone

Comments

@derselbst
Copy link
Member

The synth exposes the function fluid_synth_set_sample_rate() which allows to change the synthesizers sample-rate in real-time. The actual change is not executed immediately. Instead, it is deferred to the next rendering call.

With the implementation of #380 and #548, memory allocation in the reverb and chorus is now required when changing the sample-rate. This is still done while rendering. If the allocation fails, there is no way for the caller to know.


How is fluid_synth_set_sample_rate() used downstream?

In fluidsynth 1.1.x, fluid_synth_set_sample_rate() had the following hint:

http://www.fluidsynth.org/api-1.x/synth_8h.html#ab614bfeafc8b8b632cf1433f8dfce85a

And this is exactly what I find downstream: Create a synth with an incorrect sample-rate, followed by a call to fluid_synth_set_sample_rate() to fix the sample-rate. The real-time capability of fluid_synth_set_sample_rate() is not used. AFAIK it has never been documented as "real-time capable" either.

Because of the misusage downstream and its broken concept (returning void, no error handling possible), JJC and I have deprecated fluid_synth_set_sample_rate(). Chaning the sample-rate of the synth in real-time is not what we consider to be a valid use-case.

Yet, fluidsynth's jack audio driver may require to change the sample-rate of a synth on-the-fly. That's why we need an internal solution. Two approaches are coming to my mind:

  1. Implement an internal sample-rate change function, that may only be called, when no other thread is using the synth's API (not even rendering takes place). Else the behavior is undefined.
  2. When creating the effects units initially, allocate as much memory needed for the highest sample-rate we support.

Solution 1. would exactly fit the puropse required for the jack driver.

I'm not sure if 2. is possible. In any case, it would be a pessimization for people who use fluidsynth on low-resource-hardware. Although this proposal sounds charming, as it might serve as rescue for the public fluid_synth_set_sample_rate() function, keep in mind that an error handling is required, but fluid_synth_set_sample_rate() returns void.

Any opinion how to solve that sample-rate-change problem is highly welcome!

@derselbst derselbst added the bug label Jan 17, 2020
@derselbst derselbst added this to the 2.2 milestone Mar 20, 2020
@jjceresa
Copy link
Collaborator

jjceresa commented Apr 26, 2020

Actual code in DECLARE_FLUID_RVOICE_FUNCTION(fluid_rvoice_mixer_set_samplerate) is:

    for(i = 0; i < mixer->fx_units; i++)
    {
        if(mixer->fx[i].chorus)
        {
            delete_fluid_chorus(mixer->fx[i].chorus);
        }

        mixer->fx[i].chorus = new_fluid_chorus(samplerate);

        if(mixer->fx[i].reverb)
        {
            fluid_revmodel_samplerate_change(mixer->fx[i].reverb, samplerate);
        }
    }
  1. Solution , for the chorus unit.
    We could avoid to delete the unit and add a function fluid_chorus_samplerate_change(chorus, sample_rate). This is possible because actually chorus unit doesn't need to reallocate memory on a sample rate change. The sample rate change always succeed.

  2. Solution for the reverb unit.
    fluid_revmodel_samplerate_change() need to reallocate memory. The function return FLUID_FAILED on error (memory error). In this case we could accept that the synth continue to work but without the reverb. This is a degraded situation but is acceptable (imo) due to the fact that there is not enough memory to get the reverb working correctly. This could lead to the following code:

    for(i = 0; i < mixer->fx_units; i++)
    {
        if(mixer->fx[i].chorus)
        {
            fluid_chorus_samplerate_change(mixer->fx[i].chorus, samplerate);
        }

        if(mixer->fx[i].reverb)
        {
            if(fluid_revmodel_samplerate_change(mixer->fx[i].reverb, samplerate) == FLUID_FAILED)
            {
                   warn the user on stdout, that there is no more reverb because of memory error.
                   /* destroy reverb unit */
                   delete_fluid_revmodel(mixer->fx[i].reverb);
                   mixer->fx[i].reverb = NULL; 
            }
        }
    }

@derselbst
Copy link
Member Author

fluid_revmodel_samplerate_change() need to reallocate memory

The totally required memory increases with the sample-rate, correct? Would it be possible for the reverb to allocate a delay line for the highest sample-rate supported (96kHz) so that no matter how the sample-rate changes, we would always have enough memory for the delay line already allocated?

@jjceresa
Copy link
Collaborator

jjceresa commented Apr 26, 2020

The totally required memory increases with the sample-rate, correct?

Yes, for sample-rate > 44100Hz, the required memory increases with sample-rate.

Would it be possible for the reverb to allocate a delay line for the highest sample-rate supported (96kHz) so that no matter how the sample-rate changes, we would always have enough memory for the delay line already allocated?

Yes it is possible and should be quite simple. I will do a PR.

derselbst pushed a commit that referenced this issue May 1, 2020
This PR allows the reverb to pre-allocate the memory needed for the maximum sample-rate of the synth. That means that on sample rate change there are no memory allocation and the function fluid_revmodel_samplerate_change() should always return FLUID_OK.

The PR addresses discussion in #608.
@jjceresa
Copy link
Collaborator

jjceresa commented May 10, 2020

Now reverb initially allocates as much memory needed for the highest sample-rate we support.

it would be a pessimization for people who use fluidsynth on low-resource-hardware.

For people that want the reverb making use of the minimum of memory, there is still a solution for applications that know that the sample rate will never change.

Example for an application that wants to run at sample-rate 44100Hz:

/* Create the settings. */
 settings = new_fluid_settings();

/* This settings has registered internally a "synth.sample-rate" value:
   default = 44100Hz, minimum=8000Hz, maximum=96000Hz.
*/

/* Now we override this internal value by registering a new one with maximum value diminished to
   the default value
*/
 fluid_settings_register_num(settings, "synth.sample-rate", 44100.0f, 8000.0f, 44100.0f, 0);

/* Then we create the synthesizer. It will create internally the reverb that will take the actual
   sample-rate default value (44100Hz) and maximum value (44100Hz).
*/
 synth = new_fluid_synth(settings);

@derselbst
Copy link
Member Author

That doesn't work. fluid_settings_register_num is not part of the public API. If one really needs a version of fluidsynth which makes use of as little memory as possible, one would need to manually tweak the source code and compile that customized version. There are many things one could do here. Stripping away all log messages to save ROM memory is also one of them. That is all highly experimental and I'm not willing to support this officially. However, feel free to document those points somewhere in the wiki.

@jjceresa
Copy link
Collaborator

jjceresa commented May 11, 2020

That is all highly experimental and I'm not willing to support this officially.

Yes, this is comprehensible.

However, feel free to document those points somewhere in the wiki.

I will probably do that later. I feel very few people that could need a version that makes use of little memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants