diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index eb3c3806..09c5f361 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -12,8 +12,8 @@ import ( "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/core/agents/lastfm" "github.com/navidrome/navidrome/core/agents/listenbrainz" + "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/core/scrobbler" - "github.com/navidrome/navidrome/core/transcoder" "github.com/navidrome/navidrome/db" "github.com/navidrome/navidrome/persistence" "github.com/navidrome/navidrome/scanner" @@ -46,8 +46,8 @@ func CreateSubsonicAPIRouter() *subsonic.Router { sqlDB := db.Db() dataStore := persistence.New(sqlDB) fileCache := core.GetImageCache() - artwork := core.NewArtwork(dataStore, fileCache) - transcoderTranscoder := transcoder.New() + transcoderTranscoder := ffmpeg.New() + artwork := core.NewArtwork(dataStore, fileCache, transcoderTranscoder) transcodingCache := core.GetTranscodingCache() mediaStreamer := core.NewMediaStreamer(dataStore, transcoderTranscoder, transcodingCache) archiver := core.NewArchiver(mediaStreamer, dataStore) diff --git a/core/artwork.go b/core/artwork.go index 5b2ddf60..3e84d518 100644 --- a/core/artwork.go +++ b/core/artwork.go @@ -12,6 +12,8 @@ import ( "io" "os" "path/filepath" + "reflect" + "runtime" "strings" "time" @@ -19,6 +21,7 @@ import ( "github.com/disintegration/imaging" "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/resources" @@ -31,13 +34,14 @@ type Artwork interface { Get(ctx context.Context, id string, size int) (io.ReadCloser, error) } -func NewArtwork(ds model.DataStore, cache cache.FileCache) Artwork { - return &artwork{ds: ds, cache: cache} +func NewArtwork(ds model.DataStore, cache cache.FileCache, ffmpeg ffmpeg.FFmpeg) Artwork { + return &artwork{ds: ds, cache: cache, ffmpeg: ffmpeg} } type artwork struct { - ds model.DataStore - cache cache.FileCache + ds model.DataStore + cache cache.FileCache + ffmpeg ffmpeg.FFmpeg } func (a *artwork) Get(ctx context.Context, id string, size int) (io.ReadCloser, error) { @@ -95,6 +99,7 @@ func (a *artwork) extractAlbumImage(ctx context.Context, artID model.ArtworkID) fromExternalFile(al.ImageFiles, "albumart.png", "albumart.jpg", "albumart.jpeg", "albumart.webp"), fromExternalFile(al.ImageFiles, "front.png", "front.jpg", "front.jpeg", "front.webp"), fromTag(al.EmbedArtPath), + fromFFmpegTag(ctx, a.ffmpeg, al.EmbedArtPath), fromPlaceholder(), ) } @@ -112,6 +117,7 @@ func (a *artwork) extractMediaFileImage(ctx context.Context, artID model.Artwork return extractImage(ctx, artID, fromTag(mf.Path), + fromFFmpegTag(ctx, a.ffmpeg, mf.Path), a.fromAlbum(ctx, mf.AlbumCoverArtID()), ) } @@ -135,8 +141,9 @@ func (a *artwork) resizedFromOriginal(ctx context.Context, artID model.ArtworkID usePng := strings.ToLower(filepath.Ext(path)) == ".png" r, err = resizeImage(r, size, usePng) if err != nil { + log.Warn("Could not resize image", "artID", artID, "size", size, err) r, path := fromPlaceholder()() - return r, path, err + return r, path, nil } return r, fmt.Sprintf("%s@%d", path, size), nil } @@ -145,7 +152,7 @@ func extractImage(ctx context.Context, artID model.ArtworkID, extractFuncs ...fu for _, f := range extractFuncs { r, path := f() if r != nil { - log.Trace(ctx, "Found artwork", "artID", artID, "path", path) + log.Trace(ctx, "Found artwork", "artID", artID, "path", path, "from", getFunctionName(f)) return r, path } } @@ -153,6 +160,13 @@ func extractImage(ctx context.Context, artID model.ArtworkID, extractFuncs ...fu return nil, "" } +func getFunctionName(i interface{}) string { + name := runtime.FuncForPC(reflect.ValueOf(i).Pointer()).Name() + name = strings.TrimPrefix(name, "github.com/navidrome/navidrome/core.") + name = strings.TrimSuffix(name, ".func1") + return name +} + // This is a bit unoptimized, but we need to make sure the priority order of validNames // is preserved (i.e. png is better than jpg) func fromExternalFile(files string, validNames ...string) func() (io.ReadCloser, string) { @@ -199,6 +213,19 @@ func fromTag(path string) func() (io.ReadCloser, string) { } } +func fromFFmpegTag(ctx context.Context, ffmpeg ffmpeg.FFmpeg, path string) func() (io.ReadCloser, string) { + return func() (io.ReadCloser, string) { + if path == "" { + return nil, "" + } + r, err := ffmpeg.ExtractImage(ctx, path) + if err != nil { + return nil, "" + } + return r, path + } +} + func fromPlaceholder() func() (io.ReadCloser, string) { return func() (io.ReadCloser, string) { r, _ := resources.FS().Open(consts.PlaceholderAlbumArt) diff --git a/core/artwork_internal_test.go b/core/artwork_internal_test.go index d0b2980e..e7ead169 100644 --- a/core/artwork_internal_test.go +++ b/core/artwork_internal_test.go @@ -2,6 +2,7 @@ package core import ( "context" + "errors" "image" "github.com/navidrome/navidrome/conf" @@ -17,6 +18,7 @@ import ( var _ = Describe("Artwork", func() { var aw *artwork var ds model.DataStore + var ffmpeg *tests.MockFFmpeg ctx := log.NewContext(context.TODO()) var alOnlyEmbed, alEmbedNotFound, alOnlyExternal, alExternalNotFound, alAllOptions model.Album var mfWithEmbed, mfWithoutEmbed, mfCorruptedCover model.MediaFile @@ -38,7 +40,8 @@ var _ = Describe("Artwork", func() { conf.Server.ImageCacheSize = "0" // Disable cache cache := GetImageCache() - aw = NewArtwork(ds, cache).(*artwork) + ffmpeg = tests.NewMockFFmpeg("") + aw = NewArtwork(ds, cache, ffmpeg).(*artwork) }) Context("Empty ID", func() { @@ -70,6 +73,7 @@ var _ = Describe("Artwork", func() { 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) Expect(err).ToNot(HaveOccurred()) Expect(path).To(Equal(consts.PlaceholderAlbumArt)) @@ -124,13 +128,19 @@ var _ = Describe("Artwork", func() { Expect(err).ToNot(HaveOccurred()) Expect(path).To(Equal("tests/fixtures/test.mp3")) }) - It("returns album cover if media file has no cover art", func() { - _, path, err := aw.get(context.Background(), mfWithoutEmbed.CoverArtID(), 0) + It("returns embed cover if successfully extracted by ffmpeg", func() { + _, path, err := aw.get(context.Background(), mfCorruptedCover.CoverArtID(), 0) + Expect(err).ToNot(HaveOccurred()) + 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) Expect(err).ToNot(HaveOccurred()) Expect(path).To(Equal("tests/fixtures/front.png")) }) - It("returns album cover if cannot read embed artwork", func() { - _, path, err := aw.get(context.Background(), mfCorruptedCover.CoverArtID(), 0) + It("returns album cover if media file has no cover art", func() { + _, path, err := aw.get(context.Background(), mfWithoutEmbed.CoverArtID(), 0) Expect(err).ToNot(HaveOccurred()) Expect(path).To(Equal("tests/fixtures/front.png")) }) diff --git a/core/transcoder/transcoder.go b/core/ffmpeg/ffmpeg.go similarity index 55% rename from core/transcoder/transcoder.go rename to core/ffmpeg/ffmpeg.go index 1231df39..ed78e631 100644 --- a/core/transcoder/transcoder.go +++ b/core/ffmpeg/ffmpeg.go @@ -1,4 +1,4 @@ -package transcoder +package ffmpeg import ( "context" @@ -13,19 +13,32 @@ import ( "github.com/navidrome/navidrome/log" ) -type Transcoder interface { - Start(ctx context.Context, command, path string, maxBitRate int) (io.ReadCloser, error) +type FFmpeg interface { + Transcode(ctx context.Context, command, path string, maxBitRate int) (io.ReadCloser, error) + ExtractImage(ctx context.Context, path string) (io.ReadCloser, error) + // TODO Move scanner ffmpeg probe to here } -func New() Transcoder { - return &externalTranscoder{} +func New() FFmpeg { + return &ffmpeg{} } -type externalTranscoder struct{} +const extractImageCmd = "ffmpeg -i %s -an -vcodec copy -f image2pipe -" -func (e *externalTranscoder) Start(ctx context.Context, command, path string, maxBitRate int) (io.ReadCloser, error) { - args := createTranscodeCommand(command, path, maxBitRate) - log.Trace(ctx, "Executing transcoding command", "cmd", args) +type ffmpeg struct{} + +func (e *ffmpeg) Transcode(ctx context.Context, command, path string, maxBitRate int) (io.ReadCloser, error) { + args := createFFmpegCommand(command, path, maxBitRate) + return e.start(ctx, args) +} + +func (e *ffmpeg) ExtractImage(ctx context.Context, path string) (io.ReadCloser, error) { + args := createFFmpegCommand(extractImageCmd, path, 0) + return e.start(ctx, args) +} + +func (e *ffmpeg) start(ctx context.Context, args []string) (io.ReadCloser, error) { + log.Trace(ctx, "Executing ffmpeg command", "cmd", args) j := &Cmd{ctx: ctx, args: args} j.PipeReader, j.out = io.Pipe() err := j.start() @@ -47,7 +60,11 @@ type Cmd struct { func (j *Cmd) start() error { cmd := exec.CommandContext(j.ctx, j.args[0], j.args[1:]...) // #nosec cmd.Stdout = j.out - cmd.Stderr = os.Stderr + if log.CurrentLevel() >= log.LevelTrace { + cmd.Stderr = os.Stderr + } else { + cmd.Stderr = io.Discard + } j.cmd = cmd if err := cmd.Start(); err != nil { @@ -74,7 +91,7 @@ func (j *Cmd) wait() { } // Path will always be an absolute path -func createTranscodeCommand(cmd, path string, maxBitRate int) []string { +func createFFmpegCommand(cmd, path string, maxBitRate int) []string { split := strings.Split(cmd, " ") for i, s := range split { s = strings.ReplaceAll(s, "%s", path) diff --git a/core/transcoder/transcoder_test.go b/core/ffmpeg/ffmpeg_test.go similarity index 63% rename from core/transcoder/transcoder_test.go rename to core/ffmpeg/ffmpeg_test.go index 82722f7e..be8dad3b 100644 --- a/core/transcoder/transcoder_test.go +++ b/core/ffmpeg/ffmpeg_test.go @@ -1,4 +1,4 @@ -package transcoder +package ffmpeg import ( "testing" @@ -9,16 +9,16 @@ import ( . "github.com/onsi/gomega" ) -func TestTranscoder(t *testing.T) { +func TestFFmpeg(t *testing.T) { tests.Init(t, false) log.SetLevel(log.LevelFatal) RegisterFailHandler(Fail) - RunSpecs(t, "Transcoder Suite") + RunSpecs(t, "FFmpeg Suite") } -var _ = Describe("createTranscodeCommand", func() { +var _ = Describe("createFFmpegCommand", func() { It("creates a valid command line", func() { - args := createTranscodeCommand("ffmpeg -i %s -b:a %bk mp3 -", "/music library/file.mp3", 123) + args := createFFmpegCommand("ffmpeg -i %s -b:a %bk mp3 -", "/music library/file.mp3", 123) Expect(args).To(Equal([]string{"ffmpeg", "-i", "/music library/file.mp3", "-b:a", "123k", "mp3", "-"})) }) }) diff --git a/core/media_streamer.go b/core/media_streamer.go index 85038dc9..2a13233c 100644 --- a/core/media_streamer.go +++ b/core/media_streamer.go @@ -11,7 +11,7 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" - "github.com/navidrome/navidrome/core/transcoder" + "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" @@ -25,13 +25,13 @@ type MediaStreamer interface { type TranscodingCache cache.FileCache -func NewMediaStreamer(ds model.DataStore, t transcoder.Transcoder, cache TranscodingCache) MediaStreamer { +func NewMediaStreamer(ds model.DataStore, t ffmpeg.FFmpeg, cache TranscodingCache) MediaStreamer { return &mediaStreamer{ds: ds, transcoder: t, cache: cache} } type mediaStreamer struct { ds model.DataStore - transcoder transcoder.Transcoder + transcoder ffmpeg.FFmpeg cache cache.FileCache } @@ -191,7 +191,7 @@ func GetTranscodingCache() TranscodingCache { log.Error(ctx, "Error loading transcoding command", "format", job.format, err) return nil, os.ErrInvalid } - out, err := job.ms.transcoder.Start(ctx, t.Command, job.mf.Path, job.bitRate) + out, err := job.ms.transcoder.Transcode(ctx, t.Command, job.mf.Path, job.bitRate) if err != nil { log.Error(ctx, "Error starting transcoder", "id", job.mf.ID, err) return nil, os.ErrInvalid diff --git a/core/media_streamer_test.go b/core/media_streamer_test.go index 71145a84..f4d9c583 100644 --- a/core/media_streamer_test.go +++ b/core/media_streamer_test.go @@ -4,24 +4,21 @@ import ( "context" "io" "os" - "strings" - "sync" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" - . "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/tests" - "github.com/navidrome/navidrome/utils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) var _ = Describe("MediaStreamer", func() { - var streamer MediaStreamer + var streamer core.MediaStreamer var ds model.DataStore - ffmpeg := newFakeFFmpeg("fake data") + ffmpeg := tests.NewMockFFmpeg("fake data") ctx := log.NewContext(context.TODO()) BeforeEach(func() { @@ -32,9 +29,9 @@ var _ = Describe("MediaStreamer", func() { ds.MediaFile(ctx).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ {ID: "123", Path: "tests/fixtures/test.mp3", Suffix: "mp3", BitRate: 128, Duration: 257.0}, }) - testCache := GetTranscodingCache() + testCache := core.GetTranscodingCache() Eventually(func() bool { return testCache.Ready(context.TODO()) }).Should(BeTrue()) - streamer = NewMediaStreamer(ds, ffmpeg, testCache) + streamer = core.NewMediaStreamer(ds, ffmpeg, testCache) }) AfterEach(func() { _ = os.RemoveAll(conf.Server.DataFolder) @@ -75,32 +72,3 @@ var _ = Describe("MediaStreamer", func() { }) }) }) - -func newFakeFFmpeg(data string) *fakeFFmpeg { - return &fakeFFmpeg{Reader: strings.NewReader(data)} -} - -type fakeFFmpeg struct { - io.Reader - lock sync.Mutex - closed utils.AtomicBool -} - -func (ff *fakeFFmpeg) Start(ctx context.Context, cmd, path string, maxBitRate int) (f io.ReadCloser, err error) { - return ff, nil -} - -func (ff *fakeFFmpeg) Read(p []byte) (n int, err error) { - ff.lock.Lock() - defer ff.lock.Unlock() - return ff.Reader.Read(p) -} - -func (ff *fakeFFmpeg) Close() error { - ff.closed.Set(true) - return nil -} - -func (ff *fakeFFmpeg) IsClosed() bool { - return ff.closed.Get() -} diff --git a/core/wire_providers.go b/core/wire_providers.go index ec855b7f..50fef676 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -3,8 +3,8 @@ package core import ( "github.com/google/wire" "github.com/navidrome/navidrome/core/agents" + "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/core/scrobbler" - "github.com/navidrome/navidrome/core/transcoder" ) var Set = wire.NewSet( @@ -16,7 +16,7 @@ var Set = wire.NewSet( NewExternalMetadata, NewPlayers, agents.New, - transcoder.New, + ffmpeg.New, scrobbler.GetPlayTracker, NewShare, NewPlaylists, diff --git a/server/subsonic/api.go b/server/subsonic/api.go index 3c1ea411..e7118063 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -185,7 +185,7 @@ func hr(r chi.Router, path string, f handlerRaw) { if errors.Is(err, model.ErrNotFound) { err = newError(responses.ErrorDataNotFound, "data not found") } else { - err = newError(responses.ErrorGeneric, "Internal Error") + err = newError(responses.ErrorGeneric, fmt.Sprintf("Internal Server Error: %s", err)) } } sendError(w, r, err) diff --git a/tests/mock_ffmpeg.go b/tests/mock_ffmpeg.go new file mode 100644 index 00000000..735b1291 --- /dev/null +++ b/tests/mock_ffmpeg.go @@ -0,0 +1,52 @@ +package tests + +import ( + "context" + "io" + "strings" + "sync" + + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/resources" + "github.com/navidrome/navidrome/utils" +) + +func NewMockFFmpeg(data string) *MockFFmpeg { + return &MockFFmpeg{Reader: strings.NewReader(data)} +} + +type MockFFmpeg struct { + io.Reader + lock sync.Mutex + closed utils.AtomicBool + Error error +} + +func (ff *MockFFmpeg) Transcode(ctx context.Context, cmd, path string, maxBitRate int) (f io.ReadCloser, err error) { + if ff.Error != nil { + return nil, ff.Error + } + return ff, nil +} + +func (ff *MockFFmpeg) ExtractImage(ctx context.Context, path string) (io.ReadCloser, error) { + if ff.Error != nil { + return nil, ff.Error + } + return resources.FS().Open(consts.PlaceholderAlbumArt) +} + +func (ff *MockFFmpeg) Read(p []byte) (n int, err error) { + ff.lock.Lock() + defer ff.lock.Unlock() + return ff.Reader.Read(p) +} + +func (ff *MockFFmpeg) Close() error { + ff.closed.Set(true) + return nil +} + +func (ff *MockFFmpeg) IsClosed() bool { + return ff.closed.Get() +}