Add different time units to MidiFile #115

superbock · 2017-08-20T08:37:52Z

Note: This PR adds the enhancements mentioned in #102 to MidiFile. It is based on PR #114, i.e. it includes all commits of #114. Needs to be rebased when this PR is merged.

Content

Add different time units and relative/absolute timimg capability to MidiFile.

MidiFile.unit can hold a string indicating which unit should be used to display event timings. Possible values are ticks, seconds, beats and various shortcuts thereof (t, s, sec, and b).

Furthermore the timings can be viewed as absolute values or relative to the preceeding event as indicated by MidiFile.timing. This attribute can also hold a string, possible values are absolute (shortcuts: a, abs) or relative (shortcuts: r, rel).

To support timing information in beats, time signature event handling was added to MidiFile.__iter__ and the needed unit conversion functions to midifiles.units.

Minor modifications

These modifications are split into separate commits which can be skipped if wanted:

moved constants to midifiles.units,
remove the obsolete get_seconds_per_tick function,
add default arguments to unit conversion functions.

Before being able to be merged, these points should be discussed/addressed:

Rebase if Fix BPM <-> MIDI tempo conversions #114 is merged.
Extend the documentation to reflect the changes in MidiFile.
Import the new tick2beat and beat2tick into the main namespace?
Include tick2beat and beat2tick in the API docs.
Add docstrings, but which docstring format to use?
ticks_per_beat is a bit misleading since it rather should be ticks_per_quarter_note. Renaming the variable, however, may break existing code, so this is a rather bad option. Maybe adding a new variable (and deprecate the old one, but keep it around for backwards compatibility) is a better approach.
Tests for Python 2 fail at the moment since Python 2 and 3 round differently. This was revealed by adding tests for corner cases.
Extend the documentation to reflect all introduced changes, especially update the image in midi_files.rst.

EDIT: merged the TODO items of #114 into this PR, since they also apply here.

superbock · 2017-09-26T06:50:06Z

Comments welcome, thus ping @olemb.

cjappl · 2017-09-26T16:33:56Z

I cast my vote for numpydoc. It hasn't failed me yet!

cjappl · 2017-09-26T16:36:00Z

Did you run the tox tests?

You can do so by getting the latest revision then:

pip install -e .[dev]
tox

The first one is only necessary to make sure you have all the dev packages installed before you run tox.

cjappl · 2017-09-26T16:36:14Z

also +1 from me for adding a bunch of unit tests, that's great!!

superbock · 2017-09-27T12:19:23Z

Of course I ran the tests, but as said in #114 (and copied into this PR's TODO items) some tests fails due to different rounding in Python 2/3. My question was on what functionality is wanted. Either the rounding could be unified or the tests could be edited to check for the current Python version and test accordingly.

Either way it would be extremely helpful for all if there were some sort of continuous integration (e.g. TravisCI).

But I also wanted to get a more general feedback on this PR. Is this functionality wanted in mido? If yes, what changes to the PR are needed. If no I add them to the classes inherited from mido in my own projects. Then, still #114 should be merged, since it contains bug fixes.

carlthome · 2017-10-02T08:43:14Z

Either way it would be extremely helpful for all if there were some sort of continuous integration (e.g. TravisCI).

#129

Jimmusic

I copied the changed files into Mido. units and midifiles. The result was that every Midi file played in extreme slow motion.

superbock · 2017-12-18T10:50:28Z

Interesting. Can you explain how you checked this in order to repeat (and fix) it. Furthermore it would be interesting which commit adds this slowing down. Is #114 affected as well?

superbock · 2017-12-18T11:24:38Z

I guess I figured it out: I introduced a timing parameter which must be 'relative' instead of 'absolute' since MIDI messages have relative timing. Should be fixed now. Thanks for pointing this out!

However, this reminds me on how difficult it is to change some code without proper tests. I can write a test for this but it requires a MIDI file to be tested. Are there any suggestions on which properties such a file should have or does something like a "standard" MIDI test file exist?

Jimmusic · 2017-12-18T22:40:15Z

Many thanks for that I will give it a try. Original problem is difficult to explain but when playing a song that begins with a slow tempo it takes ages for the first note to play. In the case of the attached file about 11 secs but on a daw it starts instantly. I wondered if it was something to do with temp and time sig so tried your changes. I also think that it might be a delta time calc of some sort that is calculating the time to next message incorrectly when the first note is a rest.
ffcommon.zip

Jimmusic · 2017-12-18T23:21:25Z

The latest changes fixed the slow motion - Great work. There still remains the problem of a long time to get to the first note when playing the example file as opposed to the short wait when playing in a DAW.
This problem is in the original mido so not related to your enhancement. I will report it there.
This is not related to the iter and merge_tracks. it is after that.
By the way if it is any use: The above iter and for msg in merge_tracks(self.tracks) does not work well when looping a drum loop so I changed the class to init to include
self.mergeset = []
and the last line of def _load to have
self.mergeset = merge_tracks(self.tracks)
and the iter to have
for msg in self.mergeset
Loops now work perfectly

I will print out the time delta for each message to try and find out why it takes so long to get to the first note of the above file and similar slow tempo midi files starting with rests.

Again many thanks
PS what do your changes let me do ? Can I find out what beat I am on ? That would be great.

Regards
Jim

superbock · 2017-12-19T11:52:03Z

The long delay at the beginning is unrelated to the changes in this PR. Caching the merged tracks can indeed solve the problem -- or at least lower its extend. I'll look into it and incorporate this suggestion as well, possibly in a separate PR.

The unit parameter enables you to change the time units to be either in seconds (default), ticks (MIDI internal timing) or beats. So the time attribute of the individual messages are measured in the selected unit. Furthermore, with the timing parameter you can choose if you want the timings to be absolut or relative to the preceding message.

Jimmusic · 2017-12-19T19:34:48Z

Caching the merged tracks only solves the problem of creating loops such as drum loops where they have to be seamless. The problem I am experiencing with long time to first note when a midi file starts that has quite a lot of control changes and a slow tempo and or rests at beginning of song is that the first note on message seems to have a very large time Delta. In the file I uploaded the first one has 12 secs. Most of proceeding channel messages have 0 time Delta but some get 3secs. Saw shows them all as 0. The odd deltas seem to follow where but I only had a brief look. Will try to describe better later. This is not isolated to this midi.

Jimmusic · 2017-12-19T19:36:07Z

I missed out seems to follow sysex messages

carlthome · 2018-02-19T10:34:30Z

Status?

superbock · 2018-03-01T09:55:40Z

I am not sure what the status of this PR is. I am still waiting for any kind of response to the questions I raised.

carlthome

I think this change looks great. Very nicely done! I hope it could be merged. Particularly the unit argument to MidiFile would be useful.

Just some minor nitpicks.

carlthome · 2018-03-04T11:39:34Z

docs/midi_files.rst

-for short) have to be decided upon.
+To convert from MIDI time to absolute time in seconds, the tempo (either
+in number of beats per minute (BPM) or microseconds per quarter note, see
+above) and ticks per per quarter note have to be decided upon.


see above should be a reST hypertext reference to the relevant section instead.

carlthome · 2018-03-04T11:40:23Z

