get_end_time only inspects Notes and PitchBends

[craffel]

It should inspect all event types.

[craffel]

Resolved in 22fea72.

[areeves87]

Hi @craffel , can you elaborate on what motivated you to inspect all event types? I see advantages to only using note events.

I'm only thinking about this because I ran into an issue with a midi file that had tempo change events well after any notes were played. The tempo change events happend for about 20 minutes, yet the music lasted only 5 minutes. None of these tempo changes affected sound output because there were no note events to report the tempo. When I applied the align.extract_cqt method, the 20 minute wav file produced a large and unwieldy matrix which crashed my computer when I tried to compute its dot product with another somewhat large matrix.

If get_end_time only looked at note values it would accurately report the duration of the music. So what are the advantages to using meta events to determine the end of the track?

[craffel]

Well, there is information in events other than notes, so presumably those should be considered part of the song. For example, if the sustain pedal is on (according to a CC value), the note should ring (and be modified by pitch bends) after the note off. Or, in a MIDI karaoke file, there may be a lyrical section after the rest of the instruments stop playing. As an aside, if you'd like to get the end time of the last note, it should be a one-liner:
end_time = max(max(n.end for n in i.notes) for i in pm.instruments)

[areeves87]

Good points, thanks for elaborating and for the one-liner. My immediate problem is solved.

I just want to give my two cents on the larger issue of the get_end_time() method. The way that I see get_end_time() used in your code base suggests it is mostly for cropping wav files synthesized from midi files. And in that case the method can give misleading values. Lyrics don't get synthesized and sustained notes will only last for so long, maybe 1 second. So lots of meta events happening 5, 10, or more seconds after the end of the last note would needlessly increase the size of the midi audio arrays.

Using the one-liner you wrote, I took the difference of get_end_time() and the one-liner for a corpus of 221 videogame midis. So I ended up with 221 time differences.

• In 93% of files there was no difference at all between the two methods.
• In 4.5% of files there was a difference of more than 1 second
• In 3% of the files there was a difference of more than 10 seconds
• One of them had a difference of 900 seconds!

My test corpus shows 4.5% of files with more than 1 second of events after the end of the last note. So yeah, I acknowledge that we are talking about a small fraction of total files here. But my guess is most of these files have nonsense events at the end rather than useful information.

It could be different with your lakh midi dataset. There might be a much smaller fraction of files with meta events after the music. There might be many cases where lyrics continue after the music, etc. I haven't had time to test it.

[craffel]

The way that I see get_end_time() used in your code base suggests it is mostly for cropping wav files synthesized from midi files.

Which codebase are you referring to? It's used twice in pretty_midi as of right now; once in Instrument.get_piano_roll and once in PrettyMIDI.get_beats. The piano roll should be extended at least to the final sustain pedal, and get_beats should be extended at least to the last tempo change, so these both require more than the last end note.

And in that case the method can give misleading values. Lyrics don't get synthesized and sustained notes will only last for so long, maybe 1 second. So lots of meta events happening 5, 10, or more seconds after the end of the last note would needlessly increase the size of the midi audio arrays.

If this is an issue then get_end_time shouldn't be used there; the one-liner should be used instead. get_end_time is defined unambiguously to be "the time of the last thing that happens in the MIDI object", and nothing else. If, for a specific use-case, it's more useful to know "the end time of the last note", or something similar, then a different function should be used instead. The goal of pretty_midi is to make such a function really easy to write, and I think the one-liner is evidence that this goal has been achieved here.

One of them had a difference of 900 seconds!

Sounds like a problem with the MIDI file more than anything else :)

[areeves87]

I am referring to the way get_end_time is used in the fast_fluidsynth gist and midi-dataset.feature_extraction.fast_fluidsynth. The two cases are essentially the same code and the code is used for cropping the synthesized midi audio.

Even though the fast_fluidsynth code isn't part of the official codebase for pretty midi, I've ended up using it in the example/align_midi.py procedure as a workaround for issues with fluidsynth.py.

Yes, after looking at how you've used get_end_time() in pretty_midi I admit that I was considering a too-narrow scope for the method. There are good use cases for the method as is. And after some further research I saw that you use pretty_midi.instrument.fluidsynth to crop according to note, pitch-bend, and control-change events and ignore meta events such as lyrics and tempo changes. I initially overlooked this code because I am using fast_fluidsynth instead.

This whole rant started with me trying to figure out why that one midi file had so much silent audio. Now I understand how get_end_time works and how it fits into the larger picture, so I'm okay with how things turned out.

Thanks again for your thoughts and patience.

[craffel]

Sounds good :) now at least people will understand the rationale!