1 commit

Author	SHA1	Message	Date
Deluan Quintão	aa5aa731dc	refactor(lyrics): single ParseLyrics entry point + all-format plugin lyrics (#5632) ... * refactor(lyrics): read sidecar files via library storage FS Routes fromExternalFile reads through storage.For(mf.LibraryPath) instead of os.Open on AbsolutePath, fixing sidecar reads for non-local backends. UTF-16 LE/BE and BOM handling preserved via ioutils.UTF8Reader. * refactor(lyrics): address review feedback on sidecar FS read - Move blank local-storage import from sources.go into lyrics_suite_test.go (the test suite already imports the local package for RegisterExtractor, so local's init() runs; production binaries get the scheme via normal wiring) - Fix misleading comment: model.ParseLyrics → model.ParseLyricsFile - Replace what-comment with why-comment in BeforeSuite explaining the log.Fatal guard that requires the no-op extractor registration * test(lyrics): add subsonic e2e baseline for getLyrics endpoints Establishes a behavioral baseline for getLyricsBySongId (v2 structured) and getLyrics (legacy) before the lyrics parser refactor. Covers embedded formats (LRC synced, plain text, TTML) and sidecar formats (LRC, SRT, YAML), all isolated under a Lyrics/ fixture folder so the new fixtures do not perturb existing test behavior beyond fixture counts. Sidecar files are injected as raw &fstest.MapFile{Data: []byte(...)} entries; the scanner skips non-audio extensions (.lrc, .srt, .yaml) so they are invisible to scanning but reachable via the fake FS at request time through fromExternalFile/storage.For. Update album/artist/song counts in the album-list, multi-library, and search3 empty-query tests to reflect the six new tracks (1 new artist, 1 new album, 6 new songs). * test(lyrics): strengthen e2e lyrics baseline (lang assertions, rename helper) Rename the local helper `main` to `firstLyric` to avoid collision with the reserved-feeling built-in name. Add `Lang` assertions to both embedded and sidecar DescribeTable entries, locking the current observed values: "xxx" (ISO 639-2 "no language specified") for all embedded and LRC/SRT sidecars, and "eng" for the YAML sidecar (which explicitly sets `language: eng`). * feat(lyrics): detect Lyricsfile YAML in content-sniffing * feat(plugins): content-sniff plugin lyrics for all formats Replace model.ToLyrics (LRC/plain only) with model.ParseEmbedded so plugin responses are content-sniffed for TTML, SRT, YAML, LRC, and plain text. ParseEmbedded returns a LyricList, so the loop now flattens multiple tracks per response entry. The test-lyrics WASM plugin gains a "ttml" format mode (configured via pdk.GetConfig) that returns a minimal TTML document; rebuilt with the standard Go wasip1 toolchain (GOOS=wasip1 GOARCH=wasm). A new Ginkgo test asserts Synced==true and the exact cue value, which the old plain-text path could not produce. GetLyrics doc comment updated to reflect content-sniffing; a later task will retarget it to ParseLyrics once that function is introduced. * test(plugins): validate plugin lyrics auto-detect across all formats The test-lyrics WASM plugin now supports per-format modes via the "format" config key: ttml, srt, yaml, lrc, and plain, in addition to the existing default plain-text response. The plugin is rebuilt with the standard Go wasip1 compiler. lyrics_adapter_test.go gains a DescribeTable covering all five formats, asserting both Synced (the discriminator that proves correct format detection) and the exact line value. This validates the full auto-detect chain (TTML → SRT → YAML/Lyricsfile → LRC → plain) end-to-end through the real plugin → adapter → parser flow. * refactor(lyrics): consolidate parsers into model.ParseLyrics * refactor(lyrics): retarget legacy callers to model.ParseLyrics Pin suffix to ".lrc" to preserve byte-identical output for stored plain/LRC text that was previously handled by the now-removed ToLyrics. * test(lyrics): fix lyrics tests after parser consolidation - Rewrite the YAML-fallback test to assert the correct design: a non-Lyricsfile .yaml sidecar returns as plain text and shadows lower-priority sources (rather than falling through to .lrc). - Add LibraryPath + relative Path split to the three subsonic tests that read sidecar files via storage.For(), so they resolve against the correct fixtures directory. - Register a no-op extractor in api_suite_test.go BeforeSuite so newLocalStorage does not fatal when storage.For is called during sidecar-lyrics tests. * test(lyrics): add per-format ParseLyrics benchmarks Baseline measurements (count=2 runs) on M2: BenchmarkParseLyrics_LRC-8 5725 178796 ns/op 49.78 MB/s 427877 B/op 523 allocs/op BenchmarkParseLyrics_Plain-8 5425 230854 ns/op 32.44 MB/s 102508 B/op 16 allocs/op BenchmarkParseLyrics_EnhancedLRC-8 1942 605893 ns/op 17.66 MB/s 860678 B/op 4256 allocs/op BenchmarkParseLyrics_SRT-8 3249 373991 ns/op 25.91 MB/s 1113575 B/op 4407 allocs/op BenchmarkParseLyrics_TTML-8 1483 813027 ns/op 13.86 MB/s 2198052 B/op 8665 allocs/op BenchmarkParseLyrics_YAML-8 1700 678250 ns/op 13.01 MB/s 1235096 B/op 8288 allocs/op BenchmarkParseLyrics_SniffTTML-8 1525 776482 ns/op 14.51 MB/s 2225448 B/op 8681 allocs/op BenchmarkParseLyrics_SniffSRT-8 2528 451210 ns/op 21.48 MB/s 1157000 B/op 4422 allocs/op BenchmarkParseLyrics_SniffYAML-8 1333 827152 ns/op 10.67 MB/s 1337195 B/op 8718 allocs/op BenchmarkParseLyrics_SniffLRC-8 2820 413038 ns/op 21.55 MB/s 588934 B/op 1812 allocs/op BenchmarkParseLyrics_SniffPlain-8 2968 409091 ns/op 18.31 MB/s 254470 B/op 1491 allocs/op Content-sniff path overhead: 1.5–15% depending on format. * test(lyrics): use real public-domain fixtures for parser benchmarks Replace synthetic benchmark payloads with 'Auld Lang Syne' (Robert Burns, 1788, public domain) rendered into every supported format (LRC, plain, enhanced LRC, SRT, TTML, Lyricsfile YAML) so the numbers reflect realistic content. Same song across formats makes per-format cost comparable. Baseline (Apple M-series, -benchmem, real fixtures): LRC ~28 us/op 42 KB 147 allocs Plain ~23 us/op 18 KB 22 allocs EnhancedLRC ~37 us/op 51 KB 374 allocs SRT ~52 us/op 139 KB 581 allocs TTML ~119 us/op 276 KB 1227 allocs YAML ~142 us/op 193 KB 1732 allocs Sniff(LRC) ~47 us/op 57 KB 237 allocs Sniff(TTML) ~122 us/op 282 KB 1250 allocs Sniff(YAML) ~186 us/op 218 KB 1847 allocs Fixtures in tests/fixtures/lyrics/. * fix(lyrics): preserve [] (not null) for empty lyrics in backfill migration ParseLyrics returns nil for zero-line input (whitespace-only stored lyrics). json.Marshal(nil LyricList) produces null, violating the DB invariant that media_file.lyrics uses [] for empty lyrics, never null. Initialize to model.LyricList{} when ParseLyrics returns nil so the marshalled result is always []. * refactor(lyrics): unify parser dispatch and centralize empty-list invariant Apply thermo-nuclear review findings (behavior-preserving): - Replace the suffix switch + three single-use closure adapters (parseTTMLKnown/parseSRTKnown + inline YAML closure) with a bySuffix map of a single lyricParser(lang, contents) signature. Normalize parseTTMLWithDefaultLang/parseSRTWithLanguage to that (lang, contents) order so no adapter glue is needed. - Collapse the parallel sniffLyrics engine into one parseFirstMatch primitive shared by both the suffix and content-sniff paths (sniffOrder candidate list). TTML stays gated via parseTTMLIfDocument in sniff mode to avoid running the XML decoder on plain/LRC text. - Add LyricList.MarshalJSON so empty/nil always serializes to [] (the lyrics column invariant), in one canonical place. Delete the migration's nil-guard, which the marshaler now subsumes. Behavior verified unchanged: full suite + race + e2e green. * refactor(lyrics): single registry drives both suffix dispatch and sniff order Collapse the bySuffix map and sniffOrder slice into one ordered registry: slice order is the content-sniff probe order, each row's suffixes drive sidecar dispatch, and per-row bySuffix/byContent parsers preserve the gated-TTML-when-sniffing distinction. One source of truth, no duplicated parser references. * refactor(lyrics): self-skipping parsers collapse the format table to one column Move the TTML <tt>-document gate into parseTTMLWithDefaultLang itself (after the encoding fixup, so UTF-16-declared docs are still recognized): non-TTML content returns (nil, nil) to skip; a malformed <tt> document still errors. SRT and Lyricsfile YAML already self-skip. With every structured parser self-skipping, the format table drops to one {suffixes, parse} column named lyricFormats — no bySuffix/byContent split, no separate sniff-only TTML gate. Both the suffix and content-sniff paths share the same parser per format. * refactor(lyrics): strip BOM once at ParseLyrics entry for all paths Previously only the content-sniff path stripped the BOM; the suffix path relied on its callers (fromExternalFile via UTF8Reader) having already stripped it. That implicit contract was fragile — a caller passing raw BOM-prefixed bytes with a suffix would reach the parsers with the BOM intact (SanitizeText does not strip it). Strip once at entry so every path and parser sees clean bytes regardless of caller. No-op for already-stripped input. * refactor(lyrics): trim verbose comments to essential why * refactor(lyrics): move LRC parser to its own lyrics_lrc.go Extract parseLRC, the enhanced-LRC helpers (parseEnhancedLine, adjustGroup, stripEnhancedMarkers, shiftELRCCues), parseTime, and the LRC regexes from lyrics.go into lyrics_lrc.go, with the parseLRC tests in lyrics_lrc_test.go. This makes the layout symmetric — one file per format (lrc/srt/ttml/yaml) — and leaves lyrics.go holding only shared types and cue normalization. All moved symbols were already LRC-private; no behavior change. * refactor(lyrics): collapse ParseLyrics suffix/sniff branches into one loop Both modes differ only in which formats to try, so select candidates in a single loop (all formats when sniffing, the suffix's own otherwise) and run them through parseFirstMatch once. Drops the projected-slice make+index and the ContainsFunc closure; unmatched suffixes yield no candidates and fall to the plain-text floor, as before. * refactor(lyrics): apply simplify-review cleanups - stripBOM: bytes.TrimPrefix instead of []byte<->string round-trip (no alloc) - ParseLyrics: pre-size the candidates slice - move isTTMLDocument to lyrics_ttml.go beside its only caller (the dispatch layer should hold no per-format knowledge) * refactor(lyrics): simplify test descriptions for structured lyrics Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): fold parseLyricsfile into lyricParser signature and rename file - parseLyricsfile now matches the lyricParser signature directly (reads via bytes.NewReader), removing the parseLyricsfileBytes adapter and the string(contents) copy; the lyricFormats table references it directly. - StructuredLyrics drops the vestigial LyricList{} init (json.Unmarshal overwrites; MarshalJSON owns the empty->[] invariant). - Rename lyricsfile.go -> lyrics_lyricsfile.go (and its test) to match the lyrics_<format>.go convention used by lrc/srt/ttml. * refactor(lyrics): move test-only parseTTML/parseSRT wrappers to test files These zero-arg wrappers (defaulting lang to "xxx") had no production callers after the consolidation — only the format tests used them. Move each beside its tests so the production files carry no test-only code. * build: exclude generated *_gen.go files from linting The plugin host *_gen.go files (ndpgen output) were tripping the whitespace linter despite carrying a generated marker. Exclude them by path so make lint and the pre-push hook pass on untouched generated code. * perf(lyrics): drop []byte/string round-trips in parsers Apply code-review feedback to remove avoidable allocations in the lyrics parsers. isTTMLDocument now takes []byte directly, so parseTTMLWithDefaultLang no longer copies its buffer into a string before the TTML probe. parseSRTBlock splits its block with strings.Split instead of converting to []byte and back per line. ParseLyrics hoists strings.ToLower(suffix) out of the format loop. No behavior change; the dropped len(scanner)==0 SRT guard was dead (strings.Split never returns an empty slice, and the existing len(lines)==0 check still covers empty input). Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): colocate and unexport cue-normalization helpers Move the cue-normalization machinery out of lyrics.go into a dedicated lyrics_normalize.go (with lyrics_normalize_test.go), leaving lyrics.go to hold just the shared lyric types and their methods. lyrics.go was mixing the domain type/contract definitions with format-agnostic post-processing. Unexport normalizeLyrics, normalizeCueLines, and normalizeLineTiming: they have no callers outside the model package, so they should not be part of its public API. NormalizeCueEnds stays exported because the Subsonic enhanced-lyrics serializer (server/subsonic/lyrics.go) resolves cue ends per agent group while building the response; that is the only legitimate cross-package caller. Also includes a small no-op robustness tweak in parseLRC: len(times) == 0 instead of times == nil (equivalent here, more idiomatic). No behavior change. * test(lyrics): add direct coverage for NormalizeCueEnds NormalizeCueEnds is exported and carries the most intricate logic in the normalization cluster (fill-from-next, fill-from-fallback, both clamps, and the all-or-none clear), but was only exercised transitively. Add a focused spec covering each branch plus the empty-input and no-mutation guarantees, bringing the function to 100% coverage. * test(lyrics): cover legacy getLyrics across formats and sources Expand the legacy getLyrics e2e coverage from a single embedded-plain case to a table over all six fixtures: embedded LRC/plain/TTML and sidecar LRC/SRT/YAML. Each case asserts the v1 plain-text fallback contract — the structured lyric is flattened to LRC-style plain text with no timing markup leaking through (no LRC brackets, SRT arrows, or XML tags), regardless of the source format or whether it is embedded or a sidecar file. This pins the behavior that synced TTML/SRT/ YAML formats degrade gracefully to plain text on the legacy endpoint. * test(lyrics): cover songLyrics v1 vs v2 with word-level fixtures Correct and expand the e2e lyrics coverage to match the OpenSubsonic songLyrics extension contract: - v1 (getLyricsBySongId, no enhanced): line-level lyrics with no cueLine, kind, or agents — even for word-level formats (ELRC, Lyricsfile YAML). - v2 (getLyricsBySongId?enhanced=true): word-level cueLine surfaces for ELRC and YAML sources; kind="main" is set; a line-level source (SRT) still yields no cueLine even when enhanced. - legacy getLyrics (artist/title): the original Subsonic endpoint, flattening any format to plain text. A prior commit mislabeled this as the "v1 contract"; getLyrics predates OpenSubsonic and is unrelated to the extension versions. Drive these with the public-domain tests/fixtures/lyrics files (the same set the parser benchmarks use) so the e2e content stays in sync and actually carries the word-level timing needed to distinguish v1 from v2. The embedded "synced LRC" fixture is upgraded to ELRC (word-level); track counts are unchanged, so the rest of the suite is unaffected. * test(lyrics): parameterize v2 enhanced coverage across all formats Convert the v2 (enhanced) e2e block from three ad-hoc cases into a DescribeTable covering all six formats, matching the v1 and legacy tables. Each entry declares whether the source carries word-level timing: ELRC, TTML, and Lyricsfile YAML surface a cueLine; LRC, SRT, and plain text do not. All six get kind="main". Add word-level <span> timing to the first line of the auld-lang-syne.ttml fixture so TTML exercises the word-level cueLine path (the parser already supports <span begin/end>, but the fixture was line-level only). The first line now yields the same five word cues as the ELRC and YAML fixtures, keeping the table assertions uniform across formats. * fix(lyrics): honor caller language when Lyricsfile YAML omits it parseLyricsfile discarded the caller's language argument, so a Lyricsfile YAML parsed from an embedded tag or plugin response with no metadata.language was labeled "xxx" even when ParseLyrics was given a language. The SRT and TTML parsers already use the caller language as their default; fall back to it here too, preferring the document's own metadata.language when present. Also reword a misleading TTML comment: isTTMLDocument still runs an XML decode (it stops at the first element), so the skip avoids the full TTML parse, not the XML decoder entirely. * refactor(lyrics): consolidate lyrics parsing functions names Signed-off-by: Deluan <deluan@navidrome.org> * test(lyrics): drop test-only parse wrappers after parser rename Commit 48c0173e8 renamed the production parsers to parseTTML/parseSRT, which collided with the same-named test-only wrappers and broke the model test build (parseTTML/parseSRT redeclared). Remove the wrappers and call the production parsers directly with the placeholder language at each test site. * test(lyrics): complete the truncated enhanced-LRC fixture The auld-lang-syne.elrc fixture stopped after the first two stanzas (8 lyric lines) while every other format fixture carries the full 24-line song. Extend it to all 24 lines with per-word timing so it is a faithful enhanced-LRC sample and the EnhancedLRC parser benchmark runs on a workload comparable to the others. The first line's word timings are unchanged, so the e2e cueLine assertions still hold. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-19 18:25:35 -04:00