From 81c3597018e0061c39f5af7343b239bde59978da Mon Sep 17 00:00:00 2001 From: ranokay Date: Thu, 28 May 2026 01:32:38 +0300 Subject: [PATCH] fix: address lyrics parser review feedback --- core/lyrics/embedded_test.go | 13 +++++++++++++ core/lyrics/lyrics.go | 2 +- core/lyrics/lyrics_test.go | 32 ++++++++++++++++++++++++++++++++ core/lyrics/srt.go | 7 +++++-- core/lyrics/ttml.go | 20 +++++++++++++++----- core/lyrics/ttml_test.go | 24 ++++++++++++++++++++++++ model/lyrics.go | 8 ++++++-- model/lyrics_test.go | 36 ++++++++++++++++++++++++++++++++++++ 8 files changed, 132 insertions(+), 10 deletions(-) diff --git a/core/lyrics/embedded_test.go b/core/lyrics/embedded_test.go index a855c9686..d875bbdcf 100644 --- a/core/lyrics/embedded_test.go +++ b/core/lyrics/embedded_test.go @@ -120,6 +120,19 @@ Another subtitle line` })) }) + It("should parse embedded SRT blocks separated by whitespace-only blank lines", func() { + content := "1\n00:00:01,000 --> 00:00:02,000\nFirst subtitle\n \n2\n00:00:03,000 --> 00:00:04,000\nSecond subtitle" + + list, err := ParseEmbedded("eng", content) + + Expect(err).ToNot(HaveOccurred()) + Expect(list).To(HaveLen(1)) + Expect(list[0].Line).To(Equal([]model.Line{ + {Start: ptr(int64(1000)), End: ptr(int64(2000)), Value: "First subtitle"}, + {Start: ptr(int64(3000)), End: ptr(int64(4000)), Value: "Second subtitle"}, + })) + }) + It("should keep embedded enhanced LRC cues", func() { content := "[00:01.00]<00:01.00>Lead <00:01.50>words" diff --git a/core/lyrics/lyrics.go b/core/lyrics/lyrics.go index cc3d574b3..73a0479f3 100644 --- a/core/lyrics/lyrics.go +++ b/core/lyrics/lyrics.go @@ -83,7 +83,7 @@ func (l *lyricsService) getLyricsFromSource(ctx context.Context, mf *model.Media case strings.EqualFold(pattern, "embedded"): return fromEmbedded(ctx, mf) case strings.HasPrefix(pattern, "."): - return fromExternalFile(ctx, mf, strings.ToLower(pattern)) + return fromExternalFile(ctx, mf, pattern) default: return l.fromPlugin(ctx, mf, pattern) } diff --git a/core/lyrics/lyrics_test.go b/core/lyrics/lyrics_test.go index 6cc5978a1..68a7f844d 100644 --- a/core/lyrics/lyrics_test.go +++ b/core/lyrics/lyrics_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" @@ -192,6 +193,37 @@ var _ = Describe("sources", func() { Expect(list).To(Equal(ttmlLyrics)) }) + It("preserves configured sidecar suffix casing on case-sensitive filesystems", func() { + dir, err := os.MkdirTemp("", "lyrics-case-*") + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { + Expect(os.RemoveAll(dir)).To(Succeed()) + }) + + probe := filepath.Join(dir, "CASECHECK") + Expect(os.WriteFile(probe, []byte("probe"), 0644)).To(Succeed()) + _, err = os.Stat(filepath.Join(dir, "casecheck")) + if err == nil { + Skip("filesystem is case-insensitive") + } + Expect(os.IsNotExist(err)).To(BeTrue()) + + conf.Server.LyricsPriority = ".LRC" + Expect(os.WriteFile(filepath.Join(dir, "song.LRC"), []byte("[00:01.00]Upper suffix"), 0644)).To(Succeed()) + + svc := lyrics.NewLyrics(nil) + list, err := svc.GetLyrics(ctx, &model.MediaFile{ + LibraryPath: dir, + Path: "song.mp3", + }) + + Expect(err).To(BeNil()) + Expect(list).To(HaveLen(1)) + Expect(list[0].Line).To(Equal([]model.Line{ + {Start: ptr(int64(1000)), Value: "Upper suffix"}, + })) + }) + Context("Errors", func() { var RegularUserContext = XContext var isRegularUser = os.Getuid() != 0 diff --git a/core/lyrics/srt.go b/core/lyrics/srt.go index e16c405d5..05a43ca57 100644 --- a/core/lyrics/srt.go +++ b/core/lyrics/srt.go @@ -10,7 +10,10 @@ import ( "github.com/navidrome/navidrome/utils/str" ) -var srtTimeRegex = regexp.MustCompile(`^\s*(\d{1,2}):(\d{2}):(\d{2})[,.](\d{1,3})\s*$`) +var ( + srtTimeRegex = regexp.MustCompile(`^\s*(\d{1,2}):(\d{2}):(\d{2})[,.](\d{1,3})\s*$`) + srtBlockSeparatorRegex = regexp.MustCompile(`\n\s*\n`) +) func parseSRT(contents []byte) (model.LyricList, error) { return parseSRTWithLanguage(contents, "xxx") @@ -51,7 +54,7 @@ func splitSRTBlocks(raw string) []string { return nil } - parts := strings.Split(raw, "\n\n") + parts := srtBlockSeparatorRegex.Split(raw, -1) blocks := make([]string, 0, len(parts)) for _, part := range parts { part = strings.TrimSpace(part) diff --git a/core/lyrics/ttml.go b/core/lyrics/ttml.go index 8df7f930a..163bd657c 100644 --- a/core/lyrics/ttml.go +++ b/core/lyrics/ttml.go @@ -816,6 +816,20 @@ func contextHasRole(roles string, role string) bool { return false } +func appendTTMLRoles(existing string, roles string) string { + for _, role := range strings.Fields(roles) { + if contextHasRole(existing, role) { + continue + } + if existing == "" { + existing = role + } else { + existing += " " + role + } + } + return existing +} + func (p *ttmlParser) addMainLine(lang string, lineKey string, line model.Line) { lang = normalizeTTMLLang(lang) if _, ok := p.mainLinesByLang[lang]; !ok { @@ -866,11 +880,7 @@ func (p *ttmlParser) childContext(attrs []xml.Attr, parent ttmlTimingContext) tt if role, ok := attrValue(attrs, "role"); ok { role = strings.TrimSpace(role) if role != "" { - if ctx.role == "" { - ctx.role = role - } else if !strings.Contains(ctx.role, role) { - ctx.role = ctx.role + " " + role - } + ctx.role = appendTTMLRoles(ctx.role, role) } } diff --git a/core/lyrics/ttml_test.go b/core/lyrics/ttml_test.go index dd8d206b1..07f41a080 100644 --- a/core/lyrics/ttml_test.go +++ b/core/lyrics/ttml_test.go @@ -145,6 +145,30 @@ var _ = Describe("parseTTML", func() { Expect(line.Cue[2]).To(Equal(model.Cue{Start: ptr(int64(2000)), End: ptr(int64(2500)), Value: "echo", ByteStart: 6, ByteEnd: 9, AgentID: "__nd_bg__|main"})) }) + It("should append role tokens exactly instead of using substring matches", func() { + content := []byte(` + + +
+

LeadEcho

+
+ +`) + + list, err := parseTTML(content) + + Expect(err).ToNot(HaveOccurred()) + Expect(list).To(HaveLen(1)) + Expect(list[0].Agents).To(Equal([]model.Agent{ + {ID: "main", Role: "main"}, + {ID: "__nd_bg__|main", Role: "bg"}, + })) + Expect(list[0].Line).To(HaveLen(1)) + Expect(list[0].Line[0].Cue).To(HaveLen(2)) + Expect(list[0].Line[0].Cue[0].AgentID).To(Equal("main")) + Expect(list[0].Line[0].Cue[1].AgentID).To(Equal("__nd_bg__|main")) + }) + It("should parse named TTML agents into main, voice, and group roles", func() { content := []byte(` diff --git a/model/lyrics.go b/model/lyrics.go index 9a57ebaad..f9f21b873 100644 --- a/model/lyrics.go +++ b/model/lyrics.go @@ -46,7 +46,7 @@ type Lyrics struct { } // support the standard [mm:ss.mm], as well as [hh:*] and [*.mmm] -const timeRegexString = `\[([0-9]{1,2}:)?([0-9]{1,2}):([0-9]{1,2})(.[0-9]{1,3})?\]` +const timeRegexString = `\[([0-9]{1,2}:)?([0-9]{1,2}):([0-9]{1,2})(\.[0-9]{1,3})?\]` var ( // Should either be at the beginning of file, or beginning of line @@ -55,7 +55,7 @@ var ( lrcIdRegex = regexp.MustCompile(`\[(ar|ti|offset|lang):([^]]+)]`) // Enhanced LRC: inline word-level timing markers like <00:12.34> - enhancedLRCTimeString = `<([0-9]{1,2}:)?([0-9]{1,2}):([0-9]{1,2})(.[0-9]{1,3})?>` + enhancedLRCTimeString = `<([0-9]{1,2}:)?([0-9]{1,2}):([0-9]{1,2})(\.[0-9]{1,3})?>` enhancedLRCRegex = regexp.MustCompile(enhancedLRCTimeString) ) @@ -378,6 +378,10 @@ func NormalizeCueLines(lines []Line) []Line { copy(normalized, lines) for i := range normalized { + if len(normalized[i].Cue) > 0 { + normalized[i].Cue = slices.Clone(normalized[i].Cue) + } + var fallbackEnd *int64 if normalized[i].End != nil { v := *normalized[i].End diff --git a/model/lyrics_test.go b/model/lyrics_test.go index f04d6e04a..21bdf0e3d 100644 --- a/model/lyrics_test.go +++ b/model/lyrics_test.go @@ -139,6 +139,15 @@ var _ = Describe("ToLyrics", func() { Expect(line1.Cue[1].End).To(BeNil()) }) + It("should not parse malformed Enhanced LRC timing markers", func() { + lyrics, err := ToLyrics("xxx", "[00:01.00]<00:01a50>Not a marker") + Expect(err).ToNot(HaveOccurred()) + Expect(lyrics.Synced).To(BeTrue()) + Expect(lyrics.Line).To(Equal([]Line{ + {Start: new(int64(1000)), Value: "<00:01a50>Not a marker"}, + })) + }) + It("should ignore Enhanced LRC markers and return plain lines when no markers present", func() { a, b := int64(1000), int64(3000) lyrics, err := ToLyrics("xxx", "[00:01.00]Plain line\n[00:03.00]Another plain line") @@ -190,3 +199,30 @@ var _ = Describe("ToLyrics", func() { })) }) }) + +var _ = Describe("NormalizeCueLines", func() { + It("should not mutate caller cue slices when filling missing cue end times", func() { + start0, start1, nextLineStart := int64(1000), int64(1500), int64(3000) + lines := []Line{ + { + Start: &start0, + Value: "Some lyrics", + Cue: []Cue{ + {Start: &start0, Value: "Some ", ByteStart: 0, ByteEnd: 4}, + {Start: &start1, Value: "lyrics", ByteStart: 5, ByteEnd: 10}, + }, + }, + { + Start: &nextLineStart, + Value: "Next line", + }, + } + + normalized := NormalizeCueLines(lines) + + Expect(normalized[0].Cue[0].End).To(Equal(&start1)) + Expect(normalized[0].Cue[1].End).To(Equal(&nextLineStart)) + Expect(lines[0].Cue[0].End).To(BeNil()) + Expect(lines[0].Cue[1].End).To(BeNil()) + }) +})