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

Support Seek and other functions for fluidsynth 2+ #519

Open
TheCycoONE opened this issue May 10, 2023 · 12 comments
Open

Support Seek and other functions for fluidsynth 2+ #519

TheCycoONE opened this issue May 10, 2023 · 12 comments

Comments

@TheCycoONE
Copy link
Contributor

Fluidsynth 2 supports:
Seek: https://www.fluidsynth.org/api/group__midi__player.html#gaab5fe5b25ff513919b138d77e079c75a
Tell: https://www.fluidsynth.org/api/group__midi__player.html#gabc2d114e2a6926f4db800286b2ebb1da
Duration: https://www.fluidsynth.org/api/group__midi__player.html#gab8bec2b53e07daf2313412e4663c0fa7

When Fluidsynth 2 is used, these functions should be available.

This would fix an issue for us where Mix_PlayMusic continues from where it left off instead of the beginning of the track: CorsixTH/CorsixTH#2328

@TheCycoONE
Copy link
Contributor Author

While making these changes it might be worth hooking Play and Stop into Resume and Pause because in fluidsynth they are equivalent.

@slouken
Copy link
Collaborator

slouken commented May 10, 2023

That sounds like a good idea. Do you want to submit a PR?

@TheCycoONE
Copy link
Contributor Author

I wouldn't mind but it may be awhile before I have time. If someone else beats me to it I'm just as happy.

@joanbm
Copy link

joanbm commented Jun 3, 2023

Hmmm, there's a problem with PR #521 in that FluidSynth's ticks are not milliseconds.

For example, with this MIDI file, Mix_MusicDuration will return a false duration of 3 seconds since FluidSynth's fluid_player_get_total_ticks will return ~3000 ticks, while the actual duration is about 8 seconds long.

Some more information can be found in this issue: FluidSynth/fluidsynth#1151. The TL;DR is that FluidSynth deems reporting duration and seeking in milliseconds to be out-of-scope.

@sezero
Copy link
Contributor

sezero commented Jun 3, 2023

Well, just tried it, and playmus program reports wrong position and 0 duration for a midi file I tried. So there is definitely something wrong with the patch. And I don't know fluidsynth much to offer a solution. @TheCycoONE ?

@joanbm
Copy link

joanbm commented Jun 3, 2023

Well, just tried it, and playmus program reports wrong position and 0 duration for a midi file I tried.

The '0 duration' problem may be because FluidSynth's fluid_player_get_total_ticks returns 0 until the playback actually starts. See FluidSynth/fluidsynth#648.

A simple hack I found for this issue is that if after calling fluid_player_play, you call fluid_synth_process(synth, 1, 0, NULL, 0, NULL);, then fluid_player_get_total_ticks will return the right value. Though that's pretty much useless for us because it's measured in ticks, not milliseconds.

So far I haven' t been able to find any good way to translate ticks ↔️ milliseconds using only FluidSynth. As far as I can tell, it can be done by registering a callback using fluid_player_set_playback_callback to listen for "set tempo" events (i.e. fluid_midi_event_get_type(event) = 0x51), then processing the entire file using fluid_synth_process to get all events, but performance is not great since this triggers a lot of the logic for rendering a MIDI to an audio buffer.

For my similar use case I'm looking into using libsmf in addition to FluidSynth to deal with this problem more cleanly.
For SDL2_Mixer I think it should be possible to use/adapt the already built-in libtimidity to do it, or to try to contribute a feature upstream in FluidSynth.

@sezero
Copy link
Contributor

sezero commented Jun 3, 2023

I'm afraid it'll turn out to be way too much trouble than it's worth and we'll have to revert the patches

@slouken
Copy link
Collaborator

slouken commented Jun 3, 2023

I agree, let's revert the initial commit and this can come back as a new PR once everything is ironed out.

sezero added a commit that referenced this issue Jun 3, 2023
sezero added a commit that referenced this issue Jun 3, 2023
@sezero
Copy link
Contributor

sezero commented Jun 3, 2023

Patch reverted from both SDL2 and main branches. That reverted the FLUIDSYNTH_Pause and FLUIDSYNTH_Resume additions too: if we do really want them, one should hand-pick them and put them back.

@joanbm
Copy link

joanbm commented Jun 3, 2023

Yeah, I think that the right place to do this is in FluidSynth itself. And I don't think it's that complicated (famous last words?), since FluidSynth already has all the necessary pieces internally.

Tell can be done by exposing this property, I think.
Duration should be doable similarly to the total ticks code, but keeping track of the current tempo along the way by looking for events of type MIDI_SET_TEMPO .
Seek can also be done with the same iteration logic as Duration, but breaking after you find the first event with a timestamp >= desired time. Then you call fluid_player_seek of the corresponding ticks for that event.

@TheCycoONE
Copy link
Contributor Author

TheCycoONE commented Jun 4, 2023

The main thing I was going for was resetting the track on Play and supporting Pause/Resume.

My understanding of https://www.fluidsynth.org/api/group__sequencer.html#ga7f48dab4cde18da6e0fa76458849c19a was that ticks defaulted to ms but I admit I didn't test that aspect and reading more I see I was completely mistaken. Seek 0 on play is still useful though.

@slouken
Copy link
Collaborator

slouken commented Jun 4, 2023

The main thing I was going for was resetting the track on Play and supporting Pause/Resume.

Feel free to resubmit a PR with just those aspects implemented.

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

No branches or pull requests

4 participants