From 995ce146ed358d6af2f7222cec3c61108ecf971b Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Mon, 19 Oct 2026 14:35:05 +0200 Subject: [PATCH 1/3] fix(lyrics): read sidecar lyrics and playlists that are not utf-8 Sidecar lyrics, an imported LRC and an M3U playlist are decoded through a BOM check, then UTF-8, then an encoding guessed from the bytes, so GB18030, Big5, Shift_JIS and code-page files are read instead of skipped. --- docs/features/integrations.md | 4 +- docs/features/playlists.md | 2 +- .../flatpak/generated/cargo-sources.json | 13 +++ src-tauri/Cargo.lock | 14 +++ src-tauri/crates/app/Cargo.toml | 2 + src-tauri/crates/app/src/commands/lyrics.rs | 39 +++++-- src-tauri/crates/app/src/commands/playlist.rs | 11 +- src-tauri/crates/core/Cargo.toml | 7 ++ src-tauri/crates/core/src/lib.rs | 1 + src-tauri/crates/core/src/text_encoding.rs | 105 ++++++++++++++++++ 10 files changed, 180 insertions(+), 18 deletions(-) create mode 100644 src-tauri/crates/core/src/text_encoding.rs diff --git a/docs/features/integrations.md b/docs/features/integrations.md index 4c1831fb2..cc2d31530 100644 --- a/docs/features/integrations.md +++ b/docs/features/integrations.md @@ -140,7 +140,7 @@ Triggered from [`emit_track_changed`](../../src-tauri/crates/app/src/commands/pl 1. **Cache** — `app.lyrics` row keyed by `track.file_hash` (BLAKE3). Shared across profiles, kept for good — except a partial miss, which carries a `retry_after` date ([below](#which-providers-are-asked-for-which-tracks)). 2. **Embedded** — `LYRICS` / `USLT` / `©lyr` tag in the file (lofty), incl. synced `LRC` blocks. Lookup tries `ItemKey::UnsyncLyrics` first (the only key that maps to ID3v2's `USLT` in lofty 0.24), then `ItemKey::Lyrics` for Vorbis / MP4. For MP3s tagged with Mp3tag / foobar2000 / lame `--tg`, lyrics often live in a TXXX user-defined frame named `LYRICS` or `UNSYNCEDLYRICS` (common on K-Pop / J-Pop rips); these are invisible to the generic `Tag` interface so [`commands/lyrics.rs::read_id3v2_txxx_lyrics`](../../src-tauri/crates/app/src/commands/lyrics.rs) re-opens the file as `MpegFile`, downcasts to `Id3v2Tag`, and scans the TXXX descriptions explicitly. -3. **Sidecar file** — `{stem}.lrc` / `{stem}.txt` next to the audio file (e.g. `01 Song.mp3` + `01 Song.lrc`), or inside a sibling `Lyrics/` folder (case-insensitive, so `lyrics/` is also matched — common Linux convention). Stem matching is also case-insensitive so `Song.MP3` finds `song.lrc` on case-sensitive filesystems. `.lrc` wins over `.txt` at every probed directory because it carries timing info; same-folder hits beat `Lyrics/` hits. Format is auto-detected via `detect_format` and the row is cached with `source = lrc_file`. Whitespace-only files are treated as misses so the waterfall keeps falling through. Writing one is a choice: the destination below decides whether an edit — or a fetch — lands in the tag, in a sidecar or in the database alone. +3. **Sidecar file** — `{stem}.lrc` / `{stem}.txt` next to the audio file (e.g. `01 Song.mp3` + `01 Song.lrc`), or inside a sibling `Lyrics/` folder (case-insensitive, so `lyrics/` is also matched — common Linux convention). Stem matching is also case-insensitive so `Song.MP3` finds `song.lrc` on case-sensitive filesystems. `.lrc` wins over `.txt` at every probed directory because it carries timing info; same-folder hits beat `Lyrics/` hits. Format is auto-detected via `detect_format` and the row is cached with `source = lrc_file`. The file need not be UTF-8 (#910): [`text_encoding::decode`](../../src-tauri/crates/core/src/text_encoding.rs) honours a byte-order mark (UTF-8, UTF-16), reads valid UTF-8 as it is, and only then guesses the encoding from the bytes (`chardetng`, decoded by `encoding_rs`), so lyrics saved as GB18030, Big5, Shift_JIS, EUC-KR or a Windows code page are read too. A fixed fallback would not do: GB18030 accepts almost any byte sequence, so a Japanese or Korean file would come back as Chinese-looking nonsense. Whitespace-only files are treated as misses so the waterfall keeps falling through. Writing one is a choice: the destination below decides whether an edit — or a fetch — lands in the tag, in a sidecar or in the database alone. 4. **Generic `description` field** — last of the local tiers, and the only one that is a guess about a field meant for something else. @@ -197,7 +197,7 @@ The bulk `run_prefetch` gap-filler stays local-first regardless of the toggle: i The panel re-fetches per song — the sentinel track id stays constant for the whole session, so the effect keys on title + artist instead — and renders **statically**: synced LRC is timestamp-stripped, because the live stream position can't be aligned to a song that may have been joined mid-play. Library-row mutation actions (edit / import / refetch / clear) are hidden for radio. -`import_lrc_file` is still available for the explicit "pick this file" flow (e.g. when the sidecar lives in a non-conventional location like `~/Documents/lyrics/`); it overwrites the cached row regardless of which tier filled it. +`import_lrc_file` is still available for the explicit "pick this file" flow (e.g. when the sidecar lives in a non-conventional location like `~/Documents/lyrics/`); it decodes the file the same way as a sidecar, and overwrites the cached row regardless of which tier filled it. **In-app editor.** `save_lyrics(track_id, { content, format, write_to_file })` upserts the cache row with `source = manual` and, when `write_to_file` is true, also writes the content into the file's `USLT` (ID3v2) / `UNSYNCEDLYRICS` (Vorbis) / `©lyr` (MP4) frame via lofty. Same Windows file-lock dance as the tag editor — pause if the engine has the file open, then re-hash with blake3 and update `track.file_hash` so the cache row stays addressable after the write. Emits a typed `lyrics:updated` event the panel listens to. diff --git a/docs/features/playlists.md b/docs/features/playlists.md index af4b850c8..aaab59b79 100644 --- a/docs/features/playlists.md +++ b/docs/features/playlists.md @@ -31,7 +31,7 @@ Drag-and-drop is implemented with `@dnd-kit` over a virtualised list (`@tanstack ## M3U import / export -`export_playlist_m3u` writes a UTF-8 `.m3u8` with `#EXTINF:, - ` headers and absolute file paths, through a partial sibling of its own (created exclusively) renamed over the destination once complete ([`atomic_write`](../../src-tauri/crates/core/src/atomic_write.rs)), so overwriting an existing playlist file that fails halfway leaves the old one intact rather than truncated; a partial left by a write that never finished is removed by the next write to the same file once it is a day old. The importer accepts both `.m3u` and `.m3u8` (foobar2000, VLC, Rekordbox, hand-written sets) and matches against the active library: +`export_playlist_m3u` writes a UTF-8 `.m3u8` with `#EXTINF:<duration>,<artist> - <title>` headers and absolute file paths, through a partial sibling of its own (created exclusively) renamed over the destination once complete ([`atomic_write`](../../src-tauri/crates/core/src/atomic_write.rs)), so overwriting an existing playlist file that fails halfway leaves the old one intact rather than truncated; a partial left by a write that never finished is removed by the next write to the same file once it is a day old. The importer accepts both `.m3u` and `.m3u8` (foobar2000, VLC, Rekordbox, hand-written sets), decoded like a lyrics sidecar ([`text_encoding`](../../src-tauri/crates/core/src/text_encoding.rs)) — a legacy `.m3u` is in the code page of the machine that wrote it, Windows-1252 as often as GBK or Shift_JIS — and matches against the active library: 1. **Canonical-path match** — strip Windows `\\?\` prefix, lowercase the drive letter, normalise separators. 2. **Basename fallback** — if path-match fails, try the basename. This recovers playlists exported before a library reorganisation. diff --git a/packaging/flatpak/generated/cargo-sources.json b/packaging/flatpak/generated/cargo-sources.json index 6a8cd70d7..e5e3505a7 100644 --- a/packaging/flatpak/generated/cargo-sources.json +++ b/packaging/flatpak/generated/cargo-sources.json @@ -948,6 +948,19 @@ "dest": "cargo/vendor/chacha20-0.10.2", "dest-filename": ".cargo-checksum.json" }, + { + "type": "archive", + "archive-type": "tar-gzip", + "url": "https://static.crates.io/crates/chardetng/chardetng-1.0.0.crate", + "sha256": "13de944a44b5064ee5d3a5ceccc49a41bfec50f2580e66f82e87703acdb88b53", + "dest": "cargo/vendor/chardetng-1.0.0" + }, + { + "type": "inline", + "contents": "{\"package\": \"13de944a44b5064ee5d3a5ceccc49a41bfec50f2580e66f82e87703acdb88b53\", \"files\": {}}", + "dest": "cargo/vendor/chardetng-1.0.0", + "dest-filename": ".cargo-checksum.json" + }, { "type": "archive", "archive-type": "tar-gzip", diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index db768359c..64ccc648a 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -716,6 +716,17 @@ dependencies = [ "rand_core 0.10.1", ] +[[package]] +name = "chardetng" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "13de944a44b5064ee5d3a5ceccc49a41bfec50f2580e66f82e87703acdb88b53" +dependencies = [ + "cfg-if", + "encoding_rs", + "memchr", +] + [[package]] name = "chrono" version = "0.4.45" @@ -7750,6 +7761,7 @@ dependencies = [ "crossbeam-channel", "dirs 7.0.0", "discord-rich-presence", + "encoding_rs", "fast_image_resize", "futures", "hex", @@ -7824,7 +7836,9 @@ dependencies = [ "anyhow", "async-trait", "blake3", + "chardetng", "chrono", + "encoding_rs", "fast_image_resize", "globset", "hex", diff --git a/src-tauri/crates/app/Cargo.toml b/src-tauri/crates/app/Cargo.toml index 821553ba5..58b8bf53e 100644 --- a/src-tauri/crates/app/Cargo.toml +++ b/src-tauri/crates/app/Cargo.toml @@ -297,6 +297,8 @@ same-file = "1" # (artwork extraction, folder cover fallback, etc.). Auto-cleaned on # Drop so a panicked test doesn't leak fixtures across the temp dir. tempfile = "3" +# Writes a lyrics sidecar in a legacy encoding for the decoding test. +encoding_rs = "0.8.42" # Linux-only: direct ALSA enumeration so the output-device picker can # show human-readable descriptions ("HDA Intel PCH, ALC897 Analog diff --git a/src-tauri/crates/app/src/commands/lyrics.rs b/src-tauri/crates/app/src/commands/lyrics.rs index 937f427b2..7f3c65ed7 100644 --- a/src-tauri/crates/app/src/commands/lyrics.rs +++ b/src-tauri/crates/app/src/commands/lyrics.rs @@ -1438,8 +1438,8 @@ fn read_stem_match_in_dir(dir: &Path, stem: &str) -> Option<String> { for entry in entries.flatten() { let path = entry.path(); // Skip directories early. Without this a directory named - // `Song.lrc` would be picked into `lrc_match`, `read_to_string` - // below would fail, and a legitimate `Song.txt` in the same + // `Song.lrc` would be picked into `lrc_match`, the read below + // would fail, and a legitimate `Song.txt` in the same // directory would be silently masked. `is_file` follows // symlinks, so a symlinked sidecar still works. if !path.is_file() { @@ -1473,10 +1473,13 @@ fn read_stem_match_in_dir(dir: &Path, stem: &str) -> Option<String> { } /// Read a text file and return its trimmed contents, or `None` if -/// the file is missing, unreadable, or contains only whitespace. +/// the file is missing, unreadable, or contains only whitespace. The +/// file need not be UTF-8: lyrics saved as GB18030, Big5, Shift_JIS or +/// a Windows code page are decoded too (#910). fn read_non_empty_file(path: &Path) -> Option<String> { - let raw = std::fs::read_to_string(path).ok()?; - let trimmed = raw.trim(); + let raw = std::fs::read(path).ok()?; + let text = waveflow_core::text_encoding::decode(&raw); + let trimmed = text.trim(); if trimmed.is_empty() { None } else { @@ -2683,10 +2686,13 @@ pub async fn import_lrc_file( .ok_or_else(|| AppError::Other(format!("track {track_id} not found")))?; let path = file_path.clone(); - let content = tokio::task::spawn_blocking(move || std::fs::read_to_string(&path)) - .await - .map_err(|e| AppError::Other(format!("lyrics file read panicked: {e}")))? - .map_err(|e| AppError::Other(format!("read {file_path}: {e}")))?; + // Decoded whatever its encoding, like a sidecar (#910). + let content = tokio::task::spawn_blocking(move || { + std::fs::read(&path).map(|raw| waveflow_core::text_encoding::decode(&raw).into_owned()) + }) + .await + .map_err(|e| AppError::Other(format!("lyrics file read panicked: {e}")))? + .map_err(|e| AppError::Other(format!("read {file_path}: {e}")))?; let trimmed = content.trim(); if trimmed.is_empty() { return Err(AppError::Other("imported lyrics file is empty".into())); @@ -5425,6 +5431,21 @@ mod tests { assert_eq!(content, "fallback ok"); } + /// #910: lyrics saved by a Chinese Windows tool are GB18030, and a + /// UTF-8-only read lost them without a word. + #[test] + fn sidecar_in_gb18030_is_read() { + let dir = tempfile::tempdir().unwrap(); + let audio = dir.path().join("Song.flac"); + std::fs::write(&audio, b"").unwrap(); + let lyrics = "[00:01.00]今天的天气很好，我们一起去公园散步吧\n\ + [00:05.50]阳光照在湖面上，风吹过树林的声音"; + let (bytes, _, _) = encoding_rs::GB18030.encode(lyrics); + assert!(std::str::from_utf8(&bytes).is_err()); + std::fs::write(dir.path().join("Song.lrc"), &bytes).unwrap(); + assert_eq!(read_sidecar_lyrics(&audio).as_deref(), Some(lyrics)); + } + #[test] fn sidecar_returns_none_for_empty_file() { let dir = tempfile::tempdir().unwrap(); diff --git a/src-tauri/crates/app/src/commands/playlist.rs b/src-tauri/crates/app/src/commands/playlist.rs index fd115acfc..ef4e6ce48 100644 --- a/src-tauri/crates/app/src/commands/playlist.rs +++ b/src-tauri/crates/app/src/commands/playlist.rs @@ -864,12 +864,11 @@ pub async fn import_playlist_m3u( let src = std::path::PathBuf::from(&source_path); let raw = std::fs::read(&src).map_err(|e| AppError::Other(format!("read m3u file: {e}")))?; - // UTF-8 (.m3u8) first; fall back to byte→char lossy decode so legacy - // .m3u files in latin-1 / cp1252 still produce readable paths. - let text = match std::str::from_utf8(&raw) { - Ok(s) => s.to_string(), - Err(_) => raw.iter().map(|b| *b as char).collect::<String>(), - }; + // A `.m3u8` is UTF-8; a legacy `.m3u` is in the code page of the + // machine that wrote it — Windows-1252, but just as often GBK, Big5 + // or Shift_JIS, whose paths a byte-per-char read turned into + // nonsense that matched nothing. + let text = waveflow_core::text_encoding::decode(&raw); let parent = src.parent().unwrap_or_else(|| std::path::Path::new("")); diff --git a/src-tauri/crates/core/Cargo.toml b/src-tauri/crates/core/Cargo.toml index 12210ee0f..b440a1158 100644 --- a/src-tauri/crates/core/Cargo.toml +++ b/src-tauri/crates/core/Cargo.toml @@ -222,6 +222,13 @@ wasmtime = { version = "49", default-features = false, features = [ wasmtime-wasi = { version = "49", default-features = false, optional = true } pinyin = { version = "0.11.0", default-features = false, features = ["plain"] } unicode-normalization = { version = "0.1.25", optional = true } +# Text files the user brings (sidecar lyrics, an imported .lrc, a +# playlist) are not all UTF-8: GB18030, Big5, Shift_JIS or a Windows +# code page are common. `chardetng` guesses the encoding of what is +# not UTF-8 and `encoding_rs` decodes it (`text_encoding`). Both by +# the same author, `encoding_rs` already in the lockfile. +chardetng = "1.0.0" +encoding_rs = "0.8.42" # `ReplaceFileW`, so an atomic write keeps the replaced file's ACL the way # rewriting it in place did (`atomic_write`). Already in the lockfile diff --git a/src-tauri/crates/core/src/lib.rs b/src-tauri/crates/core/src/lib.rs index 63068062f..f0a449b70 100644 --- a/src-tauri/crates/core/src/lib.rs +++ b/src-tauri/crates/core/src/lib.rs @@ -40,3 +40,4 @@ pub mod similar; pub mod smart_playlists; pub mod sync; pub mod tagio; +pub mod text_encoding; diff --git a/src-tauri/crates/core/src/text_encoding.rs b/src-tauri/crates/core/src/text_encoding.rs new file mode 100644 index 000000000..45b954275 --- /dev/null +++ b/src-tauri/crates/core/src/text_encoding.rs @@ -0,0 +1,105 @@ +//! Decode a text file the user brought, whatever encoding it was saved in. +//! +//! Sidecar lyrics, an `.lrc` picked for import and an `.m3u` playlist come +//! from other tools, and many of those write the system's code page rather +//! than UTF-8: GB18030 on a Chinese Windows, Big5, Shift_JIS, EUC-KR, or +//! Windows-1252 for an old Western file. Read as UTF-8, such a file failed +//! outright, and the lyrics it held were simply not there (#910). +//! +//! A byte-order mark names the encoding (UTF-8, UTF-16 LE or BE) and is +//! dropped. Otherwise valid UTF-8 is UTF-8, which is what nearly every file +//! is, at no copy. Only what is neither has its encoding guessed from the +//! bytes. A fixed fallback (always GB18030, say) would not do: GB18030 +//! accepts almost any byte sequence, so a Japanese or Korean file would +//! come back as Chinese-looking nonsense instead of its text. + +use std::borrow::Cow; + +use chardetng::{EncodingDetector, Iso2022JpDetection, Utf8Detection}; + +/// The text of `bytes`, decoded as described in the module docs. Never +/// fails: a byte no encoding accounts for becomes U+FFFD. +pub fn decode(bytes: &[u8]) -> Cow<'_, str> { + if let Some((encoding, bom_length)) = encoding_rs::Encoding::for_bom(bytes) { + return encoding.decode_without_bom_handling(&bytes[bom_length..]).0; + } + if let Ok(text) = std::str::from_utf8(bytes) { + return Cow::Borrowed(text); + } + // ISO-2022-JP is 7-bit, so valid UTF-8 and already taken above. + let mut detector = EncodingDetector::new(Iso2022JpDetection::Deny); + detector.feed(bytes, true); + // Not UTF-8: that was ruled out by the check above. + let encoding = detector.guess(None, Utf8Detection::Deny); + encoding.decode_without_bom_handling(bytes).0 +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Lyrics in the shape of an `.lrc`, in the language the encoding + /// is used for. + const SIMPLIFIED: &str = "[00:01.00]今天的天气很好，我们一起去公园散步吧\n\ + [00:05.50]阳光照在湖面上，风吹过树林的声音\n\ + [00:10.20]我们一边走一边唱着这首简单的歌"; + const TRADITIONAL: &str = "[00:01.00]今天的天氣很好，我們一起去公園散步吧\n\ + [00:05.50]陽光照在湖面上，風吹過樹林的聲音\n\ + [00:10.20]我們一邊走一邊唱著這首簡單的歌"; + const JAPANESE: &str = "[00:01.00]今日はいい天気ですね、一緒に公園を散歩しましょう\n\ + [00:05.50]湖に光が映って、風が森を通り抜ける音がする\n\ + [00:10.20]歩きながらこの簡単な歌を歌っています"; + const KOREAN: &str = "[00:01.00]오늘은 날씨가 정말 좋네요, 함께 공원을 산책해요\n\ + [00:05.50]호수 위로 햇빛이 비치고 숲을 지나는 바람 소리\n\ + [00:10.20]우리는 걸으면서 이 간단한 노래를 불러요"; + const WESTERN: &str = "[00:01.00]Café crème à la fenêtre, déjà l'été\n\ + [00:05.50]Où es-tu, ma belle, sous le ciel bleu de Noël"; + + fn round_trip(text: &str, encoding: &'static encoding_rs::Encoding) { + let (bytes, _, unmappable) = encoding.encode(text); + assert!(!unmappable, "{} cannot hold the sample", encoding.name()); + assert!(std::str::from_utf8(&bytes).is_err()); + assert_eq!(decode(&bytes), text, "{}", encoding.name()); + } + + #[test] + fn utf8_is_read_as_it_is_without_a_copy() { + let decoded = decode(SIMPLIFIED.as_bytes()); + assert!(matches!(decoded, Cow::Borrowed(_))); + assert_eq!(decoded, SIMPLIFIED); + } + + #[test] + fn a_byte_order_mark_names_the_encoding_and_is_dropped() { + let mut utf8 = vec![0xEF, 0xBB, 0xBF]; + utf8.extend_from_slice(SIMPLIFIED.as_bytes()); + assert_eq!(decode(&utf8), SIMPLIFIED); + + let mut utf16le = vec![0xFF, 0xFE]; + utf16le.extend(SIMPLIFIED.encode_utf16().flat_map(u16::to_le_bytes)); + assert_eq!(decode(&utf16le), SIMPLIFIED); + + let mut utf16be = vec![0xFE, 0xFF]; + utf16be.extend(SIMPLIFIED.encode_utf16().flat_map(u16::to_be_bytes)); + assert_eq!(decode(&utf16be), SIMPLIFIED); + } + + /// The case of #910: lyrics saved by a Chinese Windows tool. + #[test] + fn gb18030_lyrics_are_read() { + round_trip(SIMPLIFIED, encoding_rs::GB18030); + } + + /// What a fixed GB18030 fallback would have turned into nonsense. + #[test] + fn other_east_asian_encodings_are_told_apart() { + round_trip(TRADITIONAL, encoding_rs::BIG5); + round_trip(JAPANESE, encoding_rs::SHIFT_JIS); + round_trip(KOREAN, encoding_rs::EUC_KR); + } + + #[test] + fn an_old_western_file_is_read() { + round_trip(WESTERN, encoding_rs::WINDOWS_1252); + } +} From a1fbd5ff28762e0ab5fdc06053ac15ab6ed34dd8 Mon Sep 17 00:00:00 2001 From: InstaZDLL <github.105mh@8shield.net> Date: Tue, 20 Oct 2026 00:24:53 +0200 Subject: [PATCH 2/3] fix(lyrics): let a cached miss give way to a sidecar readable since A complete miss is cached for good under the audio's hash, so a sidecar that was unreadable before the decoding fix, or added after the miss, stayed hidden. A cached miss is now checked against the sidecar before it is served. --- docs/features/integrations.md | 2 +- src-tauri/crates/app/src/commands/lyrics.rs | 124 ++++++++++++++++---- 2 files changed, 100 insertions(+), 26 deletions(-) diff --git a/docs/features/integrations.md b/docs/features/integrations.md index cc2d31530..b6f7bf454 100644 --- a/docs/features/integrations.md +++ b/docs/features/integrations.md @@ -140,7 +140,7 @@ Triggered from [`emit_track_changed`](../../src-tauri/crates/app/src/commands/pl 1. **Cache** — `app.lyrics` row keyed by `track.file_hash` (BLAKE3). Shared across profiles, kept for good — except a partial miss, which carries a `retry_after` date ([below](#which-providers-are-asked-for-which-tracks)). 2. **Embedded** — `LYRICS` / `USLT` / `©lyr` tag in the file (lofty), incl. synced `LRC` blocks. Lookup tries `ItemKey::UnsyncLyrics` first (the only key that maps to ID3v2's `USLT` in lofty 0.24), then `ItemKey::Lyrics` for Vorbis / MP4. For MP3s tagged with Mp3tag / foobar2000 / lame `--tg`, lyrics often live in a TXXX user-defined frame named `LYRICS` or `UNSYNCEDLYRICS` (common on K-Pop / J-Pop rips); these are invisible to the generic `Tag` interface so [`commands/lyrics.rs::read_id3v2_txxx_lyrics`](../../src-tauri/crates/app/src/commands/lyrics.rs) re-opens the file as `MpegFile`, downcasts to `Id3v2Tag`, and scans the TXXX descriptions explicitly. -3. **Sidecar file** — `{stem}.lrc` / `{stem}.txt` next to the audio file (e.g. `01 Song.mp3` + `01 Song.lrc`), or inside a sibling `Lyrics/` folder (case-insensitive, so `lyrics/` is also matched — common Linux convention). Stem matching is also case-insensitive so `Song.MP3` finds `song.lrc` on case-sensitive filesystems. `.lrc` wins over `.txt` at every probed directory because it carries timing info; same-folder hits beat `Lyrics/` hits. Format is auto-detected via `detect_format` and the row is cached with `source = lrc_file`. The file need not be UTF-8 (#910): [`text_encoding::decode`](../../src-tauri/crates/core/src/text_encoding.rs) honours a byte-order mark (UTF-8, UTF-16), reads valid UTF-8 as it is, and only then guesses the encoding from the bytes (`chardetng`, decoded by `encoding_rs`), so lyrics saved as GB18030, Big5, Shift_JIS, EUC-KR or a Windows code page are read too. A fixed fallback would not do: GB18030 accepts almost any byte sequence, so a Japanese or Korean file would come back as Chinese-looking nonsense. Whitespace-only files are treated as misses so the waterfall keeps falling through. Writing one is a choice: the destination below decides whether an edit — or a fetch — lands in the tag, in a sidecar or in the database alone. +3. **Sidecar file** — `{stem}.lrc` / `{stem}.txt` next to the audio file (e.g. `01 Song.mp3` + `01 Song.lrc`), or inside a sibling `Lyrics/` folder (case-insensitive, so `lyrics/` is also matched — common Linux convention). Stem matching is also case-insensitive so `Song.MP3` finds `song.lrc` on case-sensitive filesystems. `.lrc` wins over `.txt` at every probed directory because it carries timing info; same-folder hits beat `Lyrics/` hits. Format is auto-detected via `detect_format` and the row is cached with `source = lrc_file`. The file need not be UTF-8 (#910): [`text_encoding::decode`](../../src-tauri/crates/core/src/text_encoding.rs) honours a byte-order mark (UTF-8, UTF-16), reads valid UTF-8 as it is, and only then guesses the encoding from the bytes (`chardetng`, decoded by `encoding_rs`), so lyrics saved as GB18030, Big5, Shift_JIS, EUC-KR or a Windows code page are read too. A fixed fallback would not do: GB18030 accepts almost any byte sequence, so a Japanese or Korean file would come back as Chinese-looking nonsense. A cached miss is checked against the sidecar before it is served (`read_cached_or_new_sidecar`): the cache is keyed by the audio's hash and a complete miss is kept for good, so a sidecar added after the miss — or one that was unreadable before #910 — would otherwise stay hidden behind it forever. The sidecar is the one local source that can change without that hash changing, and it outranks a miss in every order the waterfall runs; the check costs one directory listing, and only for tracks without lyrics. Whitespace-only files are treated as misses so the waterfall keeps falling through. Writing one is a choice: the destination below decides whether an edit — or a fetch — lands in the tag, in a sidecar or in the database alone. 4. **Generic `description` field** — last of the local tiers, and the only one that is a guess about a field meant for something else. diff --git a/src-tauri/crates/app/src/commands/lyrics.rs b/src-tauri/crates/app/src/commands/lyrics.rs index 7f3c65ed7..8a91c0d26 100644 --- a/src-tauri/crates/app/src/commands/lyrics.rs +++ b/src-tauri/crates/app/src/commands/lyrics.rs @@ -1041,6 +1041,66 @@ async fn read_prefer_lrclib(pool: &sqlx::SqlitePool) -> AppResult<bool> { /// inside the embedded tier ahead of the sidecar — so a `yt-dlp` rip /// showed its YouTube blurb and never read the `.lrc` sitting beside /// it. +/// The sidecar tier on its own: a `.lrc` / `.txt` beside the track or in +/// its `Lyrics/` folder, cached when found. Cheap (a couple of stat calls +/// and at most two `read_dir` scans). +async fn try_sidecar_lyrics( + pool: &sqlx::SqlitePool, + track_id: i64, + meta: &TrackMeta, +) -> AppResult<Option<LyricsPayload>> { + let path_for_sidecar = meta.file_path.clone(); + let sidecar = + tokio::task::spawn_blocking(move || read_sidecar_lyrics(Path::new(&path_for_sidecar))) + .await + .ok() + .flatten(); + let Some(content) = sidecar else { + return Ok(None); + }; + let format = detect_format(&content); + let source = LyricsSource::LrcFile; + upsert_lyrics(pool, &meta.file_hash, &content, &format, &source, None).await?; + Ok(Some(LyricsPayload { + track_id, + content, + format, + source, + provider: None, + tag_write_skipped: None, + sidecar_write_skipped: None, + associated: Vec::new(), + })) +} + +/// [`read_cached`], except that a cached miss is first checked against +/// the track's sidecar. +/// +/// The cache is keyed by the audio's hash, and a complete miss is kept +/// for good, so a sidecar the reader could not see when the miss was +/// written would stay hidden behind it forever: one added since, or one +/// in an encoding read as UTF-8 only before #910. Fixing the reader alone +/// would only have helped tracks nobody had opened yet. The sidecar is the +/// one local source that can change without the hash changing (a tag edit +/// changes it, and with it the cache key), and it outranks a miss in +/// every order the waterfall runs, an instrumental verdict included. So a +/// miss costs one directory listing per lookup, and only for tracks +/// without lyrics. +async fn read_cached_or_new_sidecar( + pool: &sqlx::SqlitePool, + track_id: i64, +) -> AppResult<Option<LyricsPayload>> { + let cached = read_cached(pool, track_id).await?; + if cached.as_ref().is_some_and(|row| row.content.is_empty()) { + if let Some(meta) = read_track_meta(pool, track_id).await? { + if let Some(payload) = try_sidecar_lyrics(pool, track_id, &meta).await? { + return Ok(Some(payload)); + } + } + } + Ok(cached) +} + async fn try_local_lyrics( pool: &sqlx::SqlitePool, track_id: i64, @@ -1069,28 +1129,8 @@ async fn try_local_lyrics( })); } - // Local sidecar `.lrc` / `.txt`. Cheap (a couple of stat calls + at - // most two `read_dir` scans). - let path_for_sidecar = meta.file_path.clone(); - let sidecar = - tokio::task::spawn_blocking(move || read_sidecar_lyrics(Path::new(&path_for_sidecar))) - .await - .ok() - .flatten(); - if let Some(content) = sidecar { - let format = detect_format(&content); - let source = LyricsSource::LrcFile; - upsert_lyrics(pool, &meta.file_hash, &content, &format, &source, None).await?; - return Ok(Some(LyricsPayload { - track_id, - content, - format, - source, - provider: None, - tag_write_skipped: None, - sidecar_write_skipped: None, - associated: Vec::new(), - })); + if let Some(payload) = try_sidecar_lyrics(pool, track_id, meta).await? { + return Ok(Some(payload)); } // Last: the generic description field, which is a guess about a @@ -1821,7 +1861,7 @@ pub async fn get_lyrics( track_id: i64, ) -> AppResult<Option<LyricsPayload>> { let pool = state.require_profile_pool().await?; - read_cached(&pool, track_id).await + read_cached_or_new_sidecar(&pool, track_id).await } /// Multi-tier lookup: cache → embedded tag → sidecar → enhanced/API @@ -2268,8 +2308,8 @@ async fn fetch_lyrics_honouring( ) -> AppResult<Option<LyricsPayload>> { let pool = state.require_profile_pool().await?; - // 1. Cache. - if let Some(cached) = read_cached(&pool, track_id).await? { + // 1. Cache. A cached miss is checked against the sidecar first. + if let Some(cached) = read_cached_or_new_sidecar(&pool, track_id).await? { return Ok(Some(cached)); } @@ -4771,6 +4811,40 @@ mod tests { assert!(read_cached(&pool, 1).await.unwrap().is_some()); } + /// #910: a GB18030 sidecar was unreadable, so the track got a miss + /// cached for good. Once the sidecar can be read, the miss must give + /// way to it, or the fix never reaches a track already opened. + #[tokio::test] + async fn a_cached_miss_gives_way_to_a_sidecar_readable_since() { + let dir = tempfile::tempdir().unwrap(); + let pool = pool_with_app_schema(dir.path()).await; + let audio = dir.path().join("Song.flac"); + std::fs::write(&audio, b"audio").unwrap(); + seed_track(&pool, &audio, "h1").await; + cache_lyrics_miss(&pool, 1, &meta_for("h1"), false) + .await + .unwrap(); + + let still_a_miss = read_cached_or_new_sidecar(&pool, 1) + .await + .unwrap() + .expect("the miss is served while there is no sidecar"); + assert!(still_a_miss.content.is_empty()); + + let lyrics = "[00:01.00]今天的天气很好，我们一起去公园散步吧"; + let (bytes, _, _) = encoding_rs::GB18030.encode(lyrics); + std::fs::write(dir.path().join("Song.lrc"), &bytes).unwrap(); + + let found = read_cached_or_new_sidecar(&pool, 1) + .await + .unwrap() + .expect("the sidecar is found"); + assert_eq!(found.content, lyrics); + let cached = read_cached(&pool, 1).await.unwrap().expect("now cached"); + assert_eq!(cached.content, lyrics); + assert!(matches!(cached.source, LyricsSource::LrcFile)); + } + /// Lyrics found after a partial miss replace it for good. Were the /// expiry carried over, they would be thrown away a week later. #[tokio::test] From 6b7429b907606d4cbf0e1cda046a32c2f7d19191 Mon Sep 17 00:00:00 2001 From: InstaZDLL <github.105mh@8shield.net> Date: Tue, 20 Oct 2026 00:39:37 +0200 Subject: [PATCH 3/3] fix(lyrics): replace a cached miss with the sidecar only while it is a miss Lyrics a lookup or an edit wrote between the read of the miss and the write of the sidecar are kept and served instead. Also gives try_local_lyrics its doc comment back. --- docs/features/integrations.md | 2 +- src-tauri/crates/app/src/commands/lyrics.rs | 174 +++++++++++++++----- 2 files changed, 131 insertions(+), 45 deletions(-) diff --git a/docs/features/integrations.md b/docs/features/integrations.md index b6f7bf454..7349a8239 100644 --- a/docs/features/integrations.md +++ b/docs/features/integrations.md @@ -140,7 +140,7 @@ Triggered from [`emit_track_changed`](../../src-tauri/crates/app/src/commands/pl 1. **Cache** — `app.lyrics` row keyed by `track.file_hash` (BLAKE3). Shared across profiles, kept for good — except a partial miss, which carries a `retry_after` date ([below](#which-providers-are-asked-for-which-tracks)). 2. **Embedded** — `LYRICS` / `USLT` / `©lyr` tag in the file (lofty), incl. synced `LRC` blocks. Lookup tries `ItemKey::UnsyncLyrics` first (the only key that maps to ID3v2's `USLT` in lofty 0.24), then `ItemKey::Lyrics` for Vorbis / MP4. For MP3s tagged with Mp3tag / foobar2000 / lame `--tg`, lyrics often live in a TXXX user-defined frame named `LYRICS` or `UNSYNCEDLYRICS` (common on K-Pop / J-Pop rips); these are invisible to the generic `Tag` interface so [`commands/lyrics.rs::read_id3v2_txxx_lyrics`](../../src-tauri/crates/app/src/commands/lyrics.rs) re-opens the file as `MpegFile`, downcasts to `Id3v2Tag`, and scans the TXXX descriptions explicitly. -3. **Sidecar file** — `{stem}.lrc` / `{stem}.txt` next to the audio file (e.g. `01 Song.mp3` + `01 Song.lrc`), or inside a sibling `Lyrics/` folder (case-insensitive, so `lyrics/` is also matched — common Linux convention). Stem matching is also case-insensitive so `Song.MP3` finds `song.lrc` on case-sensitive filesystems. `.lrc` wins over `.txt` at every probed directory because it carries timing info; same-folder hits beat `Lyrics/` hits. Format is auto-detected via `detect_format` and the row is cached with `source = lrc_file`. The file need not be UTF-8 (#910): [`text_encoding::decode`](../../src-tauri/crates/core/src/text_encoding.rs) honours a byte-order mark (UTF-8, UTF-16), reads valid UTF-8 as it is, and only then guesses the encoding from the bytes (`chardetng`, decoded by `encoding_rs`), so lyrics saved as GB18030, Big5, Shift_JIS, EUC-KR or a Windows code page are read too. A fixed fallback would not do: GB18030 accepts almost any byte sequence, so a Japanese or Korean file would come back as Chinese-looking nonsense. A cached miss is checked against the sidecar before it is served (`read_cached_or_new_sidecar`): the cache is keyed by the audio's hash and a complete miss is kept for good, so a sidecar added after the miss — or one that was unreadable before #910 — would otherwise stay hidden behind it forever. The sidecar is the one local source that can change without that hash changing, and it outranks a miss in every order the waterfall runs; the check costs one directory listing, and only for tracks without lyrics. Whitespace-only files are treated as misses so the waterfall keeps falling through. Writing one is a choice: the destination below decides whether an edit — or a fetch — lands in the tag, in a sidecar or in the database alone. +3. **Sidecar file** — `{stem}.lrc` / `{stem}.txt` next to the audio file (e.g. `01 Song.mp3` + `01 Song.lrc`), or inside a sibling `Lyrics/` folder (case-insensitive, so `lyrics/` is also matched — common Linux convention). Stem matching is also case-insensitive so `Song.MP3` finds `song.lrc` on case-sensitive filesystems. `.lrc` wins over `.txt` at every probed directory because it carries timing info; same-folder hits beat `Lyrics/` hits. Format is auto-detected via `detect_format` and the row is cached with `source = lrc_file`. The file need not be UTF-8 (#910): [`text_encoding::decode`](../../src-tauri/crates/core/src/text_encoding.rs) honours a byte-order mark (UTF-8, UTF-16), reads valid UTF-8 as it is, and only then guesses the encoding from the bytes (`chardetng`, decoded by `encoding_rs`), so lyrics saved as GB18030, Big5, Shift_JIS, EUC-KR or a Windows code page are read too. A fixed fallback would not do: GB18030 accepts almost any byte sequence, so a Japanese or Korean file would come back as Chinese-looking nonsense. A cached miss is checked against the sidecar before it is served (`read_cached_or_new_sidecar`): the cache is keyed by the audio's hash and a complete miss is kept for good, so a sidecar added after the miss — or one that was unreadable before #910 — would otherwise stay hidden behind it forever. The sidecar is the one local source that can change without that hash changing, and it outranks a miss in every order the waterfall runs; the check costs one directory listing, and only for tracks without lyrics. The sidecar is written over the miss only while the row still holds it, so lyrics a lookup or an edit stored in between are served instead. Whitespace-only files are treated as misses so the waterfall keeps falling through. Writing one is a choice: the destination below decides whether an edit — or a fetch — lands in the tag, in a sidecar or in the database alone. 4. **Generic `description` field** — last of the local tiers, and the only one that is a guess about a field meant for something else. diff --git a/src-tauri/crates/app/src/commands/lyrics.rs b/src-tauri/crates/app/src/commands/lyrics.rs index 8a91c0d26..637383874 100644 --- a/src-tauri/crates/app/src/commands/lyrics.rs +++ b/src-tauri/crates/app/src/commands/lyrics.rs @@ -1028,49 +1028,87 @@ async fn read_prefer_lrclib(pool: &sqlx::SqlitePool) -> AppResult<bool> { Ok(value.map(|v| v == "true" || v == "1").unwrap_or(false)) } -/// Local tiers, in order: embedded lyrics tag → sidecar `.lrc`/`.txt` -/// → the generic description field. Returns the first hit, already -/// cached, or `None` when none is present. Shared by the default -/// (local-first) order and the `lyrics.prefer_lrclib` -/// (local-as-fallback) order so the two never drift on how a local hit -/// is read + persisted. -/// -/// The description comes last, and that ordering is the fix for a -/// reported bug rather than a detail: it is the one tier that is a -/// guess about a field meant for something else, and it used to sit -/// inside the embedded tier ahead of the sidecar — so a `yt-dlp` rip -/// showed its YouTube blurb and never read the `.lrc` sitting beside -/// it. -/// The sidecar tier on its own: a `.lrc` / `.txt` beside the track or in -/// its `Lyrics/` folder, cached when found. Cheap (a couple of stat calls -/// and at most two `read_dir` scans). +/// The sidecar beside the track or in its `Lyrics/` folder, read off the +/// async runtime. Cheap (a couple of stat calls and at most two +/// `read_dir` scans). +async fn read_sidecar_of(meta: &TrackMeta) -> Option<String> { + let path = meta.file_path.clone(); + tokio::task::spawn_blocking(move || read_sidecar_lyrics(Path::new(&path))) + .await + .ok() + .flatten() +} + +fn sidecar_payload(track_id: i64, content: String) -> LyricsPayload { + LyricsPayload { + track_id, + format: detect_format(&content), + content, + source: LyricsSource::LrcFile, + provider: None, + tag_write_skipped: None, + sidecar_write_skipped: None, + associated: Vec::new(), + } +} + +/// The sidecar tier on its own, cached when found. async fn try_sidecar_lyrics( pool: &sqlx::SqlitePool, track_id: i64, meta: &TrackMeta, ) -> AppResult<Option<LyricsPayload>> { - let path_for_sidecar = meta.file_path.clone(); - let sidecar = - tokio::task::spawn_blocking(move || read_sidecar_lyrics(Path::new(&path_for_sidecar))) - .await - .ok() - .flatten(); - let Some(content) = sidecar else { + let Some(content) = read_sidecar_of(meta).await else { return Ok(None); }; - let format = detect_format(&content); - let source = LyricsSource::LrcFile; - upsert_lyrics(pool, &meta.file_hash, &content, &format, &source, None).await?; - Ok(Some(LyricsPayload { - track_id, - content, - format, - source, - provider: None, - tag_write_skipped: None, - sidecar_write_skipped: None, - associated: Vec::new(), - })) + let payload = sidecar_payload(track_id, content); + upsert_lyrics( + pool, + &meta.file_hash, + &payload.content, + &payload.format, + &payload.source, + None, + ) + .await?; + Ok(Some(payload)) +} + +/// Write a sidecar's lyrics over a cached miss, and only over a miss: the +/// row is checked again by the statement that writes it, so lyrics that a +/// lookup or an edit stored since the miss was read are left as they are. +/// `false` when the row no longer holds a miss. +async fn replace_miss( + pool: &sqlx::SqlitePool, + file_hash: &str, + payload: &LyricsPayload, +) -> AppResult<bool> { + let mut tx = pool.begin().await?; + let replaced = sqlx::query( + "UPDATE app.lyrics + SET content = ?, format = ?, source = ?, provider = NULL, language = NULL, + fetched_at = ?, retry_after = NULL + WHERE file_hash = ? AND content = ''", + ) + .bind(&payload.content) + .bind(format_to_db(&payload.format)) + .bind(source_to_db(&payload.source)) + .bind(now_ms()) + .bind(file_hash) + .execute(&mut *tx) + .await? + .rows_affected() + > 0; + if replaced { + // A miss carries no bundle, but a new primary ends any bundle, + // as in `upsert_lyrics_until`. + sqlx::query("DELETE FROM app.lyrics_associated WHERE file_hash = ?") + .bind(file_hash) + .execute(&mut *tx) + .await?; + } + tx.commit().await?; + Ok(replaced) } /// [`read_cached`], except that a cached miss is first checked against @@ -1085,22 +1123,43 @@ async fn try_sidecar_lyrics( /// changes it, and with it the cache key), and it outranks a miss in /// every order the waterfall runs, an instrumental verdict included. So a /// miss costs one directory listing per lookup, and only for tracks -/// without lyrics. +/// without lyrics. The sidecar replaces the miss only if the row still +/// holds it ([`replace_miss`]); anything written in between is served +/// instead. async fn read_cached_or_new_sidecar( pool: &sqlx::SqlitePool, track_id: i64, ) -> AppResult<Option<LyricsPayload>> { let cached = read_cached(pool, track_id).await?; - if cached.as_ref().is_some_and(|row| row.content.is_empty()) { - if let Some(meta) = read_track_meta(pool, track_id).await? { - if let Some(payload) = try_sidecar_lyrics(pool, track_id, &meta).await? { - return Ok(Some(payload)); - } - } + if !cached.as_ref().is_some_and(|row| row.content.is_empty()) { + return Ok(cached); } - Ok(cached) + let Some(meta) = read_track_meta(pool, track_id).await? else { + return Ok(cached); + }; + let Some(content) = read_sidecar_of(&meta).await else { + return Ok(cached); + }; + let payload = sidecar_payload(track_id, content); + if replace_miss(pool, &meta.file_hash, &payload).await? { + return Ok(Some(payload)); + } + read_cached(pool, track_id).await } +/// Local tiers, in order: embedded lyrics tag → sidecar `.lrc`/`.txt` +/// → the generic description field. Returns the first hit, already +/// cached, or `None` when none is present. Shared by the default +/// (local-first) order and the `lyrics.prefer_lrclib` +/// (local-as-fallback) order so the two never drift on how a local hit +/// is read + persisted. +/// +/// The description comes last, and that ordering is the fix for a +/// reported bug rather than a detail: it is the one tier that is a +/// guess about a field meant for something else, and it used to sit +/// inside the embedded tier ahead of the sidecar — so a `yt-dlp` rip +/// showed its YouTube blurb and never read the `.lrc` sitting beside +/// it. async fn try_local_lyrics( pool: &sqlx::SqlitePool, track_id: i64, @@ -4845,6 +4904,33 @@ mod tests { assert!(matches!(cached.source, LyricsSource::LrcFile)); } + /// The sidecar replaces a miss, never what was written over the miss + /// while the sidecar was being read. + #[tokio::test] + async fn a_sidecar_does_not_replace_lyrics_written_since_the_miss() { + let dir = tempfile::tempdir().unwrap(); + let pool = pool_with_app_schema(dir.path()).await; + seed_track(&pool, &dir.path().join("Song.flac"), "h1").await; + cache_lyrics_miss(&pool, 1, &meta_for("h1"), false) + .await + .unwrap(); + upsert_lyrics( + &pool, + "h1", + "[00:01.00]Found", + &LyricsFormat::Lrc, + &LyricsSource::Api, + Some("lrclib"), + ) + .await + .unwrap(); + + let sidecar = sidecar_payload(1, "[00:01.00]Sidecar".into()); + assert!(!replace_miss(&pool, "h1", &sidecar).await.unwrap()); + let cached = read_cached(&pool, 1).await.unwrap().expect("cached"); + assert_eq!(cached.content, "[00:01.00]Found"); + } + /// Lyrics found after a partial miss replace it for good. Were the /// expiry carried over, they would be thrown away a week later. #[tokio::test]