diff --git a/core/artwork.go b/core/artwork.go index 8bb319b97..cfdf8f9d6 100644 --- a/core/artwork.go +++ b/core/artwork.go @@ -10,6 +10,7 @@ import ( "image/jpeg" _ "image/png" "io" + "io/ioutil" "os" "strings" "sync" @@ -29,7 +30,7 @@ import ( ) type Artwork interface { - Get(ctx context.Context, id string, size int, out io.Writer) error + Get(ctx context.Context, id string, size int) (io.ReadCloser, error) } type ArtworkCache cache.FileCache @@ -55,10 +56,10 @@ func (ci *imageInfo) Key() string { return fmt.Sprintf("%s.%d.%s.%d", ci.path, ci.size, ci.lastUpdate.Format(time.RFC3339Nano), conf.Server.CoverJpegQuality) } -func (a *artwork) Get(ctx context.Context, id string, size int, out io.Writer) error { +func (a *artwork) Get(ctx context.Context, id string, size int) (io.ReadCloser, error) { path, lastUpdate, err := a.getImagePath(ctx, id) if err != nil && err != model.ErrNotFound { - return err + return nil, err } if !conf.Server.DevFastAccessCoverArt { @@ -78,12 +79,9 @@ func (a *artwork) Get(ctx context.Context, id string, size int, out io.Writer) e r, err := a.cache.Get(ctx, info) if err != nil { log.Error(ctx, "Error accessing image cache", "path", path, "size", size, err) - return err + return nil, err } - defer r.Close() - - _, err = io.Copy(out, r) - return err + return r, err } func (a *artwork) getImagePath(ctx context.Context, id string) (path string, lastUpdated time.Time, err error) { @@ -126,7 +124,7 @@ func (a *artwork) getImagePath(ctx context.Context, id string) (path string, las return a.getImagePath(ctx, "al-"+mf.AlbumID) } -func (a *artwork) getArtwork(ctx context.Context, id string, path string, size int) (reader io.Reader, err error) { +func (a *artwork) getArtwork(ctx context.Context, id string, path string, size int) (reader io.ReadCloser, err error) { defer func() { if err != nil { log.Warn(ctx, "Error extracting image", "path", path, "size", size, err) @@ -138,35 +136,28 @@ func (a *artwork) getArtwork(ctx context.Context, id string, path string, size i return nil, errors.New("empty path given for artwork") } - var data []byte - if size == 0 { // If requested original size, just read from the file if utils.IsAudioFile(path) { - data, err = readFromTag(path) + reader, err = readFromTag(path) } else { - data, err = readFromFile(path) + reader, err = readFromFile(path) } } else { // If requested a resized image, get the original (possibly from cache) and resize it - buf := new(bytes.Buffer) - err = a.Get(ctx, id, 0, buf) + var r io.ReadCloser + r, err = a.Get(ctx, id, 0) if err != nil { return } - data, err = resizeImage(buf, size) - } - - // Confirm the image is valid. Costly, but necessary - _, _, err = image.Decode(bytes.NewReader(data)) - if err == nil { - reader = bytes.NewReader(data) + defer r.Close() + reader, err = resizeImage(r, size) } return } -func resizeImage(reader io.Reader, size int) ([]byte, error) { +func resizeImage(reader io.Reader, size int) (io.ReadCloser, error) { img, _, err := image.Decode(reader) if err != nil { return nil, err @@ -183,10 +174,10 @@ func resizeImage(reader io.Reader, size int) ([]byte, error) { buf := new(bytes.Buffer) err = jpeg.Encode(buf, m, &jpeg.Options{Quality: conf.Server.CoverJpegQuality}) - return buf.Bytes(), err + return ioutil.NopCloser(buf), err } -func readFromTag(path string) ([]byte, error) { +func readFromTag(path string) (io.ReadCloser, error) { f, err := os.Open(path) if err != nil { return nil, err @@ -202,22 +193,11 @@ func readFromTag(path string) ([]byte, error) { if picture == nil { return nil, errors.New("file does not contain embedded art") } - return picture.Data, nil + return ioutil.NopCloser(bytes.NewReader(picture.Data)), nil } -func readFromFile(path string) ([]byte, error) { - f, err := os.Open(path) - if err != nil { - return nil, err - } - defer f.Close() - - var buf bytes.Buffer - if _, err := buf.ReadFrom(f); err != nil { - return nil, err - } - - return buf.Bytes(), nil +func readFromFile(path string) (io.ReadCloser, error) { + return os.Open(path) } var ( diff --git a/core/artwork_test.go b/core/artwork_test.go index baab85864..125931b37 100644 --- a/core/artwork_test.go +++ b/core/artwork_test.go @@ -1,7 +1,6 @@ package core import ( - "bytes" "context" "image" "io/ioutil" @@ -42,102 +41,100 @@ var _ = Describe("Artwork", func() { }) It("retrieves the external artwork art for an album", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "al-444", 0) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "al-444", 0, buf)).To(BeNil()) - - _, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + _, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("jpeg")) + Expect(r.Close()).To(BeNil()) }) It("retrieves the embedded artwork art for an album", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "al-222", 0) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "al-222", 0, buf)).To(BeNil()) - - _, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + _, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("jpeg")) + Expect(r.Close()).To(BeNil()) }) It("returns the default artwork if album does not have artwork", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "al-333", 0) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "al-333", 0, buf)).To(BeNil()) - - _, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + _, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("png")) + Expect(r.Close()).To(BeNil()) }) It("returns the default artwork if album is not found", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "al-0101", 0) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "al-0101", 0, buf)).To(BeNil()) - - _, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + _, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("png")) + Expect(r.Close()).To(BeNil()) }) It("retrieves the original artwork art from a media_file", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "123", 0) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "123", 0, buf)).To(BeNil()) - - img, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + img, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("jpeg")) Expect(img.Bounds().Size().X).To(Equal(600)) Expect(img.Bounds().Size().Y).To(Equal(600)) + Expect(r.Close()).To(BeNil()) }) It("retrieves the album artwork art if media_file does not have one", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "456", 0) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "456", 0, buf)).To(BeNil()) - - _, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + _, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("jpeg")) + Expect(r.Close()).To(BeNil()) }) It("retrieves the album artwork by album id", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "222", 0) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "222", 0, buf)).To(BeNil()) - - _, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + _, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("jpeg")) + Expect(r.Close()).To(BeNil()) }) It("resized artwork art as requested", func() { - buf := new(bytes.Buffer) + r, err := artwork.Get(ctx, "123", 200) + Expect(err).To(BeNil()) - Expect(artwork.Get(ctx, "123", 200, buf)).To(BeNil()) - - img, format, err := image.Decode(bytes.NewReader(buf.Bytes())) + img, format, err := image.Decode(r) Expect(err).To(BeNil()) Expect(format).To(Equal("jpeg")) Expect(img.Bounds().Size().X).To(Equal(200)) Expect(img.Bounds().Size().Y).To(Equal(200)) + Expect(r.Close()).To(BeNil()) }) Context("Errors", func() { It("returns err if gets error from album table", func() { ds.Album(ctx).(*tests.MockAlbum).SetError(true) - buf := new(bytes.Buffer) - - Expect(artwork.Get(ctx, "al-222", 0, buf)).To(MatchError("Error!")) + _, err := artwork.Get(ctx, "al-222", 0) + Expect(err).To(MatchError("Error!")) }) It("returns err if gets error from media_file table", func() { ds.MediaFile(ctx).(*tests.MockMediaFile).SetError(true) - buf := new(bytes.Buffer) - - Expect(artwork.Get(ctx, "123", 0, buf)).To(MatchError("Error!")) + _, err := artwork.Get(ctx, "123", 0) + Expect(err).To(MatchError("Error!")) }) }) }) diff --git a/core/cache_warmer.go b/core/cache_warmer.go index 7e824a97f..0d455bc2c 100644 --- a/core/cache_warmer.go +++ b/core/cache_warmer.go @@ -2,6 +2,7 @@ package core import ( "context" + "io" "io/ioutil" "time" @@ -78,10 +79,13 @@ func (w *warmer) execute(workload interface{}) { ctx := context.Background() item := workload.(artworkItem) log.Trace(ctx, "Pre-caching album artwork", "albumID", item.albumID) - err := w.artwork.Get(ctx, item.albumID, 0, ioutil.Discard) + r, err := w.artwork.Get(ctx, item.albumID, 0) if err != nil { log.Warn("Error pre-caching artwork from album", "id", item.albumID, err) + return } + defer r.Close() + _, _ = io.Copy(ioutil.Discard, r) } type artworkItem struct { diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index db928ca02..fa5fa17f1 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -65,8 +65,8 @@ func (c *MediaRetrievalController) GetCoverArt(w http.ResponseWriter, r *http.Re size := utils.ParamInt(r, "size", 0) w.Header().Set("cache-control", "public, max-age=315360000") - err = c.artwork.Get(r.Context(), id, size, w) + imgReader, err := c.artwork.Get(r.Context(), id, size) switch { case err == model.ErrNotFound: log.Error(r, "Couldn't find coverArt", "id", id, err) @@ -76,5 +76,8 @@ func (c *MediaRetrievalController) GetCoverArt(w http.ResponseWriter, r *http.Re return nil, err } - return nil, nil + defer imgReader.Close() + _, err = io.Copy(w, imgReader) + + return nil, err } diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index dc2e6b00a..dd643b6d2 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -1,9 +1,11 @@ package subsonic import ( + "bytes" "context" "errors" "io" + "io/ioutil" "net/http/httptest" "github.com/deluan/navidrome/model" @@ -67,12 +69,11 @@ type fakeArtwork struct { recvSize int } -func (c *fakeArtwork) Get(ctx context.Context, id string, size int, out io.Writer) error { +func (c *fakeArtwork) Get(ctx context.Context, id string, size int) (io.ReadCloser, error) { if c.err != nil { - return c.err + return nil, c.err } c.recvId = id c.recvSize = size - _, err := out.Write([]byte(c.data)) - return err + return ioutil.NopCloser(bytes.NewReader([]byte(c.data))), nil }