Hello!

Recently I have completed my merge job of your changes with mine and I have extended functionality by adding of my stuff.

Especially for your convenience I made the Mercurial repository on Bitbucket you can easily review my changes and pull and merge if that needed:
https://bitbucket.org/Wohlstand/sdl_mixer/commits/all

Here is written a full changelog of stuff I made:
https://bitbucket.org/Wohlstand/sdl_mixer/commits/524433869ae11d0fb56a1464dfb75b498911e39f

My next part of the job is completing the "What NOT added from SDL Mixer X yet" list:
- Add all those libraries into "external" folder and make them be buildable through Automake and through VisualStudio projects. Then I can freely add the GME, ADLMIDI, and OPNMIDI libraries.
- Implement complete CMake build support that does not exist yet in SDL Mixer

Note that bitbucket repository is now very outdated, and soon, Bitbucket will nuke all Mercurial repositories due to stopping to support Mercurial VCS at all.

In general, these look awesome! I'd like to finish merging your changes, and promote this to SDL_mixer 2.0.

Do you have a clean set of changes that apply to the current SDL_mixer on hg.libsdl.org?

> Do you have a clean set of changes that apply to the current SDL_mixer on hg.libsdl.org?

I had, however, they got to be very outdated. So, I'll rebuild them from scratch again. Looks like I need to begin a new fork of the repository from the scratch.

With the set of changes are:
- relocating files into nice "include", "src", "src/codecs". This will break all hand-made VS and XCode projects which will require to apply these locations to them. This is optional to make the look of repository better.
- adding some new files and modules that will require modifying of VS and XCode projects to attach them.
- adding some new libraries to link, which will also require modifying of VS and XCode projects.
- CMake build is going to be default one. On my side, I should improve the look of dependencies management as now it looks not good (repeating parts of code to check the existence of certain libraries and using pre-build from AudioCodecs set by optional choice).

However, there are changes that can be applied without adding new files or libraries which can be put over current stuff now.

Alternatively, I'll give the backport of most of my changes include new files and libraries, and can you begin a new branch to work on them separately?

> to work on them separately?

I meant fixing of VS and XCode projects are will be broken after adding a set of new dependencies and files. And, posssibly, because of source files relocations (which is optionally and can be skipped to leaving files in their current places).

So, my question is: do you prefer to keep current locations of source files or I can freely relocate them into "src", "src/codec", "include"?

Speaking for myself, 

(In reply to Vitaly Novichkov from comment #4)
> - CMake build is going to be default one.

If autotools, VisualC and Xcode projects aren't touched/broken
I have no problem with that.

(In reply to Vitaly Novichkov from comment #6)
> So, my question is: do you prefer to keep current locations of source files
> or I can freely relocate them into "src", "src/codec", "include"?

I'd rather not change the tree layout for the time being.

> If autotools, VisualC and Xcode projects aren't touched/broken
I have no problem with that.

Okay, however, for some files I'll try to add new files into these projects also. Regarding new dependencies, these projects will still be buildable, however, these new dependencies and codecs are based on them, will not work until support will be implemented. There are GME, libXMP, libADLMIDI, libOPNMIDI, and libID3TAG (for MP3 tags support).

> I'd rather not change the tree layout for the time being.

Ok, will keep the tree layout the same as the original while backporting mine stuff.

Actually, with a change of this magnitude, moving files around is fine, we'll rebuild the hand-crafted projects, no problem.

One thing to be aware of is that all third party dependencies must be optional, and dynamically loadable. Any code that is loaded by default must be compatible with the zlib license. If there is any LPGL or similar code, it must default to off.

> Actually, with a change of this magnitude, moving files around is fine, we'll rebuild the hand-crafted projects, no problem.

Ok, then will restructure a tree for nicer look.

> One thing to be aware of is that all third party dependencies must be optional, and dynamically loadable. Any code that is loaded by default must be compatible with the zlib license. If there is any LPGL or similar code, it must default to off.

All new dependencies I have introduced are optional, therefore yeah, I will keep the note to make them be disabled by default. I'll also implement dynamic loading of libADLMIDI, libOPNMIDI and libGME are didn't make yet. libXMP has two variants of build: MIT and LGPL, however, I'll implement the dynamic loader for it too.

BTW, I looking for an alternative to modified libID3TAG made by a team who made MAD codec (I modded it to equip it with SDL_RWops support) library which is GPL, so, I will give the dummy only, and it needs to:
- keep as-is and don't parse ID3 tags at all
- make some minor ID3 tag parsing

Ozkan, can you hold off further SDL_mixer cleanup while Vitaly prepares his merge, and then verify it once he's created his merge patchset?

(In reply to Sam Lantinga from comment #11)
> Ozkan, can you hold off further SDL_mixer cleanup while Vitaly prepares his
> merge, and then verify it once he's created his merge patchset?

Of course. (The cleanups I did yesterday was after the merge for
bug #4865, anyway.)

I hope the merge for this one happens with multiple / broken-up
which do one thing at a time.

Ozkan, while I working on a patchset, I would grant you a write access to my repos where you would be able to make various fixes in parallel with my works.
Ooay, my workflow is:
- polish CMake build and make dynamical APIs work again (they are never used right now, and I should make them working by cmake build rework)
- make dynamical loading of libADLMIDI, libOPNMIDI, GME and libXMP
- replace GPL-licensed libID3TAG with a ZLib compatible alternative and mimimalistic implementation
And then:
- make a new branch on SDL_mixer repo (at my bitbucket)
- put my code into it
- update autotools build to make it build my update
- polish some other cases
And when works will be completed, feel free to pull that branch and merge.

Okay, I think, to simplify future works, I made on a quick hand restructure of the tree and I have put it here: https://bitbucket.org/Wohlstand/sdl_mixer/commits/608bacaa2d582870df9824c14fa7fc50772adfd5
(feel free to pull https://Wohlstand@bitbucket.org/Wohlstand/sdl_mixer and verify the stuff)
I have ported and tested build and install of Autotools build which now working with a new tree, CMake and Android also ported but didn't test yet. Android builds need a fix of include directories I guess (You should add `include`, `src`, and `src/codecs` as global include paths).

I merged the tree layout changes of Vitaly Novichkov
to help with his preparing his changesets.

Build status: Autotools, Xcode, and VicualC works for me.
Xcode-iOS and VisualC-WinRT projects are updated but NOT
tested at all: please test/update them as needed  (I can
not do that myself.)

Vitaly:  Can you give an ETA about your patch-sets for this?

Here are the first set of patches from Vitaly, currently residing at:
https://bitbucket.org/Wohlstand/sdl_mixer/branch/music-interfaces-ex2

Comments, concerns, accepts, rejects, changes requests, etc. wanted:
- Are there any of them we do _not_ want in mainstream?
- Do any of them require changing in some way before accept?

If no comments until Mon. Dec. 23, 14:00 (GMT+03), I'll most probably
apply them mainstream SDL_mixer.

Created attachment 4123 [details]
1139.patch

music_mad.c (MAD_Seek): Avoid a junk chunk be played after seek
https://bitbucket.org/Wohlstand/sdl_mixer/commits/abc0101a09bb2aaa6a390b81e19f7368719d68b6?at=music-interfaces-ex2

Created attachment 4124 [details]
1141.patch

Add Mix_GetVolume() call to retrieve the volume of a music
https://bitbucket.org/Wohlstand/sdl_mixer/commits/21eaf90a7bd22420618205ae690f81b87b876afb?at=music-interfaces-ex2

The 'Stream' suffix is there to prepare for multi-music functionality
which includes a set of new functions are equal to old Music functions
but unlike them, requiring a music pointer.

multi-music: it's an api that should allow to play multiple parallel
streams (for example, various environment SFX that is hard to play via
existing chunks api due the fact they are loading an entire song into
memory). Also the Multi-Music API should allow a cross-fade of songs
and should allow making a dynamical songs - i.e. playing separated
tracks of the same song as different streams, and controlling a volume
of every stream to mute/unmite certain stream. All multi-music related
functions will have 'Stream' suffix.

multi-music isn't ready yet on MixerX side, however, I prepared some
works for it.

Created attachment 4125 [details]
1142.patch

Add Mix_GetMusicPosition() call to retrieve a current music playing position
https://bitbucket.org/Wohlstand/sdl_mixer/commits/c639c940542cffb0a629b3d4910b0358bebe8bcb?at=music-interfaces-ex2

Created attachment 4126 [details]
1144.patch

Add Loop points information calls
https://bitbucket.org/Wohlstand/sdl_mixer/commits/ee1a67f25eae199a0b2790548d4f1d1e57e85f5f?at=music-interfaces-ex2

- Mix_GetMusicLoopStartTime() to retrieve loop start time position
- Mix_GetMusicLoopEndTime() to retrieve loop end time position
- Mix_GetMusicLoopLengthTime() to retrieve a length of looping area

Created attachment 4127 [details]
1145.patch

Introduce MetaTags api
https://bitbucket.org/Wohlstand/sdl_mixer/commits/ae9a63148ec2950b22dabf501c9f1236e31331b2?at=music-interfaces-ex2

In some games or players it's would be possible to print a title and
some minor tags about playing music.

There are 5 added functions which can give meta-tags from music if they
are available or supported by a codec:
- Mix_GetMusicTitle() gives a song title.
  Unlike Mix_GetMusicTitleTag, returns a filename if tag is blank.
- Mix_GetMusicTitleTag() gives a song title
- Mix_GetMusicArtistTag() gives an artist name
- Mix_GetMusicAlbumTag() gives an album name
- Mix_GetMusicCopyrightTag() gives a copyright tag

P.S. A note about Multi-Music: https://github.com/WohlSoft/SDL-Mixer-X/issues/2

Created attachment 4128 [details]
1147.patch

Change default sample rate to 44100
https://bitbucket.org/Wohlstand/sdl_mixer/commits/47ce1bb96fefb20fc94dfc90e30967f9daea4a55?at=music-interfaces-ex2

22050 for a very long time ago, is no more widely used. The 44100 is the most
popular for ten and more years. I don't think here is any reason to keep this
macro to have 22050 sample rate.

Created attachment 4129 [details]
1152.patch

playmus.c: Add more information printing
https://bitbucket.org/Wohlstand/sdl_mixer/commits/8d269d3ce5b093060e9f335fab4784e16c99aac9?at=music-interfaces-ex2

- Meta-tags if available
- Loop points if available
- Current position streaming if possible

This one obviously depends on functionality added by previous patches.

> music_mad.c (MAD_Seek): Avoid a junk chunk be played after seek

This is a small bugfix that would go out of this workflow

> BTW, I looking for an alternative to modified libID3TAG made by a team who made > MAD codec (I modded it to equip it with SDL_RWops support) library which is
> GPL, so, I will give the dummy only, and it needs to:
> - keep as-is and don't parse ID3 tags at all
> - make some minor ID3 tag parsing

Also, instead of this library, I built my own tag parsing, so, this dependency is no more dependency.

Created attachment 4132 [details]
1142.patch (cleaned-up)

(In reply to Ozkan Sezer from comment #20)
> Created attachment 4125 [details]
> 1142.patch
> 
> Add Mix_GetMusicPosition() call to retrieve a current music playing position
> https://bitbucket.org/Wohlstand/sdl_mixer/commits/
> c639c940542cffb0a629b3d4910b0358bebe8bcb?at=music-interfaces-ex2

Cleaned-up version of the patch, which removes an unnecessary NULL
check from FLAC_Tell().

Created attachment 4133 [details]
1145a.patch (clean-ups for 1145.patch)

(In reply to Ozkan Sezer from comment #22)
> Created attachment 4127 [details]
> 1145.patch
> 
> Introduce MetaTags api
> https://bitbucket.org/Wohlstand/sdl_mixer/commits/
> ae9a63148ec2950b22dabf501c9f1236e31331b2?at=music-interfaces-ex2
> 
> In some games or players it's would be possible to print a title and
> some minor tags about playing music.
> 
> There are 5 added functions which can give meta-tags from music if they
> are available or supported by a codec:
> - Mix_GetMusicTitle() gives a song title.
>   Unlike Mix_GetMusicTitleTag, returns a filename if tag is blank.
> - Mix_GetMusicTitleTag() gives a song title
> - Mix_GetMusicArtistTag() gives an artist name
> - Mix_GetMusicAlbumTag() gives an album name
> - Mix_GetMusicCopyrightTag() gives a copyright tag

Cleanup after MetaTags patch: (my review)

- music_flac.c (flac_metadata_music_cb): remove unnecessary style change
- music.c: fix typo utiltiy -> utility
- music.c (meta_tags_set): remove unnecessary memset(0)
- music.c (struct _Mix_Music): rename music_filename to filename
- music.c: add get_last_dirsep() inline to OS-specifically get the
  last directory separator.
- music.c (Mix_LoadMUS): use get_last_dirsep() fon music->filename

If OK, and if the original patch is OK'ed,
can apply the two combined.

> - music.c (meta_tags_set): remove unnecessary memset(0)

That memset(0) I did by historical reasons where I had a non-terminated strings caused a leading junk. memset(0) should guarantee the termination will always be. Probably, I had a deal with some buggy implementations of strcpy() in a past. Should be fine if SDL_strlcpy will guarantee a valid null termination.

(In reply to Vitaly Novichkov from comment #30)
> [...]  Should be fine if SDL_strlcpy will guarantee a valid
> null termination.

Yes it does.

(In reply to Vitaly Novichkov from comment #26)
> > music_mad.c (MAD_Seek): Avoid a junk chunk be played after seek
> 
> This is a small bugfix that would go out of this workflow

Why precisely is this needed?

(In reply to Ozkan Sezer from comment #32)
> (In reply to Vitaly Novichkov from comment #26)
> > > music_mad.c (MAD_Seek): Avoid a junk chunk be played after seek
> > 
> > This is a small bugfix that would go out of this workflow
> 
> Why precisely is this needed?

If you'll try to seek, if you'll don't clear the buffer, the garbage left after previously played frame will be played afger seek until play a frame on a target position.

(In reply to Vitaly Novichkov from comment #33)
> (In reply to Ozkan Sezer from comment #32)
> > (In reply to Vitaly Novichkov from comment #26)
> > > > music_mad.c (MAD_Seek): Avoid a junk chunk be played after seek
> > > 
> > > This is a small bugfix that would go out of this workflow
> > 
> > Why precisely is this needed?
> 
> If you'll try to seek, if you'll don't clear the buffer, the garbage left
> after previously played frame will be played afger seek until play a frame
> on a target position.

I really don't see how, what am I missing? (Maybe bad day for me.)

After seek-to-start we start doing read_next_frame(). Whether you
clear the buffer or not, it writes to input_buffer from the start.
How is this helping with what?

(In reply to Ozkan Sezer from comment #34)
> (In reply to Vitaly Novichkov from comment #33)
> I really don't see how, what am I missing? (Maybe bad day for me.)
> 
> After seek-to-start we start doing read_next_frame(). Whether you
> clear the buffer or not, it writes to input_buffer from the start.
> How is this helping with what?

I made the demo:

https://www.youtube.com/watch?v=nCut01tnNw4
- seek causes previous frame be played, it's hearable when seeking into begin of the song
- I uncommenting my fixing line and seek doesn't cause the previous fragment of a frame to be played anymore.

(In reply to Ozkan Sezer from comment #18)
> Created attachment 4123 [details]
> 1139.patch
> 
> music_mad.c (MAD_Seek): Avoid a junk chunk be played after seek

Applied this one after minor comment editing:
https://hg.libsdl.org/SDL_mixer/rev/760a8cb05f97

The issue itself can be reproduced by applying the following
to playmus.c, running "./playmus -i some.mp3"  and hitting 0,
1, 2, 3 or 4 (followed by Enter) during playback.

diff --git a/playmus.c b/playmus.c
--- a/playmus.c
+++ b/playmus.c
@@ -75,6 +75,11 @@ void Menu(void)
     fflush(stdin);
     if (scanf("%s",buf) == 1) {
         switch(buf[0]){
+        case '0': Mix_SetMusicPosition(0);break;
+        case '1': Mix_SetMusicPosition(10);break;
+        case '2': Mix_SetMusicPosition(20);break;
+        case '3': Mix_SetMusicPosition(30);break;
+        case '4': Mix_SetMusicPosition(40);break;
         case 'p': case 'P':
             Mix_PauseMusic();
             break;

Got no reaction to any of the patches, so I applied the
first patchset:

https://hg.libsdl.org/SDL_mixer/rev/9835d67a27f9
https://hg.libsdl.org/SDL_mixer/rev/70412138c859
https://hg.libsdl.org/SDL_mixer/rev/d711a86866aa
https://hg.libsdl.org/SDL_mixer/rev/24ca9a03d51c
https://hg.libsdl.org/SDL_mixer/rev/57a746548d9e
https://hg.libsdl.org/SDL_mixer/rev/71510d620652
https://hg.libsdl.org/SDL_mixer/rev/6bf0bc6d510c

Keeping this open for future patchsets.

Great!
Approximately at this evening, I'll try to backport two next things:
- Ability to initialize and use the mixer without it taking over the SDL audio callback, made by krcroft https://github.com/WohlSoft/SDL-Mixer-X/pull/20
- MIDI switch API (you'll be able to switch MIDI library between songs, that is also used at me to use a different MIDI library with different songs to define the sounding).
- Extra useful music and SFX playing calls
- Take my version of mp3utils.c to allow MP3 and WAV/AIFF-side ID3v2 be parsed

@Ozkan Sezer, can you check out my mp3utils.c and fix anything you'll find? A bit later I'll equip it with a length value I should take from ID3v2 tag and give the one another alternative way to recognize length of MP3s with using of ID3v2-stored data.

(In reply to Ozkan Sezer from comment #37)
> Got no reaction to any of the patches, so I applied the
> first patchset:
> 
> https://hg.libsdl.org/SDL_mixer/rev/9835d67a27f9
> https://hg.libsdl.org/SDL_mixer/rev/70412138c859
> https://hg.libsdl.org/SDL_mixer/rev/d711a86866aa
> https://hg.libsdl.org/SDL_mixer/rev/24ca9a03d51c
> https://hg.libsdl.org/SDL_mixer/rev/57a746548d9e
> https://hg.libsdl.org/SDL_mixer/rev/71510d620652
> https://hg.libsdl.org/SDL_mixer/rev/6bf0bc6d510c
> 
> Keeping this open for future patchsets.

After my recent addition of libxmp support, I want to wrap up this bug,
and then close it.  As far as I can see, the rest of the MixerX changes
are too specialized and need not go into mainstream SDL_mixer -- so the
feature-set for this bug entry is done, as far as I am concerned.

My comments about current state:

- I am not overly happy with the metatags patchset, i.e.:
  https://hg.libsdl.org/SDL_mixer/rev/24ca9a03d51c
  The mp3 side of it is absent, and home-brewing a mp3 tag parser seems
  time consuming and error-prone.
  My suggestion would be removing the metatags patchset.

Sam, Ryan: What are your opinions? Anything to add? Anything to remove?

(In reply to Ozkan Sezer from comment #39)
Thanks for bringing this up, I gonna make my own comments on this:

> the rest of the MixerX changes are too specialized and need not go into mainstream SDL_mixer

It's about changes that were added for libADLMIDI, libOPNMIDI, and libGME libraries I had to add for the purposes of my projects, especially the system of path arguments and track selection specific calls (mainly needed for the libGME library). Also, changes with the option of adding the __stdcall call conversion to build the VB6 binding library I made by request of some Chinese folks who developed games on this and using the VB6. Also, my recent alternative NativeMIDI player allows the seek-ability/loop tags/tempo changes/etc, and also, has the MSGS volume workaround that avoids the Windows-side bug. does use the C++ code piece that I don't backport into the mainstream until I'll recode it into the pure-C.

I can try to list changes that can be freely ported into the mainstream (at the bottom of this post)

> - I am not overly happy with the metatags patchset, i.e.:
>   https://hg.libsdl.org/SDL_mixer/rev/24ca9a03d51c
>   The mp3 side of it is absent, and home-brewing a mp3 tag parser seems
>   time consuming and error-prone.
>   My suggestion would be removing the metatags patchset.

Metatags API is still useful for OGG/Opus/FLAG/Trackers/WAV music formats. If you want to avoid the dangerous MP3 tag parsing code, just don't support meta-tags for MP3 formats at the mainstream. At least, the MP3 format is a pain because of its ineffective nature and the complicated case of tags for this format.

What features of MixerX can fit into the mainstream (may be incomplete):
- Run-time side MIDI change API: you can choose the library to play MIDI on runtime, and the next opened MIDI file will use the selected MIDI library. There are Timidity, FluidSynth, and Native MIDI. On the MixerX side, there are also libADLMIDI and libOPNMIDI. If you don't want to support these libraries, just don't support them, make the only MIDI select API.
- Ability to initialize the Mixer library without of SDL_Audio taking, I.e. Allow the Mixer to re-use an existing SDL_Audio initialized separately by the side, see the pull-request and the discussion https://github.com/WohlSoft/SDL-Mixer-X/pull/20
- Tempo change which is useful for the XMP right now, at MixerX it's also useful for libADLMIDI, libOPNMIDI, and libGME libraries. I keep some plans to add some PCM algorithms to make PCM-based streams also can change their tempo and tone. (Just making them play faster/slower is easy at least, but not good as people told me in the discussion).
- My recent clean-up where I moved all Vorbis/FLAC/Opus loop tags into the utils.c (https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/utils.c) (this file also contains other utility calls you don't need)
- Support the linking with both FluidSynth 2 and 1, the time is going and there is FluidSynth 2 getting widely. Right now at me on Linux Mint 20.1 the FluidSynth 2.1.1 is used. The difference is the return type of the delete_fluid_player() and the delete_fluid_synth() calls: https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/music_fluidsynth.c#L34-L38. Maybe there would be a better solution that allows dynamic use of each FluidSynth 1 and FluidSynth 2 by making some of the calls be optional and be a replacement to each other.
- At the libMAD I made the support for the Tell and the Duration which is still not backported into the mainstream.
- More accurate MP3 detector for the magic-number scanner (see https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/music.c#L736-L816)
- Stopping to trust the filename extension to recognize the file type (I saw several problems because some "so smart" users had to try "convert" the format by just renaming. That caused the file won't play because of an error. The file itself is valid but has the lying filename extension. I avoided this by making the format detection by the magic number only except the set of formats that is hard to recognize by the content).
- Ability to play chunks with the given volume value (calls such as "int Mix_PlayChannelTimedVolume(int which, Mix_Chunk *chunk, int loops, int ticks, int volume)"). I added this API because it's a pain when playing voices with different volumes and using the -1-random free channel that doesn't set/reset the volume, which causing the unexpected chunk volume change. I had to add those calls without ABI breaking, so, the existing Mix_FadeInChannelTimed() is a function, not a macro, but inside it calls the new Mix_FadeInChannelTimedVolume() with an ignorance of the initial volume set.

I gonna resume my work on making the mainstream patches as soon as possible. I had to pause my work because of business with the more important tasks at that moment.

The small note about the MP3 detector I made: I have several files that won't be detected by the current MP3 detector code at the mainstream Mixer, especially the case when file had an ID3 tag that was filled by zeroes, and therefore, the result is always failed.

(In reply to Vitaly Novichkov from comment #40)
> - More accurate MP3 detector for the magic-number scanner (see
> https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/music.c#L736-L816)

(In reply to Vitaly Novichkov from comment #41)
> The small note about the MP3 detector I made: I have several files that
> won't be detected by the current MP3 detector code at the mainstream Mixer,
> especially the case when file had an ID3 tag that was filled by zeroes, and
> therefore, the result is always failed.

If ID3 is the only problem with detection:

- Is the ID3 tag broken? I.e.: what does it mean "an ID3 tag that
  was filled by zeroes" ?

- We do not have any sample files here which fail detection. Care
  to send or give links?

- Does detection work if ID3 is skipped?

- Your change seems to do a lot of search, can it not lead to any
  mis-detections?

(In reply to Vitaly Novichkov from comment #40)
> - Support the linking with both FluidSynth 2 and 1, the time is going and
> there is FluidSynth 2 getting widely. Right now at me on Linux Mint 20.1 the
> FluidSynth 2.1.1 is used. The difference is the return type of the
> delete_fluid_player() and the delete_fluid_synth() calls:
> https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/codecs/
> music_fluidsynth.c#L34-L38. Maybe there would be a better solution that
> allows dynamic use of each FluidSynth 1 and FluidSynth 2 by making some of
> the calls be optional and be a replacement to each other.

This one is a simple change that can be added independent of this bug.

(In reply to Ozkan Sezer from comment #43)
> (In reply to Vitaly Novichkov from comment #40)
> > - Support the linking with both FluidSynth 2 and 1, the time is going and
> > there is FluidSynth 2 getting widely. Right now at me on Linux Mint 20.1 the
> > FluidSynth 2.1.1 is used. The difference is the return type of the

> This one is a simple change that can be added independent of this bug.

Fixed in both SDL-1.2 and SDL2.0 (default) branches.

(In reply to Ozkan Sezer from comment #42)
> 
> If ID3 is the only problem with detection:
> 
> - Is the ID3 tag broken? I.e.: what does it mean "an ID3 tag that
>   was filled by zeroes" ?

This means at beginning of the file is the just zeroes sequence until the actual MP3 data begins. I meant that in the file sample I had, was an attempt to remove the ID3 tag by just filling it with zeroes. Because of that, the MP3 file gets miss detected when using a magic-number way.

> - We do not have any sample files here which fail detection. Care
>   to send or give links?

Lemme check my archives, I have at least one of such broken files, I still remind the song name.

> 
> - Does detection work if ID3 is skipped?
It can't skip ID3 because the file has no ID3, it has just a sequence of zeroes at begin.

> 
> - Your change seems to do a lot of search, can it not lead to any
>   miss-detections?
I polished this function on many files I had around, and it doesn't miss-detect non-MP3 files in the rest. I do search the MP3 header in the first 10240 bytes. In the past, I had another variant of this detector that was less accurate, and it had some miss-detections on random files. 

How it works:
- if ID3 at the beginning of a file as usually
- gets the end of the file
- checks if the first 4 bytes are zeroes, then it skips the reading loop and goes to check the MPEG header that fails and starts the scanner (I don't remind why I made this, I may guess this part of code is junk)
- does the search loop that finds any non-zero bytes in the stream until condition:
-- found a non-zero byte
-- bytes left less than 4 to end
-- seek position is more than 10240 bytes
-- end of file has reached
- Does the well-known check

Anyway, to confirm this algorithm works fine, I gonna make the unit test that will scan thousands of files I have in my collection and give me any possible miss-detections such as:
- MP3 file got detected as not-MP3
- not-MP3 file got detected as MP3

Created attachment 4764 [details]
An example MP3 file with the sequence of zeroes at begin

This is one of the MP3 files that won't be detected properly with the older MP3 detector.

> > If ID3 is the only problem with detection:
> > 
> > - Is the ID3 tag broken? I.e.: what does it mean "an ID3 tag that
> >   was filled by zeroes" ?
> 
> This means at beginning of the file is the just zeroes sequence until the
> actual MP3 data begins. I meant that in the file sample I had, was an
> attempt to remove the ID3 tag by just filling it with zeroes. Because of
> that, the MP3 file gets miss detected when using a magic-number way.

Unless I'm missing something, this sounds like a user-shoots-foot
situation..

(In reply to Ozkan Sezer from comment #47)
> Unless I'm missing something, this sounds like a user-shoots-foot
> situation..

This file I had to attach, perfectly opens in any other players include VLC, Windows Media Player, etc. etc.

I got this file in the wild many years ago, and together with this, I have a dozen of others. I also found some examples where the file got chopped in a middle, and the MPEG frame got offset. Even that, those files also getting be played fine anywhere.

(In reply to Vitaly Novichkov from comment #48)
> (In reply to Ozkan Sezer from comment #47)
> > Unless I'm missing something, this sounds like a user-shoots-foot
> > situation..
> 
> This file I had to attach, perfectly opens in any other players include VLC,
> Windows Media Player, etc. etc.

$ file moi_drug_luchshe_vseh_igraet_blu.mp3
moi_drug_luchshe_vseh_igraet_blu.mp3: data

I'll let Sam and/or Ryan to decide.