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

Some MIDI files never finish playing, with a note lingering forever at the end, since FluidSynth v2.3.1 #1257

Closed
joanbm opened this issue Jun 25, 2023 · 11 comments · Fixed by #1264
Labels

Comments

@joanbm
Copy link

joanbm commented Jun 25, 2023

FluidSynth version

FluidSynth runtime version 2.3.3
Copyright (C) 2000-2023 Peter Hanappe and others.
Distributed under the LGPL license.
SoundFont(R) is a registered trademark of Creative Technology Ltd.

FluidSynth executable version 2.3.3
Sample type=double

(On Arch Linux)

Describe the bug

In one of the MIDI files I have: "Day of mourning.mid" by Stuart Rynn (downloadable from here), when trying to play or render the MIDI to a file using the FluidSynth command line, it never terminates playing the MIDI file. Instead, one note appears to linger at the end forever, never decreasing in intensity.

Expected behavior

I expect FluidSynth to eventually terminate playing or rendering the MIDI file.

Steps to reproduce

If I try to play that MIDI from the command line like this:

foo@bar$ fluidsynth /usr/share/soundfonts/FluidR3_GM.sf2 "Day of Mourning.mid" -v

It will play the MIDI correctly (it should be about 5 minutes long), but at the end, one note will keep playing forever, with no further events being produced.

If instead I try to render the MIDI to a WAV file like this:

foo@bar$ fluidsynth /usr/share/soundfonts/FluidR3_GM.sf2 "Day of Mourning.mid" -T wav -F DOM.wav

The WAV file will keep growing and growing (until at least 4GB in size, at which point I killed FluidSynth).

It's possible that the MIDI file has some sort of incorrect/invalid event, though I have tried to inspect the file with programs such as pw-mididump and MuseScore and all tracks have an End-Of-Track event at 326 seconds and no further events afterwards.

Additional context

I have been able to reduce the original MIDI to the following simple test case containing a single note:

OrganKeepsPlayingOnFluidSynth.mid.zip

The events in this MIDI file are:

foo@bar$ pw-mididump OrganKeepsPlayingOnFluidSynth.mid 
opened OrganKeepsPlayingOnFluidSynth.mid
track: 0 sec:0,000000 Meta: Time Signature: 4/4, 24 clocks per click, 8 notated 32nd notes per quarter note
track: 0 sec:0,000000 Meta: Key Signature: 254 sharps: Unknown major
track: 0 sec:0,000000 Meta: Tempo: 599944 microseconds per quarter note, 100,01 BPM
track: 1 sec:0,000000 Program    (channel  8): program  19 (Church Organ)
track: 1 sec:0,297628 Note On    (channel  8): note   G5, velocity  54
track: 0 sec:9,601448 Meta: End Of Track
track: 1 sec:12,148866 Meta: End Of Track

In particular, it appears that the problem is related to the instrument ("Church Organ" or other kinds of organs), changing it to for example "program 3 (Honky-Tonky)" makes the MIDI terminate.

Additionally, the problem doesn't happen in older versions of FluidSynth. I have bisected the issue to #1159 (FluidSynth ≥ v2.3.1).

I also found this PR: #1167 which appeared to solve a similar problem in the past, so maybe something similar is needed to make this case terminate.

@joanbm joanbm added the bug label Jun 25, 2023
@joanbm
Copy link
Author

joanbm commented Jun 29, 2023

So as I understand, it keeps playing forever because programs like the Church Organ usually (as defined by the SoundFont) keep producing a sound in the sustain section until the note is released. And since there is no "Note Off" event, the voice will stay active forever and the logic in #1159 will never let the MIDI stop playing.

PR #1167 deals with a similar problem, but it does not apply here since the note is not being kept on by the pedals but rather by the program definition itself.

A simple fix is to add an ALL_NOTES_OFF to the logic of PR #1167, i.e.:

diff --git a/src/midi/fluid_midi.c b/src/midi/fluid_midi.c
index c8ad9d2..f3edcb7 100644
--- a/src/midi/fluid_midi.c
+++ b/src/midi/fluid_midi.c
@@ -2201,6 +2201,7 @@ fluid_player_callback(void *data, unsigned int msec)
                     {
                         fluid_synth_cc(player->synth, i, SUSTAIN_SWITCH, 0);
                         fluid_synth_cc(player->synth, i, SOSTENUTO_SWITCH, 0);
+                        fluid_synth_cc(player->synth, i, ALL_NOTES_OFF, 0);
                     }
 
                     player->end_pedals_disabled = 1;

Though this seems a bit too aggressive, since it can also force a note in the attack/decay section to go to the release section. This can be mitigated by synth.min-note-length, but I don't think it was intended for this scenario.

So perhaps there should be a grace period before the noteoff, or a "smart noteoff" that causes the note to skip the sustain section but otherwise play the entire note.

Does that make sense?

@derselbst
Copy link
Member

Thanks for the report and for already having investigated (I didn't have any time yet).

What you describe sounds plausible and the change seems to be reasonable. I wouldn't consider this change to be "aggressive" though. That if-clause is only ever triggered when the player has processed all events of the MIDI file. Any pending notes should be turned off after this point. This is equivalent to having sent a regular note-off event before that point. I am not aware of anything like a "smart noteoff" in the SoundFont / MIDI world. That missing note-off is simply an error in the MIDI file. If it had turned off a note which is still in attack section (while not indented), one should better fix the MIDI file to add the missing off event to get the desired behavior.

Using ALL_SOUNDS_OFF would be aggressive. So, if you agree, I'll happily apply your change for the next release.

@joanbm
Copy link
Author

joanbm commented Jun 29, 2023

Maybe "agressive" wasn't the right word, but what I meant is that if you have a (broken) MIDI whose last noteon's timing matches the end of track, with my patch, that note will get immediately released, so the MIDI will end in a pretty abrupt/anticlimactic way.

E.g. with this MIDI file: three_notes.mid.zip if you play it without my patch, all three notes will sound the same, but with my patch, the last note will be much shorter.

Though this MIDI is broken due to missing noteoff events and bad end of track timing so maybe we shouldn't care too much.

OTOH, no problem with including this change in the next release :)

@felixniemeyer
Copy link

Today I observed an issue that might be related to this one.
I was converting this midi file to audio using the midi2audio python wrapper:
I_Got_You_Babe.mid.zip

When the process did not finish, I noticed the output file was already 90 GB big.
The song has a playtime of around 3 minutes.

Weirdly, when I tried it a second time, the process finished after 700MB, still 1.5h instead of the 3 minutes.

Fixing this issue by forcing an ALL_NOTES_OFF is not a good idea in my opinion. You never know, how long this note was supposed to last. And even if you wait for 10 seconds, you are behaving undefined and you're concealing the fact that there is something wrong.

I think, the best solution would be to throw an error, when a MIDI file ends with notes not being turned off. Just don't keep playing forever.

@derselbst
Copy link
Member

I think, the best solution would be to throw an error, when a MIDI file ends with notes not being turned off. Just don't keep playing forever.

Ok. But consider, that there might be multiple MIDIs files queued in fluidsynth's player. Should we really terminate fluidsynth with an error, just because there is one broken MIDI file in the queue?

What about printing a warning, sending ALL_NOTES_OFF and continuing?

When the process did not finish, I noticed the output file was already 90 GB big.

Should not happen when using fluidsynth 2.3.3

@felixniemeyer
Copy link

I'm not very familiar with the fluidsynth landscape. Is the player some kind of desktop / smartphone midi audio player? I agree then, it shouldn't terminate just because there is one corrupted midi file in the queue.

From my perspective of using fluidsynth programmatically, it would be nice if calling it with a corrupted midi file would raise an exception eventually.
I can then choose how to proceed in my program. E.g. ignoring it and printing a warning.

midi2audio calls fluidsynth like this:

subprocess.call(
            ['fluidsynth', '-ni', '-g', str(self.gain), self.sound_font, midi_file, '-F', audio_file, '-r', str(self.sample_rate)], 
            stdout=stdout, 
        )

My version of FluidSynth is 2.3.2. The file was indeed 90GB big when I terminated the process manually.

I can reproduce this issue when using FluidSynth from the command line using the file I have uploaded above.

The sound font I am using is FluidR3_GM

The full command I use:

fluidsynth -ni I_Got_You_Babe.mid ../../soundfonts/FluidR3_GM.sf2 -F out.wav  

Stopped the process after a few seconds and the file out.wav was already 600MB large.

@joanbm
Copy link
Author

joanbm commented Jul 9, 2023

Stopped the process after a few seconds and the file out.wav was already 600MB large.

With FluidSynth v2.3.3 at least it will stop after writing a few GBs of data.

@jimhen3ry
Copy link
Contributor

The FluidSynth MIDI file player is not really a robust bullet proof MIDI file player and I don't know how much effort should be put into making it deal gracefully with bad files. I suppose one could argue that a MIDI file turning on a note and leaving it on might even be intentional. In the beginning MIDI was a very dumb protocol and that dumb simplicity made it successful and long lived. Trying to make MIDI smarter can get you in trouble.

If we are going to make the MIDI file player smarter, can it match Note ON events to Note OFF events and remove Note ON events for which there is not a matching Note OFF event? This would be sort of like dealing with unmatched parenthesis. One possible complication is that multiple Note ONs for the same note can all be terminated by a single Note OFF, although I don't know if that is really a proper valid MIDI file.

@derselbst
Copy link
Member

I don't want to attempt fixing misplaced parentheses. I don't think you can anyway.

We are only facing this issue because a contributor wanted the MIDI player to respect notes that have been OFFed and are still playing in release phase at the very end of a song. Before that, the MIDI player would have just canceled any note, no matter kept ON intentionally or not. This type of broken MIDI files has never been supported. So just issuing a warning and sending ALL_NOTES_OFF at the end is good enough, IMO.

@derselbst
Copy link
Member

I just created a PR for this, see above.

@derselbst derselbst linked a pull request Aug 5, 2023 that will close this issue
@joanbm
Copy link
Author

joanbm commented Aug 5, 2023

Thanks! I built FluidSynth with the PR and ran some quick tests and everything is working fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants