From ec9f9aa243ef97593287e5460e7e7b140f6ecbf1 Mon Sep 17 00:00:00 2001
From: Kendall Garner <17521368+kgarner7@users.noreply.github.com>
Date: Wed, 30 Apr 2025 12:10:19 +0000
Subject: [PATCH] feat:(server): support reading lyrics from filesystem (#2897)

* simplified lyrics handling

* address initial feedback

* add some trace and error logging

* allow fallback lyrics

* update nit

* restore artist/title filter only
---
 conf/configuration.go                   |   3 +
 core/lyrics/lyrics.go                   |  37 +++++++
 core/lyrics/lyrics_suite_test.go        |  17 ++++
 core/lyrics/lyrics_test.go              | 124 ++++++++++++++++++++++++
 core/lyrics/sources.go                  |  51 ++++++++++
 core/lyrics/sources_test.go             | 112 +++++++++++++++++++++
 model/lyrics.go                         |   4 +-
 model/lyrics_test.go                    |   3 +-
 server/subsonic/api_suite_test.go       |   2 +
 server/subsonic/filter/filters.go       |   4 +-
 server/subsonic/media_retrieval.go      |  19 ++--
 server/subsonic/media_retrieval_test.go |  20 ++++
 tests/fixtures/test.lrc                 |   6 ++
 tests/fixtures/test.txt                 |   2 +
 14 files changed, 391 insertions(+), 13 deletions(-)
 create mode 100644 core/lyrics/lyrics.go
 create mode 100644 core/lyrics/lyrics_suite_test.go
 create mode 100644 core/lyrics/lyrics_test.go
 create mode 100644 core/lyrics/sources.go
 create mode 100644 core/lyrics/sources_test.go
 create mode 100644 tests/fixtures/test.lrc
 create mode 100644 tests/fixtures/test.txt

diff --git a/conf/configuration.go b/conf/configuration.go
index 2ffefc616..d15b0de3e 100644
--- a/conf/configuration.go
+++ b/conf/configuration.go
@@ -93,6 +93,7 @@ type configOptions struct {
 	PID                             pidOptions
 	Inspect                         inspectOptions
 	Subsonic                        subsonicOptions
+	LyricsPriority                  string
 
 	Agents       string
 	LastFM       lastfmOptions
@@ -528,6 +529,8 @@ func init() {
 	viper.SetDefault("inspect.backloglimit", consts.RequestThrottleBacklogLimit)
 	viper.SetDefault("inspect.backlogtimeout", consts.RequestThrottleBacklogTimeout)
 
+	viper.SetDefault("lyricspriority", ".lrc,.txt,embedded")
+
 	// DevFlags. These are used to enable/disable debugging and incomplete features
 	viper.SetDefault("devlogsourceline", false)
 	viper.SetDefault("devenableprofiler", false)
diff --git a/core/lyrics/lyrics.go b/core/lyrics/lyrics.go
new file mode 100644
index 000000000..858a3ffd8
--- /dev/null
+++ b/core/lyrics/lyrics.go
@@ -0,0 +1,37 @@
+package lyrics
+
+import (
+	"context"
+	"strings"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/model"
+)
+
+func GetLyrics(ctx context.Context, mf *model.MediaFile) (model.LyricList, error) {
+	var lyricsList model.LyricList
+	var err error
+
+	for pattern := range strings.SplitSeq(strings.ToLower(conf.Server.LyricsPriority), ",") {
+		pattern = strings.TrimSpace(pattern)
+		switch {
+		case pattern == "embedded":
+			lyricsList, err = fromEmbedded(ctx, mf)
+		case strings.HasPrefix(pattern, "."):
+			lyricsList, err = fromExternalFile(ctx, mf, pattern)
+		default:
+			log.Error(ctx, "Invalid lyric pattern", "pattern", pattern)
+		}
+
+		if err != nil {
+			log.Error(ctx, "error parsing lyrics", "source", pattern, err)
+		}
+
+		if len(lyricsList) > 0 {
+			return lyricsList, nil
+		}
+	}
+
+	return nil, nil
+}
diff --git a/core/lyrics/lyrics_suite_test.go b/core/lyrics/lyrics_suite_test.go
new file mode 100644
index 000000000..f87381905
--- /dev/null
+++ b/core/lyrics/lyrics_suite_test.go
@@ -0,0 +1,17 @@
+package lyrics_test
+
+import (
+	"testing"
+
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestLyrics(t *testing.T) {
+	tests.Init(t, false)
+	log.SetLevel(log.LevelFatal)
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Lyrics Suite")
+}
diff --git a/core/lyrics/lyrics_test.go b/core/lyrics/lyrics_test.go
new file mode 100644
index 000000000..f4197ccf6
--- /dev/null
+++ b/core/lyrics/lyrics_test.go
@@ -0,0 +1,124 @@
+package lyrics_test
+
+import (
+	"context"
+	"encoding/json"
+	"os"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/conf/configtest"
+	"github.com/navidrome/navidrome/core/lyrics"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/utils"
+	"github.com/navidrome/navidrome/utils/gg"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("sources", func() {
+	var mf model.MediaFile
+	var ctx context.Context
+
+	const badLyrics = "This is a set of lyrics\nThat is not good"
+	unsynced, _ := model.ToLyrics("xxx", badLyrics)
+	embeddedLyrics := model.LyricList{*unsynced}
+
+	syncedLyrics := model.LyricList{
+		model.Lyrics{
+			DisplayArtist: "Rick Astley",
+			DisplayTitle:  "That one song",
+			Lang:          "eng",
+			Line: []model.Line{
+				{
+					Start: gg.P(int64(18800)),
+					Value: "We're no strangers to love",
+				},
+				{
+					Start: gg.P(int64(22801)),
+					Value: "You know the rules and so do I",
+				},
+			},
+			Offset: gg.P(int64(-100)),
+			Synced: true,
+		},
+	}
+
+	unsyncedLyrics := model.LyricList{
+		model.Lyrics{
+			Lang: "xxx",
+			Line: []model.Line{
+				{
+					Value: "We're no strangers to love",
+				},
+				{
+					Value: "You know the rules and so do I",
+				},
+			},
+			Synced: false,
+		},
+	}
+
+	BeforeEach(func() {
+		DeferCleanup(configtest.SetupConfig())
+
+		lyricsJson, _ := json.Marshal(embeddedLyrics)
+
+		mf = model.MediaFile{
+			Lyrics: string(lyricsJson),
+			Path:   "tests/fixtures/test.mp3",
+		}
+		ctx = context.Background()
+	})
+
+	DescribeTable("Lyrics Priority", func(priority string, expected model.LyricList) {
+		conf.Server.LyricsPriority = priority
+		list, err := lyrics.GetLyrics(ctx, &mf)
+		Expect(err).To(BeNil())
+		Expect(list).To(Equal(expected))
+	},
+		Entry("embedded > lrc > txt", "embedded,.lrc,.txt", embeddedLyrics),
+		Entry("lrc > embedded > txt", ".lrc,embedded,.txt", syncedLyrics),
+		Entry("txt > lrc > embedded", ".txt,.lrc,embedded", unsyncedLyrics))
+
+	Context("Errors", func() {
+		var RegularUserContext = XContext
+		var isRegularUser = os.Getuid() != 0
+		if isRegularUser {
+			RegularUserContext = Context
+		}
+
+		RegularUserContext("run without root permissions", func() {
+			var accessForbiddenFile string
+
+			BeforeEach(func() {
+				accessForbiddenFile = utils.TempFileName("access_forbidden-", ".mp3")
+
+				f, err := os.OpenFile(accessForbiddenFile, os.O_WRONLY|os.O_CREATE, 0222)
+				Expect(err).ToNot(HaveOccurred())
+
+				mf.Path = accessForbiddenFile
+
+				DeferCleanup(func() {
+					Expect(f.Close()).To(Succeed())
+					Expect(os.Remove(accessForbiddenFile)).To(Succeed())
+				})
+			})
+
+			It("should fallback to embedded if an error happens when parsing file", func() {
+				conf.Server.LyricsPriority = ".mp3,embedded"
+
+				list, err := lyrics.GetLyrics(ctx, &mf)
+				Expect(err).To(BeNil())
+				Expect(list).To(Equal(embeddedLyrics))
+			})
+
+			It("should return nothing if error happens when trying to parse file", func() {
+				conf.Server.LyricsPriority = ".mp3"
+
+				list, err := lyrics.GetLyrics(ctx, &mf)
+				Expect(err).To(BeNil())
+				Expect(list).To(BeEmpty())
+			})
+		})
+	})
+})
diff --git a/core/lyrics/sources.go b/core/lyrics/sources.go
new file mode 100644
index 000000000..6d4a4cc6f
--- /dev/null
+++ b/core/lyrics/sources.go
@@ -0,0 +1,51 @@
+package lyrics
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path"
+
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/model"
+)
+
+func fromEmbedded(ctx context.Context, mf *model.MediaFile) (model.LyricList, error) {
+	if mf.Lyrics != "" {
+		log.Trace(ctx, "embedded lyrics found in file", "title", mf.Title)
+		return mf.StructuredLyrics()
+	}
+
+	log.Trace(ctx, "no embedded lyrics for file", "path", mf.Title)
+
+	return nil, nil
+}
+
+func fromExternalFile(ctx context.Context, mf *model.MediaFile, suffix string) (model.LyricList, error) {
+	basePath := mf.AbsolutePath()
+	ext := path.Ext(basePath)
+
+	externalLyric := basePath[0:len(basePath)-len(ext)] + suffix
+
+	contents, err := os.ReadFile(externalLyric)
+
+	if errors.Is(err, os.ErrNotExist) {
+		log.Trace(ctx, "no lyrics found at path", "path", externalLyric)
+		return nil, nil
+	} else if err != nil {
+		return nil, err
+	}
+
+	lyrics, err := model.ToLyrics("xxx", string(contents))
+	if err != nil {
+		log.Error(ctx, "error parsing lyric external file", "path", externalLyric, err)
+		return nil, err
+	} else if lyrics == nil {
+		log.Trace(ctx, "empty lyrics from external file", "path", externalLyric)
+		return nil, nil
+	}
+
+	log.Trace(ctx, "retrieved lyrics from external file", "path", externalLyric)
+
+	return model.LyricList{*lyrics}, nil
+}
diff --git a/core/lyrics/sources_test.go b/core/lyrics/sources_test.go
new file mode 100644
index 000000000..e92564c00
--- /dev/null
+++ b/core/lyrics/sources_test.go
@@ -0,0 +1,112 @@
+package lyrics
+
+import (
+	"context"
+	"encoding/json"
+
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/utils/gg"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("sources", func() {
+	ctx := context.Background()
+
+	Describe("fromEmbedded", func() {
+		It("should return nothing for a media file with no lyrics", func() {
+			mf := model.MediaFile{}
+			lyrics, err := fromEmbedded(ctx, &mf)
+
+			Expect(err).To(BeNil())
+			Expect(lyrics).To(HaveLen(0))
+		})
+
+		It("should return lyrics for a media file with well-formatted lyrics", func() {
+			const syncedLyrics = "[00:18.80]We're no strangers to love\n[00:22.801]You know the rules and so do I"
+			const unsyncedLyrics = "We're no strangers to love\nYou know the rules and so do I"
+
+			synced, _ := model.ToLyrics("eng", syncedLyrics)
+			unsynced, _ := model.ToLyrics("xxx", unsyncedLyrics)
+
+			expectedList := model.LyricList{*synced, *unsynced}
+			lyricsJson, err := json.Marshal(expectedList)
+
+			Expect(err).ToNot(HaveOccurred())
+
+			mf := model.MediaFile{
+				Lyrics: string(lyricsJson),
+			}
+
+			lyrics, err := fromEmbedded(ctx, &mf)
+			Expect(err).To(BeNil())
+			Expect(lyrics).ToNot(BeNil())
+			Expect(lyrics).To(Equal(expectedList))
+		})
+
+		It("should return an error if somehow the JSON is bad", func() {
+			mf := model.MediaFile{Lyrics: "["}
+			lyrics, err := fromEmbedded(ctx, &mf)
+
+			Expect(lyrics).To(HaveLen(0))
+			Expect(err).ToNot(BeNil())
+		})
+	})
+
+	Describe("fromExternalFile", func() {
+		It("should return nil for lyrics that don't exist", func() {
+			mf := model.MediaFile{Path: "tests/fixtures/01 Invisible (RED) Edit Version.mp3"}
+			lyrics, err := fromExternalFile(ctx, &mf, ".lrc")
+
+			Expect(err).To(BeNil())
+			Expect(lyrics).To(HaveLen(0))
+		})
+
+		It("should return synchronized lyrics from a file", func() {
+			mf := model.MediaFile{Path: "tests/fixtures/test.mp3"}
+			lyrics, err := fromExternalFile(ctx, &mf, ".lrc")
+
+			Expect(err).To(BeNil())
+			Expect(lyrics).To(Equal(model.LyricList{
+				model.Lyrics{
+					DisplayArtist: "Rick Astley",
+					DisplayTitle:  "That one song",
+					Lang:          "eng",
+					Line: []model.Line{
+						{
+							Start: gg.P(int64(18800)),
+							Value: "We're no strangers to love",
+						},
+						{
+							Start: gg.P(int64(22801)),
+							Value: "You know the rules and so do I",
+						},
+					},
+					Offset: gg.P(int64(-100)),
+					Synced: true,
+				},
+			}))
+		})
+
+		It("should return unsynchronized lyrics from a file", func() {
+			mf := model.MediaFile{Path: "tests/fixtures/test.mp3"}
+			lyrics, err := fromExternalFile(ctx, &mf, ".txt")
+
+			Expect(err).To(BeNil())
+			Expect(lyrics).To(Equal(model.LyricList{
+				model.Lyrics{
+					Lang: "xxx",
+					Line: []model.Line{
+						{
+							Value: "We're no strangers to love",
+						},
+						{
+							Value: "You know the rules and so do I",
+						},
+					},
+					Synced: false,
+				},
+			}))
+		})
+	})
+})
diff --git a/model/lyrics.go b/model/lyrics.go
index 19ec71d3b..f75f3b11b 100644
--- a/model/lyrics.go
+++ b/model/lyrics.go
@@ -32,7 +32,7 @@ var (
 	// Should either be at the beginning of file, or beginning of line
 	syncRegex  = regexp.MustCompile(`(^|\n)\s*` + timeRegexString)
 	timeRegex  = regexp.MustCompile(timeRegexString)
-	lrcIdRegex = regexp.MustCompile(`\[(ar|ti|offset):([^]]+)]`)
+	lrcIdRegex = regexp.MustCompile(`\[(ar|ti|offset|lang):([^]]+)]`)
 )
 
 func (l Lyrics) IsEmpty() bool {
@@ -72,6 +72,8 @@ func ToLyrics(language, text string) (*Lyrics, error) {
 				switch idTag[1] {
 				case "ar":
 					artist = str.SanitizeText(strings.TrimSpace(idTag[2]))
+				case "lang":
+					language = str.SanitizeText(strings.TrimSpace(idTag[2]))
 				case "offset":
 					{
 						off, err := strconv.ParseInt(strings.TrimSpace(idTag[2]), 10, 64)
diff --git a/model/lyrics_test.go b/model/lyrics_test.go
index 54352f77c..382976872 100644
--- a/model/lyrics_test.go
+++ b/model/lyrics_test.go
@@ -9,8 +9,9 @@ import (
 var _ = Describe("ToLyrics", func() {
 	It("should parse tags with spaces", func() {
 		num := int64(1551)
-		lyrics, err := ToLyrics("xxx", "[offset: 1551 ]\n[ti: A title ]\n[ar: An artist ]\n[00:00.00]Hi there")
+		lyrics, err := ToLyrics("xxx", "[lang:  eng  ]\n[offset: 1551 ]\n[ti: A title ]\n[ar: An artist ]\n[00:00.00]Hi there")
 		Expect(err).ToNot(HaveOccurred())
+		Expect(lyrics.Lang).To(Equal("eng"))
 		Expect(lyrics.Synced).To(BeTrue())
 		Expect(lyrics.DisplayArtist).To(Equal("An artist"))
 		Expect(lyrics.DisplayTitle).To(Equal("A title"))
diff --git a/server/subsonic/api_suite_test.go b/server/subsonic/api_suite_test.go
index 4d5b293e5..a83f2f0eb 100644
--- a/server/subsonic/api_suite_test.go
+++ b/server/subsonic/api_suite_test.go
@@ -4,11 +4,13 @@ import (
 	"testing"
 
 	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/tests"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
 func TestSubsonicApi(t *testing.T) {
+	tests.Init(t, false)
 	log.SetLevel(log.LevelFatal)
 	RegisterFailHandler(Fail)
 	RunSpecs(t, "Subsonic API Suite")
diff --git a/server/subsonic/filter/filters.go b/server/subsonic/filter/filters.go
index f8b42d312..ab285e154 100644
--- a/server/subsonic/filter/filters.go
+++ b/server/subsonic/filter/filters.go
@@ -108,12 +108,12 @@ func SongsByRandom(genre string, fromYear, toYear int) Options {
 	return addDefaultFilters(options)
 }
 
-func SongWithLyrics(artist, title string) Options {
+func SongWithArtistTitle(artist, title string) Options {
 	return addDefaultFilters(Options{
 		Sort:    "updated_at",
 		Order:   "desc",
 		Max:     1,
-		Filters: And{Eq{"artist": artist, "title": title}, NotEq{"lyrics": ""}},
+		Filters: And{Eq{"artist": artist, "title": title}},
 	})
 }
 
diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go
index b960c71db..35a3fd3d3 100644
--- a/server/subsonic/media_retrieval.go
+++ b/server/subsonic/media_retrieval.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/consts"
+	"github.com/navidrome/navidrome/core/lyrics"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/resources"
@@ -95,9 +96,9 @@ func (api *Router) GetLyrics(r *http.Request) (*responses.Subsonic, error) {
 	artist, _ := p.String("artist")
 	title, _ := p.String("title")
 	response := newResponse()
-	lyrics := responses.Lyrics{}
-	response.Lyrics = &lyrics
-	mediaFiles, err := api.ds.MediaFile(r.Context()).GetAll(filter.SongWithLyrics(artist, title))
+	lyricsResponse := responses.Lyrics{}
+	response.Lyrics = &lyricsResponse
+	mediaFiles, err := api.ds.MediaFile(r.Context()).GetAll(filter.SongWithArtistTitle(artist, title))
 
 	if err != nil {
 		return nil, err
@@ -107,7 +108,7 @@ func (api *Router) GetLyrics(r *http.Request) (*responses.Subsonic, error) {
 		return response, nil
 	}
 
-	structuredLyrics, err := mediaFiles[0].StructuredLyrics()
+	structuredLyrics, err := lyrics.GetLyrics(r.Context(), &mediaFiles[0])
 	if err != nil {
 		return nil, err
 	}
@@ -116,15 +117,15 @@ func (api *Router) GetLyrics(r *http.Request) (*responses.Subsonic, error) {
 		return response, nil
 	}
 
-	lyrics.Artist = artist
-	lyrics.Title = title
+	lyricsResponse.Artist = artist
+	lyricsResponse.Title = title
 
 	lyricsText := ""
 	for _, line := range structuredLyrics[0].Line {
 		lyricsText += line.Value + "\n"
 	}
 
-	lyrics.Value = lyricsText
+	lyricsResponse.Value = lyricsText
 
 	return response, nil
 }
@@ -140,13 +141,13 @@ func (api *Router) GetLyricsBySongId(r *http.Request) (*responses.Subsonic, erro
 		return nil, err
 	}
 
-	lyrics, err := mediaFile.StructuredLyrics()
+	structuredLyrics, err := lyrics.GetLyrics(r.Context(), mediaFile)
 	if err != nil {
 		return nil, err
 	}
 
 	response := newResponse()
-	response.LyricsList = buildLyricsList(mediaFile, lyrics)
+	response.LyricsList = buildLyricsList(mediaFile, structuredLyrics)
 
 	return response, nil
 }
diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go
index 1aaf3727d..a0e9754ce 100644
--- a/server/subsonic/media_retrieval_test.go
+++ b/server/subsonic/media_retrieval_test.go
@@ -9,6 +9,8 @@ import (
 	"net/http/httptest"
 	"time"
 
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/conf/configtest"
 	"github.com/navidrome/navidrome/core/artwork"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
@@ -32,6 +34,8 @@ var _ = Describe("MediaRetrievalController", func() {
 		artwork = &fakeArtwork{}
 		router = New(ds, artwork, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil)
 		w = httptest.NewRecorder()
+		DeferCleanup(configtest.SetupConfig())
+		conf.Server.LyricsPriority = "embedded,.lrc"
 	})
 
 	Describe("GetCoverArt", func() {
@@ -109,6 +113,22 @@ var _ = Describe("MediaRetrievalController", func() {
 			Expect(response.Lyrics.Title).To(Equal(""))
 			Expect(response.Lyrics.Value).To(Equal(""))
 		})
+		It("should return lyric file when finding mediafile with no embedded lyrics but present on filesystem", func() {
+			r := newGetRequest("artist=Rick+Astley", "title=Never+Gonna+Give+You+Up")
+			mockRepo.SetData(model.MediaFiles{
+				{
+					Path:   "tests/fixtures/test.mp3",
+					ID:     "1",
+					Artist: "Rick Astley",
+					Title:  "Never Gonna Give You Up",
+				},
+			})
+			response, err := router.GetLyrics(r)
+			Expect(err).To(BeNil())
+			Expect(response.Lyrics.Artist).To(Equal("Rick Astley"))
+			Expect(response.Lyrics.Title).To(Equal("Never Gonna Give You Up"))
+			Expect(response.Lyrics.Value).To(Equal("We're no strangers to love\nYou know the rules and so do I\n"))
+		})
 	})
 
 	Describe("getLyricsBySongId", func() {
diff --git a/tests/fixtures/test.lrc b/tests/fixtures/test.lrc
new file mode 100644
index 000000000..d1730dfe9
--- /dev/null
+++ b/tests/fixtures/test.lrc
@@ -0,0 +1,6 @@
+[ar:Rick Astley]
+[ti:That one song]
+[offset:-100]
+[lang:eng]
+[00:18.80]We're no strangers to love
+[00:22.801]You know the rules and so do I
diff --git a/tests/fixtures/test.txt b/tests/fixtures/test.txt
new file mode 100644
index 000000000..c5a9c852c
--- /dev/null
+++ b/tests/fixtures/test.txt
@@ -0,0 +1,2 @@
+We're no strangers to love
+You know the rules and so do I
\ No newline at end of file