Skip to content

Conversation

claui
Copy link

@claui claui commented May 21, 2024

The test project triggers several bugs in the third-party MIDIUtil dependency.

These bugs are caused by the de-duplication algorithm in MIDIUtil and a mismatch of expectations between itself and the de-interleaving step that follows.

Bug 1: Crash due to de-duplication

The first bug can be triggered by two or more NoteOn events that happen at the same time but have different durations. In that case, the de-duplication algorithm removes duplicate NoteOn events but leaves the NoteOff events untouched because they occur on different ticks.

This throws the ratio of NoteOn to NoteOff events out of balance and causes a stack underrun in the de-interleaving algorithm.

Example for such a group of events, discovered in the included test pattern:

Reproducer
from pprint import pprint
from polytrackermidi.parsers import patterns
from polytrackermidi.exporters import midi

INPUT_FILENAME = './reverse-engineering/test data/chords and arps/1 chord arp test data project/patterns/pattern_01.mtp'

pattern = patterns.PatternParser(filename=INPUT_FILENAME)
midi_exporter = midi.PatternToMidiExporter(pattern=pattern.parse())

midi_tracks = midi_exporter.generate_midi().tracks

events_for_pitch_24 = [
    (event.tick, event.evtname, getattr(event, 'duration', None))
    for event in midi_tracks[1].eventList
    if event.evtname in ['NoteOn', 'NoteOff'] and event.pitch == 24
]

pprint(sorted(events_for_pitch_24))
Output

The output reveals a duplicate note at tick 1680 but with different durations, and another at tick 1920:

[(960, 'NoteOn', 240),
 (1200, 'NoteOff', None),
 (1680, 'NoteOn', 120),
 (1680, 'NoteOn', 240),
 (1800, 'NoteOff', None),
 (1920, 'NoteOff', None),
 (1920, 'NoteOn', 80),
 (1920, 'NoteOn', 240),
 (2000, 'NoteOff', None),
 (2160, 'NoteOff', None)]

This bug is one of the causes for upstream issue MarkCWirt/MIDIUtil#34.

A fix for the de-interleaving algorithm has been proposed upstream, allowing unbalanced event ratios. However, the upstream project appears unmaintained so the fix might never make it into a new release of MIDIUtil.

As a workaround for polyendtracker-midi-export, this PR skips MIDIUtil’s de-duplication step, which seems to be unnecessary for the use case of this project anyway.

Bug 2: Crash due to zero-duration notes

If a NoteOff event appears at exactly the same MIDI tick as its corresponding NoteOn event, then MIDIUtil’s sorting key causes the NoteOff to appear first, crashing its de-interleaving algorithm.

Hence, MIDIUtil always crashes on a note whose duration is 0.

Example for a zero-duration note, discovered in the included test pattern:

Reproducer
from pprint import pprint
from polytrackermidi.parsers import patterns
from polytrackermidi.exporters import midi

INPUT_FILENAME = './reverse-engineering/test data/chords and arps/1 chord arp test data project/patterns/pattern_01.mtp'

pattern = patterns.PatternParser(filename=INPUT_FILENAME)
midi_exporter = midi.PatternToMidiExporter(pattern=pattern.parse())

midi_tracks = midi_exporter.generate_midi().tracks

events_near_arpeggio = [
    (event.tick, event.pitch, event.evtname, getattr(event, 'duration', None))
    for event in midi_tracks[1].eventList
    if event.tick in range(4400, 4560)
]

pprint(sorted(events_near_arpeggio))
Output

The output reveals a zero-duration note at tick 4559, which would cause MIDIUtil’s de-interleaver to crash:

[(4400, 44, 'NoteOn', 80),
 (4400, 48, 'NoteOff', None),
 (4479, 41, 'NoteOn', 80),
 (4480, 44, 'NoteOff', None),
 (4559, 41, 'NoteOff', None),
 (4559, 48, 'NoteOff', None),
 (4559, 48, 'NoteOn', 0)]

To work around this issue, this PR skips generating a MIDI note if its duration would be zero in MIDIUtil’s time granularity, which is currently 960 ticks per quarter beat.

This bug is probably another cause for MarkCWirt/MIDIUtil#34.

claui added 2 commits May 21, 2024 19:05
The test project included in the test data folder triggers several bugs
in the third-party `MIDIUtil` dependency.

