Allow reusing the Fluidsynth instance for synthesis

[DavidDiazGuerra]

Hello,

This PR modifies the fluidsynth method of the class Instrument to allow providing an existing Fluidsynth instance with the sf2 file already loaded instead of providing the path to the sf2 file and creating a new Fluidsynth instance inside the method. The fluidsynth method of the class PrettyMIDI is modified in a similar way and also to take advantage of the modifications in Instrument.fluidsynth so the same Fluidsynth instance is used to synthesize all the instruments in the song.

These changes really reduce the synthesis time of MIDI objects with many instruments without the users needing to modify their code and allow them to speed up their code even more if they need to synthesize many different MIDI objects by creating their own Fluidsynth instance by themselves and reusing it for all the objects.

Best, David

[craffel]

This is nice, thanks. I feel it would be best to allow the sf2_path argument to either a path or a fluidsynth.Synth instance, and behave appropriately based on which is provided. But then we'd need to change the argument name, which would break preexisting code that used the sf2_path kwarg. We could change the argument name and make a new sf2_path kwarg which would throw a deprecation warning when it was used or something. WDYT?

[DavidDiazGuerra]

I agree that would be the most elegant solution. I didn't do that because I didn't want to break any preexisting code, but the deprecation warning makes sense to me. In any case, when an instance of fluidsynth.Synth is provided, we'll still need an additional parameter (sfid) indicating the id of the soundfount that should be used.

[craffel]

Yeah, let's go ahead and do it with the shared arg (adding the sfid arg). Thanks!

[DavidDiazGuerra]

I didn't know how to call that new arg and I finally called it synthesizer, but I'm not sure if that's the best name for it. I don't have much experience working with this kind of project so I don't know if the comments are right for the documentation and if the way I'm raising the warnings is the best.

[craffel]

Sorry for not being clear - to remain backwards-compatible with calls that use positional arguments, you should keep the argument order the same apart from the moved one (i.e. it should be def fluidsynth(self, fs=44100, synthesizer=None, sfid=0, sf2_path=None):

[DavidDiazGuerra]

Oh, yeah, I had move fs to the end since it is not used when an instance of fluidsynth.Synth is provided but I hadn't realised that would break the code of those who were using the function without indicating the key words of the arguments. It should be fixed now.

[craffel]

Ah, sorry about this, but I hadn't noticed that this ended up duplicating a lot of code between pretty_midi.py and instrument.py. I'm assuming you want the logic to be in PrettyMIDI.synthesize because you want to be able to avoid re-loading the synthesizer between each call to Instrument.synthesize, but we need the logic in Instrument.synthesize too in case it's called outside of PrettyMIDI.synthesize. In that case, I'd much prefer we factor out that logic into a separate utility function (which would handle loading, or not, the synthesizer and optionally using the default SF2 etc.). The only issue there is that you'd still need to know whether to delete the SF2, but that could be inferred by testing whether the original argument to the function was a string or not. I think this function should probably go in a new file (e.g. fluidsynth.py). I recognize this is probably biting off more than you wanted to chew, but I'd prefer to avoid duplicate code and in this case it seems possible.

[DavidDiazGuerra]

I also realized the code repetition as I was doing the changes, but I wasn't sure about how to avoid it (I didn't want to create a new file). I'll try to move the logic for loading the synthesizer when needed to a new file during the week. I'll let you know when it's done.

[DavidDiazGuerra]

I've finally found some time for this. I've left the code related to the kwarg sf2_path in the original functions so the deprecation warning is raised in the function called by the user. Apart from the fluidsynth.Synth instance and the sfid, the new function also returns a bool indicating if a new instance was created and therefore needs to be deleted to avoid memory leaks.

[craffel]

Looks good, thanks for this!