diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index e7b961cf0..cbd903e3c 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -68,18 +68,11 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) insights := metrics.GetInstance(dataStore) - fileCache := artwork.GetImageCache() - fFmpeg := ffmpeg.New() broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) - matcherMatcher := matcher.New(dataStore) - provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) - cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) + manager := plugins.GetManager(dataStore, broker, metricsMetrics) library := core.NewLibrary(dataStore, modelScanner, watcher, broker, manager) user := core.NewUser(dataStore, manager) maintenance := core.NewMaintenance(dataStore) @@ -105,11 +98,9 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { agentsAgents := agents.GetAgents(dataStore, manager) matcherMatcher := matcher.New(dataStore) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) - cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) playbackServer := playback.GetInstance(dataStore) lyricsLyrics := lyrics.NewLyrics(dataStore, manager) @@ -191,38 +182,22 @@ func CreatePrometheus() metrics.Metrics { func CreateScanner(ctx context.Context) model.Scanner { sqlDB := db.Db() dataStore := persistence.New(sqlDB) - fileCache := artwork.GetImageCache() - fFmpeg := ffmpeg.New() broker := events.GetBroker() - metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) - matcherMatcher := matcher.New(dataStore) - provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) - cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + metricsMetrics := metrics.GetPrometheusInstance(dataStore) + modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) return modelScanner } func CreateScanWatcher(ctx context.Context) scanner.Watcher { sqlDB := db.Db() dataStore := persistence.New(sqlDB) - fileCache := artwork.GetImageCache() - fFmpeg := ffmpeg.New() broker := events.GetBroker() - metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) - matcherMatcher := matcher.New(dataStore) - provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) - cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) - modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlistsPlaylists, metricsMetrics) + metricsMetrics := metrics.GetPrometheusInstance(dataStore) + modelScanner := scanner.New(ctx, dataStore, broker, playlistsPlaylists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) return watcher } diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go deleted file mode 100644 index b8c395c12..000000000 --- a/core/artwork/artwork.go +++ /dev/null @@ -1,134 +0,0 @@ -package artwork - -import ( - "context" - "errors" - _ "image/gif" - "io" - "time" - - "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/external" - "github.com/navidrome/navidrome/core/ffmpeg" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/resources" - "github.com/navidrome/navidrome/utils/cache" - _ "golang.org/x/image/webp" -) - -var ErrUnavailable = errors.New("artwork unavailable") - -type Artwork interface { - Get(ctx context.Context, artID model.ArtworkID, size int, square bool) (io.ReadCloser, time.Time, error) - GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (io.ReadCloser, time.Time, error) -} - -func NewArtwork(ds model.DataStore, cache cache.FileCache, ffmpeg ffmpeg.FFmpeg, provider external.Provider) Artwork { - return &artwork{ds: ds, cache: cache, ffmpeg: ffmpeg, provider: provider} -} - -type artwork struct { - ds model.DataStore - cache cache.FileCache - ffmpeg ffmpeg.FFmpeg - provider external.Provider -} - -type artworkReader interface { - cache.Item - LastUpdated() time.Time - Reader(ctx context.Context) (io.ReadCloser, string, error) -} - -func (a *artwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (reader io.ReadCloser, lastUpdate time.Time, err error) { - artID, err := a.getArtworkId(ctx, id) - if err == nil { - reader, lastUpdate, err = a.Get(ctx, artID, size, square) - } - if errors.Is(err, ErrUnavailable) { - if artID.Kind == model.KindArtistArtwork { - reader, _ = resources.FS().Open(consts.PlaceholderArtistArt) - } else { - reader, _ = resources.FS().Open(consts.PlaceholderAlbumArt) - } - return reader, consts.ServerStart, nil - } - return reader, lastUpdate, err -} - -func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, square bool) (reader io.ReadCloser, lastUpdate time.Time, err error) { - artReader, err := a.getArtworkReader(ctx, artID, size, square) - if err != nil { - return nil, time.Time{}, err - } - - r, err := a.cache.Get(ctx, artReader) - if err != nil { - if !errors.Is(err, context.Canceled) && !errors.Is(err, ErrUnavailable) { - log.Error(ctx, "Error accessing image cache", "id", artID, "size", size, err) - } - return nil, time.Time{}, err - } - return r, artReader.LastUpdated(), nil -} - -type coverArtGetter interface { - CoverArtID() model.ArtworkID -} - -func (a *artwork) getArtworkId(ctx context.Context, id string) (model.ArtworkID, error) { - if id == "" { - return model.ArtworkID{}, ErrUnavailable - } - artID, err := model.ParseArtworkID(id) - if err == nil { - return artID, nil - } - - log.Trace(ctx, "ArtworkID invalid. Trying to figure out kind based on the ID", "id", id) - entity, err := model.GetEntityByID(ctx, a.ds, id) - if err != nil { - return model.ArtworkID{}, err - } - if e, ok := entity.(coverArtGetter); ok { - artID = e.CoverArtID() - } - switch e := entity.(type) { - case *model.Artist: - log.Trace(ctx, "ID is for an Artist", "id", id, "name", e.Name, "artist", e.Name) - case *model.Album: - log.Trace(ctx, "ID is for an Album", "id", id, "name", e.Name, "artist", e.AlbumArtist) - case *model.MediaFile: - log.Trace(ctx, "ID is for a MediaFile", "id", id, "title", e.Title, "album", e.Album) - case *model.Playlist: - log.Trace(ctx, "ID is for a Playlist", "id", id, "name", e.Name) - } - return artID, nil -} - -func (a *artwork) getArtworkReader(ctx context.Context, artID model.ArtworkID, size int, square bool) (artworkReader, error) { - var artReader artworkReader - var err error - if size > 0 || square { - artReader, err = resizedFromOriginal(ctx, a, artID, size, square) - } else { - switch artID.Kind { - case model.KindArtistArtwork: - artReader, err = newArtistArtworkReader(ctx, a, artID, a.provider) - case model.KindAlbumArtwork: - artReader, err = newAlbumArtworkReader(ctx, a, artID, a.provider) - case model.KindMediaFileArtwork: - artReader, err = newMediafileArtworkReader(ctx, a, artID) - case model.KindPlaylistArtwork: - artReader, err = newPlaylistArtworkReader(ctx, a, artID) - case model.KindDiscArtwork: - artReader, err = newDiscArtworkReader(ctx, a, artID) - case model.KindRadioArtwork: - artReader, err = newRadioArtworkReader(ctx, a, artID) - default: - return nil, ErrUnavailable - } - } - return artReader, err -} diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go deleted file mode 100644 index f5312361d..000000000 --- a/core/artwork/artwork_internal_test.go +++ /dev/null @@ -1,628 +0,0 @@ -package artwork - -import ( - "context" - "errors" - "image" - "image/jpeg" - "image/png" - "io" - "os" - "path/filepath" - "time" - - _ "github.com/gen2brain/webp" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Artwork", func() { - var aw *artwork - var ds model.DataStore - var ffmpeg *tests.MockFFmpeg - var folderRepo *fakeFolderRepo - ctx := log.NewContext(context.TODO()) - var alOnlyEmbed, alEmbedNotFound, alOnlyExternal, alExternalNotFound, alMultipleCovers, alSingleDisc model.Album - var arMultipleCovers model.Artist - var mfWithEmbed, mfAnotherWithEmbed, mfWithoutEmbed, mfCorruptedCover model.MediaFile - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.ImageCacheSize = "0" // Disable cache - conf.Server.CoverArtPriority = "folder.*, cover.*, embedded , front.*" - - folderRepo = &fakeFolderRepo{} - libRepo := &tests.MockLibraryRepo{} - repoRoot, _ := os.Getwd() - libRepo.SetData(model.Libraries{{ID: 0, Path: testFileLibPath(repoRoot)}}) - ds = &tests.MockDataStore{ - MockedTranscoding: &tests.MockTranscodingRepo{}, - MockedFolder: folderRepo, - MockedLibrary: libRepo, - } - // Paths use forward slashes because the scanner stores fs.FS-relative paths in the DB. - alOnlyEmbed = model.Album{ID: "222", Name: "Only embed", EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3", FolderIDs: []string{"f1"}} - alEmbedNotFound = model.Album{ID: "333", Name: "Embed not found", EmbedArtPath: "tests/fixtures/NON_EXISTENT.mp3", FolderIDs: []string{"f1"}} - alOnlyExternal = model.Album{ID: "444", Name: "Only external", FolderIDs: []string{"f1"}, Discs: model.Discs{1: "", 2: ""}} - alExternalNotFound = model.Album{ID: "555", Name: "External not found", FolderIDs: []string{"f2"}} - alSingleDisc = model.Album{ID: "888", Name: "Single disc", FolderIDs: []string{"f1"}, Discs: model.Discs{1: ""}} - arMultipleCovers = model.Artist{ID: "777", Name: "All options"} - alMultipleCovers = model.Album{ - ID: "666", - Name: "All options", - EmbedArtPath: "tests/fixtures/artist/an-album/test.mp3", - FolderIDs: []string{"f1"}, - AlbumArtistID: "777", - } - mfWithEmbed = model.MediaFile{ID: "22", Path: "tests/fixtures/test.mp3", HasCoverArt: true, AlbumID: "222"} - mfAnotherWithEmbed = model.MediaFile{ID: "23", Path: "tests/fixtures/artist/an-album/test.mp3", HasCoverArt: true, AlbumID: "666"} - mfWithoutEmbed = model.MediaFile{ID: "44", Path: "tests/fixtures/test.ogg", AlbumID: "444"} - mfCorruptedCover = model.MediaFile{ID: "45", Path: "tests/fixtures/test.ogg", HasCoverArt: true, AlbumID: "444"} - - cache := GetImageCache() - ffmpeg = tests.NewMockFFmpeg("content from ffmpeg") - aw = NewArtwork(ds, cache, ffmpeg, nil).(*artwork) - }) - - Describe("albumArtworkReader", func() { - Context("ID not found", func() { - It("returns ErrNotFound if album is not in the DB", func() { - _, err := newAlbumArtworkReader(ctx, aw, model.MustParseArtworkID("al-NOT-FOUND"), nil) - Expect(err).To(MatchError(model.ErrNotFound)) - }) - }) - Context("Embed images", func() { - BeforeEach(func() { - folderRepo.result = nil - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ - alOnlyEmbed, - alEmbedNotFound, - }) - }) - It("returns embed cover", func() { - aw, err := newAlbumArtworkReader(ctx, aw, alOnlyEmbed.CoverArtID(), nil) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("tests/fixtures/artist/an-album/test.mp3")) - }) - It("returns ErrUnavailable if embed path is not available", func() { - ffmpeg.Error = errors.New("not available") - aw, err := newAlbumArtworkReader(ctx, aw, alEmbedNotFound.CoverArtID(), nil) - Expect(err).ToNot(HaveOccurred()) - _, _, err = aw.Reader(ctx) - Expect(err).To(MatchError(ErrUnavailable)) - }) - }) - Context("External images", func() { - BeforeEach(func() { - folderRepo.result = []model.Folder{} - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ - alOnlyExternal, - alExternalNotFound, - }) - }) - It("returns external cover", func() { - folderRepo.result = []model.Folder{{ - Path: "tests/fixtures/artist/an-album", - ImageFiles: []string{"front.png"}, - }} - aw, err := newAlbumArtworkReader(ctx, aw, alOnlyExternal.CoverArtID(), nil) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("tests/fixtures/artist/an-album/front.png")) - }) - It("returns ErrUnavailable if external file is not available", func() { - folderRepo.result = []model.Folder{} - aw, err := newAlbumArtworkReader(ctx, aw, alExternalNotFound.CoverArtID(), nil) - Expect(err).ToNot(HaveOccurred()) - _, _, err = aw.Reader(ctx) - Expect(err).To(MatchError(ErrUnavailable)) - }) - }) - Context("Multiple covers", func() { - BeforeEach(func() { - folderRepo.result = []model.Folder{{ - Path: "tests/fixtures/artist/an-album", - ImageFiles: []string{"cover.jpg", "front.png", "artist.png"}, - }} - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ - alMultipleCovers, - }) - }) - DescribeTable("CoverArtPriority", - func(priority string, expected string) { - conf.Server.CoverArtPriority = priority - aw, err := newAlbumArtworkReader(ctx, aw, alMultipleCovers.CoverArtID(), nil) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal(expected)) - }, - Entry(nil, " folder.* , cover.*,embedded,front.*", "tests/fixtures/artist/an-album/cover.jpg"), - Entry(nil, "front.* , cover.*, embedded ,folder.*", "tests/fixtures/artist/an-album/front.png"), - Entry(nil, " embedded , front.* , cover.*,folder.*", "tests/fixtures/artist/an-album/test.mp3"), - ) - }) - Context("LastUpdated", func() { - // Regression test for #5377: LastUpdated feeds the HTTP Last-Modified header. - // It must return max(album.UpdatedAt, ImagesUpdatedAt) so browsers revalidate - // cached cover art when only the image file changes. - now := time.Now().Truncate(time.Second) - DescribeTable("returns the max of album.UpdatedAt and ImagesUpdatedAt", - func(albumUpdatedAt, imagesUpdatedAt, expected time.Time) { - album := model.Album{ID: "al1", UpdatedAt: albumUpdatedAt} - folderRepo.result = []model.Folder{{ImagesUpdatedAt: imagesUpdatedAt}} - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{album}) - - ar, err := newAlbumArtworkReader(ctx, aw, album.CoverArtID(), nil) - Expect(err).ToNot(HaveOccurred()) - Expect(ar.LastUpdated()).To(Equal(expected)) - }, - Entry("album newer than images", now, now.Add(-1*time.Hour), now), - Entry("images newer than album", now.Add(-24*time.Hour), now.Add(-1*time.Hour), now.Add(-1*time.Hour)), - Entry("equal timestamps", now, now, now), - ) - }) - }) - Describe("discArtworkReader", func() { - Context("LastUpdated", func() { - // Regression test for #5377: same bug as albumArtworkReader — disc covers - // must also revalidate when the image file changes, not only when media files do. - now := time.Now().Truncate(time.Second) - DescribeTable("returns the max of album.UpdatedAt and ImagesUpdatedAt", - func(albumUpdatedAt, imagesUpdatedAt, expected time.Time) { - album := model.Album{ID: "al1", UpdatedAt: albumUpdatedAt} - folderRepo.result = []model.Folder{{ImagesUpdatedAt: imagesUpdatedAt}} - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{album}) - ds.MediaFile(ctx).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ - {ID: "mf1", AlbumID: "al1", DiscNumber: 1, Path: "tests/fixtures/test.mp3"}, - }) - - artID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID("al1", 1), nil) - dr, err := newDiscArtworkReader(ctx, aw, artID) - Expect(err).ToNot(HaveOccurred()) - Expect(dr.LastUpdated()).To(Equal(expected)) - }, - Entry("album newer than images", now, now.Add(-1*time.Hour), now), - Entry("images newer than album", now.Add(-24*time.Hour), now.Add(-1*time.Hour), now.Add(-1*time.Hour)), - Entry("equal timestamps", now, now, now), - ) - }) - }) - Describe("artistArtworkReader", func() { - Context("Multiple covers", func() { - BeforeEach(func() { - repoRoot, err := os.Getwd() - Expect(err).ToNot(HaveOccurred()) - folderRepo.result = []model.Folder{{ - LibraryPath: testFileLibPath(repoRoot), - Path: "tests/fixtures/artist/an-album", - ImageFiles: []string{"artist.png"}, - }} - ds.Artist(ctx).(*tests.MockArtistRepo).SetData(model.Artists{ - arMultipleCovers, - }) - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ - alMultipleCovers, - }) - ds.MediaFile(ctx).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ - mfAnotherWithEmbed, - }) - }) - DescribeTable("ArtistArtPriority", - func(priority string, expected string) { - conf.Server.ArtistArtPriority = priority - aw, err := newArtistArtworkReader(ctx, aw, arMultipleCovers.CoverArtID(), nil) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(filepath.ToSlash(path)).To(HaveSuffix(expected)) - }, - Entry(nil, " folder.* , artist.*,album/artist.*", "tests/fixtures/artist/artist.jpg"), - Entry(nil, "album/artist.*, folder.*,artist.*", "tests/fixtures/artist/an-album/artist.png"), - ) - }) - }) - Describe("mediafileArtworkReader", func() { - Context("ID not found", func() { - It("returns ErrNotFound if mediafile is not in the DB", func() { - _, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-NOT-FOUND")) - Expect(err).To(MatchError(model.ErrNotFound)) - }) - }) - Context("Embed images", func() { - BeforeEach(func() { - folderRepo.result = []model.Folder{{ - Path: "tests/fixtures/artist/an-album", - ImageFiles: []string{"front.png"}, - }} - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ - alOnlyEmbed, - alOnlyExternal, - alSingleDisc, - }) - ds.MediaFile(ctx).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ - mfWithEmbed, - mfWithoutEmbed, - mfCorruptedCover, - }) - }) - It("returns embed cover", func() { - aw, err := newMediafileArtworkReader(ctx, aw, mfWithEmbed.CoverArtID()) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("tests/fixtures/test.mp3")) - }) - It("returns embed cover if successfully extracted by ffmpeg", func() { - aw, err := newMediafileArtworkReader(ctx, aw, mfCorruptedCover.CoverArtID()) - Expect(err).ToNot(HaveOccurred()) - r, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - data, _ := io.ReadAll(r) - Expect(data).ToNot(BeEmpty()) - Expect(path).To(Equal("tests/fixtures/test.ogg")) - }) - It("returns album cover if cannot read embed artwork", func() { - // Force fromTag to fail - mfCorruptedCover.Path = "tests/fixtures/DOES_NOT_EXIST.ogg" - Expect(ds.MediaFile(ctx).(*tests.MockMediaFileRepo).Put(&mfCorruptedCover)).To(Succeed()) - // Simulate ffmpeg error - ffmpeg.Error = errors.New("not available") - - aw, err := newMediafileArtworkReader(ctx, aw, mfCorruptedCover.CoverArtID()) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("al-444")) - }) - It("returns album cover if media file has no cover art", func() { - aw, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-"+mfWithoutEmbed.ID)) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("al-444")) - }) - It("falls back to disc cover art when media file has a disc number on a multi-disc album", func() { - mfWithDisc := model.MediaFile{ID: "46", Path: "tests/fixtures/test.ogg", AlbumID: "444", DiscNumber: 2} - Expect(ds.MediaFile(ctx).(*tests.MockMediaFileRepo).Put(&mfWithDisc)).To(Succeed()) - - aw, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-"+mfWithDisc.ID)) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - // Should fall back to disc art, which itself falls back to album art - Expect(path).To(Equal("dc-444:2")) - }) - It("falls back to album cover art for single-disc albums even with a disc number", func() { - mfOnSingleDisc := model.MediaFile{ID: "47", Path: "tests/fixtures/test.ogg", AlbumID: "888", DiscNumber: 1} - Expect(ds.MediaFile(ctx).(*tests.MockMediaFileRepo).Put(&mfOnSingleDisc)).To(Succeed()) - - aw, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-"+mfOnSingleDisc.ID)) - Expect(err).ToNot(HaveOccurred()) - _, path, err := aw.Reader(ctx) - Expect(err).ToNot(HaveOccurred()) - // Single-disc album should skip disc art and go straight to album art - Expect(path).To(Equal("al-888")) - }) - }) - }) - Describe("playlistArtworkReader", func() { - Describe("findPlaylistSidecarPath", func() { - It("discovers sidecar image next to playlist file", func() { - tmpDir := GinkgoT().TempDir() - plsPath := filepath.Join(tmpDir, "MyPlaylist.m3u") - imgPath := filepath.Join(tmpDir, "MyPlaylist.jpg") - Expect(os.WriteFile(plsPath, []byte("#EXTM3U\n"), 0600)).To(Succeed()) - Expect(os.WriteFile(imgPath, []byte("fake image"), 0600)).To(Succeed()) - - result := findPlaylistSidecarPath(GinkgoT().Context(), plsPath) - Expect(result).To(Equal(imgPath)) - }) - - It("returns empty string when no sidecar image exists", func() { - tmpDir := GinkgoT().TempDir() - plsPath := filepath.Join(tmpDir, "MyPlaylist.m3u") - Expect(os.WriteFile(plsPath, []byte("#EXTM3U\n"), 0600)).To(Succeed()) - - result := findPlaylistSidecarPath(GinkgoT().Context(), plsPath) - Expect(result).To(BeEmpty()) - }) - - It("returns empty string when playlist has no path", func() { - result := findPlaylistSidecarPath(GinkgoT().Context(), "") - Expect(result).To(BeEmpty()) - }) - - It("finds sidecar with different case base name", func() { - tmpDir := GinkgoT().TempDir() - plsPath := filepath.Join(tmpDir, "myplaylist.m3u") - imgPath := filepath.Join(tmpDir, "MyPlaylist.jpg") - Expect(os.WriteFile(plsPath, []byte("#EXTM3U\n"), 0600)).To(Succeed()) - Expect(os.WriteFile(imgPath, []byte("fake image"), 0600)).To(Succeed()) - - result := findPlaylistSidecarPath(GinkgoT().Context(), plsPath) - Expect(result).To(Equal(imgPath)) - }) - }) - - Describe("fromPlaylistExternalImage", func() { - It("opens local path from ExternalImageURL", func() { - tmpDir := GinkgoT().TempDir() - imgPath := filepath.Join(tmpDir, "cover.jpg") - Expect(os.WriteFile(imgPath, []byte("external image data"), 0600)).To(Succeed()) - - reader := &playlistArtworkReader{ - pl: model.Playlist{ExternalImageURL: imgPath}, - } - r, path, err := reader.fromPlaylistExternalImage(ctx)() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - data, _ := io.ReadAll(r) - Expect(string(data)).To(Equal("external image data")) - r.Close() - }) - - It("returns nil when ExternalImageURL is empty", func() { - reader := &playlistArtworkReader{ - pl: model.Playlist{ExternalImageURL: ""}, - } - r, path, err := reader.fromPlaylistExternalImage(ctx)() - Expect(err).ToNot(HaveOccurred()) - Expect(r).To(BeNil()) - Expect(path).To(BeEmpty()) - }) - - It("returns error when local file does not exist", func() { - reader := &playlistArtworkReader{ - pl: model.Playlist{ExternalImageURL: "/non/existent/path/cover.jpg"}, - } - r, _, err := reader.fromPlaylistExternalImage(ctx)() - Expect(err).To(HaveOccurred()) - Expect(r).To(BeNil()) - }) - - It("skips HTTP URL when EnableM3UExternalAlbumArt is false", func() { - conf.Server.EnableM3UExternalAlbumArt = false - - reader := &playlistArtworkReader{ - pl: model.Playlist{ExternalImageURL: "https://example.com/cover.jpg"}, - } - r, path, err := reader.fromPlaylistExternalImage(ctx)() - Expect(err).ToNot(HaveOccurred()) - Expect(r).To(BeNil()) - Expect(path).To(BeEmpty()) - }) - - It("still opens local path when EnableM3UExternalAlbumArt is false", func() { - conf.Server.EnableM3UExternalAlbumArt = false - - tmpDir := GinkgoT().TempDir() - imgPath := filepath.Join(tmpDir, "cover.jpg") - Expect(os.WriteFile(imgPath, []byte("local image"), 0600)).To(Succeed()) - - reader := &playlistArtworkReader{ - pl: model.Playlist{ExternalImageURL: imgPath}, - } - r, path, err := reader.fromPlaylistExternalImage(ctx)() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - r.Close() - }) - }) - }) - - Describe("resizedArtworkReader", func() { - BeforeEach(func() { - folderRepo.result = []model.Folder{{ - Path: "tests/fixtures/artist/an-album", - ImageFiles: []string{"cover.jpg", "front.png"}, - }} - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ - alMultipleCovers, - }) - }) - When("Square is false", func() { - It("returns PNG if original image is a PNG", func() { - conf.Server.CoverArtPriority = "front.png" - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 15, false) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - Expect(format).To(Equal("png")) - Expect(img.Bounds().Size().X).To(Equal(15)) - Expect(img.Bounds().Size().Y).To(Equal(15)) - }) - It("returns JPEG if original image is not a PNG", func() { - conf.Server.CoverArtPriority = "cover.jpg" - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 200, false) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(format).To(Equal("jpeg")) - Expect(err).ToNot(HaveOccurred()) - Expect(img.Bounds().Size().X).To(Equal(200)) - Expect(img.Bounds().Size().Y).To(Equal(200)) - }) - }) - When("When square is true", func() { - var alCover model.Album - - DescribeTable("resize", - func(srcFormat string, expectedFormat string, landscape bool, size int) { - coverFileName := "cover." + srcFormat - dirName := createImage(srcFormat, landscape, size) - alCover = model.Album{ - ID: "444", - Name: "Only external", - FolderIDs: []string{"tmp"}, - } - folderRepo.result = []model.Folder{{ImageFiles: []string{coverFileName}}} - rootLibRepo := &tests.MockLibraryRepo{} - rootLibRepo.SetData(model.Libraries{{ID: 0, Path: testFileLibPath(dirName)}}) - ds.(*tests.MockDataStore).MockedLibrary = rootLibRepo - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{ - alCover, - }) - - conf.Server.CoverArtPriority = coverFileName - r, _, err := aw.Get(context.Background(), alCover.CoverArtID(), size, true) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - Expect(format).To(Equal(expectedFormat)) - Expect(img.Bounds().Size().X).To(Equal(size)) - Expect(img.Bounds().Size().Y).To(Equal(size)) - }, - Entry("portrait png image", "png", "png", false, 200), - Entry("landscape png image", "png", "png", true, 200), - Entry("portrait jpg image", "jpg", "png", false, 200), - Entry("landscape jpg image", "jpg", "png", true, 200), - ) - }) - When("EnableWebPEncoding is true and square is false", func() { - BeforeEach(func() { - conf.Server.EnableWebPEncoding = true - }) - It("returns WebP even if original image is a PNG", func() { - conf.Server.CoverArtPriority = "front.png" - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 15, false) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - Expect(format).To(Equal("webp")) - Expect(img.Bounds().Size().X).To(Equal(15)) - Expect(img.Bounds().Size().Y).To(Equal(15)) - }) - It("returns WebP if original image is not a PNG", func() { - conf.Server.CoverArtPriority = "cover.jpg" - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 200, false) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(format).To(Equal("webp")) - Expect(err).ToNot(HaveOccurred()) - Expect(img.Bounds().Size().X).To(Equal(200)) - Expect(img.Bounds().Size().Y).To(Equal(200)) - }) - }) - When("EnableWebPEncoding is false and square is false", func() { - BeforeEach(func() { - conf.Server.EnableWebPEncoding = false - }) - It("returns PNG if original image is a PNG", func() { - conf.Server.CoverArtPriority = "front.png" - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 15, false) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - Expect(format).To(Equal("png")) - Expect(img.Bounds().Size().X).To(Equal(15)) - Expect(img.Bounds().Size().Y).To(Equal(15)) - }) - It("returns JPEG if original image is a JPG", func() { - conf.Server.CoverArtPriority = "cover.jpg" - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 200, false) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - Expect(format).To(Equal("jpeg")) - Expect(img.Bounds().Size().X).To(Equal(200)) - Expect(img.Bounds().Size().Y).To(Equal(200)) - }) - }) - When("EnableWebPEncoding is false and square is true", func() { - var alCover model.Album - - BeforeEach(func() { - conf.Server.EnableWebPEncoding = false - }) - It("returns PNG for square mode", func() { - dirName := createImage("png", false, 200) - alCover = model.Album{ - ID: "444", - Name: "Only external", - FolderIDs: []string{"tmp"}, - } - folderRepo.result = []model.Folder{{ImageFiles: []string{"cover.png"}}} - rootLibRepo := &tests.MockLibraryRepo{} - rootLibRepo.SetData(model.Libraries{{ID: 0, Path: testFileLibPath(dirName)}}) - ds.(*tests.MockDataStore).MockedLibrary = rootLibRepo - ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{alCover}) - - conf.Server.CoverArtPriority = "cover.png" - r, _, err := aw.Get(context.Background(), alCover.CoverArtID(), 200, true) - Expect(err).ToNot(HaveOccurred()) - - img, format, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - Expect(format).To(Equal("png")) - Expect(img.Bounds().Size().X).To(Equal(200)) - Expect(img.Bounds().Size().Y).To(Equal(200)) - }) - }) - When("Requested size is larger than original", func() { - It("clamps size to original dimensions", func() { - conf.Server.CoverArtPriority = "front.png" - // front.png is 16x16, requesting 99999 should return at original size - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 99999, false) - Expect(err).ToNot(HaveOccurred()) - - img, _, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - // Should be clamped to original size (16), not 99999 - Expect(img.Bounds().Size().X).To(Equal(16)) - Expect(img.Bounds().Size().Y).To(Equal(16)) - }) - - It("clamps square size to original dimensions", func() { - conf.Server.CoverArtPriority = "front.png" - // front.png is 16x16, requesting 99999 with square should return 16x16 square - r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID(), 99999, true) - Expect(err).ToNot(HaveOccurred()) - - img, _, err := image.Decode(r) - Expect(err).ToNot(HaveOccurred()) - // Should be clamped to original size (16), not 99999 - Expect(img.Bounds().Size().X).To(Equal(16)) - Expect(img.Bounds().Size().Y).To(Equal(16)) - }) - }) - }) -}) - -func createImage(format string, landscape bool, size int) string { - var img image.Image - - if landscape { - img = image.NewRGBA(image.Rect(0, 0, size, size/2)) - } else { - img = image.NewRGBA(image.Rect(0, 0, size/2, size)) - } - - tmpDir := GinkgoT().TempDir() - f, _ := os.Create(filepath.Join(tmpDir, "cover."+format)) - defer f.Close() - switch format { - case "png": - _ = png.Encode(f, img) - case "jpg": - _ = jpeg.Encode(f, img, &jpeg.Options{Quality: 75}) - } - - return tmpDir -} diff --git a/core/artwork/artwork_suite_test.go b/core/artwork/artwork_suite_test.go index e2a1a591c..ffb7d1148 100644 --- a/core/artwork/artwork_suite_test.go +++ b/core/artwork/artwork_suite_test.go @@ -28,9 +28,6 @@ func TestArtwork(t *testing.T) { goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*recursiveTree).dispatch"), goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*nonrecursiveTree).dispatch"), goleak.IgnoreTopFunction("github.com/rjeczalik/notify.(*nonrecursiveTree).internal"), - // The old cache_warmer.go starts a goroutine per NewCacheWarmer call with - // no shutdown path (dark-launch target for Phase 2, not touched here). - goleak.IgnoreTopFunction("github.com/navidrome/navidrome/core/artwork.(*cacheWarmer).waitSignal"), ) tests.Init(t, false) @@ -40,7 +37,7 @@ func TestArtwork(t *testing.T) { } // osDirFS wraps os.DirFS as a storage.MusicFS for integration tests. -// ReadTags is not used by albumArtworkReader, so it is left as a stub. +// ReadTags is not exercised by these tests, so it is left as a stub. type osDirFS struct{ fs.FS } func (o osDirFS) ReadTags(...string) (map[string]metadata.Info, error) { return nil, nil } diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go deleted file mode 100644 index adddd0dc3..000000000 --- a/core/artwork/artwork_test.go +++ /dev/null @@ -1,57 +0,0 @@ -package artwork_test - -import ( - "context" - "io" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/artwork" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/resources" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Artwork", func() { - var aw artwork.Artwork - var ds model.DataStore - var ffmpeg *tests.MockFFmpeg - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.ImageCacheSize = "0" // Disable cache - cache := artwork.GetImageCache() - ffmpeg = tests.NewMockFFmpeg("content from ffmpeg") - aw = artwork.NewArtwork(ds, cache, ffmpeg, nil) - }) - - Context("GetOrPlaceholder", func() { - Context("Empty ID", func() { - It("returns placeholder if album is not in the DB", func() { - r, _, err := aw.GetOrPlaceholder(context.Background(), "", 0, false) - Expect(err).ToNot(HaveOccurred()) - - ph, err := resources.FS().Open(consts.PlaceholderAlbumArt) - Expect(err).ToNot(HaveOccurred()) - phBytes, err := io.ReadAll(ph) - Expect(err).ToNot(HaveOccurred()) - - result, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - - Expect(result).To(Equal(phBytes)) - }) - }) - }) - Context("Get", func() { - Context("Empty ID", func() { - It("returns an ErrUnavailable error", func() { - _, _, err := aw.Get(context.Background(), model.ArtworkID{}, 0, false) - Expect(err).To(MatchError(artwork.ErrUnavailable)) - }) - }) - }) -}) diff --git a/core/artwork/benchmark_e2e_test.go b/core/artwork/benchmark_e2e_test.go deleted file mode 100644 index bf3d435a8..000000000 --- a/core/artwork/benchmark_e2e_test.go +++ /dev/null @@ -1,189 +0,0 @@ -package artwork - -import ( - "context" - "fmt" - "image/jpeg" - "io" - "os" - "path/filepath" - "runtime" - "sync" - "testing" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/tests" - "github.com/navidrome/navidrome/utils/cache" -) - -// setupE2EBenchmark creates an artwork instance with a real album cover image on disk, -// backed by either a real file cache or disabled cache depending on cacheSize. -// Note: This benchmarks artwork.Get() directly (not the full HTTP handler), which covers -// the critical path (source selection, decode, resize, encode, cache). This is a deliberate -// spec deviation — the full HTTP round-trip benchmark requires significant infrastructure -// (DB, scanner, fake filesystem) and can be added later if HTTP overhead proves significant. -// -// Depends on fakeFolderRepo defined in reader_artist_test.go (same package, compiled together). -func setupE2EBenchmark(b *testing.B, cacheSize string) (Artwork, model.ArtworkID, func()) { - b.Helper() - cleanup := configtest.SetupConfig() - b.Cleanup(cleanup) - - tmpDir, err := os.MkdirTemp("", "artwork-bench-*") - if err != nil { - b.Fatal(err) - } - - // Create a realistic cover image on disk - coverPath := filepath.Join(tmpDir, "cover.jpg") - coverImg := generateGradientImage(1000, 1000) - f, err := os.Create(coverPath) - if err != nil { - b.Fatal(err) - } - if err := jpeg.Encode(f, coverImg, &jpeg.Options{Quality: 90}); err != nil { - f.Close() - b.Fatal(err) - } - f.Close() - - // Configure cache - conf.Server.ImageCacheSize = cacheSize - conf.Server.CacheFolder = conf.NewDir(tmpDir) - conf.Server.CoverArtQuality = 75 - conf.Server.CoverArtPriority = "cover.*" - - // Set up mock data store with album pointing to our cover. - // Set UpdatedAt so CoverArtID().LastUpdate is consistent across calls. - album := model.Album{ - ID: "bench-album-1", - Name: "Benchmark Album", - FolderIDs: []string{"f1"}, - UpdatedAt: time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC), - } - folderRepo := &fakeFolderRepo{ - result: []model.Folder{{ - Path: tmpDir, - ImageFiles: []string{"cover.jpg"}, - }}, - } - ds := &tests.MockDataStore{ - MockedTranscoding: &tests.MockTranscodingRepo{}, - MockedFolder: folderRepo, - } - ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{album}) - - artID := album.CoverArtID() - - imgCache := cache.NewFileCache("BenchImage", cacheSize, "bench-images", 0, - func(ctx context.Context, arg cache.Item) (io.Reader, error) { - r, _, err := arg.(artworkReader).Reader(ctx) - return r, err - }) - - // Wait for cache init if enabled - if cacheSize != "0" { - for !imgCache.Available(context.Background()) && !imgCache.Disabled(context.Background()) { - runtime.Gosched() // Yield to allow background init goroutine to run - } - } - - ffmpeg := tests.NewMockFFmpeg("fallback content") - aw := NewArtwork(ds, imgCache, ffmpeg, nil) - - cleanupAll := func() { - os.RemoveAll(tmpDir) - } - return aw, artID, cleanupAll -} - -func BenchmarkArtworkGetE2E(b *testing.B) { - cacheConfigs := []struct { - name string - cacheSize string - }{ - {"no_cache", "0"}, - {"with_cache", "100MB"}, - } - sizes := []int{0, 300} - - for _, cc := range cacheConfigs { - for _, size := range sizes { - b.Run(fmt.Sprintf("%s/size_%d", cc.name, size), func(b *testing.B) { - aw, artID, cleanup := setupE2EBenchmark(b, cc.cacheSize) - defer cleanup() - - // Warm the cache on first call if cache is enabled - if cc.cacheSize != "0" { - r, _, err := aw.Get(context.Background(), artID, size, size > 0) - if err != nil { - b.Fatal(err) - } - _, _ = io.ReadAll(r) - r.Close() - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - r, _, err := aw.Get(context.Background(), artID, size, size > 0) - if err != nil { - b.Fatal(err) - } - _, _ = io.ReadAll(r) - r.Close() - } - }) - } - } -} - -func BenchmarkArtworkGetE2EConcurrent(b *testing.B) { - cacheConfigs := []struct { - name string - cacheSize string - }{ - {"no_cache", "0"}, - {"with_cache", "100MB"}, - } - concurrencyLevels := []int{10, 50} - - for _, cc := range cacheConfigs { - for _, n := range concurrencyLevels { - b.Run(fmt.Sprintf("%s/goroutines_%d", cc.name, n), func(b *testing.B) { - aw, artID, cleanup := setupE2EBenchmark(b, cc.cacheSize) - defer cleanup() - - // Warm cache - if cc.cacheSize != "0" { - r, _, _ := aw.Get(context.Background(), artID, 300, true) - if r != nil { - _, _ = io.ReadAll(r) - r.Close() - } - } - - b.ResetTimer() - for i := 0; i < b.N; i++ { - var wg sync.WaitGroup - wg.Add(n) - for range n { - go func() { - defer wg.Done() - r, _, err := aw.Get(context.Background(), artID, 300, true) - if err != nil { - b.Error(err) - return - } - _, _ = io.ReadAll(r) - r.Close() - }() - } - wg.Wait() - } - }) - } - } -} diff --git a/core/artwork/cache_warmer.go b/core/artwork/cache_warmer.go deleted file mode 100644 index 5090d638e..000000000 --- a/core/artwork/cache_warmer.go +++ /dev/null @@ -1,162 +0,0 @@ -package artwork - -import ( - "context" - "fmt" - "io" - "maps" - "slices" - "sync" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" - "github.com/navidrome/navidrome/utils/cache" - "github.com/navidrome/navidrome/utils/pl" -) - -type CacheWarmer interface { - PreCache(artID model.ArtworkID) -} - -// NewCacheWarmer creates a new CacheWarmer instance. The CacheWarmer will pre-cache Artwork images in the background -// to speed up the response time when the image is requested by the UI. The cache is pre-populated with the original -// image size, as well as the size defined by the UICoverArtSize config option. -func NewCacheWarmer(artwork Artwork, cache cache.FileCache) CacheWarmer { - // If image cache is disabled, return a NOOP implementation - if conf.Server.ImageCacheSize == "0" || !conf.Server.EnableArtworkPrecache { - return &noopCacheWarmer{} - } - - // If the file cache is disabled, return a NOOP implementation - if cache.Disabled(context.Background()) { - log.Debug("Image cache disabled. Cache warmer will not run") - return &noopCacheWarmer{} - } - - a := &cacheWarmer{ - artwork: artwork, - cache: cache, - buffer: make(map[model.ArtworkID]struct{}), - wakeSignal: make(chan struct{}, 1), - coverArtSize: conf.Server.UICoverArtSize, - } - - // Create a context with a fake admin user, to be able to pre-cache Playlist CoverArts - ctx := request.WithUser(context.TODO(), model.User{IsAdmin: true}) - go a.run(ctx) - return a -} - -type cacheWarmer struct { - artwork Artwork - buffer map[model.ArtworkID]struct{} - mutex sync.Mutex - cache cache.FileCache - wakeSignal chan struct{} - coverArtSize int -} - -func (a *cacheWarmer) PreCache(artID model.ArtworkID) { - if a.cache.Disabled(context.Background()) { - return - } - a.mutex.Lock() - defer a.mutex.Unlock() - a.buffer[artID] = struct{}{} - a.sendWakeSignal() -} - -func (a *cacheWarmer) sendWakeSignal() { - // Don't block if the previous signal was not read yet - select { - case a.wakeSignal <- struct{}{}: - default: - } -} - -func (a *cacheWarmer) run(ctx context.Context) { - for { - a.waitSignal(ctx, 10*time.Second) - if ctx.Err() != nil { - break - } - - if a.cache.Disabled(ctx) { - a.mutex.Lock() - pending := len(a.buffer) - a.buffer = make(map[model.ArtworkID]struct{}) - a.mutex.Unlock() - if pending > 0 { - log.Trace(ctx, "Cache disabled, discarding precache buffer", "bufferLen", pending) - } - return - } - - // If cache not available, keep waiting - if !a.cache.Available(ctx) { - a.mutex.Lock() - bufferLen := len(a.buffer) - a.mutex.Unlock() - if bufferLen > 0 { - log.Trace(ctx, "Cache not available, buffering precache request", "bufferLen", bufferLen) - } - continue - } - - a.mutex.Lock() - - // If there's nothing to send, keep waiting - if len(a.buffer) == 0 { - a.mutex.Unlock() - continue - } - - batch := slices.Collect(maps.Keys(a.buffer)) - a.buffer = make(map[model.ArtworkID]struct{}) - a.mutex.Unlock() - - a.processBatch(ctx, batch) - } -} - -func (a *cacheWarmer) waitSignal(ctx context.Context, timeout time.Duration) { - select { - case <-time.After(timeout): - case <-a.wakeSignal: - case <-ctx.Done(): - } -} - -func (a *cacheWarmer) processBatch(ctx context.Context, batch []model.ArtworkID) { - log.Trace(ctx, "PreCaching a new batch of artwork", "batchSize", len(batch)) - input := pl.FromSlice(ctx, batch) - errs := pl.Sink(ctx, 4, input, a.doCacheImage) - for err := range errs { - log.Debug(ctx, "Error warming cache", err) - } -} - -func (a *cacheWarmer) doCacheImage(ctx context.Context, id model.ArtworkID) error { - ctx, cancel := context.WithTimeout(ctx, 10*time.Second) - defer cancel() - - size := a.coverArtSize - r, _, err := a.artwork.Get(ctx, id, size, true) - if err != nil { - return fmt.Errorf("caching id='%s', size=%d: %w", id, size, err) - } - _, err = io.Copy(io.Discard, r) - r.Close() - return err -} - -func NoopCacheWarmer() CacheWarmer { - return &noopCacheWarmer{} -} - -type noopCacheWarmer struct{} - -func (a *noopCacheWarmer) PreCache(model.ArtworkID) {} diff --git a/core/artwork/cache_warmer_test.go b/core/artwork/cache_warmer_test.go deleted file mode 100644 index a5da2004c..000000000 --- a/core/artwork/cache_warmer_test.go +++ /dev/null @@ -1,245 +0,0 @@ -package artwork - -import ( - "context" - "errors" - "fmt" - "io" - "strings" - "sync" - "sync/atomic" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils/cache" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("CacheWarmer", func() { - var ( - fc *mockFileCache - aw *mockArtwork - ) - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - fc = &mockFileCache{} - aw = &mockArtwork{} - }) - - Context("initialization", func() { - It("returns noop when cache is disabled", func() { - fc.SetDisabled(true) - cw := NewCacheWarmer(aw, fc) - _, ok := cw.(*noopCacheWarmer) - Expect(ok).To(BeTrue()) - }) - - It("returns noop when ImageCacheSize is 0", func() { - conf.Server.ImageCacheSize = "0" - cw := NewCacheWarmer(aw, fc) - _, ok := cw.(*noopCacheWarmer) - Expect(ok).To(BeTrue()) - }) - - It("returns noop when EnableArtworkPrecache is false", func() { - conf.Server.EnableArtworkPrecache = false - cw := NewCacheWarmer(aw, fc) - _, ok := cw.(*noopCacheWarmer) - Expect(ok).To(BeTrue()) - }) - - It("returns real implementation when properly configured", func() { - conf.Server.ImageCacheSize = "100MB" - conf.Server.EnableArtworkPrecache = true - fc.SetDisabled(false) - cw := NewCacheWarmer(aw, fc) - _, ok := cw.(*cacheWarmer) - Expect(ok).To(BeTrue()) - }) - }) - - Context("buffer management", func() { - BeforeEach(func() { - conf.Server.ImageCacheSize = "100MB" - conf.Server.EnableArtworkPrecache = true - fc.SetDisabled(false) - }) - - It("drops buffered items when cache becomes disabled", func() { - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - cw.PreCache(model.MustParseArtworkID("al-test")) - fc.SetDisabled(true) - Eventually(func() int { - cw.mutex.Lock() - defer cw.mutex.Unlock() - return len(cw.buffer) - }).Should(Equal(0)) - }) - - It("adds multiple items to buffer", func() { - fc.SetReady(false) // Make cache unavailable so items stay in buffer - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - cw.PreCache(model.MustParseArtworkID("al-1")) - cw.PreCache(model.MustParseArtworkID("al-2")) - cw.mutex.Lock() - defer cw.mutex.Unlock() - Expect(len(cw.buffer)).To(Equal(2)) - }) - - It("deduplicates items in buffer", func() { - fc.SetReady(false) // Make cache unavailable so items stay in buffer - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - cw.PreCache(model.MustParseArtworkID("al-1")) - cw.PreCache(model.MustParseArtworkID("al-1")) - cw.mutex.Lock() - defer cw.mutex.Unlock() - Expect(len(cw.buffer)).To(Equal(1)) - }) - }) - - Context("error handling", func() { - BeforeEach(func() { - conf.Server.ImageCacheSize = "100MB" - conf.Server.EnableArtworkPrecache = true - fc.SetDisabled(false) - }) - - It("continues processing after artwork retrieval error", func() { - aw.err = errors.New("artwork error") - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - cw.PreCache(model.MustParseArtworkID("al-error")) - cw.PreCache(model.MustParseArtworkID("al-1")) - - Eventually(func() int { - cw.mutex.Lock() - defer cw.mutex.Unlock() - return len(cw.buffer) - }).Should(Equal(0)) - }) - - It("continues processing after cache error", func() { - fc.err = errors.New("cache error") - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - cw.PreCache(model.MustParseArtworkID("al-error")) - cw.PreCache(model.MustParseArtworkID("al-1")) - - Eventually(func() int { - cw.mutex.Lock() - defer cw.mutex.Unlock() - return len(cw.buffer) - }).Should(Equal(0)) - }) - }) - - Context("background processing", func() { - BeforeEach(func() { - conf.Server.ImageCacheSize = "100MB" - conf.Server.EnableArtworkPrecache = true - fc.SetDisabled(false) - }) - - It("processes items in batches", func() { - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - for i := range 5 { - cw.PreCache(model.MustParseArtworkID(fmt.Sprintf("al-%d", i))) - } - - Eventually(func() int { - cw.mutex.Lock() - defer cw.mutex.Unlock() - return len(cw.buffer) - }).Should(Equal(0)) - }) - - It("wakes up on new items", func() { - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - - // Add first batch - cw.PreCache(model.MustParseArtworkID("al-1")) - Eventually(func() int { - cw.mutex.Lock() - defer cw.mutex.Unlock() - return len(cw.buffer) - }).Should(Equal(0)) - - // Add second batch - cw.PreCache(model.MustParseArtworkID("al-2")) - Eventually(func() int { - cw.mutex.Lock() - defer cw.mutex.Unlock() - return len(cw.buffer) - }).Should(Equal(0)) - }) - - It("pre-caches UICoverArtSize", func() { - cw := NewCacheWarmer(aw, fc).(*cacheWarmer) - cw.PreCache(model.MustParseArtworkID("al-1")) - - Eventually(func() []int { - return aw.getCachedSizes() - }).Should(ContainElements(conf.Server.UICoverArtSize)) - }) - }) -}) - -type mockArtwork struct { - err error - mu sync.Mutex - cachedSizes []int -} - -func (m *mockArtwork) Get(ctx context.Context, artID model.ArtworkID, size int, square bool) (io.ReadCloser, time.Time, error) { - if m.err != nil { - return nil, time.Time{}, m.err - } - m.mu.Lock() - m.cachedSizes = append(m.cachedSizes, size) - m.mu.Unlock() - return io.NopCloser(strings.NewReader("test")), time.Now(), nil -} - -func (m *mockArtwork) getCachedSizes() []int { - m.mu.Lock() - defer m.mu.Unlock() - result := make([]int, len(m.cachedSizes)) - copy(result, m.cachedSizes) - return result -} - -func (m *mockArtwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (io.ReadCloser, time.Time, error) { - return m.Get(ctx, model.ArtworkID{}, size, square) -} - -type mockFileCache struct { - disabled atomic.Bool - ready atomic.Bool - err error -} - -func (f *mockFileCache) Get(ctx context.Context, item cache.Item) (*cache.CachedStream, error) { - if f.err != nil { - return nil, f.err - } - return &cache.CachedStream{Reader: io.NopCloser(strings.NewReader("cached"))}, nil -} - -func (f *mockFileCache) Available(ctx context.Context) bool { - return f.ready.Load() && !f.disabled.Load() -} - -func (f *mockFileCache) Disabled(ctx context.Context) bool { - return f.disabled.Load() -} - -func (f *mockFileCache) SetDisabled(v bool) { - f.disabled.Store(v) - f.ready.Store(true) -} - -func (f *mockFileCache) SetReady(v bool) { - f.ready.Store(v) -} diff --git a/core/artwork/reader_disc.go b/core/artwork/disc.go similarity index 79% rename from core/artwork/reader_disc.go rename to core/artwork/disc.go index 0f648c987..8bc566af2 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/disc.go @@ -2,26 +2,22 @@ package artwork import ( "context" - "crypto/md5" "fmt" "io" "path" "path/filepath" "strconv" "strings" - "time" "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils" ) +// discArtworkReader resolves disc-level artwork from a library's folder images +// and embedded tags. It is used by the serving path's provisional disc read-through. type discArtworkReader struct { - cacheKey - a *artwork album model.Album discNumber int imgFiles []string // library-relative, forward-slash, no leading slash @@ -29,27 +25,26 @@ type discArtworkReader struct { isMultiFolder bool firstTrackRel string // library-relative; for fromTag / ffmpeg via lib.Abs lib libraryView - updatedAt *time.Time } -func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID) (*discArtworkReader, error) { +func newDiscArtworkReader(ctx context.Context, ds model.DataStore, artID model.ArtworkID) (*discArtworkReader, error) { albumID, discNumber, err := model.ParseDiscArtworkID(artID.ID) if err != nil { return nil, fmt.Errorf("invalid disc artwork id '%s': %w", artID.ID, err) } - al, err := a.ds.Album(ctx).Get(albumID) + al, err := ds.Album(ctx).Get(albumID) if err != nil { return nil, err } - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, a.ds, *al) + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, *al) if err != nil { return nil, err } // Query mediafiles for this album + disc to find folder associations and first track - mfs, err := a.ds.MediaFile(ctx).GetAll(model.QueryOptions{ + mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ Sort: "track_number", Order: "ASC", Filters: squirrel.Eq{"album_id": albumID, "disc_number": discNumber}, @@ -58,7 +53,7 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID return nil, err } - lib, err := loadLibraryView(ctx, a.ds, al.LibraryID) + lib, err := loadLibraryView(ctx, ds, al.LibraryID) if err != nil { return nil, err } @@ -80,7 +75,7 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID for id := range allFolderIDs { folderIDs = append(folderIDs, id) } - folders, err := a.ds.Folder(ctx).GetAll(model.QueryOptions{ + folders, err := ds.Folder(ctx).GetAll(model.QueryOptions{ Filters: squirrel.Eq{"folder.id": folderIDs}, }) if err != nil { @@ -92,46 +87,15 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID } } - isMultiFolder := len(al.FolderIDs) > 1 - - r := &discArtworkReader{ - a: a, + return &discArtworkReader{ album: *al, discNumber: discNumber, imgFiles: imgFiles, discFoldersRel: discFoldersRel, - isMultiFolder: isMultiFolder, + isMultiFolder: len(al.FolderIDs) > 1, firstTrackRel: firstTrackRel, lib: lib, - updatedAt: imagesUpdatedAt, - } - r.cacheKey.artID = artID - r.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) - if imagesUpdatedAt != nil { - r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *imagesUpdatedAt) - } - return r, nil -} - -func (d *discArtworkReader) Key() string { - hash := md5.Sum([]byte(conf.Server.DiscArtPriority)) - return fmt.Sprintf( - "%s.%x", - d.cacheKey.Key(), - hash, - ) -} - -func (d *discArtworkReader) LastUpdated() time.Time { - return d.lastUpdate -} - -func (d *discArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { - var ff = d.fromDiscArtPriority(ctx, d.a.ffmpeg, conf.Server.DiscArtPriority) - // Fallback to album cover art - albumArtID := model.NewArtworkID(model.KindAlbumArtwork, d.album.ID, &d.album.UpdatedAt) - ff = append(ff, fromAlbum(ctx, d.a, albumArtID)) - return selectImageReader(ctx, d.cacheKey.artID, ff...) + }, nil } func (d *discArtworkReader) fromDiscArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []sourceFunc { diff --git a/core/artwork/reader_disc_test.go b/core/artwork/disc_test.go similarity index 100% rename from core/artwork/reader_disc_test.go rename to core/artwork/disc_test.go diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go deleted file mode 100644 index 5e61684cc..000000000 --- a/core/artwork/e2e/album_test.go +++ /dev/null @@ -1,469 +0,0 @@ -package artworke2e_test - -import ( - "testing/fstest" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -const ( - defaultCoverPriority = "cover.*, folder.*, front.*, embedded, external" - defaultDiscPriority = "disc*.*, cd*.*, cover.*, folder.*, front.*, discsubtitle, embedded" -) - -var _ = Describe("Album artwork resolution", func() { - BeforeEach(func() { - setupHarness() - }) - - When("an album has a single folder with cover.jpg at the album root", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── cover.jpg ← matched by cover.* - It("returns the album-root cover", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("album-root"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) - }) - }) - - // https://github.com/navidrome/navidrome/issues/5376 - // cover.* basenames tie across album-root and per-disc folders; - // compareImageFiles must prefer shallower paths. - When("a multi-disc album has a cover.jpg at the album root and per-disc covers", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── cover.jpg ← should not win - // ├── CD2/ - // │ ├── 01 - Track.mp3 - // │ └── cover.jpg - // └── cover.jpg ← should win (album-root fallback) - It("prefers the album-root cover over per-disc covers", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "Track CD2"), - "Artist/Album/cover.jpg": imageFile("album-root"), - "Artist/Album/CD1/cover.jpg": imageFile("disc1"), - "Artist/Album/CD2/cover.jpg": imageFile("disc2"), - }) - scan() - - al := firstAlbum() - Expect(al.FolderIDs).To(HaveLen(2), - "sanity check: scanner should treat the two disc subfolders as one multi-disc album") - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) - }) - }) - - // https://github.com/navidrome/navidrome/issues/5376 - // folder.jpg basenames tie across album-root and per-disc folders; - // compareImageFiles must prefer shallower paths. - When("a multi-disc album has folder.jpg at the album root AND in each disc subfolder", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg ← should not win - // ├── CD2/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg - // └── folder.jpg ← should win (album-root fallback) - It("prefers the album-root folder.jpg over per-disc folder.jpg", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "Track CD2"), - "Artist/Album/folder.jpg": imageFile("album-root"), - "Artist/Album/CD1/folder.jpg": imageFile("disc1"), - "Artist/Album/CD2/folder.jpg": imageFile("disc2"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) - }) - }) - - // https://github.com/navidrome/navidrome/issues/5376 - // Single-subfolder albums must still consider the parent folder's images. - When("an album lives entirely under a single disc subfolder with cover.jpg at the parent", func() { - // Artist/ - // └── Album/ - // ├── disc1/ - // │ └── 01 - Track.mp3 - // └── cover.jpg ← should win (parent-folder fallback) - It("uses the parent-folder cover for single-disc-subfolder albums", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/disc1/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("album-root"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) - }) - }) - - // https://github.com/navidrome/navidrome/issues/5456 - When("a top-level multi-disc album has cover.jpg at the album root and per-disc folder.jpg", func() { - // Album/ (top-level folder, Path=".") - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg - // ├── CD2/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg - // └── cover.jpg ← should win (album-root) - It("prefers the album-root cover.jpg", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), - "Album/CD2/01 - Track.mp3": trackFile(1, "Track CD2"), - "Album/cover.jpg": imageFile("album-root"), - "Album/CD1/folder.jpg": imageFile("disc1"), - "Album/CD2/folder.jpg": imageFile("disc2"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) - }) - }) - - When("CoverArtPriority puts embedded first and the album has both embedded and external art", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 ← has embedded picture (wins via "embedded") - // └── cover.jpg - It("returns the embedded image", func() { - conf.Server.CoverArtPriority = "embedded, cover.*, folder.*, front.*, external" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"has_picture": "true"}), - "Artist/Album/cover.jpg": imageFile("external"), - }) - scan() - // Swap in real MP3 bytes so libFS.Open returns a taglib-readable stream. - replaceWithRealMP3("Artist/Album/01 - Track.mp3") - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(embeddedArtBytes)) - }) - }) - - When("CoverArtPriority lists external first but no external file is present", func() { - // Artist/ - // └── Album/ - // └── 01 - Track.mp3 ← has embedded picture (falls through to "embedded") - It("falls through to embedded artwork", func() { - conf.Server.CoverArtPriority = "external, embedded" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"has_picture": "true"}), - }) - scan() - replaceWithRealMP3("Artist/Album/01 - Track.mp3") - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(embeddedArtBytes)) - }) - }) - - When("the only cover file uses uppercase extension and a different case in its name", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── Cover.JPG ← matched case-insensitively by cover.* - It("matches case-insensitively against cover.*", func() { - conf.Server.CoverArtPriority = "cover.*, folder.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/Cover.JPG": imageFile("case-insensitive"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("case-insensitive"))) - }) - }) - - When("two cover files have basenames that tie under the natural-sort tiebreaker", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // ├── cover.jpg ← wins (no numeric suffix) - // └── cover.1.jpg - It("prefers the file without a numeric suffix", func() { - conf.Server.CoverArtPriority = "cover.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("primary"), - "Artist/Album/cover.1.jpg": imageFile("secondary"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("primary"))) - }) - }) - - When("the album has no cover and CoverArtPriority lists only file patterns", func() { - // Artist/ - // └── Album/ - // └── 01 - Track.mp3 (no image files — returns ErrUnavailable) - It("returns ErrUnavailable", func() { - conf.Server.CoverArtPriority = "cover.*, folder.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - }) - scan() - - al := firstAlbum() - _, err := readArtworkOrErr(model.NewArtworkID(model.KindAlbumArtwork, al.ID, &al.UpdatedAt)) - Expect(err).To(HaveOccurred()) - }) - }) - - // Doc scenarios from: - // https://www.navidrome.org/docs/usage/library/artwork/#albums - // Default CoverArtPriority is "cover.*, folder.*, front.*, embedded, external". - When("only folder.jpg is present (cover.* and front.* missing)", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── folder.jpg ← matched by folder.* - It("falls through to folder.jpg", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/folder.jpg": imageFile("folder"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("folder"))) - }) - }) - - When("only front.jpg is present (cover.* and folder.* missing)", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── front.jpg ← matched by front.* - It("falls through to front.jpg", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/front.jpg": imageFile("front"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("front"))) - }) - }) - - When("cover.*, folder.*, and front.* all exist in the same folder", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // ├── cover.jpg ← wins (cover.* is first in priority) - // ├── folder.jpg - // └── front.jpg - It("prefers cover.* (first in CoverArtPriority)", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("cover"), - "Artist/Album/folder.jpg": imageFile("folder"), - "Artist/Album/front.jpg": imageFile("front"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("cover"))) - }) - }) - - When("only folder.* and front.* exist (priority order check)", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // ├── folder.jpg ← wins (folder.* comes before front.*) - // └── front.jpg - It("prefers folder.* over front.*", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/folder.jpg": imageFile("folder"), - "Artist/Album/front.jpg": imageFile("front"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("folder"))) - }) - }) - - When("three cover files tie by basename and differ only by numeric suffix", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // ├── cover.jpg ← wins (no numeric suffix) - // ├── cover.1.jpg - // └── cover.2.jpg - It("selects the unsuffixed file first regardless of numeric-suffix order", func() { - conf.Server.CoverArtPriority = "cover.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.2.jpg": imageFile("second"), - "Artist/Album/cover.jpg": imageFile("primary"), - "Artist/Album/cover.1.jpg": imageFile("first"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("primary"))) - }) - }) - - When("CoverArtPriority contains an unknown pattern before a matching one", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── cover.jpg ← wins (unknown "bogus.*" is skipped) - It("skips the unknown pattern and falls through to the matching one", func() { - conf.Server.CoverArtPriority = "bogus.*, cover.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("cover"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("cover"))) - }) - }) - - // Regression introduced in v0.62.0 (#5451 + #5457): the parent-folder - // fallback can pick up images from the ARTIST folder, serving the artist - // thumbnail as album art for any album without its own image files. - When("an album has no images and the artist folder has folder.jpg", func() { - // Artist/ - // ├── folder.jpg ← artist thumbnail, must NOT become album art - // ├── Album A/ - // │ └── 01 - Track.mp3 (no images) - // └── Album B/ - // ├── 01 - Track.mp3 - // └── cover.jpg - It("does not use the artist image as album art", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/folder.jpg": imageFile("artist-thumbnail"), - "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A", map[string]any{"album": "Album A", "albumartist": "Artist"}), - "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), - "Artist/Album B/cover.jpg": imageFile("album-b"), - }) - scan() - - alA := albumByName("Album A") - _, err := readArtworkOrErr(alA.CoverArtID()) - Expect(err).To(HaveOccurred(), - "Album A has no images of its own, so it must fall through to the placeholder "+ - "instead of inheriting the artist folder's folder.jpg") - - alB := albumByName("Album B") - Expect(readArtwork(alB.CoverArtID())).To(Equal(imageBytes("album-b"))) - }) - }) - - When("a single-disc album is spread across sibling folders under the artist folder", func() { - // Artist/ - // ├── folder.jpg ← artist thumbnail, must NOT become album art - // ├── Album A/ - // │ └── 01 - Track.mp3 (album: "Album A") - // ├── Album A bonus/ - // │ └── 02 - Track.mp3 (album: "Album A" — same album, second folder) - // └── Album B/ - // ├── 01 - Track.mp3 - // └── cover.jpg - It("does not use the artist image as album art for the spread album", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/folder.jpg": imageFile("artist-thumbnail"), - "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}), - "Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}), - "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), - "Artist/Album B/cover.jpg": imageFile("album-b"), - }) - scan() - - alA := albumByName("Album A") - Expect(alA.FolderIDs).To(HaveLen(2), - "sanity check: scanner should treat the two sibling folders as one spread album") - _, err := readArtworkOrErr(alA.CoverArtID()) - Expect(err).To(HaveOccurred(), - "the spread album has no images of its own, so it must fall through to the "+ - "placeholder instead of inheriting the artist folder's folder.jpg") - }) - }) - - When("a spread album has its own front.jpg but the artist folder has cover.jpg", func() { - // Artist/ - // ├── cover.jpg ← artist image; matches cover.* (first pattern), - // │ must NOT shadow the album's own front.jpg - // ├── Album A/ - // │ ├── 01 - Track.mp3 (album: "Album A") - // │ └── front.jpg ← should win - // ├── Album A bonus/ - // │ └── 02 - Track.mp3 (album: "Album A") - // └── Album B/ - // └── 01 - Track.mp3 - It("prefers the album's own art over the artist image", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/cover.jpg": imageFile("artist-image"), - "Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}), - "Artist/Album A/front.jpg": imageFile("album-a-front"), - "Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}), - "Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}), - }) - scan() - - alA := albumByName("Album A") - Expect(alA.FolderIDs).To(HaveLen(2), - "sanity check: scanner should treat the two sibling folders as one spread album") - Expect(readArtwork(alA.CoverArtID())).To(Equal(imageBytes("album-a-front"))) - }) - }) - - When("embedded is first in CoverArtPriority but the track has no embedded art", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 (no embedded picture) - // └── cover.jpg ← wins (embedded skipped, falls through) - It("falls through to the next priority entry", func() { - conf.Server.CoverArtPriority = "embedded, cover.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("cover"), - }) - scan() - - al := firstAlbum() - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("cover"))) - }) - }) -}) diff --git a/core/artwork/e2e/artist_test.go b/core/artwork/e2e/artist_test.go deleted file mode 100644 index d959b1d60..000000000 --- a/core/artwork/e2e/artist_test.go +++ /dev/null @@ -1,167 +0,0 @@ -package artworke2e_test - -import ( - "os" - "path/filepath" - "testing/fstest" - - "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -// Doc reference: -// https://www.navidrome.org/docs/usage/library/artwork/#artists -// Default ArtistArtPriority is "artist.*, album/artist.*, external". -var _ = Describe("Artist artwork resolution", func() { - BeforeEach(func() { - setupHarness() - }) - - When("the artist folder contains an artist.jpg", func() { - // Artist/ - // ├── artist.jpg ← matched by artist.* - // └── Album/ - // └── 01 - Track.mp3 - It("returns the artist.* image from the artist folder", func() { - conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), - "Artist/artist.jpg": imageFile("artist-folder"), - }) - scan() - - ar := soleArtist() - artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) - Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) - }) - }) - - When("artist.* only exists inside an album folder", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── artist.jpg ← matched by album/artist.* - It("falls through to album/artist.* and returns that image", func() { - conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), - "Artist/Album/artist.jpg": imageFile("album-artist"), - }) - scan() - - ar := soleArtist() - artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) - Expect(readArtwork(artID)).To(Equal(imageBytes("album-artist"))) - }) - }) - - When("both the artist folder and an album folder have an artist.* image", func() { - // Artist/ - // ├── artist.jpg ← wins (artist.* before album/artist.*) - // └── Album/ - // ├── 01 - Track.mp3 - // └── artist.jpg - It("prefers the artist-folder image (artist.* comes before album/artist.*)", func() { - conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), - "Artist/artist.jpg": imageFile("artist-folder"), - "Artist/Album/artist.jpg": imageFile("album-artist"), - }) - scan() - - ar := soleArtist() - artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) - Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder"))) - }) - }) - - When("an artist has an uploaded image and a matching artist.* file", func() { - // / - // └── artwork/ - // └── artist/ - // └── _upload.jpg ← wins (uploaded image beats the priority chain) - // Library: - // Artist/ - // ├── artist.jpg (ignored — uploaded image comes first) - // └── Album/ - // └── 01 - Track.mp3 - It("prefers the uploaded image over any priority-chain match", func() { - conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), - "Artist/artist.jpg": imageFile("artist-folder"), - }) - scan() - ar := soleArtist() - - uploaded := ar.ID + "_upload.jpg" - writeUploadedImage(consts.EntityArtist, uploaded, imageBytes("artist-uploaded")) - ar.UploadedImage = uploaded - Expect(ds.Artist(ctx).Put(&ar)).To(Succeed()) - - artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) - Expect(readArtwork(artID)).To(Equal(imageBytes("artist-uploaded"))) - }) - }) - - When("ArtistArtPriority uses album/ (not just album/artist.*)", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── artist.jpg ← matched by album/artist.* - It("resolves the pattern against the artist's album image files", func() { - conf.Server.ArtistArtPriority = "album/artist.*, external" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), - "Artist/Album/artist.jpg": imageFile("album-artist"), - }) - scan() - - ar := soleArtist() - artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) - Expect(readArtwork(artID)).To(Equal(imageBytes("album-artist"))) - }) - }) - - When("ArtistArtPriority starts with image-folder and ArtistImageFolder has a name-matching image", func() { - // / - // └── Artist.jpg ← matched by artist name (image-folder source) - // Library: - // Artist/ - // └── Album/ - // └── 01 - Track.mp3 (no artist.* present in library) - It("returns the image from the configured artist image folder", func() { - imgFolder := GinkgoT().TempDir() - Expect(os.WriteFile(filepath.Join(imgFolder, "Artist.jpg"), imageBytes("image-folder"), 0600)).To(Succeed()) - conf.Server.ArtistImageFolder = imgFolder - conf.Server.ArtistArtPriority = "image-folder, artist.*, album/artist.*" - - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}), - }) - scan() - - ar := soleArtist() - artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil) - Expect(readArtwork(artID)).To(Equal(imageBytes("image-folder"))) - }) - }) -}) - -func soleArtist() model.Artist { - GinkgoHelper() - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"artist.name": "Artist"}, - }) - Expect(err).ToNot(HaveOccurred()) - if len(artists) == 0 { - Fail("sole artist not found") - return model.Artist{} - } - return artists[0] -} diff --git a/core/artwork/e2e/disc_test.go b/core/artwork/e2e/disc_test.go deleted file mode 100644 index 667079458..000000000 --- a/core/artwork/e2e/disc_test.go +++ /dev/null @@ -1,371 +0,0 @@ -package artworke2e_test - -import ( - "fmt" - "testing/fstest" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Disc artwork resolution", func() { - BeforeEach(func() { - setupHarness() - }) - - When("the album is single-disc with a disc1.jpg in the only folder", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── disc1.jpg ← matched by disc*.* - It("returns the disc1.jpg image (matched as disc*.*)", func() { - conf.Server.DiscArtPriority = "disc*.*, cd*.*, cover.*, folder.*, front.*, embedded" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/disc1.jpg": imageFile("disc1-image"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("disc1-image"))) - }) - }) - - When("the album has no per-disc image and no album cover", func() { - // Artist/ - // └── Album/ - // └── 01 - Track.mp3 (no disc or album art — returns ErrUnavailable) - It("returns ErrUnavailable for the disc lookup", func() { - conf.Server.DiscArtPriority = "disc*.*, cd*.*" - conf.Server.CoverArtPriority = "cover.*, folder.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - _, err := readArtworkOrErr(discID) - Expect(err).To(HaveOccurred()) - }) - }) - - When("the album has no per-disc image but has an album cover", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // └── cover.jpg ← album-level fallback (no disc art present) - It("falls back to the album cover", func() { - conf.Server.DiscArtPriority = "disc*.*, cd*.*" - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("album-cover"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("album-cover"))) - }) - }) - - When("multiple disc images exist in the same folder (disc1 vs disc10)", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 - // ├── disc1.jpg ← matches request for disc 1 - // └── disc10.jpg - It("matches the requested disc number, not a higher-numbered one", func() { - conf.Server.DiscArtPriority = "disc*.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/disc1.jpg": imageFile("disc-one"), - "Artist/Album/disc10.jpg": imageFile("disc-ten"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("disc-one"))) - }) - }) - - When("a multi-disc album has per-disc covers", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── disc1.jpg ← matches request for disc 1 - // └── CD2/ - // ├── 01 - Track.mp3 - // └── disc2.jpg ← matches request for disc 2 - It("returns the requested disc's image", func() { - conf.Server.DiscArtPriority = "disc*.*" - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/CD1/disc1.jpg": imageFile("disc-1"), - "Artist/Album/CD2/disc2.jpg": imageFile("disc-2"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 2), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("disc-2"))) - }) - }) - - // Doc scenarios from: - // https://www.navidrome.org/docs/usage/library/artwork/#disc-cover-art - // Default DiscArtPriority is "disc*.*, cd*.*, cover.*, folder.*, front.*, discsubtitle, embedded". - When("a disc subfolder has a cd2.png image", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── disc1.jpg - // └── CD2/ - // ├── 01 - Track.mp3 - // └── cd2.png ← matched by cd*.* for disc 2 - It("matches via the cd*.* pattern", func() { - conf.Server.DiscArtPriority = defaultDiscPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/CD1/disc1.jpg": imageFile("disc-1"), - "Artist/Album/CD2/cd2.png": imageFile("cd-2"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 2), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("cd-2"))) - }) - }) - - When("a disc subfolder has cover.jpg but no disc*.*/cd*.* image", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── cover.jpg ← matched by cover.* inside disc folder - // └── CD2/ - // ├── 01 - Track.mp3 - // └── cover.jpg - It("falls through to cover.* inside the disc folder", func() { - conf.Server.DiscArtPriority = defaultDiscPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/CD1/cover.jpg": imageFile("disc1-cover"), - "Artist/Album/CD2/cover.jpg": imageFile("disc2-cover"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("disc1-cover"))) - }) - }) - - When("DiscArtPriority is the empty string", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── disc1.jpg (ignored — DiscArtPriority is empty) - // ├── CD2/ - // │ ├── 01 - Track.mp3 - // │ └── cd2.png (ignored — DiscArtPriority is empty) - // └── cover.jpg ← used for every disc (album-level fallback) - It("skips every disc-level source and returns the album cover", func() { - conf.Server.DiscArtPriority = "" - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/CD1/disc1.jpg": imageFile("disc-1"), - "Artist/Album/CD2/cd2.png": imageFile("cd-2"), - "Artist/Album/cover.jpg": imageFile("album-cover"), - }) - scan() - - al := firstAlbum() - for _, n := range []int{1, 2} { - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, n), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("album-cover")), - "disc %d should use the album cover when DiscArtPriority is empty", n) - } - }) - }) - - When("the documented multi-disc layout is used (disc1.jpg + cd2.png + album-root cover.jpg)", func() { - // Artist/ - // └── Album/ - // ├── disc1/ - // │ ├── disc1.jpg ← matched by disc*.* for disc 1 - // │ ├── 01 - Track.mp3 - // │ └── 02 - Track.mp3 - // ├── disc2/ - // │ ├── cd2.png ← matched by cd*.* for disc 2 - // │ ├── 01 - Track.mp3 - // │ └── 02 - Track.mp3 - // └── cover.jpg (album-level fallback, unused here) - It("matches the per-disc image for each disc", func() { - conf.Server.DiscArtPriority = defaultDiscPriority - conf.Server.CoverArtPriority = defaultCoverPriority - setLayout(fstest.MapFS{ - "Artist/Album/disc1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/disc1/02 - Track.mp3": trackFile(2, "T2", map[string]any{"disc": "1"}), - "Artist/Album/disc2/01 - Track.mp3": trackFile(1, "T3", map[string]any{"disc": "2"}), - "Artist/Album/disc2/02 - Track.mp3": trackFile(2, "T4", map[string]any{"disc": "2"}), - "Artist/Album/disc1/disc1.jpg": imageFile("disc-1"), - "Artist/Album/disc2/cd2.png": imageFile("cd-2"), - "Artist/Album/cover.jpg": imageFile("album-root"), - }) - scan() - - al := firstAlbum() - disc1ID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - disc2ID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 2), &al.UpdatedAt) - Expect(readArtwork(disc1ID)).To(Equal(imageBytes("disc-1"))) - Expect(readArtwork(disc2ID)).To(Equal(imageBytes("cd-2"))) - }) - }) - - When("discsubtitle keyword matches an image whose stem equals the disc's subtitle", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 (discsubtitle="Bonus Tracks") - // └── Bonus Tracks.jpg ← matched by "discsubtitle" keyword - It("selects the subtitle-named image", func() { - conf.Server.DiscArtPriority = "discsubtitle" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1", "discsubtitle": "Bonus Tracks"}), - "Artist/Album/Bonus Tracks.jpg": imageFile("bonus-tracks"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("bonus-tracks"))) - }) - }) - - // Reproduces https://github.com/navidrome/navidrome/issues/5456 - // Deeply nested layout matching the reporter's actual structure. - When("a deeply nested multi-disc album has cover.jpg and per-disc folder.jpg", func() { - // Genre/Artist/Album/ ← album root with cover.jpg - // ├── cover.jpg ← album-level cover - // ├── Disc 01 (Subtitle)/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg ← disc 1 art - // ├── Disc 02 (Subtitle)/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg - // └── ... (12 discs) - It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { - conf.Server.DiscArtPriority = defaultDiscPriority - conf.Server.CoverArtPriority = defaultCoverPriority - discNames := []string{ - "Disc 01 (Birth of the Dead - The Studio Sides)", - "Disc 02 (Birth of the Dead - The Live Sides)", - "Disc 03 (The Grateful Dead)", - "Disc 04 (Anthem of the Sun)", - "Disc 05 (Aoxomoxoa)", - "Disc 06 (Live; Dead)", - "Disc 07 (Workingman's Dead)", - "Disc 08 (American Beauty)", - "Disc 09 (Grateful Dead)", - "Disc 10 (Europe '72)", - "Disc 11 (Europe '72)", - "Disc 12 (History of the Grateful Dead, Volume One (Bear's Choice))", - } - layout := fstest.MapFS{ - "Pop; Rock/Grateful Dead/(2001) The Golden Road/cover.jpg": imageFile("album-root-cover"), - } - for i, name := range discNames { - discNum := i + 1 - prefix := fmt.Sprintf("Pop; Rock/Grateful Dead/(2001) The Golden Road/%s/", name) - layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", discNum), map[string]any{"disc": fmt.Sprintf("%d", discNum)}) - layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", discNum)) - } - setLayout(layout) - scan() - - al := firstAlbum() - - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) - - for i := range discNames { - discNum := i + 1 - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, discNum), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", discNum))), - "disc %d should use its own folder.jpg", discNum) - } - }) - }) - - // https://github.com/navidrome/navidrome/issues/5456 - // Top-level album variant — album folder at library root (Path="."). - When("a top-level multi-disc album has cover.jpg and per-disc folder.jpg", func() { - // Album/ (top-level, Path=".") - // ├── cover.jpg ← album-level cover - // ├── Disc 01/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg ← disc 1 art - // ├── Disc 02/ - // │ ├── 01 - Track.mp3 - // │ └── folder.jpg - // └── Disc 03/ - // ├── 01 - Track.mp3 - // └── folder.jpg - It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { - conf.Server.DiscArtPriority = defaultDiscPriority - conf.Server.CoverArtPriority = defaultCoverPriority - layout := fstest.MapFS{ - "Album/cover.jpg": imageFile("album-root-cover"), - } - for i := 1; i <= 3; i++ { - prefix := fmt.Sprintf("Album/Disc %02d/", i) - layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", i), map[string]any{"disc": fmt.Sprintf("%d", i)}) - layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", i)) - } - setLayout(layout) - scan() - - al := firstAlbum() - - Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) - - for i := 1; i <= 3; i++ { - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, i), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", i))), - "disc %d should use its own folder.jpg", i) - } - }) - }) - - When("discsubtitle is set but no image filename matches the subtitle", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 (discsubtitle="Bonus Tracks") - // └── cover.jpg ← wins (discsubtitle has no match, falls through) - It("falls through to the next priority entry", func() { - conf.Server.DiscArtPriority = "discsubtitle, cover.*" - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1", "discsubtitle": "Bonus Tracks"}), - "Artist/Album/cover.jpg": imageFile("cover"), - }) - scan() - - al := firstAlbum() - discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, 1), &al.UpdatedAt) - Expect(readArtwork(discID)).To(Equal(imageBytes("cover"))) - }) - }) -}) diff --git a/core/artwork/e2e/helpers_test.go b/core/artwork/e2e/helpers_test.go deleted file mode 100644 index 4e99f7a5c..000000000 --- a/core/artwork/e2e/helpers_test.go +++ /dev/null @@ -1,187 +0,0 @@ -package artworke2e_test - -import ( - "bytes" - "context" - _ "embed" - "errors" - "hash/fnv" - "image" - "image/color" - "image/png" - "io" - "maps" - "net/url" - "os" - "path/filepath" - "testing/fstest" - - "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/external" - "github.com/navidrome/navidrome/core/storage/storagetest" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/resources" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "go.senan.xyz/taglib" -) - -// realMP3WithEmbeddedArt is the bytes of the canonical test fixture that -// contains a valid MP3 stream with an embedded picture. Used in the -// embedded-art e2e scenarios where FakeFS's JSON-encoded tag data isn't -// readable by taglib. Swap this into fakeFS.MapFS *after* scanning so the -// scanner still populates EmbedArtPath via the JSON-tagged track, and the -// artwork reader gets real bytes when it calls libFS.Open. -// -//go:embed testdata/embedded_art.mp3 -var realMP3WithEmbeddedArt []byte - -// embeddedArtBytes is the exact image payload that the artwork reader will -// extract from realMP3WithEmbeddedArt. Computed once via taglib so tests can -// assert byte-for-byte equality — if this ever differs it means the reader -// pulled from a different source. -var embeddedArtBytes = extractEmbeddedArt(realMP3WithEmbeddedArt) - -func extractEmbeddedArt(mp3 []byte) []byte { - tf, err := taglib.OpenStream(bytes.NewReader(mp3)) - if err != nil { - panic("embedded-art fixture: taglib.OpenStream failed: " + err.Error()) - } - defer tf.Close() - images := tf.Properties().Images - if len(images) == 0 { - panic("embedded-art fixture has no embedded images") - } - data, err := tf.Image(0) - if err != nil || len(data) == 0 { - panic("embedded-art fixture: could not read image 0") - } - return data -} - -// replaceWithRealMP3 swaps the FakeFS entry at the given library-relative -// path so libFS.Open returns an MP3 stream taglib can parse. -func replaceWithRealMP3(relPath string) { - GinkgoHelper() - fakeFS.MapFS[relPath] = &fstest.MapFile{Data: realMP3WithEmbeddedArt} -} - -// placeholderBytes returns the bundled album-placeholder image bytes — the -// same stream the artwork reader emits when every source falls through. -func placeholderBytes() []byte { - GinkgoHelper() - r, err := resources.FS().Open(consts.PlaceholderAlbumArt) - Expect(err).ToNot(HaveOccurred()) - defer r.Close() - data, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - return data -} - -// writeUploadedImage drops `filename` into /artwork// with -// the given bytes, matching the on-disk layout expected by -// model.UploadedImagePath. -func writeUploadedImage(entity, filename string, data []byte) { - GinkgoHelper() - dir := filepath.Dir(model.UploadedImagePath(entity, filename)) - Expect(os.MkdirAll(dir, 0755)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(dir, filename), data, 0600)).To(Succeed()) -} - -func newNoopFFmpeg() *tests.MockFFmpeg { - ff := tests.NewMockFFmpeg("") - ff.Error = errors.New("noop") - return ff -} - -// trackFile builds a FakeFS MP3 entry with optional tag overrides. -func trackFile(num int, title string, extra ...map[string]any) *fstest.MapFile { - tags := storagetest.Track(num, title) - for _, e := range extra { - maps.Copy(tags, e) - } - return storagetest.MP3(tags) -} - -// imageFile builds a label-keyed image entry. The bytes are deterministic -// per-label so tests can assert which file won. -func imageFile(label string) *fstest.MapFile { - return &fstest.MapFile{Data: []byte("image:" + label)} -} - -// realPNG builds a minimal 2x2 PNG with a color derived from label. Needed by -// tests that feed the bytes into image.Decode (e.g. playlist tiled covers). -func realPNG(label string) *fstest.MapFile { - img := image.NewRGBA(image.Rect(0, 0, 2, 2)) - // Derive a deterministic color per label. - h := fnv.New32a() - _, _ = h.Write([]byte(label)) - sum := h.Sum32() - c := color.RGBA{R: byte(sum), G: byte(sum >> 8), B: byte(sum >> 16), A: 255} - for y := range 2 { - for x := range 2 { - img.Set(x, y, c) - } - } - var buf bytes.Buffer - Expect(png.Encode(&buf, img)).To(Succeed()) - return &fstest.MapFile{Data: buf.Bytes()} -} - -// imageBytes returns the bytes that imageFile(label) writes. -func imageBytes(label string) []byte { return imageFile(label).Data } - -// setLayout populates fakeFS with the given map. Call after setupHarness. -// All paths must be forward-slash and relative (no leading "/"). -func setLayout(files fstest.MapFS) { - GinkgoHelper() - fakeFS.SetFiles(files) -} - -func readArtwork(artID model.ArtworkID) []byte { - GinkgoHelper() - r, _, err := aw.Get(ctx, artID, 0, false) - Expect(err).ToNot(HaveOccurred()) - defer r.Close() - b, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - return b -} - -func readArtworkOrErr(artID model.ArtworkID) ([]byte, error) { - r, _, err := aw.Get(ctx, artID, 0, false) - if err != nil { - return nil, err - } - defer r.Close() - return io.ReadAll(r) -} - -// noopProvider implements external.Provider with not-found returns so the -// "external" priority entry never produces a result. -type noopProvider struct{} - -func (n *noopProvider) UpdateAlbumInfo(context.Context, string) (*model.Album, error) { - return nil, model.ErrNotFound -} -func (n *noopProvider) UpdateArtistInfo(context.Context, string, int, bool) (*model.Artist, error) { - return nil, model.ErrNotFound -} -func (n *noopProvider) SimilarSongs(context.Context, string, int) (model.MediaFiles, error) { - return nil, nil -} -func (n *noopProvider) TopSongs(context.Context, string, int) (model.MediaFiles, error) { - return nil, nil -} -func (n *noopProvider) ArtistImage(context.Context, string) (*url.URL, error) { - return nil, model.ErrNotFound -} -func (n *noopProvider) ArtistImageResult(context.Context, string) (*url.URL, error) { - return nil, model.ErrNotFound -} -func (n *noopProvider) AlbumImage(context.Context, string) (*url.URL, error) { - return nil, model.ErrNotFound -} - -var _ external.Provider = (*noopProvider)(nil) diff --git a/core/artwork/e2e/mediafile_test.go b/core/artwork/e2e/mediafile_test.go deleted file mode 100644 index 1f43a3827..000000000 --- a/core/artwork/e2e/mediafile_test.go +++ /dev/null @@ -1,110 +0,0 @@ -package artworke2e_test - -import ( - "testing/fstest" - - "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -// Doc reference: -// https://www.navidrome.org/docs/usage/library/artwork/#mediafiles -// Navidrome resolves mediafile artwork in this order: -// 1. Embedded image from the mediafile itself -// 2. For multi-disc albums, disc-level artwork -// 3. Album cover art -// -// FakeFS cannot synthesize taglib-readable embedded JPEGs, so scenario (1) -// is covered by the existing embedded-art album tests (which currently -// Skip under FakeFS). The tests below cover (2) and (3): the fallback -// chain for tracks without embedded art. -var _ = Describe("MediaFile artwork fallback", func() { - BeforeEach(func() { - setupHarness() - }) - - When("a multi-disc album track has no embedded art", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ ├── 01 - Track.mp3 - // │ └── disc1.jpg - // ├── CD2/ - // │ ├── 01 - Track.mp3 ← track requested - // │ └── disc2.jpg ← wins (disc-level before album-level) - // └── cover.jpg - It("falls back to the disc-level artwork (not the album cover)", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - conf.Server.DiscArtPriority = defaultDiscPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/CD1/disc1.jpg": imageFile("disc-1"), - "Artist/Album/CD2/disc2.jpg": imageFile("disc-2"), - "Artist/Album/cover.jpg": imageFile("album-root"), - }) - scan() - - mf := mediafileOn("Artist/Album/CD2/01 - Track.mp3") - Expect(readArtwork(mf.CoverArtID())).To(Equal(imageBytes("disc-2"))) - }) - }) - - When("a single-disc album track has no embedded art", func() { - // Artist/ - // └── Album/ - // ├── 01 - Track.mp3 ← track requested - // └── cover.jpg ← wins (album-level fallback, no disc subfolder) - It("falls back to the album cover", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - conf.Server.DiscArtPriority = defaultDiscPriority - setLayout(fstest.MapFS{ - "Artist/Album/01 - Track.mp3": trackFile(1, "Track"), - "Artist/Album/cover.jpg": imageFile("album-cover"), - }) - scan() - - mf := mediafileOn("Artist/Album/01 - Track.mp3") - Expect(readArtwork(mf.CoverArtID())).To(Equal(imageBytes("album-cover"))) - }) - }) - - When("a multi-disc album track has no embedded art and the disc has no disc-level image", func() { - // Artist/ - // └── Album/ - // ├── CD1/ - // │ └── 01 - Track.mp3 - // ├── CD2/ - // │ └── 01 - Track.mp3 ← track requested - // └── cover.jpg ← wins (no disc image → album-level fallback) - It("falls through from disc to album cover", func() { - conf.Server.CoverArtPriority = defaultCoverPriority - conf.Server.DiscArtPriority = defaultDiscPriority - setLayout(fstest.MapFS{ - "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "T1", map[string]any{"disc": "1"}), - "Artist/Album/CD2/01 - Track.mp3": trackFile(1, "T2", map[string]any{"disc": "2"}), - "Artist/Album/cover.jpg": imageFile("album-root"), - }) - scan() - - mf := mediafileOn("Artist/Album/CD2/01 - Track.mp3") - Expect(readArtwork(mf.CoverArtID())).To(Equal(imageBytes("album-root"))) - }) - }) -}) - -func mediafileOn(relPath string) model.MediaFile { - GinkgoHelper() - mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Like{"media_file.path": relPath}, - }) - Expect(err).ToNot(HaveOccurred()) - if len(mfs) == 0 { - Fail("mediafile not found: " + relPath) - return model.MediaFile{} - } - return mfs[0] -} diff --git a/core/artwork/e2e/playlist_test.go b/core/artwork/e2e/playlist_test.go deleted file mode 100644 index d28efca8e..000000000 --- a/core/artwork/e2e/playlist_test.go +++ /dev/null @@ -1,158 +0,0 @@ -package artworke2e_test - -import ( - "os" - "path/filepath" - "testing/fstest" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -// Playlist artwork resolves in this priority order: -// 1. Uploaded image (/artwork/playlist/) -// 2. Sidecar image next to the .m3u file (same basename, any image ext) -// 3. ExternalImageURL (http/https requires EnableM3UExternalAlbumArt; local path always allowed) -// 4. Generated 2x2 tiled cover from the playlist's albums -// 5. Album placeholder image -// -// The library FS is FakeFS, but uploaded/sidecar/local-external images are -// real files on disk — the reader reads them via os.Open, so the tests -// place them in a real tempdir under DataFolder. -var _ = Describe("Playlist artwork resolution", func() { - BeforeEach(func() { - setupHarness() - }) - - When("a playlist has an uploaded image", func() { - // / - // └── artwork/ - // └── playlist/ - // └── pl-1_upload.jpg ← matched by UploadedImagePath() (highest priority) - It("returns the uploaded image bytes", func() { - writeUploadedImage(consts.EntityPlaylist, "pl-1_upload.jpg", imageBytes("playlist-upload")) - - pl := putPlaylist(model.Playlist{ID: "pl-1", Name: "Test", UploadedImage: "pl-1_upload.jpg"}) - - Expect(readArtwork(pl.CoverArtID())).To(Equal(imageBytes("playlist-upload"))) - }) - }) - - When("a playlist has no uploaded image but a sidecar image beside its .m3u file", func() { - // / - // ├── MyList.m3u - // └── MyList.jpg ← matched by sidecar (same basename, case-insensitive) - It("returns the sidecar image", func() { - dir := GinkgoT().TempDir() - m3uPath := filepath.Join(dir, "MyList.m3u") - Expect(os.WriteFile(m3uPath, []byte("#EXTM3U\n"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(dir, "MyList.jpg"), imageBytes("sidecar"), 0600)).To(Succeed()) - - pl := putPlaylist(model.Playlist{ID: "pl-2", Name: "MyList", Path: m3uPath}) - - Expect(readArtwork(pl.CoverArtID())).To(Equal(imageBytes("sidecar"))) - }) - }) - - When("a playlist's sidecar uses a different extension case", func() { - // / - // ├── MyList.m3u - // └── MyList.PNG ← matched case-insensitively - It("matches case-insensitively", func() { - dir := GinkgoT().TempDir() - m3uPath := filepath.Join(dir, "MyList.m3u") - Expect(os.WriteFile(m3uPath, []byte("#EXTM3U\n"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(dir, "MyList.PNG"), imageBytes("sidecar-png"), 0600)).To(Succeed()) - - pl := putPlaylist(model.Playlist{ID: "pl-3", Name: "MyList", Path: m3uPath}) - - Expect(readArtwork(pl.CoverArtID())).To(Equal(imageBytes("sidecar-png"))) - }) - }) - - When("a playlist has an ExternalImageURL pointing to a local file", func() { - // / - // └── cover.jpg ← absolute path stored in ExternalImageURL - It("returns the local file regardless of EnableM3UExternalAlbumArt", func() { - conf.Server.EnableM3UExternalAlbumArt = false // local paths bypass the toggle - dir := GinkgoT().TempDir() - imgPath := filepath.Join(dir, "cover.jpg") - Expect(os.WriteFile(imgPath, imageBytes("external-local"), 0600)).To(Succeed()) - - pl := putPlaylist(model.Playlist{ID: "pl-4", Name: "WithExt", ExternalImageURL: imgPath}) - - Expect(readArtwork(pl.CoverArtID())).To(Equal(imageBytes("external-local"))) - }) - }) - - When("a playlist has an http(s) ExternalImageURL and EnableM3UExternalAlbumArt is false", func() { - // (no local files — http source is gated off, reader falls through to placeholder) - It("skips the URL and falls through to the bundled placeholder", func() { - conf.Server.EnableM3UExternalAlbumArt = false - - pl := putPlaylist(model.Playlist{ID: "pl-5", Name: "HttpGated", ExternalImageURL: "https://example.com/cover.jpg"}) - - Expect(readArtwork(pl.CoverArtID())).To(Equal(placeholderBytes())) - }) - }) - - When("a playlist has no images and no tracks", func() { - // (reader falls all the way through to the bundled album placeholder) - It("returns the album placeholder", func() { - pl := putPlaylist(model.Playlist{ID: "pl-6", Name: "Empty"}) - - Expect(readArtwork(pl.CoverArtID())).To(Equal(placeholderBytes())) - }) - }) - - When("a playlist has no uploaded/sidecar/external image but has tracks with album covers", func() { - // Library: - // Artist/ - // ├── AlbumA/ - // │ ├── 01 - Track.mp3 - // │ └── cover.png (real PNG — wins as tile 1 source) - // └── AlbumB/ - // ├── 01 - Track.mp3 - // └── cover.png (real PNG — wins as tile 2 source) - // Playlist "pl-7" references tracks from both albums, so the reader - // generates a 2x2 tiled cover from 2 distinct album art tiles (the - // tiled generator mirrors when it has fewer than 4 unique tiles). - It("generates a tiled cover from album art", func() { - conf.Server.CoverArtPriority = "cover.*" - setLayout(fstest.MapFS{ - "Artist/AlbumA/01 - Track.mp3": trackFile(1, "TA", map[string]any{"album": "AlbumA"}), - "Artist/AlbumA/cover.png": realPNG("albumA"), - "Artist/AlbumB/01 - Track.mp3": trackFile(1, "TB", map[string]any{"album": "AlbumB"}), - "Artist/AlbumB/cover.png": realPNG("albumB"), - }) - scan() - - // Pull the scanned mediafile IDs so we can attach them to the playlist. - mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{}) - Expect(err).ToNot(HaveOccurred()) - Expect(mfs).To(HaveLen(2)) - - pl := model.Playlist{ID: "pl-7", Name: "Mix", OwnerID: "admin-1"} - pl.AddMediaFilesByID([]string{mfs[0].ID, mfs[1].ID}) - Expect(ds.Playlist(ctx).Put(&pl)).To(Succeed()) - - data := readArtwork(pl.CoverArtID()) - // The tiled cover is a PNG-encoded 600x600 image (tileSize const). - // Exact bytes vary (random album order), so assert format + non-trivial size. - Expect(data[:8]).To(Equal([]byte{0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a})) - Expect(len(data)).To(BeNumerically(">", 1000)) - }) - }) -}) - -func putPlaylist(pl model.Playlist) model.Playlist { - GinkgoHelper() - if pl.OwnerID == "" { - pl.OwnerID = "admin-1" - } - Expect(ds.Playlist(ctx).Put(&pl)).To(Succeed()) - return pl -} diff --git a/core/artwork/e2e/radio_test.go b/core/artwork/e2e/radio_test.go deleted file mode 100644 index 73ee5f377..000000000 --- a/core/artwork/e2e/radio_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package artworke2e_test - -import ( - "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Radio artwork resolution", func() { - BeforeEach(func() { - setupHarness() - }) - - When("a radio has an uploaded image", func() { - // / - // └── artwork/ - // └── radio/ - // └── rd-1_logo.jpg ← matched by UploadedImagePath() - It("returns the uploaded image bytes", func() { - writeUploadedImage(consts.EntityRadio, "rd-1_logo.jpg", imageBytes("radio-logo")) - - rd := model.Radio{ID: "rd-1", Name: "Test Radio", StreamUrl: "https://example.com/stream", UploadedImage: "rd-1_logo.jpg"} - Expect(ds.Radio(ctx).Put(&rd)).To(Succeed()) - - artID := model.NewArtworkID(model.KindRadioArtwork, rd.ID, nil) - Expect(readArtwork(artID)).To(Equal(imageBytes("radio-logo"))) - }) - }) - - When("a radio has no uploaded image", func() { - // (no files on disk — reader has no sources to fall back to) - It("returns ErrUnavailable", func() { - rd := model.Radio{ID: "rd-2", Name: "Bare Radio", StreamUrl: "https://example.com/stream"} - Expect(ds.Radio(ctx).Put(&rd)).To(Succeed()) - - artID := model.NewArtworkID(model.KindRadioArtwork, rd.ID, nil) - _, err := readArtworkOrErr(artID) - Expect(err).To(HaveOccurred()) - }) - }) -}) diff --git a/core/artwork/e2e/suite_test.go b/core/artwork/e2e/suite_test.go deleted file mode 100644 index 06cc05b6f..000000000 --- a/core/artwork/e2e/suite_test.go +++ /dev/null @@ -1,120 +0,0 @@ -package artworke2e_test - -import ( - "context" - "fmt" - "path/filepath" - "testing" - - _ "github.com/navidrome/navidrome/adapters/gotaglib" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" - "github.com/navidrome/navidrome/core/metrics" - "github.com/navidrome/navidrome/core/playlists" - "github.com/navidrome/navidrome/core/storage/storagetest" - "github.com/navidrome/navidrome/db" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" - "github.com/navidrome/navidrome/persistence" - "github.com/navidrome/navidrome/scanner" - "github.com/navidrome/navidrome/server/events" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -func TestArtworkE2E(t *testing.T) { - tests.Init(t, false) - log.SetLevel(log.LevelFatal) - RegisterFailHandler(Fail) - RunSpecs(t, "Artwork E2E Suite") -} - -const fakeLibScheme = "artworkfake" -const fakeLibPath = fakeLibScheme + ":///music" - -var ( - ctx context.Context - ds *tests.MockDataStore - aw artwork.Artwork - fakeFS *storagetest.FakeFS -) - -// The DB file lives in a suite-level tempdir: the go-sqlite3 singleton keeps -// the file open for the whole suite, and Ginkgo's per-spec TempDir cleanup -// can't unlink a file with a live handle on Windows. A suite-level tempdir -// combined with an AfterSuite close avoids the lock conflict. -var suiteDBTempDir string - -var _ = BeforeSuite(func() { - suiteDBTempDir = GinkgoT().TempDir() -}) - -var _ = AfterSuite(func() { - db.Close(GinkgoT().Context()) -}) - -func setupHarness() { - DeferCleanup(configtest.SetupConfig()) - - tempDir := GinkgoT().TempDir() - // Reuse the suite-level DB path so the singleton connection keeps working - // across specs (see suiteDBTempDir comment). - conf.Server.DbPath = filepath.Join(suiteDBTempDir, "artwork-e2e.db") + "?_journal_mode=WAL" - conf.Server.DataFolder = conf.NewDir(tempDir) - conf.Server.MusicFolder = fakeLibPath - conf.Server.DevExternalScanner = false - conf.Server.ImageCacheSize = "0" // disabled cache → reader runs on every call - conf.Server.EnableExternalServices = false - - db.Db().SetMaxOpenConns(1) - ctx = request.WithUser(GinkgoT().Context(), model.User{ID: "admin-1", UserName: "admin", IsAdmin: true}) - db.Init(ctx) - DeferCleanup(func() { Expect(tests.ClearDB()).To(Succeed()) }) - - ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())} - - adminUser := model.User{ID: "admin-1", UserName: "admin", Name: "Admin", IsAdmin: true, NewPassword: "password"} - Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) - - lib := model.Library{ID: 1, Name: "Music", Path: fakeLibPath} - Expect(ds.Library(ctx).Put(&lib)).To(Succeed()) - Expect(ds.User(ctx).SetUserLibraries(adminUser.ID, []int{lib.ID})).To(Succeed()) - - fakeFS = &storagetest.FakeFS{} - storagetest.Register(fakeLibScheme, fakeFS) - - aw = artwork.NewArtwork(ds, artwork.GetImageCache(), newNoopFFmpeg(), &noopProvider{}) -} - -func scan() { - GinkgoHelper() - s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), - playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) - _, err := s.ScanAll(ctx, true) - Expect(err).ToNot(HaveOccurred()) -} - -func firstAlbum() model.Album { - GinkgoHelper() - albums, err := ds.Album(ctx).GetAll(model.QueryOptions{}) - Expect(err).ToNot(HaveOccurred()) - Expect(albums).To(HaveLen(1), "expected exactly one album, got %d", len(albums)) - return albums[0] -} - -func albumByName(name string) model.Album { - GinkgoHelper() - albums, err := ds.Album(ctx).GetAll(model.QueryOptions{}) - Expect(err).ToNot(HaveOccurred()) - for _, al := range albums { - if al.Name == name { - return al - } - } - Fail(fmt.Sprintf("album %q not found among %d albums", name, len(albums))) - return model.Album{} -} diff --git a/core/artwork/e2e/testdata/embedded_art.mp3 b/core/artwork/e2e/testdata/embedded_art.mp3 deleted file mode 100644 index 18cb90674..000000000 Binary files a/core/artwork/e2e/testdata/embedded_art.mp3 and /dev/null differ diff --git a/core/artwork/reader_album.go b/core/artwork/folders_album.go similarity index 63% rename from core/artwork/reader_album.go rename to core/artwork/folders_album.go index 8ad07773b..d5abee2ee 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/folders_album.go @@ -3,106 +3,18 @@ package artwork import ( "cmp" "context" - "crypto/md5" "errors" - "fmt" - "io" "path" "slices" "strings" "time" "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/core/external" - "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/natural" ) -type albumArtworkReader struct { - cacheKey - a *artwork - provider external.Provider - album model.Album - updatedAt *time.Time - imgFiles []string // library-relative, forward-slash, no leading slash - lib libraryView -} - -func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.ArtworkID, provider external.Provider) (*albumArtworkReader, error) { - al, err := artwork.ds.Album(ctx).Get(artID.ID) - if err != nil { - return nil, err - } - _, imgFiles, imagesUpdateAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, *al) - if err != nil { - return nil, err - } - lib, err := loadLibraryView(ctx, artwork.ds, al.LibraryID) - if err != nil { - return nil, err - } - a := &albumArtworkReader{ - a: artwork, - provider: provider, - album: *al, - updatedAt: imagesUpdateAt, - imgFiles: imgFiles, - lib: lib, - } - a.cacheKey.artID = artID - a.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) - if imagesUpdateAt != nil { - a.cacheKey.lastUpdate = utils.TimeNewest(a.cacheKey.lastUpdate, *imagesUpdateAt) - } - return a, nil -} - -func (a *albumArtworkReader) Key() string { - hashInput := conf.Server.CoverArtPriority - if conf.Server.EnableExternalServices { - hashInput = conf.Server.Agents + hashInput - } - hash := md5.Sum([]byte(hashInput)) - return fmt.Sprintf( - "%s.%x.%t", - a.cacheKey.Key(), - hash, - conf.Server.EnableExternalServices, - ) -} -func (a *albumArtworkReader) LastUpdated() time.Time { - return a.lastUpdate -} - -func (a *albumArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { - var ff = a.fromCoverArtPriority(ctx, a.a.ffmpeg, conf.Server.CoverArtPriority) - return selectImageReader(ctx, a.artID, ff...) -} - -func (a *albumArtworkReader) fromCoverArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []sourceFunc { - var ff []sourceFunc - for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") { - pattern = strings.TrimSpace(pattern) - switch { - case pattern == "embedded": - embedRel := a.album.EmbedArtPath - ff = append(ff, - fromTag(ctx, a.lib.FS, embedRel), - fromFFmpegTag(ctx, ffmpeg, a.lib.Abs(embedRel)), - ) - case pattern == "external": - ff = append(ff, fromAlbumExternalSource(ctx, a.album, a.provider)) - case len(a.imgFiles) > 0: - ff = append(ff, fromExternalFile(ctx, a.lib.FS, a.imgFiles, pattern)) - } - } - return ff -} - func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...model.Album) ([]string, []string, *time.Time, error) { var folderIDs []string for _, album := range albums { diff --git a/core/artwork/reader_artist.go b/core/artwork/folders_artist.go similarity index 52% rename from core/artwork/reader_artist.go rename to core/artwork/folders_artist.go index 37b7b6dee..b75cddde6 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/folders_artist.go @@ -2,7 +2,6 @@ package artwork import ( "context" - "crypto/md5" "fmt" "io" "io/fs" @@ -14,9 +13,7 @@ import ( "time" "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/str" @@ -28,127 +25,6 @@ const ( maxArtistFolderTraversalDepth = 3 ) -type artistReader struct { - cacheKey - a *artwork - provider external.Provider - artist model.Artist - artistFolder string - imgFiles []string - imgFolderImgPath string // cached path from ArtistImageFolder lookup - lib libraryView -} - -func newArtistArtworkReader(ctx context.Context, artwork *artwork, artID model.ArtworkID, provider external.Provider) (*artistReader, error) { - ar, err := artwork.ds.Artist(ctx).Get(artID.ID) - if err != nil { - return nil, err - } - // Only consider albums where the artist is the sole album artist. - als, err := artwork.ds.Album(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.And{ - squirrel.Eq{"album_artist_id": artID.ID}, - squirrel.Eq{"json_array_length(participants, '$.albumartist')": 1}, - }, - }) - if err != nil { - return nil, err - } - albumPaths, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, als...) - if err != nil { - return nil, err - } - artistFolder, artistFolderLastUpdate, err := loadArtistFolder(ctx, artwork.ds, als, albumPaths) - if err != nil { - return nil, err - } - var lib libraryView - if len(als) > 0 { - lib, err = loadLibraryView(ctx, artwork.ds, als[0].LibraryID) - if err != nil { - return nil, err - } - } - a := &artistReader{ - a: artwork, - provider: provider, - artist: *ar, - artistFolder: artistFolder, - imgFiles: imgFiles, - lib: lib, - } - // TODO Find a way to factor in the ExternalUpdateInfoAt in the cache key. Problem is that it can - // change _after_ retrieving from external sources, making the key invalid - //a.cacheKey.lastUpdate = ar.ExternalInfoUpdatedAt - - a.cacheKey.lastUpdate = *imagesUpdatedAt - if ar.UpdatedAt != nil && ar.UpdatedAt.After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = *ar.UpdatedAt - } - if artistFolderLastUpdate.After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = artistFolderLastUpdate - } - if conf.Server.ArtistImageFolder != "" && strings.Contains(strings.ToLower(conf.Server.ArtistArtPriority), "image-folder") { - a.imgFolderImgPath = findImageInArtistFolder(conf.Server.ArtistImageFolder, ar.MbzArtistID, ar.Name) - if a.imgFolderImgPath != "" { - if info, err := os.Stat(a.imgFolderImgPath); err == nil && info.ModTime().After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = info.ModTime() - } - } - } - a.cacheKey.artID = artID - return a, nil -} - -func (a *artistReader) Key() string { - hash := md5.Sum([]byte(conf.Server.Agents)) - return fmt.Sprintf( - "%s.%t.%x", - a.cacheKey.Key(), - conf.Server.EnableExternalServices, - hash, - ) -} - -func (a *artistReader) LastUpdated() time.Time { - return a.lastUpdate -} - -func (a *artistReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { - ff := []sourceFunc{a.fromArtistUploadedImage()} - ff = append(ff, a.fromArtistArtPriority(ctx, conf.Server.ArtistArtPriority)...) - return selectImageReader(ctx, a.artID, ff...) -} - -func (a *artistReader) fromArtistUploadedImage() sourceFunc { - return fromLocalFile(a.artist.UploadedImagePath()) -} - -func (a *artistReader) fromArtistArtPriority(ctx context.Context, priority string) []sourceFunc { - var ff []sourceFunc - for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") { - pattern = strings.TrimSpace(pattern) - switch { - case pattern == "external": - ff = append(ff, fromArtistExternalSource(ctx, a.artist, a.provider)) - case pattern == "image-folder": - ff = append(ff, a.fromArtistImageFolder(ctx)) - case strings.HasPrefix(pattern, "album/"): - if a.lib.FS != nil { - ff = append(ff, fromExternalFile(ctx, a.lib.FS, a.imgFiles, strings.TrimPrefix(pattern, "album/"))) - } - default: - ff = append(ff, fromArtistFolder(ctx, a.lib.FS, a.lib.absRoot, a.artistFolder, pattern)) - } - } - return ff -} - -// fromArtistFolder walks up from artistFolder toward libPath looking for a -// file matching pattern. Traversal is bounded by both maxArtistFolderTraversalDepth -// and the library root: once we reach libPath (or if artistFolder is outside -// libPath), the walk stops. All reads go through libFS, which keeps artwork -// resolution scoped to the configured library. func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, pattern string) sourceFunc { return func() (io.ReadCloser, string, error) { if libFS == nil { @@ -262,29 +138,6 @@ func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albu return folderPath, folders[0].ImagesUpdatedAt, nil } -func (a *artistReader) fromArtistImageFolder(ctx context.Context) sourceFunc { - return func() (io.ReadCloser, string, error) { - folder := conf.Server.ArtistImageFolder - if folder == "" { - return nil, "", nil - } - // Use cached path from newArtistArtworkReader if available, - // avoiding a second directory scan. - path := a.imgFolderImgPath - if path == "" { - path = findImageInArtistFolder(folder, a.artist.MbzArtistID, a.artist.Name) - } - if path == "" { - return nil, "", fmt.Errorf("no image found for artist %q in %s", a.artist.Name, folder) - } - f, err := os.Open(path) - if err != nil { - return nil, "", err - } - return f, path, nil - } -} - // findImageInArtistFolder scans a folder for an image file matching the artist's MBID or name // (case-insensitive). Returns the full path, or empty string if not found. func findImageInArtistFolder(folder, mbzArtistID, artistName string) string { diff --git a/core/artwork/image_cache.go b/core/artwork/image_cache.go index ac0f63794..7d73b1bae 100644 --- a/core/artwork/image_cache.go +++ b/core/artwork/image_cache.go @@ -2,29 +2,21 @@ package artwork import ( "context" - "fmt" "io" "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/utils/cache" "github.com/navidrome/navidrome/utils/singleton" ) -type cacheKey struct { - artID model.ArtworkID - lastUpdate time.Time -} - -func (k *cacheKey) Key() string { - return fmt.Sprintf( - "%s-%s.%d", - k.artID.Kind, - k.artID.ID, - k.lastUpdate.UnixMilli(), - ) +// artworkReader is the cache.Item the image cache loader dispatches on: Reader +// produces the (possibly resized) bytes to store under Key. +type artworkReader interface { + cache.Item + LastUpdated() time.Time + Reader(ctx context.Context) (io.ReadCloser, string, error) } type imageCache struct { diff --git a/core/artwork/playlist_cover.go b/core/artwork/playlist_cover.go new file mode 100644 index 000000000..627a06b49 --- /dev/null +++ b/core/artwork/playlist_cover.go @@ -0,0 +1,100 @@ +package artwork + +import ( + "context" + "image" + "image/draw" + "io" + "os" + "path/filepath" + "strings" + + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + xdraw "golang.org/x/image/draw" +) + +const tileSize = 600 + +func fromLocalFile(path string) sourceFunc { + return func() (io.ReadCloser, string, error) { + if path == "" { + return nil, "", nil + } + f, err := os.Open(path) + if err != nil { + return nil, "", err + } + return f, path, nil + } +} + +// findPlaylistSidecarPath scans the directory of the playlist file for a sidecar +// image file with the same base name (case-insensitive). Returns empty string if +// no matching image is found or if plsPath is empty. +func findPlaylistSidecarPath(ctx context.Context, plsPath string) string { + if plsPath == "" { + return "" + } + dir := filepath.Dir(plsPath) + base := strings.TrimSuffix(filepath.Base(plsPath), filepath.Ext(plsPath)) + + entries, err := os.ReadDir(dir) + if err != nil { + log.Warn(ctx, "Could not read directory for playlist sidecar", "dir", dir, err) + return "" + } + for _, entry := range entries { + name := entry.Name() + nameBase := strings.TrimSuffix(name, filepath.Ext(name)) + if !entry.IsDir() && strings.EqualFold(nameBase, base) && model.IsImageFile(name) { + return filepath.Join(dir, name) + } + } + return "" +} + +func rect(pos int) image.Rectangle { + r := image.Rectangle{} + switch pos { + case 1: + r.Min.X = tileSize / 2 + case 2: + r.Min.Y = tileSize / 2 + case 3: + r.Min.X = tileSize / 2 + r.Min.Y = tileSize / 2 + } + r.Max.X = r.Min.X + tileSize/2 + r.Max.Y = r.Min.Y + tileSize/2 + return r +} + +// fillCenter crops the source image from the center and scales it to fill dstW x dstH exactly, +// equivalent to imaging.Fill with Center anchor. +func fillCenter(src image.Image, dstW, dstH int) image.Image { + srcBounds := src.Bounds() + srcW := srcBounds.Dx() + srcH := srcBounds.Dy() + + // Calculate crop rectangle (center crop to match destination aspect ratio) + srcAspect := float64(srcW) / float64(srcH) + dstAspect := float64(dstW) / float64(dstH) + + var cropRect image.Rectangle + if srcAspect > dstAspect { + // Source is wider — crop horizontally + cropW := int(float64(srcH) * dstAspect) + cropX := (srcW - cropW) / 2 + cropRect = image.Rect(srcBounds.Min.X+cropX, srcBounds.Min.Y, srcBounds.Min.X+cropX+cropW, srcBounds.Max.Y) + } else { + // Source is taller — crop vertically + cropH := int(float64(srcW) / dstAspect) + cropY := (srcH - cropH) / 2 + cropRect = image.Rect(srcBounds.Min.X, srcBounds.Min.Y+cropY, srcBounds.Max.X, srcBounds.Min.Y+cropY+cropH) + } + + dst := image.NewNRGBA(image.Rect(0, 0, dstW, dstH)) + xdraw.CatmullRom.Scale(dst, dst.Bounds(), src, cropRect, draw.Src, nil) + return dst +} diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go deleted file mode 100644 index fe4a1a545..000000000 --- a/core/artwork/reader_album_test.go +++ /dev/null @@ -1,454 +0,0 @@ -package artwork - -import ( - "context" - "errors" - "time" - - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Album Artwork Reader", func() { - Describe("loadAlbumFoldersPaths", func() { - var ( - ctx context.Context - ds *fakeDataStore - repo *fakeFolderRepo - album model.Album - now time.Time - expectedAt time.Time - ) - - BeforeEach(func() { - ctx = context.Background() - now = time.Now().Truncate(time.Second) - expectedAt = now.Add(5 * time.Minute) - - // Set up the test folders with image files - repo = &fakeFolderRepo{} - ds = &fakeDataStore{ - folderRepo: repo, - } - album = model.Album{ - ID: "album1", - Name: "Album", - FolderIDs: []string{"folder1", "folder2", "folder3"}, - } - }) - - It("returns sorted image files", func() { - repo.result = []model.Folder{ - { - Path: "Artist/Album/Disc1", - ImagesUpdatedAt: expectedAt, - ImageFiles: []string{"cover.jpg", "back.jpg", "cover.1.jpg"}, - }, - { - Path: "Artist/Album/Disc2", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - { - Path: "Artist/Album/Disc10", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - } - - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(*imagesUpdatedAt).To(Equal(expectedAt)) - - // Check that image files are sorted by base name (without extension) - Expect(imgFiles).To(HaveLen(5)) - - // Files should be sorted by base filename without extension, then by full path - // "back" < "cover", so back.jpg comes first - // Then all cover.jpg files, sorted by path - Expect(imgFiles[0]).To(Equal("Artist/Album/Disc1/back.jpg")) - Expect(imgFiles[1]).To(Equal("Artist/Album/Disc1/cover.jpg")) - Expect(imgFiles[2]).To(Equal("Artist/Album/Disc2/cover.jpg")) - Expect(imgFiles[3]).To(Equal("Artist/Album/Disc10/cover.jpg")) - Expect(imgFiles[4]).To(Equal("Artist/Album/Disc1/cover.1.jpg")) - }) - - It("prioritizes files without numeric suffixes", func() { - // Test case for issue #4683: cover.jpg should come before cover.1.jpg - repo.result = []model.Folder{ - { - Path: "Artist/Album", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.1.jpg", "cover.jpg", "cover.2.jpg"}, - }, - } - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(HaveLen(3)) - - // cover.jpg should come first because "cover" < "cover.1" < "cover.2" - Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg")) - Expect(imgFiles[1]).To(Equal("Artist/Album/cover.1.jpg")) - Expect(imgFiles[2]).To(Equal("Artist/Album/cover.2.jpg")) - }) - - It("handles case-insensitive sorting", func() { - // Test that Cover.jpg and cover.jpg are treated as equivalent - repo.result = []model.Folder{ - { - Path: "Artist/Album", - ImagesUpdatedAt: now, - ImageFiles: []string{"Folder.jpg", "cover.jpg", "BACK.jpg"}, - }, - } - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(HaveLen(3)) - - // Files should be sorted case-insensitively: BACK, cover, Folder - Expect(imgFiles[0]).To(Equal("Artist/Album/BACK.jpg")) - Expect(imgFiles[1]).To(Equal("Artist/Album/cover.jpg")) - Expect(imgFiles[2]).To(Equal("Artist/Album/Folder.jpg")) - }) - - It("includes images from parent folder for multi-disc albums", func() { - // Simulates: Artist/Album/cover.jpg with tracks in Artist/Album/CD1/ and Artist/Album/CD2/ - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist/Album", - Name: "CD1", - ParentID: "parentFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - { - ID: "folder2", - Path: "Artist/Album", - Name: "CD2", - ParentID: "parentFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - repo.parentResult = &model.Folder{ - ID: "parentFolder", - Path: "Artist", - Name: "Album", - ParentID: "artistFolder", - ImagesUpdatedAt: expectedAt, - ImageFiles: []string{"cover.jpg", "back.jpg"}, - } - - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(*imagesUpdatedAt).To(Equal(expectedAt)) - Expect(imgFiles).To(HaveLen(2)) - Expect(imgFiles[0]).To(Equal("Artist/Album/back.jpg")) - Expect(imgFiles[1]).To(Equal("Artist/Album/cover.jpg")) - }) - - It("does not query parent when parent ID is already in album folders", func() { - // When the parent folder is already one of the album's folders, skip it - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist", - Name: "Album", - ParentID: "folder2", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - { - ID: "folder2", - Path: "", - Name: "Artist", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(HaveLen(1)) - Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg")) - // Get should not have been called (parent already in folder set) - Expect(repo.getCallCount).To(Equal(0)) - }) - - It("does not query parent when folders have different parents", func() { - // When album folders span different parents, don't search any parent - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist1/Album", - Name: "part1", - ParentID: "parentA", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - { - ID: "folder2", - Path: "Artist2/Album", - Name: "part2", - ParentID: "parentB", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(HaveLen(1)) - Expect(imgFiles[0]).To(Equal("Artist1/Album/part1/cover.jpg")) - // Get should not have been called (different parents) - Expect(repo.getCallCount).To(Equal(0)) - }) - - It("does not include library root parent for multi-folder albums", func() { - // Two album parts directly under the library root — parent is the root itself - repo.result = []model.Folder{ - { - ID: "folder1", - Path: ".", - Name: "AlbumPart1", - ParentID: "rootFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - { - ID: "folder2", - Path: ".", - Name: "AlbumPart2", - ParentID: "rootFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - repo.parentResult = &model.Folder{ - ID: "rootFolder", - Path: "", - Name: ".", - ParentID: "", - ImageFiles: []string{"unrelated.jpg"}, - } - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(HaveLen(1)) - Expect(imgFiles[0]).To(Equal("AlbumPart1/cover.jpg")) - Expect(repo.getCallCount).To(Equal(1)) - }) - - It("includes top-level album folder for multi-disc albums", func() { - // Album folder directly under library root, with disc subfolders - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Album", - Name: "Disc1", - ParentID: "albumFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{"folder.jpg"}, - }, - { - ID: "folder2", - Path: "Album", - Name: "Disc2", - ParentID: "albumFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{"folder.jpg"}, - }, - } - repo.parentResult = &model.Folder{ - ID: "albumFolder", - Path: ".", - Name: "Album", - ParentID: "rootFolder", - ImagesUpdatedAt: expectedAt, - ImageFiles: []string{"cover.jpg"}, - } - - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(*imagesUpdatedAt).To(Equal(expectedAt)) - Expect(imgFiles).To(HaveLen(3)) - Expect(imgFiles[0]).To(Equal("Album/cover.jpg")) - Expect(imgFiles[1]).To(Equal("Album/Disc1/folder.jpg")) - Expect(imgFiles[2]).To(Equal("Album/Disc2/folder.jpg")) - Expect(repo.getCallCount).To(Equal(1)) - }) - - It("does not query parent for single-folder albums that already have images", func() { - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist", - Name: "Album", - ParentID: "artistFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - } - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(HaveLen(1)) - Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg")) - Expect(repo.getCallCount).To(Equal(0)) - }) - - It("includes parent images for single-disc-subfolder albums", func() { - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist/Album", - Name: "disc1", - ParentID: "albumFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - repo.parentResult = &model.Folder{ - ID: "albumFolder", - Path: "Artist", - Name: "Album", - ParentID: "artistFolder", - ImagesUpdatedAt: expectedAt, - ImageFiles: []string{"cover.jpg"}, - } - - _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(*imagesUpdatedAt).To(Equal(expectedAt)) - Expect(imgFiles).To(HaveLen(1)) - Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg")) - Expect(repo.getCallCount).To(Equal(1)) - }) - - It("does not include parent images when other albums' audio lives under the parent", func() { - // Simulates: Artist/folder.jpg with Artist/Album (no images) and - // another album's tracks elsewhere under the artist folder - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist", - Name: "Album", - ParentID: "artistFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - repo.parentResult = &model.Folder{ - ID: "artistFolder", - Path: ".", - Name: "Artist", - ParentID: "libraryRoot", - ImagesUpdatedAt: expectedAt, - ImageFiles: []string{"folder.jpg"}, - } - repo.hasOtherAudio = true - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(BeEmpty()) - }) - - It("propagates errors from the album-root check", func() { - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist/Album", - Name: "disc1", - ParentID: "albumFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - repo.parentResult = &model.Folder{ - ID: "albumFolder", - Path: "Artist", - Name: "Album", - ParentID: "artistFolder", - ImagesUpdatedAt: expectedAt, - ImageFiles: []string{"cover.jpg"}, - } - repo.otherAudioErr = errors.New("db connection failed") - - _, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).To(MatchError("db connection failed")) - }) - - It("propagates non-ErrNotFound errors from parent folder lookup", func() { - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist/Album", - Name: "CD1", - ParentID: "parentFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - { - ID: "folder2", - Path: "Artist/Album", - Name: "CD2", - ParentID: "parentFolder", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - repo.getErr = errors.New("db connection failed") - - _, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).To(MatchError("db connection failed")) - Expect(repo.getCallCount).To(Equal(1)) - }) - - It("continues gracefully when parent folder is not found", func() { - // Parent folder may have been deleted; should log a warning and continue - repo.result = []model.Folder{ - { - ID: "folder1", - Path: "Artist/Album", - Name: "CD1", - ParentID: "missingParent", - ImagesUpdatedAt: now, - ImageFiles: []string{"cover.jpg"}, - }, - { - ID: "folder2", - Path: "Artist/Album", - Name: "CD2", - ParentID: "missingParent", - ImagesUpdatedAt: now, - ImageFiles: []string{}, - }, - } - // parentResult is nil, so Get will return ErrNotFound - - _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) - - Expect(err).ToNot(HaveOccurred()) - Expect(imgFiles).To(HaveLen(1)) - Expect(imgFiles[0]).To(Equal("Artist/Album/CD1/cover.jpg")) - Expect(repo.getCallCount).To(Equal(1)) - }) - }) -}) diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go deleted file mode 100644 index 6d6d58fc5..000000000 --- a/core/artwork/reader_artist_test.go +++ /dev/null @@ -1,748 +0,0 @@ -package artwork - -import ( - "context" - "errors" - "io" - "io/fs" - "os" - "path/filepath" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("artistArtworkReader", func() { - var _ = Describe("loadArtistFolder", func() { - var ( - ctx context.Context - fds *fakeDataStore - repo *fakeFolderRepo - albums model.Albums - paths []string - now time.Time - expectedUpdTime time.Time - ) - - BeforeEach(func() { - ctx = context.Background() - DeferCleanup(stubCoreAbsolutePath()) - - now = time.Now().Truncate(time.Second) - expectedUpdTime = now.Add(5 * time.Minute) - repo = &fakeFolderRepo{ - result: []model.Folder{ - { - ImagesUpdatedAt: expectedUpdTime, - }, - }, - err: nil, - } - fds = &fakeDataStore{ - folderRepo: repo, - } - albums = model.Albums{ - {LibraryID: 1, ID: "album1", Name: "Album 1"}, - } - }) - - When("no albums provided", func() { - It("returns empty and zero time", func() { - folder, upd, err := loadArtistFolder(ctx, fds, model.Albums{}, []string{"/dummy/path"}) - Expect(err).ToNot(HaveOccurred()) - Expect(folder).To(BeEmpty()) - Expect(upd).To(BeZero()) - }) - }) - - When("artist has only one album", func() { - It("returns the parent folder", func() { - paths = []string{ - filepath.FromSlash("/music/artist/album1"), - } - folder, upd, err := loadArtistFolder(ctx, fds, albums, paths) - Expect(err).ToNot(HaveOccurred()) - Expect(folder).To(Equal(filepath.FromSlash("/music/artist"))) - Expect(upd).To(Equal(expectedUpdTime)) - }) - }) - - When("the artist have multiple albums", func() { - It("returns the common prefix for the albums paths", func() { - paths = []string{ - filepath.FromSlash("/music/library/artist/one"), - filepath.FromSlash("/music/library/artist/two"), - } - folder, upd, err := loadArtistFolder(ctx, fds, albums, paths) - Expect(err).ToNot(HaveOccurred()) - Expect(folder).To(Equal(filepath.FromSlash("/music/library/artist"))) - Expect(upd).To(Equal(expectedUpdTime)) - }) - }) - - When("the album paths contain same prefix", func() { - It("returns the common prefix", func() { - paths = []string{ - filepath.FromSlash("/music/artist/album1"), - filepath.FromSlash("/music/artist/album2"), - } - folder, upd, err := loadArtistFolder(ctx, fds, albums, paths) - Expect(err).ToNot(HaveOccurred()) - Expect(folder).To(Equal(filepath.FromSlash("/music/artist"))) - Expect(upd).To(Equal(expectedUpdTime)) - }) - }) - - When("ds.Folder().GetAll returns an error", func() { - It("returns an error", func() { - paths = []string{ - filepath.FromSlash("/music/artist/album1"), - filepath.FromSlash("/music/artist/album2"), - } - repo.err = errors.New("fake error") - folder, upd, err := loadArtistFolder(ctx, fds, albums, paths) - Expect(err).To(MatchError(ContainSubstring("fake error"))) - // Folder and time are empty on error. - Expect(folder).To(BeEmpty()) - Expect(upd).To(BeZero()) - }) - }) - }) - - var _ = Describe("fromArtistFolder", func() { - var ( - ctx context.Context - tempDir string - libFS fs.FS - testFunc sourceFunc - ) - - BeforeEach(func() { - ctx = context.Background() - tempDir = GinkgoT().TempDir() - libFS = os.DirFS(tempDir) - }) - - When("artist folder contains matching image", func() { - BeforeEach(func() { - // Create test structure: /temp/artist/artist.jpg - artistDir := filepath.Join(tempDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - artistImagePath := filepath.Join(artistDir, "artist.jpg") - Expect(os.WriteFile(artistImagePath, []byte("fake image data"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("finds and returns the image", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - Expect(path).To(ContainSubstring("artist.jpg")) - - // Verify we can read the content - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("fake image data")) - reader.Close() - }) - }) - - When("artist folder name contains glob metacharacters", func() { - BeforeEach(func() { - artistDir := filepath.Join(tempDir, "Artist [Live]") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - artistImagePath := filepath.Join(artistDir, "artist.jpg") - Expect(os.WriteFile(artistImagePath, []byte("bracketed artist image"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("treats the folder path literally when globbing through the library fs", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - Expect(path).To(ContainSubstring("Artist [Live]" + string(filepath.Separator) + "artist.jpg")) - - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("bracketed artist image")) - reader.Close() - }) - }) - - When("artist folder is empty but parent contains image", func() { - BeforeEach(func() { - // Create test structure: /temp/parent/artist.jpg and /temp/parent/artist/album/ - parentDir := filepath.Join(tempDir, "parent") - artistDir := filepath.Join(parentDir, "artist") - albumDir := filepath.Join(artistDir, "album") - Expect(os.MkdirAll(albumDir, 0755)).To(Succeed()) - - // Put artist image in parent directory - artistImagePath := filepath.Join(parentDir, "artist.jpg") - Expect(os.WriteFile(artistImagePath, []byte("parent image"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("finds image in parent directory", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - Expect(path).To(ContainSubstring("parent" + string(filepath.Separator) + "artist.jpg")) - - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("parent image")) - reader.Close() - }) - }) - - When("image is two levels up", func() { - BeforeEach(func() { - // Create test structure: /temp/grandparent/artist.jpg and /temp/grandparent/parent/artist/ - grandparentDir := filepath.Join(tempDir, "grandparent") - parentDir := filepath.Join(grandparentDir, "parent") - artistDir := filepath.Join(parentDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - // Put artist image in grandparent directory - artistImagePath := filepath.Join(grandparentDir, "artist.jpg") - Expect(os.WriteFile(artistImagePath, []byte("grandparent image"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("finds image in grandparent directory", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - Expect(path).To(ContainSubstring("grandparent" + string(filepath.Separator) + "artist.jpg")) - - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("grandparent image")) - reader.Close() - }) - }) - - When("images exist at multiple levels", func() { - BeforeEach(func() { - // Create test structure with images at multiple levels - grandparentDir := filepath.Join(tempDir, "grandparent") - parentDir := filepath.Join(grandparentDir, "parent") - artistDir := filepath.Join(parentDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - // Put artist images at all levels - Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("artist level"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(parentDir, "artist.jpg"), []byte("parent level"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(grandparentDir, "artist.jpg"), []byte("grandparent level"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("prioritizes the closest (artist folder) image", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - Expect(path).To(ContainSubstring("artist" + string(filepath.Separator) + "artist.jpg")) - - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("artist level")) - reader.Close() - }) - }) - - When("pattern matches multiple files", func() { - BeforeEach(func() { - artistDir := filepath.Join(tempDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - // Create multiple matching files - Expect(os.WriteFile(filepath.Join(artistDir, "artist.abc"), []byte("text file"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(artistDir, "artist.png"), []byte("png image"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("jpg image"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("returns the first valid image file in sorted order", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - - // Should return an image file, - // Files are sorted: jpg comes before png alphabetically. - // .abc comes first, but it's not an image. - Expect(path).To(ContainSubstring("artist.jpg")) - reader.Close() - }) - }) - - When("prioritizing files without numeric suffixes", func() { - BeforeEach(func() { - // Test case for issue #4683: artist.jpg should come before artist.1.jpg - artistDir := filepath.Join(tempDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - // Create multiple matches with and without numeric suffixes - Expect(os.WriteFile(filepath.Join(artistDir, "artist.1.jpg"), []byte("artist 1"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("artist main"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(artistDir, "artist.2.jpg"), []byte("artist 2"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("returns artist.jpg before artist.1.jpg and artist.2.jpg", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - Expect(path).To(ContainSubstring("artist.jpg")) - - // Verify it's the main file, not a numbered variant - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("artist main")) - reader.Close() - }) - }) - - When("handling case-insensitive sorting", func() { - BeforeEach(func() { - // Test case to ensure case-insensitive natural sorting - artistDir := filepath.Join(tempDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - // Create files with mixed case names - Expect(os.WriteFile(filepath.Join(artistDir, "Folder.jpg"), []byte("folder"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("artist"), 0600)).To(Succeed()) - Expect(os.WriteFile(filepath.Join(artistDir, "BACK.jpg"), []byte("back"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "*.*") - }) - - It("sorts case-insensitively", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - - // Should return artist.jpg first (case-insensitive: "artist" < "back" < "folder") - Expect(path).To(ContainSubstring("artist.jpg")) - - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("artist")) - reader.Close() - }) - }) - - When("no matching files exist anywhere", func() { - BeforeEach(func() { - artistDir := filepath.Join(tempDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - // Create non-matching files - Expect(os.WriteFile(filepath.Join(artistDir, "cover.jpg"), []byte("cover image"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("returns an error", func() { - reader, path, err := testFunc() - Expect(err).To(HaveOccurred()) - Expect(reader).To(BeNil()) - Expect(path).To(BeEmpty()) - Expect(err.Error()).To(ContainSubstring("no matches for 'artist.*'")) - Expect(err.Error()).To(ContainSubstring("parent directories")) - }) - }) - - When("directory traversal reaches filesystem root", func() { - BeforeEach(func() { - // Start from a shallow directory to test root boundary - artistDir := filepath.Join(tempDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("handles root boundary gracefully", func() { - reader, path, err := testFunc() - Expect(err).To(HaveOccurred()) - Expect(reader).To(BeNil()) - Expect(path).To(BeEmpty()) - // Should not panic or cause infinite loop - }) - }) - - When("file exists but cannot be opened", func() { - BeforeEach(func() { - artistDir := filepath.Join(tempDir, "artist") - Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) - - // Create a file that cannot be opened (permission denied) - restrictedFile := filepath.Join(artistDir, "artist.jpg") - Expect(os.WriteFile(restrictedFile, []byte("restricted"), 0600)).To(Succeed()) - - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("logs warning and continues searching", func() { - // This test depends on the ability to restrict file permissions - // For now, we'll just ensure it doesn't panic and returns appropriate error - reader, _, err := testFunc() - // The file should be readable in test environment, so this will succeed - // In a real scenario with permission issues, it would continue searching - if err == nil { - Expect(reader).ToNot(BeNil()) - reader.Close() - } - }) - }) - - When("single album artist scenario (original issue)", func() { - BeforeEach(func() { - // Simulate the exact folder structure from the issue: - // /music/artist/album1/ (single album) - // /music/artist/artist.jpg (artist image that should be found) - artistDir := filepath.Join(tempDir, "music", "artist") - albumDir := filepath.Join(artistDir, "album1") - Expect(os.MkdirAll(albumDir, 0755)).To(Succeed()) - - // Create artist.jpg in the artist folder (this was not being found before) - artistImagePath := filepath.Join(artistDir, "artist.jpg") - Expect(os.WriteFile(artistImagePath, []byte("single album artist image"), 0600)).To(Succeed()) - - // The fromArtistFolder is called with the artist folder path - testFunc = fromArtistFolder(ctx, libFS, tempDir, artistDir, "artist.*") - }) - - It("finds artist.jpg in artist folder for single album artist", func() { - reader, path, err := testFunc() - Expect(err).ToNot(HaveOccurred()) - Expect(reader).ToNot(BeNil()) - Expect(path).To(ContainSubstring("artist.jpg")) - Expect(path).To(ContainSubstring("artist")) - - // Verify the content - data, err := io.ReadAll(reader) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("single album artist image")) - reader.Close() - }) - }) - }) - - Describe("fromArtistUploadedImage", func() { - var ( - tempDir string - reader *artistReader - ) - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - tempDir = GinkgoT().TempDir() - conf.Server.DataFolder = conf.NewDir(tempDir) - - // Create the artwork/artist directory - Expect(os.MkdirAll(filepath.Join(tempDir, "artwork", "artist"), 0755)).To(Succeed()) - - reader = &artistReader{} - }) - - When("artist has an uploaded image", func() { - It("returns the uploaded image", func() { - imgPath := filepath.Join(tempDir, "artwork", "artist", "ar-1_test.jpg") - Expect(os.WriteFile(imgPath, []byte("uploaded artist image"), 0600)).To(Succeed()) - - reader.artist = model.Artist{ID: "ar-1", UploadedImage: "ar-1_test.jpg"} - sf := reader.fromArtistUploadedImage() - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - - data, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("uploaded artist image")) - r.Close() - }) - }) - - When("artist has no uploaded image", func() { - It("returns nil reader (falls through)", func() { - reader.artist = model.Artist{ID: "ar-1"} - sf := reader.fromArtistUploadedImage() - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).To(BeNil()) - Expect(path).To(BeEmpty()) - }) - }) - }) - - Describe("fromArtistImageFolder", func() { - var ( - ctx context.Context - tempDir string - ar *artistReader - ) - - BeforeEach(func() { - ctx = context.Background() - DeferCleanup(configtest.SetupConfig()) - tempDir = GinkgoT().TempDir() - ar = &artistReader{} - }) - - When("ArtistImageFolder is not configured", func() { - It("returns nil (skips)", func() { - conf.Server.ArtistImageFolder = "" - ar.artist = model.Artist{Name: "Test Artist"} - sf := ar.fromArtistImageFolder(ctx) - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).To(BeNil()) - Expect(path).To(BeEmpty()) - }) - }) - - When("image exists matching MBID", func() { - It("finds the image by MBID", func() { - conf.Server.ArtistImageFolder = tempDir - mbid := "f27ec8db-af05-4f36-916e-3d57f91ecf5e" - imgPath := filepath.Join(tempDir, mbid+".jpg") - Expect(os.WriteFile(imgPath, []byte("mbid image"), 0600)).To(Succeed()) - - ar.artist = model.Artist{Name: "Test Artist", MbzArtistID: mbid} - sf := ar.fromArtistImageFolder(ctx) - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - - data, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("mbid image")) - r.Close() - }) - }) - - When("MBID match is case-insensitive", func() { - It("finds the image regardless of case", func() { - conf.Server.ArtistImageFolder = tempDir - mbid := "F27EC8DB-AF05-4F36-916E-3D57F91ECF5E" - imgPath := filepath.Join(tempDir, "f27ec8db-af05-4f36-916e-3d57f91ecf5e.png") - Expect(os.WriteFile(imgPath, []byte("mbid case image"), 0600)).To(Succeed()) - - ar.artist = model.Artist{Name: "Test Artist", MbzArtistID: mbid} - sf := ar.fromArtistImageFolder(ctx) - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - r.Close() - }) - }) - - When("no MBID file exists but artist name file does", func() { - It("falls back to artist name match", func() { - conf.Server.ArtistImageFolder = tempDir - imgPath := filepath.Join(tempDir, "Test Artist.jpg") - Expect(os.WriteFile(imgPath, []byte("name image"), 0600)).To(Succeed()) - - ar.artist = model.Artist{Name: "Test Artist", MbzArtistID: "nonexistent-mbid"} - sf := ar.fromArtistImageFolder(ctx) - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - - data, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("name image")) - r.Close() - }) - }) - - When("artist name match is case-insensitive", func() { - It("matches regardless of case", func() { - conf.Server.ArtistImageFolder = tempDir - imgPath := filepath.Join(tempDir, "test artist.jpg") - Expect(os.WriteFile(imgPath, []byte("case insensitive"), 0600)).To(Succeed()) - - ar.artist = model.Artist{Name: "Test Artist"} - sf := ar.fromArtistImageFolder(ctx) - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - r.Close() - }) - }) - - When("both MBID and name files exist", func() { - It("prefers MBID over name match", func() { - conf.Server.ArtistImageFolder = tempDir - mbid := "f27ec8db-af05-4f36-916e-3d57f91ecf5e" - mbidPath := filepath.Join(tempDir, mbid+".jpg") - namePath := filepath.Join(tempDir, "Test Artist.jpg") - Expect(os.WriteFile(mbidPath, []byte("mbid image"), 0600)).To(Succeed()) - Expect(os.WriteFile(namePath, []byte("name image"), 0600)).To(Succeed()) - - ar.artist = model.Artist{Name: "Test Artist", MbzArtistID: mbid} - sf := ar.fromArtistImageFolder(ctx) - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(mbidPath)) - - data, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("mbid image")) - r.Close() - }) - }) - - When("no matching image found", func() { - It("returns an error", func() { - conf.Server.ArtistImageFolder = tempDir - // Create an unrelated file - Expect(os.WriteFile(filepath.Join(tempDir, "other.jpg"), []byte("other"), 0600)).To(Succeed()) - - ar.artist = model.Artist{Name: "Test Artist"} - sf := ar.fromArtistImageFolder(ctx) - r, _, err := sf() - Expect(err).To(HaveOccurred()) - Expect(r).To(BeNil()) - Expect(err.Error()).To(ContainSubstring("no image found")) - }) - }) - - When("cached imgFolderImgPath is set", func() { - It("uses cached path instead of scanning", func() { - conf.Server.ArtistImageFolder = tempDir - imgPath := filepath.Join(tempDir, "cached.jpg") - Expect(os.WriteFile(imgPath, []byte("cached image"), 0600)).To(Succeed()) - - ar.artist = model.Artist{Name: "Test Artist"} - ar.imgFolderImgPath = imgPath - sf := ar.fromArtistImageFolder(ctx) - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - - data, err := io.ReadAll(r) - Expect(err).ToNot(HaveOccurred()) - Expect(string(data)).To(Equal("cached image")) - r.Close() - }) - }) - }) - - Describe("findImageInArtistFolder", func() { - var tempDir string - - BeforeEach(func() { - tempDir = GinkgoT().TempDir() - }) - - When("matching file exists by MBID", func() { - It("returns the file path", func() { - mbid := "f27ec8db-af05-4f36-916e-3d57f91ecf5e" - imgPath := filepath.Join(tempDir, mbid+".jpg") - Expect(os.WriteFile(imgPath, []byte("image"), 0600)).To(Succeed()) - - path := findImageInArtistFolder(tempDir, mbid, "Test") - Expect(path).To(Equal(imgPath)) - }) - }) - - When("matching file exists by name", func() { - It("returns the file path", func() { - imgPath := filepath.Join(tempDir, "Test Artist.png") - Expect(os.WriteFile(imgPath, []byte("image"), 0600)).To(Succeed()) - - path := findImageInArtistFolder(tempDir, "", "Test Artist") - Expect(path).To(Equal(imgPath)) - }) - }) - - When("no matching file exists", func() { - It("returns empty string", func() { - path := findImageInArtistFolder(tempDir, "", "Unknown Artist") - Expect(path).To(BeEmpty()) - }) - }) - - When("folder does not exist", func() { - It("returns empty string", func() { - path := findImageInArtistFolder("/nonexistent/path", "", "Test") - Expect(path).To(BeEmpty()) - }) - }) - }) -}) - -type fakeFolderRepo struct { - model.FolderRepository - result []model.Folder - parentResult *model.Folder - getErr error - getCallCount int - err error - // hasOtherAudio is returned by HasAudioOutsideFolders (the album-root - // check). False means the parent qualifies as an album root. - hasOtherAudio bool - otherAudioErr error -} - -func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { - return f.result, f.err -} - -func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, error) { - return f.hasOtherAudio, f.otherAudioErr -} - -func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) { - f.getCallCount++ - if f.getErr != nil { - return nil, f.getErr - } - if f.parentResult != nil { - return f.parentResult, nil - } - return nil, model.ErrNotFound -} - -type fakeDataStore struct { - model.DataStore - folderRepo *fakeFolderRepo -} - -func (fds *fakeDataStore) Folder(_ context.Context) model.FolderRepository { - return fds.folderRepo -} - -func stubCoreAbsolutePath() func() { - // Override core.AbsolutePath to return a fixed string during tests. - original := core.AbsolutePath - core.AbsolutePath = func(_ context.Context, ds model.DataStore, libID int, p string) string { - return filepath.FromSlash("/music") - } - return func() { - core.AbsolutePath = original - } -} diff --git a/core/artwork/reader_mediafile.go b/core/artwork/reader_mediafile.go deleted file mode 100644 index eac3c5e70..000000000 --- a/core/artwork/reader_mediafile.go +++ /dev/null @@ -1,82 +0,0 @@ -package artwork - -import ( - "context" - "fmt" - "io" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/model" -) - -type mediafileArtworkReader struct { - cacheKey - a *artwork - mediafile model.MediaFile - album model.Album - lib libraryView -} - -func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID model.ArtworkID) (*mediafileArtworkReader, error) { - mf, err := artwork.ds.MediaFile(ctx).Get(artID.ID) - if err != nil { - return nil, err - } - al, err := artwork.ds.Album(ctx).Get(mf.AlbumID) - if err != nil { - return nil, err - } - _, _, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, *al) - if err != nil { - return nil, err - } - lib, err := loadLibraryView(ctx, artwork.ds, mf.LibraryID) - if err != nil { - return nil, err - } - a := &mediafileArtworkReader{ - a: artwork, - mediafile: *mf, - album: *al, - lib: lib, - } - a.cacheKey.artID = artID - a.cacheKey.lastUpdate = mf.UpdatedAt - if al.UpdatedAt.After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = al.UpdatedAt - } - if imagesUpdatedAt != nil && imagesUpdatedAt.After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = *imagesUpdatedAt - } - return a, nil -} - -func (a *mediafileArtworkReader) Key() string { - return fmt.Sprintf( - "%s.%t", - a.cacheKey.Key(), - conf.Server.EnableMediaFileCoverArt, - ) -} -func (a *mediafileArtworkReader) LastUpdated() time.Time { - return a.lastUpdate -} - -func (a *mediafileArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { - var ff []sourceFunc - if a.mediafile.CoverArtID().Kind == model.KindMediaFileArtwork { - ff = []sourceFunc{ - fromTag(ctx, a.lib.FS, a.mediafile.Path), - fromFFmpegTag(ctx, a.a.ffmpeg, a.lib.Abs(a.mediafile.Path)), - } - } - // For multi-disc albums, fall back to disc artwork first; for single-disc albums, - // skip disc resolution (it would just fall through to album art anyway). - if len(a.album.Discs) > 1 { - ff = append(ff, fromAlbum(ctx, a.a, a.mediafile.DiscCoverArtID())) - } else { - ff = append(ff, fromAlbum(ctx, a.a, a.mediafile.AlbumCoverArtID())) - } - return selectImageReader(ctx, a.artID, ff...) -} diff --git a/core/artwork/reader_playlist.go b/core/artwork/reader_playlist.go deleted file mode 100644 index 09707843d..000000000 --- a/core/artwork/reader_playlist.go +++ /dev/null @@ -1,269 +0,0 @@ -package artwork - -import ( - "bytes" - "context" - "errors" - "image" - "image/draw" - "image/png" - "io" - "net/url" - "os" - "path/filepath" - "strings" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils/slice" - xdraw "golang.org/x/image/draw" -) - -type playlistArtworkReader struct { - cacheKey - a *artwork - pl model.Playlist -} - -const tileSize = 600 - -func newPlaylistArtworkReader(ctx context.Context, artwork *artwork, artID model.ArtworkID) (*playlistArtworkReader, error) { - pl, err := artwork.ds.Playlist(ctx).Get(artID.ID) - if err != nil { - return nil, err - } - a := &playlistArtworkReader{ - a: artwork, - pl: *pl, - } - a.cacheKey.artID = artID - a.cacheKey.lastUpdate = pl.UpdatedAt - - // Check sidecar and ExternalImageURL local file ModTimes for cache invalidation. - // If either is newer than the playlist's UpdatedAt, use that instead so the - // cache is busted when a user replaces a sidecar image or local file reference. - for _, path := range []string{ - findPlaylistSidecarPath(ctx, pl.Path), - pl.ExternalImageURL, - } { - if path == "" || strings.HasPrefix(path, "http://") || strings.HasPrefix(path, "https://") { - continue - } - if info, err := os.Stat(path); err == nil { - if info.ModTime().After(a.cacheKey.lastUpdate) { - a.cacheKey.lastUpdate = info.ModTime() - } - } - } - - return a, nil -} - -func (a *playlistArtworkReader) LastUpdated() time.Time { - return a.lastUpdate -} - -func (a *playlistArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { - return selectImageReader(ctx, a.artID, - a.fromPlaylistUploadedImage(), - a.fromPlaylistSidecar(ctx), - a.fromPlaylistExternalImage(ctx), - a.fromGeneratedTiledCover(ctx), - fromAlbumPlaceholder(), - ) -} - -func (a *playlistArtworkReader) fromPlaylistUploadedImage() sourceFunc { - return fromLocalFile(a.pl.UploadedImagePath()) -} - -func (a *playlistArtworkReader) fromPlaylistSidecar(ctx context.Context) sourceFunc { - return fromLocalFile(findPlaylistSidecarPath(ctx, a.pl.Path)) -} - -func (a *playlistArtworkReader) fromPlaylistExternalImage(ctx context.Context) sourceFunc { - return func() (io.ReadCloser, string, error) { - imgURL := a.pl.ExternalImageURL - if imgURL == "" { - return nil, "", nil - } - parsed, err := url.Parse(imgURL) - if err != nil { - return nil, "", err - } - if parsed.Scheme == "http" || parsed.Scheme == "https" { - if !conf.Server.EnableM3UExternalAlbumArt { - return nil, "", nil - } - return fromURL(ctx, parsed) - } - return fromLocalFile(imgURL)() - } -} - -// fromLocalFile returns a sourceFunc that opens the given local path. -// Returns (nil, "", nil) if path is empty — signalling "not found, try next source". -func fromLocalFile(path string) sourceFunc { - return func() (io.ReadCloser, string, error) { - if path == "" { - return nil, "", nil - } - f, err := os.Open(path) - if err != nil { - return nil, "", err - } - return f, path, nil - } -} - -// findPlaylistSidecarPath scans the directory of the playlist file for a sidecar -// image file with the same base name (case-insensitive). Returns empty string if -// no matching image is found or if plsPath is empty. -func findPlaylistSidecarPath(ctx context.Context, plsPath string) string { - if plsPath == "" { - return "" - } - dir := filepath.Dir(plsPath) - base := strings.TrimSuffix(filepath.Base(plsPath), filepath.Ext(plsPath)) - - entries, err := os.ReadDir(dir) - if err != nil { - log.Warn(ctx, "Could not read directory for playlist sidecar", "dir", dir, err) - return "" - } - for _, entry := range entries { - name := entry.Name() - nameBase := strings.TrimSuffix(name, filepath.Ext(name)) - if !entry.IsDir() && strings.EqualFold(nameBase, base) && model.IsImageFile(name) { - return filepath.Join(dir, name) - } - } - return "" -} - -func (a *playlistArtworkReader) fromGeneratedTiledCover(ctx context.Context) sourceFunc { - return func() (io.ReadCloser, string, error) { - tiles, err := a.loadTiles(ctx) - if err != nil { - return nil, "", err - } - r, err := a.createTiledImage(ctx, tiles) - return r, "", err - } -} - -func toAlbumArtworkIDs(albumIDs []string) []model.ArtworkID { - return slice.Map(albumIDs, func(id string) model.ArtworkID { - al := model.Album{ID: id} - return al.CoverArtID() - }) -} - -func (a *playlistArtworkReader) loadTiles(ctx context.Context) ([]image.Image, error) { - tracksRepo := a.a.ds.Playlist(ctx).Tracks(a.pl.ID, false) - albumIds, err := tracksRepo.GetAlbumIDs(model.QueryOptions{Max: 4, Sort: "random()"}) - if err != nil { - log.Error(ctx, "Error getting album IDs for playlist", "id", a.pl.ID, "name", a.pl.Name, err) - return nil, err - } - ids := toAlbumArtworkIDs(albumIds) - - var tiles []image.Image - for _, id := range ids { - r, _, err := fromAlbum(ctx, a.a, id)() - if err == nil { - tile, err := a.createTile(ctx, r) - if err == nil { - tiles = append(tiles, tile) - } - _ = r.Close() - } - if len(tiles) == 4 { - break - } - } - switch len(tiles) { - case 0: - return nil, errors.New("could not find any eligible cover") - case 2: - tiles = append(tiles, tiles[1], tiles[0]) - case 3: - tiles = append(tiles, tiles[0]) - } - return tiles, nil -} - -func (a *playlistArtworkReader) createTile(_ context.Context, r io.ReadCloser) (image.Image, error) { - img, _, err := image.Decode(r) - if err != nil { - return nil, err - } - return fillCenter(img, tileSize/2, tileSize/2), nil -} - -func (a *playlistArtworkReader) createTiledImage(_ context.Context, tiles []image.Image) (io.ReadCloser, error) { - buf := new(bytes.Buffer) - var rgba draw.Image - var err error - if len(tiles) == 4 { - rgba = image.NewRGBA(image.Rectangle{Max: image.Point{X: tileSize - 1, Y: tileSize - 1}}) - draw.Draw(rgba, rect(0), tiles[0], image.Point{}, draw.Src) - draw.Draw(rgba, rect(1), tiles[1], image.Point{}, draw.Src) - draw.Draw(rgba, rect(2), tiles[2], image.Point{}, draw.Src) - draw.Draw(rgba, rect(3), tiles[3], image.Point{}, draw.Src) - err = png.Encode(buf, rgba) - } else { - err = png.Encode(buf, tiles[0]) - } - if err != nil { - return nil, err - } - return io.NopCloser(buf), nil -} - -func rect(pos int) image.Rectangle { - r := image.Rectangle{} - switch pos { - case 1: - r.Min.X = tileSize / 2 - case 2: - r.Min.Y = tileSize / 2 - case 3: - r.Min.X = tileSize / 2 - r.Min.Y = tileSize / 2 - } - r.Max.X = r.Min.X + tileSize/2 - r.Max.Y = r.Min.Y + tileSize/2 - return r -} - -// fillCenter crops the source image from the center and scales it to fill dstW x dstH exactly, -// equivalent to imaging.Fill with Center anchor. -func fillCenter(src image.Image, dstW, dstH int) image.Image { - srcBounds := src.Bounds() - srcW := srcBounds.Dx() - srcH := srcBounds.Dy() - - // Calculate crop rectangle (center crop to match destination aspect ratio) - srcAspect := float64(srcW) / float64(srcH) - dstAspect := float64(dstW) / float64(dstH) - - var cropRect image.Rectangle - if srcAspect > dstAspect { - // Source is wider — crop horizontally - cropW := int(float64(srcH) * dstAspect) - cropX := (srcW - cropW) / 2 - cropRect = image.Rect(srcBounds.Min.X+cropX, srcBounds.Min.Y, srcBounds.Min.X+cropX+cropW, srcBounds.Max.Y) - } else { - // Source is taller — crop vertically - cropH := int(float64(srcW) / dstAspect) - cropY := (srcH - cropH) / 2 - cropRect = image.Rect(srcBounds.Min.X, srcBounds.Min.Y+cropY, srcBounds.Max.X, srcBounds.Min.Y+cropY+cropH) - } - - dst := image.NewNRGBA(image.Rect(0, 0, dstW, dstH)) - xdraw.CatmullRom.Scale(dst, dst.Bounds(), src, cropRect, draw.Src, nil) - return dst -} diff --git a/core/artwork/reader_radio.go b/core/artwork/reader_radio.go deleted file mode 100644 index 22db6e302..000000000 --- a/core/artwork/reader_radio.go +++ /dev/null @@ -1,40 +0,0 @@ -package artwork - -import ( - "context" - "io" - "time" - - "github.com/navidrome/navidrome/model" -) - -type radioArtworkReader struct { - cacheKey - a *artwork - radio model.Radio -} - -func newRadioArtworkReader(ctx context.Context, artwork *artwork, artID model.ArtworkID) (*radioArtworkReader, error) { - r, err := artwork.ds.Radio(ctx).Get(artID.ID) - if err != nil { - return nil, err - } - a := &radioArtworkReader{a: artwork, radio: *r} - a.cacheKey.artID = artID - a.cacheKey.lastUpdate = r.UpdatedAt - return a, nil -} - -func (a *radioArtworkReader) LastUpdated() time.Time { - return a.lastUpdate -} - -func (a *radioArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { - return selectImageReader(ctx, a.artID, - a.fromRadioUploadedImage(), - ) -} - -func (a *radioArtworkReader) fromRadioUploadedImage() sourceFunc { - return fromLocalFile(a.radio.UploadedImagePath()) -} diff --git a/core/artwork/reader_radio_test.go b/core/artwork/reader_radio_test.go deleted file mode 100644 index 37ce1d827..000000000 --- a/core/artwork/reader_radio_test.go +++ /dev/null @@ -1,84 +0,0 @@ -package artwork - -import ( - "context" - "os" - "path/filepath" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/model" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("radioArtworkReader", func() { - var ( - tempDir string - reader *radioArtworkReader - ) - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - tempDir = GinkgoT().TempDir() - conf.Server.DataFolder = conf.NewDir(tempDir) - - Expect(os.MkdirAll(filepath.Join(tempDir, "artwork", "radio"), 0755)).To(Succeed()) - - reader = &radioArtworkReader{} - }) - - Describe("fromRadioUploadedImage", func() { - When("radio has an uploaded image", func() { - It("returns the uploaded image", func() { - imgPath := filepath.Join(tempDir, "artwork", "radio", "rd-1_test.jpg") - Expect(os.WriteFile(imgPath, []byte("uploaded radio image"), 0600)).To(Succeed()) - - reader.radio = model.Radio{ID: "rd-1", UploadedImage: "rd-1_test.jpg"} - sf := reader.fromRadioUploadedImage() - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - Expect(path).To(Equal(imgPath)) - r.Close() - }) - }) - - When("radio has no uploaded image", func() { - It("returns nil reader (falls through)", func() { - reader.radio = model.Radio{ID: "rd-1"} - sf := reader.fromRadioUploadedImage() - r, path, err := sf() - Expect(err).ToNot(HaveOccurred()) - Expect(r).To(BeNil()) - Expect(path).To(BeEmpty()) - }) - }) - }) - - Describe("Reader", func() { - When("radio has an uploaded image", func() { - It("returns the image reader", func() { - imgPath := filepath.Join(tempDir, "artwork", "radio", "rd-1_test.jpg") - Expect(os.WriteFile(imgPath, []byte("uploaded radio image"), 0600)).To(Succeed()) - - reader.radio = model.Radio{ID: "rd-1", UploadedImage: "rd-1_test.jpg"} - reader.cacheKey.artID = model.ArtworkID{Kind: model.KindRadioArtwork, ID: "rd-1"} - r, _, err := reader.Reader(context.Background()) - Expect(err).ToNot(HaveOccurred()) - Expect(r).ToNot(BeNil()) - r.Close() - }) - }) - - When("radio has no uploaded image", func() { - It("returns ErrUnavailable", func() { - reader.radio = model.Radio{ID: "rd-1"} - reader.cacheKey.artID = model.ArtworkID{Kind: model.KindRadioArtwork, ID: "rd-1"} - r, _, err := reader.Reader(context.Background()) - Expect(err).To(MatchError(ErrUnavailable)) - Expect(r).To(BeNil()) - }) - }) - }) -}) diff --git a/core/artwork/reader_resized.go b/core/artwork/reader_resized.go deleted file mode 100644 index 7d920e790..000000000 --- a/core/artwork/reader_resized.go +++ /dev/null @@ -1,87 +0,0 @@ -package artwork - -import ( - "context" - "fmt" - "io" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" -) - -type resizedArtworkReader struct { - artID model.ArtworkID - cacheKey string - lastUpdate time.Time - size int - square bool - a *artwork -} - -func resizedFromOriginal(ctx context.Context, a *artwork, artID model.ArtworkID, size int, square bool) (*resizedArtworkReader, error) { - r := &resizedArtworkReader{a: a} - r.artID = artID - r.size = size - r.square = square - - // Get lastUpdated and cacheKey from original artwork - original, err := a.getArtworkReader(ctx, artID, 0, false) - if err != nil { - return nil, err - } - r.cacheKey = original.Key() - r.lastUpdate = original.LastUpdated() - return r, nil -} - -func (a *resizedArtworkReader) Key() string { - baseKey := fmt.Sprintf("%s.%d", a.cacheKey, a.size) - if a.square { - return baseKey + ".square" - } - return fmt.Sprintf("%s.%d", baseKey, conf.Server.CoverArtQuality) -} - -func (a *resizedArtworkReader) LastUpdated() time.Time { - return a.lastUpdate -} - -func (a *resizedArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { - // Get artwork in original size, possibly from cache - orig, _, err := a.a.Get(ctx, a.artID, 0, false) - if err != nil { - return nil, "", err - } - defer orig.Close() - - resized, origSize, err := a.resizeImage(ctx, orig) - if resized == nil { - log.Trace(ctx, "Image smaller than requested size", "artID", a.artID, "original", origSize, "resized", a.size, "square", a.square) - } else { - log.Trace(ctx, "Resizing artwork", "artID", a.artID, "original", origSize, "resized", a.size, "square", a.square) - } - if err != nil { - log.Warn(ctx, "Could not resize image. Will return image as is", "artID", a.artID, "size", a.size, "square", a.square, err) - } - if err != nil || resized == nil { - // if we couldn't resize the image, return the original - orig, _, err = a.a.Get(ctx, a.artID, 0, false) - return orig, "", err - } - // Preserve ReadCloser semantics if the resized reader already supports Close - // (e.g., ffmpeg pipe), otherwise wrap with NopCloser - if rc, ok := resized.(io.ReadCloser); ok { - return rc, fmt.Sprintf("%s@%d", a.artID, a.size), nil - } - return io.NopCloser(resized), fmt.Sprintf("%s@%d", a.artID, a.size), nil -} - -func (a *resizedArtworkReader) resizeImage(ctx context.Context, reader io.Reader) (io.Reader, int, error) { - data, err := io.ReadAll(reader) - if err != nil { - return nil, 0, fmt.Errorf("reading image data: %w", err) - } - return resizeImageData(ctx, a.a.ffmpeg, data, a.size, a.square) -} diff --git a/core/artwork/reader_resized_test.go b/core/artwork/reader_resized_test.go deleted file mode 100644 index 7c14f5e44..000000000 --- a/core/artwork/reader_resized_test.go +++ /dev/null @@ -1,176 +0,0 @@ -package artwork - -import ( - "bytes" - "context" - "errors" - "io" - - "github.com/navidrome/navidrome/core/ffmpeg" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("resizeImage", func() { - var mockFF *tests.MockFFmpeg - var r *resizedArtworkReader - - BeforeEach(func() { - mockFF = tests.NewMockFFmpeg("converted-animated-data") - r = &resizedArtworkReader{ - size: 300, - square: false, - a: &artwork{ffmpeg: mockFF}, - } - }) - - Describe("animated GIF handling", func() { - It("converts animated GIF via ffmpeg when available", func() { - data := createAnimatedGIF(3) - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - Expect(err).ToNot(HaveOccurred()) - Expect(result).ToNot(BeNil()) - - // Should have been processed by ffmpeg (mock returns "converted-animated-data") - output, err := io.ReadAll(result) - Expect(err).ToNot(HaveOccurred()) - Expect(output).To(Equal(data)) // MockFFmpeg echoes input back - }) - - It("falls back to static resize when ffmpeg fails for animated GIF", func() { - mockFF.Error = errors.New("ffmpeg failed") - // Use size smaller than image so static resize actually produces output - r.size = 1 - data := createAnimatedGIF(3) - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - // Should fall through to static resize successfully (no ffmpeg error propagated) - Expect(err).ToNot(HaveOccurred()) - Expect(result).ToNot(BeNil()) - - // Verify it's a static image (WebP encoded), not the ffmpeg error - output, err := io.ReadAll(result) - Expect(err).ToNot(HaveOccurred()) - Expect(len(output)).To(BeNumerically(">", 0)) - }) - - It("preserves animation for square thumbnails with animated GIF", func() { - r.square = true - data := createAnimatedGIF(3) - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - Expect(err).ToNot(HaveOccurred()) - Expect(result).ToNot(BeNil()) - - // Should have been processed by ffmpeg (mock returns input data) - output, err := io.ReadAll(result) - Expect(err).ToNot(HaveOccurred()) - Expect(output).To(Equal(data)) - }) - }) - - Describe("animated WebP handling", func() { - It("returns animated WebP data as-is when not square", func() { - data := createAnimatedWebPBytes() - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - Expect(err).ToNot(HaveOccurred()) - Expect(result).ToNot(BeNil()) - - // Should return original data unchanged - output, err := io.ReadAll(result) - Expect(err).ToNot(HaveOccurred()) - Expect(output).To(Equal(data)) - }) - - It("preserves animated WebP for square thumbnails", func() { - r.square = true - data := createAnimatedWebPBytes() - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - Expect(err).ToNot(HaveOccurred()) - Expect(result).ToNot(BeNil()) - - // Should return original data unchanged - output, err := io.ReadAll(result) - Expect(err).ToNot(HaveOccurred()) - Expect(output).To(Equal(data)) - }) - }) - - Describe("animated PNG handling", func() { - It("returns animated PNG data as-is when not square", func() { - data := createAPNGBytes() - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - Expect(err).ToNot(HaveOccurred()) - Expect(result).ToNot(BeNil()) - - // Should return original data unchanged - output, err := io.ReadAll(result) - Expect(err).ToNot(HaveOccurred()) - Expect(output).To(Equal(data)) - }) - - It("preserves animated PNG for square thumbnails", func() { - r.square = true - data := createAPNGBytes() - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - Expect(err).ToNot(HaveOccurred()) - Expect(result).ToNot(BeNil()) - - // Should return original data unchanged - output, err := io.ReadAll(result) - Expect(err).ToNot(HaveOccurred()) - Expect(output).To(Equal(data)) - }) - }) - - Describe("static image handling", func() { - It("resizes a static PNG normally", func() { - data := createStaticPNGBytes() - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - // Static PNG is 2x2, size 300 is larger, so should return nil (no upscale) - Expect(err).ToNot(HaveOccurred()) - Expect(result).To(BeNil()) - }) - }) - - Describe("ReadCloser preservation", func() { - It("preserves Close semantics from ffmpeg ReadCloser", func() { - // Create a trackable ReadCloser - tracker := &closeTracker{Reader: bytes.NewReader([]byte("test data"))} - mockFF2 := &mockFFmpegWithCloser{tracker: tracker} - r.a = &artwork{ffmpeg: mockFF2} - - data := createAnimatedGIF(3) - result, _, err := r.resizeImage(context.Background(), bytes.NewReader(data)) - Expect(err).ToNot(HaveOccurred()) - - // The result should be an io.ReadCloser (the tracker) - rc, ok := result.(io.ReadCloser) - Expect(ok).To(BeTrue()) - Expect(rc.Close()).ToNot(HaveOccurred()) - Expect(tracker.closed).To(BeTrue()) - }) - }) -}) - -// closeTracker is an io.ReadCloser that tracks whether Close was called. -type closeTracker struct { - io.Reader - closed bool -} - -func (c *closeTracker) Close() error { - c.closed = true - return nil -} - -// mockFFmpegWithCloser is a minimal FFmpeg mock that returns a specific ReadCloser -// for ConvertAnimatedImage, allowing us to verify Close propagation. -type mockFFmpegWithCloser struct { - ffmpeg.FFmpeg - tracker *closeTracker -} - -func (m *mockFFmpegWithCloser) IsAvailable() bool { return true } -func (m *mockFFmpegWithCloser) ConvertAnimatedImage(_ context.Context, _ io.Reader, _ int, _ int) (io.ReadCloser, error) { - return m.tracker, nil -} diff --git a/core/artwork/serving.go b/core/artwork/serving.go index 7bfc46c81..a2f9c57ef 100644 --- a/core/artwork/serving.go +++ b/core/artwork/serving.go @@ -18,6 +18,8 @@ import ( "github.com/navidrome/navidrome/utils/cache" ) +var ErrUnavailable = errors.New("artwork unavailable") + // errStaleSource signals that a backing file's mtime no longer matches the state // row's RefMtime: the stored hash may be stale, so the load is aborted (dangling). var errStaleSource = errors.New("artwork: source file changed since resolution") @@ -254,7 +256,7 @@ func (s *service) provisionalEmbedded(ctx context.Context, artID model.ArtworkID // serveDisc serves disc-level artwork as a pure provisional read-through: no state rows, // no enqueue. It tries the disc-folder selection chain and falls back to the album cover. func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) { - dr, err := newDiscArtworkReader(ctx, &artwork{ds: s.ds, ffmpeg: s.ffmpeg}, artID) + dr, err := newDiscArtworkReader(ctx, s.ds, artID) if err != nil { return nil, err } diff --git a/core/artwork/sources.go b/core/artwork/sources.go index 04a9257fb..9e9916e3e 100644 --- a/core/artwork/sources.go +++ b/core/artwork/sources.go @@ -16,11 +16,9 @@ import ( "time" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/resources" "go.senan.xyz/taglib" ) @@ -171,44 +169,6 @@ type readCloser struct { io.Closer } -func fromAlbum(ctx context.Context, a *artwork, id model.ArtworkID) sourceFunc { - return func() (io.ReadCloser, string, error) { - r, _, err := a.Get(ctx, id, 0, false) - if err != nil { - return nil, "", err - } - return r, id.String(), nil - } -} - -func fromAlbumPlaceholder() sourceFunc { - return func() (io.ReadCloser, string, error) { - r, _ := resources.FS().Open(consts.PlaceholderAlbumArt) - return r, consts.PlaceholderAlbumArt, nil - } -} -func fromArtistExternalSource(ctx context.Context, ar model.Artist, provider external.Provider) sourceFunc { - return func() (io.ReadCloser, string, error) { - imageUrl, err := provider.ArtistImage(ctx, ar.ID) - if err != nil { - return nil, "", err - } - - return fromURL(ctx, imageUrl) - } -} - -func fromAlbumExternalSource(ctx context.Context, al model.Album, provider external.Provider) sourceFunc { - return func() (io.ReadCloser, string, error) { - imageUrl, err := provider.AlbumImage(ctx, al.ID) - if err != nil { - return nil, "", err - } - - return fromURL(ctx, imageUrl) - } -} - func fromURL(ctx context.Context, imageUrl *url.URL) (io.ReadCloser, string, error) { hc := http.Client{Timeout: 5 * time.Second} req, _ := http.NewRequestWithContext(ctx, http.MethodGet, imageUrl.String(), nil) diff --git a/core/artwork/testhelpers_test.go b/core/artwork/testhelpers_test.go new file mode 100644 index 000000000..97f69136a --- /dev/null +++ b/core/artwork/testhelpers_test.go @@ -0,0 +1,37 @@ +package artwork + +import ( + "github.com/navidrome/navidrome/model" +) + +type fakeFolderRepo struct { + model.FolderRepository + result []model.Folder + parentResult *model.Folder + getErr error + getCallCount int + err error + // hasOtherAudio is returned by HasAudioOutsideFolders (the album-root + // check). False means the parent qualifies as an album root. + hasOtherAudio bool + otherAudioErr error +} + +func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { + return f.result, f.err +} + +func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, error) { + return f.hasOtherAudio, f.otherAudioErr +} + +func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) { + f.getCallCount++ + if f.getErr != nil { + return nil, f.getErr + } + if f.parentResult != nil { + return f.parentResult, nil + } + return nil, model.ErrNotFound +} diff --git a/core/artwork/wire_providers.go b/core/artwork/wire_providers.go index 2c93c27e9..8efb30da2 100644 --- a/core/artwork/wire_providers.go +++ b/core/artwork/wire_providers.go @@ -5,10 +5,8 @@ import ( ) var Set = wire.NewSet( - NewArtwork, NewService, GetImageCache, - NewCacheWarmer, NewWorker, ProvideImageStore, ) diff --git a/core/external/provider.go b/core/external/provider.go index 42da7eb45..0a61947a5 100644 --- a/core/external/provider.go +++ b/core/external/provider.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "net/url" "sort" "strings" "time" @@ -35,10 +34,6 @@ type Provider interface { UpdateArtistInfo(ctx context.Context, id string, count int, includeNotPresent bool) (*model.Artist, error) SimilarSongs(ctx context.Context, id string, count int) (model.MediaFiles, error) TopSongs(ctx context.Context, artist string, count int) (model.MediaFiles, error) - ArtistImage(ctx context.Context, id string) (*url.URL, error) - // ArtistImageResult is like ArtistImage but reports a transient agent failure as a real error, not ErrNotFound. - ArtistImageResult(ctx context.Context, id string) (*url.URL, error) - AlbumImage(ctx context.Context, id string) (*url.URL, error) } type provider struct { @@ -372,94 +367,6 @@ func (e *provider) similarSongsFallback(ctx context.Context, id string, count in return similarSongs, nil } -func (e *provider) ArtistImage(ctx context.Context, id string) (*url.URL, error) { - u, _, err := e.artistImage(ctx, id) - return u, err -} - -// ArtistImageResult is like ArtistImage but surfaces a transient agent failure as the -// real error, so an agent outage is not mistaken for a definitive no-image (ErrNotFound). -func (e *provider) ArtistImageResult(ctx context.Context, id string) (*url.URL, error) { - u, agentErr, err := e.artistImage(ctx, id) - if agentErr != nil && errors.Is(err, model.ErrNotFound) { - return nil, agentErr - } - return u, err -} - -// artistImage returns the agent error (agentErr) separately from the caller-facing err, -// so ArtistImageResult can tell "agent errored" apart from "definitively no image". -func (e *provider) artistImage(ctx context.Context, id string) (u *url.URL, agentErr error, err error) { - artist, err := e.getArtist(ctx, id) - if err != nil { - return nil, nil, err - } - - imageUrl := artist.ArtistImageUrl() - if imageUrl == "" { - // No cached URL — must fetch from external source synchronously - agentErr = e.callGetImage(ctx, e.ag, &artist) - if utils.IsCtxDone(ctx) { - log.Warn(ctx, "ArtistImage call canceled", ctx.Err()) - return nil, agentErr, ctx.Err() - } - imageUrl = artist.ArtistImageUrl() - } else { - // If cached info is expired, enqueue a background refresh so that config changes - // (e.g. disabling an agent) take effect without waiting for a full artist info refresh. - updatedAt := V(artist.ExternalInfoUpdatedAt) - if !updatedAt.IsZero() && time.Since(updatedAt) > conf.Server.DevArtistInfoTimeToLive { - log.Debug(ctx, "Artist image info expired, enqueuing background refresh", "artist", artist.Name(), "updatedAt", updatedAt) - e.artistQueue.enqueue(&artist) - } - } - - if imageUrl == "" { - return nil, agentErr, model.ErrNotFound - } - u, err = url.Parse(imageUrl) - return u, agentErr, err -} - -func (e *provider) AlbumImage(ctx context.Context, id string) (*url.URL, error) { - album, err := e.getAlbum(ctx, id) - if err != nil { - return nil, err - } - - albumName := album.Name() - images, err := e.ag.GetAlbumImages(ctx, albumName, album.AlbumArtist, album.MbzAlbumID) - if err != nil { - switch { - case errors.Is(err, agents.ErrNotFound): - log.Trace(ctx, "Album not found in agent", "albumID", id, "name", albumName, "artist", album.AlbumArtist) - return nil, model.ErrNotFound - case errors.Is(err, context.Canceled): - log.Debug(ctx, "GetAlbumImages call canceled", err) - default: - log.Warn(ctx, "Error getting album images from agent", "albumID", id, "name", albumName, "artist", album.AlbumArtist, err) - } - return nil, err - } - - if len(images) == 0 { - log.Warn(ctx, "Agent returned no images without error", "albumID", id, "name", albumName, "artist", album.AlbumArtist) - return nil, model.ErrNotFound - } - - // Return the biggest image - var img agents.ExternalImage - for _, i := range images { - if img.Size <= i.Size { - img = i - } - } - if img.URL == "" { - return nil, model.ErrNotFound - } - return url.Parse(img.URL) -} - func (e *provider) TopSongs(ctx context.Context, artistName string, count int) (model.MediaFiles, error) { artist, err := e.findArtistByName(ctx, artistName) if err != nil { diff --git a/core/external/provider_albumimage_test.go b/core/external/provider_albumimage_test.go deleted file mode 100644 index e801b7cce..000000000 --- a/core/external/provider_albumimage_test.go +++ /dev/null @@ -1,365 +0,0 @@ -package external_test - -import ( - "context" - "errors" - "net/url" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core/agents" - . "github.com/navidrome/navidrome/core/external" - "github.com/navidrome/navidrome/core/matcher" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "github.com/stretchr/testify/mock" -) - -var _ = Describe("Provider - AlbumImage", func() { - var ds *tests.MockDataStore - var provider Provider - var mockArtistRepo *mockArtistRepo - var mockAlbumRepo *mockAlbumRepo - var mockMediaFileRepo *mockMediaFileRepo - var mockAlbumAgent *mockAlbumInfoAgent - var ctx context.Context - - BeforeEach(func() { - ctx = GinkgoT().Context() - DeferCleanup(configtest.SetupConfig()) - conf.Server.Agents = "mockAlbum" // Configure mock agent - - mockArtistRepo = newMockArtistRepo() - mockAlbumRepo = newMockAlbumRepo() - mockMediaFileRepo = newMockMediaFileRepo() - - ds = &tests.MockDataStore{ - MockedArtist: mockArtistRepo, - MockedAlbum: mockAlbumRepo, - MockedMediaFile: mockMediaFileRepo, - } - - mockAlbumAgent = newMockAlbumInfoAgent() - - agentsCombined := &mockAgents{albumInfoAgent: mockAlbumAgent} - provider = NewProvider(ds, agentsCombined, matcher.New(ds)) - - // Default mocks - // Mocks for GetEntityByID sequence (initial failed lookups) - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() - mockArtistRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once() - - // Default mock for non-existent entities - Use Maybe() for flexibility - mockArtistRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe() - mockAlbumRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe() - mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe() - }) - - It("returns the largest image URL when successful", func() { - // Arrange - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - // Explicitly mock agent call for this test - mockAlbumAgent.On("GetAlbumImages", ctx, "Album One", "", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/large.jpg", Size: 1000}, - {URL: "http://example.com/medium.jpg", Size: 500}, - {URL: "http://example.com/small.jpg", Size: 200}, - }, nil).Once() - - expectedURL, _ := url.Parse("http://example.com/large.jpg") - imgURL, err := provider.AlbumImage(ctx, "album-1") - - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") // From GetEntityByID - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockArtistRepo.AssertNotCalled(GinkgoT(), "Get", "artist-1") // Artist lookup no longer happens in getAlbum - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, "Album One", "", "") // Expect empty artist name - }) - - It("returns ErrNotFound if the album is not found in the DB", func() { - // Arrange: Explicitly expect the full GetEntityByID sequence for "not-found" - mockArtistRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once() - mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once() - - imgURL, err := provider.AlbumImage(ctx, "not-found") - - Expect(err).To(MatchError("data not found")) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockAlbumAgent.AssertNotCalled(GinkgoT(), "GetAlbumImages", mock.Anything, mock.Anything, mock.Anything) - }) - - It("returns the agent error if the agent fails", func() { - // Arrange - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - - agentErr := errors.New("agent failure") - // Explicitly mock agent call for this test - mockAlbumAgent.On("GetAlbumImages", ctx, "Album One", "", "").Return(nil, agentErr).Once() // Expect empty artist - - imgURL, err := provider.AlbumImage(ctx, "album-1") - - Expect(err).To(MatchError("agent failure")) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockArtistRepo.AssertNotCalled(GinkgoT(), "Get", "artist-1") - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, "Album One", "", "") // Expect empty artist - }) - - It("returns ErrNotFound if the agent returns ErrNotFound", func() { - // Arrange - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - - // Explicitly mock agent call for this test - mockAlbumAgent.On("GetAlbumImages", ctx, "Album One", "", "").Return(nil, agents.ErrNotFound).Once() // Expect empty artist - - imgURL, err := provider.AlbumImage(ctx, "album-1") - - Expect(err).To(MatchError("data not found")) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, "Album One", "", "") // Expect empty artist - }) - - It("returns ErrNotFound if the agent returns no images", func() { - // Arrange - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - - // Explicitly mock agent call for this test - mockAlbumAgent.On("GetAlbumImages", ctx, "Album One", "", ""). - Return([]agents.ExternalImage{}, nil).Once() // Expect empty artist - - imgURL, err := provider.AlbumImage(ctx, "album-1") - - Expect(err).To(MatchError("data not found")) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, "Album One", "", "") // Expect empty artist - }) - - It("returns context error if context is canceled", func() { - // Arrange - cctx, cancelCtx := context.WithCancel(ctx) - // Mock the necessary DB calls *before* canceling the context - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - // Expect the agent call even if context is cancelled, returning the context error - mockAlbumAgent.On("GetAlbumImages", cctx, "Album One", "", "").Return(nil, context.Canceled).Once() - // Cancel the context *before* calling the function under test - cancelCtx() - - imgURL, err := provider.AlbumImage(cctx, "album-1") - - Expect(err).To(MatchError("context canceled")) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1") - // Agent should now be called, verify this expectation - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", cctx, "Album One", "", "") - }) - - It("derives album ID from MediaFile ID", func() { - // Arrange: Mock full GetEntityByID for "mf-1" and recursive "album-1" - mockArtistRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once() - mockMediaFileRepo.On("Get", "mf-1").Return(&model.MediaFile{ID: "mf-1", Title: "Track One", ArtistID: "artist-1", AlbumID: "album-1"}, nil).Once() - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - - // Explicitly mock agent call for this test - mockAlbumAgent.On("GetAlbumImages", ctx, "Album One", "", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/large.jpg", Size: 1000}, - {URL: "http://example.com/medium.jpg", Size: 500}, - {URL: "http://example.com/small.jpg", Size: 200}, - }, nil).Once() - - expectedURL, _ := url.Parse("http://example.com/large.jpg") - imgURL, err := provider.AlbumImage(ctx, "mf-1") - - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "mf-1") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "mf-1") - mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "mf-1") - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockArtistRepo.AssertNotCalled(GinkgoT(), "Get", "artist-1") - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, "Album One", "", "") - }) - - It("handles different image orders from agent", func() { - // Arrange - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - // Explicitly mock agent call for this test - mockAlbumAgent.On("GetAlbumImages", ctx, "Album One", "", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/small.jpg", Size: 200}, - {URL: "http://example.com/large.jpg", Size: 1000}, - {URL: "http://example.com/medium.jpg", Size: 500}, - }, nil).Once() - - expectedURL, _ := url.Parse("http://example.com/large.jpg") - imgURL, err := provider.AlbumImage(ctx, "album-1") - - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) // Should still pick the largest - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, "Album One", "", "") - }) - - It("handles agent returning only one image", func() { - // Arrange - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() // Expect GetEntityByID sequence - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Once() - // Explicitly mock agent call for this test - mockAlbumAgent.On("GetAlbumImages", ctx, "Album One", "", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/single.jpg", Size: 700}, - }, nil).Once() - - expectedURL, _ := url.Parse("http://example.com/single.jpg") - imgURL, err := provider.AlbumImage(ctx, "album-1") - - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, "Album One", "", "") - }) - - It("returns ErrNotFound if deriving album ID fails", func() { - // Arrange: Mock full GetEntityByID for "mf-no-album" and recursive "not-found" - mockArtistRepo.On("Get", "mf-no-album").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "mf-no-album").Return(nil, model.ErrNotFound).Once() - mockMediaFileRepo.On("Get", "mf-no-album").Return(&model.MediaFile{ID: "mf-no-album", Title: "Track No Album", ArtistID: "artist-1", AlbumID: "not-found"}, nil).Once() - mockArtistRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once() - mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Once() - - imgURL, err := provider.AlbumImage(ctx, "mf-no-album") - - Expect(err).To(MatchError("data not found")) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "mf-no-album") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "mf-no-album") - mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "mf-no-album") - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockAlbumAgent.AssertNotCalled(GinkgoT(), "GetAlbumImages", mock.Anything, mock.Anything, mock.Anything) - }) - - Context("Unicode handling in album names", func() { - var albumWithEnDash *model.Album - var expectedURL *url.URL - - const ( - originalAlbumName = "Raising Hell–Deluxe" // Album name with en dash - normalizedAlbumName = "Raising Hell-Deluxe" // Normalized version with hyphen - ) - - BeforeEach(func() { - // Test with en dash (–) in album name - albumWithEnDash = &model.Album{ID: "album-endash", Name: originalAlbumName, AlbumArtistID: "artist-1"} - mockArtistRepo.Mock = mock.Mock{} // Reset default expectations - mockAlbumRepo.Mock = mock.Mock{} // Reset default expectations - mockArtistRepo.On("Get", "album-endash").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "album-endash").Return(albumWithEnDash, nil).Once() - - expectedURL, _ = url.Parse("http://example.com/album.jpg") - - // Mock the album agent to return an image for the album - mockAlbumAgent.On("GetAlbumImages", ctx, mock.AnythingOfType("string"), "", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/album.jpg", Size: 1000}, - }, nil).Once() - }) - - When("DevPreserveUnicodeInExternalCalls is true", func() { - BeforeEach(func() { - conf.Server.DevPreserveUnicodeInExternalCalls = true - }) - - It("preserves Unicode characters in album names", func() { - // Act - imgURL, err := provider.AlbumImage(ctx, "album-endash") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-endash") - // This is the key assertion: ensure the original Unicode name is used - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, originalAlbumName, "", "") - }) - }) - - When("DevPreserveUnicodeInExternalCalls is false", func() { - BeforeEach(func() { - conf.Server.DevPreserveUnicodeInExternalCalls = false - }) - - It("normalizes Unicode characters", func() { - // Act - imgURL, err := provider.AlbumImage(ctx, "album-endash") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-endash") - // This assertion ensures the normalized name is used (en dash → hyphen) - mockAlbumAgent.AssertCalled(GinkgoT(), "GetAlbumImages", ctx, normalizedAlbumName, "", "") - }) - }) - }) -}) - -// mockAlbumInfoAgent implementation -type mockAlbumInfoAgent struct { - mock.Mock - agents.AlbumInfoRetriever - agents.AlbumImageRetriever -} - -func newMockAlbumInfoAgent() *mockAlbumInfoAgent { - m := new(mockAlbumInfoAgent) - m.On("AgentName").Return("mockAlbum").Maybe() - return m -} - -func (m *mockAlbumInfoAgent) AgentName() string { - args := m.Called() - return args.String(0) -} - -func (m *mockAlbumInfoAgent) GetAlbumInfo(ctx context.Context, name, artist, mbid string) (*agents.AlbumInfo, error) { - args := m.Called(ctx, name, artist, mbid) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).(*agents.AlbumInfo), args.Error(1) -} - -func (m *mockAlbumInfoAgent) GetAlbumImages(ctx context.Context, name, artist, mbid string) ([]agents.ExternalImage, error) { - args := m.Called(ctx, name, artist, mbid) - if args.Get(0) == nil { - return nil, args.Error(1) - } - return args.Get(0).([]agents.ExternalImage), args.Error(1) -} - -// Ensure mockAgent implements the interfaces -var _ agents.AlbumInfoRetriever = (*mockAlbumInfoAgent)(nil) -var _ agents.AlbumImageRetriever = (*mockAlbumInfoAgent)(nil) diff --git a/core/external/provider_artistimage_test.go b/core/external/provider_artistimage_test.go deleted file mode 100644 index 621aac574..000000000 --- a/core/external/provider_artistimage_test.go +++ /dev/null @@ -1,469 +0,0 @@ -package external_test - -import ( - "bytes" - "context" - "errors" - "net/url" - "time" - - "github.com/navidrome/navidrome/conf" - "github.com/navidrome/navidrome/conf/configtest" - "github.com/navidrome/navidrome/core/agents" - . "github.com/navidrome/navidrome/core/external" - "github.com/navidrome/navidrome/core/matcher" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "github.com/stretchr/testify/mock" -) - -var _ = Describe("Provider - ArtistImage", func() { - var ds *tests.MockDataStore - var provider Provider - var mockArtistRepo *mockArtistRepo - var mockAlbumRepo *mockAlbumRepo - var mockMediaFileRepo *mockMediaFileRepo - var mockImageAgent *mockArtistImageAgent - var agentsCombined *mockAgents - var ctx context.Context - - BeforeEach(func() { - DeferCleanup(configtest.SetupConfig()) - conf.Server.Agents = "mockImage" // Configure only the mock agent - ctx = GinkgoT().Context() - - mockArtistRepo = newMockArtistRepo() - mockAlbumRepo = newMockAlbumRepo() - mockMediaFileRepo = newMockMediaFileRepo() - - ds = &tests.MockDataStore{ - MockedArtist: mockArtistRepo, - MockedAlbum: mockAlbumRepo, - MockedMediaFile: mockMediaFileRepo, - } - - mockImageAgent = newMockArtistImageAgent() - - // Use the mockAgents from helper, setting the specific agent - agentsCombined = &mockAgents{ - imageAgent: mockImageAgent, - } - - provider = NewProvider(ds, agentsCombined, matcher.New(ds)) - - // Default mocks for successful Get calls - mockArtistRepo.On("Get", "artist-1").Return(&model.Artist{ID: "artist-1", Name: "Artist One"}, nil).Maybe() - mockAlbumRepo.On("Get", "album-1").Return(&model.Album{ID: "album-1", Name: "Album One", AlbumArtistID: "artist-1"}, nil).Maybe() - mockMediaFileRepo.On("Get", "mf-1").Return(&model.MediaFile{ID: "mf-1", Title: "Track One", ArtistID: "artist-1"}, nil).Maybe() - // Default mock for non-existent entities - mockArtistRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe() - mockAlbumRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe() - mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe() - - // Default successful image agent response - mockImageAgent.On("GetArtistImages", mock.Anything, "artist-1", "Artist One", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/large.jpg", Size: 1000}, - {URL: "http://example.com/medium.jpg", Size: 500}, - {URL: "http://example.com/small.jpg", Size: 200}, - }, nil).Maybe() - }) - - AfterEach(func() { - mockArtistRepo.AssertExpectations(GinkgoT()) - mockAlbumRepo.AssertExpectations(GinkgoT()) - mockMediaFileRepo.AssertExpectations(GinkgoT()) - mockImageAgent.AssertExpectations(GinkgoT()) - }) - - It("returns the largest image URL when successful", func() { - // Arrange - expectedURL, _ := url.Parse("http://example.com/large.jpg") - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-1") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("returns ErrNotFound if the artist is not found in the DB", func() { - // Arrange - - // Act - imgURL, err := provider.ArtistImage(ctx, "not-found") - - // Assert - Expect(err).To(MatchError(model.ErrNotFound)) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockImageAgent.AssertNotCalled(GinkgoT(), "GetArtistImages", mock.Anything, mock.Anything, mock.Anything, mock.Anything) - }) - - It("returns the agent error if the agent fails", func() { - // Arrange - agentErr := errors.New("agent failure") - mockImageAgent.Mock = mock.Mock{} // Reset default expectation - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return(nil, agentErr).Once() - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-1") - - // Assert - Expect(err).To(MatchError(model.ErrNotFound)) // Corrected Expectation: The provider maps agent errors (other than canceled) to ErrNotFound if no image was found/populated - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("returns ErrNotFound if the agent returns ErrNotFound", func() { - // Arrange - mockImageAgent.Mock = mock.Mock{} // Reset default expectation - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return(nil, agents.ErrNotFound).Once() - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-1") - - // Assert - Expect(err).To(MatchError(model.ErrNotFound)) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("returns ErrNotFound if the agent returns no images", func() { - // Arrange - mockImageAgent.Mock = mock.Mock{} // Reset default expectation - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return([]agents.ExternalImage{}, nil).Once() - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-1") - - // Assert - Expect(err).To(MatchError(model.ErrNotFound)) // Implementation maps empty result to ErrNotFound - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("returns context error if context is canceled before agent call", func() { - // Arrange - cctx, cancelCtx := context.WithCancel(context.Background()) - mockArtistRepo.Mock = mock.Mock{} // Reset default expectation for artist repo as well - mockArtistRepo.On("Get", "artist-1").Return(&model.Artist{ID: "artist-1", Name: "Artist One"}, nil).Run(func(args mock.Arguments) { - cancelCtx() // Cancel context *during* the DB call simulation - }).Once() - - // Act - imgURL, err := provider.ArtistImage(cctx, "artist-1") - - // Assert - Expect(err).To(MatchError(context.Canceled)) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") - }) - - It("derives artist ID from MediaFile ID", func() { - // Arrange: Add mocks for the initial GetEntityByID lookups - mockArtistRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "mf-1").Return(nil, model.ErrNotFound).Once() - // Default mocks for MediaFileRepo.Get("mf-1") and ArtistRepo.Get("artist-1") handle the rest - expectedURL, _ := url.Parse("http://example.com/large.jpg") - - // Act - imgURL, err := provider.ArtistImage(ctx, "mf-1") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "mf-1") // GetEntityByID sequence - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "mf-1") // GetEntityByID sequence - mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "mf-1") - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") // Should be called after getting MF - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("derives artist ID from Album ID", func() { - // Arrange: Add mock for the initial GetEntityByID lookup - mockArtistRepo.On("Get", "album-1").Return(nil, model.ErrNotFound).Once() - // Default mocks for AlbumRepo.Get("album-1") and ArtistRepo.Get("artist-1") handle the rest - expectedURL, _ := url.Parse("http://example.com/large.jpg") - - // Act - imgURL, err := provider.ArtistImage(ctx, "album-1") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "album-1") // GetEntityByID sequence - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "album-1") - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") // Should be called after getting Album - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("returns ErrNotFound if derived artist is not found", func() { - // Arrange - // Add mocks for the initial GetEntityByID lookups - mockArtistRepo.On("Get", "mf-bad-artist").Return(nil, model.ErrNotFound).Once() - mockAlbumRepo.On("Get", "mf-bad-artist").Return(nil, model.ErrNotFound).Once() - mockMediaFileRepo.On("Get", "mf-bad-artist").Return(&model.MediaFile{ID: "mf-bad-artist", ArtistID: "not-found"}, nil).Once() - // Add expectation for the recursive GetEntityByID call for the MediaFileRepo - mockMediaFileRepo.On("Get", "not-found").Return(nil, model.ErrNotFound).Maybe() - // The default mocks for ArtistRepo/AlbumRepo handle the final "not-found" lookups - - // Act - imgURL, err := provider.ArtistImage(ctx, "mf-bad-artist") - - // Assert - Expect(err).To(MatchError(model.ErrNotFound)) - Expect(imgURL).To(BeNil()) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "mf-bad-artist") // GetEntityByID sequence - mockAlbumRepo.AssertCalled(GinkgoT(), "Get", "mf-bad-artist") // GetEntityByID sequence - mockMediaFileRepo.AssertCalled(GinkgoT(), "Get", "mf-bad-artist") - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "not-found") - mockImageAgent.AssertNotCalled(GinkgoT(), "GetArtistImages", mock.Anything, mock.Anything, mock.Anything, mock.Anything) - }) - - It("handles different image orders from agent", func() { - // Arrange - mockImageAgent.Mock = mock.Mock{} // Reset default expectation - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/small.jpg", Size: 200}, - {URL: "http://example.com/large.jpg", Size: 1000}, - {URL: "http://example.com/medium.jpg", Size: 500}, - }, nil).Once() - expectedURL, _ := url.Parse("http://example.com/large.jpg") - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-1") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) // Still picks the largest - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("handles agent returning only one image", func() { - // Arrange - mockImageAgent.Mock = mock.Mock{} // Reset default expectation - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/medium.jpg", Size: 500}, - }, nil).Once() - expectedURL, _ := url.Parse("http://example.com/medium.jpg") - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-1") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-1") - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-1", "Artist One", "") - }) - - It("returns cached URL and does not call agent when info is not expired", func() { - // Arrange: artist has a cached image URL with recent ExternalInfoUpdatedAt - cachedArtist := &model.Artist{ - ID: "artist-cached", - Name: "Cached Artist", - LargeImageUrl: "http://example.com/cached-large.jpg", - ExternalInfoUpdatedAt: new(time.Now().Add(-1 * time.Minute)), - } - mockArtistRepo.On("Get", "artist-cached").Return(cachedArtist, nil).Maybe() - expectedURL, _ := url.Parse("http://example.com/cached-large.jpg") - - // Capture log output - var logBuf bytes.Buffer - log.SetOutput(&logBuf) - defer log.SetOutput(GinkgoWriter) - log.SetLevel(log.LevelDebug) - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-cached") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockImageAgent.AssertNotCalled(GinkgoT(), "GetArtistImages", mock.Anything, "artist-cached", mock.Anything, mock.Anything) - - // Assert: background refresh was NOT enqueued - Expect(logBuf.String()).ToNot(ContainSubstring("Artist image info expired, enqueuing background refresh")) - - }) - - It("returns stale URL and enqueues refresh when info is expired", func() { - // Arrange - conf.Server.DevArtistInfoTimeToLive = 1 * time.Nanosecond - staleArtist := &model.Artist{ - ID: "artist-expired", - Name: "Expired Artist", - LargeImageUrl: "http://example.com/expired-large.jpg", - ExternalInfoUpdatedAt: new(time.Now().Add(-1 * time.Hour)), - } - mockArtistRepo.On("Get", "artist-expired").Return(staleArtist, nil).Maybe() - expectedURL, _ := url.Parse("http://example.com/expired-large.jpg") - - // Capture log output - var logBuf bytes.Buffer - log.SetOutput(&logBuf) - defer log.SetOutput(GinkgoWriter) - log.SetLevel(log.LevelDebug) - - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-expired") - - // Assert: returns stale URL immediately, no agent call - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockImageAgent.AssertNotCalled(GinkgoT(), "GetArtistImages", mock.Anything, "artist-expired", mock.Anything, mock.Anything) - - // Assert: background refresh was enqueued - Expect(logBuf.String()).To(ContainSubstring("Artist image info expired, enqueuing background refresh")) - }) - - Describe("ArtistImageResult", func() { - It("returns the real agent error on a transient failure, not ErrNotFound", func() { - agentErr := errors.New("agent timed out") - mockImageAgent.Mock = mock.Mock{} - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return(nil, agentErr).Once() - - imgURL, err := provider.ArtistImageResult(ctx, "artist-1") - - Expect(err).To(MatchError(agentErr)) - Expect(err).ToNot(MatchError(model.ErrNotFound)) - Expect(imgURL).To(BeNil()) - }) - - It("returns ErrNotFound when the agent definitively has no image", func() { - mockImageAgent.Mock = mock.Mock{} - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return(nil, agents.ErrNotFound).Once() - - imgURL, err := provider.ArtistImageResult(ctx, "artist-1") - - Expect(err).To(MatchError(model.ErrNotFound)) - Expect(imgURL).To(BeNil()) - }) - - It("returns ErrNotFound when the agent returns no images without error", func() { - mockImageAgent.Mock = mock.Mock{} - mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return([]agents.ExternalImage{}, nil).Once() - - imgURL, err := provider.ArtistImageResult(ctx, "artist-1") - - Expect(err).To(MatchError(model.ErrNotFound)) - Expect(imgURL).To(BeNil()) - }) - - It("returns the largest image URL on success", func() { - expectedURL, _ := url.Parse("http://example.com/large.jpg") - - imgURL, err := provider.ArtistImageResult(ctx, "artist-1") - - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - }) - }) - - Context("Unicode handling in artist names", func() { - var artistWithEnDash *model.Artist - var expectedURL *url.URL - - const ( - originalArtistName = "Run–D.M.C." // Artist name with en dash - normalizedArtistName = "Run-D.M.C." // Normalized version with hyphen - ) - - BeforeEach(func() { - // Test with en dash (–) in artist name like "Run–D.M.C." - artistWithEnDash = &model.Artist{ID: "artist-endash", Name: originalArtistName} - mockArtistRepo.Mock = mock.Mock{} // Reset default expectations - mockArtistRepo.On("Get", "artist-endash").Return(artistWithEnDash, nil).Once() - - expectedURL, _ = url.Parse("http://example.com/rundmc.jpg") - - // Mock the image agent to return an image for the artist - mockImageAgent.On("GetArtistImages", ctx, "artist-endash", mock.AnythingOfType("string"), ""). - Return([]agents.ExternalImage{ - {URL: "http://example.com/rundmc.jpg", Size: 1000}, - }, nil).Once() - - }) - - When("DevPreserveUnicodeInExternalCalls is true", func() { - BeforeEach(func() { - conf.Server.DevPreserveUnicodeInExternalCalls = true - }) - It("preserves Unicode characters in artist names", func() { - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-endash") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-endash") - // This is the key assertion: ensure the original Unicode name is used - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-endash", originalArtistName, "") - }) - }) - - When("DevPreserveUnicodeInExternalCalls is false", func() { - BeforeEach(func() { - conf.Server.DevPreserveUnicodeInExternalCalls = false - }) - - It("normalizes Unicode characters", func() { - // Act - imgURL, err := provider.ArtistImage(ctx, "artist-endash") - - // Assert - Expect(err).ToNot(HaveOccurred()) - Expect(imgURL).To(Equal(expectedURL)) - mockArtistRepo.AssertCalled(GinkgoT(), "Get", "artist-endash") - // This assertion ensures the normalized name is used (en dash → hyphen) - mockImageAgent.AssertCalled(GinkgoT(), "GetArtistImages", ctx, "artist-endash", normalizedArtistName, "") - }) - }) - }) -}) - -// mockArtistImageAgent implementation using testify/mock -// This remains local as it's specific to testing the ArtistImage functionality -type mockArtistImageAgent struct { - mock.Mock - agents.ArtistImageRetriever // Embed interface -} - -// Constructor for the mock agent -func newMockArtistImageAgent() *mockArtistImageAgent { - mock := new(mockArtistImageAgent) - // Set default AgentName if needed, although usually called via mockAgents - mock.On("AgentName").Return("mockImage").Maybe() - return mock -} - -func (m *mockArtistImageAgent) AgentName() string { - args := m.Called() - return args.String(0) -} - -func (m *mockArtistImageAgent) GetArtistImages(ctx context.Context, id, artistName, mbid string) ([]agents.ExternalImage, error) { - args := m.Called(ctx, id, artistName, mbid) - // Need careful type assertion for potentially nil slice - var res []agents.ExternalImage - if args.Get(0) != nil { - res = args.Get(0).([]agents.ExternalImage) - } - return res, args.Error(1) -} - -// Ensure mockAgent implements the interface -var _ agents.ArtistImageRetriever = (*mockArtistImageAgent)(nil) diff --git a/persistence/e2e/e2e_suite_test.go b/persistence/e2e/e2e_suite_test.go index 1ff2139e6..ac7668218 100644 --- a/persistence/e2e/e2e_suite_test.go +++ b/persistence/e2e/e2e_suite_test.go @@ -14,7 +14,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" @@ -275,7 +274,7 @@ var _ = BeforeSuite(func() { ctx = request.WithUser(GinkgoT().Context(), adminUser) buildTestFS() - s := scanner.New(ctx, initDS, artwork.NoopCacheWarmer(), events.NoopBroker(), + s := scanner.New(ctx, initDS, events.NoopBroker(), playlists.NewPlaylists(initDS, core.NewImageUploadService()), metrics.NewNoopInstance()) _, err = s.ScanAll(ctx, true) Expect(err).ToNot(HaveOccurred()) diff --git a/scanner/controller.go b/scanner/controller.go index 463718ba3..df5aeb6f9 100644 --- a/scanner/controller.go +++ b/scanner/controller.go @@ -11,7 +11,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" @@ -28,12 +27,11 @@ var ( ErrAlreadyScanning = errors.New("already scanning") ) -func New(rootCtx context.Context, ds model.DataStore, cw artwork.CacheWarmer, broker events.Broker, +func New(rootCtx context.Context, ds model.DataStore, broker events.Broker, pls playlists.Playlists, m metrics.Metrics) model.Scanner { c := &controller{ rootCtx: rootCtx, ds: ds, - cw: cw, broker: broker, pls: pls, metrics: m, @@ -49,7 +47,7 @@ func (s *controller) getScanner() scanner { if s.devExternalScanner { return &scannerExternal{} } - return &scannerImpl{ds: s.ds, cw: s.cw, pls: s.pls} + return &scannerImpl{ds: s.ds, pls: s.pls} } // CallScan starts an in-process scan of specific library/folder pairs. @@ -66,7 +64,7 @@ func CallScan(ctx context.Context, ds model.DataStore, pls playlists.Playlists, progress := make(chan *ProgressInfo, 100) go func() { defer close(progress) - scanner := &scannerImpl{ds: ds, cw: artwork.NoopCacheWarmer(), pls: pls} + scanner := &scannerImpl{ds: ds, pls: pls} scanner.scanFolders(ctx, fullScan, targets, progress) }() return progress, nil @@ -97,7 +95,6 @@ type scanner interface { type controller struct { rootCtx context.Context ds model.DataStore - cw artwork.CacheWarmer broker events.Broker metrics metrics.Metrics pls playlists.Playlists diff --git a/scanner/controller_test.go b/scanner/controller_test.go index e4814da64..921841a2d 100644 --- a/scanner/controller_test.go +++ b/scanner/controller_test.go @@ -6,7 +6,6 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/db" @@ -32,7 +31,7 @@ var _ = Describe("Controller", func() { DeferCleanup(configtest.SetupConfig()) ds = &tests.MockDataStore{RealDS: persistence.New(db.Db())} ds.MockedProperty = &tests.MockedPropertyRepo{} - ctrl = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) + ctrl = scanner.New(ctx, ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) }) It("includes last scan error", func() { diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 65f26b9d9..f55f975dc 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -16,7 +16,6 @@ import ( ppl "github.com/google/go-pipeline/pkg/pipeline" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/storage" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -26,7 +25,7 @@ import ( "github.com/navidrome/navidrome/utils/slice" ) -func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStore, cw artwork.CacheWarmer) *phaseFolders { +func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStore) *phaseFolders { var jobs []*scanJob // Create scan jobs for all libraries @@ -37,7 +36,7 @@ func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStor targetFolders = state.targets[lib.ID] } - job, err := newScanJob(ctx, ds, cw, lib, state.fullScan, targetFolders) + job, err := newScanJob(ctx, ds, lib, state.fullScan, targetFolders) if err != nil { log.Error(ctx, "Scanner: Error creating scan context", "lib", lib.Name, err) state.sendError(err) @@ -52,14 +51,13 @@ func createPhaseFolders(ctx context.Context, state *scanState, ds model.DataStor type scanJob struct { lib model.Library fs storage.MusicFS - cw artwork.CacheWarmer lastUpdates map[string]model.FolderUpdateInfo // Holds last update info for all (DB) folders in this library targetFolders []string // Specific folders to scan (including all descendants) lock sync.Mutex numFolders atomic.Int64 } -func newScanJob(ctx context.Context, ds model.DataStore, cw artwork.CacheWarmer, lib model.Library, fullScan bool, targetFolders []string) (*scanJob, error) { +func newScanJob(ctx context.Context, ds model.DataStore, lib model.Library, fullScan bool, targetFolders []string) (*scanJob, error) { // Get folder updates, optionally filtered to specific target folders lastUpdates, err := ds.Folder(ctx).GetFolderUpdateInfo(lib, targetFolders...) if err != nil { @@ -85,7 +83,6 @@ func newScanJob(ctx context.Context, ds model.DataStore, cw artwork.CacheWarmer, return &scanJob{ lib: lib, fs: fsys, - cw: cw, lastUpdates: lastUpdates, targetFolders: targetFolders, }, nil @@ -330,8 +327,6 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) defer p.measure(entry)() p.state.changesDetected.Store(true) - // Collect artwork IDs to pre-cache after the transaction commits - var artworkIDs []model.ArtworkID // Collect artwork queue items for changed albums/artists, enqueued in the same transaction var queueItems []model.ArtworkQueueItem @@ -373,7 +368,6 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) return err } if entry.artists[i].Name != consts.UnknownArtist && entry.artists[i].Name != consts.VariousArtists { - artworkIDs = append(artworkIDs, entry.artists[i].CoverArtID()) queueItems = append(queueItems, model.ArtworkQueueItem{ ItemKind: "ar", ItemID: entry.artists[i].ID, ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityScan, @@ -389,7 +383,6 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) return err } if entry.albums[i].Name != consts.UnknownAlbum { - artworkIDs = append(artworkIDs, entry.albums[i].CoverArtID()) queueItems = append(queueItems, model.ArtworkQueueItem{ ItemKind: "al", ItemID: entry.albums[i].ID, ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityScan, @@ -449,13 +442,6 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) log.Error(p.ctx, "Scanner: Error persisting changes to DB", "folder", entry.path, err) } - // Pre-cache artwork after the transaction commits successfully - if err == nil { - for _, artID := range artworkIDs { - entry.job.cw.PreCache(artID) - } - } - return entry, err } diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index 15798b054..04ad2c276 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -12,7 +12,6 @@ import ( ppl "github.com/google/go-pipeline/pkg/pipeline" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -24,18 +23,16 @@ type phasePlaylists struct { scanState *scanState ds model.DataStore pls playlists.Playlists - cw artwork.CacheWarmer refreshed atomic.Uint32 pendingImport bool } -func createPhasePlaylists(ctx context.Context, scanState *scanState, ds model.DataStore, pls playlists.Playlists, cw artwork.CacheWarmer) *phasePlaylists { +func createPhasePlaylists(ctx context.Context, scanState *scanState, ds model.DataStore, pls playlists.Playlists) *phasePlaylists { return &phasePlaylists{ ctx: ctx, scanState: scanState, ds: ds, pls: pls, - cw: cw, } } @@ -148,7 +145,6 @@ func (p *phasePlaylists) processPlaylistsInFolder(folder *model.Folder) (*model. } else { log.Debug("Scanner: Imported playlist", "name", pls.Name, "lastUpdated", pls.UpdatedAt, "path", pls.Path, "numTracks", len(pls.Tracks), "elapsed", time.Since(started)) } - p.cw.PreCache(pls.CoverArtID()) item := model.ArtworkQueueItem{ItemKind: "pl", ItemID: pls.ID, ImageType: model.ImageTypePrimary, Priority: model.ArtworkPriorityScan} if err := p.ds.ArtworkQueue(p.ctx).Enqueue(item); err != nil { diff --git a/scanner/phase_4_playlists_test.go b/scanner/phase_4_playlists_test.go index 1adf34a63..93ec1a36d 100644 --- a/scanner/phase_4_playlists_test.go +++ b/scanner/phase_4_playlists_test.go @@ -10,7 +10,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" @@ -27,7 +26,6 @@ var _ = Describe("phasePlaylists", func() { folderRepo *mockFolderRepository ds *tests.MockDataStore pls *mockPlaylists - cw artwork.CacheWarmer ) var userRepo *tests.MockedUserRepo @@ -48,9 +46,8 @@ var _ = Describe("phasePlaylists", func() { MockedProperty: propRepo, } pls = &mockPlaylists{} - cw = artwork.NoopCacheWarmer() state = &scanState{} - phase = createPhasePlaylists(ctx, state, ds, pls, cw) + phase = createPhasePlaylists(ctx, state, ds, pls) }) Describe("description", func() { diff --git a/scanner/scanner.go b/scanner/scanner.go index 27e2b19d2..8f47c0b7d 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -11,7 +11,6 @@ import ( ppl "github.com/google/go-pipeline/pkg/pipeline" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" @@ -21,7 +20,6 @@ import ( type scannerImpl struct { ds model.DataStore - cw artwork.CacheWarmer pls playlists.Playlists } @@ -136,7 +134,7 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] err = run.Sequentially( // Phase 1: Scan all libraries and import new/updated files - runPhase[*folderEntry](ctx, 1, createPhaseFolders(ctx, &state, s.ds, s.cw)), + runPhase[*folderEntry](ctx, 1, createPhaseFolders(ctx, &state, s.ds)), // Phase 2: Process missing files, checking for moves runPhase[*missingTracks](ctx, 2, createPhaseMissingTracks(ctx, &state, s.ds)), @@ -147,7 +145,7 @@ func (s *scannerImpl) scanFolders(ctx context.Context, fullScan bool, targets [] runPhase[*model.Album](ctx, 3, createPhaseRefreshAlbums(ctx, &state, s.ds)), // Phase 4: Import/update playlists - runPhase[*model.Folder](ctx, 4, createPhasePlaylists(ctx, &state, s.ds, s.pls, s.cw)), + runPhase[*model.Folder](ctx, 4, createPhasePlaylists(ctx, &state, s.ds, s.pls)), ), // Final Steps (cannot be parallelized): diff --git a/scanner/scanner_benchmark_test.go b/scanner/scanner_benchmark_test.go index 8f0dcd340..6252b272a 100644 --- a/scanner/scanner_benchmark_test.go +++ b/scanner/scanner_benchmark_test.go @@ -13,7 +13,6 @@ import ( "github.com/google/uuid" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" @@ -40,7 +39,7 @@ func BenchmarkScan(b *testing.B) { ds := persistence.New(db.Db()) conf.Server.DevExternalScanner = false - s := scanner.New(context.Background(), ds, artwork.NoopCacheWarmer(), events.NoopBroker(), + s := scanner.New(context.Background(), ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) fs := storagetest.FakeFS{} diff --git a/scanner/scanner_multilibrary_test.go b/scanner/scanner_multilibrary_test.go index 3ae50933c..83e174444 100644 --- a/scanner/scanner_multilibrary_test.go +++ b/scanner/scanner_multilibrary_test.go @@ -12,7 +12,6 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" @@ -78,7 +77,7 @@ var _ = Describe("Scanner - Multi-Library", Ordered, func() { } Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) - s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), + s = scanner.New(ctx, ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) // Create two test libraries (let DB auto-assign IDs) diff --git a/scanner/scanner_selective_test.go b/scanner/scanner_selective_test.go index 17772bf9d..4eec23e89 100644 --- a/scanner/scanner_selective_test.go +++ b/scanner/scanner_selective_test.go @@ -11,7 +11,6 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" @@ -66,7 +65,7 @@ var _ = Describe("ScanFolders", Ordered, func() { } Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) - s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), + s = scanner.New(ctx, ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) lib = model.Library{ID: 1, Name: "Fake Library", Path: "fake:///music"} diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index b8044c587..46c45e522 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -14,7 +14,6 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" @@ -86,7 +85,7 @@ var _ = Describe("Scanner", Ordered, func() { } Expect(ds.User(ctx).Put(&adminUser)).To(Succeed()) - s = scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), + s = scanner.New(ctx, ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) lib = model.Library{ID: 1, Name: "Fake Library", Path: "fake:///music"} diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index bd4651897..c84680a11 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -357,18 +357,6 @@ func (n noopProvider) TopSongs(context.Context, string, int) (model.MediaFiles, return nil, nil } -func (n noopProvider) ArtistImage(context.Context, string) (*url.URL, error) { - return nil, model.ErrNotFound -} - -func (n noopProvider) ArtistImageResult(context.Context, string) (*url.URL, error) { - return nil, model.ErrNotFound -} - -func (n noopProvider) AlbumImage(context.Context, string) (*url.URL, error) { - return nil, model.ErrNotFound -} - // Compile-time interface checks var ( _ artwork.Service = noopArtwork{} @@ -420,7 +408,7 @@ func setupTestDB() { // Create the Subsonic Router with real DS, streamer spy, and real Decider streamerSpy = &harness.SpyStreamer{} decider := stream.NewTranscodeDecider(ds, harness.NoopFFmpeg{}) - s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), + s := scanner.New(ctx, ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) router = subsonic.New( ds, diff --git a/server/subsonic/e2e/subsonic_multilibrary_test.go b/server/subsonic/e2e/subsonic_multilibrary_test.go index e652cf492..2ecc3d863 100644 --- a/server/subsonic/e2e/subsonic_multilibrary_test.go +++ b/server/subsonic/e2e/subsonic_multilibrary_test.go @@ -7,7 +7,6 @@ import ( "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/core/storage/storagetest" @@ -53,7 +52,7 @@ var _ = Describe("Multi-Library Support", Ordered, func() { adminWithLibs = *loadedAdmin // Run incremental scan to import lib2 content (lib1 files unchanged → skipped) - s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), + s := scanner.New(ctx, ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) _, err = s.ScanAll(ctx, false) Expect(err).ToNot(HaveOccurred()) diff --git a/tests/harness/harness.go b/tests/harness/harness.go index ff5ce8919..dabb6f0ea 100644 --- a/tests/harness/harness.go +++ b/tests/harness/harness.go @@ -14,7 +14,6 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" - "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/core/metrics" @@ -74,7 +73,7 @@ func SetupDB(ctx context.Context, users ...*model.User) *DB { u.Libraries = loaded.Libraries } - s := scanner.New(ctx, ds, artwork.NoopCacheWarmer(), events.NoopBroker(), + s := scanner.New(ctx, ds, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), metrics.NewNoopInstance()) _, err := s.ScanAll(ctx, true) Expect(err).ToNot(HaveOccurred())