Add GME for video game music support

[connorjclark]

This is a partial merge of the work by @Wohlstand referenced in #233.

• ~Grabs the latest GME from https://bitbucket.org/mpyne/game-music-emu/src/master/ (commit b3d158)~
• Grabs music_gme.c and edits to music.c from https://github.com/WohlSoft/SDL-Mixer-X (commit bed335)
• Modifies music_gme.c to use a new GME_StartTrack, which is simpler to work with than the "process string args" approach that exists in the fork. With this, a single Mix_Music handle can be used and its track changed at any time
• Modifies music_gme.c to better match the code style in other files (probably could use more work here)

This is working for me when compiling with Emscripten and running in the browser. I verified these formats work:

• gbs
• nsf
• spc
• vgm

I don't have an m3u. There are probably other formats, but I couldn't find a list of what GME supports.

Unfortunately I'm not sure how to verify this would work on other platforms, hopefully someone can help out there. Particularly, I have no idea how to edit the configure file.

[Wohlstand]

Don't worry about M3U at all, it's Playlist format. The Mixer intended to play individual files, not playlists. Also, keep a note that the proper support for multi-track files the path arguments sub-system is needed to be ported too. I made it to allow per-song settings being written as a small appendix to the input file path, or being passes as a separated string if using specific API calls and/or memory reading based stuff.

Why path arguments system made: its a major feature of MixerX that allows client-side apps without major modifications, be able to set up song individual properties such as track number, gain factor, initial tempo, etc. Just my small appendix to the music file path. I do have the separated music open call that sets the track number, but it's rarely used as my first idea works successfully. Path arguments were also used not just for GME only: that is also used for custom MIDI synthesizer libraries to allow per-song fine tunes, etc.

[connorjclark]

I thought it'd make the PR simpler for SDL_mixer maintainers to review without the path stuff. I'll wait for their input before either adding it back in this PR or doing it as a followup.

BTW, thanks for making all this!

[sezero]

Adjusting configury is the easy part - and since you asked, here it is: patch_configure.txt

However: I don't think that this is acceptable as it is:

1. Adding a horde of someone else's c++ code to this tree is too much. What might possibly be acceptable would be linking to a libgme: That part is a new feature, and @slouken and/or @icculus should decide whether they want that feature.

2. The c++ code doesn't even compile for me as it is: it seems to have a lot of assumptions about c++0x (I guess), and with my old g++-4.9 with -std=c++0x still doesn't accept the code. Not to mention its utterly broken debug_printf macro. So, my arguments from the first part about adding someone else's large amount of c++ crapola repeat here.

Warnings from music_gme.c

../src/codecs/music_gme.c:426: warning: ‘GME_SetTempo’ defined but not used
../src/codecs/music_gme.c:437: warning: ‘GME_GetTempo’ defined but not used
../src/codecs/music_gme.c:446: warning: ‘GME_GetTracksCount’ defined but not used
../src/codecs/music_gme.c:455: warning: ‘GME_SetTrackMute’ defined but not used

The c++ side: Without c++0x:

In file included from ../src/codecs/gme/Ay_Apu.h:7:0,
                 from ../src/codecs/gme/Ay_Apu.cpp:3:
../src/codecs/gme/blargg_common.h:55:2: warning: identifier 'nullptr' is a keyword in C++11 [-Wc++0x-compat]
  void clear() { free( begin_ ); begin_ = nullptr; size_ = 0; }
  ^
../src/codecs/gme/blargg_common.h: In member function 'void blargg_vector<T>::clear()':
../src/codecs/gme/blargg_common.h:55:42: error: 'nullptr' was not declared in this scope
  void clear() { free( begin_ ); begin_ = nullptr; size_ = 0; }
                                          ^
../src/codecs/gme/blargg_common.h: In member function 'T& blargg_vector<T>::operator[](size_t) const':
../src/codecs/gme/blargg_common.h:58:22: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
   assert( n <= size_ ); // <= to allow past-the-end value
                      ^
../src/codecs/gme/blargg_common.h:58:22: note: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
In file included from ../src/codecs/gme/Ay_Apu.h:8:0,
                 from ../src/codecs/gme/Ay_Apu.cpp:3:
../src/codecs/gme/Blip_Buffer.h: At global scope:
../src/codecs/gme/Blip_Buffer.h:236:79: warning: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11
  Blip_Synth<quality, range>           (const Blip_Synth<quality, range>  &) = delete;
                                                                               ^
../src/codecs/gme/Blip_Buffer.h:237:73: error: expected ',' or '...' before '&&' token
  Blip_Synth<quality, range>           (      Blip_Synth<quality, range> &&) = delete;
                                                                         ^
../src/codecs/gme/Blip_Buffer.h:237:79: warning: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11
  Blip_Synth<quality, range>           (      Blip_Synth<quality, range> &&) = delete;
                                                                               ^
../src/codecs/gme/Blip_Buffer.h:237:79: error: invalid constructor; you probably meant 'Blip_Synth<quality, range> (const Blip_Synth<quality, range>&)'
../src/codecs/gme/Blip_Buffer.h:238:79: warning: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11
  Blip_Synth<quality, range>& operator=(const Blip_Synth<quality, range> &)  = delete;
                                                                               ^
../src/codecs/gme/Blip_Buffer.h: In member function 'void Blip_Synth<quality, range>::offset_resampled(blip_resampled_time_t, int, Blip_Buffer*) const':
../src/codecs/gme/Blip_Buffer.h:347:78: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
  assert( (blip_long) (time >> BLIP_BUFFER_ACCURACY) < blip_buf->buffer_size_ );
                                                                              ^
In file included from ../src/codecs/gme/Ay_Apu.cpp:3:0:
../src/codecs/gme/Ay_Apu.h: In member function 'void Ay_Apu::osc_output(int, Blip_Buffer*)':
../src/codecs/gme/Ay_Apu.h:86:35: error: 'assert' was not declared in this scope
  assert( (unsigned) i < osc_count );
                                   ^
../src/codecs/gme/Ay_Apu.h: In member function 'void Ay_Apu::end_frame(blip_time_t)':
../src/codecs/gme/Ay_Apu.h:102:28: error: 'assert' was not declared in this scope
  assert( last_time >= time );
                            ^
../src/codecs/gme/Ay_Apu.cpp: In member function 'void Ay_Apu::write_data_(int, int)':
../src/codecs/gme/Ay_Apu.cpp:122:38: error: 'assert' was not declared in this scope
  assert( (unsigned) addr < reg_count );
                                      ^
../src/codecs/gme/Ay_Apu.cpp:127:52: warning: left operand of comma operator has no effect [-Wunused-value]
    debug_printf( "Wrote to I/O port %02X\n", (int) addr );
                                                    ^
In file included from ../src/codecs/gme/Ay_Apu.cpp:16:0:
../src/codecs/gme/Ay_Apu.cpp: In member function 'void Ay_Apu::run_until(blip_time_t)':
../src/codecs/gme/blargg_source.h:19:38: error: 'assert' was not declared in this scope
 #define require( expr ) assert( expr )
                                      ^
../src/codecs/gme/Ay_Apu.cpp:166:2: note: in expansion of macro 'require'
  require( final_end_time >= last_time );
  ^
In file included from ../src/codecs/gme/Ay_Apu.h:8:0,
                 from ../src/codecs/gme/Ay_Apu.cpp:3:
../src/codecs/gme/Blip_Buffer.h: In instantiation of 'void Blip_Synth<quality, range>::offset_resampled(blip_resampled_time_t, int, Blip_Buffer*) const [with int quality = 12; int range = 1; blip_resampled_time_t = unsigned int]':
../src/codecs/gme/Blip_Buffer.h:457:64:   required from 'void Blip_Synth<quality, range>::offset(blip_time_t, int, Blip_Buffer*) const [with int quality = 12; int range = 1; blip_time_t = int]'
../src/codecs/gme/Ay_Apu.cpp:278:51:   required from here
../src/codecs/gme/Blip_Buffer.h:347:78: error: 'assert' was not declared in this scope
  assert( (blip_long) (time >> BLIP_BUFFER_ACCURACY) < blip_buf->buffer_size_ );
                                                                              ^
make: *** [build/Ay_Apu.lo] Error 1

With c++0x:

In file included from ../src/codecs/gme/Ay_Apu.h:7:0,
                 from ../src/codecs/gme/Ay_Apu.cpp:3:
