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.

One note about music_mpg123.c changes: mpg123_framelength()
is available only in mpg123 >= 1.23.0. Can you not at least
add a fallback for mpg123_position(), so that the code can
still work with older libmpg123 versions (possibly down to
1.12.0)?

(In reply to Ozkan Sezer from comment #1)
> One note about music_mpg123.c changes: mpg123_framelength()
> is available only in mpg123 >= 1.23.0. Can you not at least
> add a fallback for mpg123_position(), so that the code can
> still work with older libmpg123 versions (possibly down to
> 1.12.0)?

It's the first time I've use mpg123, so I'm still learning how to use it,
but yes, I will try to add support for older mpg123

I'm currently in vacation and don't  have a lot of time with my compuer though,
So it might take me some time, but I'll definitively try to do that.

Thanks a lot for you review,

Matthias

Created attachment 3903 [details]
V2

V2 that should support older mpg123 versions.

Created attachment 3904 [details]
V3 (fixes V2 so it behaves at runtime)

Attached an updated V3:  It fixes your V2 so it behaves at runtime.
Otherwise libmpg123.so loading would have always failed if an old
version of the library was present.  Please check to make sure that
I didn't break anything.

Created attachment 3905 [details]
V4 (fixes V2 and V3 so it behaves at runtime)

Attached a further fix (V4):  Calls mpg123_scan() upon first file load.
This seems to ensure that mpg123_position() behaves: the mpg123_info()
call in old_mpg123_duration() didn't help with that (I got 0 duration)
and it isn't necessary now.

Created attachment 3907 [details]
V5: v4 + comments fixes

Thanks,

I've test v4 it work.

v5 improve some comment(I explain that duration is in us) and remove a commented printf.

ping :)

If you want I can add support for more formats ?

Thanks,
Matthias

Created attachment 4072 [details]
V6: rebased to current hg

Rebased the latest version of the patch to current hg.

If you can complete the patch by adding support for other
formats, it can be reviewed and merged.

Created attachment 4097 [details]
support wav,flac,ogg,opus.mpeg123 and timidity

I didn't add support for all codecs, because I don't know how to use some (example music_mad), also I didn't find how to test timidity, but the function I've use seems pretty explicit, so I've commit it anyway.

I've add support for codecs I was able to test with playmus.
But I guess they are the most used formats.

Should I try to support all codecs ?

Thanks,
Matthias,

(In reply to matthias gatto from comment #9)
> Created attachment 4097 [details]
> support wav,flac,ogg,opus.mpeg123 and timidity

Thanks.


> Should I try to support all codecs ?

You don't have to: Not all can be supported.

For codecs with no duration implemented:
- music_mikmod.c -> no such libmikmod api, so not possible
- music_mad.c -> possible, but requires work, possibly not worth it
- music_cmd.c -> not possible, AFAIK
- music_fluidsynth.c -> don't know whether it is possible
- music_nativemidi.c -> don't know whether it is possible
- music_modplug.c -> can be implemented using ModPlug_GetLength():
  see attached modplug_duration.patch

Timidity: seems correct to me.

Question about ogg vs opus:
- Is there a reason you use ov_raw_total and not ov_pcm_total for ogg
 like you use op_pcm_total in music_opus? Or vice versa? (didn't look
 at the docs, so just asking.)

* NOTE: music_opus reads op_pcm_total() result as full_length in
 OPUS_CreateFromRW(). You can use that like you did in music_flac?

* NOTE: Same full_length note for music_ogg, maybe?


Sam, Ryan:  What do you think?  Should we merge?

Created attachment 4098 [details]
duration support for music_modplug.c

I already has this and even more in MixerX - a fork or SDL_mixer. Now I am in process of backporting of new features from MixerX to Mixer. Will check out the code here to give somw opinion on that once I'm be able.

On MixerX side I returning -1 for codecs are don't support specific feature such as tell or fullTime(), or loop points (returning of loop points are used in my testing player to indicate looping area on a seek bar).

(In reply to Ozkan Sezer from comment #10)
> (In reply to matthias gatto from comment #9)
> > Created attachment 4097 [details]
> > support wav,flac,ogg,opus.mpeg123 and timidity
> 
> Thanks.
> 
> 
> > Should I try to support all codecs ?
> 
> You don't have to: Not all can be supported.
> 
> For codecs with no duration implemented:
> - music_mikmod.c -> no such libmikmod api, so not possible
> - music_mad.c -> possible, but requires work, possibly not worth it
> - music_cmd.c -> not possible, AFAIK
> - music_fluidsynth.c -> don't know whether it is possible
> - music_nativemidi.c -> don't know whether it is possible
> - music_modplug.c -> can be implemented using ModPlug_GetLength():
>   see attached modplug_duration.patch
> 
> Timidity: seems correct to me.
> 
> Question about ogg vs opus:
> - Is there a reason you use ov_raw_total and not ov_pcm_total for ogg
>  like you use op_pcm_total in music_opus? Or vice versa? (didn't look
>  at the docs, so just asking.)
> 
No particlar reason, I've made opus code after readind how seek work yesterday,
I've made ogg ~3 month ago.

Though I've just re-read opus header, op_pcm_total doc say that timestamps in Opus are fixed at 48 kHz, vorbis say nothing about that, so maybe it's safer to keep ogg the way I've made it first ?

> * NOTE: music_opus reads op_pcm_total() result as full_length in
>  OPUS_CreateFromRW(). You can use that like you did in music_flac?
> 
Done I'll push a patch V8 asap
> * NOTE: Same full_length note for music_ogg, maybe?
> 
Maybe if we can use ov_pcm_total, but here as the functions are used only in Duration, maybe it's better to keep it that way, so if thoses functions had to fail, loading the music would still work (I don't know how likely this could happen ?)
> 
> Sam, Ryan:  What do you think?  Should we merge?

Created attachment 4099 [details]
V8: better code in opus and merge music_modplug

V8: that megre all other patch and use full_length in opus instead of calling a 2nd time opus.op_pcm_total

(In reply to matthias gatto from comment #15)
> Created attachment 4099 [details]
> V8: better code in opus and merge music_modplug
> 
> V8: that megre all other patch and use full_length in opus instead of
> calling a 2nd time opus.op_pcm_total

OK, thanks.

Comments?  (See comment #14 for some discussion about implementing
the ogg duration.)

Merge?

(In reply to Ozkan Sezer from comment #16)
> (In reply to matthias gatto from comment #15)
> > Created attachment 4099 [details]
> > V8: better code in opus and merge music_modplug
> > 
> > V8: that megre all other patch and use full_length in opus instead of
> > calling a 2nd time opus.op_pcm_total
> 
> OK, thanks.
> 
> Comments?  (See comment #14 for some discussion about implementing
> the ogg duration.)
> 
> Merge?

I have one question: can you return double in seconds instead of int in milliseconds? 
- To be consistent with an existing `Mix_SetMusicPosition()` call which accepts double values
- Maybe you'll rename call into `Mix_GetMusicTotalTime`?

In MixerX I have it called as `Mix_GetMusicTotalTime` and it returns double in seconds:

https://github.com/WohlSoft/SDL-Mixer-X/blob/master/include/SDL_mixer_ext/SDL_mixer_ext.h#L771-L775

As I already said, I am in the process of submitting my features into mainstream Mixer. Here are details are shown: https://bugzilla.libsdl.org/show_bug.cgi?id=3905

Also, you better to put the "FullLength" call into Mix_MusicInterface structure after "Seek" rather in the bottom to give the consistence:

https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/music.h#L130

It's the private API, so, backward compatibility is not matters here.

(In reply to Vitaly Novichkov from comment #17)
> (In reply to Ozkan Sezer from comment #16)
> > (In reply to matthias gatto from comment #15)
> > > Created attachment 4099 [details]
> > > V8: better code in opus and merge music_modplug
> > > 
> > > V8: that megre all other patch and use full_length in opus instead of
> > > calling a 2nd time opus.op_pcm_total
> > 
> > OK, thanks.
> > 
> > Comments?  (See comment #14 for some discussion about implementing
> > the ogg duration.)
> > 
> > Merge?
> 
> I have one question: can you return double in seconds instead of int in
> milliseconds? 
> - To be consistent with an existing `Mix_SetMusicPosition()` call which
> accepts double values
Ok, I'll do that

> - Maybe you'll rename call into `Mix_GetMusicTotalTime`?
> 
Well Javascript and ffmpeg(libav) seems to use "duration", so I don't see why we should use "TotalTime" in SDL_mixer ?

I try to make a V9 before tomorrow, I'll move Duration call as you suggest in comment #17

Thanks for the review.

(In reply to matthias gatto from comment #19)
> > I have one question: can you return double in seconds instead of int in
> > milliseconds? 
> > - To be consistent with an existing `Mix_SetMusicPosition()` call which
> > accepts double values
> Ok, I'll do that

Your call.  I'm good either way.  (but he has a point about
consistence wrt Mix_FadeInMusicPos and Mix_SetMusicPosition
which both accept milliseconds in double.)

> > - Maybe you'll rename call into `Mix_GetMusicTotalTime`?
> > 
> Well Javascript and ffmpeg(libav) seems to use "duration",
> so I don't see why we should use "TotalTime" in SDL_mixer ?

No need renaming, indeed.

>   I'll move Duration call as you suggest

Your call.

> Well Javascript and ffmpeg(libav) seems to use "duration", so I don't see why
> we should use "TotalTime" in SDL_mixer ?

I have that in MixerX since ~2015/2016 year (don't remind when I introduced them) and I don't want to get duplicated calls because mainstream Mixer has a call that not matching origin (for backward compatibility: MixerX shouldn't break compatibility with a mainstream).

If you really think that "Duration" is better, so, I'll keep both "Duration" and "TotalTime" for backward compatibility with both: old apps are used MixerX and apps are used "Duration" which is a mainstream variant of call.

BTW: You would calculate MPG123's duration on load time and store it as a value rather calculate it every call:

P.S. It's my implementation where I captured a full length: https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_mpg123.c

If any cons or missings up are on my side, please notice.

(In reply to Vitaly Novichkov from comment #23)
> BTW: You would calculate MPG123's duration on load time and store it as a
> value rather calculate it every call:

It would be better if we cache it at load time, I guess.

Created attachment 4101 [details]
v9

-use double instead of int
-use second instead of MS
-move duration struct field under seek
-and store duration in mpg123

Looks better, however a question: why you are used `mpg123_position` and `mpg123_info` tricks instead of mpg123_length() that already returns a full length of a song? I know that it may return an incorrect length of a song in a condition of bad ID3 tags. However, since we are skipping them, MPG123 never reads them, and with this, it returns a correct length of a song via `mpg123_length()` call.

About OGG Vorbis: instead of this code, you can make it simply:

```
vorbis.ov_time_total(&music->vf, -1);
```

For the case of Tremor, this code:
```
return (double)(vorbis.ov_time_total(&music->vf, -1)) / 1000.0;
```

About WAV: don't forget to cast sample_size into (double):

```
WAV_Music *music = (WAV_Music *)context;
Sint64 sample_size = music->spec.freq * music->samplesize;
return (double)(music->stop - music->start) / (double)(sample_size);
```

Also, please rename the "duration" into "Duration" inside of Mix_MusicInterface to be in consistence with other fields.

Created attachment 4102 [details]
v10

v10:
- spelling fixes here and there.
- adds missings double casts or '.0' here and there.
- very minor tidy-ups.

(In reply to Vitaly Novichkov from comment #30)
> Also, please rename the "duration" into "Duration"

I can do that before applying, if there is nothing else
and the patch is approved for pushing.

Created attachment 4103 [details]
v11

Fixes duration from old libmpg123 < 1.23 (no mpg123_framelength())
after the 'store duration in mpg123' change (needed seeking to 0.)

Sam, Ryan:  This is pretty much complete.  The discussion
thread is there for you to follow.  OK to apply?  Reject?
Or, any suggestions?

Please capitalize the duration function pointer in music.h, to match convention of the other function pointers, but otherwise seems fine to add.

I would prefer to have Vitaly cleanly merge good changes from this patch into his code, so we get a clean merge from his code into SDL_mixer.

In general, his merge has highest priority because it will be the most disrupting and everything else should come in after that.

Created attachment 4104 [details]
v12

V12: Capitalized the duration function pointer in music.h

Vitaly:  if you merge this (v12) to your fork, I can merge it here
and it would be easier for later to prepare patches for you.

(In reply to Ozkan Sezer from comment #37)
> Vitaly:  if you merge this (v12) to your fork, I can merge it here
> and it would be easier for later to prepare patches for you.

I gave some code related questions that are still not answered...

Okay, I'll repeat them:

* MPG123: why you have used `mpg123_position` and `mpg123_info` tricks instead of MUCH SIMPLER `mpg123_length` that already returns a full length of a song? I know that it may return an incorrect length of a song in a condition of bad ID3 tags. However, since we are skipping them, MPG123 never reads them, and with this, it returns a correct length of a song via `mpg123_length()` call.
https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_mpg123.c#L298


* OGG Vorbis: instead of this code, you can make it simply:

```
vorbis.ov_time_total(&music->vf, -1);
```
and Tremor:
```
return (double)(vorbis.ov_time_total(&music->vf, -1)) / 1000.0;
```
https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_ogg.c#L518-L526



* WAV: don't forget to cast sample_size into (double):

```
WAV_Music *music = (WAV_Music *)context;
Sint64 sample_size = music->spec.freq * music->samplesize;
return (double)(music->stop - music->start) / (double)(sample_size);
```
https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_wav.c#L596-L601



> I would prefer to have Vitaly cleanly merge good changes from this patch into his code, so we get a clean merge from his code into SDL_mixer.

I already had a duration API call which was called differently and in some cases, there is implementation were was different. I have even a duration for MAD I made some years ago and it's on a board of MixerX.

(In reply to Vitaly Novichkov from comment #38)
> * WAV: don't forget to cast sample_size into (double):

This one is handled in the latest version of the patch.

The rest, Matthias should answer.

(In reply to Vitaly Novichkov from comment #38)
> (In reply to Ozkan Sezer from comment #37)
> > Vitaly:  if you merge this (v12) to your fork, I can merge it here
> > and it would be easier for later to prepare patches for you.
> 
> I gave some code related questions that are still not answered...
> 
> Okay, I'll repeat them:
> 
> * MPG123: why you have used `mpg123_position` and `mpg123_info` tricks
> instead of MUCH SIMPLER `mpg123_length` that already returns a full length
> of a song? I know that it may return an incorrect length of a song in a
> condition of bad ID3 tags. However, since we are skipping them, MPG123 never
> reads them, and with this, it returns a correct length of a song via
> `mpg123_length()` call.
> https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_mpg123.
> c#L298
> 
mpg123_position is here for retro compatibility, with old mpg123 versions
I don't know when length was introduce, I can change that to use length for new_mpg_duration

> 
> * OGG Vorbis: instead of this code, you can make it simply:
> 
> ```
> vorbis.ov_time_total(&music->vf, -1);
> ```
> and Tremor:
> ```
> return (double)(vorbis.ov_time_total(&music->vf, -1)) / 1000.0;
> ```
> https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_ogg.
> c#L518-L526
> 
> 
Ok, I'll do that

> 
> * WAV: don't forget to cast sample_size into (double):
> 
> ```
> WAV_Music *music = (WAV_Music *)context;
> Sint64 sample_size = music->spec.freq * music->samplesize;
> return (double)(music->stop - music->start) / (double)(sample_size);
> ```
> https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_wav.
> c#L596-L601
> 
> 
> 
> > I would prefer to have Vitaly cleanly merge good changes from this patch into his code, so we get a clean merge from his code into SDL_mixer.
> 
> I already had a duration API call which was called differently and in some
> cases, there is implementation were was different. I have even a duration
> for MAD I made some years ago and it's on a board of MixerX.

I can merge your mad code into my patch, or you can merge my timidity code...

I make a V13 for asap

> I can merge your mad code into my patch, 

It's a major work, so, feel free to compare and take it
https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_mad.c
One note that it gives some restructuring, so, be careful. In my code, I also have Tell() call, you may don't make it now, but please keep guts be ready for easier Tell() adding.

Also, wait a sec, I need to make minor refactoring of field names...

> you can merge my timidity code...

I already took that ;)

> Also, wait a sec, I need to make minor refactoring of field names...

Done!

> mpg123_position is here for retro compatibility, with old mpg123 versions

I think, I would like to dig up the history of mpg123 to get the information about this thing...

Okay, just now I have checked the SVN history (I have cloned a repo as a GIT and I did a local inspection of a history).
I found that the mpg123_length() call was existed in even older than 1.5.1 version (I found mentions in the code of August 2008).
So, I did even more deep inspection, and I have found that mpg123_length() was added in 18 of October 2007, and that was version... Since 0.67!

Okay, just for a fun, I'll try to build one of ancient MPG123 tarballs and link MixerX with it, and try the seekability in action...

I have tried to use 1.0.1, however, I have failed:
- It lacks mpg123_open_handle() that is required for us, and it was introduced since 1.12
- It lacks MPG123_ENC_SIGNED_32 and MPG123_ENC_FLOAT_32 are was introduced a bit later than 1.0.1, going to check 1.12...

So, mpg123 1.12 is a MINIMAL version of MPG123 that Mixer can support. mpg123_position() is here and it works with 1.12.0. However, some files are failing when opening with an "Unable to set up output format! (code 1)" error. Trying to figure out, is this a bug of 1.12.0 or because MP3 file has a newer format which is not supported by mpg123 v1.12.0.

**** Damn, a typo, I wanted to say "mpg123_length()", not mpg123_position()!!!!

The minimum required libmpg123 version is 1.12 which
has MPG123_API_VERSION 24.  That doesn't mean it will
work with any and all mp3 files: the library received
tons of bug fixes since that version.

As for mpg123_length(), it is an old api available in
all libmpg123 versions we support.  However I never
used it myself, so make sure it returns the expected
values if we are to use it in MPG123_Duration().

I did a small check of mpg123_length() with mpg123 1.12, and for most of the files it returns a correct song length. For some files it returns an error (-1 code). Modern MPG123 (that is in packages of Linux Mint 19.2), is works with any files and I had no fails with it.

Created attachment 4106 [details]
v13 - rebased and uses mpg123_length()

We had to apply a seek fix to music_mpg123.c:
https://hg.libsdl.org/SDL_mixer/rev/2019adc079cc

After that, using mpg123_length() is simple and seems to work
just fine, reporting the same value as mpg123_position() does.

Adding a new v13 of the duration patch with that change.

Created attachment 4107 [details]
v13+ - A patch over patch that extends and fixes v13

Bugfixes and clean-ups
- FLAC: Warning fixes
- ModPlug: Align function declaration and warning fixes
- MPG123: Warning fixes
- OGG: Use simplier and much more accurate ov_time_total()
- OPUS: Minor warning fix
- Timidity: Minor warning fix
- WAV: Minor warning fix
- MAD: Backporting of a better seekability support from MixerX

(In reply to Vitaly Novichkov from comment #52)
> Created attachment 4107 [details]
> v13+ - A patch over patch that extends and fixes v13
> 
> Bugfixes and clean-ups
> - FLAC: Warning fixes

None of these warning fixes has anything to do with duration
patch, except for the last hunk (see below for that.) In fact
it does more than just warning fixes: explain the non-warning
changes to me in private mail (or maybe create a new bugzilla
entry for them with explanations.)

> - ModPlug: Align function declaration and warning fixes
> - MPG123: Warning fixes
> - OPUS: Minor warning fix
> - Timidity: Minor warning fix

About your changes like the following:
-    return (double)music->full_length / music->sample_rate;
+    return (double)music->full_length / (double)music->sample_rate;

There is already a (double) cast, do you _really_ get a warning
if you don't add the second cast??

> - WAV: Minor warning fix

Name typo fix is OK (i.e. WAW_Duration -> WAV_Duration)

> - OGG: Use simplier and much more accurate ov_time_total()

Matthias: Is it OK with you?

> - MAD: Backporting of a better seekability support from MixerX

Will test.

(In reply to Ozkan Sezer from comment #53)
> (In reply to Vitaly Novichkov from comment #52)
> > Created attachment 4107 [details]
> > v13+ - A patch over patch that extends and fixes v13
> > 
> > Bugfixes and clean-ups
> > - FLAC: Warning fixes
> 
> None of these warning fixes has anything to do with duration
> patch, except for the last hunk (see below for that.) In fact
> it does more than just warning fixes: explain the non-warning
> changes to me in private mail (or maybe create a new bugzilla
> entry for them with explanations.)
I agree here, could we keep unrelated fix in another issue ?
> 
> > - ModPlug: Align function declaration and warning fixes
> > - MPG123: Warning fixes
> > - OPUS: Minor warning fix
> > - Timidity: Minor warning fix
> 
> About your changes like the following:
> -    return (double)music->full_length / music->sample_rate;
> +    return (double)music->full_length / (double)music->sample_rate;
> 
> There is already a (double) cast, do you _really_ get a warning
> if you don't add the second cast??
> 
> > - WAV: Minor warning fix
> 
> Name typo fix is OK (i.e. WAW_Duration -> WAV_Duration)
> 
> > - OGG: Use simplier and much more accurate ov_time_total()
> 
> Matthias: Is it OK with you?

I'm okay with this
> 
> > - MAD: Backporting of a better seekability support from MixerX
> 
> Will test.

Thanks for the reviews, comments and improvements

Ozkan Sezer, feel free to split up my patch and please give the v14 that includes all necessary changes, then, I'll be able to make v14+ which should go separately from this.

> There is already a (double) cast, do you _really_ get a warning
> if you don't add the second cast??

This additional cast of `(double)x/(double)y` is a paranoia against odd compilers which may compute this incorrectly when any of operands is an integer. This should guarantee that there will not be any integer divisions. If it really works everywhere even on clunky compilers, then, ok.

(In reply to Vitaly Novichkov from comment #56)
> > There is already a (double) cast, do you _really_ get a warning
> > if you don't add the second cast??
> 
> This additional cast of `(double)x/(double)y` is a paranoia against odd
> compilers which may compute this incorrectly when any of operands is an
> integer. This should guarantee that there will not be any integer divisions.
> If it really works everywhere even on clunky compilers, then, ok.

There are gazillions of tons of code out there which simply do
 double result = (double)integer1 / integer2;
.. or:
 double result = integer1 / 1000.0;

Have you actually run into any miscompilations because of this?
I.e. which compilers?

> Have you actually run into any miscompilations because of this?
> I.e. which compilers?

In my memory, there are some old versions of GCC or MinGW compilers, I had some miss-calculations bugs in another project 3 or 4 years ago which I have been fixed with adding these castings. Therefore, to avoid any possible miscalculation issues I adding casts to all non-floating operands even actually these castings are unneeded. I may guess, the misscalculation may happen when destination variable is an integer, or something like.

(In reply to Vitaly Novichkov from comment #56)
> > There is already a (double) cast, do you _really_ get a warning
> > if you don't add the second cast??
> 
> This additional cast of `(double)x/(double)y` is a paranoia against odd
> compilers which may compute this incorrectly when any of operands is an
> integer. This should guarantee that there will not be any integer divisions.
> If it really works everywhere even on clunky compilers, then, ok.

I think the standard say that if any operand is double then they're all converted to double
https://busybox.net/~landley/c99-draft.html#6.3.1.8
(I've say "I think" because I'm bad at understanding standard)

(In reply to matthias gatto from comment #59)
> (In reply to Vitaly Novichkov from comment #56)
> > > There is already a (double) cast, do you _really_ get a warning
> > > if you don't add the second cast??
> > 
> > This additional cast of `(double)x/(double)y` is a paranoia against odd
> > compilers which may compute this incorrectly when any of operands is an
> > integer. This should guarantee that there will not be any integer divisions.
> > If it really works everywhere even on clunky compilers, then, ok.
> 
> I think the standard say that if any operand is double then they're all
> converted to double
> https://busybox.net/~landley/c99-draft.html#6.3.1.8
> (I've say "I think" because I'm bad at understanding standard)

Third paragraph of 6.3.1.8:

"Otherwise, if the corresponding real type of either operand is double,
 the other operand is converted, without change of type domain, to a
 type whose corresponding real type is double."

So the additional cast is actually not necessary.

Okay, I also have checked an older C89 standard: http://port70.net/~nsz/c/c89/c89-draft.html#3.2.1.5 and it's the same. I think, yeah, the (double)x/(double)y is just an unnecessary paranoia.

(In reply to Vitaly Novichkov from comment #61)
> Okay, I also have checked an older C89 standard:
> http://port70.net/~nsz/c/c89/c89-draft.html#3.2.1.5 and it's the same. I
> think, yeah, the (double)x/(double)y is just an unnecessary paranoia.

OK then, will attach a new V14 here soon with the unnecessary casts
removed.

Created attachment 4108 [details]
V14 - updated patch

Ogg: changed to use ov_time_total(): suggested by Vitaly
  and OK'ed by Matthias.

WAV: typo fix: WAW_Duration -> WAV_Duration

ModPlug: minor reorder in modplug_loader structure

mpg123: return -1.0 explicitly instead of -1

Created attachment 4109 [details]
duration patch for MAD

music_mad.c: Backports seek support from SDL_Mixer-X:
Needs _real_ good review, so keeping this separate for
now.

(initial seek-to-start dropped in calculate_total_time()
from the original patch: mp3_skiptags() does that.)

As far as I can see, Vitaly synchronized his fork with patch V14?
If so, I can merge V14 mainstream, and we can continue improving,
i.e. review the music_mad part of the patch from comment #64 (or
maybe create a new bugzilla entry for it.)

(In reply to Ozkan Sezer from comment #65)
> As far as I can see, Vitaly synchronized his fork with patch V14?
> If so, I can merge V14 mainstream, and we can continue improving,
> i.e. review the music_mad part of the patch from comment #64 (or
> maybe create a new bugzilla entry for it.)

Yes, I have already synchronized the stuff from here, it's should be fine now.

V14 patch merged: https://hg.libsdl.org/SDL_mixer/rev/543b77a3c0eb

The MAD patch to support duration still needs review.
I'm moving it to a new bugzilla entry (see bug #4902),
and adding Matthias and Vitaly to its CC list.

Closing this one.