Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Patch] Map filename-based Mix_Load* functions to RWops-based ones #99

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed
Labels
enhancement New feature or request

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: unspecified
Reported for operating system, platform: All, All

Comments on the original bug report:

On 2011-07-23 12:33:51 +0000, Nikos Chantziaras wrote:

Created attachment 653
Patch against current Hg

I already posted this on the mailing list, but I figured having a proper bug item for this is better.

Here is a patch that introduces a new SDL_mixer function:

Mix_Music* Mix_LoadMUSType_RW(SDL_RWops *rw, Mix_MusicType type)

It works similar to Mix_LoadMUS_RW(), except that the format of the data can be explicitly specified (MUS_WAV, MUS_MP3, etc.) Auto-detection is done only when passing MUS_NONE. I assume that not breaking the current API is preferred, which is why I didn't modify the Mix_LoadMUS_RW() signature and introduced a new function instead.

This new function is needed because some MP3s get wrongly detected as being MOD due to non-standard or currupted file headers (which seems to be quite common for MP3 files.)

If anything in this patch doesn't look right, please let me know so I can change it.

On 2011-07-24 08:43:15 +0000, Nikos Chantziaras wrote:

Please don't commit this right now. I'll be reworking it due to:

http://lists.libsdl.org/pipermail/sdl-libsdl.org/2011-July/081439.html

On 2011-07-24 12:15:53 +0000, Nikos Chantziaras wrote:

Created attachment 654
map_file_api_to_rwops.patch

OK, here's a patch that uses the new Mix_LoadMUSType_RW() function in order to clean up the filename-based loading functions:

Map filename-based Mix_Load* functions to RWops-based ones

Mix_LoadMUS() and Mix_LoadWAV(), which take a filename argument, pretty much
duplicate some functionality of what the RWops-based functions do. This patch
cleans some of this up, using the RWops-based functions wherever possible.

The following functions have been deleted entirely, as they're no longer needed:

SMPEG_new(), FLAC_new(), map_openFile(), MOD_new(), modplug_new(), OGG_new(),
native_midi_loadsong(), Timidity_LoadSong(), WAVStream_LoadSong().

In order to avoid code duplication when detecting the format of the loaded
files, two new functions were introduced. An internal helper function:

static Mix_MusicType detect_music_type(SDL_RWops *rw)

and a new API function:

Mix_Music *Mix_LoadMUSType_RW(SDL_RWops *rw, Mix_MusicType type)

The new function allows to override SDL_mixer's auto-detection of the
music format. Mix_LoadMUS_RW() is now a simple wrapper for
Mix_LoadMUSType_RW(). Mix_LoadMUS() uses this in order to guess the file format
from the file's extension.

This patch should preserve source and binary backwards compatibility, as well
as behavior. (And that means that RWops leaks that occured before will still
occur; this patch doesn't fix those.)

On 2011-07-24 12:48:12 +0000, Nikos Chantziaras wrote:

Created attachment 655
fix_stray_else.patch

Oopsy, there's a stray "else" in the code that can mess with Mix_LoadMUS(). Here's a small followup patch.

On 2011-12-31 11:39:15 +0000, Ryan C. Gordon wrote:

Bumping priority on a few SDL_mixer bugs. This is just so we know to look at them for an upcoming release, but once we look at them more closely, we may decide to flag them as WONTFIX or push them back to a later release, so don't take this change in priority as any promise of anything, yet. :)

--ryan.

On 2011-12-31 15:48:07 +0000, Sam Lantinga wrote:

I like the idea of this patch, but I just fixed the music memory leak and this patch doesn't apply anymore.

Could you create a new version of this patch based on the latest source available here?
http://hg.libsdl.org/SDL_mixer

If you need a snapshot archive, let me know and I can arrange that.

Thanks!

On 2012-01-02 17:37:36 +0000, Nikos Chantziaras wrote:

Created attachment 756
map_file_api_to_rwops.patch

(In reply to comment # 5)

Could you create a new version of this patch based on the latest source
available here?
http://hg.libsdl.org/SDL_mixer

I rebased it to current Hg. Attaching.

This build error:

music.c: In function 'music_halt_or_loop':
music.c:203:41: error: 'native_midi_ok' undeclared (first use in this function)
music.c:203:41: note: each undeclared identifier is reported only once for each function it appears in

Was there before, it's not due to this patch.

On 2012-01-02 21:02:07 +0000, Sam Lantinga wrote:

I just did a fresh build of SDL_mixer and didn't get any errors. Are you're sure it's not due to local changes?

On 2012-01-03 00:56:36 +0000, Nikos Chantziaras wrote:

(In reply to comment # 7)

I just did a fresh build of SDL_mixer and didn't get any errors. Are you're
sure it's not due to local changes?

Yes. The error is there with a fresh clone from hg. I just opened bug 1358 for this.

On 2012-01-03 21:18:24 +0000, Sam Lantinga wrote:

Okay, I reviewed the patch and it looks good.
I made a few tweaks, including adding a freesrc parameter, looking at the file extension first, and greatly improving the MP3 detection.

Thanks!
http://hg.libsdl.org/SDL_mixer/rev/3de4970b36d4

@SDLBugzilla SDLBugzilla added the enhancement New feature or request label Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant