Fix player termination after stopping and restarting

[albedozero]

If using the sample timer, reset cur_msec=0 in fluid_player_play() since the sample timer is also reset. This way fluid_player_callback() will not misinterpret the player's cur_msec < msec as being due to a fluid_synth_get_ticks overflow.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

[albedozero]

Fixes #1272 and perhaps #1270?

[albedozero]

#1236 added a check that fluid_synth_get_ticks() has not overflowed by comparing the sample timer's msec provided to fluid_player_callback() to the cur_msec of the player. However, fluid_player_play() calls fluid_sample_timer_reset(), which resets the startticks of the timer. Before, this was fine because the player callback updates cur_msec on its own, but now we need to reset the player's cur_msec as well so it's not misinterpreted as a ticks overflow.

[derselbst]

First of all, thanks for looking into this. However, I don't think this is correct. cur_msec is needed to process tempo change events correctly. We can't just reset it because the user has called e.g. fluid_player_stop(); fluid_player_play();. Yes, the fluid_player_callback restores cur_msec, but still I believe this could easily break something else that we currently don't see (we don't know because there is not a single test for the player).

I prefer a more conservative change here, which is to simply memorize the msec send by the synth in fluid_player_callback and compare against that accordingly.

#1270 doesn't use the player, so this is unrelated.

[albedozero]

Sorry it's been a while for me to come back to this. I still think cur_msec is supposed to be the thing that memorizes the last msec sent by the synth - that's why it's set in fluid_player_callback(). cur_msec is used by fluid_player_update_tempo() to set the synth-time of the tempo change, but that is based on the assumption that fluid_player_callback() has kept it updated. cur_msec isn't used anywhere else.

If we are as of #1236 also going to use cur_msec as a flag to determine whether the sample timer has overflowed, we have to reset it if we reset the sample timer. A more conservative solution would be to add an unsigned int last_msec to fluid_player_t, reset that in fluid_player_play(), and use it to check for overflow in fluid_player_callback(). I can do that if you think it's safer.

[derselbst]

A more conservative solution would be to add an unsigned int last_msec to fluid_player_t, reset that in fluid_player_play(), and use it to check for overflow in fluid_player_callback(). I can do that if you think it's safer.

Well, if you're sure that directly manipulating cur_msec is fine and doesn't break any other existing use-case, tell me and you can keep it. Otherwise, use last_msec.

I'm thinking of use-cases like seeking in the file with and without stopping / starting the player; manipulating the tempo via an external timesource and switching back to internal, etc.

[derselbst]

I changed my opinion. I once again looked through the code, checked any side-effects but couldn't find one. It should work, just as you said. I'll take this now hoping to release 2.3.4 later today.

Excuse the confusion and thanks again!

[albedozero]

Thanks! I was confident it would work, but I was trying to find time to write a player test to prove it. I will hopefully do this for an upcoming commit.