search

libsdl-org / SDL_mixer

An audio mixer that supports various file formats for Simple Directmedia Layer.
zlib License
434 stars 146 forks source link

music_mad.c: Added the support for the Tell and Duration #300

Closed Wohlstand closed 3 years ago

Wohlstand commented 3 years ago

This update I developed a while ago for the MixerX fork. It allows reporting the song duration and the current time position while playing the song.

233

slouken commented 3 years ago

Thanks!

sezero commented 3 years ago

@slouken (and @Wohlstand): How well did you test this? See madplay tag.c:tag_parse() for possible gotchas.

slouken commented 3 years ago

I didn't test it, I relied on this being mature code in the MixerX fork.

Do you have test cases where this doesn't work or is worse than not having it implemented at all?

sezero commented 3 years ago

I didn't test it, I relied on this being mature code in the MixerX fork.

IMO, not a good idea.

Do you have test cases where this doesn't work or is worse than not having it implemented at all?

I don't. I haven't got the time to thoroughly reviewing / testing it yet.

Wohlstand commented 3 years ago

@slouken (and @Wohlstand): How well did you test this? See madplay tag.c:tag_parse() for possible gotchas.

// Checked out the madplay's source Wow, why you didn't mention it to me a year ago? I see it also does parse of the LAME tag also.

test cases where this doesn't work or is worse than not having it implemented at all?

Speaking about Duration, it may improperly compute the duration of the file contains an invalid Xing/IMFO/VBRI tag, doesn't contain it, contains a false value, etc. Also, it fails to compute the duration when playing Frankenstein streams (a concoction of two or more different-type files). Position reporting will work properly as it simply counts the played amount of PCM samples. In the past, I had a different duration code that makes the full scan of the song and computes the actual duration. This way had a disadvantage: the file got being loaded longer due to this scan. For the VBR streams, if the VBRI tag is broken, the computed duration may be wrong without the full scan of the song.

The code I made a year ago is based on this manual https://www.codeproject.com/Articles/8295/MPEG-Audio-Frame-Header#VBRHeaders. At all, I do the minimal check to simply retrieve the song duration with nothing also.

sezero commented 3 years ago

// Checked out the madplay's source Wow, why you didn't mention it to me a year ago?

I remember that I did, but whatever.

Wohlstand commented 3 years ago

I remember that I did, but whatever.

I don't remind when and where... I did the check of my mailbox archive, and I see nothing on that...

The only thing you gave me, 4 links to various discussions and specifications against Xing support, etc.

sezero commented 3 years ago

The only thing you gave me, 4 links to various discussions and specifications against Xing support, etc.

I said whatever, didn't I..

In any case, madplay source is GPL (not LGPL), so be careful when using stuff from it.

Wohlstand commented 3 years ago

In any case, madplay source is GPL (not LGPL), so be careful when using stuff from it.

if I use some, I would just look at the methods, but code the new with no copypasting, the same as a retelling the plot of the book. At least, even this I sent isn't ideal, it just works for ~1.5 years (at my side I do use the libmad by default until I'll develop the proper CMake build for the mpg123, people didn't blame the bad workflow of MP3 playback at least).

Wohlstand commented 3 years ago

Anyway, a bit later I'll try to add more checks and the support for the LAME header in the next patch...

sezero commented 2 years ago

One mp3 I have reports 56.607347s duration instead of about 31.320816s with libmad backend: I can attach the mp3 in question here.

slouken commented 2 years ago

One mp3 I have reports 56.607347s duration instead of about 31.320816s with libmad backend: I can attach the mp3 in question here.

Can you enter a new issue for this, for tracking purposes?

sezero commented 2 years ago

Done: https://github.com/libsdl-org/SDL_mixer/issues/403