Pass sample rate to Instrument.fluidsynth

[jvlmdr]

The sample-rate fs is only passed implicitly with the synthesizer. However, it is used within Instrument.fluidsynth to compute note positions at the given sample rate.

[craffel]

The trouble is that if a pre-created fluidsynth.Synth is passed via the synthesizer argument, fs won't necessarily match the sampling rate of that synthesizer. Is there a way to retrieve the sampling rate of synthesizer directly? @DavidDiazGuerra

[DavidDiazGuerra]

I didn't realize that Instrument.fluidsynth was using fs for more stuff than creating the fluidsynth instance. This is indeed a bug and will create problems when using a sampling frequency different than 44,1 kHz. Sorry about this.

The sampling frequency of a fluidsynth instance can be retrieved by synthesizer.get_setting('synth.sample-rate'). I think the best solution for this would be leaving PrettyMIDI.fluidsynth as it is in pretty-midi/main now and modifying Instrument.fluidsynth so, when a fluidsynth instance is provided, its sampling frequency is retrieved with that function instead of using the argument function.

I'm thinking that maybe it could be interesting raising a warning (or even an error) from pretty_midi.fluidsynth.get_fluidsynth_instance if both fs and a fluidsynth instance are provided, at least if fs is different than the sampling rate of the fluidsynth instance.

[jvlmdr]

I modified the code to raise a ValueError if both fs and synthesizer are passed but the sample rates don't match.

(The other option would be to create a new synthesizer if they don't match.)

[DavidDiazGuerra]

I think that's the best option since, in order to create a new instance of fluidsynth, we would need the path to the sfz file.

Now I'm wondering what would be the best way to deal with the default values of fs in PrettyMIDI.fluidsynth and Instrument.fluidsynth. In the current implementation, not indicating the value of fs when using an instance of fluidsynth with a sampling rate different than 44.1 kHz (the default value) will raise an error when it was quite clear that the user just wanted to continue using the same sampling rate of the fluidsynth instance. Maybe the best option would be leaving the default value to None and then managing this inside of pretty_midi.fluidsynth.get_fluidsynth_instance and returning the sampling rate of the synthesizer if it was None? But I'm not sure, maybe we're adding too much functionality to pretty_midi.fluidsynth.get_fluidsynth_instance. What do you think @craffel?

[DavidDiazGuerra]

I hadn't seen the last commit when I wrote my previous message. That's indeed in the line of what I was saying, but I'm not sure how it would work since I think not fs would be True only if fs == 0.

[jvlmdr]

I just modified Instrument.fluidsynth to take fs from synthesizer if falsey. I think you're right, though, that the default kwarg of 44100 should be removed to allow fluidsynth(synthesizer=synthesizer). Maybe we should change it to None (and we could use 44100 if fs is None and synthesizer is None to avoid breaking backwards compatibility).

[jvlmdr]

Cool! Maybe fs is None rather than not fs would be more explicit

[craffel]

This looks right to me, requesting mostly cosmetic changes. Thanks.

[jvlmdr]

Thanks! I think I have addressed all requested changes

[craffel]

Thanks!