Fix MIDI player tempo reset issues

[jjceresa]

This PR addresses issue #707 according to the last discussion.
@glm35, please, your are welcome to pull this branch fix-player-tempo an test the new function fluid_player_set_tempo(), and fluid_player_get_tempo().

[jjceresa]

Please, as I am not familiar with doxygen formatting rule, could you have a check , in particular for fluid_player_tempo_type enum in midi.h file ?.

[derselbst]

Looks good to me. I'll keep this open for a few days to give @glm35 a chance to test this.

[jjceresa]

Looks good to me....

Ok. Thanks for reviewing and fixing API docs, and grammar.

[jjceresa]

Could you precise that if no tempo is given in the midi file, a default value of 120 bpm will be used?

Yes, when no tempo change is given in a midi file, a default value of 120 bpm should be used. This is a MIDI file specification.
It will be useful to add this mention in the API comment.

---

60000000 bpm, really?

Yes, internally valid tempo value should be in the range [1..600000000] regardless if the value is in bpm or in us/quarter note.

[jjceresa]

But I think that it might be enough to have a simpler API that would just return the current tempo used by the player in the desired format.

This is done in recent commit 0836a52 . One just need to call fluid_player_get_tempo() with FLUID_PLAYER_GET_TEMPO_USED_BPM or FLUID_PLAYER_GET_TEMPO_USED_MIDI.

[derselbst]

First of all, I'm glad to see that everything is working as expected. Thanks for testing @glm35!

Regarding the get_tempo() function: I have a similar feeling as @glm35: It is too overengineered. A client app won't need this. The client app knows whether it has set an absolute tempo or a relative one. Splitting up the enum into a fluid_player_get_tempo_type and fluid_player_set_tempo_type doesn't really make this simpler. Thinking about it I would rather say to drop fluid_player_get_tempo() entirely, due to a missing use case at the present. Instead, the existing functions fluid_player_get_bpm() and fluid_player_get_midi_tempo() should be continued to use. Those functions obtain the tempo the player is using right now. This might be

• the tempo given by the MIDI file multiplied by the relative factor, or
• the absolute tempo dictated by the client app.

[jjceresa]

Thinking about it I would rather say to drop fluid_player_get_tempo() entirely, due to a missing use case at the present. Instead, the existing functions fluid_player_get_bpm() and fluid_player_get_midi_tempo() should be continued to use. Those functions obtain the tempo the player is using right now.

Ok.

[jjceresa]

Now, the tempo multiplier factor set by fluid_player_set_tempo(..,FLUID_PLAYER_TEMPO_RELATIVE, multiplier) is only used when the player follow the internal tempo changes. So it seems that we should do the following changes:
1)pass this multiplier value in the tempo parameter when calling fluid_player_set_tempo(..,FLUID_PLAYER_TEMPO_INTERNAL, multiplier)
2)Get rid of FLUID_PLAYER_TEMPO_RELATIVE enum value.

These simplifications allows the caller to set the player in internal tempo mode and supplying the multiplier factor using only one API function call.
Opinions ?

[derselbst]

Now, the tempo multiplier factor set by fluid_player_set_tempo(..,FLUID_PLAYER_TEMPO_RELATIVE, multiplier) is only used when the player follow the internal tempo changes. So it seems that we should do the following changes:

So, rather than ignoring the tempo parameter with FLUID_PLAYER_TEMPO_INTERNAL, we use it to pass the multiplier directly. Yes, that sounds good to me.

[derselbst]

Looks good now. I'll wait for the CI and then squash it. Thanks!

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication