[MU3] Restrict the first exported MIDI track to timing events. #7523
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@OmarEmaraDev, your code changes look ok, however the mtest for midi failed (Test #28 tst_midi), as shown in the build log (https://github.com/musescore/MuseScore/pull/7523/checks?check_run_id=1894954476#step:5:739) with messages: I believe to fix this, you will need to update those mtest midi reference files (*-ref.mid) to have the tempo track on the first track, since presumably those reference files were made according to the old behavior of not having the first track be tempo only. |
ahh, I see you did ask that question. I would say that since your PR updates the behavior to produce different midi output files, then I would say to simply copy the resulting midi files that your PR produces into the MuseScore/mtest/libmscore/midi/ folder and rename them by appending "-ref" before the ".mid" extension. Then when we look at the diff of those midi files we should see that the only thing different is that tempo events have been moved to a tempo-only initial track. |
|
@ericfont Thanks! I updated the test files. Here is a diff for one of the test files. Looks correct to me. diff --git a/tmp/testTimeStretchFermataTempoEdit-200-ref.txt b/tmp/testTimeStretchFermataTempoEdit-page-test-init.txt
index 24e2bb9..ee0c7d0 100644
--- a/tmp/testTimeStretchFermataTempoEdit-200-ref.txt
+++ b/tmp/testTimeStretchFermataTempoEdit-page-test-init.txt
@@ -1,8 +1,18 @@
=====Track Start=====
-TrackNameEvent(deltaTime=0, name='Piano\x00')
TimeSignatureEvent(deltaTime=0, numerator=2, denominator=2, clocksPerClick=24, thirtySecondPer24Clocks=8)
-KeySignatureEvent(deltaTime=0, flatsSharps=0, majorMinor=0)
TempoEvent(deltaTime=0, tempo=300000)
+TempoEvent(deltaTime=960, tempo=1000000)
+TempoEvent(deltaTime=960, tempo=300000)
+TempoEvent(deltaTime=1920, tempo=900001)
+TempoEvent(deltaTime=479, tempo=300000)
+TempoEvent(deltaTime=1441, tempo=900001)
+TempoEvent(deltaTime=479, tempo=300000)
+TempoEvent(deltaTime=1441, tempo=1000000)
+TempoEvent(deltaTime=960, tempo=300000)
+EndOfTrackEvent(deltaTime=1)
+=====Track Start=====
+TrackNameEvent(deltaTime=0, name='Piano\x00')
+KeySignatureEvent(deltaTime=0, flatsSharps=0, majorMinor=0)
ControllerEvent(deltaTime=0, channel=0, controller=121, value=0)
ControllerEvent(deltaTime=0, channel=0, controller=100, value=0)
ControllerEvent(deltaTime=0, channel=0, controller=101, value=0)
@@ -19,9 +29,7 @@ NoteOnEvent(deltaTime=0, channel=0, note=72, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=72, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=74, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=74, velocity=0)
-TempoEvent(deltaTime=1, tempo=1000000)
-TempoEvent(deltaTime=960, tempo=300000)
-NoteOnEvent(deltaTime=0, channel=0, note=72, velocity=80)
+NoteOnEvent(deltaTime=961, channel=0, note=72, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=72, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=74, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=74, velocity=0)
@@ -29,29 +37,23 @@ NoteOnEvent(deltaTime=1, channel=0, note=69, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=69, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=71, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=71, velocity=0)
-TempoEvent(deltaTime=1, tempo=900001)
-NoteOnEvent(deltaTime=0, channel=0, note=69, velocity=80)
-TempoEvent(deltaTime=479, tempo=300000)
-NoteOffEvent(deltaTime=0, channel=0, note=69, velocity=0)
+NoteOnEvent(deltaTime=1, channel=0, note=69, velocity=80)
+NoteOffEvent(deltaTime=479, channel=0, note=69, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=71, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=71, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=65, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=65, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=67, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=67, velocity=0)
-TempoEvent(deltaTime=1, tempo=900001)
-NoteOnEvent(deltaTime=0, channel=0, note=65, velocity=80)
-TempoEvent(deltaTime=479, tempo=300000)
-NoteOffEvent(deltaTime=0, channel=0, note=65, velocity=0)
+NoteOnEvent(deltaTime=1, channel=0, note=65, velocity=80)
+NoteOffEvent(deltaTime=479, channel=0, note=65, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=67, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=67, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=60, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=60, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=62, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=62, velocity=0)
-TempoEvent(deltaTime=1, tempo=1000000)
-TempoEvent(deltaTime=960, tempo=300000)
-NoteOnEvent(deltaTime=0, channel=0, note=60, velocity=80)
+NoteOnEvent(deltaTime=961, channel=0, note=60, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=60, velocity=0)
NoteOnEvent(deltaTime=1, channel=0, note=62, velocity=80)
NoteOffEvent(deltaTime=479, channel=0, note=62, velocity=0) |
|
Thanks. Yeah looking at that diff and it seems the new midi output file correctly only differs in having the tempo events in the first channel, and the only change with other events like note on events is that the deltaTimes of the note on events are appropriately adjusted. For the record, which other midi programs have you opened these new musescore output midi files with, and do they seem to import fine? |
|
And now that you've updated the ref files, the mtest naturally is now passing, good. |
|
@ericfont I only tried QTractor and our parser and they seems to import fine. I don't have other programs installed at the moment. But I can install and test more if it is required. |
|
Well this PR looks good to me at least. If you can try importing into any
popular midi programs and let us know, that would be a good sanity check.
…On Sun, Feb 14, 2021, 7:54 AM Omar Emara ***@***.***> wrote:
@ericfont <https://github.com/ericfont> I only tried QTractor and our
parser and they seems to import fine. I don't have other programs installed
at the moment. But I can install and test more if is is required.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7523 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRTQSQNZ3FGWLL4UOLRJHTS67BZ5ANCNFSM4XSP3ISQ>
.
|
|
I tried importing and comparing the old and the new files into the following programs with the mentioned results.
In conclusion, it works everywhere I tested and is identical or better than the old files. For Timidity++ in And this for the new file: So it seems it detects the first track as a "Sequence" and not a track. I don't know what that means, but the result for the the new files seems more correct. Here is the diff for that file: diff --git a/tmp/2.txt b/tmp/1.txt
index ad159fa..f4a3df9 100644
--- a/tmp/2.txt
+++ b/tmp/1.txt
@@ -1,8 +1,10 @@
=====Track Start=====
-TrackNameEvent(deltaTime=0, name='Electric Guitar\x00')
TimeSignatureEvent(deltaTime=0, numerator=4, denominator=2, clocksPerClick=24, thirtySecondPer24Clocks=8)
-KeySignatureEvent(deltaTime=0, flatsSharps=0, majorMinor=0)
TempoEvent(deltaTime=0, tempo=500000)
+EndOfTrackEvent(deltaTime=1)
+=====Track Start=====
+TrackNameEvent(deltaTime=0, name='Electric Guitar\x00')
+KeySignatureEvent(deltaTime=0, flatsSharps=0, majorMinor=0)
ControllerEvent(deltaTime=0, channel=0, controller=121, value=0)
ControllerEvent(deltaTime=0, channel=0, controller=100, value=0)
ControllerEvent(deltaTime=0, channel=0, controller=101, value=0)I couldn't test on any proprietary software. But I think the patch is fine testing wise by now. |
|
@OmarEmaraDev, nice job! I was looking at your patch and I had a thought: what if we construct the tempo track at the beginning of |
|
@mattmcclinch I like the way you implemented that! I would maybe rename the input of the function to tempoTrack as well for clarity. |
|
Well, yes, so would I. And then I would rename |
|
@mattmcclinch Updated the patch with your suggestion. Thanks! |
|
@OmarEmaraDev, I would squash the first and third commits, since the latter basically reverts the former. I might even squash the second along with them, but definitely the first and third. |
|
@mattmcclinch I think the pull request should be squashed when merging instead. I see no value in doing it on the source branch directly. |
|
Squashing is in the responsibility of the requestor, not the merger. |
OmarEmaraDev
force-pushed
the
midi-tempo-track
branch
from
e6c6adc
to
5149e9d
Compare
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346
|
@Jojo-Schmitz Done. I am not used to this workflow, so let me know if I should have done this differently. |
mattmcclinch
approved these changes
Feb 15, 2021
|
@mattmcclinch I wrote a small MIDI parser a while back (https://github.com/OmarEmaraDev/midi-parser) and just used it to dump all events as follows: import sys
from midiparser.parser import MidiFile
midi = MidiFile.fromFile(sys.argv[1])
for track in midi.tracks:
print("=" * 5 + "Track Start" + "=" * 5)
for event in track.events:
print(event) |
|
Regarding PR commit squashing, I'm looking up the github docs and it seems GitHub has a feature to optionally automatically do that: https://docs.github.com/en/github/administering-a-repository/configuring-commit-squashing-for-pull-requests
|
|
As far as I can tell that is only for those doing the merges, not for us 'mere mortals' though |
|
Is there a reason why this haven't been merged yet? Do I need to do something? |
|
As there's no further 3.x release planned, you'd probably have to rebase/port this to the master branch, for it to be in MuseScore 4 |
|
@Jojo-Schmitz If I recall correctly, the MIDI exporter wasn't in master. You mean port the whole exporter first? |
|
No idea, sorry. |
|
You addded 15 MIDI reference files, but no tests using them? |
Jojo-Schmitz
added a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 4, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523 (I can't attribute it to @OmarEmaraDev though)
|
@Jojo-Schmitz Those are updated files to the existing ones in the tree (https://github.com/musescore/MuseScore/tree/3.x/mtest/libmscore/midi), not sure why Github shows them as new files. |
|
I see, maybe it is just a git/GitHub quirk with binary files? |
Jojo-Schmitz
added a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 4, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523 (I can't attribute it to @OmarEmaraDev though)
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 4, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
|
@Jojo-Schmitz Looks good then. Should I close this pull request? |
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 4, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
|
I guess so. You still may want to port it over to master, for inclusion in MuseScore 4 (Maybe let's see first whether #9000 passes all the mtests? Edit: they did pass) |
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 4, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 4, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 4, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 9, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 10, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 23, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 24, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 26, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Sep 29, 2021
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
May 12, 2022
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
|
3.x is closed for any changes |
Jojo-Schmitz
pushed a commit
to Jojo-Schmitz/MuseScore
that referenced
this pull request
Mar 5, 2023
Currently, exported MIDI files will have data events in the first MIDI track. While this is not forbidden by the standard, the industry convention is to only put timing related events in the first track, in which case it is called a tempo track. MuseScore should follow that convention because a lot of the available software assumes this convention. This has been discussed and agreed upon in: https://musescore.org/en/node/207346 Duplicate of musescore#7523
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: https://musescore.org/en/node/207346
Resolves: https://musescore.org/en/node/273557
Currently, exported MIDI files will have data events in the first MIDI
track. While this is not forbidden by the standard, the industry
convention is to only put timing related events in the first track, in
which case it is called a tempo track. MuseScore should follow that
convention because a lot of the available software assumes this
convention. This has been discussed and agreed upon in:
https://musescore.org/en/node/207346
This doesn't update the tests, which I guess needs to be updated. Any pointers of how I should do that?