We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

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

(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!

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.

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

Created attachment 4043 [details]
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.

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

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) ?

> 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)`

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.

(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?

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! :)

(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.

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.

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.

(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

(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.

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

(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!