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

FmMidi: naive harmony-like loop support #2245

Merged
merged 2 commits into from
Jun 17, 2020

Conversation

tyrone-sudeium
Copy link
Member

Pretty naive implementation but follows the same MIDI message that harmony/rm2k3 uses for looping, namely the b06f event, which is a Control Change event on channel 1 with value 111. You can quickly look for that by seeing if the MIDI message is 0x6fb0, which is what that magical number check is.

Unfortunately fmmidi looks like it's the only decoder that lets me inspect the MIDI messages as they're passing through like this, which is what made this such a simple patch.

@Ghabry
Copy link
Member

Ghabry commented Jun 13, 2020

Looks fine to me. Will also work for anything else that uses the sequencer.
Maybe add a small comment that this equals event 111 on channel 1.

Do you have any midi files for testing?

Copy link
Contributor

@fdelapena fdelapena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, however there's a bug somewhere while looping, it seems to play all notes at once on restart. Here's a test:

https://easyrpg.org/play/pr2245/?game=issue-834

MIDI loop test download.

@Ghabry
Copy link
Member

Ghabry commented Jun 13, 2020

Seeking in fmmidi works by replaying all messages from the beginning until the time is reached. This makes sense because you need e.g. tempo information.

That fmmidi fires everything at once is problematic because of my external tempo tracking for the ticks. To keep this working a seek must be programmed as a play-loop ( similar to FillBuffer just without synth) or all the callbacks must be patched to also send the time.

@tyrone-sudeium tyrone-sudeium force-pushed the fmmidi-naive-loop branch 2 times, most recently from d758bbc to 9c3c933 Compare June 14, 2020 02:52
@tyrone-sudeium
Copy link
Member Author

@Ghabry I've made some fundamental changes to how this works now. I'd be interested to see what the behaviour is with regards to tempo changes that you mentioned. I used the MIDI loop test provided by @fdelapena but I added an instrument change in the middle of the looping section, so I could see whether RPG_RT reverted the instrument back to harp after the loop. It doesn't. This made me suspect RPG_RT doesn't play back all the instructions between the start and the loop point, so I've replicated that behaviour. Unfortunately that necessitates modifying midisequencer, I hope that's ok. The only alternative is to roll our own sequencer, which wouldn't be a terrible idea, but the fmmidi one is already pretty decent, IMO.

@fdelapena fdelapena added the MIDI label Jun 14, 2020
@Ghabry
Copy link
Member

Ghabry commented Jun 14, 2020

okay one test I would still like to see then in harmony is "tempo change, [...], loop-point, [...], tempo change 2, [...], loop".

Based on your tests it should continue at the speed of "tempo change 2" when the loop is hit.
Low priority imo but this would be hard to replicate in fmmidi because the time is precalculated while parsing :(. (but not impossible because you could apply a multiplier from the outside)

@Ghabry Ghabry added the Audio label Jun 14, 2020
@tyrone-sudeium
Copy link
Member Author

"tempo change, [...], loop-point, [...], tempo change 2, [...], loop".

I made this with 120BPM -> Loop Start -> 200 BPM -> Loop.

Harmony/RPG_RT plays at 120BPM until it hits 200BPM the first time, then retains 200BPM permanently for all subsequent loops.

With fmmidi in this PR, it reverts to 120BPM at the loop start, and only switches back to 200BPM after crossing the 200BPM event. So, we're not consistent with Harmony/RPG_RT. 😭

There's an argument to be made that what FmMidi is doing is more "correct", but I suppose we want to follow RPG_RT, warts and all, right? I can think of a few ways to fix it but it makes my brain hurt......

@Ghabry
Copy link
Member

Ghabry commented Jun 14, 2020

I made a quick analysis through 28404 MIDI files of mostly German games and only 1034 files use a loop.

So this is less than <4%. Don't think it is worth to support this edge case. The feature is in general seldom used and when it was used I guess they were aware of the limitations :)

@Ghabry
Copy link
Member

Ghabry commented Jun 14, 2020

After removing duplicates and only considering MIDIs with more than one tempo command only 398 files are left.
So I don't see a need in supporting this.

@fmatthew5876
Copy link
Contributor

It doesn't have to be done in this PR, but fuzzing or unit testing this midi control stuff would make a lot of sense. Especially if we can do it in a way that covers all the relevant combinations of audio libraries.

How hard would it be to generate some synthetic midi example streams in code, play all the samples instantaneously and piped to /dev/null, and test the edge cases with unit tests? Particularly a unit test on the tempo issue with a comment saying RPG_RT does x and but we do y.

@carstene1ns carstene1ns added the Awaiting Rebase Pull requests with conflicting files due to former merge label Jun 16, 2020
@tyrone-sudeium
Copy link
Member Author

As part of rebasing this I had to also make sure the looping didn't break the ticks support. @Ghabry can you please sanity check me here? I'm not completely sure I understand how the ticks work so I made some guesses

src/decoder_fmmidi.cpp Outdated Show resolved Hide resolved
@carstene1ns carstene1ns removed the Awaiting Rebase Pull requests with conflicting files due to former merge label Jun 17, 2020
@Ghabry
Copy link
Member

Ghabry commented Jun 17, 2020

@tyrone-sudeium
I rewrote my tick code. Is now stack-based. Everytime a tempo event is encountered it is pushed on a stack. When Seeking all tempo data after the seek position is discarded (yeah, not Harmony compatible, but fmmidi isn't either ;)).

Test case (csvmidi bell.csv > bell.mid):
Also contains an edge case test for "multiple tempo changes at same time as 111".

0, 0, Header, 1, 2, 120
1, 0, Start_track
1, 0, Sequencer_specific, 10, 0, 0, 91, 35, 2, 0, 3, 0, 51, 0
1, 0, MIDI_port, 0
1, 0, Tempo, 500000
1, 0, Time_signature, 4, 2, 24, 8
1, 0, End_track
2, 0, Start_track
2, 0, MIDI_port, 0
2, 0, Program_c, 0, 46
2, 360, Note_on_c, 0, 76, 60
2, 479, Note_on_c, 0, 76, 0
2, 480, Note_on_c, 0, 74, 60
2, 599, Note_on_c, 0, 74, 0
2, 600, Note_on_c, 0, 72, 60
2, 719, Note_on_c, 0, 72, 0
2, 720, Note_on_c, 0, 71, 60
2, 839, Note_on_c, 0, 71, 0
2, 840, Note_on_c, 0, 69, 60
2, 959, Note_on_c, 0, 69, 0
2, 960, Note_on_c, 0, 67, 60
2, 960, Tempo, 100000
2, 960, Control_c, 0, 111, 0
2, 960, Tempo, 200000
2, 1079, Note_on_c, 0, 67, 0
2, 1080, Note_on_c, 0, 74, 60
2, 1199, Note_on_c, 0, 74, 0
2, 1200, Note_on_c, 0, 67, 60
2, 1319, Note_on_c, 0, 67, 0
2, 1320, Note_on_c, 0, 74, 60
2, 1439, Note_on_c, 0, 74, 0
2, 1440, Note_on_c, 0, 62, 60
2, 1441, Tempo, 300000
2, 1559, Note_on_c, 0, 62, 0
2, 1560, Note_on_c, 0, 69, 60
2, 1580, Tempo, 400000
2, 1679, Note_on_c, 0, 69, 0
2, 1680, Note_on_c, 0, 71, 60
2, 1799, Note_on_c, 0, 71, 0
2, 1800, Note_on_c, 0, 69, 60
2, 1919, Note_on_c, 0, 69, 0
2, 1919, End_track
0, 0, End_of_file

@Ghabry Ghabry force-pushed the fmmidi-naive-loop branch 2 times, most recently from 3b2407e to 4f4f8d9 Compare June 17, 2020 12:14
@Ghabry
Copy link
Member

Ghabry commented Jun 17, 2020

Tested with:

  • The example above
  • Multiple tempo before loop
  • No tempo at all

They all work. The last hits the sentinel default tempo on the stack at "mtime = 0".

Loop at 0.0 hits the ".clear()" codepath.

@fmatthew5876
Copy link
Contributor

Tested with:

  • The example above
  • Multiple tempo before loop
  • No tempo at all

They all work. The last hits the sentinel default tempo on the stack at "mtime = 0".

Loop at 0.0 hits the ".clear()" codepath.

What about tempo after the loop? Can it be erasing 1 too many?

@Ghabry
Copy link
Member

Ghabry commented Jun 17, 2020

  • The example above but all tempo removed except 300000 and 400000: Only the sentinel value survives -> correct

@carstene1ns carstene1ns added this to the 0.6.3 milestone Jun 17, 2020
@Ghabry Ghabry merged commit fe6373a into EasyRPG:master Jun 17, 2020
@tyrone-sudeium tyrone-sudeium mentioned this pull request Jul 10, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants