search

atsushieno / ktmidi

Kotlin multiplatform library for MIDI access abstraction and data processing for MIDI 1.0, MIDI 2.0, SMF, SMF2 (MIDI Clip File), and MIDI-CI.
MIT License
70 stars 7 forks source link

Fix delta calculations in Midi1TrackMerger #51

Closed AlecJY closed 1 year ago

AlecJY commented 1 year ago

Currently deltaTime of all Midi1CompoundMessage and the dummy Midi1SimpleMessage won't be updated. All other messages after the message will be affected by the wrong deltaTime. Therefore, I filter all dummy messages out when rebuilding the list and remain other valid Midi1SimpleMessage and Midi1CompoundMessage.

atsushieno commented 1 year ago

Hi, thanks for the PR here too!

I am curious, are we receiving "dummy" Midi1SimpleMessage from somewhere?

AlecJY commented 1 year ago

I'm not sure when the thing will happend currently, except when a user manually adds a dummy message to Midi1Track. I just noticed that the original code handled the scenario when I read the originnal code. At least on JVM, any remaining dummy messages can lead to an InvalidMidiDataException when playing with Midi1Player. However, other malformed messages can also trigger the error. Therefore, I think both removing a dummy message or leaving it are proper behaviors. If you think not removing a dummy message is a better approch, I can modify the code.

atsushieno commented 1 year ago

Yes, please. Midi1TrackMerger is responsible only on merging tracks, not removing invalid messages. Removing invalid data should be done be something like "Midi1SequenceValidator" or whatever that ktmidi currently does not provide, and then the verification should not be incomplete like only checking 0.

AlecJY commented 1 year ago

Updated. But it appears that CI failed while attempting to download Node.js. It may require a rerun.

atsushieno commented 1 year ago

merged, thanks!