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

Several fixes, improving robustness and compatibility with MIDI files. #2

Merged
merged 9 commits into from
Apr 24, 2017

Conversation

Boscop
Copy link
Member

@Boscop Boscop commented Nov 4, 2016

I added some things that I found necessary in my usage:

  • For events that have content, Display didn't print their contents, now it does. Useful for debugging, which I did a lot :)
  • In practice, all MIDI files use latin1 encoding for strings. When assuming utf8 like it was before, it crashes on a lot of MIDI files containing upper range latin1 chars (e.g. from eastern europe). Now it uses latin1 (with utf8 as fallback but I never saw latin1 fail in practice). I tested this on a large archive of several GB of midi files of songs from several decades.
  • channel() should return the correct MIDI channel according to the MIDI spec, starting at 0. Now it does, also returns None for events that don't have a channel
  • Before it choked on MIDI files containing SysEx msgs, now it skips them.
  • Some .mid files contain a RIFF header, which this lib previously couldn't handle, now it does (skips it).
  • There was a bug handling running status. What it did was try to use the status from the last event, and if the last event was a Meta event, it silently used status 0, which is incorrect. It failed on several midi files in my huge collection. So I looked it up, the correct behavior is to use the status of the last midi (non-meta) event (i.e. events that actually have a status). Now it does the correct thing.
  • Previously if a midi file was corrupted / chopped off at a certain length, it got stuck in an infinite loop trying to read more, now it doesn't, instead it returns Err(Error::new(ErrorKind::InvalidData, "file ends before it should")).
  • In order to play back midi files it's useful to have them in Type 1 multi-track format instead of Type 0 one-track. So I added a function that converts a SMF into multi-track:
+    /// Convert a type 0 (single track) to type 1 (multi track) SMF
+    /// Does nothing if the SMF is already in type 1
+    /// Returns None if the SMF is in type 2 (multi song)
+    pub fn to_multi_track(&self) -> Option<SMF> {

The reverse operation would be easy to add, too, but I haven't had a need for it.
After these fixes, I haven't encountered a MIDI file that this lib couldn't handle. In fact, some MIDI files that AIMP 3 can't parse/play (e.g. certain type 0 one-track files) and that freeze MidiEditor.exe (like when a MIDI file ends before it should) are handled fine by this lib now.

@mitchmindtree
Copy link
Member

Thanks @Boscop! I don't have much experience with this crate yet so it's hard for me to review.

@nicklan any chance you might be able to take a quick look at this?

@Boscop
Copy link
Member Author

Boscop commented Nov 22, 2016

I added three more fixes:
When writing SMF files:

  • Track length was accumulated over ALL tracks, instead of for each track, which lead to midi files that some software couldn't open because the reported length was wrong
  • Similarly, saw_eot was also carried over all tracks, so only the first track's EndOfTrack event was actually written because for the tracks after that it looked like their marker was already written. This also caused some software to not be able to open the midi files.

The fix was just to put the variables in the loop that goes over the tracks when writing, so that they are reset before writing each track.

The third fix was: In the SMF conversion from type 0 (single-track) to 1 (multi-track) files, the times for the events were converted back to delta time at the end, but not for the meta-event track aka track 0. This caused some software like AIMP to report a very long song length (and of course meta events didn't get triggered at the right time). Now this is fixed too.

@mitchmindtree
Copy link
Member

Nice work @Boscop :)

I'll leave this up a little longer to give nicklan a chance to reply, otherwise I'll merge.

@Boscop
Copy link
Member Author

Boscop commented Mar 23, 2017

@nicklan Any comments? :)

@Boscop
Copy link
Member Author

Boscop commented Apr 24, 2017

@mitchmindtree Can you merge, please :)

@mitchmindtree mitchmindtree merged commit 75007aa into RustAudio:master Apr 24, 2017
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.

2 participants