Tempo set with fluidsynth_player_set_bpm() is reset by fluidsynth

glm35 commented 3 years ago

FluidSynth version

2.1.1

Describe the bug

I'd like to be able to play midi files at my own tempo. I'm aware of the well documented limitation that "Tempo change events contained in the MIDI file can override the specified BPM at any time". So I created midi files without "set tempo" meta message.

But I observed several similar issues:

1) when I set the tempo with fluid_player_set_bpm() before calling fluid_player_play(), the midi file is always played at 120 bpm, whatever the value I configure.

2) when I play the file in a loop: I can change the tempo if I call fluid_player_set_bpm() after fluid_player_play(). But when the file is looped, the tempo is reset to 120 bpm.

3) it is the same thing if I add several midi files to the playlist with fluid_player_add(): when the next midi file starts playing, the tempo is reset to 120 bpm.

Expected behavior

I expect that:

midi files without "set tempo" meta message are played at the tempo set with fluid_player_set_bpm()
the tempo configured with fluid_player_set_bpm() is preserved when the file is looped.
when several files without "set tempo" meta messages are added to the playlist, a single call to fluid_player_set_bpm() will configure the tempo for all the files

Additional context

Looking at fluidsynth code in fluid_midi.c: I understand that fluid_player_reset() is called just before a midi file is played or looped, and fluid_player_reset() resets the tempo to 120 bpm (player->miditempo = 500000).

Commenting out the "player->miditempo = 500000" line in fluid_player_reset() fixed the issue for me.

jjceresa commented 3 years ago

Commenting out the "player->miditempo = 500000" line in fluid_player_reset() fixed the issue for me.

This fixed your issue because the MIDI file you are using doesn't contains no tempo-change meta event. This fix doesn't works with MIDI file with tempo-change meta message because any tempo-change meta message played will override the current tempo this is a normal behaviour (not a bug).

Did you try to change the state of player.reset-synth setting to FALSE ?

For each of your 3 expectations to be possible (regardless if the MIDI file contains tempo-change messages or not), we should change the default behaviour of the player so that the fluid_player_set_bpm() API should have precedence over any potential tempo-changes events during the playing. Not sure that player.reset-synth setting will solve this.

jjceresa commented 3 years ago

Did you try to change the state of player.reset-synth setting to FALSE ?

After looking in the code "player.reset-synth" seems not related to tempo and cannot solve your issue.

jjceresa commented 3 years ago

we should change the default behaviour of the player so that the fluid_player_set_bpm() API should have precedence over any potential tempo-changes events during the playing.

fluid_player_set_bpm() API seems incomplete. This could be completed by a new function that sets the player in the mode "external sync" which means that the player is now driven by external tempo set via fluid_player_set_bpm(). In this mode internal tempo changes sourced by the MIDI file are still memorized but ignored. The same new function should allow to set the player in the mode "internal sync" (default mode) in which the player is driven by the most recent internal tempo change. Calling fluid_player_set_bpm() when the player is in mode "internal sync" should return FLUID_FAILED.

derselbst commented 3 years ago

Calling fluid_player_set_bpm() when the player is in mode "internal sync" should return FLUID_FAILED.

While this is a good idea in general, this would break the existing behaviour. How about switching to this external sync mode as soon as fluid_player_set_bpm has been called? The internal sync mode could be restored by passing -1 or 0 to fluid_player_set_bpm.

jjceresa commented 3 years ago

While this is a good idea in general, this would break the existing behaviour. How about switching to this external sync mode as soon as fluid_player_set_bpm has been called?

Using fluid_player_set_bpm() as a dual function (setting the mode + setting the external tempo value) is the simpler way for now, this seems adequate and sufficient for fluidsynth player. (If in the future, the player will be intended to use external MIDI clock, then a new API to set the player's sync mode would be probably required).

Concerning fluid_player_set_bpm() that makes the player ignoring internal tempo change by taking only external absolute tempo. It would be very useful to have also function that allows to set an external relative tempo value allowing to diminish or augment the actual internal tempo. This way all the original tempo changes sourced by the MIDI file would be still honored (instead of being ignored). This relative value parameter could be a real number (e.g, 0.5 to 2.0) multiplied by the internal tempo to produce the desired tempo.

derselbst commented 3 years ago

It would be very useful to have also function that allows to set an external relative tempo

Yes, that would be very useful indeed. @glm35 Would a relative tempo multiplier satisfy your need?

glm35 commented 3 years ago

First, I'd like to say that I'm quite happy to see that the discussion is going beyond the initial scope of my bug report. I would find it very useful to be able to override the "set tempo" meta messages found in the midi files.

Back to the tempo multiplier idea: for my use case, which is about playing playback tracks for musical practice, it would be simpler to just set the desired tempo in bpm. Else I would need to get the tempo of the tune (if any) and compute the multiplier factor. Also, I would like to be able to queue several tunes and to play them seamlessly with the same tempo, regardless the original tempo of the tunes. If the different tunes have different tempos, the multiplier will not allow me to reach that goal.

But later on in my project, I might wish to be able to play tunes with tempo changes in them. In that case, a relative tempo multiplier would be necessary.

So I find both approaches useful and complementary. A new function allowing to set a relative tempo multiplier could coexist with the existing one to set an aboslute tempo. But being able to set an absolute tempo is more important for me now.

jjceresa commented 3 years ago

Back to the tempo multiplier idea: for my use case, which is about playing playback tracks for musical practice, it would be simpler to just set the desired tempo in bpm.

Yes, of course having a well know steady tempo during a whole section for musical practice is typical and usually a musician need to remember this value (in bpm) as a repair to evaluate future practice progress.

So I find both approaches useful and complementary. A new function allowing to set a relative tempo multiplier could coexist with the existing one to set an aboslute tempo.But being able to set an absolute tempo is more important for me now.

@derselbst In fact, perhaps we don't need to add a specific function to be able to set a relative tempo. I mean, we could use the same function fluid_player_set_bpm(player, int tempo_type, double tempo) with one more parameter (tempo_type) that tells to the player that the tempo value is a "absolute tempo" or a "tempo multiplier". As actual fluid_player_set_bpm() is a bit "broken by design", is it possible to change fluid_player_set_bpm() signature without requiring a new version major number ?

derselbst commented 3 years ago

is it possible to change fluid_player_set_bpm() signature without requiring a new version major number ?

No. And although we'll be having an SOVERSION bump in 2.2.0, I'm not willing to introduce that kind of API breakage. We will need a new function. My straightforward proposal would be fluid_player_set_bpm2().

jjceresa commented 3 years ago

No. And although we'll be having an SOVERSION bump in 2.2.0, I'm not willing to introduce that kind of API breakage.

Ok, I understand your point.

We will need a new function. My straightforward proposal would be fluid_player_set_bpm2().

This name should be fine.

glm35 commented 3 years ago

What about a more generic function name that would cover all the existing usages, the new ones we are discussing, and allow possible extensions in the future?

Something like:

fluid_player_set_tempo(fluid_player_t *player, enum tempo_type, double tempo)

with:

enum fluid_tempo_type {
    FLUID_TEMPO_DEFAULT,   // Use midi file tempo or 120bpm if tempo is not set in midi file
    FLUID_TEMPO_BPM,       // Set player tempo in bpm, override midi file tempo
    FLUID_TEMPO_MIDI,      // Set player tempo in us per quarter note, override midi file tempo
    FLUID_TEMPO_RELATIVE   // Speed up or slow down tempo with a multiplying factor
}

fluid_player_set_bpm() would be the same as fluid_player_set_tempo() with tempo_type=FLUID_TEMPO_BPM

fluid_player_set_midi_tempo() would be the same as fluid_player_set_tempo() with tempo_type=FLUID_TEMPO_MIDI

Calling fluid_player_set_tempo() with FLUID_TEMPO_DEFAULT would cancel the previous calls to fluid_player_set_tempo() and restore the default behaviour.

derselbst commented 3 years ago

Yes, I like that proposal.

jjceresa commented 3 years ago

What about a more generic function name that would cover all the existing usages

Fine proposal.

mawe42 commented 3 years ago

Great idea!

jjceresa commented 3 years ago

It worth to note that this bug report applies to fluid_player_set_tempo() and fluid_player_set_midi_tempo() as well. Also, both functions suffer of multi task race condition when computing player->deltatime which is another bug.

glm35 commented 3 years ago

Glad to see you like the idea!

Do you have some short term plans to work on this? Else I can try to do it...

derselbst commented 3 years ago

I may find some time over the Christmas holidays. But there are also a couple of other tasks for fluidsynth waiting to be done. So, you are very welcome to give it a try. If you need any support, just let us know.

jjceresa commented 3 years ago

Do you have some short term plans to work on this? Else I can try to do it...

1)This could be done in a relative short term by first solving the bug report that applies to both fluid_player_set_tempo() and fluid_player_set_midi_tempo(). This will fix these functions API without breaking the current API and will facilitate greatly implementation of step 2. To get this result we need just to apply Tom's suggestion, that is:

a tempo parameter value > 0, instructs the player to switch using only this external tempo value.
a tempo parameter value 0, instructs the player to switch back using most recent internal tempo event(s) provided by the MIDI file. Very small internal player code need to be changed to get this result (also this should fix the multi-tasking race condition pointed in previous comment).

2)After step (1), we could add implementation of the new function fluid_player_set_tempo().

If you want, I think I can do step 1 in a branch named fix-player-tempo, for the next 3, 4 days...week. This will require test using your MIDI file set (in particular these without tempo changes). Please, may I asking you to contribute doing the required tests steps ?

derselbst commented 3 years ago

Do we really want to change the behaviour of those old functions? If we introduce the new function fluid_player_set_tempo() it would also mean to deprecate the old functions fluid_player_set_midi_tempo() and fluid_player_set_bpm(). And it seems not right to me to change their behaviour just before deprecating them.

mawe42 commented 3 years ago

And it seems not right to me to change their behaviour just before deprecating them.

I thought the old functions would simply be implemented internally via fluid_player_set_midi_tempo() but retain their old behaviour. But if their behaviour would actually change, then I agree that leaving them as-is and simply deprecating them would be better.

jjceresa commented 3 years ago

And it seems not right to me to change their behaviour just before deprecating them.

But if their behaviour would actually change, then I agree that leaving them as-is and simply deprecating them would be better.

Ofc, this leads to deprecate old functions, however the fastest way to get the final result is to split the changes in the 2 steps described above because implementation of the new function (step 2) will call directly a new static function (fluid_player_update_tempo()) already implemented in step 1 without requiring step1's behaviour changes. These 2 steps could be part of the same PR. @glm35 your help is welcome to check result of step1 before continuing.

jjceresa commented 3 years ago

Actually the player is accessible from fluidsynth application console. This allow to play MIDI file(s) or render MIDI to audio file. What do you think about adding a realtime setting player.tempo-bmp ?. This should allow user to set their own tempo. Also, this tempo could be adjusted using the set command line while the MIDI file is playing.

jjceresa commented 3 years ago

Do we really want to change the behaviour of those old functions? If we introduce the new function fluid_player_set_tempo() it would also mean to deprecate the old functions fluid_player_set_midi_tempo() and fluid_player_set_bpm(). And it seems not right to me to change their behaviour just before deprecating them.

You are right, please ignore my comment about this. So the steps should be: 1) fix the bug report by introducing the new function fluid_player_set_tempo(). 2) then deprecate older functions fluid_player_set_midi_tempo() and fluid_player_set_bpm().

glm35 commented 3 years ago

@jjceresa sure I can help with the testing. I'll start to use the fix as soon as it's ready.

In case you're interested to "improve" your midi file test database, I'm also attaching the midi file without tempo I created to work on the issue: c_scale_no_tempo.zip. It's just two bars in 4/4 with a C major scale ascending and descending. Zipped because github does not accept .mid files.

jjceresa commented 3 years ago

@jjceresa sure I can help with the testing. I'll start to use the fix as soon as it's ready.

Ok, Thanks you.

derselbst commented 3 years ago

What do you think about adding a realtime setting player.tempo-bmp ?

No, that doesn't fit.

Also, this tempo could be adjusted using the set command line while the MIDI file is playing.

That's better.

jjceresa commented 3 years ago

In case you're interested to "improve" your midi file test database, I'm also attaching the midi file without tempo I created to work on the issue: c_scale_no_tempo.zip. It's just two bars in 4/4 with a C major scale ascending and descending.

Thanks you. c_scale_no_tempo.mid is fine. This is a 4 bars (bars 1,2 ascending and bars 3,4 descending). In the meanwhile another midi file with 2 tempo changes would be also useful. For example, c_scale_60-120bpm.mid with 2 tempo changes: 60 bpm at bar 1 and 120 bpm at bar 3.

jjceresa commented 3 years ago

What do you think about adding a realtime setting player.tempo-bmp ? No, that doesn't fit.

With the help of real time setting player.tempo-bmp the user can set the initial tempo value before starting the playing and than change the setting value with the set command during the playing if needed. Why setting player.tempo-bmp doesn't fit ?.

derselbst commented 3 years ago

The new function fluid_player_set_tempo supports various use-cases. If you want player.tempo-bpm, you'll also need player.midi-tempo and player.relative-tempo. This would be redundant. Let's first concentrate on getting the new API function working. Then, we can take care about a shell command for it (currently the midi player doesn't have any shell commands btw.). If one wants to set an initial tempo, just use a command file for shell that contains the desired tempo command.

jjceresa commented 3 years ago

Let's first concentrate on getting the new API function working. Then, we can take care about a shell command for it (currently the midi player doesn't have any shell commands btw.).

Ok.

Another point. The introduction of the new function fluid_player_set_tempo requires to change actual behaviour of fluid_player_get_bpm(), fluid_player_get_midi_tempo(). So for consistency it seems that a new function fluid_player_get_tempo(fluid_player_t player, enum tempo_type, double tempo) is also required and actual fluid_player_get_bpm(), fluid_player_get_midi_tempo() should deprecated as well. Opinion ?

derselbst commented 3 years ago

a new function fluid_player_get_tempo(fluid_player_t player, enum tempo_type, double tempo) is also required and actual fluid_player_get_bpm(), fluid_player_get_midi_tempo() should deprecated as well.

Ok.

glm35 commented 3 years ago

@jjceresa I uploaded a new zip with 3 small midi test files: c_scale_midi_samples.zip

All the samples have 4 bars of C scale as you noticed (bars 1,2 ascending and bars 3,4 descending).

c_scale_no_tempo.mid: midi file without "set tempo"
c_scale_90_bpm.mid: tempo set to 90 bpm from the beginning
c_scale_90_180_bpm.mid: tempo set to 90 bpm for the first two bars and 180 bpm for the last two bars

Remark: I prefer not to use 120 bpm in the test files because it is the default tempo set by fluidsynth when no tempo is defined

jjceresa commented 3 years ago

I uploaded a new zip with 3 small midi test files.

Many thanks !

FluidSynth / fluidsynth