../src/codecs/gme/blargg_common.h:55:2: warning: identifier 'nullptr' is a keyword in C++11 [-Wc++0x-compat]
  void clear() { free( begin_ ); begin_ = nullptr; size_ = 0; }
  ^
../src/codecs/gme/blargg_common.h: In member function 'void blargg_vector<T>::clear()':
../src/codecs/gme/blargg_common.h:55:42: error: 'nullptr' was not declared in this scope
  void clear() { free( begin_ ); begin_ = nullptr; size_ = 0; }
                                          ^
../src/codecs/gme/blargg_common.h: In member function 'T& blargg_vector<T>::operator[](size_t) const':
../src/codecs/gme/blargg_common.h:58:22: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
   assert( n <= size_ ); // <= to allow past-the-end value
                      ^
../src/codecs/gme/blargg_common.h:58:22: note: (if you use '-fpermissive', G++ will accept your code, but allowing the use of an undeclared name is deprecated)
In file included from ../src/codecs/gme/Ay_Apu.h:8:0,
                 from ../src/codecs/gme/Ay_Apu.cpp:3:
../src/codecs/gme/Blip_Buffer.h: At global scope:
../src/codecs/gme/Blip_Buffer.h:236:79: warning: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11
  Blip_Synth<quality, range>           (const Blip_Synth<quality, range>  &) = delete;
                                                                               ^
../src/codecs/gme/Blip_Buffer.h:237:73: error: expected ',' or '...' before '&&' token
  Blip_Synth<quality, range>           (      Blip_Synth<quality, range> &&) = delete;
                                                                         ^
../src/codecs/gme/Blip_Buffer.h:237:79: warning: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11
  Blip_Synth<quality, range>           (      Blip_Synth<quality, range> &&) = delete;
                                                                               ^
../src/codecs/gme/Blip_Buffer.h:237:79: error: invalid constructor; you probably meant 'Blip_Synth<quality, range> (const Blip_Synth<quality, range>&)'
../src/codecs/gme/Blip_Buffer.h:238:79: warning: defaulted and deleted functions only available with -std=c++11 or -std=gnu++11
  Blip_Synth<quality, range>& operator=(const Blip_Synth<quality, range> &)  = delete;
                                                                               ^
../src/codecs/gme/Blip_Buffer.h: In member function 'void Blip_Synth<quality, range>::offset_resampled(blip_resampled_time_t, int, Blip_Buffer*) const':
../src/codecs/gme/Blip_Buffer.h:347:78: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
  assert( (blip_long) (time >> BLIP_BUFFER_ACCURACY) < blip_buf->buffer_size_ );
                                                                              ^
In file included from ../src/codecs/gme/Ay_Apu.cpp:3:0:
../src/codecs/gme/Ay_Apu.h: In member function 'void Ay_Apu::osc_output(int, Blip_Buffer*)':
../src/codecs/gme/Ay_Apu.h:86:35: error: 'assert' was not declared in this scope
  assert( (unsigned) i < osc_count );
                                   ^
../src/codecs/gme/Ay_Apu.h: In member function 'void Ay_Apu::end_frame(blip_time_t)':
../src/codecs/gme/Ay_Apu.h:102:28: error: 'assert' was not declared in this scope
  assert( last_time >= time );
                            ^
../src/codecs/gme/Ay_Apu.cpp: In member function 'void Ay_Apu::write_data_(int, int)':
../src/codecs/gme/Ay_Apu.cpp:122:38: error: 'assert' was not declared in this scope
  assert( (unsigned) addr < reg_count );
                                      ^
../src/codecs/gme/Ay_Apu.cpp:127:52: warning: left operand of comma operator has no effect [-Wunused-value]
    debug_printf( "Wrote to I/O port %02X\n", (int) addr );
                                                    ^
In file included from ../src/codecs/gme/Ay_Apu.cpp:16:0:
../src/codecs/gme/Ay_Apu.cpp: In member function 'void Ay_Apu::run_until(blip_time_t)':
../src/codecs/gme/blargg_source.h:19:38: error: 'assert' was not declared in this scope
 #define require( expr ) assert( expr )
                                      ^
../src/codecs/gme/Ay_Apu.cpp:166:2: note: in expansion of macro 'require'
  require( final_end_time >= last_time );
  ^
In file included from ../src/codecs/gme/Ay_Apu.h:8:0,
                 from ../src/codecs/gme/Ay_Apu.cpp:3:
../src/codecs/gme/Blip_Buffer.h: In instantiation of 'void Blip_Synth<quality, range>::offset_resampled(blip_resampled_time_t, int, Blip_Buffer*) const [with int quality = 12; int range = 1; blip_resampled_time_t = unsigned int]':
../src/codecs/gme/Blip_Buffer.h:457:64:   required from 'void Blip_Synth<quality, range>::offset(blip_time_t, int, Blip_Buffer*) const [with int quality = 12; int range = 1; blip_time_t = int]'
../src/codecs/gme/Ay_Apu.cpp:278:51:   required from here
../src/codecs/gme/Blip_Buffer.h:347:78: error: 'assert' was not declared in this scope
  assert( (blip_long) (time >> BLIP_BUFFER_ACCURACY) < blip_buf->buffer_size_ );
                                                                              ^
make: *** [build/Ay_Apu.lo] Error 1

[connorjclark]

Removed the GME folder, having the user link against the library is better.

re: the unused functions for tempo, is there a preference for adding additional methods to Mix_MusicInterface in this PR to expose setting the speed, or as a separate PR?

[slouken]

A separate PR would be best.

[Wohlstand]

P.S. Late reply to @sezero about GME compilation: it requires the C++11 support by the compiler side, and elder GCCs obviously lack the support for it. The -std=c++11 is mainly used to build the thing. Also, using CMake build provided by the mainstream version, or my custom minified CMake build at my AudioCodecs aggregation repo. I don't remind, but something worked at GCC 4.8 or not, I do remind about my other C++11 project, not speaking about GME that probably also failed, and I installed the system-wide version from packages or something also...

[sezero]

P.S. Late reply to @sezero about GME compilation: it requires the C++11

The mainstream code does compile for me using gcc4.9 (its cmake specifically adds -std=c++11)

[sezero]

Here are some clean-ups:

• gme_disable_echo is not in mainstream yet, only a pull request: remove.
• GME_HAS_SET_AUTOLOAD_PLAYBACK_LIMIT isn't defined anywhere: Safest way would be a GME_VERSION check. And unlike what is documented in gme.h, it is in 0.6.3 and newer, not 0.6.2.
• Followin commit ecce55a in master, the macOS case needs revising.

Patch attached: patch1.txt, and also inlined below:

diff --git a/src/codecs/music_gme.c b/src/codecs/music_gme.c
index 841a86c..3b44ef8 100644
--- a/src/codecs/music_gme.c
+++ b/src/codecs/music_gme.c
@@ -44,7 +44,6 @@ typedef struct {
     void (*gme_set_fade)(Music_Emu*, int start_msec);
 #endif
     void (*gme_set_autoload_playback_limit)(Music_Emu*, int do_autoload_limit);
-    void (*gme_disable_echo)(Music_Emu*, int disable);
     gme_err_t (*gme_track_info)(Music_Emu const*, gme_info_t** out, int track);
     void (*gme_free_info)(gme_info_t*);
     gme_err_t (*gme_seek)(Music_Emu*, int msec);
@@ -61,7 +60,8 @@ static gme_loader gme;
     if (gme.FUNC == NULL) { SDL_UnloadObject(gme.handle); return -1; }
 #else
 #define FUNCTION_LOADER(FUNC, SIG) \
-    gme.FUNC = FUNC;
+    gme.FUNC = FUNC; \
+    if (gme.FUNC == NULL) { Mix_SetError("Missing GME.framework"); return -1; }
 #endif
 
 static int GME_Load(void)
@@ -72,13 +72,6 @@ static int GME_Load(void)
         if (gme.handle == NULL) {
             return -1;
         }
-#elif defined(__MACOSX__)
-        extern gme_err_t gme_open_data(void const*,long,Music_Emu**,int) __attribute__((weak_import));
-        if (gme_open_data == NULL) {
-            /* Missing weakly linked framework */
-            Mix_SetError("Missing GME.framework");
-            return -1;
-        }
 #endif
         FUNCTION_LOADER(gme_open_data, gme_err_t (*)(void const*,long,Music_Emu**,int))
         FUNCTION_LOADER(gme_track_count, int (*)(Music_Emu const*))
@@ -100,19 +93,11 @@ static int GME_Load(void)
         FUNCTION_LOADER(gme_delete, void (*)(Music_Emu*))
 #if defined(GME_DYNAMIC)
         gme.gme_set_autoload_playback_limit = (void (*)(Music_Emu*,int)) SDL_LoadFunction(gme.handle, "gme_set_autoload_playback_limit");
-#elif defined(GME_HAS_SET_AUTOLOAD_PLAYBACK_LIMIT)
+#elif (GME_VERSION >= 0x000603)
         gme.gme_set_autoload_playback_limit = gme_set_autoload_playback_limit;
 #else
         gme.gme_set_autoload_playback_limit = NULL;
 #endif
-
-#if defined(GME_DYNAMIC)
-        gme.gme_disable_echo = (void (*)(Music_Emu*,int)) SDL_LoadFunction(gme.handle, "gme_disable_echo");
-#elif defined(GME_HAS_DISABLE_ECHO)
-        gme.gme_disable_echo = gme_disable_echo;
-#else
-        gme.gme_disable_echo = NULL;
-#endif
     }
     ++gme.loaded;
 

