libsdl-org / SDL_mixer

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

WavPack support #472

Closed sezero closed 1 year ago

sezero commented 1 year ago

This is initial WavPack support for SDL_mixer.

@dbry: Can you browse it briefly for any possible gotchas, please ?

NOTES:

TODO:

How to test: Apart from running wavpack on existing wav files, there is a wavpack test suite (174MB) with several wavpack files which is useful: http://www.rarewares.org/wavpack/test_suite.zip

Closes: https://github.com/libsdl-org/SDL_mixer/issues/382

madebr commented 1 year ago

We need to create a wavpack fork in the libsdl-org organization, and keep it to date with upstream. Once that's done, I'll handle the cmake bits.

sezero commented 1 year ago

We need to create a wavpack fork in the libsdl-org organization, and keep it to date with upstream. Once that's done, I'll handle the cmake bits.

WavPack fork created, with a 5.6.0-sdl branch.

sezero commented 1 year ago

Implemented correction file loading as of commit 8b34106c34fd6b06bfd9f975b52e798bea326f3c. Limitations are, as noted in the commit message:

Another solution would be adding a wavpack-specific loader function to the api, which I don't like.

If anyone has a better solution, I am all ears. On the other hand, the files I tried sounded pretty fair without their correction file present, but I'm not an audiophile either.

sezero commented 1 year ago

As it is now, I think I did all I could with this: We can merge, preferably after cmake work is done.

Remaining stuff are the TODO items listed at top of the PR. In particular, DSD loading is disabled for now - I need help with it. Adding OPEN_DSD_AS_PCM to WavpackOpenFileInputEx64 flags will work, but the result will be accompanied with an unacceptable hiss. Can be done after merge, I guess, unless someone does it here.

madebr commented 1 year ago

I need to push a little fix to wavpack's cmake script, but don't have push access rights.

sezero commented 1 year ago

I need to push a little fix to wavpack's cmake script, but don't have push access rights.

You should have admin rights now along with all @libsdl-org/a-team members

sezero commented 1 year ago

Waiting for any further reviews. If nothing shows up, I can merge after about two days.

sezero commented 1 year ago

@madebr: One minor thing which seems to be missing in cmake: In autotools, I check for <wavpack.h> and define HAVE_WAVPACK_H if it's found. It affects how wavpack.h is included, i.e. <wavpack/wavpack.h> vs <wavpack.h>. I don't think there are any new installations without wavpack/ include subdirectory, but I did it anyway. (And we can live without it in cmake, but wanted to mention anyway..)

madebr commented 1 year ago

@madebr: One minor thing which seems to be missing in cmake: In autotools, I check for <wavpack.h> and define HAVE_WAVPACK_H if it's found. It affects how wavpack.h is included, i.e. <wavpack/wavpack.h> vs <wavpack.h>. I don't think there are any new installations without wavpack/ include subdirectory, but I did it anyway. (And we can live without it in cmake, but wanted to mention anyway..)

I made Findwavpack.cmake such that it returned the folder containing wavpack.h, so you could always do #include <wavpack.h>. But it looks like an installed wavpack-config.cmake required #include <wavpack/wavpack.h>. Thus now a vendored wavpack needs #include <wavpack.h> and a system one #include <wavpack/wavpack.h>.

sezero commented 1 year ago

I made Findwavpack.cmake such that it returned the folder containing wavpack.h, so you could always do #include <wavpack.h>. But it looks like an installed wavpack-config.cmake required #include <wavpack/wavpack.h>. Thus now a vendored wavpack needs #include <wavpack.h> and a system one #include <wavpack/wavpack.h>.

OK, that's good.

sezero commented 1 year ago

This is now in.