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

Don't format LRC timestamp with DDdHH:MM:SS #297

Merged
merged 1 commit into from
Dec 21, 2024

Conversation

gnattu
Copy link
Contributor

@gnattu gnattu commented Dec 21, 2024

LRC format generally doesn’t support days and hours because the original format explicitly uses the [mm:ss.xx]lyric format. Encoding synchronized lyrics with the current DDdHH:MM:SS format would break certain LRC parsers that weren’t designed to handle hours and days. For example, the LrcParser used by Jellyfin was using regex to match the mm:ss pattern: https://github.com/kfstorm/LrcParser/blob/95aaffde330efaa252b53654803e7d8794048fc2/LrcParser/LrcFile.cs#L157

This PR now formats the timestamps in MM:SS.UUUU format and allows MM to extend beyond double digits when necessary, which should be more compatible than current format.

See jellyfin/jellyfin#13260 for more info

LRC format generally doesn’t support days and hours because the original format explicitly uses the `[mm:ss.xx]lyric` format. Encoding synchronized lyrics with the current DDdHH:MM:SS format would break certain LRC parsers that weren’t designed to handle hours and days.
@Zeugma440
Copy link
Owner

Looks super fine. Thanks for the PR!

@Zeugma440 Zeugma440 merged commit 4c2cd9a into Zeugma440:main Dec 21, 2024
1 check failed
@gnattu gnattu deleted the don't-format-lrc-with-hoursdays branch December 21, 2024 18:19
@Zeugma440
Copy link
Owner

PR has just been shipped with today's v6.11

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

Successfully merging this pull request may close these issues.

2 participants