One of the bugs is caused by the de-duplication algorithm in `MIDIUtil`
and a mismatch of expectations between itself and the de-interleaving
step that follows.

The bug can be triggered by two or more `NoteOn` events that happen
at the same time but have different durations. In that case, the
de-duplication algorithm removes duplicate `NoteOn` events but
leaves the `NoteOff` events untouched because they occur on
different ticks.

This throws the ratio of `NoteOn` to `NoteOff` events out of balance
and causes a stack underrun in the de-interleaving algorithm.

Example for such a group of events, discovered in the included test
pattern:

```py
from pprint import pprint
from polytrackermidi.parsers import patterns
from polytrackermidi.exporters import midi

INPUT_FILENAME = './reverse-engineering/test data/chords and arps/1 chord arp test data project/patterns/pattern_01.mtp'

pattern = patterns.PatternParser(filename=INPUT_FILENAME)
midi_exporter = midi.PatternToMidiExporter(pattern=pattern.parse())

midi_tracks = midi_exporter.generate_midi().tracks

events_for_pitch_24 = [
    (event.tick, event.evtname, getattr(event, 'duration', None))
    for event in midi_tracks[1].eventList
    if event.evtname in ['NoteOn', 'NoteOff'] and event.pitch == 24
]

pprint(sorted(events_for_pitch_24))
```

The output reveals a duplicate note at tick 1680 but with different
durations, and another at tick 1920:

```plain
[(960, 'NoteOn', 240),
 (1200, 'NoteOff', None),
 (1680, 'NoteOn', 120),
 (1680, 'NoteOn', 240),
 (1800, 'NoteOff', None),
 (1920, 'NoteOff', None),
 (1920, 'NoteOn', 80),
 (1920, 'NoteOn', 240),
 (2000, 'NoteOff', None),
 (2160, 'NoteOff', None)]
```

This bug is one of the causes for upstream issue 34. [1]

A fix for the de-interleaving algorithm has been proposed upstream
[2], allowing unbalanced event ratios. However, the upstream project
appears unmaintained so the fix might never make it into a new
release of `MIDIUtil`.

As a workaround for `polyendtracker-midi-export`, skip MIDIUtil’s
de-duplication step, which seems to be unnecessary for the use case
of this project anyway.

[1]: MarkCWirt/MIDIUtil#34
[2]: MarkCWirt/MIDIUtil#36
If a `NoteOff` event appears at exactly the same MIDI tick as its
corresponding `NoteOn` event, then MIDIUtil’s sorting key causes the
`NoteOff` to appear first, crashing its de-interleaving algorithm.

Hence, MIDIUtil always crashes on a note whose duration is 0.

Example for a zero-duration note, discovered in the included test
pattern:

```py
from pprint import pprint
from polytrackermidi.parsers import patterns
from polytrackermidi.exporters import midi

INPUT_FILENAME = './reverse-engineering/test data/chords and arps/1 chord arp test data project/patterns/pattern_01.mtp'

pattern = patterns.PatternParser(filename=INPUT_FILENAME)
midi_exporter = midi.PatternToMidiExporter(pattern=pattern.parse())

midi_tracks = midi_exporter.generate_midi().tracks

events_near_arpeggio = [
    (event.tick, event.pitch, event.evtname, getattr(event, 'duration', None))
    for event in midi_tracks[1].eventList
    if event.tick in range(4400, 4560)
]

pprint(sorted(events_near_arpeggio))
```

The output reveals a zero-duration note at tick 4559, which would
cause MIDIUtil’s de-interleaver to crash:

```plain
[(4400, 44, 'NoteOn', 80),
 (4400, 48, 'NoteOff', None),
 (4479, 41, 'NoteOn', 80),
 (4480, 44, 'NoteOff', None),
 (4559, 41, 'NoteOff', None),
 (4559, 48, 'NoteOff', None),
 (4559, 48, 'NoteOn', 0)]
```

To work around this issue, skip generating a MIDI note if its duration
would be zero in MIDIUtil’s time granularity, which is currently
960 ticks per quarter beat.

This bug is probably another cause for upstream issue #34. [1]

[1]: MarkCWirt/MIDIUtil#34
@claui
Copy link
Author

claui commented May 21, 2024

@DataGreed I have uploaded a PKGBUILD, which applies this patch along with the one from #4, and contains two small automated checks. According to those checks, the test pattern now exports to MIDI successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant