Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bogus MIDI time signature and/or tempo imports #13

Open
atsushieno opened this issue Feb 9, 2019 · 4 comments
Open

bogus MIDI time signature and/or tempo imports #13

atsushieno opened this issue Feb 9, 2019 · 4 comments

Comments

@atsushieno
Copy link

atsushieno commented Feb 9, 2019

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
Copy link
Author

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
Copy link
Author

repro SMF: issue13repro.mid.zip

@atsushieno
Copy link
Author

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

@drowaudio
Copy link
Contributor

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

atsushieno added a commit to atsushieno/ntracktive that referenced this issue Mar 12, 2019
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

No branches or pull requests

2 participants