Yikai-Liao / symusic

A cross platform note level midi decoding library with lightening speed, based on minimidi.
https://yikai-liao.github.io/symusic/
MIT License
108 stars 8 forks source link

MIDI files with simultaneous note on/off ticks result in incorrect note durations #38

Closed ilya16 closed 2 months ago

ilya16 commented 2 months ago

Describe the bug Notes with simultaneous note on and note off events (zero duration) are parsed incorrectly. The note off message is ignored and the duration is extended until the next note off event.

To Reproduce An example MIDI file that has such corrupted messages: https://github.com/fosfrancesco/asap-dataset/blob/master/Bach/Fugue/bwv_883/KaiRuiR03.mid

For example, there is a zero-length note with pitch 72 at tick 114351. Printing all events with pitch 72:

mido 1.2.10 ``` note_on channel=0 note=72 velocity=88 time=67 note_off channel=0 note=72 velocity=64 time=25 note_on channel=0 note=72 velocity=88 time=16 note_off channel=0 note=72 velocity=56 time=28 note_on channel=0 note=72 velocity=81 time=11 note_off channel=0 note=72 velocity=65 time=64 note_on channel=0 note=72 velocity=89 time=14 note_off channel=0 note=72 velocity=63 time=2 note_on channel=0 note=72 velocity=90 time=80 note_off channel=0 note=72 velocity=64 time=3 note_on channel=0 note=72 velocity=77 time=16 note_off channel=0 note=72 velocity=63 time=9 note_on channel=0 note=72 velocity=85 time=7 note_off channel=0 note=72 velocity=64 time=10 note_on channel=0 note=72 velocity=90 time=5 note_off channel=0 note=72 velocity=59 time=39 note_on channel=0 note=72 velocity=35 time=17 note_off channel=0 note=72 velocity=127 time=0 note_on channel=0 note=72 velocity=89 time=2 note_off channel=0 note=72 velocity=13 time=14 ```
miditoolkit 1.0.1 ``` Note(velocity=88, pitch=72, start=28440, end=28739), duration=299 Note(velocity=88, pitch=72, start=75032, end=75159), duration=127 Note(velocity=81, pitch=72, start=75966, end=76030), duration=64 Note(velocity=89, pitch=72, start=77486, end=77568), duration=82 Note(velocity=90, pitch=72, start=78786, end=78941), duration=155 Note(velocity=77, pitch=72, start=87932, end=87994), duration=62 Note(velocity=85, pitch=72, start=90025, end=90085), duration=60 Note(velocity=90, pitch=72, start=113164, end=113263), duration=99 Note(velocity=35, pitch=72, start=114351, end=114351), duration=0 Note(velocity=89, pitch=72, start=126793, end=127634), duration=841 ```
symusic 0.4.5 ``` Note(time=28440, duration=299, pitch=72, velocity=88) Note(time=75032, duration=127, pitch=72, velocity=88) Note(time=75966, duration=64, pitch=72, velocity=81) Note(time=77486, duration=82, pitch=72, velocity=89) Note(time=78786, duration=155, pitch=72, velocity=90) Note(time=87932, duration=62, pitch=72, velocity=77) Note(time=90025, duration=60, pitch=72, velocity=85) Note(time=113164, duration=99, pitch=72, velocity=90) Note(time=114351, duration=13283, pitch=72, velocity=35) ```

Expected behavior I think events for notes with same note on/off should at least be parsed as notes with 0 duration (as done in miditoolkit) or ignored as they have no meaning. Giving them a duration of 1 tick is probably not the right choice.

Yikai-Liao commented 2 months ago

image

After checking, I found that this was a bug caused by an error in one if condition. I should use >= here rather than >.

Changing the comparasion operator will lead to the right answer:

image

You could further test this version just from the main branch. If there is no other problem, I will release this version.

Yikai-Liao commented 2 months ago

I just released the new version 0.4.6

ilya16 commented 2 months ago

@Yikai-Liao sorry for not replying right away, thanks for the quick response!