search

libsdl-org / SDL_mixer

An audio mixer that supports various file formats for Simple Directmedia Layer.
zlib License
432 stars 147 forks source link

Fix flac-in-ogg handling #566

Closed ankith26 closed 1 year ago

ankith26 commented 1 year ago

The first commit of this PR is for fixing the detection of FLAC files when it is encapsulated in an OGG transport later (currenty these kind of files go down the Ogg-Vorbis path, which cannot handle it).

The second commit fixes the handling of such files in the libflac backend (the drflac backend seems to already take care of this).

I wrote and tested these commits against the SDL2 branch. With few small porting changes I'm making this PR against main. If the maintainers wish I'd be happy to also open the SDL2 branch backport PR for this.

sezero commented 1 year ago

I'm not against this, but not sure how common are the ogg-flac files are.

In any case, shouldn't we handle the extension in Mix_LoadMUS too? IIRC, ogg-flac files usually have oga extension.

ankith26 commented 1 year ago

I'm not sure we can assume every .oga file is a flac-in-ogg. There can be even more obscure formats that use ogg as the container format. I think it's instead better to go down the detector path instead of making incorrect guesses

sezero commented 1 year ago

OK then. @slouken and/or @icculus: Please review + decide for this one.

sezero commented 1 year ago

OK then. @slouken and/or @icculus: Please review + decide for this one.

@slouken: PING: Do we want this one?

slouken commented 1 year ago

Conceptually this is great. I added a couple of style notes and we can merge this when they're complete.

ankith26 commented 1 year ago

I have fixed formatting issues. Also let me know if I can open a backport to SDL2 for this PR

sezero commented 1 year ago

This is in, thanks!

if I can open a backport to SDL2 for this PR

Please do. (It's only different in two lines..)

sezero commented 1 year ago

if I can open a backport to SDL2 for this PR

Please do. (It's only different in two lines..)

Well, I went ahead and cherry-picked it to SDL2 branch as https://github.com/libsdl-org/SDL_mixer/commit/cef595086bf20ca12b31d591fbaaf212721d0b81 Please confirm.

ankith26 commented 1 year ago

Yup, that looks good to me