Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDL3] Fixed detection of FLAC files with ID3 tag at begin and extending MP3 detection #527

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Wohlstand
Copy link
Contributor

@Wohlstand Wohlstand commented May 31, 2023

Recently I found that some FLAC files won't play at all. I had checked, and I found it begins with an ID3 tag. Also, I checked around the stuff, and it's not a paranomrmal mess of Audacity: it's one of legacy forms of FLAC files that used ID3 tags at begin.

There is a comment where I explained about this a first time: WohlSoft/SDL-Mixer-X@94e8750

Example file (I exported it in 2019'th year by the old version of Audacity): Fox fantasy.mid-battle-chess-gmized.flac.zip

Also, I had to extended the MP3 detection code to ensure that more MP3 files will be properly detected:

  • Veber_-_Rio_Rita.mp3.zip - contains an unexpected junk after ID3 tag, it WILL be misdetected by the given formula.
  • Also, a lot of MP3 files that starts with a sequence of NULL bytes (possibly because of shitty tag remover processing)
  • Some MP3 files may begin at the middle of a frame, so, need to detect the nearest valid frame.

@sezero
Copy link
Contributor

sezero commented May 31, 2023

Like I said in original commit: The idea of FLAC files with ID3 tags??
I shall vomit...

This may be one way of handling it. (Labeling a file starting with ID3
as mp3 is flaky to start with...) Any other ways to do this?

@Wohlstand
Copy link
Contributor Author

Wohlstand commented May 31, 2023

Maybe use my own implementation of detect_mp3()? https://github.com/WohlSoft/SDL-Mixer-X/blob/master/src/music.c#L1385-L1456

I mean, call it after skipping the ID3 tag (and inside the detect_mp3() itself, I should remove the check for the presense of ID3).

I polished it for many years on thousands of various MP3 files that I had at my personal collections.

Does this sounds better?

Also: put the detect_mp3 calling into the LOWEST position of the whole list to ensure all previous checks hadn't detected any known file format, like I did at MixerX.

@sezero sezero requested review from slouken and icculus May 31, 2023 01:27
@sezero
Copy link
Contributor

sezero commented May 31, 2023

Like I said in original commit: The idea of FLAC files with ID3 tags?? I shall vomit...

And I wonder: how much should we care about flac files with id3?
I.e.: how common are they?

CC: @ktmf01 as libflac maintainer.

@Wohlstand
Copy link
Contributor Author

Wohlstand commented May 31, 2023

At the mainstream FLAC I also do see the code that skips ID3 tags: https://github.com/xiph/flac/blob/1619af5a36fc0343cdf6b3517bb78d8aee85fe59/src/libFLAC/metadata_iterators.c#L3143-L3176

And at the changelog I see, the ID3 tags were being supported in the past, but had been removed, and now, if any FLAC file contains these tags, they just gets skipped. Basically to keep compatibility to let any modern player keep opening these old files.

@ktmf01
Copy link

ktmf01 commented May 31, 2023

And at the changelog I see, the ID3 tags were being supported in the past

We're talking almost 19 years ago here. Support from plugins was removed in October 2004, and the command line programs never supported reading or writing them, only skipping them. There are a few programs around that are able to are able to transcode ID3v1 and ID3v2 to FLAC tags.

edit: the flac command line program gained the ability to re-encode files in 2006, where it just actively removed those tags from the file. So I'd say ID3 tags have been actively unsupported since then. Of course it is not my place to say anything about this, but I would discourage adding any support for them anywhere.

@sezero
Copy link
Contributor

sezero commented May 31, 2023

Then I'd say that we don't apply this patch. @slouken, @icculus?

@Wohlstand
Copy link
Contributor Author

Wohlstand commented May 31, 2023

My patch doesn't adds support: it adds skipping as all current FLAC implementations. Without this patch, all these FLAC files gets recognised as MP3 files which obviously gets refused as invalid by MP3 decoders.

Also, recently at MixerX I slightly tweaked my code to also process the check for MP3 frames after ID3 tag to ensure that it's really an MP3 file and not something also. I could extend this patch by these changes I did.

@Wohlstand Wohlstand changed the title [SDL3] Fixed detection of FLAC files with ID3 tag at begin [SDL3] Fixed detection of FLAC files with ID3 tag at begin and extending MP3 detection May 31, 2023
@ktmf01
Copy link

ktmf01 commented May 31, 2023

The problem is that FLAC files with an ID3v2 block are not valid, according to the FLAC spec. Now, the flac command line tool does have quite some handling of other 'deviations' from the spec. For example, it'll also partially read corrupted files (if asked for), skip corrupted metadata chains, read files with no metadata at all and read files of which the coded number is incorrect.

But 'salvaging' broken files is something that is expected of the flac software, because it is expected to be the most capable piece of software concerning all things FLAC. Ironically, it isn't, because ffmpeg can do a lot of things that flac can't do. Anyway, just because the flac command line tool can read broken files doesn't mean all FLAC reading software should.

@Wohlstand
Copy link
Contributor Author

Wohlstand commented May 31, 2023

I talked about the library libflac, it still able to open these files. The problem at Mixer here is a necessary to properly detect these files, and then let libflac to deal with it by its own. The Mixer doesn't implements actual decode of FLAC, it only detects the file according small fragment of the file data, and then it lets the native library (or the official libflac, or the custom header-only drflac) to deal with these files.

@Wohlstand Wohlstand force-pushed the flacid3-fix-sdl3 branch 2 times, most recently from c24c210 to 43a8d0c Compare May 31, 2023 17:25
@Wohlstand
Copy link
Contributor Author

Rebased the thing.

@Wohlstand
Copy link
Contributor Author

Just now I rebased these changes to the latest state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants