[PATCH] Add looping support for FLAC

[SDLBugzilla]

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• Add looping to music_flac.c (music_flac_add_looping.patch, text/plain, 2019-11-11 14:32:46 +0000, 10311 bytes)
• Version 2 - Add looping to music_flac.c (music_flac_add_looping_v2.patch, text/plain, 2019-11-12 03:06:20 +0000, 10292 bytes)
• Version 3 - Add looping to music_flac.c (v3_music_flac_add_looping.patch, text/plain, 2019-11-12 14:13:51 +0000, 10546 bytes)
• Replace str_to_int64 with SDL_strtoull(x, NULL, 10) (984-ogg-sdl-strtull10.patch, text/plain, 2019-11-17 14:28:33 +0000, 2045 bytes)

Reported in version: 2.0.4
Reported for operating system, platform: All, All

Comments on the original bug report:

On 2019-11-11 14:32:46 +0000, Michael Day wrote:

Created attachment 4039
Add looping to music_flac.c

Attached is a patch for music_flac.c which adds looping support for FLAC files. It mirrors the behavior and interface of the OGG looping. When loop tags LOOPSTART, LOOPEND and/or LOOPLENGTH are present in the FLAC Vorbis comment metadata, the module will use those specified values when looping the file. (It also accepts LOOP-* and LOOP_*.) Loop points can be expressed as raw PCM sample positions or as time strings of the form HH:MM:SS.ss.

On 2019-11-11 15:09:51 +0000, Ozkan Sezer wrote:

Is http://hg.libsdl.org/SDL_mixer/rev/b2b18baf65d9 (i.e. bug # 4853)
needed for this too?

On 2019-11-11 17:37:36 +0000, Michael Day wrote:

(In reply to Ozkan Sezer from comment # 1)

Is http://hg.libsdl.org/SDL_mixer/rev/b2b18baf65d9 (i.e. bug # 4853)
needed for this too?

This looks like a sensible addition. When I get a chance I'll update the patch to incorporate this. Thanks!

On 2019-11-12 03:04:47 +0000, Michael Day wrote:

Question: Could we accomplish the same thing by telling strtoull that the number is base 10? Right now the code passes 0 for the base argument which tells strtoull to determine the base from the prefix. If there's a leading 0, it will interpret the number as octal which would likely be wrong. If you explicitly specify that it's base 10 however, it should correctly parse the leading zeros.

We simply change the function calls from:

SDL_strtoull(value, NULL, 0)

to:

SDL_strtoull(value, NULL, 10)

I tested it out a bit and it seems to work as expected. Also, strtoull appears to correctly handle whitespace and trailing non-numeric characters. For example, if the number string is " 000100abc" and is specified as decimal it will interpret that as 100.

See my updated patch v2 for code implemented this way. Also I cleaned up some formatting so it matches the SDL style.

On 2019-11-12 03:06:20 +0000, Michael Day wrote:

Created attachment 4040
Version 2 - Add looping to music_flac.c

On 2019-11-12 14:13:51 +0000, Michael Day wrote:

Created attachment 4043
Version 3 - Add looping to music_flac.c

I found a bug which could cause the first repeat instance to be glitchy. This is now fixed in Version 3 of the patch.

On 2019-11-17 06:12:43 +0000, Sam Lantinga wrote:

Patch added, thanks!
https://hg.libsdl.org/SDL_mixer/rev/e5b4ff96668b

On 2019-11-17 07:31:43 +0000, Ozkan Sezer wrote:

Should we remove str_to_int64() stuff in music_ogg.c and
replace them with SDL_strtoull(value, NULL, 10) as it is
done here (see comment # 3) ?

On 2019-11-17 13:31:52 +0000, Vitaly Novichkov wrote:

Should we remove str_to_int64() stuff in music_ogg.c and
replace them with SDL_strtoull(value, NULL, 10) as it is
done here (see comment # 3) ?

I should verify this, and yeah, if it will work, then, give (ogg_int64_t)SDL_strtoull(value, NULL, 10)

On 2019-11-17 13:43:32 +0000, Vitaly Novichkov wrote:

Okay, look the difference:

A sample code:

#include <SDL2/SDL.h>
#include <stdio.h>

static const char *ku[] =
{
    "123461",
    "000324234",
    "heck312rg28342",
    "314135135000310135",
    "00000000000000000000013513",
    "000003303333305ff1"
};

static Uint64 str_to_int64(const char *param)
{
    char *copy = SDL_strdup(param);
    char *front = copy;
    char *back;
    Uint64 ret;

    /* Find digit between of 1 and 9 at begin */
    while (*front != '\0' && (*front < '1' || *front > '9')) {
        front++;
    }
    /* Find any non-digit character or NULL */
    back = front;
    while (*back != '\0' && (*back >= '0' && *back <= '9')) {
        back++;
    }
    *back = '\0';
    ret = SDL_strtoull(front, NULL, 0);
    SDL_free(copy);
    return ret;
}

int main()
{
    size_t i = 0;
    for(i = 0; i < sizeof(ku) / sizeof(const char*); i++)
        printf("%s:\n -> %lu\n -> %lu\n", ku[i], SDL_strtoull(ku[i], NULL, 10), str_to_int64(ku[i]));
    return 0;
}

and it's output:

123461:
 -> 123461
 -> 123461
000324234:
 -> 324234
 -> 324234
heck312rg28342:
 -> 0
 -> 312
314135135000310135:
 -> 314135135000310135
 -> 314135135000310135
00000000000000000000013513:
 -> 13513
 -> 13513
000003303333305ff1:
 -> 3303333305
 -> 3303333305

The SDL_strtoull() fails to parse the heck312rg28342 and it can't extract "312" from off the garbage and returns 0. Shouldn't be critical as the sort of garbage like this shouldn't be valid at all.

On 2019-11-17 14:01:19 +0000, Ozkan Sezer wrote:

(In reply to Vitaly Novichkov from comment # 9)

The SDL_strtoull() fails to parse the heck312rg28342 and it can't extract
"312" from off the garbage and returns 0. Shouldn't be critical as the sort
of garbage like this shouldn't be valid at all.

OK, we leave this as is. For ogg side: Should we revert
https://hg.libsdl.org/SDL_mixer/rev/b2b18baf65d9 and use
the 10-base with strtoull method here?

On 2019-11-17 14:28:33 +0000, Vitaly Novichkov wrote:

Created attachment 4055
Replace str_to_int64 with SDL_strtoull(x, NULL, 10)

Yes, it can. I made a test on my side, and it works fine. So, the workaround is no more needed! Thanks for finding a true solution! :)

On 2019-11-17 14:51:13 +0000, Ozkan Sezer wrote:

(In reply to Vitaly Novichkov from comment # 11)

Created attachment 4055 [details]
Replace str_to_int64 with SDL_strtoull(x, NULL, 10)

Yes, it can. I made a test on my side, and it works fine. So, the workaround
is no more needed! Thanks for finding a true solution! :)

Applied your patch: https://hg.libsdl.org/SDL_mixer/rev/48f61cd20cdd
Thanks.

On 2019-11-17 16:02:08 +0000, Vitaly Novichkov wrote:

Okay, just now I have found in the code one of C90 violations:

for (int i = 0; i < vc->num_comments; ++i) {

The int i is incompatible with C90. It's the feature of C11.

On 2019-11-17 16:13:09 +0000, Vitaly Novichkov wrote:

Another case:

This code

        if (is_loop_length) {
            music->loop_end = music->loop_start + music->loop_len;
        } else {
            music->loop_len = music->loop_end - music->loop_start;
        }

should be placed after vorbis comments fetching.

On 2019-11-17 16:16:41 +0000, Ozkan Sezer wrote:

(In reply to Vitaly Novichkov from comment # 13)

Okay, just now I have found in the code one of C90 violations:
for (int i = 0; i < vc->num_comments; ++i) {

Fixed: https://hg.libsdl.org/SDL_mixer/rev/229b6f0fa03f

On 2019-11-17 16:17:49 +0000, Ozkan Sezer wrote:

(In reply to Vitaly Novichkov from comment # 14)

Another case:

This code

        if (is_loop_length) {
            music->loop_end = music->loop_start + music->loop_len;
        } else {
            music->loop_len = music->loop_end - music->loop_start;
        }

should be placed after vorbis comments fetching.

Michael Day: that one is for you.

On 2019-11-17 16:21:47 +0000, Vitaly Novichkov wrote:

I meant, place this code after loop, not inside the loop

On 2019-11-17 19:41:49 +0000, Michael Day wrote:

(In reply to Vitaly Novichkov from comment # 14)

Another case:

This code

        if (is_loop_length) {
            music->loop_end = music->loop_start + music->loop_len;
        } else {
            music->loop_len = music->loop_end - music->loop_start;
        }

should be placed after vorbis comments fetching.

Firstly thank you for taking the time to review this code. I will watch out for C99-isms in the future. :)

Regarding the if (is_loop_length) block, I currently have this located at the end of vorbis comment case in the flac_music_metadata_cb callback. As far as I can tell I am running this code after fetching all of the vorbis comments. I'm confused about where exactly you want me to move it. Can you clarify please? Thanks!