[SDL Mixer X] Seek and skip: review, clarifications, improvements, additional features

Description: Report bugs and ask questions regarding PGE here.
Forum rules: Here you can ask any question related to PGE Project components:


Any questions related to LunaLUA project please ask HERE (LunaLUA subsection)
Any questions related to SMBX-38A (1.4.x) Chinese project please ask HERE (SMBX-38A subsection)
Moderators: Semi-moderatos, Moderators

StefG
Topic author, Nice citizen
Nice citizen
StefG
Topic author, Nice citizen
Nice citizen
Reputation: 2
Posts: 14
Joined: 7 Jan 2019

Post #1by StefG » 19 Jan 2019, 19:16

I use the terms 'seeking' for 'absolute seeking' and 'skipping' for 'relative seeking'.

About `Mix_MusicSetPosition`, in the fact it's always used for absolute position and nothing relative, and always in seconds unit. Even on the side of official SDL Mixer this function works for absolute seconds position. Maybe, in the past it had some crap, but on the moment when I have picked it up initially, it had the sense "absolute seconds position".

OK, so it seems that there is no documentation for SDL_Mixer 2 and that the doc to which SDL_Mixer 2 points is in fact the same SDL_Mixer 1.2 documentation : https://www.libsdl.org/projects/SDL_mixer/docs/SDL_mixer.html#SEC65 :shout: , but the code is indeed as you say: always absolute.

However, I think the Mikmod version does not use seconds but straight 'positions', which seem to be some kind of ticks (like MIDI). It does not seem to be pattern numbers as SDL_Mixer (1.2) documentation says, but rather a row number inside a pattern.

I did a quick investigation last night, and took the following notes. I was interested in behaviour when seeking out of bounds, which is unspecified (undocumented) by SDL Mixer and by most underlying libraries too. It may not be very important when doing pure seeking which has little reason to seek out of bounds, but it is useful to have a fixed specified behaviour when doing some skipping.

As you can see, it is completely inconsistent :(

(It is possible I overlooked or misunderstood a few points as I did it quickly, just by reading code and not by testing, but well, the general picture should be rather correct.)

Code: Select all

           | seek type   |  seek < 0     |  seek > end    | notes
-------------------------------------------------------------------------------------------
flac       | absolute    |  unsigned^1   |   no action    | libFLAC behaviour undocumented
fluidsynth |    n/a      |     n/a       |     n/a        |
gme        | absolute    |     bug^3     | to end + err?  | libGME behaviour undocumented
mad        |             |  unsigned^1   | to end + err   | no dedicated lib function
midi_adl   | absolute    |   no action   |   seek to 0    | never returns errors
midi_opn   | absolute    |   no action   |   seek to 0    | never returns errors
mikmod     | abs tick pos|  unsigned^1   |    to end      | libmikmod behaviour undocumented^4, no error possible
modplug    | absolute    |  unsigned^1   |    to end      | no doc at all for libmodplug
mpg123     | absolute^2  |   seek to 0   |      ?         | libmpg123 behaviour undocumented
nativemidi |    n/a      |     n/a       |     n/a        |
ogg        | absolute    | no action+err | no action+err  | libvorbis behaviour not clearly documented
opus       | absolute    | no action+err | no action+err  | libopusfile behaviour not clearly documented
timidity   | absolute    |  unsigned^5   |    to end      | no doc at all
wav        | absolute    |  variable!^6  | no action+err  |

^1: music_flac casts negative double to unsigned int without any check! -> UB
    music_mad casts negative double to unsigned long without any check! -> UB
    music_mikmod casts negative double to unsigned short without any check -> UB
    libmodplug casts negative double to uint32_t without any check -> UB
    music_timidity casts negative double to Uint32 without any check -> UB

^2: the underlying library also supports relative seeking.

^3: it seems that no check is performed before the argument is used in
    various calculations and loop conditions, so it probably does... whatever.

^4: bad comment in music_mikmod.c, refering to time (seconds) instead of
    pattern rows.

^5: timidity casts double to unsigned and then back to signed -> UB

^6: if the underlying SDL_RWops is a file (stdio or windows_file) seeking
below zero will result in: no action taken + error emitted. But if it is a
memory buffer, then the result is: seeking to 0!


That's why, I'd still like to add properly specified functions, possibly configurable, like [Seek/Skip]Seconds(..., double seconds, enum seek_bounds mode), which operate on seconds for really all formats, and with enum seek_bounds being SB_SATURATE for seeking/skipping to 0 for when seeking/skipping below 0, and to the end (or just before?) when seeking/skipping beyond the end, SB_ERROR for doing nothing and returning an error when seeking/skipping out of bounds, SB_SATURATE_ERROR for performing the same action as SB_SATURATE and signalling an error as SB_ERROR too.

I don't have a definitive API in mind, but something like that.

MAD MP3 hadn't seekability originally, I have implemented my own seeker which does two algorithms in dependence from seek the direction (when position is going before, drop position into begin and seek frames forward; but when it going after, just skip some frames)

Oh, that's your work in the official SDL_mixer too?

Yeah, MP3 is a mess to seek, but it's possible.

In fact, my understanding of my readings on this matter (very fresh readings, I am no expert at all) is that it is not so complicated.

From what I understood:

  • MP3 frames have a fixed length: always the same number of samples;
  • this number only depend on the MPEG version used;
  • in a MP3 stream/file, all frames should have the same MPEG version and the same sample rate (otherwise it's a Frankenstein MP3 as a library called them);
  • same sample rate + same number of samples / frame => all frames in a MP3 stream/file should have the same time duration;
  • contrarily to what is written in music_mad.c comments, the bitrate doesn't matter (it just means a frame will occupy more or less bytes, but the duration it describes remains constant), so VBR MP3 can be handled the same as CBR.

So, for regular MP3 files (including VBR) each frame has a fixed time duration, all frames have the same.

The only small difficulty is if we want to handle irregular Frankenstein files, which have different MPEG version, or different sample rates, in some frames.

But is not very difficult either, since now we already have to parse the full file when we load it in order to calculate its duration (in your calculate_total_time() function). We just have to store a table of frames timestamps at the same time, those time points are already calculated anyway. Then we also need to store the offsets to which they refer in the source stream/buffer, which is a bit more annoying (because of the way music_mad operates on a small buffer and not directly on the whole file buffer) but doable.

Then we can seek directly to the right frame in regular MP3 files, and almost directly for Frankenstein MP3 files (the difference between Frankenstein and regular files has also been very simply detected at the first parsing time, during file duration calculation). No need to decode again each frame, and the next, and so on, until we find the right one to perform a seek.

I have written code for that, but I have been doing something else since, so I need to review it again, and it probably requires some testing and cleaning before I show it. Just wait a few days ;-) well, I hope :)

Wohlstand M
Lead Developer
Lead Developer
Avatar
Wohlstand M
Lead Developer
Lead Developer
Age: 27
Reputation: 385
Posts: 1420
Joined: 15 Feb 2014
Location: Moscow, Russia
Website Skype YouTube

Post #2by Wohlstand » 19 Jan 2019, 22:45

StefG wrote:Oh, that's your work in the official SDL_mixer too?
The seekability over libMAD was made before me, however, my work is was retrieving the current position and full length in seconds.

Yeah, it's would be better to optimize existing seeking MP3 code to speed it up in whole.

StefG
Topic author, Nice citizen
Nice citizen
StefG
Topic author, Nice citizen
Nice citizen
Reputation: 2
Posts: 14
Joined: 7 Jan 2019

Post #3by StefG » 21 Jan 2019, 15:28

Wohlstand wrote:
StefG wrote:Oh, that's your work in the official SDL_mixer too?
The seekability over libMAD was made before me, however, my work is was retrieving the current position and full length in seconds.

Ah, all right, that's how I understood it at first.

Wohlstand wrote:Yeah, it's would be better to optimize existing seeking MP3 code to speed it up in whole.

OK, so I did a bit of testing of my new code. At first sight it seems to work. Even better than the present code, because I noticed while I was testing, that the present code has problems on my setup with long seeks (long = just a couple of minutes), causing underruns probably because parsing each frame until the right one is too slow :

Code: Select all

ALSA lib pcm.c:8323:(snd_pcm_recover) underrun occurred


And those underruns do not happen in my version.

But I still have a few tests to make and I cannot make them today. I hope I can show something next evening.

StefG
Topic author, Nice citizen
Nice citizen
StefG
Topic author, Nice citizen
Nice citizen
Reputation: 2
Posts: 14
Joined: 7 Jan 2019

Post #4by StefG » 22 Jan 2019, 16:32

I didn't spot any problem so far with my few tests.

I put a patch and an example test program in the attached Zip file.

Patch has to be applied with -p3 from project root (root = where there is README.txt, CMakeLists.txt, include/, src/ and so on; there is apparently a small difference between the base version I used and the current version, but patch manages to deal with it).

Example takes a MP3 file as command-line argument.

I explain the patch:

  • because I did it during my project of adding new Seek/Skip functions, it is presented in the form of new functions next to the existing ones. The benefit is that you can test both old and new functions together in the same program.
  • the drawback is that since Mix_MusicInterface has been augmented with 2 functions, others codecs music_XXX.c will warn at compilation because they have not been updated and their interface won't match, but as long as you don't call them in a test program, that won't be a problem for runtime.
  • music.h and SDL_mixer_ext.h are touched just to declare the new functions interfaces.
  • music.c is touched for similar boilerplate code.
  • real stuff is in music_mad.c, in 2 steps: taking advantage of existing parsing in calculate_total_time() to memorise timing and offsets information; and then the actual new seek function which uses that memorised information: MAD_SeekSet().
  • a good part of the added code in calculate_total_time() is just to create/grow/delete the storage where I memorise information, not very interesting, but in C it has to be done.
  •  storing time information (frame_timestamps_ms) is useful for Frankenstein MP3; for regular MP3 it is not needed because all frames should have the same duration.
  • storing offsets is needed for all cases, and it is more complicated than storing timestamps, because calculate_total_time() and MAD related functions and variables act on the small intermediate music->input_buffer[tt] and not the complete [tt]music->src . read_update_buffer() had to be modified to trace the real offsets in the real source buffer.
  • the VBR detection is present but not used (I had introduced it trusting old SDL_mixer comments). But well, it doesn't hurt, it doesn't take time to calculate it and it doesn't take room to store it. It could be removed anyway.
  • music->sample_position is still present but not really used any more, it wasn't more accurate than the new music->frame_position, I think.
  • seeking is simple, we just go to the memorised offset for a regular MP3. The annoying part is clearing buffers: I don't feel so sure about it, so if you could double/triple-check it, that would be good. I mean, it works, but I did a bit by trial-and-error, and I don't like that much, I don't feel safe about the resulting code of that process...
  • for a Frankenstein MP3, it is a bit more involved; I chose to go to the average frame position as if it was a regular MP3, and then iterate 1 by 1 in the timestamps table until I go over the wanted one. One could have iterated always from 0, or used a dichotomy, or used an average frame position + dichotomy, etc. The present solution is simple, and efficient enough in most cases, I guess?
  • so, we have code for timing calculating and for seeking into Frankenstein MP3s. But our existing code can't play them correctly :biggrin: so I don't know if it is very useful... On the other hand, that's now the only program in those I tested (Windows: Winamp, SMPlayer, VLC, Linux : mplayer, cmus) which calculate the correct time duration of a Frankenstein MP3, all those existing players show a wrong duration! :cool: but except Winamp, the others play it with the correct sound. Winamp and SDL_mixer don't: in my test, I have a 48 kHz piece followed by a a 44,1 kHz one, and the second plays too high-pitched because Winamp and SDL_mixer play the 44,1 kHz piece as if it was 48 kHz. For a player, it is good to be able to play all files correctly, even if they are not standard compliant; for a game library, it is not so needed since you are supposed to control your input assets in general (and convert them to a proper usable format if they are not). On the other hand, my own use case is for a player... :crazy:

