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

Writing and reading back in should be roughly lossless #52

Open
craffel opened this issue Nov 25, 2015 · 11 comments
Open

Writing and reading back in should be roughly lossless #52

craffel opened this issue Nov 25, 2015 · 11 comments

Comments

@craffel
Copy link
Owner

craffel commented Nov 25, 2015

I.e. if you write a MIDI file out and read it back in, you should get at least the same collection of notes, time signature changes, tempo changes, pitch bends, key signatures, etc. (all of the data that pretty_midi stores)

@douglaseck
Copy link
Contributor

I was going to file a bug but I'll attach it to this bug. In TimeSignature, you don't keep around the notated 32nd-notes in a MIDI quarter-note (24 MIDI Clocks) so when you write a file, this information is lost. An incoming TimeSignature might be:
midi.TimeSignatureEvent(tick=0, data=[6, 3, 36, 8]).
If you load that into pretty_midi and write it back out you get:
midi.TimeSignatureEvent(tick=0, data=[6, 3, 0, 0]),

If you don't want to store info, you could at least set some reasonable default such as you do when you write the default 4/4 time signature:
timing_track += [midi.TimeSignatureEvent(tick=0, data=[4, 2, 24, 8])]
(24 midi clocks per metronome click, 8 32nd notes in a midi qn)

That seems like a reasonable default. I personally don't care all that much. I just don't want these files to break some sequencer software because of 0 values in these fields.

@douglaseck
Copy link
Contributor

Also it's probably not a good idea to always add a default time signature. At the very least only add a time signature if there's not already a time signature defined at time 0.

@craffel
Copy link
Owner Author

craffel commented Mar 10, 2016

In TimeSignature, you don't keep around the notated 32nd-notes in a MIDI quarter-note (24 MIDI Clocks) so when you write a file, this information is lost.

Ah, interesting, I hadn't noticed that we were missing that information when writing out (here). Do you know if this is totally meta-information, or will they ever effect playback?

Also it's probably not a good idea to always add a default time signature. At the very least only add a time signature if there's not already a time signature defined at time 0.

I think I read somewhere that it's bad practice in a MIDI file to not include any time signature changes, but it turns out about 10% of the MIDI files I've found "in the wild" have no time signature events. So, not sure which the best choice is, but I agree that we shouldn't add a default time signature at time 0 if there's already one there.

@douglaseck
Copy link
Contributor

I doubt if it affects playback. It broke a unit test that verifies that a
PM to proto to PM loop creates the same prettymidi info. Seems worthwhile
doing the minimally invasive fix for now. Those kinds of tests are useful
and in complex pipelines your could end up with a lot of 4/4 meter events
:-). I should learn to really work with git and submit a fix myself. But
as before the workflow is so different from Google that I forget.
On Mar 9, 2016 6:02 PM, "Colin Raffel" notifications@github.com wrote:

In TimeSignature, you don't keep around the notated 32nd-notes in a MIDI
quarter-note (24 MIDI Clocks) so when you write a file, this information is
lost.

Ah, interesting, I hadn't noticed that we were missing that information
when writing out (here
https://github.com/craffel/pretty-midi/blob/master/pretty_midi/pretty_midi.py#L919).
Do you know if this is totally meta-information, or will they ever effect
playback?

Also it's probably not a good idea to always add a default time signature.
At the very least only add a time signature if there's not already a time
signature defined at time 0.

I think I read somewhere that it's bad practice in a MIDI file to not
include any time signature changes, but it turns out about 10% of the MIDI
files I've found "in the wild" have no time signature events. So, not sure
which the best choice is, but I agree that we shouldn't add a default time
signature at time 0 if there's already one there.


Reply to this email directly or view it on GitHub
#52 (comment).

@craffel
Copy link
Owner Author

craffel commented Mar 10, 2016

Seems worthwhile doing the minimally invasive fix for now.

For sure. Since this issue is kind of a far-reaching one, I created a separate issue, which I can resolve soon #61.

@craffel
Copy link
Owner Author

craffel commented Mar 10, 2016

Out of curiosity, I grabbed 10,000 random MIDI files and computed the relative occurrence of different values in the third and fourth data slots of TimeSignatureChange events:

Number of MIDI ticks in a metronome click
24: 72.33%
12: 10.18%
96:  9.14%
0:   1.50%
36:  1.46%
48:  1.42%
6:   0.82%
...
Number of notated 32nd notes in a MIDI quarter note
8:   97.00%
42:   0.45%
161:  0.24%
239:  0.17%
10:   0.12%
227:  0.12%
82:   0.11%
...

Thank goodness 8 appears in slot 4 97% of the time, I'm curious about the files for which it doesn't (161 32nd notes in a quarter note in 24 different time signature change events??)... I would think that slot 3 only effects metronomes, so can be safely ignored (with a suitable warning saying it is always set to 24 on write-out), but I am a little suspicious that having a "number of 32nd notes per quarter note" which is not 8 will effect playback. I will investigate, at some point.

@bzamecnik
Copy link
Contributor

bach-js_kunst_der_fuge_01_(c)unknown[2].mid:

image

After writing and reading back artifact appear (very long notes).

@craffel
Copy link
Owner Author

craffel commented Jul 17, 2017

Can you post this MIDI file somewhere so we can test it out?

@bzamecnik
Copy link
Contributor

@bzamecnik
Copy link
Contributor

I created a separate issue #131 with more details about the cause.

@cifkao
Copy link

cifkao commented Feb 6, 2019

Related: #161 "Inconsistent behaviour for short (zero-duration) notes"

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

4 participants