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

fix #74921: implement pauses for MIDI files (hack) #3229

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

shoogle
Copy link
Contributor

@shoogle shoogle commented Jul 5, 2017

This fixes https://musescore.org/en/node/74921 but not https://musescore.org/en/node/32696.

However, my code as it currently stands introduces a new problem which does affect the bug where section breaks pauses are played on repeats. For playback or WAV/MP3 audio export the pause "belongs" to the measure before the section break, but now for MIDI export the pause "belongs" to the measure after the section break. This affects when the extra pause takes place (see new test case).

@shoogle
Copy link
Contributor Author

shoogle commented Jul 5, 2017

Changing these three lines to measureTicks - 1 makes exported MIDI equivalent to WAV:

This makes the section break pause belong to the last tick of the measure before the section break, rather than the first tick of the next measure. However, this means the pauses happen on ticks ending 9 rather than ticks ending 0, which is where tempo changes usually happen. This results in tempo changes on consecutive ticks.

Perhaps it would be better to fix the WAV export so that it considers the pause to belong to the measure after the section break.

@ericfont
Copy link
Contributor

ericfont commented Jul 5, 2017

In my mind, the section break is conceptually "between" the two measures, right in the middle of the barline. I realize that is not how it is stored in the data structure, of course. The pause should only occur when going through the two measures, so it conceptually should not occur before an end-repeat nor should it occur after at start-repeat if the section break is on the repeat barline.

@shoogle
Copy link
Contributor Author

shoogle commented Jul 5, 2017

I agree, but I don't think this is the right place to solve that issue. If there are going to be events "between" measures it needs to happen at a more fundamental point in the code.

But an easier way would simply be for each event to store a list of which repeats it should be included in. When the events are "unwound" a counter would keep track of how many repeats there have been, and events would only be included in the repeats it is supposed to be in. You could insert a tempo marking and then set a right-click property to specify it should only be included in repeats 2, 4 and 6.

If that's too much work for now then there just needs to be a playOnce boolean added to the TEvent struct. playOnce would be true for section breaks and false for breaths and other pauses.

@ericfont
Copy link
Contributor

ericfont commented Jul 5, 2017

well I think this is a good time to solve these issues together. You are adding an extra Class specifically for pause events and you are adding a extra function for unrolling the repeat list into pause events. So that is extra code, and that also means that fixing the problem of unrolling section break pauses on repeats will have to be fixed separately for export and playback. I'd much rather have one set of code.

I don't like having a counter, since the semantics of the section break is that the pause should occur when passing through the section break, since that is when the break occurs. Having user manage the count is unnecessarily complicated and may cause other problems. The playOnce boolean still doesn't fix the original problem which is figuring out when to apply the pause: If there is an end repeat at the section break, then the pause needs to occur after the second time through, while if there is a start repeat at the section break, then the pause needs to occur only before the first time through the next section.

@shoogle
Copy link
Contributor Author

shoogle commented Jul 6, 2017

I'm not suggesting the user should manage the playback of section breaks. I was saying that it would be nice to be able to manage the playback of other things like fermatas and tempo changes (e.g. play 2nd time faster, etc.). I was saying the same mechanism could be used internally to make sure section break pauses only play once, but specifically for section breaks the user wouldn't have any control over this behaviour (section breaks would be locked on 1st play only).

I was suggesting that the section break pause should happen in the bar after the section break. That way the "playOnce" boolean would solve it even if the bar after the section break is the beginning of a repeat section.

What I would really like to know is where the code is currently that says section breaks pauses happen in the measure before the section break. If I can make them happen in the measure after the section break then they become much easier to control.

@ericfont
Copy link
Contributor

ericfont commented Jul 6, 2017

Ok, if have 3.0 allow users to control behavior of elements like fermatas, tempo changes, dyanmics on a per-repeat basis, then including pauses (both caesuras and section breaks) as elements that can be controlled will nicely solve the problem...I remember that being discussed on that issue...and thinking about it now that sounds like the best way.

Regarding the playOnce boolean with pause happening in bar after the section break, I now understand what your are saying. But if I start playback at the measure with the start repeat after the sectionbreak, then that pause will undesirably be played according to how you describe (unless added some extra handling). So I think the per-repeat control is a better solution since it is a generalized feature.

Regarding "where the code is currently that says section breaks pauses happen in the measure before the section break" the answer is: the Measure class treats Line, Page, and Section Breaks similarly, and to be consistent among them, therefore understands them to mean the break is after the Measure. Therefore section breaks are stored in the measure before the section break in the measure list that makes up the Score. I don't think you should make a change to have the Section breaks be interpreted as at the beginning of the measure while Line & Page are at the end of the measure.

So anyway I have come to the realization that per-repeat control of elements is the best way forward. So I won't distract this PR with section break repeat concerns anymore.

@shoogle
Copy link
Contributor Author

shoogle commented Jul 6, 2017

That's a very good about starting playback the measure after a section break. As you say, this would either require special handling (e.g. playback cannot start on a pause), or the pause needs to belong to the final tick of the previous measure. For now I went ahead and made the changes I described above, which shifts the pause back by a single tick so it occurs in the previous measure.

This means exported MIDI files are now equivalent to exported WAV, even if it does lead to a few extra tempo changes in the MIDI file.

@shoogle
Copy link
Contributor Author

shoogle commented Jul 7, 2017

@lasconic, I've tested this on 2.2 and it should be safe to include, but minor changes to the test files are needed. I can create a PR directly into 2.2 if you like?

Also, I noticed the midi tests produce text files with note_on and note_off events but no other MIDI events like tempo changes. Should I create MIDI files as reference instead?

@lasconic
Copy link
Contributor

lasconic commented Jul 7, 2017

How would you compare MIDI files then? Maybe we can add the tempo change event to the text output?

@wschweer what do you think of this approach?

@shoogle
Copy link
Contributor Author

shoogle commented Jul 7, 2017

John Walker's public domain program midicsv (written in C) produces a good output for comparison:
midicsv testPausesRepeats.mid

0, 0, Header, 1, 1, 480
1, 0, Start_track
1, 0, Time_signature, 4, 2, 24, 8
1, 0, Key_signature, 0, "major"
1, 0, Tempo, 500000
1, 0, Control_c, 0, 121, 0
1, 0, Program_c, 0, 0
1, 0, Control_c, 0, 7, 100
1, 0, Control_c, 0, 10, 64
1, 0, Control_c, 0, 91, 0
1, 0, Control_c, 0, 93, 0
1, 0, MIDI_port, 0
1, 0, Note_on_c, 0, 60, 80
1, 455, Note_on_c, 0, 60, 0
1, 480, Note_on_c, 0, 62, 80
1, 935, Note_on_c, 0, 62, 0
1, 960, Note_on_c, 0, 64, 80
1, 1415, Note_on_c, 0, 64, 0
1, 1440, Note_on_c, 0, 65, 80
1, 1895, Note_on_c, 0, 65, 0
1, 1919, Tempo, 375000
1, 3839, Tempo, 500000
1, 3840, Time_signature, 4, 2, 24, 8
1, 3840, Tempo, 500000
1, 3840, Note_on_c, 0, 60, 80
...
1, 15336, End_track
0, 0, End_of_file

It even preserves SysEx: (select lines from MIDI file in this post)
midicsv UserSong5.MID

1, 0, SMPTE_offset, 96, 0, 0, 0, 0
1, 0, Time_signature, 4, 2, 24, 8
1, 0, Tempo, 631578
1, 0, Sequencer_specific, 6, 67, 115, 10, 0, 4, 1
1, 0, Sequencer_specific, 9, 67, 123, 0, 88, 70, 48, 50, 0, 0
1, 0, Sequencer_specific, 5, 67, 123, 12, 1, 2
1, 0, System_exclusive, 5, 126, 127, 9, 1, 247
1, 1920, System_exclusive, 8, 67, 16, 76, 0, 0, 126, 0, 247

@shoogle
Copy link
Contributor Author

shoogle commented Jul 7, 2017

(Also worth mentioning Audacity has a handy ability to import MIDI files and display them next to a waveform so you can see my MIDI output is equivalent to MuseScore's WAV export.)

@shoogle
Copy link
Contributor Author

shoogle commented Jul 19, 2017

@lasconic @wschweer any progress on this? (@wschweer, you can ignore the discussion above. All this PR does is insert an extra measure of ticks where there is supposed to be a pause in a MIDI file, and adds tempo changes to make the pause the correct length. Inserting a full measure of ticks ensures barlines occur in the same places if the MIDI file is reimported into a notation program.)

@lasconic
Copy link
Contributor

From @wschweer: "it looks ugly but i have no better solution; maybe we should merge it..."
So let's merge it :)

@lasconic lasconic merged commit ac54591 into musescore:master Jul 20, 2017
@shoogle
Copy link
Contributor Author

shoogle commented Jul 21, 2017

Thanks for merging! I agree it's ugly, but it confines the ugliness to mscore/exportmidi.cpp so we don't have to make special considerations for MIDI export elsewhere.

@shoogle shoogle deleted the 74921-midi-pauses branch June 9, 2019 20:58
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.

None yet

3 participants