Skip to content

Commit

Permalink
Detect MP3 files with ID3v1 and APETAG tags
Browse files Browse the repository at this point in the history
  • Loading branch information
Wohlstand committed Nov 27, 2019
1 parent 3852840 commit 29a9e82
Showing 1 changed file with 4 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/music.c
Original file line number Diff line number Diff line change
Expand Up @@ -714,14 +714,16 @@ static int detect_ea_rsxx(SDL_RWops *in, Sint64 start, Uint8 magic_byte)
static int detect_mp3(Uint8 *magic, SDL_RWops *src, Sint64 start)
{
Uint32 null = 0;
Uint8 magic2[5];
Uint8 magic2[8];
unsigned char byte = 0;
Sint64 endPos = 0;
Sint64 notNullPos = 0;

SDL_memcpy(magic2, magic, 5);
SDL_memcpy(magic2, magic, 9);

if (SDL_strncmp((char *)magic2, "ID3", 3) == 0 ||
SDL_strncmp((char *)magic2, "APETAGEX", 8) == 0 ||
SDL_strncmp((char *)magic2, "TAG", 3) == 0 ||
(magic[0] == 0xFF && (magic[1] & 0xFE) == 0xFA)) {
SDL_RWseek(src, start, RW_SEEK_SET);
return 1;
Expand Down

8 comments on commit 29a9e82

@sezero
Copy link
Collaborator

@sezero sezero commented on 29a9e82 Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. 'TAG' (i.e. id3v1 magic) is at the end of the file. 'APETAGEX' very most usually appears at the end, too (don't know of any where APE magic is at the start.) See music_mad.c where I detect the tags.

@Wohlstand
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. 'TAG' (i.e. id3v1 magic) is at the end of the file. 'APETAGEX' very most usually appears at the end, too (don't know of any where APE magic is at the start.) See music_mad.c where I detect the tags.

Yeah, I'll remove the "TAG", my mistake. On my side I working for the code that will parse all tags in a place, then I'll drop the use of libID3TAG library.

@sezero
Copy link
Collaborator

@sezero sezero commented on 29a9e82 Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not going out of scope? SDL_mixer is supposed to be a mixer library, not a Swiss army knife of all audio/music formats, no?

@Wohlstand
Copy link
Member Author

@Wohlstand Wohlstand commented on 29a9e82 Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the library is already getting to be a Swiss army knife for audio management (even without of MixerX), it already has:

  • effects system include some built-in effects are procesing audio stream
  • internal API intended to support many audio formats to play and stream (so, having this structure it's easy to quickly implement a support for a new format)

About meta-tags: I have two reasons why I added them:

  • in one of games where MixerX is used, is made a feature to print out music title and an artist by user's request (rather to enforce manual filling of a song title in some external file, it's much better to read it from tags)
  • because of my player for tests that can print these tags

And:

  • most of codecs we have are having built-in ability to retrieve these tags (Vorbis, Opus, FLAC, ModPlug, MikMod), so, here was a question of building a bridge to allow users extract such information
  • MP3 is one mess that requies additional code to parse these tags

BTW: here is one missing functionality that would be cool to have, and I have a plan to make that: did you saw in my header functions like Mix_*MusicStream() are always requiring Mix_Music* context? I want to provide ability to play multiple music streams like it's possible with chunks now. Why? It's a required functionality for cross-fade effect and it's required to process environment SFX streams (forest leaves, wind, etc.) are usually heavy and long like musics. Also this will allow to play music saved as different instrument parties that will allow to enable/disable certain track to introduce more dynamic for in-game music. #2

@Wohlstand
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S. I think, after merge of current stuff with mainstream Mixer, I'll keep MixerX as a playgroud for new functionality which can be later send into regular Mixer on stabilizing. What do you think?

@sezero
Copy link
Collaborator

@sezero sezero commented on 29a9e82 Nov 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, don't add any additional stuff and let the existing ones get
reviewed, picked and merged.

When preparing your changesets, break things into many individual,
preferably independent changesets, like a new api addition with
rationale. Keep 'specialist' additions as the last, e.g.: an api
addition only serving your GME or libadlmidi's needs, and keep them
independent of everything else too whenever possible.

@Wohlstand
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, first off I going to backport guts without libraries and APIs yet:

  • FORMATS: extending of WAV parser (I have polished it to support much more WAV formats which wasn't supported good). Looks like I'll port this now, it can be backported now
  • GUTS: extending of Mix_MusicInterface with new calls
  • API: Ability to set custom patches path for Timidity (for example, allow game developer to pick-up own bank with a game and don't rely on these are installed in the system).
  • API & GUTS: ability to switch MIDI device in real time (will add new API calls), adding support of path arguments to allow select the MIDI library by using of |sX; tail.
  • API: meta-tags
  • API: more music related calls
  • API: more SFX related calls (mainly Mix_PlayChanTimedVolume() to solve one of oldest problems when we usually getting old volume value of channel and we can't tell which volume we want to play, mainly, when we using -1 for automatical look up of the channel index)
  • API + GUTS: this Add ability to initialize and use the mixer without it taking over the SDL audio callback #20
  • FORMATS & API: Adding a support for more format libraries (ADLMIDI, OPNMIDI, GME and XMP), adding specific API to allow tuning of every of these libraries

(I'll add this list into the specific issue for better visibility...)

@Wohlstand
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.