Skip to content

Do not push size of the data into the SysEx#103

Open
Monceber wants to merge 1 commit intocraigsapp:masterfrom
Monceber:remove_SysEx_headersize
Open

Do not push size of the data into the SysEx#103
Monceber wants to merge 1 commit intocraigsapp:masterfrom
Monceber:remove_SysEx_headersize

Conversation

@Monceber
Copy link

@Monceber Monceber commented Jun 9, 2022

I've done some testing with mts-type2.cpp and adding size to the SysEx seems to break the functionality. The message is ignored and no tune changes are made.

Here is an archive with two WAV files (converted from MIDI to WAV via Timidity) and two original MIDI files. quarter_with_size has size of the data array added to the message, while quarter_without_size - does not.
quarters.zip

Additionally, if I were to compare results from #99 with the ones that I get, they seem to match with the ones, made without adding size.
Size of file from archive in this matches with the size of MIDI generated without size (368 bytes). As does MIDI generated from Ratioscore lack this one byte, representing size of the SysEx message.

@infojunkie
Copy link

infojunkie commented Oct 15, 2025

Hi @craigsapp , any plan on merging this fix? At the moment, Verovio's tuning (and any sysex) feature is broken because the exported MIDI is invalid due to this bug. Thanks!

@craigsapp
Copy link
Owner

craigsapp commented Oct 15, 2025

Yes I should fix it soon. To summarize: (1) the size of the metamessage should be written as a Variable Length Value, or (2) for sysex message the length of the dat shoule be removed sinde there is a byte code to end the message.

@infojunkie
Copy link

When I analyzed this issue, I found that MidiFile.cpp already sends the required VLV, so the one inserted in MidiMessage.cpp is redundant and invalidates the SMF.

@infojunkie
Copy link

Hello, is there any support I can bring to help get this fix merged?

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.

3 participants