search

libsdl-org / SDL_mixer

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

Merge SDL_mixer with SDL Mixer X (my extended fork of SDL_mixer) #233

Open SDLBugzilla opened 3 years ago

SDLBugzilla commented 3 years ago

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: unspecified Reported for operating system, platform: All, All

Comments on the original bug report:

On 2017-10-21 20:07:04 +0000, Vitaly Novichkov wrote:

Hello! I'm a creator of my own fork of SDL Mixer library I began by many purposes and a wish to extend it:

https://github.com/WohlSoft/SDL-Mixer-X

What I implemented into this fork:

  • Added new codecs: libGME (Game music Emulators), libADLMIDI (Emulated FM synth MIDI player based on OPL3 chip), libOPNMIDI (another emulated FM synth MIDI player based on OPN2 chip). (The reason I added libADLMIDI and libOPNMIDI - there are requires no extra banks and there are able to play MIDI everywhere, just everything is generated from scratch by formulas. Also, I'm a fan of FM synthesis and I like to use FM chips to create/listen music :-) ).
  • Added ability to toggle MIDI sequencer in real time (unlike of internal auto-choosing, you can manually choose which MIDI sequencer you wish to use).
  • Added ability to read meta-tags from some files (song title, author, copyright, album title)
  • Because was "seek" the only command, I have added "tell" to retrieve a current position of a song (I have used that in my PGE MusPlay tool) and I also added support to retrieve loop tags positions too (which are in OGG file).
  • Added a workaround - simple internal resampler while in that time SDL had it very bad and glitchy.
  • libMAD playing code where I fixed various bugs and I added usage of the libid3tag to recognize start position of the song data (some MP3 with ID3 tags are causing CRASH of library when libMAD tries to play it, and it plays, but result is glitchy until crash will appear)
  • Wavestream renamed into music_wave, where I changed strategy to make it simpler and less glitchy. I added the ability to support multiple streams with the same codec. I want to make "Multi-Music" API that will play multiple music streams by the same strategy as music.c (for example, very long atmosphere SFX or dynamical music tracks where you can turn on/off some musical instruments, I heard that Half-Life had that). (This also required to add support of cross-fade)
  • I have added ability to redefine timidity config patch to get ability have timidity patches everywhere
  • Extra arguments are can be given together with file path (to choose ADLMIDI bank or choose track in the multi-track NSF/HES file)
  • Rewritten music.c to have friendly interface instead of heavy and scary switch-cases. (I did that before you recently did the same).
  • Reorganized folders and files (instead of a flood of the root folder, I made a beautiful structure where I put codecs modules independently from common source code, header into "include" and all internal files into "src").
  • Nuked Autotools and used QMake and CMake

I want to synchronize our works to support same. For now, I graduated applying your latest changes to my fork to support same new things as you are introduced recently.

P.S. My code style in most is "Allman", therefore our codes are having differences (I did use "Artistic Style" beautifier to beautify some of the source files) and I'm a fan of spaces rather tabs. So, If you wish to keep your style as you like, can you describe style you following strictly (brackets, intending, spaces/tabs/ etc.) and which are doesn't matter and I'm free to use that style I using myself?

P.P.S. Myself I using QMake and CMake to build stuff and I did remove Autotools from me as hard to maintain it. Also, I made usage of Qmake and CMake for each dependency (except SDL2 which already has CMake). Therefore I would need your help to keep the correct support of Autotools without breaking of them.

On 2018-01-21 00:06:45 +0000, Vitaly Novichkov wrote:

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

On 2019-11-17 17:36:22 +0000, Vitaly Novichkov wrote:

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.

On 2019-11-17 18:55:33 +0000, Sam Lantinga wrote:

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?

On 2019-11-17 19:10:56 +0000, Vitaly Novichkov wrote:

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?

On 2019-11-17 19:12:49 +0000, Vitaly Novichkov wrote:

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).

On 2019-11-17 19:16:48 +0000, Vitaly Novichkov wrote:

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"?

On 2019-11-17 19:32:59 +0000, Ozkan Sezer wrote:

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.

On 2019-11-17 20:01:51 +0000, Vitaly Novichkov wrote:

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.

On 2019-11-18 01:38:48 +0000, Sam Lantinga wrote:

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.

On 2019-11-18 08:21:27 +0000, Vitaly Novichkov wrote:

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

On 2019-11-19 06:23:24 +0000, Sam Lantinga wrote:

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?

On 2019-11-19 06:34:16 +0000, Ozkan Sezer wrote:

(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.

On 2019-11-19 06:50:43 +0000, Vitaly Novichkov wrote:

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.

On 2019-11-19 09:41:51 +0000, Vitaly Novichkov wrote:

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).

On 2019-11-20 15:44:01 +0000, Ozkan Sezer wrote:

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.)

On 2019-12-17 08:03:51 +0000, Ozkan Sezer wrote:

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

On 2019-12-21 12:19:28 +0000, Ozkan Sezer wrote:

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.

On 2019-12-21 12:19:59 +0000, Ozkan Sezer wrote:

Created attachment 4123 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

On 2019-12-21 12:20:34 +0000, Ozkan Sezer wrote:

Created attachment 4124 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.

On 2019-12-21 12:21:01 +0000, Ozkan Sezer wrote:

Created attachment 4125 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

On 2019-12-21 12:21:36 +0000, Ozkan Sezer wrote:

Created attachment 4126 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

On 2019-12-21 12:22:08 +0000, Ozkan Sezer wrote:

Created attachment 4127 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

On 2019-12-21 12:22:34 +0000, Vitaly Novichkov wrote:

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

On 2019-12-21 12:22:41 +0000, Ozkan Sezer wrote:

Created attachment 4128 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.

On 2019-12-21 12:23:08 +0000, Ozkan Sezer wrote:

Created attachment 4129 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.

On 2019-12-21 12:28:26 +0000, Vitaly Novichkov wrote:

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

On 2019-12-21 12:32:25 +0000, Vitaly Novichkov wrote:

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.

On 2019-12-21 17:10:33 +0000, Ozkan Sezer wrote:

Created attachment 4132 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().

On 2019-12-21 17:13:22 +0000, Ozkan Sezer wrote:

Created attachment 4133 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.

On 2019-12-21 20:35:43 +0000, Vitaly Novichkov wrote:

  • 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.

On 2019-12-21 20:52:07 +0000, Ozkan Sezer wrote:

(In reply to Vitaly Novichkov from comment # 30)

[...] Should be fine if SDL_strlcpy will guarantee a valid null termination.

Yes it does.

On 2019-12-22 06:14:00 +0000, Ozkan Sezer wrote:

(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?

On 2019-12-22 07:39:56 +0000, Vitaly Novichkov wrote:

(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.

On 2019-12-22 12:45:11 +0000, Ozkan Sezer wrote:

(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?

On 2019-12-22 13:23:53 +0000, Vitaly Novichkov wrote:

(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.

On 2019-12-22 14:26:11 +0000, Ozkan Sezer wrote:

(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;

On 2019-12-23 11:15:20 +0000, Ozkan Sezer wrote:

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.

On 2019-12-23 15:38:45 +0000, Vitaly Novichkov wrote:

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.

On 2021-02-05 12:16:44 +0000, Ozkan Sezer wrote:

(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?

On 2021-02-05 13:05:34 +0000, Vitaly Novichkov wrote:

(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.

On 2021-02-05 13:10:44 +0000, Vitaly Novichkov wrote:

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.

On 2021-02-05 14:49:28 +0000, Ozkan Sezer wrote:

(In reply to Vitaly Novichkov from comment # 40)

(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?

On 2021-02-05 14:52:00 +0000, Ozkan Sezer wrote:

(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.

On 2021-02-05 18:41:59 +0000, Ozkan Sezer wrote:

(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.

On 2021-02-05 19:11:12 +0000, Vitaly Novichkov wrote:

(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

On 2021-02-05 19:13:50 +0000, Vitaly Novichkov wrote:

Created attachment 4764 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.

On 2021-02-05 19:26:54 +0000, Ozkan Sezer wrote:

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..

On 2021-02-05 19:28:42 +0000, Vitaly Novichkov wrote:

(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.

On 2021-02-05 19:31:49 +0000, Vitaly Novichkov wrote:

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.

On 2021-02-05 19:43:25 +0000, Ozkan Sezer wrote:

(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.

sezero commented 3 years ago

@slouken: Waiting your comments. See comments from 2021-02-05 12:16:44 - and later. (I miss Bugzilla...)

sezero commented 3 years ago

Adding Vitaly's mad duration patch here, so I don't lose it (and others may possibly review it, too.)

mad-tell-duration-patch.txt

Wohlstand commented 3 years ago

Adding Vitaly's mad duration patch here, so I don't lose it (and others may possibly review it, too.)

mad-tell-duration-patch.txt

Maybe post it as a pull-request?

P.S. I'll be available tomorrow (UTC+3), now I gonna sleep.

slouken commented 5 months ago

@Wohlstand, let's revisit this for SDL_mixer 3.0?

It would be nice to put our heads together on features available in each library and see if it makes sense to create a merged version.

Wohlstand commented 5 months ago

Hello! A good note, I can help. At first, I'll write the full list of MixerX's features that were already being implemented and supported, and then you can give some about that. Just a note, we can perform the thing FASTER if we reduce the amount of buerocraty against every feature. There is a serious problem that many features at MixerX had been developed a while ago, and several of them depends on each other, and it's pretty hard to port each of them separately, but its possible to port as a "biggie" that can be integrated much easier.

P.S. everything was done over Mixer 2, and I took an effort to keep the full forward compatibility (including ABI compatibility to apps linked to original Mixer).

What about stability: the whole thing gets tested and polished while it used in several projects (mine as well, as other's people whom I don't know ever, and they reports me my bugs sometimes).

Wohlstand commented 4 months ago

@slouken

Okay, I going to share my set of features that MixerX has right now which original Mixer doesn't:

ALSO:

I may extend the list if I remember something also I forgot to mention.