Support libmpg123 for MP3 playback #818

Closed
fdelapena opened this Issue Mar 12, 2016 · 18 comments

Projects

None yet

5 participants

@fdelapena
Member

By using this amazing library, we would get the following:

  • Plays broken MP3 created with broken tools using libLAME incorrectly, properly skips data until finding the first valid frame (compliant decoder), which would be a fix for #721.
  • Gapless playback loops where possible (it even supports special LAME gapless tags if they exist)
  • libmpg123 is maintained (libmad is dead from more than a decade and needs a bunch of patches) and packaged in popular distros.
  • Because we need new code for this, it would prevent noises or crashes due to buffer issues in existing libmad's dynamic_mp3/Mix_PlayMusic code when resampling (#781, #677).
  • Supports MPEG-1/2/2.5 without issues (SMPEG crashes with some 2/2.5). Also supports I/II/III layers like libmad (layer III is enough I guess).
  • LGPLv2.1, this license is compatible with code combination (and therefore static linking) with GPLv3. With libmad was also fine because it is GPLv2+.

Below there is a test program to demonstrate how it plays files properly.

@fdelapena fdelapena added this to the 0.5.0 milestone Mar 12, 2016
@BlisterB
Member

That would be really nice to avoid the noise problem :).

@Zegeri
Member
Zegeri commented Mar 13, 2016

It could also add MP3 playback in the OpenAL backend (sndfile doesn't support it).

@Ghabry
Member
Ghabry commented Mar 13, 2016

How can this be implemented for SDL2? Is there any API in SDL_(mixer) to inject additional audio?

@Zegeri
Member
Zegeri commented Mar 13, 2016

Yes, Mix_HookMusic.

@fdelapena
Member

There are several examples about using SDL_MixAudio in the callback used by Mix_HookMusic.
This one uses libmad: SDL_mad.c.

@Ghabry
Member
Ghabry commented Mar 14, 2016

Ah okay, that's great! I only checked the documentation of Mix_HookAudio and I had no idea what the purpose of that is.

@fdelapena
Member

Some initial code for testing, it works with any frequency and will work with proper speed in Wii ๐Ÿ‘.

Tested with songs of Space Funeral, the Killer Instinct song from the HOME game and the usual noisy 48 KHz MP3 files. All cases worked without issues.

Default code will loop the song indefinitely, stop it with ctrl+c or uncomment the couple of lines from musicFinished().

#include "SDL.h"
#include "SDL_mixer.h"
#include <mpg123.h>
#include <stdio.h>

int musicPlaying;
mpg123_handle *handle = NULL;
int err = MPG123_OK;

void musicFinished() {
    printf("Music finished\n");
    /*Mix_HookMusic(NULL, NULL);*/
    /*musicPlaying = 0;*/
}

void play_mp3(void *udata, Uint8 *stream, int len) {
    Uint8 *buffer = malloc(len);
    size_t done = 0;
    (void)udata; /* Unused yet, silence the compiler */

    do err = mpg123_read(handle, buffer, len, &done);
        while (done && err != MPG123_OK);
    if (err != MPG123_DONE)
        SDL_MixAudio(stream, buffer, len, SDL_MIX_MAXVOLUME);
    else {
        mpg123_seek(handle, 0, SEEK_SET);
        musicFinished();
    }
    free(buffer);
}

int main(int argc, char *argv[]) {
    if (argc < 2) {
        printf("Usage: %s filename.mp3\n", argv[0]);
        return 1;
    }
    SDL_Init(SDL_INIT_AUDIO);
    Mix_OpenAudio(44100, MIX_DEFAULT_FORMAT, 2, 2048);
    mpg123_init();
    handle = mpg123_new(NULL, &err);
    mpg123_format_none(handle);
    mpg123_format(handle, 44100, 2, MPG123_ENC_SIGNED_16);
    mpg123_open(handle, argv[1]);

    Mix_HookMusic(play_mp3, NULL);
    musicPlaying = 1;
    while (musicPlaying) {
        SDL_Delay(200);
    }
    mpg123_close(handle);
    mpg123_delete(handle);
    mpg123_exit();
    Mix_Quit();
    SDL_Quit();
    return 0;
}

You can build it with:

gcc mp3player.c -o mp3player `pkg-config --cflags --libs SDL2_mixer libmpg123`

Usage:

./mp3player filename.mp3

This code has been merged in Ghabry's audio branch.

@fdelapena
Member

Because Mix_HookMusic seems to work properly, I've integrated fmmidi (#752), too.

@Zegeri I'm not familiar with the OpenAL API, feel free to provide an equivalent implementation or a minimal test case to start with.

@Ghabry
Member
Ghabry commented Mar 18, 2016

Very cool, and so simple ๐Ÿ‘

@fdelapena fdelapena assigned BlisterB and fdelapena and unassigned BlisterB Mar 29, 2016
@fdelapena
Member

The Mix_HookMusic documentation says about pause and resume working, but refers to paused music in the native player before using the custom HookMusic player. After stopping the HookMusic and returning the control to the native player it would allow to resume the previous native music player paused. In short, we need to handle our own pause and resume too.

Apart of pause and resume and the obvious play/stop, we also will need to handle fade in/out, volume and set the corresponding status flags.

I've updated the code example, now it will loop music and will print a message when the song is looping (finished). Useful for loop and gapless tests (there are mp3 files with bad gaps).

Suggested MP3 file for loop testing: Monigote Fantasy's "Music/Tiny Toon Adventures - Special.mp3", this one is short, looks fine and feels gapless, compared to SDL_mixer's playmus program ๐Ÿ‘.

@Ghabry
Member
Ghabry commented Mar 30, 2016

Is really lazy from the SDL mixer developers to not support Pause and Resume.
Do you tell us that Fade in/out and volume are also not supported???

Shame that the RWOps Api is so limited (concerning using buffers, whole file must be decoded already).
Because then you could create a WAV stream in memory, point a RWOps to it and pass this through the WAV code of SDL ;)...

