lyrics: write synced LRC lyrics to SYLT ID3 frame#6566
Conversation
When the lyrics plugin writes LRC-formatted (timestamped) lyrics to an ID3-tagged audio file, store the timing data in the SYLT (synchronized lyrics) frame and plain text in USLT, rather than writing raw LRC text verbatim to USLT. Fixes beetbox#6541
72bca2f to
dc0a0bf
Compare
2dab9c0 to
1b7b7a2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6566 +/- ##
==========================================
+ Coverage 72.46% 72.69% +0.23%
==========================================
Files 161 161
Lines 20719 20735 +16
Branches 3280 3283 +3
==========================================
+ Hits 15014 15074 +60
+ Misses 4979 4934 -45
- Partials 726 727 +1
🚀 New features to boost your workflow:
|
|
Have you considered adding support for this tag to |
30026c3 to
26759b9
Compare
|
Went ahead and implemented these. Heres a breakdown: -- do we have another option than hardcoding english for everything? -- you should log the error here, completely ignoring it will make debugging harder -- would it make sense to move this into beet's main tag-writing function? / Have you considered adding support to mediafile first? |
Ultimately, writing tags is Would you be happy to add support for |
|
I'll give it a go! |
|
Thank you so much! |
|
Ok I opened a PR in mediafile with the changes. Wasn't as difficult as it looked so I got it done pretty quickly. Its issue #108 in the mediafile repo if you'd like to take a look. If everything looks good there I'll update this PR to accommodate it and we can go from there! |
d9b659a to
78d9f89
Compare
655b409 to
edb1934
Compare
|
Ok it looks like everything is working end to end. Both PR's are up to date. Let me know if there are any more changes that need to be made. |
There was a problem hiding this comment.
Pull request overview
This PR try make lyrics plugin write synced LRC better for ID3: put timing in SYLT, keep plain text for USLT, so more players show lyrics right.
Changes:
- Add
Lyrics.syltto convert[mm:ss.cc]LRC lines into(text, milliseconds)pairs. - Change lyrics plugin write path to store plain text in
item.lyricsand passsynced_lyricstag data when writing. - Update docs/changelog and add tests for SYLT conversion + write-call behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
beets/util/lyrics.py |
Add Lyrics.sylt property to derive SYLT timing tuples from LRC timestamps. |
beetsplug/lyrics.py |
Adjust add_item_lyrics to split plain vs synced timing and pass synced_lyrics via try_write(). |
test/plugins/test_lyrics.py |
Update overwrite expectation for synced lyrics + add new unit tests for Lyrics.sylt and write kwargs. |
docs/plugins/lyrics.rst |
Document SYLT/USLT behavior for synced lyrics on ID3 files. |
docs/changelog.rst |
Add changelog entry for SYLT/USLT behavior change. |
| if lyrics_text not in {None, item.lyrics}: | ||
| item.lyrics = lyrics_text | ||
| item.store() | ||
| if write: | ||
| item.try_write() | ||
| item.try_write(tags={"synced_lyrics": sylt_data}) |
| class TestSyncedLyricsWrite(LyricsPluginMixin): | ||
| """Tests that add_item_lyrics passes the correct synced_lyrics tag.""" | ||
|
|
||
| SYNCED_LRC = "[00:01.00] hello\n[00:02.00] world" | ||
|
|
||
| @pytest.fixture | ||
| def backend_name(self): | ||
| return "lrclib" | ||
|
|
||
| def test_sylt_data_passed_for_synced_lyrics( | ||
| self, monkeypatch, helper, lyrics_plugin | ||
| ): | ||
| monkeypatch.setattr( | ||
| lyrics_plugin, "find_lyrics", lambda _: Lyrics(self.SYNCED_LRC) | ||
| ) | ||
| item = helper.create_item(id=1, lyrics="") | ||
| calls = [] | ||
| monkeypatch.setattr( | ||
| type(item), "try_write", lambda self, **kw: calls.append(kw) or True | ||
| ) | ||
|
|
||
| lyrics_plugin.add_item_lyrics(item, write=True) | ||
|
|
||
| assert calls == [ | ||
| {"tags": {"synced_lyrics": [("hello", 1000), ("world", 2000)]}} | ||
| ] | ||
|
|
||
| def test_plain_text_stored_in_lyrics_for_synced( | ||
| self, monkeypatch, helper, lyrics_plugin | ||
| ): | ||
| monkeypatch.setattr( | ||
| lyrics_plugin, "find_lyrics", lambda _: Lyrics(self.SYNCED_LRC) | ||
| ) | ||
| item = helper.create_item(id=1, lyrics="") | ||
| monkeypatch.setattr(type(item), "try_write", lambda self, **kw: True) | ||
|
|
||
| lyrics_plugin.add_item_lyrics(item, write=True) | ||
|
|
||
| assert item.lyrics == "hello\nworld" | ||
|
|
||
| def test_sylt_cleared_for_plain_lyrics( | ||
| self, monkeypatch, helper, lyrics_plugin | ||
| ): | ||
| monkeypatch.setattr( | ||
| lyrics_plugin, "find_lyrics", lambda _: Lyrics("plain lyrics") | ||
| ) | ||
| item = helper.create_item(id=1, lyrics="") | ||
| calls = [] | ||
| monkeypatch.setattr( | ||
| type(item), "try_write", lambda self, **kw: calls.append(kw) or True | ||
| ) | ||
|
|
||
| lyrics_plugin.add_item_lyrics(item, write=True) | ||
|
|
||
| assert calls == [{"tags": {"synced_lyrics": None}}] |
|
Now that mediafile is released, update the requirement in Resolve whichever @ShimmerGlass's and Copilot's comments you've addressed, and I will have a final look then. |
e62466c to
c9b1bae
Compare
193b777 to
d20c916
Compare
|
@benstev1 are you planning to come back to this by any chance? |
|
Sorry about that yeah i'll come back and tackle this. My term ended for school and I forgot about it. |
|
No worries! Regarding git checkout master poetry.lock
uv tool run 'poetry<2' lock |
|
All I did was run the command you posted and I'm failing a doc check? Its not something I've seen before its asking for a 'line_length' positional arg, do you know how to fix this? |
|
Just fixed |
snejus
left a comment
There was a problem hiding this comment.
Looks good, just a pair of small comments
| for ts, text in self._split_lines: | ||
| if not ts: | ||
| continue | ||
| ts_m = re.match(r"\[(\d{2}):(\d{2})\.(\d{2})\]", ts) |
There was a problem hiding this comment.
Compile this pattern as the class attribute so that it's reused.
| minutes = int(ts_m[1]) | ||
| seconds = int(ts_m[2]) | ||
| centiseconds = int(ts_m[3]) |
There was a problem hiding this comment.
I think you can reduce this to
| minutes = int(ts_m[1]) | |
| seconds = int(ts_m[2]) | |
| centiseconds = int(ts_m[3]) | |
| m, s, cs = map(int, ts_m.groups()) |
When the lyrics plugin writes LRC-formatted (timestamped) lyrics to an ID3-tagged audio file, store the timing data in the SYLT (synchronized lyrics) frame and plain text in USLT, rather than writing raw LRC text verbatim to USLT.
Fixes #6541
Description
This PR:
Lyrics.syltproperty that converts[mm:ss.cc]LRC lines to(text, milliseconds)pairs for use with the ID3v2SYLTframe.item.try_write(), opens the file with mutagen to replaceUSLTwith clean plain text and write a properSYLTframe.SYLTframe when lyrics become unsynced.(...)
To Do