mido/midifiles/midifiles.py

 class MidiFile(object):
 def __init__(self, filename=None, file=None,
 type=1, ticks_per_beat=DEFAULT_TICKS_PER_BEAT,
- charset='latin1',
+ unit='seconds', timing='relative', charset='latin1',


Will this break backwards compatibility or were these defaults implicit before (I think yes, just wanna double check)?

Yes, these were implicit before, so this change shouldn't break anything. There was a bug in a previous version which changed the default behaviour, but this is fixed (see comment above). However, I can't guarantee this because there are no tests covering this aspect.

carlthome · 2018-03-04T11:45:06Z

mido/midifiles/midifiles.py

+ delta = tick2beat(msg.time, self.ticks_per_beat,
+ time_signature)
+ else:
+ raise ValueError("`unit` must be either 'ticks', 't', "


The shorthands should be removed ('t', 's', 'b' are not obvious) as they provide very little benefit at a big cost. MidiFile(unit='beats') is a lot cleaner and easier to search in the docs after.

I.e.

if self.unit.lower() == 'ticks':

is better.

IMHO the supported units should be defined by an Enum with sensible names.
This would make the code clearer and simplify types verification.

carlthome · 2018-03-04T11:46:36Z

mido/midifiles/midifiles.py

 else:
 delta = 0
+ # Convert relative time to absolute values if needed
+ if self.timing.lower() in ('a', 'abs', 'absolute'):


Same here. Shorthands are harder to search after on StackOverflow etc.

Again, an Enum would solve this.
No need to check for multiple strings variations.

carlthome · 2018-03-04T11:47:59Z

mido/midifiles/units.py

 """
+ # Note: both tempo (microseconds) and ticks are per quarter note


Remove comment.

carlthome · 2018-03-04T11:48:03Z

mido/midifiles/units.py

 """
+ # Note: both tempo (microseconds) and ticks are per quarter note


Remove comment.

carlthome · 2018-03-04T11:52:47Z

mido/midifiles/units.py

 scale = tempo * 1e-6 / ticks_per_beat
 return tick * scale


-def second2tick(second, ticks_per_beat, tempo):
+def second2tick(second, ticks_per_beat=DEFAULT_TICKS_PER_BEAT,


"Convert absolute time in seconds to ticks." > "Round absolute time in seconds to ticks." IMO, or something of that sort to signal that the conversion is lossy.

…idiFile. The unit (MidiFile.unit) holds a string indicating which unit to use to display event timings. Possible values are ticks, seconds and beats. Furthermore the timings can be viewed as absolute values or relative to the preceeding event as indicated by MidiFile.timing. To support timing information in beats, time signature event handling was added to MidiFile.__iter__.

… note to units.py

andresbrocco · 2021-01-31T18:27:07Z

Any advancements?! I can't wait for this improvement

superbock · 2021-02-18T20:47:11Z

Unfortunately, I haven't heard back from the developers after I made some changes to the PR. I will update the code and rebase on the current master branch.

Closes mido#115

This was referenced Aug 20, 2017

Classes need their parameters documented #111

Open

possible unit conversion bug #102

Closed

superbock force-pushed the add_different_time_units branch from a75bff9 to bc2988c Compare August 23, 2017 08:52

superbock mentioned this pull request Sep 1, 2017

Add defaults to second2tick and tick2second #119

Closed

superbock force-pushed the add_different_time_units branch from bc2988c to a3532bc Compare September 21, 2017 16:01

superbock force-pushed the add_different_time_units branch from a3532bc to 658eb6b Compare September 27, 2017 12:27

superbock mentioned this pull request Oct 16, 2017

[WIP] replace old MIDI code in utils.midi with mido CPJKU/madmom#308

Closed

2 tasks

superbock force-pushed the add_different_time_units branch 2 times, most recently from 274d411 to 37e920b Compare December 14, 2017 15:38

Jimmusic reviewed Dec 17, 2017

View reviewed changes

superbock force-pushed the add_different_time_units branch from 37e920b to 3b989cf Compare December 18, 2017 11:16

carlthome approved these changes Mar 4, 2018

View reviewed changes

Sebastian Böck added 3 commits March 4, 2018 14:52

consider time signature when convertig between BPM and MIDI tempo

19651b7

second2tick() returns integer values only

477d553

clarify docstring for tick2second() and tick2second()

d38a848

Sebastian Böck added 5 commits March 4, 2018 15:31

update docs to include time signature relevance for event timings

c086918

move default constants for tempo, time signatre and ticks per quarter…

02dbe04

… note to units.py

remove obsolete get_seconds_per_tick() function

2f796b8

add default arguments to unit conversion functions

f03c898

superbock force-pushed the add_different_time_units branch from 3b989cf to f03c898 Compare March 4, 2018 14:36

rdoursenaud added the feature:midifile Standard MIDI File (SMF) implementation label Jan 19, 2023

rdoursenaud added this to the Version 2.0.0 milestone Jan 19, 2023

rdoursenaud self-assigned this Jan 29, 2023

rdoursenaud modified the milestones: Version 2.0.0, Future Jan 29, 2023

rdoursenaud added a commit to rdoursenaud/mido that referenced this pull request Jan 29, 2023

[WIP] Add different time units to MidiFile

c7e015b

Closes mido#115

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add different time units to MidiFile #115

Add different time units to MidiFile #115

superbock commented Aug 20, 2017 •

edited

Loading

superbock commented Sep 26, 2017

cjappl commented Sep 26, 2017

cjappl commented Sep 26, 2017

cjappl commented Sep 26, 2017

superbock commented Sep 27, 2017

carlthome commented Oct 2, 2017

Jimmusic left a comment

superbock commented Dec 18, 2017

superbock commented Dec 18, 2017

Jimmusic commented Dec 18, 2017

Jimmusic commented Dec 18, 2017

superbock commented Dec 19, 2017 •

edited

Loading

Jimmusic commented Dec 19, 2017

Jimmusic commented Dec 19, 2017

carlthome commented Feb 19, 2018

superbock commented Mar 1, 2018

carlthome left a comment

carlthome Mar 4, 2018

carlthome Mar 4, 2018

superbock Mar 4, 2018

carlthome Mar 4, 2018

rdoursenaud Jan 19, 2023

carlthome Mar 4, 2018 •

edited

Loading

rdoursenaud Jan 19, 2023

carlthome Mar 4, 2018

carlthome Mar 4, 2018

carlthome Mar 4, 2018 •

edited

Loading

andresbrocco commented Jan 31, 2021

superbock commented Feb 18, 2021

		"""
		# Note: both tempo (microseconds) and ticks are per quarter note

Add different time units to MidiFile #115

Are you sure you want to change the base?

Add different time units to MidiFile #115

Conversation

superbock commented Aug 20, 2017 • edited Loading

Content

Minor modifications

superbock commented Sep 26, 2017

cjappl commented Sep 26, 2017

cjappl commented Sep 26, 2017

cjappl commented Sep 26, 2017

superbock commented Sep 27, 2017

carlthome commented Oct 2, 2017

Jimmusic left a comment

Choose a reason for hiding this comment

superbock commented Dec 18, 2017

superbock commented Dec 18, 2017

Jimmusic commented Dec 18, 2017

Jimmusic commented Dec 18, 2017

superbock commented Dec 19, 2017 • edited Loading

Jimmusic commented Dec 19, 2017

Jimmusic commented Dec 19, 2017

carlthome commented Feb 19, 2018

superbock commented Mar 1, 2018

carlthome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlthome Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlthome Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

andresbrocco commented Jan 31, 2021

superbock commented Feb 18, 2021

superbock commented Aug 20, 2017 •

edited

Loading

superbock commented Dec 19, 2017 •

edited

Loading

carlthome Mar 4, 2018 •

edited

Loading

carlthome Mar 4, 2018 •

edited

Loading