I will try to send you a couple of test files as PM.
Attachments
improved_mad_seek-01.zip
(5.18 KiB) Downloaded 18 times

Wohlstand M
Lead Developer
Lead Developer
Avatar
Wohlstand M
Lead Developer
Lead Developer
Age: 27
Reputation: 385
Posts: 1420
Joined: 15 Feb 2014
Location: Moscow, Russia
Website Skype YouTube

Post #5by Wohlstand » 23 Jan 2019, 2:39

Just now I have got your test MP3 files and quickly reviewed your code, however, I can't test it now as I have very tired from my job today, and I gonna sleep now. I'll try it again tomorrow after my job ;-)
Thanks for a work! :fox: :)

P.S. In my VLC the Frankenstein-MP3 song played correctly, but duration value wasn't persistent, but changed very weirdly... :crazy:

StefG
Topic author, Nice citizen
Nice citizen
StefG
Topic author, Nice citizen
Nice citizen
Reputation: 2
Posts: 14
Joined: 7 Jan 2019

Post #6by StefG » 23 Jan 2019, 13:34

Wohlstand wrote:P.S. In my VLC the Frankenstein-MP3 song played correctly, but duration value wasn't persistent, but changed very weirdly... :crazy:

Yes, that's what I experienced too with my VLC (Windows), and it also changed the current time value to wrong values. That's VLC doing what it always does best: rubbish :bads: Good luck seeking (with the cursor) to a specific point in time with that :biggrin:

Wohlstand M
Lead Developer
Lead Developer
Avatar
Wohlstand M
Lead Developer
Lead Developer
Age: 27
Reputation: 385
Posts: 1420
Joined: 15 Feb 2014
Location: Moscow, Russia
Website Skype YouTube

Post #7by Wohlstand » 24 Jan 2019, 0:24

StefG wrote:because I did it during my project of adding new Seek/Skip functions, it is presented in the form of new functions next to the existing ones. The benefit is that you can test both old and new functions together in the same program.
Oh, I forgot not tell you, once you adding new Interface function, you must to specify "NULL" or implementation into EVERY codec SDL Mixer X has, otherwise, all other codecs will work clunky or even crash :wacko:
Be careful!

Also, I have found the sort of tabs vs spaces mess and code style inaccuracy I trying to fix... In a whole, this patch looks too experimental, and yeah, I'll put this into separated branch to test this later.

P.S. For codecs are lacks the "skip" call, let's simulate it with "seek( tell() + howmuch )"? ;-)
P.P.S. I see no reason to have duplicated "seek" call which looks 1:1 to current "seek". All current seek calls are using seconds unit, the documentation that tells different than second units, lying for now. And when some codec doesn't gives "seconds" unit, then it's a bug that must be fixed! :fox-comando:

Added after 15 minutes 8 seconds:
Done, your patch with my fixes is now on experimental branch: https://github.com/WohlSoft/SDL-Mixer-X/tree/experimental-mad-seek
I think, it needs some polishing until it will be merged with master. You can pull it and continue work on it if you want ;-) , for now myself I have other tasks to do.
I have tried the seeker on your MP3s, it isn't so accurate, and still think each jump longer than even my current code which isn't good...


Return to “Troubleshooting”

Who is online (over the past 5 minutes)

Users browsing this forum: 1 guest