diff --git a/core/lyrics/lyrics_test.go b/core/lyrics/lyrics_test.go
index b00bcd576..7ce09c28a 100644
--- a/core/lyrics/lyrics_test.go
+++ b/core/lyrics/lyrics_test.go
@@ -230,7 +230,7 @@ var _ = Describe("Lyrics", func() {
}))
})
- It("returns a non-Lyricsfile YAML sidecar as plain text, shadowing lower-priority sources", func() {
+ It("skips a non-Lyricsfile YAML sidecar and resolves the next source", func() {
dir, err := os.MkdirTemp("", "lyrics-yaml-fallback-*")
Expect(err).ToNot(HaveOccurred())
DeferCleanup(func() {
@@ -247,14 +247,11 @@ var _ = Describe("Lyrics", func() {
Path: "song.mp3",
})
- // ParseLyrics falls back to plain text for any suffix when the content
- // doesn't match the structured format, so the .yaml hit is non-empty and
- // shadows the lower-priority .lrc entirely.
Expect(err).To(BeNil())
Expect(list).To(HaveLen(1))
- Expect(list[0].Synced).To(BeFalse())
+ Expect(list[0].Synced).To(BeTrue())
Expect(list[0].Line).To(Equal([]model.Line{
- {Value: "title: not lyricsfile"},
+ {Start: new(int64(1000)), Value: "Fallback line"},
}))
})
diff --git a/model/lyrics_lyricsfile.go b/model/lyrics_lyricsfile.go
index 49d416f3f..51073593e 100644
--- a/model/lyrics_lyricsfile.go
+++ b/model/lyrics_lyricsfile.go
@@ -71,6 +71,16 @@ func parseLyricsfile(lang string, contents []byte) (LyricList, error) {
const lyricsfileVersion = "1.0"
+func hasLyricsfileVersion(contents []byte) bool {
+ var header struct {
+ Version string `yaml:"version"`
+ }
+ if err := yaml.Unmarshal(contents, &header); err != nil {
+ return false
+ }
+ return strings.TrimSpace(header.Version) == lyricsfileVersion
+}
+
type lyricsfileDocument struct {
Version string `yaml:"version"`
Metadata lyricsfileMetadata `yaml:"metadata"`
diff --git a/model/lyrics_parse.go b/model/lyrics_parse.go
index 8aa095c50..04afd985b 100644
--- a/model/lyrics_parse.go
+++ b/model/lyrics_parse.go
@@ -3,32 +3,46 @@ package model
import (
"bytes"
"context"
+ "errors"
"fmt"
+ "regexp"
"slices"
"strings"
"github.com/navidrome/navidrome/log"
)
-// lyricParser returns an empty list (not an error) when the input is not its
-// format, so parsers can be tried in order. lang is the default for formats that
-// do not carry their own.
+// lyricParser parses content already claimed by its format. A nil list with no
+// error means a recognized, valid document that contains no lyrics.
type lyricParser func(lang string, contents []byte) (LyricList, error)
+var errLyricsFormatMismatch = errors.New("lyrics format mismatch")
+
+type lyricFormat struct {
+ name string
+ suffixes []string
+ claims func([]byte) bool
+ parse lyricParser
+}
+
// lyricFormats is the structured formats in content-sniff probe order; each
// row's suffixes drive sidecar dispatch. LRC/plain is the unlisted fallback floor.
-var lyricFormats = []struct {
- suffixes []string
- parse lyricParser
-}{
- {[]string{".ttml"}, parseTTML},
- {[]string{".srt"}, parseSRT},
- {[]string{".yaml", ".yml"}, parseLyricsfile},
+var lyricFormats = []lyricFormat{
+ {name: "TTML", suffixes: []string{".ttml"}, claims: claimsTTML, parse: parseTTML},
+ {name: "SRT", suffixes: []string{".srt"}, claims: claimsSRT, parse: parseSRT},
+ {name: "Lyricsfile", suffixes: []string{".yaml", ".yml"}, claims: claimsLyricsfile, parse: parseLyricsfile},
}
+var (
+ ttmlRootPrefixRegex = regexp.MustCompile(`(?is)^\s*(?:<\?xml\b[^>]*\?>\s*)?(?:\s*)*<(?:[[:alpha:]_][[:alnum:]_.-]*:)?tt(?:\s|/?>)`)
+ srtClaimRegex = regexp.MustCompile(`(?m)^\s*(?:\d+\s*\n\s*)?\d{1,2}:\d{2}:\d{2}[,.]\d{1,3}\s*-->\s*\d{1,2}:\d{2}:\d{2}[,.]\d{1,3}(?:\s|$)`)
+ lyricsfileRegex = regexp.MustCompile(`(?mi)^\s*["']?version["']?\s*:\s*["']?1\.0["']?\s*(?:#.*)?$`)
+)
+
// ParseLyrics is the single entry point for parsing lyrics. A known suffix routes
-// to that format's parser; an empty or "auto" suffix content-sniffs. Either way,
-// a structured parser that does not match falls back to the LRC/plain-text floor.
+// to that format's parser; an empty or "auto" suffix content-sniffs. Explicit
+// structured suffixes are strict: malformed or mismatched structured content is
+// returned as an error so a source resolver can continue to its next source.
//
// Parse failures are logged through ctx; callers that know the source should
// attach it for attribution, e.g. log.NewContext(ctx, "file", path).
@@ -37,37 +51,80 @@ func ParseLyrics(ctx context.Context, suffix, lang string, contents []byte) (Lyr
suffix = strings.ToLower(suffix)
sniff := suffix == "" || suffix == "auto"
- // Sniffing tries every format in order; a known suffix selects just its own.
- // Unmatched suffixes leave no candidates, so parseFirstMatch falls to plain.
- candidates := make([]lyricParser, 0, len(lyricFormats))
+ // Sniffing tries every structured format in order. A known structured suffix
+ // selects one strict parser; LRC/text and unknown textual suffixes retain the
+ // longstanding LRC/plain fallback.
+ candidates := make([]lyricFormat, 0, len(lyricFormats))
for _, f := range lyricFormats {
if sniff || slices.Contains(f.suffixes, suffix) {
- candidates = append(candidates, f.parse)
+ candidates = append(candidates, f)
}
}
- return parseFirstMatch(ctx, sniff, lang, contents, candidates...)
-}
-func parseFirstMatch(ctx context.Context, sniff bool, lang string, contents []byte, candidates ...lyricParser) (LyricList, error) {
- for _, parse := range candidates {
- list, err := parse(lang, contents)
- if err == nil && len(list) > 0 {
- return list, nil
+ if !sniff && len(candidates) > 0 {
+ format := candidates[0]
+ list, err := parseClaimedLyrics(format, lang, contents)
+ if errors.Is(err, errLyricsFormatMismatch) {
+ err = fmt.Errorf("declared %s lyrics do not match the format: %w", format.name, err)
}
if err != nil {
- // While sniffing, a probe rejecting content it does not own is expected
- // control flow, so keep it at trace. A failure under an explicit suffix
- // means the declared format is malformed and deserves a warning.
- if sniff {
- log.Trace(ctx, "Lyrics probe did not match, trying next format", err)
- } else {
- log.Warn(ctx, "Error parsing lyrics, falling back to plain text", err)
- }
+ log.Warn(ctx, "Error parsing declared lyrics", "format", format.name, err)
}
+ return list, err
+ }
+
+ return parseFirstMatch(ctx, lang, contents, candidates...)
+}
+
+func parseFirstMatch(ctx context.Context, lang string, contents []byte, candidates ...lyricFormat) (LyricList, error) {
+ for _, format := range candidates {
+ list, err := parseClaimedLyrics(format, lang, contents)
+ if errors.Is(err, errLyricsFormatMismatch) {
+ log.Trace(ctx, "Lyrics probe did not match, trying next format", "format", format.name)
+ continue
+ }
+ if err != nil {
+ return nil, fmt.Errorf("parsing claimed %s lyrics: %w", format.name, err)
+ }
+ // A claimed, valid-empty document owns the content and deliberately stops
+ // sniffing instead of being reinterpreted as another format or plain text.
+ return list, nil
}
return plainLRC(lang, contents)
}
+func parseClaimedLyrics(format lyricFormat, lang string, contents []byte) (LyricList, error) {
+ if !format.claims(contents) {
+ return nil, errLyricsFormatMismatch
+ }
+ list, err := format.parse(lang, contents)
+ if err != nil {
+ return nil, err
+ }
+ return list, nil
+}
+
+func claimsTTML(contents []byte) bool {
+ trimmed := bytes.TrimSpace(contents)
+ if len(trimmed) == 0 || trimmed[0] != '<' {
+ return false
+ }
+ return isTTMLDocument(contents) || ttmlRootPrefixRegex.Match(contents)
+}
+
+func claimsSRT(contents []byte) bool {
+ raw := bytes.ReplaceAll(contents, []byte("\r\n"), []byte("\n"))
+ raw = bytes.ReplaceAll(raw, []byte("\r"), []byte("\n"))
+ return srtClaimRegex.Match(raw)
+}
+
+func claimsLyricsfile(contents []byte) bool {
+ if lyricsfileRegex.Match(contents) {
+ return true
+ }
+ return hasLyricsfileVersion(contents)
+}
+
func plainLRC(lang string, contents []byte) (LyricList, error) {
lyric, err := parseLRC(lang, string(contents))
if err != nil {
diff --git a/model/lyrics_parse_test.go b/model/lyrics_parse_test.go
index 0b47e55e9..dc2b1b439 100644
--- a/model/lyrics_parse_test.go
+++ b/model/lyrics_parse_test.go
@@ -1,8 +1,6 @@
package model
import (
- "strings"
-
"github.com/navidrome/navidrome/log"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
@@ -34,6 +32,19 @@ var _ = Describe("ParseLyrics", func() {
Expect(list[0].Line[0].Value).To(Equal("auto ttml"))
})
+ DescribeTable("accepts valid TTML XML prologs",
+ func(suffix, prolog string) {
+ contents := prolog + ``
+ list, err := ParseLyrics(GinkgoT().Context(), suffix, "eng", []byte(contents))
+
+ Expect(err).ToNot(HaveOccurred())
+ Expect(list).To(HaveLen(1))
+ Expect(list[0].Line[0].Value).To(Equal("prolog ttml"))
+ },
+ Entry("explicit suffix with processing instruction", ".ttml", ``),
+ Entry("content sniffing with doctype", "", ``),
+ )
+
It("empty suffix content-sniffs (YAML)", func() {
yaml := "version: \"1.0\"\nmetadata:\n language: eng\nlines:\n - text: auto yaml\n start_ms: 1000\n"
list, err := ParseLyrics(GinkgoT().Context(), "auto", "eng", []byte(yaml))
@@ -42,14 +53,62 @@ var _ = Describe("ParseLyrics", func() {
Expect(list[0].Line[0].Value).To(Equal("auto yaml"))
})
- It("falls back to plain text when a known suffix fails to parse structurally", func() {
+ DescribeTable("accepts quoted Lyricsfile version keys",
+ func(suffix string) {
+ contents := "\"version\": \"1.0\"\nmetadata:\n language: eng\nlines:\n - text: quoted version\n start_ms: 1000\n"
+ list, err := ParseLyrics(GinkgoT().Context(), suffix, "eng", []byte(contents))
+
+ Expect(err).ToNot(HaveOccurred())
+ Expect(list).To(HaveLen(1))
+ Expect(list[0].Line[0].Value).To(Equal("quoted version"))
+ },
+ Entry("explicit YAML", ".yaml"),
+ Entry("content sniffing", "auto"),
+ )
+
+ It("returns an error when an explicit structured suffix does not match", func() {
list, err := ParseLyrics(GinkgoT().Context(), ".srt", "eng", []byte("not actually an srt file"))
+ Expect(err).To(MatchError(ContainSubstring("declared SRT lyrics do not match")))
+ Expect(list).To(BeNil())
+ })
+
+ It("retains LRC/plain fallback for textual suffixes", func() {
+ list, err := ParseLyrics(GinkgoT().Context(), ".lrc", "eng", []byte("not actually timed"))
Expect(err).ToNot(HaveOccurred())
Expect(list).To(HaveLen(1))
Expect(list[0].Synced).To(BeFalse())
- Expect(list[0].Line[0].Value).To(Equal("not actually an srt file"))
+ Expect(list[0].Line[0].Value).To(Equal("not actually timed"))
})
+ DescribeTable("recognized valid-empty structured documents stop sniffing",
+ func(contents string) {
+ list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(contents))
+ Expect(err).ToNot(HaveOccurred())
+ Expect(list).To(BeEmpty())
+ },
+ Entry("TTML", ``),
+ Entry("SRT", "1\n00:00:01,000 --> 00:00:02,000\n"),
+ Entry("Lyricsfile", "version: \"1.0\"\nmetadata:\n language: eng\n"),
+ )
+
+ It("does not reinterpret malformed claimed structured content as plain text", func() {
+ list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(`Broken`))
+ Expect(err).To(MatchError(ContainSubstring("parsing claimed TTML lyrics")))
+ Expect(list).To(BeNil())
+ })
+
+ DescribeTable("generic content remains eligible for plain fallback",
+ func(contents string) {
+ list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(contents))
+ Expect(err).ToNot(HaveOccurred())
+ Expect(list).To(HaveLen(1))
+ Expect(list[0].Synced).To(BeFalse())
+ },
+ Entry("prose", "A plain lyric"),
+ Entry("generic XML", "A generic lyric"),
+ Entry("generic YAML", "title: A generic lyric"),
+ )
+
Describe("logging on parser probe failures", func() {
var hook *test.Hook
@@ -92,10 +151,10 @@ var _ = Describe("ParseLyrics", func() {
// failure is worth surfacing loudly — and it must name the file.
It("warns and names the file when a requested suffix fails to parse", func() {
ctx := log.NewContext(GinkgoT().Context(), "file", "/music/song.yaml")
- list, err := ParseLyrics(ctx, ".yaml", "eng", []byte("not: [valid, yaml\n"))
+ list, err := ParseLyrics(ctx, ".yaml", "eng", []byte("version: \"1.0\"\nnot: [valid, yaml\n"))
- Expect(err).ToNot(HaveOccurred())
- Expect(list).To(HaveLen(1)) // still falls back to plain text
+ Expect(err).To(HaveOccurred())
+ Expect(list).To(BeNil())
entry := hook.LastEntry()
Expect(entry).ToNot(BeNil())
Expect(entry.Level).To(Equal(logrus.WarnLevel))
@@ -234,25 +293,13 @@ Another subtitle line`
Expect(list[0].Line[0].Cue).To(HaveLen(2))
})
- It("should fall back to plain lyrics when embedded TTML is invalid", func() {
- content := `
-
-
Broken
-
-`
+ It("should reject malformed embedded TTML instead of returning raw markup", func() {
+ content := `Broken`
list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(content))
- Expect(err).ToNot(HaveOccurred())
- Expect(list).To(HaveLen(1))
- Expect(list[0].Lang).To(Equal("eng"))
- Expect(list[0].Synced).To(BeFalse())
- Expect(list[0].Line).ToNot(BeEmpty())
- values := make([]string, 0, len(list[0].Line))
- for _, line := range list[0].Line {
- values = append(values, line.Value)
- }
- Expect(strings.Join(values, "\n")).To(ContainSubstring("Broken"))
+ Expect(err).To(HaveOccurred())
+ Expect(list).To(BeNil())
})
It("detects a Lyricsfile YAML payload via content-sniffing", func() {
diff --git a/model/lyrics_ttml.go b/model/lyrics_ttml.go
index 43d4699af..8feadc4bd 100644
--- a/model/lyrics_ttml.go
+++ b/model/lyrics_ttml.go
@@ -4,6 +4,7 @@ import (
"bytes"
"encoding/xml"
"errors"
+ "fmt"
"io"
"math"
"regexp"
@@ -123,6 +124,9 @@ func parseTTML(defaultLang string, contents []byte) (LyricList, error) {
// text — isTTMLDocument does a cheap decode that stops at the first element.
// Checked after the encoding fixup so UTF-16-declared documents are recognized.
if !isTTMLDocument(contents) {
+ if claimsTTML(contents) {
+ return nil, fmt.Errorf("malformed TTML document")
+ }
return nil, nil
}
diff --git a/plugins/lyrics_adapter_test.go b/plugins/lyrics_adapter_test.go
index d110665f5..3269f58c9 100644
--- a/plugins/lyrics_adapter_test.go
+++ b/plugins/lyrics_adapter_test.go
@@ -150,6 +150,22 @@ var _ = Describe("LyricsPlugin", Ordered, func() {
Entry("lrc", "lrc", true, "plugin lrc line"),
Entry("plain", "plain", false, "plugin plain line"),
)
+
+ It("keeps valid entries when another plugin lyric is malformed", func() {
+ manager, _ := createTestManagerWithPlugins(map[string]map[string]string{
+ "test-lyrics": {"format": "mixed"},
+ }, "test-lyrics"+PackageExtension)
+
+ p, ok := manager.LoadLyricsProvider("test-lyrics")
+ Expect(ok).To(BeTrue())
+
+ result, err := p.GetLyrics(GinkgoT().Context(), &model.MediaFile{ID: "track-1"})
+ Expect(err).ToNot(HaveOccurred())
+ Expect(result).To(HaveLen(1))
+ Expect(result[0].Line).To(Equal([]model.Line{
+ {Start: new(int64(1000)), Value: "valid plugin line"},
+ }))
+ })
})
Describe("PluginNames", func() {
diff --git a/plugins/testdata/test-lyrics/main.go b/plugins/testdata/test-lyrics/main.go
index 2ee2dabbf..062aab4bc 100644
--- a/plugins/testdata/test-lyrics/main.go
+++ b/plugins/testdata/test-lyrics/main.go
@@ -48,6 +48,13 @@ func (t *testLyrics) GetLyrics(input lyrics.GetLyricsRequest) (lyrics.GetLyricsR
case "plain":
lang = "eng"
text = "plugin plain line"
+ case "mixed":
+ return lyrics.GetLyricsResponse{
+ Lyrics: []lyrics.LyricsText{
+ {Lang: "eng", Text: `
malformed`},
+ {Lang: "eng", Text: "[00:01.00]valid plugin line"},
+ },
+ }, nil
}
if text != "" {
return lyrics.GetLyricsResponse{