[Wohlstand]

GME_HAS_SET_AUTOLOAD_PLAYBACK_LIMIT

This is a part of the CMake-side auto-check result I wrote to detect compatible ABI, there are older GME versions that lack this call, and without this call, on newer GME some songs won't loop when expected they will loop. Just a check via version number may fail at some places, so, safer to use the configure-side check and macro.

[sezero]

GME_HAS_SET_AUTOLOAD_PLAYBACK_LIMIT

This is a part of the CMake-side auto-check result I wrote to detect compatible ABI,

In any case, the GME_VERSION check I used above should be equivalent and enough.

there are older GME versions that lack this call, and without this call, on newer GME some songs won't loop when expected they will loop.

Should we require at least version 0.6.3 and newer then? (I guess that would be too much but...)

[Wohlstand]

Should we require at least version 0.6.3 and newer then? (I guess that would be too much but...)

There are several copies of the 0.6.3 version without this call, I know that because I long time watching for the GME side, and I met compilation errors at some machines when using GME out of my own build (taken from the mainstream repo and later)

[sezero]

There are several copies of the 0.6.3 version without this call,

Eh?? Can you give links?

(taken from the mainstream repo and later)

I can't even download release tarballs linked at main bitbucket site (OpenSSL errors)

[Wohlstand]

Eh?? Can you give links?

Can't remind, you if they were found at package managers or there are was middle versions of 0.6.3 while its development process

[sezero]

Looked at fedora and ubuntu sources for 0.6.3, they have the gme_set_autoload_playback_limit call, and so does the 0.6.3 tag in mainstream git, therefore the above check should be good.

[Wohlstand]

Some news: no need to remove SPC Echo-related usage, because my pull request got been finally merged: https://bitbucket.org/mpyne/game-music-emu/pull-requests/30, now it's the mainstream part.

[sezero]

The OP (@connorjclark) seems to have lost interest?

[connorjclark]

I'll try to find the time this evening to apply the suggested patches.

Some news: no need to remove SPC Echo-related usage, because my pull request got been finally merged: bitbucket.org/mpyne/game-music-emu/pull-requests/30, now it's the mainstream part.

Would you like to handle this in a new PR? I wouldn't know how to test this.

[sezero]

I'll try to find the time this evening to apply the suggested patches.

OK good.

While doing that I suggest that you drop changes to configure itself: I will regenerate that for you, so only concern yourself with configure.ac

Some news: no need to remove SPC Echo-related usage, because my pull request got been finally merged: bitbucket.org/mpyne/game-music-emu/pull-requests/30, now it's the mainstream part.

Would you like to handle this in a new PR? I wouldn't know how to test this.

There is no code at all using gme_disable_echo in here: we remove it for the time being. If music_gme.c gets real use of it then we can think about what to do with that.

Here are the patches that need applying (same as above but repeated here for convenience, and with changes to generated configure file dropped):

1. Fixes to configure.ac: patch-01.txt
2. Fixes / cleanups to music_gme.c (patch file has explanations): patch-02.txt

[Wohlstand]

There is no code at all using gme_disable_echo in here: we remove it for the time being. If music_gme.c gets real use of it then we can think about what to do with that.

On my side I already use it: I do apply various sound effects using Mix_SetPostMix() call and siblings, and when I play SPC files, their built-in echo conflicts with effects I do apply on my side, they result in the mess. So, that option I introduced, allows disabling the native echo effect of SPC files, making them play in plain form. This resolves the conflict with custom sound effects of the echo and reverb family.

[sezero]

@slouken: Do we want this feature?

[slouken]

I'm not opposed to it, although the official library page says that it's on maintenance life support rather than being actively maintained. Do we have examples of games that use this music format? Are there good test files that we can use to verify the functionality?

[connorjclark]

The Zelda Classic game engine (github) uses GME to enable its game creators to leverage tons of video game music in their own games. I recently ported this engine to the web, using SDL as the backend (and a fork of SDL_mixer with this patch to support GME).

Here's one example game using gbs music: https://hoten.cc/zc/play/?quest=623/Rite+of+the+Storm/Rite+of+the+Storm.qst . You'll see the gbs file is downloaded and played once the title sequence for the game loads (sorry, you'll have to wait for the page to download some things first)

I have access to a few dozen files in these various formats, should I add a few for test cases?

[Wohlstand]

The set of test files, crafted by various people (and two of them crafted by me a while ago): test-spc-files.zip

Also, very important note, I forgot to tell you: on my side, through path arguments its possible to set the so-called "gaining factor" - its multiplier of the volume level, it allows to make too silent music sound louder and allows to calm down too loud music, making it a bit quite. Initially, I also hadn't the gaining factor, but with a time people noticed that it's VERY important to add it because there are lots of chiptunes that sound too quiet with default output. The constant gaining factor is not suitable, because, among silent files, there are opposite loud files that may cause clipping while using integer output formats.

[connorjclark]

Any other changes needed?

[sezero]

Looking at this again, every music interface other than music_gme have the StartTrack and GetNumTracks fields uninitialized: they should be. Here is a patch (attached and also inlined below) which does that, and also makes a tiny clean-up in music_gme. patch-03.txt

diff -u a/src/codecs/music_gme.c b/src/codecs/music_gme.c
--- a/src/codecs/music_gme.c
+++ b/src/codecs/music_gme.c
@@ -1,6 +1,6 @@
 /*
   SDL_mixer:  An audio mixer library based on the SDL library
-  Copyright (C) 1997-2020 Sam Lantinga <[email protected]>
+  Copyright (C) 1997-2022 Sam Lantinga <[email protected]>
 
   This software is provided 'as-is', without any express or implied
   warranty.  In no event will the authors be held liable for any damages
@@ -124,7 +124,6 @@
     Music_Emu* game_emu;
     int freesrc;
     SDL_bool has_track_length;
-    int echo_disabled;
     int track_length;
     int intro_length;
     int loop_length;
@@ -432,7 +431,7 @@
     GME_Play,
     NULL,   /* IsPlaying */
     GME_PlayAudio,
-    NULL,       /* Jump */
+    NULL,   /* Jump */
     GME_Seek,
     GME_Tell,
     GME_Duration,
@@ -449,6 +448,5 @@
-
     GME_StartTrack,
-    GME_GetNumTracks,
+    GME_GetNumTracks
 };
 
-#endif /* MUSIC_GME */
\ No newline at end of file
+#endif /* MUSIC_GME */
diff --git a/src/codecs/music_cmd.c b/src/codecs/music_cmd.c
index a293f92..444f342 100644
--- a/src/codecs/music_cmd.c
+++ b/src/codecs/music_cmd.c
@@ -298,7 +298,9 @@ Mix_MusicInterface Mix_MusicInterface_CMD =
     MusicCMD_Stop,
     MusicCMD_Delete,
     NULL,   /* Close */
-    NULL    /* Unload */
+    NULL,   /* Unload */
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_CMD */
diff --git a/src/codecs/music_drflac.c b/src/codecs/music_drflac.c
index 635e975..67f6fce 100644
--- a/src/codecs/music_drflac.c
+++ b/src/codecs/music_drflac.c
@@ -414,7 +414,9 @@ Mix_MusicInterface Mix_MusicInterface_DRFLAC =
     DRFLAC_Stop,
     DRFLAC_Delete,
     NULL,   /* Close */
-    NULL    /* Unload */
+    NULL,   /* Unload */
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_FLAC_DRFLAC */
diff --git a/src/codecs/music_drmp3.c b/src/codecs/music_drmp3.c
index 7af022d..5f75f2f 100644
--- a/src/codecs/music_drmp3.c
+++ b/src/codecs/music_drmp3.c
@@ -287,7 +287,9 @@ Mix_MusicInterface Mix_MusicInterface_DRMP3 =
     DRMP3_Stop,
     DRMP3_Delete,
     NULL,   /* Close */
-    NULL    /* Unload */
+    NULL,   /* Unload */
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_MP3_DRMP3 */
diff --git a/src/codecs/music_flac.c b/src/codecs/music_flac.c
index 2e19d80..60b2e54 100644
--- a/src/codecs/music_flac.c
+++ b/src/codecs/music_flac.c
@@ -748,13 +748,15 @@ Mix_MusicInterface Mix_MusicInterface_FLAC =
     FLAC_LoopStart,
     FLAC_LoopEnd,
     FLAC_LoopLength,
-    FLAC_GetMetaTag,/* GetMetaTag */
+    FLAC_GetMetaTag,
     NULL,   /* Pause */
     NULL,   /* Resume */
     FLAC_Stop,   /* Stop */
     FLAC_Delete,
     NULL,   /* Close */
-    FLAC_Unload
+    FLAC_Unload,
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_FLAC_LIBFLAC */
diff --git a/src/codecs/music_fluidsynth.c b/src/codecs/music_fluidsynth.c
index 49d9699..39c633a 100644
--- a/src/codecs/music_fluidsynth.c
+++ b/src/codecs/music_fluidsynth.c
@@ -368,7 +368,9 @@ Mix_MusicInterface Mix_MusicInterface_FLUIDSYNTH =
     FLUIDSYNTH_Stop,
     FLUIDSYNTH_Delete,
     NULL,   /* Close */
-    FLUIDSYNTH_Unload
+    FLUIDSYNTH_Unload,
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_MID_FLUIDSYNTH */
diff --git a/src/codecs/music_modplug.c b/src/codecs/music_modplug.c
index aca5d5e..f1744eb 100644
--- a/src/codecs/music_modplug.c
+++ b/src/codecs/music_modplug.c
@@ -368,7 +368,9 @@ Mix_MusicInterface Mix_MusicInterface_MODPLUG =
     MODPLUG_Stop,
     MODPLUG_Delete,
     NULL,   /* Close */
-    MODPLUG_Unload
+    MODPLUG_Unload,
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_MOD_MODPLUG */
diff --git a/src/codecs/music_mpg123.c b/src/codecs/music_mpg123.c
index a8198d8..4b23c34 100644
--- a/src/codecs/music_mpg123.c
+++ b/src/codecs/music_mpg123.c
@@ -539,7 +539,9 @@ Mix_MusicInterface Mix_MusicInterface_MPG123 =
     MPG123_Stop,
     MPG123_Delete,
     MPG123_Close,
-    MPG123_Unload
+    MPG123_Unload,
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_MP3_MPG123 */
diff --git a/src/codecs/music_nativemidi.c b/src/codecs/music_nativemidi.c
index 22fbcc0..408881e 100644
--- a/src/codecs/music_nativemidi.c
+++ b/src/codecs/music_nativemidi.c
@@ -113,7 +113,9 @@ Mix_MusicInterface Mix_MusicInterface_NATIVEMIDI =
     NATIVEMIDI_Stop,
     NATIVEMIDI_Delete,
     NULL,   /* Close */
-    NULL    /* Unload */
+    NULL,   /* Unload */
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_MID_NATIVE */
diff --git a/src/codecs/music_ogg.c b/src/codecs/music_ogg.c
index 16af47b..15688ac 100644
--- a/src/codecs/music_ogg.c
+++ b/src/codecs/music_ogg.c
@@ -548,7 +548,9 @@ Mix_MusicInterface Mix_MusicInterface_OGG =
     OGG_Stop,
     OGG_Delete,
     NULL,   /* Close */
-    OGG_Unload
+    OGG_Unload,
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_OGG */
diff --git a/src/codecs/music_ogg_stb.c b/src/codecs/music_ogg_stb.c
index b33f38e..c19e523 100644
--- a/src/codecs/music_ogg_stb.c
+++ b/src/codecs/music_ogg_stb.c
@@ -477,7 +477,9 @@ Mix_MusicInterface Mix_MusicInterface_OGG =
     OGG_Stop,
     OGG_Delete,
     NULL,   /* Close */
-    NULL    /* Unload */
+    NULL,   /* Unload */
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_OGG */
diff --git a/src/codecs/music_opus.c b/src/codecs/music_opus.c
index c298095..d1048a8 100644
--- a/src/codecs/music_opus.c
+++ b/src/codecs/music_opus.c
@@ -519,7 +519,9 @@ Mix_MusicInterface Mix_MusicInterface_Opus =
     OPUS_Stop,
     OPUS_Delete,
     NULL,   /* Close */
-    OPUS_Unload
+    OPUS_Unload,
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_OPUS */
diff --git a/src/codecs/music_timidity.c b/src/codecs/music_timidity.c
index 5822c4b..cdc3916 100644
--- a/src/codecs/music_timidity.c
+++ b/src/codecs/music_timidity.c
@@ -289,7 +289,9 @@ Mix_MusicInterface Mix_MusicInterface_TIMIDITY =
     TIMIDITY_Stop,
     TIMIDITY_Delete,
     TIMIDITY_Close,
-    NULL    /* Unload */
+    NULL,   /* Unload */
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_MID_TIMIDITY */
diff --git a/src/codecs/music_xmp.c b/src/codecs/music_xmp.c
index 39d46c3..057ac7f 100644
--- a/src/codecs/music_xmp.c
+++ b/src/codecs/music_xmp.c
@@ -431,7 +431,9 @@ Mix_MusicInterface Mix_MusicInterface_XMP =
     XMP_Stop,
     XMP_Delete,
     NULL,   /* Close */
-    XMP_Unload
+    XMP_Unload,
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_MOD_XMP */
diff --git a/src/codecs/music_wav.c b/src/codecs/music_wav.c
index e4140c2..11b92af 100644
--- a/src/codecs/music_wav.c
+++ b/src/codecs/music_wav.c
@@ -931,8 +931,7 @@ static SDL_bool LoadWAVMusic(WAV_Music *wave)
         if (chunk_length == 0)
             break;
 
-        switch (chunk_type)
-        {
+        switch (chunk_type) {
         case FMT:
             found_FMT = SDL_TRUE;
             if (!ParseFMT(wave, chunk_length))
@@ -1135,7 +1134,6 @@ static SDL_bool LoadAIFFMusic(WAV_Music *wave)
         return SDL_FALSE;
     }
 
-
     wave->samplesize = channels * (samplesize / 8);
     wave->stop = wave->start + channels * numsamples * (samplesize / 8);
 
@@ -1259,7 +1257,9 @@ Mix_MusicInterface Mix_MusicInterface_WAV =
     WAV_Stop, /* Stop */
     WAV_Delete,
     NULL,   /* Close */
-    NULL    /* Unload */
+    NULL,   /* Unload */
+    NULL,   /* StartTrack   */
+    NULL    /* GetNumTracks */
 };
 
 #endif /* MUSIC_WAV */

[sezero]

OK, I have nothing further.

The source files and any necessary defines & etc will still need adding to cmake, msvc and xcode project files, but those can be done later.

[sezero]

OK, I have nothing further.

The source files and any necessary defines & etc will still need adding to cmake, msvc and xcode project files, but those can be done later.

@slouken: Further review and final decision is for you.

[connorjclark]

New conflict now... But good for merging otherwise?

[smcv]

the official library page says that it's on maintenance life support rather than being actively maintained.

It's probably worth bearing in mind that decoding no-longer-mainstream music formats from classic game engines has been part of a security vulnerability exploit chain in the past, for example the batch of CVEs described in https://bugs.mageia.org/show_bug.cgi?id=19952 (which affected general-purpose audio/video players and potentially even desktop environments' automatic video thumbnailing, via a GME GStreamer plugin).

Perhaps it would be enough to gate GME support behind a MIX_INIT_GME flag, and document that flag as unsafe/unsupported to use in programs that can be made to load untrusted data?

[Wohlstand]

Speaking about security, some year ago I watched Michael Pyne made a lot of tweaks and updates to resolve such problems, and I also helped him to fix the HES support that was broken accidentally. The topic you linked was made in 2016 and marked as resolved. Anyway, these cases also would basically crash the stuff because of bad data in the music file, I caught some files that can crash the library and reported/fixed them with Michael.