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

Implement AUDIO header #951

Merged
merged 5 commits into from
Jan 26, 2025

Conversation

TheNailDev
Copy link
Contributor

Format 1.1.0 introduced the mandatory AUDIO header as a replacement for the MP3 header.

USDX now requires songs with VERSION >= 1.1.0 to include the AUDIO header and reads the audio file from this header.
The MP3 header is ignored for songs with VERSION >= 1.1.0.

When saving songs, the AUDIO and MP3 headers are written depending on the VERSION of the song, as shown in the table below:

VERSION MP3 AUDIO
< 1.1.0 ✔️
>= 1.1.0 / < 2.0.0 ✔️ ✔️
>= 2.0.01 ✔️

Footnotes

  1. VERSION >= 2.0.0 is currently not supported by USDX. This is just future proofing

for format < 1.1.0 only MP3 is written
for format in range [1.1.0, 2.0.0) AUDIO and MP3 is written
for format >= 2.0.0 only AUDIO is written
Copy link
Member

@barbeque-squared barbeque-squared left a comment

Choose a reason for hiding this comment

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

Done a quick initial pass. Is this the only change in 1.1.0 or is there more? The last thing I want to do is half-support something now that people are actually putting effort into standardizing it. Because people will hack around it and/or report bugs because we're only implementing it partially (and then comes the hotfix but it misses some edge case and now we're stuck with n+1 things for the next 20 years)

Not sure if I'll get to really dig into the spec until the weekend, hence this quick review.

EDIT: personally I'm not really against partial support, especially if it's an added tag like this, but if it also for example demands that #START and #END use sane values, then this is not getting in in its current state. It's a bit of a chicken-egg problem, especially with bigger spec changes it'll be good to split them up across multiple PR's, but ideally it's still all in a single release. Like I said, I need to actually research the spec soon.

src/base/USong.pas Outdated Show resolved Hide resolved
src/base/USong.pas Show resolved Hide resolved
src/base/USong.pas Outdated Show resolved Hide resolved
src/base/UFiles.pas Show resolved Hide resolved
@TheNailDev
Copy link
Contributor Author

Is this the only change in 1.1.0 or is there more?

There are more changes for 1.1.0 (see #921):
New headers in 1.1.0 are AUDIO, VOCALS, INSTRUMENTAL, TAGS and PROVIDEDBY.
Additionally 1.1.0 also changes GENRE, CREATOR, EDITION, LANGUAGE and TAGS to multi-valued headers.

IMO support for format 1.1.0 can be classified in distinct sub-features which can be implemented independently:

  • Implementation of AUDIO header (this PR)
  • Implementation of VOCALS and INSTRUMENTAL (use add karaoke mode via #INSTRUMENTALS or #KARAOKE tag, key to toggle karaoke mode is K while playing. #850 as base)
  • Implementation of the multi valued headers GENRE, CREATOR, EDITION, LANGUAGE and (new header) TAGS
  • I don't see a reason to specifically implement PROVIDEDBY, as it is mostly just an information, from which song hosting service (USDB, spanish site, etc.) the song was downloaded. I don't think this information is relevant in USDX. (Naturally we could implement filtering by the song providers, but is that really something people would use?)

I only checked The english and german languages.
Most other languages simply use the values from the english localisation.
@TheNailDev
Copy link
Contributor Author

I just changed TSong.Mp3 to TSong.Audio.
I did not notice any unexpected side effects.

I also changed the English and German localization to use Audio instead of MP3.

.gitignore Outdated Show resolved Hide resolved
@barbeque-squared
Copy link
Member

re: #951 (comment)

Yeah independent (or at least multiple) PRs is the way to go for that. But can we release this without all the other things? Personally I'm fine with partial support as long as it doesn't really break the not-yet implemented bits, but I'm not an end user.

Also generally agree with your comments on the subfeatures.

@TheNailDev
Copy link
Contributor Author

But can we release this without all the other things?

In my opinion yes. As long as we don't say that we fully support 1.1.0 there should be no Issues.

For songs with Format < 1.1.0 nothing changes.

For songs with Format >= 1.1.0 the AUDIO and MP3 tag should (hopefully) both be valid audio files (usually the same file).
So for those songs also nothing would change. (Unless AUDIO is an invalid file, while MP3 is a valid file. But then I would say that the user should have the responsibility of having valid files and this should not be our issue)

Copy link
Member

@barbeque-squared barbeque-squared left a comment

Choose a reason for hiding this comment

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

Code looks good. Verified that existing songs still work. Did not actually test the new header, but the code looks good enough. Will merge tomorrow.

@barbeque-squared barbeque-squared merged commit 28feced into UltraStar-Deluxe:master Jan 26, 2025
5 checks passed
@TheNailDev TheNailDev deleted the audio-header branch January 27, 2025 15:53
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