Stop/Start/Pause/Resume for Fluidsynth

[TheCycoONE]

The fluidsynth player would resume from the point when the track was stopped instead of the beginning when Play was called. This was the desired behaviour for the unsupported Pause/Resume functions, so I moved the definitions to those functions and added new Start/Stop functions that seek to the beginning of the track before playing.

I previously submitted a larger version of this patch that included seek/tell but the ticks in fluidsynth turned out to be problematic so this version only covers stop/play, which fixes the jukebox functionality in CorsixTH.

[sezero]

The previous version was https://github.com/libsdl-org/SDL_mixer/pull/521 / https://github.com/libsdl-org/SDL_mixer/issues/519

[sezero]

D:/a/SDL_mixer/SDL_mixer/src/codecs/music_fluidsynth.c:392:5: error: initialization of 'void (*)(void *)' from incompatible pointer type 'int (*)(void *)' [-Werror=incompatible-pointer-types]
  392 |     FLUIDSYNTH_Resume,
      |     ^~~~~~~~~~~~~~~~~
D:/a/SDL_mixer/SDL_mixer/src/codecs/music_fluidsynth.c:392:5: note: (near initialization for 'Mix_MusicInterface_FLUIDSYNTH.Resume')

The problem is FLUIDSYNTH.Resume must not be a int returning procedure, it must be void (previous version had the same issue.)

[TheCycoONE]

Let me know if you want that rolled up in one commit. I didn't notice any other warnings for fluidsynth in the build.

[sezero]

Let me know if you want that rolled up in one commit.

Yes please.

@slouken: Can you review? ( CC: @Wohlstand )

[Wohlstand]

Lemme take a look...

[Wohlstand]

Actually, I ... applied this thing to my side of the MixerX a while ago, but actually, I didn't used it, because my builds mostly used my custom FluidLite thing and my custom C++-coded MIDI sequencer with loop points, etc. Just now I built it with FluidSynth and I see this patch on my side being applied a while ago, and I confirm here is abug:

• I do pause - it stops
• I do resume - it REWINDS to begin, and plays from begining of the song:

I do use the system-installed Fluidsynth of the version 2.2.5 (Ubuntu 22.04).

In this demo I play the song with two synths: with FluidSynth and your patch and trying to pause/resume, and with libADLMIDI as "how it should work".

https://github.com/libsdl-org/SDL_mixer/assets/6751442/3d24c197-1912-4aad-b7a1-cec5e5e67f9f

[sezero]

I do resume - it REWINDS to begin, and plays from begining of the song:

Hmm, the patch calls fluidsynth.fluid_player_seek(music->player, 0); in FLUIDSYNTH_Stop() which is probably a mistake..

[TheCycoONE]

It isn't clear what the semantics of stop should be when paired with resume or play with pause. If you would rather not seek on stop and make it the same as pause we can.

[sezero]

As far as I can see, no other music code under codecs/ rewind to 0 in their stop procedure, so we shouldn't do that in fluidsynth either. @slouken?

[Wohlstand]

• Resume after pause should NOT rewind to 0.
• The only real STOP (i.e. completely stop the playing) should lead the rewind to 0.

[TheCycoONE]

@Wohlstand I'm not entirely clear on what you're saying, resume after pause does not seek to 0 and does not restart the track.

The fluid_player_stop does not reset, and play resumes which is why the patch was necessary to fix the current play/stop behaviour.

• https://www.fluidsynth.org/api/group__midi__player.html#gae630aa680bb891be30bffa3d6d5e1b21
• https://www.fluidsynth.org/api/group__midi__player.html#ga80cc6b4c4a81696a2d07466f3da6d6b9
• https://github.com/CorsixTH/CorsixTH/issues/2328

[sezero]

@TheCycoONE: I think @Wohlstand is referring to SDL_mixer calls

I think the patch is acceptable after removing the offending rewind call from the _STOP procedure, like:

diff --git a/src/codecs/music_fluidsynth.c b/src/codecs/music_fluidsynth.c
index bc6a78e..55ba900 100644
--- a/src/codecs/music_fluidsynth.c
+++ b/src/codecs/music_fluidsynth.c
@@ -327,9 +327,6 @@ static void FLUIDSYNTH_Stop(void *context)
 {
     FLUIDSYNTH_Music *music = (FLUIDSYNTH_Music *)context;
     fluidsynth.fluid_player_stop(music->player);
-#if (FLUIDSYNTH_VERSION_MAJOR >= 2)
-    fluidsynth.fluid_player_seek(music->player, 0);
-#endif
 }
 
 static void FLUIDSYNTH_Pause(void *context)

@Wohlstand, @slouken: This does match the rest of SDL_mixer behavior, yes?

[Wohlstand]

Okay, I debugged the thing just now, and... it actually calls the "Play" command... and...

In my case, the logic is messed up:

• The FluidSynth reports "is playing" as "false". When it PAUSED, it should report "true", but NOT the "false". The "false" must be returned when it's STOPPED, not PAUSED.
• My thing checks whatever music "is playing" to see, should it being played, paused or resumed.

[Wohlstand]

I made another solution:

1. At the structure, I added the SDL_bool is_paused;.
2. At FLUIDSYNTH_Play, FLUIDSYNTH_Resume, FLUIDSYNTH_Stop I added the music->is_paused = SDL_FALSE;.
3. At the FLUIDSYNTH_Pause I added the music->is_paused = SDL_TRUE;
4. At the FLUIDSYNTH_IsPlaying I added the condition:

 static SDL_bool FLUIDSYNTH_IsPlaying(void *context)
 {
     FLUIDSYNTH_Music *music = (FLUIDSYNTH_Music *)context;
-    return fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING ? SDL_TRUE : SDL_FALSE;
+    return music->is_paused || fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING ? SDL_TRUE : SDL_FALSE;
 }

And then, the thing works correctly.

[TheCycoONE]

Thanks. That makes sense.

In a bout of bad luck my computer died. I'll make and test the suggested changes when my replacement arrives. I am also open to either of you patching in the mean time.

[sezero]

I made another solution:

I think all those and your changes are specific to your own fork, yes?

[sezero]

I made another solution:

I think all those and your changes are specific to your own fork, yes?

Oh hell, never mind -- IsPlaying is already in music_fluidsynth...

[Wohlstand]

I made another solution:

I think all those and your changes are specific to your own fork, yes?

This change I shown is not: the music_fluidsynth.c at me is the almost same as mainstream (the difference is in a number of callback functions at the bottom). The general logic on how to manage musics, their play/pause/resume/etc. is matching to mainstream.

~~Additionally, I forgot to remove the seek/tell/duration which are invalid (they return and operate with MIDI ticks but not with a real-time). For my needs I made a separated module called music_fluidlite.c which is totally unrelated to this.~~ Just now removed that.

[Wohlstand]

Changes I did in format of compatible patch:

diff --git a/src/codecs/music_fluidsynth.c b/src/codecs/music_fluidsynth.c
@@ -137,6 +137,7 @@
     void *buffer;
     int buffer_size;
     int volume;
+    SDL_bool is_paused;
 } FLUIDSYNTH_Music;
 
 static void FLUIDSYNTH_Delete(void *context);
@@ -283,6 +284,7 @@
     fluidsynth.fluid_player_seek(music->player, 0);
 #endif
     fluidsynth.fluid_player_play(music->player);
+    music->is_paused = SDL_FALSE;
     return 0;
 }
 
@@ -290,12 +292,13 @@
 {
     FLUIDSYNTH_Music *music = (FLUIDSYNTH_Music *)context;
     fluidsynth.fluid_player_play(music->player);
+    music->is_paused = SDL_FALSE;
 }
 
 static SDL_bool FLUIDSYNTH_IsPlaying(void *context)
 {
     FLUIDSYNTH_Music *music = (FLUIDSYNTH_Music *)context;
-    return fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING ? SDL_TRUE : SDL_FALSE;
+    return music->is_paused || fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING ? SDL_TRUE : SDL_FALSE;
 }
 
 static int FLUIDSYNTH_GetSome(void *context, void *data, int bytes, SDL_bool *done)
@@ -330,12 +333,14 @@
 #if (FLUIDSYNTH_VERSION_MAJOR >= 2)
     fluidsynth.fluid_player_seek(music->player, 0);
 #endif
+    music->is_paused = SDL_FALSE;
 }
 
 static void FLUIDSYNTH_Pause(void *context)
 {
     FLUIDSYNTH_Music *music = (FLUIDSYNTH_Music *)context;
     fluidsynth.fluid_player_stop(music->player);
+    music->is_paused = SDL_TRUE;
 }
 
 static void FLUIDSYNTH_Delete(void *context)

[sezero]

About return music->is_paused || fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING ? SDL_TRUE : SDL_FALSE;

Is it not supposed to be like:

return ( !music->is_paused &&
                fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING) ?
        SDL_TRUE : SDL_FALSE;

[sezero]

Also: You keep the rewinding call in FLUIDSYNTH_Stop() :

If I remove it as I described above by removing seek-to-0 call, playmus -i xxx.mid seems to work as expected without your is_paused patch.

Am I missing something?

[Wohlstand]

About return music->is_paused || fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING ? SDL_TRUE : SDL_FALSE;

Is it not supposed to be like:

return ( !music->is_paused &&
                fluidsynth.fluid_player_get_status(music->player) == FLUID_PLAYER_PLAYING) ?
        SDL_TRUE : SDL_FALSE;

No, it's wrong: it should always return "true" when paused. In your case, it WILL return the "false" when paused, it's wrong. If it doesn't returns "true", it starts behave differently than general Mixer behaviour:

• The Mix_PlayingMusic() should return 1 when it's currently playing OR paused.
• When using Pause, the Mix_PlayingMusic() still return 1 with ANY codec.

The Mix_PlayingMusic() is being used to detect the fact music is launched and it uses the slot with no matter paused it or not. It should return 0 in only condition when the STOP is called.

[Wohlstand]

Okay, look:

• When you call the PAUSE, it just sets the state of music_active flag that suspends the streaming of music: https://github.com/libsdl-org/SDL_mixer/blob/main/src/music.c#L1311
• The state of "is music playing" remains same unchanged: https://github.com/libsdl-org/SDL_mixer/blob/main/src/music.c#L1379 (it just gets read from the music->playing field which gets set once on Mix_PlayingMusic and gets unset once the Mix_HaltMusic() gets called or when stream finishes)

[Wohlstand]

And so, the declaration of the pause call is only required for really asynchronious players like the Native MIDI or CMD music. For music that gets pulled locally, there is no necessary to specify the Pause/Resume calls. And if specify, then, do as Mixer itself does: return true when music is paused.

[Wohlstand]

Also: You keep the rewinding call in FLUIDSYNTH_Stop() :

If I remove it as I described above by removing seek-to-0 call, playmus -i xxx.mid seems to work as expected without your is_paused patch.

Am I missing something?

• FluidSynth's "stop" actually is a "pauae".
• The rewind IS needed when "stop", otherwise it's not a stop.
• The IsPlaying should report true when playing OR paused. It's important to return the true while paused to match the general behaviour of the Mixer.

[Wohlstand]

Also: calling FluidSynth's "stop" has sense in only condition when its audio output gets persistently pulled even while paused (that's needed to play the note releasing decays as MIDI sequencer gets paused, but sound generator still works persistently. But, in a case of Mixer here is no sence as Mixer totally suspends the input stream even without calling the stop of FluidSynth itself.

[sezero]

It is all contradictory with music_timidity and timidity.

But I'm confused enough, and won't bother more. Letting you and @slouken to decide and finalize this.

[TheCycoONE]

I'm available again. I don't think my say should count for a lot here but @Wohlstand 's pause patch makes sense to me.

I'll merge that and squash if no one objects.

[slouken]

Merged, thanks!