| Summary: | Mix_PauseMusic and Mix_ResumeMusic not working on MIDI (.mid) | ||
|---|---|---|---|
| Product: | SDL_mixer | Reporter: | Anton <anton.synytsia> |
| Component: | misc | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | normal | ||
| Priority: | P2 | CC: | philipp.wiesemann, vikas.ag |
| Version: | 2.0.0 | ||
| Hardware: | x86_64 | ||
| OS: | Windows 7 | ||
| Attachments: |
Patch for pause and resume music with Windows native MIDI decoder
Patch for pause and resume music with Windows native MIDI decoder, 2nd version |
||
|
Description
Anton
2015-03-15 08:44:35 UTC
Any solution for this? No, do you have a proposed patch? Created attachment 2245 [details]
Patch for pause and resume music with Windows native MIDI decoder
This is a patch to make Mix_PauseMusic() and Mix_ResumeMusic() work with the Windows native MIDI decoder.
I only tested it with "playmus.c" and it may break something else.
Also, there should be a better and less fragile way to implement this.
I think there can be a better way
In proposed scenario:
1. If Mixer is built with MID support, then for every music format mix_music(music_mixer) will be called(unnecessarily) even music is in pause state. However there are no side effect but since this method will be called so many time, we should avoid this call, if possible.
2. If MID format is being played, then native_midi_resume will be called(Unnecessarily) for each music chunk.
Solution:
I think it will be better If We call native_midi_resume from Mix_ResumeMusic
void Mix_ResumeMusic(void)
{
music_active = 1;
#ifdef MID_MUSIC
# ifdef USE_NATIVE_MIDI
// Native midi is handled asynchronously, so it needs to be resumed separately
if (native_midi_ok && native_midi_active())
{
native_midi_resume();
}
# endif
#endif
}
Similarly We can make changes for Mix_PauseMusic
Also We need to make some changes in native_midi_active to avoid crash
int native_midi_active()
{
return currentsong && currentsong->MusicPlaying;
}
The variable music_active is also changed in Mix_FadeInMusicPos(). If the functionality to pause/resume MIDI would be in Mix_PauseMusic() and Mix_ResumeMusic() only then this would have no effect. 1. We can play music using Mix_PlayMusic/Mix_FadeInMusic/Mix_FadeInMusicPos but ultimately Mix_FadeInMusicPoswill be called in these cases. If this method is not called then there is no music to pause/resume.
2. Now music_active is being set to 1 in Mix_FadeInMusicPos. This is initialization of this variable, if this is not set to 1, music will not even start. If music does not gets started, there is nothing to pause.
check this code snippet
void music_mixer(void *udata, Uint8 *stream, int len)
{
int left = 0;
if ( music_playing && music_active ) {
/* Logic to play next chunk of music for every format except mid, because mid is being played in a seperate thread */
}
}
Now by default also music_active is 1, consider the following call sequence
Mix_PlayMusic(music, -1); /* music_active = 1 */
Mix_PauseMusic(); /* music_active = 0 */
Mix_HaltMusic(); /* music_active still 0 */
Mix_PlayMusic(music, -1); /* Since value is 0, it must be set to 1, so that next chunk of music gets played */
3. As for as mid format is concerned, it is not much dependent on music_active. native_midi_start gets called irrespective of value of music_active. Once this method gets called, it will be played in seperate thread.
void music_mixer(void *udata, Uint8 *stream, int len)
{
int left = 0;
if ( music_playing && music_active ) {
......
#ifdef MID_MUSIC
case MUS_MID:
#ifdef USE_NATIVE_MIDI
if ( native_midi_ok ) {
/* Native midi is handled asynchronously */
goto skip;
}
#endif
.....
}
So as per my understanding functionality to pause/resume MIDI should be in Mix_PauseMusic() and Mix_ResumeMusic().
It will fix the problem as well as perfornce wise(which I already explained in prevous post) it will be a better approch.
Created attachment 2251 [details]
Patch for pause and resume music with Windows native MIDI decoder, 2nd version
Thank you for looking into this problem. I was confused about music_active (but still think that it is also needed for fading volume of MIDI).
I attached a different version of the patch. Now it also has locking and a switch statement to add pause/resume for CMD_MUSIC later.
I am still not sure if the implementation is generally correct and does not break something else (e.g. also because the two functions now lock the audio thread which they did not before).
Fixed, thanks! https://hg.libsdl.org/SDL_mixer/rev/5cb894f15335 |