Fix some milkyplay bugs/crashes

exelotl commented 4 years ago

Hi! I've been trying to use milkyplay as a library and ran into several issues. I put together a minimal example using SDL2:

#include "SDL.h"
#include "MilkyPlay.h" 
#include "AudioDriver_SDL.h"

int main(int argc, char *argv[]) {

    SDL_Init(SDL_INIT_EVERYTHING);

    SDL_Window *window = SDL_CreateWindow(
        "MilkyPlay Test",
        SDL_WINDOWPOS_UNDEFINED, SDL_WINDOWPOS_UNDEFINED,
        800, 480, SDL_WINDOW_SHOWN
    );
    SDL_Renderer *renderer = SDL_CreateRenderer(window, -1, SDL_RENDERER_SOFTWARE);

    AudioDriver_SDL driver;
    PlayerGeneric player(44100, &driver);
    XModule mod;

    if (mod.loadModule("Goodboy4.xm") != MP_OK) {
        printf("Could not open .xm file.\n");
        return 1;
    }
    if (player.startPlaying(&mod) != MP_OK) {
        printf("Failed to start playing!\n");
        return 1;
    }

    while (true) {
        SDL_Event e;
        while (SDL_PollEvent(&e)) {
            if (e.type == SDL_QUIT) {
                goto Exit;
            }
        }
        SDL_SetRenderDrawColor(renderer, 0, 0, 0, 255);
        SDL_RenderClear(renderer);
        SDL_RenderPresent(renderer);
        SDL_Delay(20);
    }

    Exit:
    player.stopPlaying();
    SDL_DestroyRenderer(renderer);
    SDL_DestroyWindow(window);
    SDL_Quit();
    return 0;
}

Literally so many things went wrong, I can only wonder if I somehow missed a crucial step and forced the library down some untested code path. Or if simply nobody ever ran this code outside of MilkyTracker, where things are set up differently.

1. Initialisation fails with "SDL: Audio driver doesn't support 16-bit signed samples!"

AudioDriver_SDL has several checks which seem counterproductive. Many computers won't give the desired signed 16-bit format.

We can pass NULL as the second argument to SDL_OpenAudio, to force SDL to emulate the desired spec even if it's not natively available. Instead I opted to use SDL_OpenAudioDevice which gives control over which parts of the spec are allowed to change.

2. Mixer inadvertently closes when setBufferSize() is called during initialisation?

Now we call startPlaying(), but nothing happens! I traced it down to here:

mp_sint32 ChannelMixer::setBufferSize(mp_uint32 bufferSize)
{
    if (this->mixBufferSize == bufferSize)
        return MP_OK;

    if (initialized)
    {
        mp_sint32 err = closeDevice();
        if (err != MP_OK)
            return err;
    }
    ...

Seems like initDevice() was already called by the time this code runs, so the player gives up. Frustratingly, this occurs inside a listener, so the failure is never reported and startPlaying() returns MP_OK even though everything is very not OK!

I could not find a suitable earlier place to call setBufferSize(), but deleting the whole if (initialized) block seems to fix the problem.

3. Mixer channels are uninitialised

The app will segfault while mixing. At first I managed to fix it by commenting out the calls to storeTimeRecordData() in ChannelMixer.cpp. But it seems like I just got lucky, because similar crashes happen further down the line.

According to the debugger, the mixer has 32 channels while my song only has 8? If the remaining channels are uninitialised, that would explain why it crashes on the 9th element, and chn->sample gets a rather suspect value of 0xabababababababab on my machine.

I finally fixed this by making it so that setNumChannels() will change the value of mixerNumActiveChannels. Now everything seems to be working! But I'm not confident it won't fall over as soon as I want to add or remove channels...

4. Bug in `GenericPlayer` destructor

Ah this one was funny. It tries to use the mixer immediately after deleting it! Easy fix :)

Hope these changes are all OK! Am eager for comments on anything I missed or should have done differently.

Deltafire commented 4 years ago

Hi exelotl,

Thanks for your PR and excellent write up. Milkyplay has been used outside of Milkytracker for a few projects but mostly it's been developed to support Milkytracker.

Do you mind if I include your example code? It could be useful for someone else in the future.

exelotl commented 4 years ago

Sure, you can use that code wherever, no problem!

One of the reasons I've been working with milkyplay is because I'd love to have a tracker running on the 3DS. However a direct port of MilkyTracker isn't quite what I'm after - I used NitroTracker on my DS many years ago and I think they absolutely nailed the user interface, so I'd like to try and replicate that where possible.

I did manage to get a 3DS audiodriver working. But I think what I had in mind is weeks or months of work, and I have a lot of other things on my plate right now. Ah well, maybe it's something I can pick up in the future...

milkytracker / MilkyTracker