tracktion_engine icon indicating copy to clipboard operation
tracktion_engine copied to clipboard

bogus MIDI time signature and/or tempo imports

Open atsushieno opened this issue 5 years ago • 4 comments

I am trying to import some MIDI file into Waveform 9 (tried 10beta too), but the imported data is corrupt around where time signature changes. I suspect that tracktion_engine fails to deal with them.

I can't share my song in production, but my pseudo MIDI event list for my master track (0) is like this:

	BEAT4,4 r1
	MARKER "Section A"
	t110 [r1]2  t112[r1]2  t114[r1]2  t115[r1]2
	[r1]8
	MARKER "Section B"
	[ r1  BEAT7,8r2..BEAT4,4 ]3  r1  BEAT3,4r2.BEAT4,4
	[ r1  BEAT7,8r2..BEAT4,4  r1  BEAT3,4r2.BEAT4,4 ]2
	MARKER "Section C"
	t120
	[ BEAT3,4r2.r2.  BEAT4,4r1BEAT7,8r2..]2
	BEAT3,4r2.r2.  BEAT4,4r1BEAT7,8r2..

(t is tempo, r is rest, BEAT is for time signature, and MARKER is marker meta text. Edit: and '[' to ']'n means "repeat n times".)

Starting Section B, Waveform fails to generate valid tempo/timesig changes, I can't really logically explain the actual behavior. It would be reproducible but changes them at many points.

Tracking the MIDI importer sources, this code smells:

            if (msg2.getTimeStamp() == msg.getTimeStamp())
            {
                ++i;

                if (msg2.isTempoMetaEvent())
                    secsPerQuarterNote = msg2.getTempoSecondsPerQuarterNote();
                else if (msg2.isTimeSignatureMetaEvent())
                    msg2.getTimeSignatureInfo (numer, denom);
}

I guess this assumes that they are either "both time signature" or "both tempo" and thus ignores the other. But my song contains a time signature change and a tempo change at the same time at Section C.

atsushieno avatar Feb 09 '19 07:02 atsushieno

I tried some workaround by avoiding this "timesig and tempo changes at the same time" pattern, but I still see weird imports. I will try to create some repro song that only contains the master track later.

So far, GarageBand could successfully import the song, and timidity player played it without problem, so I assume the song itself was not bogus (it was generated my own compiler tool).

atsushieno avatar Feb 09 '19 08:02 atsushieno

repro SMF: issue13repro.mid.zip

atsushieno avatar Feb 09 '19 09:02 atsushieno

On further investigation, this is another piece of code in doubt:

bpms.add (4.0 * 60.0 / (denom * secsPerQuarterNote));

where secsPerQuarterNote is:

 secsPerQuarterNote = msg.getTempoSecondsPerQuarterNote();

The value is assigned only when the MIDI track contains the tempo change, which looks correct. But then the call to bpms.add() above re-calculates BPMs every time based on the changed denom value, which results in different BPM value, while the imported song has never changed the tempo.

This denom multiplication (so as 4 in the same expression) is unnecessary. That seems like mis-interpretation of the specification around MIDI tempo value.

Attaching another simplified repro, which (in the same pseudo code) is like:

   // track 0
   GM_SYSTEM_ON  t120 [ BEAT4,4 r1 BEAT4,8 r1 ]4  t100

   // track 1
   PROGRAM 0 VOLUME 110  [ o5c1 o5c2 o5c2 ]4

Untitled-1.mid.zip

atsushieno avatar Feb 10 '19 10:02 atsushieno

Thanks for the report and detailed info. I'll take a look at this soon hopefully.

drowaudio avatar Feb 13 '19 10:02 drowaudio