@Ghabry
Member
Ghabry commented Mar 30, 2016

Just got an idea how to abuse RWOps to do this for us :P
Only need to emit a fake WAV header and the PCM file size (if it's possible to calculate this)

Will write some code later to test this :D

@fdelapena
Member

If you want to use RWOps, there is mpg123_open_fd() and other ways to load the file.

@carstene1ns
Member

๐Ÿ‘ for using the native library api instead of implementing even uglier workarounds than we have currently (ghabry stahp /meme). How about adding an additional fade_volume variable and use that while mixing, there is a function in sdl(_mixer) for that. Then we just need to take care of changing that variable and handling of pause and stop is also not really black magic.

@fdelapena fdelapena assigned Ghabry and unassigned fdelapena Mar 31, 2016
@Ghabry
Member
Ghabry commented Mar 31, 2016

Well as pointed out by fdelapena the Mixer API supports volume changes so my suggestion RWops is not necessary ;)

@Ghabry
Member
Ghabry commented Apr 8, 2016

Same here @fdelapena or @carstene1ns.
Though slightly different: MP3 support in SDL is just broken, so I suggest enabling this by default when mpg123 was found, otherwise compile without it.

I need a way to detect mpg123 and a flag to disable it if somebody wants to enjoy smpeg2 or libmad, probably "--disable-mpg123".

Do you know how to print a summary at the end of the configure step like some libraries do?
A warning to maintainers that SDL mp3 support is of questionable quality when mpg123 is not found would be useful.

@fdelapena
Member

There is already a --disable-mpg123 option, however it forces to disable it explicitly to fallback to SDL_mixer's libmad, otherwise it won't build. This is a temporal solution, though.

As commented with @carstene1ns some days ago, a proper way would be a final summary table when configure script ends, then show some warnings about what will happen if using libmad instead mpg123, otherwise we will get invalid bug reports from people not reading the discreet warnings appearing between lines.

Having a final summary is relevant, already happened with some users building Player and forgetting to install SDL2 devel libs and building with SDL1.2 instead, then complaining about wrong music speed.

For the optional dependency it is explained at https://autotools.io/autoconf/arguments.html in the section 3.2.

@carstene1ns
Member

๐Ÿ‘ for automagic dependencies and a summary table!

btw. I already added one for the converter tools.
It is basically shell syntax and interpolates variables.

@Ghabry Ghabry modified the milestone: 0.5.0, 0.4.2 Apr 11, 2016
@fdelapena fdelapena closed this in #873 May 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment