[audio] Music looping based on FPS issue

[Kabcorp]

• I tested it on latest raylib version from master branch
• I checked there is no similar issue already reported
• My code has no errors or misuse of raylib

Issue description

Music looping is based on FPS, which is odd in 99% of time.
With this approach, if the music reach the end of its own length, you have to wait the next frame to restart looping.

It was tried of course using WAV pcm file with basic code:

Environment

Windows 10, mingw_w64, GTX1050 3gB
Provide your Platform, Operating System, OpenGL version, GPU details where you experienced the issue.

Issue Screenshot

Code Example

(sfx.wav is downloadable here: http://kabcorp.fr/sendfiles/ray/sfx.wav)
sfx.wav is a pure sine wave with correct repeat sample.

#include <raylib.h>

int main(void)
{
    int Fps=30;
    InitWindow(700, 200, "Music Loop Issue");
    SetTargetFPS(Fps);

    InitAudioDevice();

    Music Sfx=LoadMusicStream("sfx.wav");
    PlayMusicStream(Sfx);

    while (!WindowShouldClose())
    {
            UpdateMusicStream(Sfx);

            Fps+=(IsKeyPressed(KEY_RIGHT)-IsKeyPressed(KEY_LEFT))*10;
            Fps=Fps < 10 ? 10 : Fps;
            SetTargetFPS(Fps);

            BeginDrawing();
            ClearBackground(GRAY);

            DrawText(TextFormat("(Press L/R to change FPS)\n%i/%i fps", GetFPS(), Fps), 8, 8, 20, LIGHTGRAY);

            EndDrawing();
    }

    CloseWindow();
    UnloadMusicStream(Sfx);
    return 0;
}

[raysan5]

@Kabcorp Yes, I'm aware of this issue, raudio module needs some improvements on that regards. There are some open PRs on that line but not completely reviewed yet.

[jdeokkim]

Could you tell me if this issue is still being worked on?

[raysan5]

@jdeokkim this issue is open but I'm not working on it right now. Feel free to work on it and send a PR if you wish.

[jdeokkim]

@raysan5 Oh, I see. Thank you!

[raysan5]

@mackron Please, could you take a look to this issue? I tried to review UpdateMusicStream() to implement a solution but it doesn't work and I think I'm missing something.

What I tried: If the frames left are less than buffer size, I append to them the required frames from the beginning of the file. As per my understanding it should work but it doesn't. I did many tries and I feel I'm probably missing something...

Current implementation seems to kind of work but it's wrong.

Here the code I used to test:

#include "raylib.h"

//------------------------------------------------------------------------------------
// Program main entry point
//------------------------------------------------------------------------------------
int main(void)
{
    // Initialization
    //--------------------------------------------------------------------------------------
    const int screenWidth = 800;
    const int screenHeight = 450;

    InitWindow(screenWidth, screenHeight, "raylib [audio] example - music playing (streaming)");

    InitAudioDevice();              // Initialize audio device

    Music music = LoadMusicStream("resources/sfx.wav");
    music.looping = true;

    PlayMusicStream(music);

    float timePlayed = 0.0f;        // Time played normalized [0.0f..1.0f]
    bool pause = false;             // Music playing paused

    SetTargetFPS(30);               // Set our game to run at 60 frames-per-second
    //--------------------------------------------------------------------------------------

    // Main game loop
    while (!WindowShouldClose())    // Detect window close button or ESC key
    {
        // Update
        //----------------------------------------------------------------------------------
        UpdateMusicStream(music);   // Update music buffer with new stream data

        // Get normalized time played for current music stream
        timePlayed = GetMusicTimePlayed(music)/GetMusicTimeLength(music);

        if (timePlayed > 1.0f) timePlayed = 1.0f;   // Make sure time played is no longer than music
        //----------------------------------------------------------------------------------

        // Draw
        //----------------------------------------------------------------------------------
        BeginDrawing();

            ClearBackground(RAYWHITE);

            DrawText("MUSIC SHOULD BE PLAYING!", 255, 150, 20, LIGHTGRAY);

            DrawRectangle(200, 200, 400, 12, LIGHTGRAY);
            DrawRectangle(200, 200, (int)(timePlayed*400.0f), 12, MAROON);
            DrawRectangleLines(200, 200, 400, 12, GRAY);

            DrawText("PRESS SPACE TO RESTART MUSIC", 215, 250, 20, LIGHTGRAY);
            DrawText("PRESS P TO PAUSE/RESUME MUSIC", 208, 280, 20, LIGHTGRAY);

        EndDrawing();
        //----------------------------------------------------------------------------------
    }

    // De-Initialization
    //--------------------------------------------------------------------------------------
    UnloadMusicStream(music);   // Unload music stream buffers from RAM

    CloseAudioDevice();         // Close audio device (music streaming is automatically stopped)

    CloseWindow();              // Close window and OpenGL context
    //--------------------------------------------------------------------------------------

    return 0;
}

Here the sound file I use for testing: sfx.zip

If you think the implementation cost does not worth it let me know. I can skip this issue.

[mackron]

I've had a look at this and I'm not actually sure what's going on. When I tried it, I also had another glitch with wav files which I'm not sure is related.

I'm strongly of the opinion that the audio system needs a complete overhaul. And by that I mean both the implementation and the public API. Generally, these are the high level things I think you need to change.

• You have a bunch of separate code paths for loading different file formats. Just use ma_decoder from miniaudio. For formats that are not supported natively by miniaudio, plug in a custom decoder. That way all loading is done in a unified manner via ma_decoder_*(). This would certainly simplify loading code and make it easier to debug.
• There are separate objects for "AudioStream" and "Music" and "Sound". When you break them down they're all the same thing - they're just sounds. The only difference is how data is delivered. Just have a unified "Sound" API and get rid of the other two. You can still have functions for initializing the "Sound" object as a stream (maybe LoadSoundStream() or something) which will enable a function like UpdateAudioStream().
• You're still using the low-level API and doing mixing yourself. You could simplify the code significantly by just using ma_engine and let miniaudio do the mixing for you. I think it's just this way because you started using miniaudio way before the high level stuff came out.

If you're willing to do a complete rewrite of the audio system (which I think you should seriously consider), I'd be happy to do a fresh implementation with you. My preference would be to do a complete redesign of the audio API though which would obviously break compatibility, but I have ideas that I think could make the audio system even simpler for the user and at the same time make it more powerful. Happy to talk further about that if you're interested.

[raysan5]

Hi David! Thank you very much for taking a look to this issue! I agree that the audio system needs a review, I know miniaudio has grown a lot in the last years and provides lots of high-level features but redesigning the public API will be a breaking change I prefer to avoid at this moment; at least not intended for the upcoming raylib 4.2 release.

You have a bunch of separate code paths for loading different file formats. Just use ma_decoder from miniaudio. For formats that are not supported natively by miniaudio, plug in a custom decoder. That way all loading is done in a unified manner via ma_decoder_*(). This would certainly simplify loading code and make it easier to debug.

Current implementation discretizes the different file formats loading using SUPPORT_FILEFORMAT_* and it keeps the data and/or the context stored in the structure, it would be great to remove that context, at least from user exposure, how would it be using ma_decoder approach? I like from current implementation the transparency of every file format, seeing the libraries used for every format.

There are separate objects for "AudioStream" and "Music" and "Sound". When you break them down they're all the same thing - they're just sounds. The only difference is how data is delivered. Just have a unified "Sound" API and get rid of the other two. You can still have functions for initializing the "Sound" object as a stream (maybe LoadSoundStream() or something) which will enable a function like UpdateAudioStream().

Current approach uses Sound and Music data types, both include a AudioStream type that implements the AudioBuffer. The structs are simple and just expose the basic elements to the user, also considering they are mostly passed by value, that's a common approach in all raylib functions. I would like to maintain the current API (or most of it) but maybe it will be possible to jsut have AudioStream and make the Sound and Music types an alias of it, what do you think?

You're still using the low-level API and doing mixing yourself. You could simplify the code significantly by just using ma_engine and let miniaudio do the mixing for you. I think it's just this way because you started using miniaudio way before the high level stuff came out.

Agree. That would be a nice improvement. As per my understanding this is mostly an internal change, right?

My preference would be to do a complete redesign of the audio API though which would obviously break compatibility, but I have ideas that I think could make the audio system even simpler for the user and at the same time make it more powerful. Happy to talk further about that if you're interested.

Sure! At this moment I'm targeting raylib 4.2 release and I'm a bit reticent of API breaking changes but I want to hear your ideas and we can see how we can redesign the API for a future version.

[mackron]

Current implementation discretizes the different file formats loading using SUPPORT_FILEFORMAT_* and it keeps the data and/or the context stored in the structure, it would be great to remove that context, at least from user exposure, how would it be using ma_decoder approach? I like from current implementation the transparency of every file format, seeing the libraries used for every format.

The way it would work is the context would be a ma_decoder instead of drwav, drmp3, etc. The MusicContextType enum would no longer be necessary for discriminating against the different types. For formats that are not supported internally by miniaudio, such as XM and MOD, a custom decoder can be implemented and plugged into miniaudio. This is a much cleaner separation of interests and will keep the code much easier to maintain.

For example, in UpdateMusicStream(), you have code that looks like this:

switch (music.ctxType)
{
#if defined(SUPPORT_FILEFORMAT_WAV)
    case MUSIC_AUDIO_WAV:
    {
    } break;
#endif
#if defined(SUPPORT_FILEFORMAT_OGG)
    case MUSIC_AUDIO_OGG:
    {
    } break;
#endif
#if defined(SUPPORT_FILEFORMAT_FLAC)
    case MUSIC_AUDIO_FLAC:
    {
    } break;
#endif
#if defined(SUPPORT_FILEFORMAT_MP3)
    case MUSIC_AUDIO_MP3:
    {
    } break;
#endif
#if defined(SUPPORT_FILEFORMAT_XM)
    case MUSIC_MODULE_XM:
    {
    } break;
#endif
#if defined(SUPPORT_FILEFORMAT_MOD)
    case MUSIC_MODULE_MOD:
    {
    } break;
#endif
    default: break;
}

That entire section would be replaced with a call to ma_decoder_read_pcm_frames():

ma_decoder_read_pcm_frames((ma_decoder*)music.ctxData, frameCountToStream, (float*)AUDIO.System.pcm);

It's just so much cleaner. No #if blocks and no need for discriminating against the file type and then having separate branching logic everywhere. It'll also just make it so much easier to reason with when debugging. You can even use miniaudio to handle looping for you as well.

Current approach uses Sound and Music data types, both include a AudioStream type that implements the AudioBuffer. The structs are simple and just expose the basic elements to the user, also considering they are mostly passed by value, that's a common approach in all raylib functions. I would like to maintain the current API (or most of it) but maybe it will be possible to jsut have AudioStream and make the Sound and Music types an alias of it, what do you think?

Could be possible, but I haven't really looked at the code in enough detail. Certainly if you couldn't make it an alias, you could simplify them a bit. There's something that feels wrong about the double buffering system that's currently being used - I think that needs to be re-written and simplified, and I have an idea for that. I'm suspecting that has something to do with the error in this bug report.

But yes, there's still a bunch we could do to improve the code while keeping the same API. They way I would approach it is to first do that unified decoding thing. After then once that's done I would transition away from the low-level API and start using the high level API so you can avoid having to do the mixing yourself.

Sure! At this moment I'm targeting raylib 4.2 release and I'm a bit reticent of API breaking changes but I want to hear your ideas and we can see how we can redesign the API for a future version.

We can talk on Discord about that if you want.

[raysan5]

The way it would work is the context would be a ma_decoder instead of drwav, drmp3, etc. The MusicContextType enum would no longer be necessary for discriminating against the different types. For formats that are not supported internally by miniaudio, such as XM and MOD, a custom decoder can be implemented and plugged into miniaudio. This is a much cleaner separation of interests and will keep the code much easier to maintain.

Agree. This approximation is more cleaner.

Could be possible, but I haven't really looked at the code in enough detail. Certainly if you couldn't make it an alias, you could simplify them a bit. There's something that feels wrong about the double buffering system that's currently being used - I think that needs to be re-written and simplified, and I have an idea for that. I'm suspecting that has something to do with the error in this bug report.

That's nice. I personally never liked the double-buffer approach.

They way I would approach it is to first do that unified decoding thing. After then once that's done I would transition away from the low-level API and start using the high level API so you can avoid having to do the mixing yourself.

That's sounds good for me. @mackron we can start working on it on the raudio repo if you want and once we have something good enough I will move it into raylib. Currently raudio.c is the same as raylib module and raudio.h implements the API exposed by raylib.h.

[raysan5]

This issue has been fixed but the raudio module needs a good redesign, as commented above.