We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 1499 - Loop support in mod file is broken
Summary: Loop support in mod file is broken
Status: RESOLVED FIXED
Alias: None
Product: SDL_mixer
Classification: Unclassified
Component: misc (show other bugs)
Version: 2.0.4
Hardware: All All
: P2 normal
Assignee: Ozkan Sezer
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-18 10:06 UTC by Florent Boudet
Modified: 2021-01-30 20:20 UTC (History)
4 users (show)

See Also:

Attachments
Allow looping / Pattern jumps of MOD music using libmodplug (2.58 KB, patch)
2014-08-30 13:50 UTC, Travis R. 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Florent Boudet 2012-05-18 10:06:27 UTC 
When playing a mod file using Mix_PlayMusic, the loops of the mod file
are ignored, resulting in the music being played incorrectly.

This has been discussed in this forum entry (there is a workaround provided):
http://forums.libsdl.org/viewtopic.php?t=7160

This is due to the 'Jump to order' command being ignored while the
file is being played.
http://www.milkytracker.org/docs/MilkyTracker.html#fxBxx

In my particular case,  my game uses a mod file containing multiple music loops.
Music loops are switched dynamically by the game using
Mix_SetMusicPosition(position), which has the particular behaviour of
setting the pattern being played in the case of a mod file.
Having SDL_mixer ignoring the 'Jump' commands totally breaks the
playback of the music in my case.

The reason for mod playback not supporting loops is that this has been
explicitely disabled in the MOD_new_RW() function of the music_mod.c
file, with the line:
module->loop    = 0; (see source code below)

Is there any reason for this? I cannot imagine why anyone would have
loop commands in mod files ignored by default.
(I didn't notice this problem earlier, because before ubuntu 12.04,
the binary SDL packages provided with ubuntu didn't exhibit this
behaviour... Maybe someone had patched it downstream?)

Thanks,
Florent

/* Load a MOD stream from an SDL_RWops object */
MODULE *MOD_new_RW(SDL_RWops *rw, int freerw)
{
       MODULE *module;

       /* Make sure the mikmod library is loaded */
       if ( !Mix_Init(MIX_INIT_MOD) ) {
               if ( freerw ) {
                       SDL_RWclose(rw);
               }
               return NULL;
       }

       module = MikMod_LoadSongRW(rw,64);
       if (!module) {
               Mix_SetError("%s", mikmod.MikMod_strerror(*mikmod.MikMod_errno));
               if ( freerw ) {
                       SDL_RWclose(rw);
               }
               return NULL;
       }

       /* Stop implicit looping, fade out and other flags. */
       module->extspd  = 1;
       module->panflag = 1;
       module->wrap    = 0;
       module->loop    = 0;
#if 0 /* Don't set fade out by default - unfortunately there's no real way
to query the status of the song or set trigger actions.  Hum. */
       module->fadeout = 1;
#endif

       if ( freerw ) {
               SDL_RWclose(rw);
       }
       return module;
}
Comment 1 Travis R. 2014-08-30 13:50:44 UTC 
Created attachment 1857 [details]
Allow looping / Pattern jumps of MOD music using libmodplug

This patch allows explicit looping for module music (MOD, XM, etc). This only works for the Modplug library. To loop a module, simply call Mix_SetMODLoopCount(x) where x is how many times you wish the module to loop for (-1 loops forever). This fixes songs that have pattern jumps that go backwards in a song.

Note that once you call this function, all subsequent loaded modules will loop the given amount of times you have set. If you don't want a module to loop, make sure you specify that before loading it.

Due to how Modplug works, you have to call this function before you call Mix_LoadMUS. Refer to line 97 in modplug.h for more information on this.
Comment 2 Ralph Versteegen 2021-01-28 21:57:02 UTC 
This bug is still present in latest SDL_mixer from hg when using modplug, which is now the default. However the bug where mikmod didn't loop either (bug 1558) was fixed in 826:4359e193d93f (that fix is only on the SDL_mixer 2 branch, not SDL_mixer 1.2).

Note that this breaks normal playback of module files, using Mix_SetMusicPosition as in the original report isn't required.

The patch from Travis looks very strange to me: why add a new API to specify loop count when Mix_PlayMusic already takes a loops arg?

This bug is fixed in SDL Mixer X.
Comment 3 Ralph Versteegen 2021-01-28 22:32:15 UTC 
BTW, using modplug's builtin looping of the whole track also avoids silence at the end of the track which can happen when using SDL_mixer's looping.


I noticed in the bug 1558 discussion that mikmod has separate settings to enable repeating the whole song ("wrap") and to enable loop counts. Modplug doesn't; `settings.mLoopCount = 0` disables both. I did some testing and found mLoopCount=1 makes it play twice, respecting internal loop points but still stopping instead of playing forever. So it seems SDL_mixer should set mLoopCount and not do any additional looping itself.

However because ModPlug_SetSettings has to be called before ModPlug_Load (currently called from MODPLUG_CreateFromRW) to set the loop count, but the loop count is only specified by the user when calling Mix_PlayMusic, it seems like it's necessary to call ModPlug_Load twice. The second time can be skipped if the loop count is the same as it was the last time ModPlug_SetSettings was called.
Comment 4 Ozkan Sezer 2021-01-29 17:14:58 UTC 
(In reply to Ralph Versteegen from comment #2)
> This bug is fixed in SDL Mixer X.

Can you point to the specific commit fixing this?
Comment 5 Ralph Versteegen 2021-01-30 12:37:03 UTC 
Sorry, I should have clarified, all SDL Mixer X did was to enable loop points by setting settings.mLoopCount = -1 as below. This makes modules play correctly but means the 'loops' arg to Mix_PlayMusic() will be ignored. Respecting the loop count is much trickier, as I discussed. But IMO it's much more important to play the tracks correctly (and loop points are very common in modules) than to support playing a module n times, and I and others would much appreciate that quick fix. In fact SDL Mixer X even lists "enabled internal loops" in Modplug as a feature.


--- a/src/codecs/music_modplug.c        Mon Jan 11 05:55:02 2021 +0300
+++ b/src/codecs/music_modplug.c        Mon Jan 25 23:06:20 2021 +1300
@@ -152,7 +152,7 @@
     settings.mBassRange = 50;
     settings.mSurroundDepth = 0;
     settings.mSurroundDelay = 10;
-    settings.mLoopCount = 0;
+    settings.mLoopCount = -1;
     modplug.ModPlug_SetSettings(&settings);
     return 0;
 }
Comment 6 Ozkan Sezer 2021-01-30 15:59:26 UTC 
(In reply to Ralph Versteegen from comment #2)
> This bug is still present in latest SDL_mixer from hg when using modplug,
> which is now the default. However the bug where mikmod didn't loop either
> (bug 1558) was fixed in 826:4359e193d93f (that fix is only on the SDL_mixer
> 2 branch, not SDL_mixer 1.2).

Backported bug #1558 fix to SDL-1.2 branch:
https://hg.libsdl.org/SDL_mixer/rev/fb2e01f829d6

Will be looking at modplug later.
Comment 7 Ozkan Sezer 2021-01-30 17:03:18 UTC 
Set mLoopCount to -1 in modplug loader in both SDL-1.2 and
default 2.0 branches:
https://hg.libsdl.org/SDL_mixer/rev/9d7507e205fe
https://hg.libsdl.org/SDL_mixer/rev/1c494a54b6f8

Closing as fixed.
Comment 8 Vitaly Novichkov 2021-01-30 20:20:33 UTC 
The fact, I had to fix this bug at MixerX fork for a long time. I didn't backport this change as I thought that was intended to set at the mainstream SDL Mixer side. I'm glad you finally applied this change to the mainstream!