From 9f4f2f7381039b135b4bfe4595b03395d4625774 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 24 Jul 2020 13:30:27 -0400 Subject: [PATCH] Use new FileCache in cover service --- core/core_suite_test.go | 18 ---------- core/cover.go | 72 ++++++++++++++++--------------------- core/cover_test.go | 27 ++++++-------- core/file_caches.go | 9 ++--- core/file_caches_test.go | 50 ++++++++++++++++++++++++-- core/media_streamer.go | 2 +- core/media_streamer_test.go | 3 +- 7 files changed, 97 insertions(+), 84 deletions(-) diff --git a/core/core_suite_test.go b/core/core_suite_test.go index b03ee16e5..6146df115 100644 --- a/core/core_suite_test.go +++ b/core/core_suite_test.go @@ -1,13 +1,10 @@ package core import ( - "io/ioutil" - "os" "testing" "github.com/deluan/navidrome/log" "github.com/deluan/navidrome/tests" - "github.com/djherbis/fscache" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -18,18 +15,3 @@ func TestEngine(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Core Suite") } - -var testCache fscache.Cache -var testCacheDir string - -var _ = Describe("Core Suite Setup", func() { - BeforeSuite(func() { - testCacheDir, _ = ioutil.TempDir("", "core_test_cache") - fs, _ := fscache.NewFs(testCacheDir, 0755) - testCache, _ = fscache.NewCache(fs, nil) - }) - - AfterSuite(func() { - os.RemoveAll(testCacheDir) - }) -}) diff --git a/core/cover.go b/core/cover.go index 43c70fa19..af1150529 100644 --- a/core/cover.go +++ b/core/cover.go @@ -22,22 +22,30 @@ import ( "github.com/deluan/navidrome/utils" "github.com/dhowden/tag" "github.com/disintegration/imaging" - "github.com/djherbis/fscache" ) type Cover interface { Get(ctx context.Context, id string, size int, out io.Writer) error } -type ImageCache fscache.Cache - -func NewCover(ds model.DataStore, cache ImageCache) Cover { +func NewCover(ds model.DataStore, cache *FileCache) Cover { return &cover{ds: ds, cache: cache} } type cover struct { ds model.DataStore - cache fscache.Cache + cache *FileCache +} + +type coverInfo struct { + c *cover + path string + size int + lastUpdate time.Time +} + +func (ci *coverInfo) String() string { + return fmt.Sprintf("%s.%d.%s.%d", ci.path, ci.size, ci.lastUpdate.Format(time.RFC3339Nano), conf.Server.CoverJpegQuality) } func (c *cover) Get(ctx context.Context, id string, size int, out io.Writer) error { @@ -46,41 +54,18 @@ func (c *cover) Get(ctx context.Context, id string, size int, out io.Writer) err return err } - // If cache is disabled, just read the coverart directly from file - if c.cache == nil { - log.Trace(ctx, "Retrieving cover art from file", "path", path, "size", size, err) - reader, err := c.getCover(ctx, path, size) - if err != nil { - log.Error(ctx, "Error loading cover art", "path", path, "size", size, err) - } else { - _, err = io.Copy(out, reader) - } - return err + info := &coverInfo{ + c: c, + path: path, + size: size, + lastUpdate: lastUpdate, } - cacheKey := imageCacheKey(path, size, lastUpdate) - r, w, err := c.cache.Get(cacheKey) + r, err := c.cache.Get(ctx, info) if err != nil { - log.Error(ctx, "Error reading from image cache", "path", path, "size", size, err) + log.Error(ctx, "Error accessing image cache", "path", path, "size", size, err) return err } - defer r.Close() - if w != nil { - log.Trace(ctx, "Image cache miss", "path", path, "size", size, "lastUpdate", lastUpdate) - go func() { - defer w.Close() - reader, err := c.getCover(ctx, path, size) - if err != nil { - log.Error(ctx, "Error loading cover art", "path", path, "size", size, err) - return - } - if _, err := io.Copy(w, reader); err != nil { - log.Error(ctx, "Error saving covert art to cache", "path", path, "size", size, err) - } - }() - } else { - log.Trace(ctx, "Loading image from cache", "path", path, "size", size, "lastUpdate", lastUpdate) - } _, err = io.Copy(out, r) return err @@ -118,10 +103,6 @@ func (c *cover) getCoverPath(ctx context.Context, id string) (path string, lastU return c.getCoverPath(ctx, "al-"+mf.AlbumID) } -func imageCacheKey(path string, size int, lastUpdate time.Time) string { - return fmt.Sprintf("%s.%d.%s.%d", path, size, lastUpdate.Format(time.RFC3339Nano), conf.Server.CoverJpegQuality) -} - func (c *cover) getCover(ctx context.Context, path string, size int) (reader io.Reader, err error) { defer func() { if err != nil { @@ -201,6 +182,15 @@ func readFromFile(path string) ([]byte, error) { return buf.Bytes(), nil } -func NewImageCache() (ImageCache, error) { - return newFSCache("Image", conf.Server.ImageCacheSize, consts.ImageCacheDir, consts.DefaultImageCacheMaxItems) +func NewImageCache() (*FileCache, error) { + return NewFileCache("Image", conf.Server.ImageCacheSize, consts.ImageCacheDir, consts.DefaultImageCacheMaxItems, + func(ctx context.Context, arg fmt.Stringer) (io.Reader, error) { + info := arg.(*coverInfo) + reader, err := info.c.getCover(ctx, info.path, info.size) + if err != nil { + log.Error(ctx, "Error loading cover art", "path", info.path, "size", info.size, err) + return nil, err + } + return reader, nil + }) } diff --git a/core/cover_test.go b/core/cover_test.go index e7a2594e8..48dd1782e 100644 --- a/core/cover_test.go +++ b/core/cover_test.go @@ -4,7 +4,10 @@ import ( "bytes" "context" "image" + "io/ioutil" + "os" + "github.com/deluan/navidrome/conf" "github.com/deluan/navidrome/log" "github.com/deluan/navidrome/model" "github.com/deluan/navidrome/persistence" @@ -25,7 +28,14 @@ var _ = Describe("Cover", func() { Context("Cache is configured", func() { BeforeEach(func() { - cover = NewCover(ds, testCache) + conf.Server.DataFolder, _ = ioutil.TempDir("", "file_caches") + conf.Server.ImageCacheSize = "100MB" + cache, _ := NewImageCache() + cover = NewCover(ds, cache) + }) + + AfterEach(func() { + os.RemoveAll(conf.Server.DataFolder) }) It("retrieves the external cover art for an album", func() { @@ -118,19 +128,4 @@ var _ = Describe("Cover", func() { }) }) }) - Context("Cache is NOT configured", func() { - BeforeEach(func() { - cover = NewCover(ds, nil) - }) - - It("retrieves the original cover art from an album", func() { - buf := new(bytes.Buffer) - - Expect(cover.Get(ctx, "al-222", 0, buf)).To(BeNil()) - - _, format, err := image.Decode(bytes.NewReader(buf.Bytes())) - Expect(err).To(BeNil()) - Expect(format).To(Equal("jpeg")) - }) - }) }) diff --git a/core/file_caches.go b/core/file_caches.go index 29ac0216e..8ae50d933 100644 --- a/core/file_caches.go +++ b/core/file_caches.go @@ -123,14 +123,15 @@ func copyAndClose(ctx context.Context, w io.WriteCloser, r io.Reader) { } func newFSCache(name, cacheSize, cacheFolder string, maxItems int) (fscache.Cache, error) { - if cacheSize == "0" { - log.Warn(fmt.Sprintf("%s cache disabled", name)) - return nil, nil - } size, err := humanize.ParseBytes(cacheSize) if err != nil { + log.Error("Invalid cache size. Using default size", "cache", name, "size", cacheSize, "defaultSize", consts.DefaultCacheSize) size = consts.DefaultCacheSize } + if size == 0 { + log.Warn(fmt.Sprintf("%s cache disabled", name)) + return nil, nil + } lru := fscache.NewLRUHaunter(maxItems, int64(size), consts.DefaultCacheCleanUpInterval) h := fscache.NewLRUHaunterStrategy(lru) cacheFolder = filepath.Join(conf.Server.DataFolder, cacheFolder) diff --git a/core/file_caches_test.go b/core/file_caches_test.go index be25084d0..14b540111 100644 --- a/core/file_caches_test.go +++ b/core/file_caches_test.go @@ -1,9 +1,13 @@ package core import ( + "context" + "fmt" + "io" "io/ioutil" "os" "path/filepath" + "strings" "github.com/deluan/navidrome/conf" . "github.com/onsi/ginkgo" @@ -20,21 +24,21 @@ var _ = Describe("File Caches", func() { Describe("NewFileCache", func() { It("creates the cache folder", func() { - Expect(NewFileCache("test", "1k", "test", 10, nil)).ToNot(BeNil()) + Expect(NewFileCache("test", "1k", "test", 0, nil)).ToNot(BeNil()) _, err := os.Stat(filepath.Join(conf.Server.DataFolder, "test")) Expect(os.IsNotExist(err)).To(BeFalse()) }) It("creates the cache folder with invalid size", func() { - fc, err := NewFileCache("test", "abc", "test", 10, nil) + fc, err := NewFileCache("test", "abc", "test", 0, nil) Expect(err).To(BeNil()) Expect(fc.cache).ToNot(BeNil()) Expect(fc.disabled).To(BeFalse()) }) It("returns empty if cache size is '0'", func() { - fc, err := NewFileCache("test", "0", "test", 10, nil) + fc, err := NewFileCache("test", "0", "test", 0, nil) Expect(err).To(BeNil()) Expect(fc.cache).To(BeNil()) Expect(fc.disabled).To(BeTrue()) @@ -42,6 +46,46 @@ var _ = Describe("File Caches", func() { }) Describe("FileCache", func() { + It("caches data if cache is enabled", func() { + called := false + fc, _ := NewFileCache("test", "1KB", "test", 0, func(ctx context.Context, arg fmt.Stringer) (io.Reader, error) { + called = true + return strings.NewReader(arg.String()), nil + }) + // First call is a MISS + s, err := fc.Get(context.TODO(), &testArg{"test"}) + Expect(err).To(BeNil()) + Expect(ioutil.ReadAll(s)).To(Equal([]byte("test"))) + // Second call is a HIT + called = false + s, err = fc.Get(context.TODO(), &testArg{"test"}) + Expect(err).To(BeNil()) + Expect(ioutil.ReadAll(s)).To(Equal([]byte("test"))) + Expect(called).To(BeFalse()) + }) + + It("does not cache data if cache is disabled", func() { + called := false + fc, _ := NewFileCache("test", "0", "test", 0, func(ctx context.Context, arg fmt.Stringer) (io.Reader, error) { + called = true + return strings.NewReader(arg.String()), nil + }) + // First call is a MISS + s, err := fc.Get(context.TODO(), &testArg{"test"}) + Expect(err).To(BeNil()) + Expect(ioutil.ReadAll(s)).To(Equal([]byte("test"))) + + // Second call is also a MISS + called = false + s, err = fc.Get(context.TODO(), &testArg{"test"}) + Expect(err).To(BeNil()) + Expect(ioutil.ReadAll(s)).To(Equal([]byte("test"))) + Expect(called).To(BeTrue()) + }) }) }) + +type testArg struct{ s string } + +func (t *testArg) String() string { return t.s } diff --git a/core/media_streamer.go b/core/media_streamer.go index 5c9d6dc91..e92240084 100644 --- a/core/media_streamer.go +++ b/core/media_streamer.go @@ -83,7 +83,7 @@ func (ms *mediaStreamer) NewStream(ctx context.Context, id string, reqFormat str } r, err := ms.cache.Get(ctx, job) if err != nil { - log.Error(ctx, "Error accessing cache", "id", mf.ID, err) + log.Error(ctx, "Error accessing transcoding cache", "id", mf.ID, err) return nil, err } diff --git a/core/media_streamer_test.go b/core/media_streamer_test.go index d7ea3c195..0f7c7bd17 100644 --- a/core/media_streamer_test.go +++ b/core/media_streamer_test.go @@ -21,7 +21,6 @@ var _ = Describe("MediaStreamer", func() { var ds model.DataStore ffmpeg := &fakeFFmpeg{Data: "fake data"} ctx := log.NewContext(context.TODO()) - log.SetLevel(log.LevelTrace) BeforeEach(func() { conf.Server.DataFolder, _ = ioutil.TempDir("", "file_caches") @@ -61,6 +60,8 @@ var _ = Describe("MediaStreamer", func() { It("returns a seekable stream if the file is complete in the cache", func() { s, err := streamer.NewStream(ctx, "123", "mp3", 32) Expect(err).To(BeNil()) + _, _ = ioutil.ReadAll(s) + _ = s.Close() Eventually(func() bool { return ffmpeg.closed }, "3s").Should(BeTrue()) s, err = streamer.NewStream(ctx, "123", "mp3", 32)