craffel / pretty-midi

Utility functions for handling MIDI data in a nice/intuitive way.
MIT License
856 stars 151 forks source link

Note starting point can be greater than its ending point #231

Closed TomRolB closed 1 year ago

TomRolB commented 1 year ago

When instantiating a Note object, it's possible to pass to the start parameter a value greater than the one passed to end. If this isn't noticed, it may lead to an error which at first is difficult to track down. Apparently, this is caused by the slice synthesized[current_sample:end], which results in an empty array due to the aforementioned possibility.

After making sure start was always lower than end before passing them to the Note constructor, the problem dissapeared.

/usr/local/lib/python3.10/dist-packages/pretty_midi/pretty_midi.py in fluidsynth(self, fs, sf2_path)
    972             return np.array([])
    973         # Get synthesized waveform for each instrument
--> 974         waveforms = [i.fluidsynth(fs=fs,
    975                                   sf2_path=sf2_path) for i in self.instruments]
    976         # Allocate output waveform, with #sample = max length of all waveforms

/usr/local/lib/python3.10/dist-packages/pretty_midi/pretty_midi.py in <listcomp>(.0)
    972             return np.array([])
    973         # Get synthesized waveform for each instrument
--> 974         waveforms = [i.fluidsynth(fs=fs,
    975                                   sf2_path=sf2_path) for i in self.instruments]
    976         # Allocate output waveform, with #sample = max length of all waveforms

/usr/local/lib/python3.10/dist-packages/pretty_midi/instrument.py in fluidsynth(self, fs, sf2_path)
    522             end = int(fs*(current_time + event[0]))
    523             samples = fl.get_samples(end - current_sample)[::2]
--> 524             synthesized[current_sample:end] += samples
    525             # Increment the current sample
    526             current_time += event[0]

ValueError: operands could not be broadcast together with shapes (0,) (818,) (0,)
craffel commented 1 year ago

Yes, it would make sense to do some validation on construction of the container objects. Note start/end validation is a no-brainer, PRs welcome!