From 8f3387a894a67b130a26c4d4871fad8f9897a3fe Mon Sep 17 00:00:00 2001
From: Deluan <deluan@navidrome.org>
Date: Tue, 27 Dec 2022 16:36:13 -0500
Subject: [PATCH] Fix tests and clean up code a bit
---
core/artwork/artwork.go | 37 ---------
core/artwork/artwork_internal_test.go | 81 ++++++++++---------
core/artwork/artwork_suite_test.go | 17 ++++
core/artwork/artwork_test.go | 47 +++++++++++
...rtwork_cache_warmer.go => cache_warmer.go} | 0
core/artwork/image_cache.go | 47 +++++++++++
core/artwork/reader_album.go | 6 +-
core/artwork/reader_mediafile.go | 6 +-
core/artwork/reader_resized.go | 8 +-
core/wire_providers.go | 8 +-
model/artwork_id.go | 3 +
11 files changed, 169 insertions(+), 91 deletions(-)
create mode 100644 core/artwork/artwork_suite_test.go
create mode 100644 core/artwork/artwork_test.go
rename core/artwork/{artwork_cache_warmer.go => cache_warmer.go} (100%)
create mode 100644 core/artwork/image_cache.go
diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go
index 48d3d68ac..f6cc2a516 100644
--- a/core/artwork/artwork.go
+++ b/core/artwork/artwork.go
@@ -3,18 +3,14 @@ package artwork
import (
"context"
"errors"
- "fmt"
_ "image/gif"
"io"
"time"
- "github.com/navidrome/navidrome/conf"
- "github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/core/ffmpeg"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/utils/cache"
- "github.com/navidrome/navidrome/utils/singleton"
_ "golang.org/x/image/webp"
)
@@ -79,36 +75,3 @@ func (a *artwork) getArtworkReader(ctx context.Context, artID model.ArtworkID, s
}
return artReader, err
}
-
-type cacheItem struct {
- artID model.ArtworkID
- size int
- lastUpdate time.Time
-}
-
-func (i *cacheItem) Key() string {
- return fmt.Sprintf(
- "%s.%d.%d.%d.%t",
- i.artID.ID,
- i.lastUpdate.UnixMilli(),
- i.size,
- conf.Server.CoverJpegQuality,
- conf.Server.EnableMediaFileCoverArt,
- )
-}
-
-type imageCache struct {
- cache.FileCache
-}
-
-func GetImageCache() cache.FileCache {
- return singleton.GetInstance(func() *imageCache {
- return &imageCache{
- FileCache: cache.NewFileCache("Image", conf.Server.ImageCacheSize, consts.ImageCacheDir, consts.DefaultImageCacheMaxItems,
- func(ctx context.Context, arg cache.Item) (io.Reader, error) {
- r, _, err := arg.(artworkReader).Reader(ctx)
- return r, err
- }),
- }
- })
-}
diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go
index 6c0a257b0..90e2e5cdf 100644
--- a/core/artwork/artwork_internal_test.go
+++ b/core/artwork/artwork_internal_test.go
@@ -5,7 +5,6 @@ import (
"errors"
"image"
"io"
- "testing"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/conf/configtest"
@@ -17,13 +16,6 @@ import (
. "github.com/onsi/gomega"
)
-func TestArtwork(t *testing.T) {
- tests.Init(t, false)
- log.SetLevel(log.LevelFatal)
- RegisterFailHandler(Fail)
- RunSpecs(t, "Artwork Suite")
-}
-
var _ = Describe("Artwork", func() {
var aw *artwork
var ds model.DataStore
@@ -54,20 +46,11 @@ var _ = Describe("Artwork", func() {
aw = NewArtwork(ds, cache, ffmpeg).(*artwork)
})
- Context("Empty ID", func() {
- It("returns placeholder if album is not in the DB", func() {
- _, path, err := aw.get(context.Background(), model.ArtworkID{}, 0)
- Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal(consts.PlaceholderAlbumArt))
- })
- })
-
- Context("Albums", func() {
+ Describe("albumArtworkReader", func() {
Context("ID not found", func() {
- It("returns placeholder if album is not in the DB", func() {
- _, path, err := aw.get(context.Background(), model.MustParseArtworkID("al-NOT_FOUND-0"), 0)
- Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal(consts.PlaceholderAlbumArt))
+ It("returns ErrNotFound if album is not in the DB", func() {
+ _, err := newAlbumArtworkReader(ctx, aw, model.MustParseArtworkID("al-NOT_FOUND"))
+ Expect(err).To(MatchError(model.ErrNotFound))
})
})
Context("Embed images", func() {
@@ -78,13 +61,17 @@ var _ = Describe("Artwork", func() {
})
})
It("returns embed cover", func() {
- _, path, err := aw.get(context.Background(), alOnlyEmbed.CoverArtID(), 0)
+ aw, err := newAlbumArtworkReader(ctx, aw, alOnlyEmbed.CoverArtID())
+ Expect(err).ToNot(HaveOccurred())
+ _, path, err := aw.Reader(ctx)
Expect(err).ToNot(HaveOccurred())
Expect(path).To(Equal("tests/fixtures/test.mp3"))
})
It("returns placeholder if embed path is not available", func() {
ffmpeg.Error = errors.New("not available")
- _, path, err := aw.get(context.Background(), alEmbedNotFound.CoverArtID(), 0)
+ aw, err := newAlbumArtworkReader(ctx, aw, alEmbedNotFound.CoverArtID())
+ Expect(err).ToNot(HaveOccurred())
+ _, path, err := aw.Reader(ctx)
Expect(err).ToNot(HaveOccurred())
Expect(path).To(Equal(consts.PlaceholderAlbumArt))
})
@@ -93,15 +80,20 @@ var _ = Describe("Artwork", func() {
BeforeEach(func() {
ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{
alOnlyExternal,
+ alExternalNotFound,
})
})
It("returns external cover", func() {
- _, path, err := aw.get(context.Background(), alOnlyExternal.CoverArtID(), 0)
+ aw, err := newAlbumArtworkReader(ctx, aw, alOnlyExternal.CoverArtID())
+ Expect(err).ToNot(HaveOccurred())
+ _, path, err := aw.Reader(ctx)
Expect(err).ToNot(HaveOccurred())
Expect(path).To(Equal("tests/fixtures/front.png"))
})
It("returns placeholder if external file is not available", func() {
- _, path, err := aw.get(context.Background(), alExternalNotFound.CoverArtID(), 0)
+ aw, err := newAlbumArtworkReader(ctx, aw, alExternalNotFound.CoverArtID())
+ Expect(err).ToNot(HaveOccurred())
+ _, path, err := aw.Reader(ctx)
Expect(err).ToNot(HaveOccurred())
Expect(path).To(Equal(consts.PlaceholderAlbumArt))
})
@@ -115,7 +107,9 @@ var _ = Describe("Artwork", func() {
DescribeTable("CoverArtPriority",
func(priority string, expected string) {
conf.Server.CoverArtPriority = priority
- _, path, err := aw.get(context.Background(), alMultipleCovers.CoverArtID(), 0)
+ aw, err := newAlbumArtworkReader(ctx, aw, alMultipleCovers.CoverArtID())
+ Expect(err).ToNot(HaveOccurred())
+ _, path, err := aw.Reader(ctx)
Expect(err).ToNot(HaveOccurred())
Expect(path).To(Equal(expected))
},
@@ -125,12 +119,11 @@ var _ = Describe("Artwork", func() {
)
})
})
- Context("MediaFiles", func() {
+ Describe("mediafileArtworkReader", func() {
Context("ID not found", func() {
- It("returns placeholder if album is not in the DB", func() {
- _, path, err := aw.get(context.Background(), model.MustParseArtworkID("mf-NOT_FOUND-0"), 0)
- Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal(consts.PlaceholderAlbumArt))
+ It("returns ErrNotFound if mediafile is not in the DB", func() {
+ _, err := newAlbumArtworkReader(ctx, aw, alMultipleCovers.CoverArtID())
+ Expect(err).To(MatchError(model.ErrNotFound))
})
})
Context("Embed images", func() {
@@ -146,30 +139,38 @@ var _ = Describe("Artwork", func() {
})
})
It("returns embed cover", func() {
- _, path, err := aw.get(context.Background(), mfWithEmbed.CoverArtID(), 0)
+ 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() {
- r, path, err := aw.get(context.Background(), mfCorruptedCover.CoverArtID(), 0)
+ aw, err := newMediafileArtworkReader(ctx, aw, mfCorruptedCover.CoverArtID())
+ Expect(err).ToNot(HaveOccurred())
+ r, path, err := aw.Reader(ctx)
Expect(err).ToNot(HaveOccurred())
Expect(io.ReadAll(r)).To(Equal([]byte("content from ffmpeg")))
Expect(path).To(Equal("tests/fixtures/test.ogg"))
})
It("returns album cover if cannot read embed artwork", func() {
ffmpeg.Error = errors.New("not available")
- _, path, err := aw.get(context.Background(), mfCorruptedCover.CoverArtID(), 0)
+ aw, err := newMediafileArtworkReader(ctx, aw, mfCorruptedCover.CoverArtID())
Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal("tests/fixtures/front.png"))
+ _, 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() {
- _, path, err := aw.get(context.Background(), mfWithoutEmbed.CoverArtID(), 0)
+ aw, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-"+mfWithoutEmbed.ID))
Expect(err).ToNot(HaveOccurred())
- Expect(path).To(Equal("tests/fixtures/front.png"))
+ _, path, err := aw.Reader(ctx)
+ Expect(err).ToNot(HaveOccurred())
+ Expect(path).To(Equal("al-444"))
})
})
})
- Context("Resize", func() {
+ Describe("resizedArtworkReader", func() {
BeforeEach(func() {
ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{
alMultipleCovers,
@@ -177,7 +178,7 @@ var _ = Describe("Artwork", func() {
})
It("returns a PNG if original image is a PNG", func() {
conf.Server.CoverArtPriority = "front.png"
- r, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID().String(), 300)
+ r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID().String(), 300)
Expect(err).ToNot(HaveOccurred())
br, format, err := asImageReader(r)
@@ -191,7 +192,7 @@ var _ = Describe("Artwork", func() {
})
It("returns a JPEG if original image is not a PNG", func() {
conf.Server.CoverArtPriority = "cover.jpg"
- r, _, err := aw.get(context.Background(), alMultipleCovers.CoverArtID(), 200)
+ r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID().String(), 200)
Expect(err).ToNot(HaveOccurred())
br, format, err := asImageReader(r)
diff --git a/core/artwork/artwork_suite_test.go b/core/artwork/artwork_suite_test.go
new file mode 100644
index 000000000..dfd66e5e5
--- /dev/null
+++ b/core/artwork/artwork_suite_test.go
@@ -0,0 +1,17 @@
+package artwork
+
+import (
+ "testing"
+
+ "github.com/navidrome/navidrome/log"
+ "github.com/navidrome/navidrome/tests"
+ . "github.com/onsi/ginkgo/v2"
+ . "github.com/onsi/gomega"
+)
+
+func TestArtwork(t *testing.T) {
+ tests.Init(t, false)
+ log.SetLevel(log.LevelFatal)
+ RegisterFailHandler(Fail)
+ RunSpecs(t, "Artwork Suite")
+}
diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go
new file mode 100644
index 000000000..ec1d5095b
--- /dev/null
+++ b/core/artwork/artwork_test.go
@@ -0,0 +1,47 @@
+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)
+ })
+
+ Context("Empty ID", func() {
+ It("returns placeholder if album is not in the DB", func() {
+ r, _, err := aw.Get(context.Background(), "", 0)
+ 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))
+ })
+ })
+})
diff --git a/core/artwork/artwork_cache_warmer.go b/core/artwork/cache_warmer.go
similarity index 100%
rename from core/artwork/artwork_cache_warmer.go
rename to core/artwork/cache_warmer.go
diff --git a/core/artwork/image_cache.go b/core/artwork/image_cache.go
new file mode 100644
index 000000000..13c8b8fdd
--- /dev/null
+++ b/core/artwork/image_cache.go
@@ -0,0 +1,47 @@
+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
+ size int
+ lastUpdate time.Time
+}
+
+func (k *cacheKey) Key() string {
+ return fmt.Sprintf(
+ "%s.%d.%d.%d.%t",
+ k.artID.ID,
+ k.lastUpdate.UnixMilli(),
+ k.size,
+ conf.Server.CoverJpegQuality,
+ conf.Server.EnableMediaFileCoverArt,
+ )
+}
+
+type imageCache struct {
+ cache.FileCache
+}
+
+func GetImageCache() cache.FileCache {
+ return singleton.GetInstance(func() *imageCache {
+ return &imageCache{
+ FileCache: cache.NewFileCache("Image", conf.Server.ImageCacheSize, consts.ImageCacheDir, consts.DefaultImageCacheMaxItems,
+ func(ctx context.Context, arg cache.Item) (io.Reader, error) {
+ r, _, err := arg.(artworkReader).Reader(ctx)
+ return r, err
+ }),
+ }
+ })
+}
diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go
index e89435af7..dbe804b60 100644
--- a/core/artwork/reader_album.go
+++ b/core/artwork/reader_album.go
@@ -12,7 +12,7 @@ import (
)
type albumArtworkReader struct {
- cacheItem
+ cacheKey
a *artwork
album model.Album
}
@@ -26,8 +26,8 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar
a: artwork,
album: *al,
}
- a.cacheItem.artID = artID
- a.cacheItem.lastUpdate = al.UpdatedAt
+ a.cacheKey.artID = artID
+ a.cacheKey.lastUpdate = al.UpdatedAt
return a, nil
}
diff --git a/core/artwork/reader_mediafile.go b/core/artwork/reader_mediafile.go
index 50f608534..13b35fcc0 100644
--- a/core/artwork/reader_mediafile.go
+++ b/core/artwork/reader_mediafile.go
@@ -9,7 +9,7 @@ import (
)
type mediafileArtworkReader struct {
- cacheItem
+ cacheKey
a *artwork
mediafile model.MediaFile
album model.Album
@@ -29,8 +29,8 @@ func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID mode
mediafile: *mf,
album: *al,
}
- a.cacheItem.artID = artID
- a.cacheItem.lastUpdate = a.LastUpdated()
+ a.cacheKey.artID = artID
+ a.cacheKey.lastUpdate = a.LastUpdated()
return a, nil
}
diff --git a/core/artwork/reader_resized.go b/core/artwork/reader_resized.go
index b0fa3da5a..d7594b12c 100644
--- a/core/artwork/reader_resized.go
+++ b/core/artwork/reader_resized.go
@@ -20,21 +20,21 @@ import (
)
type resizedArtworkReader struct {
- cacheItem
+ cacheKey
a *artwork
}
func resizedFromOriginal(ctx context.Context, a *artwork, artID model.ArtworkID, size int) (*resizedArtworkReader, error) {
r := &resizedArtworkReader{a: a}
- r.cacheItem.artID = artID
- r.cacheItem.size = size
+ r.cacheKey.artID = artID
+ r.cacheKey.size = size
// Get lastUpdated from original artwork
original, err := a.getArtworkReader(ctx, artID, 0)
if err != nil {
return nil, err
}
- r.cacheItem.lastUpdate = original.LastUpdated()
+ r.cacheKey.lastUpdate = original.LastUpdated()
return r, nil
}
diff --git a/core/wire_providers.go b/core/wire_providers.go
index 0a4c365f1..699db3bfc 100644
--- a/core/wire_providers.go
+++ b/core/wire_providers.go
@@ -9,17 +9,17 @@ import (
)
var Set = wire.NewSet(
- artwork.NewArtwork,
NewMediaStreamer,
GetTranscodingCache,
- artwork.GetImageCache,
NewArchiver,
NewExternalMetadata,
NewPlayers,
+ NewShare,
+ NewPlaylists,
agents.New,
ffmpeg.New,
scrobbler.GetPlayTracker,
- NewShare,
- NewPlaylists,
+ artwork.NewArtwork,
+ artwork.GetImageCache,
artwork.NewCacheWarmer,
)
diff --git a/model/artwork_id.go b/model/artwork_id.go
index 8928929c3..28ea8ef2b 100644
--- a/model/artwork_id.go
+++ b/model/artwork_id.go
@@ -19,6 +19,9 @@ type ArtworkID struct {
}
func (id ArtworkID) String() string {
+ if id.ID == "" {
+ return ""
+ }
return fmt.Sprintf("%s-%s", id.Kind.prefix, id.ID)
}