Created dedicated artwork readers

core/artwork/artwork.go

@@ -1,21 +1,13 @@
 package artwork
 
 import (
-	"bufio"
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
-	"image"
 	_ "image/gif"
-	"image/jpeg"
-	"image/png"
 	"io"
-	"net/http"
-	"strings"
 	"time"
 
-	"github.com/disintegration/imaging"
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/consts"
 	"github.com/navidrome/navidrome/core/ffmpeg"
@@ -40,12 +32,17 @@ type artwork struct {
 	ffmpeg ffmpeg.FFmpeg
 }
 
-func (a *artwork) Get(ctx context.Context, id string, size int) (io.ReadCloser, error) {
+type artworkReader interface {
+	cache.Item
+	LastUpdated() time.Time
+	Reader(ctx context.Context) (io.ReadCloser, string, error)
+}
+
+func (a *artwork) Get(ctx context.Context, id string, size int) (reader io.ReadCloser, err error) {
 	ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
 	defer cancel()
 
 	var artID model.ArtworkID
-	var err error
 	if id != "" {
 		artID, err = model.ParseArtworkID(id)
 		if err != nil {
@@ -53,181 +50,49 @@ func (a *artwork) Get(ctx context.Context, id string, size int) (io.ReadCloser,
 		}
 	}
 
-	item := &artItem{a: a, artID: artID, size: size}
+	var artReader artworkReader
+	switch artID.Kind {
+	case model.KindAlbumArtwork:
+		artReader, err = newAlbumArtworkReader(ctx, a, artID)
+	case model.KindMediaFileArtwork:
+		artReader, err = newMediafileArtworkReader(ctx, a, artID)
+	default:
+		artReader, err = newEmptyIDReader(ctx, artID)
+	}
+	if err != nil {
+		return nil, err
+	}
+	if size > 0 {
+		artReader = resizedFromOriginal(artReader, artID, size)
+	}
 
-	r, err := a.cache.Get(ctx, item)
+	r, err := a.cache.Get(ctx, artReader)
 	if err != nil && !errors.Is(err, context.Canceled) {
 		log.Error(ctx, "Error accessing image cache", "id", id, "size", size, err)
 	}
 	return r, err
 }
 
-func (a *artwork) get(ctx context.Context, artID model.ArtworkID, size int) (reader io.ReadCloser, path string, err error) {
-	// If requested a resized image, get the original (possibly from cache)
-	if size > 0 {
-		r, err := a.Get(ctx, artID.String(), 0)
-		if err != nil {
-			return nil, "", err
-		}
-		defer r.Close()
-		resized, err := a.resizedFromOriginal(ctx, artID, r, size)
-		return io.NopCloser(resized), fmt.Sprintf("%s@%d", artID, size), err
+type cacheItem struct {
+	artID      model.ArtworkID
+	size       int
+	lastUpdate time.Time
 }
 
-	switch artID.Kind {
-	case model.KindAlbumArtwork:
-		reader, path = a.extractAlbumImage(ctx, artID)
-	case model.KindMediaFileArtwork:
-		reader, path = a.extractMediaFileImage(ctx, artID)
-	default:
-		reader, path, _ = fromPlaceholder()()
-	}
-	return reader, path, ctx.Err()
-}
-
-func (a *artwork) extractAlbumImage(ctx context.Context, artID model.ArtworkID) (io.ReadCloser, string) {
-	al, err := a.ds.Album(ctx).Get(artID.ID)
-	if errors.Is(err, model.ErrNotFound) {
-		r, path, _ := fromPlaceholder()()
-		return r, path
-	}
-	if err != nil {
-		log.Error(ctx, "Could not retrieve album", "id", artID.ID, err)
-		return nil, ""
-	}
-	var ff = fromCoverArtPriority(ctx, a.ffmpeg, conf.Server.CoverArtPriority, *al)
-	ff = append(ff, fromPlaceholder())
-	return extractImage(ctx, artID, ff...)
-}
-
-func (a *artwork) extractMediaFileImage(ctx context.Context, artID model.ArtworkID) (reader io.ReadCloser, path string) {
-	mf, err := a.ds.MediaFile(ctx).Get(artID.ID)
-	if errors.Is(err, model.ErrNotFound) {
-		r, path, _ := fromPlaceholder()()
-		return r, path
-	}
-	if err != nil {
-		log.Error(ctx, "Could not retrieve mediafile", "id", artID.ID, err)
-		return nil, ""
-	}
-
-	var ff []sourceFunc
-	if mf.CoverArtID().Kind == model.KindMediaFileArtwork {
-		ff = []sourceFunc{
-			fromTag(mf.Path),
-			fromFFmpegTag(ctx, a.ffmpeg, mf.Path),
-		}
-	}
-	ff = append(ff, a.fromAlbum(ctx, mf.AlbumCoverArtID()))
-	return extractImage(ctx, artID, ff...)
-}
-
-func (a *artwork) resizedFromOriginal(ctx context.Context, artID model.ArtworkID, original io.Reader, size int) (io.Reader, error) {
-	// Keep a copy of the original data. In case we can't resize it, send it as is
-	buf := new(bytes.Buffer)
-	r := io.TeeReader(original, buf)
-
-	resized, err := resizeImage(r, size)
-	if err != nil {
-		log.Warn(ctx, "Could not resize image. Will return image as is", "artID", artID, "size", size, err)
-		// Force finish reading any remaining data
-		_, _ = io.Copy(io.Discard, r)
-		return buf, nil
-	}
-	return resized, nil
-}
-
-func extractImage(ctx context.Context, artID model.ArtworkID, extractFuncs ...sourceFunc) (io.ReadCloser, string) {
-	for _, f := range extractFuncs {
-		if ctx.Err() != nil {
-			return nil, ""
-		}
-		r, path, err := f()
-		if r != nil {
-			log.Trace(ctx, "Found artwork", "artID", artID, "path", path, "source", f)
-			return r, path
-		}
-		log.Trace(ctx, "Tried to extract artwork", "artID", artID, "source", f, err)
-	}
-	log.Error(ctx, "extractImage should never reach this point!", "artID", artID, "path")
-	return nil, ""
-}
-
-func fromCoverArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string, al model.Album) []sourceFunc {
-	var ff []sourceFunc
-	for _, pattern := range strings.Split(strings.ToLower(priority), ",") {
-		pattern = strings.TrimSpace(pattern)
-		if pattern == "embedded" {
-			ff = append(ff, fromTag(al.EmbedArtPath), fromFFmpegTag(ctx, ffmpeg, al.EmbedArtPath))
-			continue
-		}
-		if al.ImageFiles != "" {
-			ff = append(ff, fromExternalFile(ctx, al.ImageFiles, pattern))
-		}
-	}
-	return ff
-}
-
-func asImageReader(r io.Reader) (io.Reader, string, error) {
-	br := bufio.NewReader(r)
-	buf, err := br.Peek(512)
-	if err != nil {
-		return nil, "", err
-	}
-	return br, http.DetectContentType(buf), nil
-}
-
-func resizeImage(reader io.Reader, size int) (io.Reader, error) {
-	r, format, err := asImageReader(reader)
-	if err != nil {
-		return nil, err
-	}
-
-	img, _, err := image.Decode(r)
-	if err != nil {
-		return nil, err
-	}
-
-	// Preserve the aspect ratio of the image.
-	var m *image.NRGBA
-	bounds := img.Bounds()
-	if bounds.Max.X > bounds.Max.Y {
-		m = imaging.Resize(img, size, 0, imaging.Lanczos)
-	} else {
-		m = imaging.Resize(img, 0, size, imaging.Lanczos)
-	}
-
-	buf := new(bytes.Buffer)
-	buf.Reset()
-	if format == "image/png" {
-		err = png.Encode(buf, m)
-	} else {
-		err = jpeg.Encode(buf, m, &jpeg.Options{Quality: conf.Server.CoverJpegQuality})
-	}
-	return buf, err
+func (i *cacheItem) Key() string {
+	return fmt.Sprintf("%s.%d.%d.%d", i.artID.ID, i.lastUpdate.UnixMilli(), i.size, conf.Server.CoverJpegQuality)
 }
 
 type imageCache struct {
 	cache.FileCache
 }
 
-type artItem struct {
-	a     *artwork
-	artID model.ArtworkID
-	size  int
-}
-
-func (k *artItem) Key() string {
-	return fmt.Sprintf("%s.%d.%d", k.artID, k.size, conf.Server.CoverJpegQuality)
-}
-
 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) {
-					info := arg.(*artItem)
-					r, _, err := info.a.get(ctx, info.artID, info.size)
+					r, _, err := arg.(artworkReader).Reader(ctx)
 					return r, err
 				}),
 		}

core/artwork/artwork_internal_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"image"
 	"io"
+	"testing"
 
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/conf/configtest"
@@ -16,6 +17,13 @@ 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

core/artwork/reader_album.go

@@ -0,0 +1,58 @@
+package artwork
+
+import (
+	"context"
+	"io"
+	"strings"
+	"time"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/core/ffmpeg"
+	"github.com/navidrome/navidrome/model"
+)
+
+type albumArtworkReader struct {
+	cacheItem
+	a     *artwork
+	album model.Album
+}
+
+func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.ArtworkID) (*albumArtworkReader, error) {
+	al, err := artwork.ds.Album(ctx).Get(artID.ID)
+	if err != nil {
+		return nil, err
+	}
+	a := &albumArtworkReader{
+		a:     artwork,
+		album: *al,
+	}
+	a.cacheItem.artID = artID
+	a.cacheItem.lastUpdate = al.UpdatedAt
+	return a, nil
+}
+
+func (a *albumArtworkReader) LastUpdated() time.Time {
+	return a.album.UpdatedAt
+}
+
+func (a *albumArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) {
+	var ff = fromCoverArtPriority(ctx, a.a.ffmpeg, conf.Server.CoverArtPriority, a.album)
+	ff = append(ff, fromPlaceholder())
+	r, source := extractImage(ctx, a.artID, ff...)
+	return r, source, nil
+}
+
+func fromCoverArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string, al model.Album) []sourceFunc {
+	var ff []sourceFunc
+	for _, pattern := range strings.Split(strings.ToLower(priority), ",") {
+		pattern = strings.TrimSpace(pattern)
+		if pattern == "embedded" {
+			ff = append(ff, fromTag(al.EmbedArtPath), fromFFmpegTag(ctx, ffmpeg, al.EmbedArtPath))
+			continue
+		}
+		if al.ImageFiles != "" {
+			ff = append(ff, fromExternalFile(ctx, al.ImageFiles, pattern))
+		}
+	}
+	return ff
+}

core/artwork/reader_emptyid.go

@@ -0,0 +1,35 @@
+package artwork
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"time"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/model"
+)
+
+type emptyIDReader struct {
+	artID model.ArtworkID
+}
+
+func newEmptyIDReader(_ context.Context, artID model.ArtworkID) (*emptyIDReader, error) {
+	a := &emptyIDReader{
+		artID: artID,
+	}
+	return a, nil
+}
+
+func (a *emptyIDReader) LastUpdated() time.Time {
+	return time.Now() // Basically make it non-cacheable
+}
+
+func (a *emptyIDReader) Key() string {
+	return fmt.Sprintf("0.%d.0.%d", a.LastUpdated().UnixMilli(), conf.Server.CoverJpegQuality)
+}
+
+func (a *emptyIDReader) Reader(ctx context.Context) (io.ReadCloser, string, error) {
+	r, source := extractImage(ctx, a.artID, fromPlaceholder())
+	return r, source, nil
+}

core/artwork/reader_mediafile.go

@@ -0,0 +1,65 @@
+package artwork
+
+import (
+	"context"
+	"io"
+	"time"
+
+	"github.com/navidrome/navidrome/model"
+)
+
+type mediafileArtworkReader struct {
+	cacheItem
+	a         *artwork
+	mediafile model.MediaFile
+	album     model.Album
+}
+
+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
+	}
+	a := &mediafileArtworkReader{
+		a:         artwork,
+		mediafile: *mf,
+		album:     *al,
+	}
+	a.cacheItem.artID = artID
+	a.cacheItem.lastUpdate = a.LastUpdated()
+	return a, nil
+}
+
+func (a *mediafileArtworkReader) LastUpdated() time.Time {
+	if a.album.UpdatedAt.After(a.mediafile.UpdatedAt) {
+		return a.album.UpdatedAt
+	}
+	return a.mediafile.UpdatedAt
+}
+
+func (a *mediafileArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) {
+	var ff []sourceFunc
+	if a.mediafile.CoverArtID().Kind == model.KindMediaFileArtwork {
+		ff = []sourceFunc{
+			fromTag(a.mediafile.Path),
+			fromFFmpegTag(ctx, a.a.ffmpeg, a.mediafile.Path),
+		}
+	}
+	ff = append(ff, fromAlbum(ctx, a.a, a.mediafile.AlbumCoverArtID()))
+	r, source := extractImage(ctx, a.artID, ff...)
+	return r, source, nil
+}
+
+func fromAlbum(ctx context.Context, a *artwork, id model.ArtworkID) sourceFunc {
+	return func() (io.ReadCloser, string, error) {
+		r, err := a.Get(ctx, id.String(), 0)
+		if err != nil {
+			return nil, "", err
+		}
+		return r, id.String(), nil
+	}
+}

core/artwork/reader_resized.go

@@ -0,0 +1,96 @@
+package artwork
+
+import (
+	"bufio"
+	"bytes"
+	"context"
+	"fmt"
+	"image"
+	"image/jpeg"
+	"image/png"
+	"io"
+	"net/http"
+	"time"
+
+	"github.com/disintegration/imaging"
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/utils/number"
+)
+
+type resizedArtworkReader struct {
+	cacheItem
+	original artworkReader
+}
+
+func resizedFromOriginal(original artworkReader, artID model.ArtworkID, size int) *resizedArtworkReader {
+	r := &resizedArtworkReader{original: original}
+	r.cacheItem.artID = artID
+	r.cacheItem.size = size
+	r.cacheItem.lastUpdate = original.LastUpdated()
+	return r
+}
+
+func (a *resizedArtworkReader) LastUpdated() time.Time {
+	return a.lastUpdate
+}
+
+func (a *resizedArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) {
+	orig, path, err := a.original.Reader(ctx)
+	if err != nil {
+		return nil, "", err
+	}
+	// Keep a copy of the original data. In case we can't resize it, send it as is
+	buf := new(bytes.Buffer)
+	r := io.TeeReader(orig, buf)
+
+	resized, origSize, err := resizeImage(r, a.size)
+	log.Trace(ctx, "Resizing artwork", "artID", a.artID, "original", origSize, "resized", a.size)
+	if err != nil {
+		log.Warn(ctx, "Could not resize image. Will return image as is", "artID", a.artID, "size", a.size, err)
+		// Force finish reading any remaining data
+		_, _ = io.Copy(io.Discard, r)
+		return io.NopCloser(buf), "", nil
+	}
+	return io.NopCloser(resized), fmt.Sprintf("%s@%d", path, a.size), nil
+}
+
+func asImageReader(r io.Reader) (io.Reader, string, error) {
+	br := bufio.NewReader(r)
+	buf, err := br.Peek(512)
+	if err != nil {
+		return nil, "", err
+	}
+	return br, http.DetectContentType(buf), nil
+}
+
+func resizeImage(reader io.Reader, size int) (io.Reader, int, error) {
+	r, format, err := asImageReader(reader)
+	if err != nil {
+		return nil, 0, err
+	}
+
+	img, _, err := image.Decode(r)
+	if err != nil {
+		return nil, 0, err
+	}
+
+	// Preserve the aspect ratio of the image.
+	var m *image.NRGBA
+	bounds := img.Bounds()
+	if bounds.Max.X > bounds.Max.Y {
+		m = imaging.Resize(img, size, 0, imaging.Lanczos)
+	} else {
+		m = imaging.Resize(img, 0, size, imaging.Lanczos)
+	}
+
+	buf := new(bytes.Buffer)
+	buf.Reset()
+	if format == "image/png" {
+		err = png.Encode(buf, m)
+	} else {
+		err = jpeg.Encode(buf, m, &jpeg.Options{Quality: conf.Server.CoverJpegQuality})
+	}
+	return buf, number.Max(bounds.Max.X, bounds.Max.Y), err
+}

core/artwork/sources.go

@@ -19,26 +19,32 @@ import (
 	"github.com/navidrome/navidrome/resources"
 )
 
-type sourceFunc func() (io.ReadCloser, string, error)
+func extractImage(ctx context.Context, artID model.ArtworkID, extractFuncs ...sourceFunc) (io.ReadCloser, string) {
+	for _, f := range extractFuncs {
+		if ctx.Err() != nil {
+			return nil, ""
+		}
+		r, path, err := f()
+		if r != nil {
+			log.Trace(ctx, "Found artwork", "artID", artID, "path", path, "source", f)
+			return r, path
+		}
+		log.Trace(ctx, "Tried to extract artwork", "artID", artID, "source", f, err)
+	}
+	log.Error(ctx, "extractImage should never reach this point!", "artID", artID, "path")
+	return nil, ""
+}
+
+type sourceFunc func() (r io.ReadCloser, path string, err error)
 
 func (f sourceFunc) String() string {
 	name := runtime.FuncForPC(reflect.ValueOf(f).Pointer()).Name()
-	name = strings.TrimPrefix(name, "github.com/navidrome/navidrome/core.")
+	name = strings.TrimPrefix(name, "github.com/navidrome/navidrome/core/artwork.")
 	name = strings.TrimPrefix(name, "(*artwork).")
 	name = strings.TrimSuffix(name, ".func1")
 	return name
 }
 
