Added Vitaly Novichkov and Michael Day to CC list (they are authors
of the loop code.)

One thing you messed up: you made a memory leak: you forgot to clean-up the "param" in "break".

P.S. In MixerX I shouldn't break, because together with loop I also have to parse other tags such as title, artist, album and copyright for the tag retrieving API.

Anyway, about FLAC, I told some time ago that it may have a bigger size than max signed value.

However, after small computing:

The max signed value is 18446744073709551615. Divide it with 44100 to get a count of seconds: ~418293516410647. So, it allows storing 1,33x10^7 years which is totally unreal file size. So, it's should be fine with signed.

Created attachment 4115 [details]
loop.patch

> One thing you messed up: you made a memory leak: you forgot to
> clean-up the "param" in "break".

Attaching updated patch which doesn't leak memory.

(In reply to Vitaly Novichkov from comment #3)
> count of seconds: ~418293516410647. So, it allows storing 1,33x10^7 years
> which is totally unreal file size. So, it's should be fine with signed.

Indeed.

Michael Day:  What do you think?  (Flac loop code was yours.)

Also, be careful with SDL_atof() and SDL_strtod(), they have oddness:

- when a debugger is detached, they are using my local comma (comma a s a decimal separator like "1,2" is "1.2")
- when I attaching a debugger, it, by a wonder, works correctly

So, look a result without debugger:
ToF: 0 from str: .577
ToF: 0 from str: .091

And with debugger:
ToF: 0,577 from str: .577
ToF: 0,091 from str: .091

In MixerX I have in "utils.c" the workaround "str_to_float()" which allows to handle such cases.

In general I think this an excellent improvement. If you are in a hurry I am okay with going ahead and applying this patch (I see nothing obviously wrong based on a simple review of the code) but if you are okay to wait a bit I will test this out tonight.

(In reply to Michael Day from comment #7)
> In general I think this an excellent improvement. If you are in a hurry I am
> okay with going ahead and applying this patch (I see nothing obviously wrong
> based on a simple review of the code) but if you are okay to wait a bit I
> will test this out tonight.

I'll wait for you to test it.

(In reply to Ozkan Sezer from comment #8)
> (In reply to Michael Day from comment #7)
> > In general I think this an excellent improvement. If you are in a hurry I am
> > okay with going ahead and applying this patch (I see nothing obviously wrong
> > based on a simple review of the code) but if you are okay to wait a bit I
> > will test this out tonight.
> 
> I'll wait for you to test it.

I found one issue. In each of the *CreateFromRW functions, loop_start is initialized to -1. This causes looping to break. I think it should instead be initialized to 0.

Created attachment 4116 [details]
loop1.patch

> I found one issue. In each of the *CreateFromRW functions, loop_start is
> initialized to -1. This causes looping to break. I think it should instead
> be initialized to 0.

OK good catch. New patch attached which initializes music->loop to 0, and
not -1.

Doing that also allows simplifying the final loop decision. The interdiff
between the old and new patches is the like following,  same in all three
files in CreateFromRW:
     music->loop = -1;
-    music->loop_start = -1;
+    music->loop_start = 0;
     music->loop_end = 0;
     music->loop_len = 0;

and:

-    if (((music->loop_start >= 0) || (music->loop_end > 0)) &&
-        ((music->loop_start < music->loop_end) || (music->loop_end > 0)) &&
-         (music->loop_start < full_length) &&
-         (music->loop_end <= full_length)) {
-        if (music->loop_start < 0) music->loop_start = 0;
+    if ((music->loop_end > 0) && (music->loop_start < music->loop_end)
+        (music->loop_start < full_length) && (music->loop_end <= full_length)) {
         if (music->loop_end == 0)  music->loop_end = full_length;
         music->loop = 1;
     }


Please review.  OK to apply?

Created attachment 4117 [details]
loop2.patch -- fixed, v2

Bad day, perhaps.  Here is a version that actually builds.
Changes after the original:
- removes all music->loop??? initializers from CreateFromRW:
  music is allocated using calloc().
- simplifies the final loop decision, as showed in previous
  comment.


Please review / test.  OK to apply?

Going to test/review and backport this into MixerX at evening of UTC+3

I think the big if statement can be reduced further to something like:

if ((music->loop_end > 0) && (music->loop_end <= full_length) &&
    (music->loop_start < music->loop_end)) {
    music->loop = 1;
}

Basically, if:

1. loop_end is positive AND
2. loop_end occurs before the end of the track AND
3. loop_start occurs before loop_end (which implies that loop_start occurs before the end position because of condition #2)

...then the loop points are valid and we can use them for looping.

What do you think?

Created attachment 4119 [details]
loop3.patch -- fixed, further simplified

(In reply to Michael Day from comment #13)
> I think the big if statement can be reduced further to something like:
> 
> if ((music->loop_end > 0) && (music->loop_end <= full_length) &&
>     (music->loop_start < music->loop_end)) {
>     music->loop = 1;
> }
> 
> Basically, if:
> 
> 1. loop_end is positive AND
> 2. loop_end occurs before the end of the track AND
> 3. loop_start occurs before loop_end (which implies that loop_start occurs
> before the end position because of condition #2)
> 
> ...then the loop points are valid and we can use them for looping.
> 
> What do you think?

Oh yes.  Will apply the attached version shortly.

Applied the patch from comment #14, here:
https://hg.libsdl.org/SDL_mixer/rev/a3c637d1a698

Thanks for the reviews and suggestions. If there are
any issues with the change, or improvements can be made
to it, please drop a note here.  Closing as fixed.

Closing as fixed.