Events are dropped when reading in MidiFile

[chriscoomber]

Context

It is perfectly valid for a MidiFile to contain two noteOns followed by two noteOffs:

tick : event
0    : NoteOn channel 0 key 66 vel 100
1000 : NoteOn channel 0 key 66 vel 100
2000 : NoteOff channel 0 key 66
2000 : NoteOff channel 0 key 66 

It is up to the synthesizer to decide whether it wants to voice this as two notes or not. Here's what the spec has to say:

ASSIGNMENT OF NOTE ON/OFF COMMANDS
If an instrument receives two or more Note On messages with the same key number and MIDI channel, it must make a determination of how to handle the additional Note Ons. It is up to the receiver as to whether the same voice or another voice will be sounded, or if the messages will be ignored. The transmitter, however, must send a corresponding Note Off message for every Note On sent. If the transmitter were to send only one Note Off message, and if the receiver in fact assigned the two Note On messages to different voices, then one note would linger. Since there is no harm or negative side effect in sending redundant Note Off messages this is the recommended practice.

The important thing here is that we need the same number of NoteOffs as NoteOns, otherwise some synthesizers may have a lingering note.

Problem

Some MIDI files, when read by this library, will drop one of these NoteOffs. This is because each track stores its events in a TreeSet, so if two events are "equal" (due to the implementation of Comparable), only one can be stored. This is a bug because perfectly valid MIDI files can cause this to happen, resulting in more NoteOns than NoteOffs, resulting in a lingering note on some (valid) synthesizers.

Reproduction

As a pathological example, the following code (Kotlin) throws an exception (i.e. the error code was run).

val noteTrack = MidiTrack()
noteTrack.insertEvent(NoteOff(0, 0, 66, 64))
noteTrack.insertEvent(NoteOff(0, 0, 66, 64))
if (noteTrack.eventCount != 2) error("One event was dropped")

A more realistic example is as follows. This produces a lingering note on my synthesizer.

private fun generateMidiFile(): MidiFile {
        val tempoTrack = MidiTrack()
        val noteTrack = MidiTrack()

        val ts = TimeSignature()
        ts.setTimeSignature(
            4,
            4,
            TimeSignature.DEFAULT_METER,
            TimeSignature.DEFAULT_DIVISION
        )
        val tempo = Tempo()
        tempo.bpm = 228f

        tempoTrack.insertEvent(ts)
        tempoTrack.insertEvent(tempo)

        noteTrack.insertEvent(ProgramChange(0, 0, 0))

        // Two noteOns - some synthesizers choose to play 2 voices here
        noteTrack.insertEvent(NoteOn(0, 0, 66, 80))
        noteTrack.insertEvent(NoteOn(1000, 0, 66, 100))

        // Two simultaneous noteOffs and an unrelated noteOn at the same time.
        // The first noteOff will be replaced by the second, leaving one more noteOn
        // than noteOff.
        noteTrack.insertEvent(NoteOn(2000, 0, 73, 100))
        noteTrack.insertEvent(NoteOff(2000, 0, 66, 64))
        noteTrack.insertEvent(NoteOff(2000, 0, 66, 64))

        // Kill the unrelated note
        noteTrack.insertEvent(NoteOff(2500, 0, 73, 64))

        // Long pause to hear the ringing, then another note to end the file
        noteTrack.insertEvent(NoteOn(10000, 0, 73, 100))
        noteTrack.insertEvent(NoteOff(11000, 0, 73, 64))

        val tracks: MutableList<MidiTrack> = ArrayList()
        tracks.add(tempoTrack)
        tracks.add(noteTrack)

        return MidiFile(MidiFile.DEFAULT_RESOLUTION, tracks)
    }

And here is how it sounds on my synthesizer (using the violin preset so we can hear the lingering notes):

LingeringNote.mp4

Here's how these MIDI notes should sound (this was achieved by tweaking the velocity of one of the NoteOffs so they weren't equal):

HowItShouldSound.mp4

Fix ideas

I think it's surprising that a set (in this case, a TreeSet) is used to store the events for a track, since you run into exactly this problem with sets (they cannot hold several identical objects). Moreover, it is prudent to remember the order that events were added, even if they occur on the same tick, as you should really honour the order they appeared in the file (e.g. the difference between NoteOn then simultaneous NoteOff, vs NoteOff then simultaneous NoteOn - they may sounds different). So I really think that a simple array or list is a more sensible data structure.

Alternatively, consider having some way to distinguish (i.e. order) two otherwise identical events. Maybe every event gets given some extra integer property tracking the order it was added to the track. This way, we can order two otherwise identical events, so they don't overwrite each other.