-func (a *artwork) fromAlbum(ctx context.Context, id model.ArtworkID) sourceFunc {
-	return func() (io.ReadCloser, string, error) {
-		r, path, err := a.get(ctx, id, 0)
-		if err != nil {
-			return nil, "", err
-		}
-		return r, path, nil
-	}
-}
-
 func fromExternalFile(ctx context.Context, files string, pattern string) sourceFunc {
 	return func() (io.ReadCloser, string, error) {
 		for _, file := range filepath.SplitList(files) {

model/artwork_id.go

@@ -3,9 +3,7 @@ package model
 import (
 	"errors"
 	"fmt"
-	"strconv"
 	"strings"
-	"time"
 )
 
 type Kind struct{ prefix string }
@@ -18,33 +16,23 @@ var (
 type ArtworkID struct {
 	Kind Kind
 	ID   string
-	LastUpdate time.Time
 }
 
 func (id ArtworkID) String() string {
-	s := fmt.Sprintf("%s-%s", id.Kind.prefix, id.ID)
-	if id.LastUpdate.Unix() < 0 {
-		return s + "-0"
-	}
-	return fmt.Sprintf("%s-%x", s, id.LastUpdate.Unix())
+	return fmt.Sprintf("%s-%s", id.Kind.prefix, id.ID)
 }
 
 func ParseArtworkID(id string) (ArtworkID, error) {
 	parts := strings.Split(id, "-")
-	if len(parts) != 3 {
+	if len(parts) != 2 {
 		return ArtworkID{}, errors.New("invalid artwork id")
 	}
-	lastUpdate, err := strconv.ParseInt(parts[2], 16, 64)
-	if err != nil {
-		return ArtworkID{}, err
-	}
 	if parts[0] != KindAlbumArtwork.prefix && parts[0] != KindMediaFileArtwork.prefix {
 		return ArtworkID{}, errors.New("invalid artwork kind")
 	}
 	return ArtworkID{
 		Kind: Kind{parts[0]},
 		ID:   parts[1],
-		LastUpdate: time.Unix(lastUpdate, 0),
 	}, nil
 }
 
@@ -60,7 +48,6 @@ func artworkIDFromAlbum(al Album) ArtworkID {
 	return ArtworkID{
 		Kind: KindAlbumArtwork,
 		ID:   al.ID,
-		LastUpdate: al.UpdatedAt,
 	}
 }
 
@@ -68,6 +55,5 @@ func artworkIDFromMediaFile(mf MediaFile) ArtworkID {
 	return ArtworkID{
 		Kind: KindMediaFileArtwork,
 		ID:   mf.ID,
-		LastUpdate: mf.UpdatedAt,
 	}
 }

model/mediafile.go

@@ -78,7 +78,7 @@ func (mf MediaFile) CoverArtID() ArtworkID {
 }
 
 func (mf MediaFile) AlbumCoverArtID() ArtworkID {
-	return artworkIDFromAlbum(Album{ID: mf.AlbumID, UpdatedAt: mf.UpdatedAt})
+	return artworkIDFromAlbum(Album{ID: mf.AlbumID})
 }
 
 type MediaFiles []MediaFile

ui/src/subsonic/index.js

@@ -51,12 +51,10 @@ const getCoverArtUrl = (record, size) => {
   }
 
   // TODO Move this logic to server. `song` and `album` should have a CoverArtID
-  const lastUpdate = Math.floor(Date.parse(record.updatedAt) / 1000)
-  const id = record.id + '-' + Math.max(lastUpdate, 0).toString(16)
   if (record.album) {
-    return baseUrl(url('getCoverArt', 'mf-' + id, options))
+    return baseUrl(url('getCoverArt', 'mf-' + record.id, options))
   } else {
-    return baseUrl(url('getCoverArt', 'al-' + id, options))
+    return baseUrl(url('getCoverArt', 'al-' + record.id, options))
   }
 }