diff --git a/.github/actions/prepare-docker/action.yml b/.github/actions/prepare-docker/action.yml index 760a0528b..b8cde4aaf 100644 --- a/.github/actions/prepare-docker/action.yml +++ b/.github/actions/prepare-docker/action.yml @@ -53,13 +53,13 @@ runs: - name: Login to Docker Hub if: inputs.hub_username != '' && inputs.hub_password != '' - uses: docker/login-action@v3 + uses: docker/login-action@v4 with: username: ${{ inputs.hub_username }} password: ${{ inputs.hub_password }} - name: Login to GitHub Container Registry - uses: docker/login-action@v3 + uses: docker/login-action@v4 with: registry: ghcr.io username: ${{ github.actor }} @@ -67,12 +67,13 @@ runs: - name: Set up Docker Buildx id: buildx - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@v4 - name: Extract metadata for Docker image id: meta - uses: docker/metadata-action@v5 + uses: docker/metadata-action@v6 with: + github-token: ${{ inputs.github_token }} labels: | maintainer=deluan@navidrome.org images: | diff --git a/.github/workflows/download-link-on-pr.yml b/.github/workflows/download-link-on-pr.yml index 38b7b8a86..076f963d4 100644 --- a/.github/workflows/download-link-on-pr.yml +++ b/.github/workflows/download-link-on-pr.yml @@ -8,7 +8,7 @@ jobs: if: github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success' runs-on: ubuntu-latest steps: - - uses: actions/github-script@v3 + - uses: actions/github-script@v7 with: # This snippet is public-domain, taken from # https://github.com/oprypin/nightly.link/blob/master/.github/workflows/pr-comment.yml @@ -19,8 +19,7 @@ jobs: const pull_user_id = ${{github.event.sender.id}}; const issue_number = await (async () => { - const pulls = await github.pulls.list({owner, repo}); - for await (const {data} of github.paginate.iterator(pulls)) { + for await (const {data} of github.paginate.iterator(github.rest.pulls.list, {owner, repo})) { for (const pull of data) { if (pull.head.sha === pull_head_sha && pull.user.id === pull_user_id) { return pull.number; @@ -34,7 +33,7 @@ jobs: return core.error(`No matching pull request found`); } - const {data: {artifacts}} = await github.actions.listWorkflowRunArtifacts({owner, repo, run_id}); + const {data: {artifacts}} = await github.rest.actions.listWorkflowRunArtifacts({owner, repo, run_id}); if (!artifacts.length) { return core.error(`No artifacts found`); } @@ -43,12 +42,12 @@ jobs: body += `\n* [${art.name}.zip](https://nightly.link/${owner}/${repo}/actions/artifacts/${art.id}.zip)`; } - const {data: comments} = await github.issues.listComments({repo, owner, issue_number}); + const {data: comments} = await github.rest.issues.listComments({repo, owner, issue_number}); const existing_comment = comments.find((c) => c.user.login === 'github-actions[bot]'); if (existing_comment) { core.info(`Updating comment ${existing_comment.id}`); - await github.issues.updateComment({repo, owner, comment_id: existing_comment.id, body}); + await github.rest.issues.updateComment({repo, owner, comment_id: existing_comment.id, body}); } else { core.info(`Creating a comment`); - await github.issues.createComment({repo, owner, issue_number, body}); + await github.rest.issues.createComment({repo, owner, issue_number, body}); } diff --git a/.github/workflows/pipeline.yml b/.github/workflows/pipeline.yml index 09fca2572..6f858a5a7 100644 --- a/.github/workflows/pipeline.yml +++ b/.github/workflows/pipeline.yml @@ -145,7 +145,7 @@ jobs: - name: Cache ffmpeg id: ffmpeg-cache - uses: actions/cache@v4 + uses: actions/cache@v5 with: path: C:\ffmpeg key: ffmpeg-${{ env.FFMPEG_VERSION }}-win64 diff --git a/.github/workflows/stale.yml b/.github/workflows/stale.yml index 69e6ac99e..33c8fadbd 100644 --- a/.github/workflows/stale.yml +++ b/.github/workflows/stale.yml @@ -28,7 +28,7 @@ jobs: This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. - - uses: actions/stale@v9 + - uses: actions/stale@v10 with: operations-per-run: 999 days-before-issue-stale: 180 diff --git a/Dockerfile b/Dockerfile index 105656afb..ad1e2a41c 100644 --- a/Dockerfile +++ b/Dockerfile @@ -164,6 +164,7 @@ RUN touch /.nddockerenv EXPOSE ${ND_PORT} WORKDIR /app +ENV PATH="/app:${PATH}" ENTRYPOINT ["/app/navidrome"] diff --git a/Makefile b/Makefile index ad96afd31..e303017c7 100644 --- a/Makefile +++ b/Makefile @@ -20,7 +20,7 @@ IMAGE_PLATFORMS ?= $(shell echo $(SUPPORTED_PLATFORMS) | tr ',' '\n' | grep "lin PLATFORMS ?= $(SUPPORTED_PLATFORMS) DOCKER_TAG ?= deluan/navidrome:develop -GOLANGCI_LINT_VERSION ?= v2.11.1 +GOLANGCI_LINT_VERSION ?= v2.12.0 UI_SRC_FILES := $(shell find ui -type f -not -path "ui/build/*" -not -path "ui/node_modules/*") diff --git a/adapters/lastfm/agent.go b/adapters/lastfm/agent.go index b3e89a9dc..02c198120 100644 --- a/adapters/lastfm/agent.go +++ b/adapters/lastfm/agent.go @@ -416,6 +416,10 @@ func (l *lastfmAgent) IsAuthorized(ctx context.Context, userId string) bool { return err == nil && sk != "" } +func (l *lastfmAgent) PlaybackReport(context.Context, scrobbler.PlaybackSession) error { + return nil +} + func init() { conf.AddHook(func() { agents.Register(lastFMAgentName, func(ds model.DataStore) agents.Interface { diff --git a/adapters/listenbrainz/agent.go b/adapters/listenbrainz/agent.go index 019c6e9f4..826a9672e 100644 --- a/adapters/listenbrainz/agent.go +++ b/adapters/listenbrainz/agent.go @@ -212,6 +212,10 @@ func (l *listenBrainzAgent) GetSimilarSongsByTrack(ctx context.Context, id strin return songs, nil } +func (l *listenBrainzAgent) PlaybackReport(context.Context, scrobbler.PlaybackSession) error { + return nil +} + func init() { conf.AddHook(func() { if conf.Server.ListenBrainz.Enabled { diff --git a/conf/configuration.go b/conf/configuration.go index 9bd79d781..887e8c68a 100644 --- a/conf/configuration.go +++ b/conf/configuration.go @@ -95,6 +95,7 @@ type configOptions struct { EnableReplayGain bool EnableCoverAnimation bool EnableNowPlaying bool + UIPlaybackReportInterval time.Duration GATrackingID string EnableLogRedacting bool AuthRequestLimit int @@ -133,6 +134,7 @@ type configOptions struct { DevArtworkMaxRequests int DevArtworkThrottleBacklogLimit int DevArtworkThrottleBacklogTimeout time.Duration + DevArtworkThrottleBuffered bool DevArtistInfoTimeToLive time.Duration DevAlbumInfoTimeToLive time.Duration DevExternalScanner bool @@ -776,6 +778,7 @@ func setViperDefaults() { viper.SetDefault("enablereplaygain", true) viper.SetDefault("enablecoveranimation", true) viper.SetDefault("enablenowplaying", true) + viper.SetDefault("uiplaybackreportinterval", consts.DefaultUIPlaybackReportInterval) viper.SetDefault("enableartworkupload", true) viper.SetDefault("maximageuploadsize", consts.DefaultMaxImageUploadSize) viper.SetDefault("enablesharing", false) @@ -859,6 +862,7 @@ func setViperDefaults() { viper.SetDefault("devartworkmaxrequests", max(2, runtime.NumCPU()/2)) viper.SetDefault("devartworkthrottlebackloglimit", consts.RequestThrottleBacklogLimit) viper.SetDefault("devartworkthrottlebacklogtimeout", consts.RequestThrottleBacklogTimeout) + viper.SetDefault("devartworkthrottlebuffered", true) viper.SetDefault("devartistinfotimetolive", consts.ArtistInfoTimeToLive) viper.SetDefault("devalbuminfotimetolive", consts.AlbumInfoTimeToLive) viper.SetDefault("devexternalscanner", true) diff --git a/consts/consts.go b/consts/consts.go index 3db0b831a..bf32006d6 100644 --- a/consts/consts.go +++ b/consts/consts.go @@ -67,11 +67,12 @@ const ( ScanIgnoreFile = ".ndignore" ArtworkFolder = "artwork" - PlaceholderArtistArt = "artist-placeholder.webp" - PlaceholderAlbumArt = "album-placeholder.webp" - PlaceholderAvatar = "logo-192x192.png" - DefaultUIVolume = 100 - DefaultUISearchDebounceMs = 200 + PlaceholderArtistArt = "artist-placeholder.webp" + PlaceholderAlbumArt = "album-placeholder.webp" + PlaceholderAvatar = "logo-192x192.png" + DefaultUIVolume = 100 + DefaultUISearchDebounceMs = 200 + DefaultUIPlaybackReportInterval = time.Minute DefaultHttpClientTimeOut = 10 * time.Second diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index 3d5523afd..e765e1b1b 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -37,20 +37,20 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 2 variant: cover.* basenames tie across album-root and per-disc folders; - // compareImageFiles' lexicographic full-path tiebreaker ranks disc-subfolder - // files first. Flip from PIt to It once it prefers shorter/parent paths. + // https://github.com/navidrome/navidrome/issues/5376 + // cover.* basenames tie across album-root and per-disc folders; + // compareImageFiles must prefer shallower paths. When("a multi-disc album has a cover.jpg at the album root and per-disc covers", func() { // Artist/ // └── Album/ // ├── CD1/ // │ ├── 01 - Track.mp3 - // │ └── cover.jpg ← currently wins (bug) + // │ └── cover.jpg ← should not win // ├── CD2/ // │ ├── 01 - Track.mp3 // │ └── cover.jpg // └── cover.jpg ← should win (album-root fallback) - PIt("uses the album-root cover (currently picks a disc subfolder image — bug)", func() { + It("prefers the album-root cover over per-disc covers", func() { conf.Server.CoverArtPriority = defaultCoverPriority setLayout(fstest.MapFS{ "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), @@ -68,21 +68,20 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 2: folder.jpg basenames tie across album-root and per-disc folders; - // the lexicographic full-path tiebreaker in compareImageFiles ranks - // "Artist/Album/CD1/folder.jpg" ahead of "Artist/Album/folder.jpg". - // Flip from PIt to It once compareImageFiles prefers shorter/parent paths. + // https://github.com/navidrome/navidrome/issues/5376 + // folder.jpg basenames tie across album-root and per-disc folders; + // compareImageFiles must prefer shallower paths. When("a multi-disc album has folder.jpg at the album root AND in each disc subfolder", func() { // Artist/ // └── Album/ // ├── CD1/ // │ ├── 01 - Track.mp3 - // │ └── folder.jpg ← currently wins (bug) + // │ └── folder.jpg ← should not win // ├── CD2/ // │ ├── 01 - Track.mp3 // │ └── folder.jpg // └── folder.jpg ← should win (album-root fallback) - PIt("uses the album-root folder.jpg (currently picks a disc subfolder image — bug)", func() { + It("prefers the album-root folder.jpg over per-disc folder.jpg", func() { conf.Server.CoverArtPriority = defaultCoverPriority setLayout(fstest.MapFS{ "Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), @@ -98,17 +97,15 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 1: commonParentFolder's `len(folders) < 2` guard skips the parent-folder - // lookup whenever an album lives entirely under a single subfolder, so an - // album-root cover is never considered. Flip from PIt to It once the guard - // accepts single-folder albums whose parent isn't already in the folder set. + // https://github.com/navidrome/navidrome/issues/5376 + // Single-subfolder albums must still consider the parent folder's images. When("an album lives entirely under a single disc subfolder with cover.jpg at the parent", func() { // Artist/ // └── Album/ // ├── disc1/ // │ └── 01 - Track.mp3 - // └── cover.jpg ← should win (parent-folder fallback, currently ignored — bug) - PIt("uses the parent-folder cover (currently ignored — bug)", func() { + // └── cover.jpg ← should win (parent-folder fallback) + It("uses the parent-folder cover for single-disc-subfolder albums", func() { conf.Server.CoverArtPriority = defaultCoverPriority setLayout(fstest.MapFS{ "Artist/Album/disc1/01 - Track.mp3": trackFile(1, "Track"), @@ -121,6 +118,32 @@ var _ = Describe("Album artwork resolution", func() { }) }) + // https://github.com/navidrome/navidrome/issues/5456 + When("a top-level multi-disc album has cover.jpg at the album root and per-disc folder.jpg", func() { + // Album/ (top-level folder, Path=".") + // ├── CD1/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // ├── CD2/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── cover.jpg ← should win (album-root) + It("prefers the album-root cover.jpg", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), + "Album/CD2/01 - Track.mp3": trackFile(1, "Track CD2"), + "Album/cover.jpg": imageFile("album-root"), + "Album/CD1/folder.jpg": imageFile("disc1"), + "Album/CD2/folder.jpg": imageFile("disc2"), + }) + scan() + + al := firstAlbum() + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) + }) + }) + When("CoverArtPriority puts embedded first and the album has both embedded and external art", func() { // Artist/ // └── Album/ diff --git a/core/artwork/e2e/disc_test.go b/core/artwork/e2e/disc_test.go index 7569cbc32..667079458 100644 --- a/core/artwork/e2e/disc_test.go +++ b/core/artwork/e2e/disc_test.go @@ -1,6 +1,7 @@ package artworke2e_test import ( + "fmt" "testing/fstest" "github.com/navidrome/navidrome/conf" @@ -255,6 +256,100 @@ var _ = Describe("Disc artwork resolution", func() { }) }) + // Reproduces https://github.com/navidrome/navidrome/issues/5456 + // Deeply nested layout matching the reporter's actual structure. + When("a deeply nested multi-disc album has cover.jpg and per-disc folder.jpg", func() { + // Genre/Artist/Album/ ← album root with cover.jpg + // ├── cover.jpg ← album-level cover + // ├── Disc 01 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg ← disc 1 art + // ├── Disc 02 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── ... (12 discs) + It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { + conf.Server.DiscArtPriority = defaultDiscPriority + conf.Server.CoverArtPriority = defaultCoverPriority + discNames := []string{ + "Disc 01 (Birth of the Dead - The Studio Sides)", + "Disc 02 (Birth of the Dead - The Live Sides)", + "Disc 03 (The Grateful Dead)", + "Disc 04 (Anthem of the Sun)", + "Disc 05 (Aoxomoxoa)", + "Disc 06 (Live; Dead)", + "Disc 07 (Workingman's Dead)", + "Disc 08 (American Beauty)", + "Disc 09 (Grateful Dead)", + "Disc 10 (Europe '72)", + "Disc 11 (Europe '72)", + "Disc 12 (History of the Grateful Dead, Volume One (Bear's Choice))", + } + layout := fstest.MapFS{ + "Pop; Rock/Grateful Dead/(2001) The Golden Road/cover.jpg": imageFile("album-root-cover"), + } + for i, name := range discNames { + discNum := i + 1 + prefix := fmt.Sprintf("Pop; Rock/Grateful Dead/(2001) The Golden Road/%s/", name) + layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", discNum), map[string]any{"disc": fmt.Sprintf("%d", discNum)}) + layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", discNum)) + } + setLayout(layout) + scan() + + al := firstAlbum() + + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) + + for i := range discNames { + discNum := i + 1 + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, discNum), &al.UpdatedAt) + Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", discNum))), + "disc %d should use its own folder.jpg", discNum) + } + }) + }) + + // https://github.com/navidrome/navidrome/issues/5456 + // Top-level album variant — album folder at library root (Path="."). + When("a top-level multi-disc album has cover.jpg and per-disc folder.jpg", func() { + // Album/ (top-level, Path=".") + // ├── cover.jpg ← album-level cover + // ├── Disc 01/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg ← disc 1 art + // ├── Disc 02/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── Disc 03/ + // ├── 01 - Track.mp3 + // └── folder.jpg + It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { + conf.Server.DiscArtPriority = defaultDiscPriority + conf.Server.CoverArtPriority = defaultCoverPriority + layout := fstest.MapFS{ + "Album/cover.jpg": imageFile("album-root-cover"), + } + for i := 1; i <= 3; i++ { + prefix := fmt.Sprintf("Album/Disc %02d/", i) + layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", i), map[string]any{"disc": fmt.Sprintf("%d", i)}) + layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", i)) + } + setLayout(layout) + scan() + + al := firstAlbum() + + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) + + for i := 1; i <= 3; i++ { + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, i), &al.UpdatedAt) + Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", i))), + "disc %d should use its own folder.jpg", i) + } + }) + }) + When("discsubtitle is set but no image filename matches the subtitle", func() { // Artist/ // └── Album/ diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 8d7e14fd0..73ba9b5ee 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -18,6 +18,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/natural" ) @@ -53,10 +54,9 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar lib: lib, } a.cacheKey.artID = artID - if a.updatedAt != nil && a.updatedAt.After(al.UpdatedAt) { - a.cacheKey.lastUpdate = *a.updatedAt - } else { - a.cacheKey.lastUpdate = al.UpdatedAt + a.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) + if imagesUpdateAt != nil { + a.cacheKey.lastUpdate = utils.TimeNewest(a.cacheKey.lastUpdate, *imagesUpdateAt) } return a, nil } @@ -118,19 +118,22 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo folderIDSet[id] = true } - // For multi-disc albums (2+ folders), check if all folders share a common parent - // that is not already included. This finds cover art in the album root folder - // (e.g., "Artist/Album/cover.jpg" when tracks are in "Artist/Album/CD1/" and "Artist/Album/CD2/"). - // We skip single-folder albums to avoid pulling images from the artist folder. + // Check if all folders share a common parent that is not already included. + // This finds cover art in the album root folder (e.g., "Artist/Album/cover.jpg" + // when tracks are in disc subfolders like "Artist/Album/CD1/" and "Artist/Album/CD2/"). + // For single-folder albums, the parent is only included when the folder has no + // images of its own (indicating a disc subfolder needing parent artwork). if commonParentID := commonParentFolder(folders, folderIDSet); commonParentID != "" { - parentFolder, err := ds.Folder(ctx).Get(commonParentID) - if errors.Is(err, model.ErrNotFound) { - log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID) - } else if err != nil { - return nil, nil, nil, err - } - if parentFolder != nil { - folders = append(folders, *parentFolder) + if len(folders) >= 2 || !anyFolderHasImages(folders) { + parentFolder, err := ds.Folder(ctx).Get(commonParentID) + if errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID) + } else if err != nil { + return nil, nil, nil, err + } + if parentFolder != nil && parentFolder.ParentID != "" { + folders = append(folders, *parentFolder) + } } } @@ -156,10 +159,19 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo return paths, imgFiles, &updatedAt, nil } +func anyFolderHasImages(folders []model.Folder) bool { + for _, f := range folders { + if len(f.ImageFiles) > 0 { + return true + } + } + return false +} + // commonParentFolder returns the shared parent folder ID when all folders have the // same parent and that parent is not already in folderIDSet. Returns "" otherwise. func commonParentFolder(folders []model.Folder, folderIDSet map[string]bool) string { - if len(folders) < 2 { + if len(folders) == 0 { return "" } parentID := folders[0].ParentID @@ -174,11 +186,8 @@ func commonParentFolder(folders []model.Folder, folderIDSet map[string]bool) str return parentID } -// compareImageFiles compares two image file paths for sorting. -// It extracts the base filename (without extension) and compares case-insensitively. -// This ensures that "cover.jpg" sorts before "cover.1.jpg" since "cover" < "cover.1". -// Note: This function is called O(n log n) times during sorting, but in practice albums -// typically have only 1-20 image files, making the repeated string operations negligible. +// compareImageFiles sorts image paths by: base filename (natural order), +// then path depth (shallower first), then full path (stable tiebreaker). func compareImageFiles(a, b string) int { // Case-insensitive comparison a = strings.ToLower(a) @@ -188,9 +197,10 @@ func compareImageFiles(a, b string) int { baseA := strings.TrimSuffix(path.Base(a), path.Ext(a)) baseB := strings.TrimSuffix(path.Base(b), path.Ext(b)) - // Compare base names first, then full paths if equal + // Compare base names first, then prefer shallower paths, then full path as tiebreaker return cmp.Or( natural.Compare(baseA, baseB), + cmp.Compare(strings.Count(a, "/"), strings.Count(b, "/")), natural.Compare(a, b), ) } diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index 03412b6d9..1cf039bee 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -141,6 +141,7 @@ var _ = Describe("Album Artwork Reader", func() { ID: "parentFolder", Path: "Artist", Name: "Album", + ParentID: "artistFolder", ImagesUpdatedAt: expectedAt, ImageFiles: []string{"cover.jpg", "back.jpg"}, } @@ -213,9 +214,83 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(0)) }) - It("does not query parent for single-folder albums", func() { - // A single-folder album's parent is typically the artist folder, - // which should not be searched for cover art + It("does not include library root parent for multi-folder albums", func() { + // Two album parts directly under the library root — parent is the root itself + repo.result = []model.Folder{ + { + ID: "folder1", + Path: ".", + Name: "AlbumPart1", + ParentID: "rootFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + { + ID: "folder2", + Path: ".", + Name: "AlbumPart2", + ParentID: "rootFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "rootFolder", + Path: "", + Name: ".", + ParentID: "", + ImageFiles: []string{"unrelated.jpg"}, + } + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(HaveLen(1)) + Expect(imgFiles[0]).To(Equal("AlbumPart1/cover.jpg")) + Expect(repo.getCallCount).To(Equal(1)) + }) + + It("includes top-level album folder for multi-disc albums", func() { + // Album folder directly under library root, with disc subfolders + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Album", + Name: "Disc1", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"folder.jpg"}, + }, + { + ID: "folder2", + Path: "Album", + Name: "Disc2", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"folder.jpg"}, + }, + } + repo.parentResult = &model.Folder{ + ID: "albumFolder", + Path: ".", + Name: "Album", + ParentID: "rootFolder", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg"}, + } + + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(*imagesUpdatedAt).To(Equal(expectedAt)) + Expect(imgFiles).To(HaveLen(3)) + Expect(imgFiles[0]).To(Equal("Album/cover.jpg")) + Expect(imgFiles[1]).To(Equal("Album/Disc1/folder.jpg")) + Expect(imgFiles[2]).To(Equal("Album/Disc2/folder.jpg")) + Expect(repo.getCallCount).To(Equal(1)) + }) + + It("does not query parent for single-folder albums that already have images", func() { repo.result = []model.Folder{ { ID: "folder1", @@ -232,10 +307,38 @@ var _ = Describe("Album Artwork Reader", func() { Expect(err).ToNot(HaveOccurred()) Expect(imgFiles).To(HaveLen(1)) Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg")) - // Get should not have been called (single folder, no parent lookup) Expect(repo.getCallCount).To(Equal(0)) }) + It("includes parent images for single-disc-subfolder albums", func() { + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist/Album", + Name: "disc1", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "albumFolder", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg"}, + } + + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(*imagesUpdatedAt).To(Equal(expectedAt)) + Expect(imgFiles).To(HaveLen(1)) + Expect(imgFiles[0]).To(Equal("Artist/Album/cover.jpg")) + Expect(repo.getCallCount).To(Equal(1)) + }) + It("propagates non-ErrNotFound errors from parent folder lookup", func() { repo.result = []model.Folder{ { diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index de0a765f0..0f648c987 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" ) type discArtworkReader struct { @@ -105,10 +106,9 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID updatedAt: imagesUpdatedAt, } r.cacheKey.artID = artID - if r.updatedAt != nil && r.updatedAt.After(al.UpdatedAt) { - r.cacheKey.lastUpdate = *r.updatedAt - } else { - r.cacheKey.lastUpdate = al.UpdatedAt + r.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) + if imagesUpdatedAt != nil { + r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *imagesUpdatedAt) } return r, nil } diff --git a/core/auth/auth.go b/core/auth/auth.go index a75111b35..7b3511bdf 100644 --- a/core/auth/auth.go +++ b/core/auth/auth.go @@ -100,7 +100,7 @@ func WithAdminUser(ctx context.Context, ds model.DataStore) context.Context { } else { log.Error(ctx, "No admin user found!", err) } - u = &model.User{} + u = &model.User{IsAdmin: true, UserName: "admin"} } ctx = request.WithUsername(ctx, u.UserName) diff --git a/core/scrobbler/buffered_scrobbler.go b/core/scrobbler/buffered_scrobbler.go index be36e1f24..67593e9eb 100644 --- a/core/scrobbler/buffered_scrobbler.go +++ b/core/scrobbler/buffered_scrobbler.go @@ -80,6 +80,14 @@ func (b *bufferedScrobbler) Scrobble(ctx context.Context, userId string, s Scrob return nil } +func (b *bufferedScrobbler) PlaybackReport(ctx context.Context, info PlaybackSession) error { + s, ok := b.loader() + if !ok { + return errors.New("scrobbler not available") + } + return s.PlaybackReport(ctx, info) +} + func (b *bufferedScrobbler) sendWakeSignal() { // Don't block if the previous signal was not read yet select { diff --git a/core/scrobbler/interfaces.go b/core/scrobbler/interfaces.go index f8567e91b..8a18bb37e 100644 --- a/core/scrobbler/interfaces.go +++ b/core/scrobbler/interfaces.go @@ -23,6 +23,7 @@ type Scrobbler interface { IsAuthorized(ctx context.Context, userId string) bool NowPlaying(ctx context.Context, userId string, track *model.MediaFile, position int) error Scrobble(ctx context.Context, userId string, s Scrobble) error + PlaybackReport(ctx context.Context, info PlaybackSession) error } type Constructor func(ds model.DataStore) Scrobbler diff --git a/core/scrobbler/nowplaying_worker.go b/core/scrobbler/nowplaying_worker.go new file mode 100644 index 000000000..1bacec689 --- /dev/null +++ b/core/scrobbler/nowplaying_worker.go @@ -0,0 +1,78 @@ +package scrobbler + +import ( + "context" + "time" + + "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" +) + +func (p *playTracker) enqueueNowPlaying(ctx context.Context, playerId string, userId string, track *model.MediaFile, position int) { + p.npMu.Lock() + defer p.npMu.Unlock() + ctx = context.WithoutCancel(ctx) // Prevent cancellation from affecting background processing + p.npQueue[playerId] = nowPlayingEntry{ + ctx: ctx, + userId: userId, + track: track, + position: position, + } + p.sendNowPlayingSignal() +} + +func (p *playTracker) sendNowPlayingSignal() { + // Don't block if the previous signal was not read yet + select { + case p.npSignal <- struct{}{}: + default: + } +} + +func (p *playTracker) nowPlayingWorker() { + defer close(p.workerDone) + for { + select { + case <-p.shutdown: + return + case <-time.After(time.Second): + case <-p.npSignal: + } + + p.npMu.Lock() + if len(p.npQueue) == 0 { + p.npMu.Unlock() + continue + } + + // Keep a copy of the entries to process and clear the queue + entries := p.npQueue + p.npQueue = make(map[string]nowPlayingEntry) + p.npMu.Unlock() + + // Process entries without holding lock + for _, entry := range entries { + p.dispatchNowPlaying(entry.ctx, entry.userId, entry.track, entry.position) + } + } +} + +func (p *playTracker) dispatchNowPlaying(ctx context.Context, userId string, t *model.MediaFile, position int) { + if t.Artist == consts.UnknownArtist { + log.Debug(ctx, "Ignoring external NowPlaying update for track with unknown artist", "track", t.Title, "artist", t.Artist) + return + } + allScrobblers := p.getActiveScrobblers() + for name, s := range allScrobblers { + if !s.IsAuthorized(ctx, userId) { + continue + } + log.Debug(ctx, "Sending NowPlaying update", "scrobbler", name, "track", t.Title, "artist", t.Artist, "position", position) + err := s.NowPlaying(ctx, userId, t, position) + if err != nil { + log.Error(ctx, "Error sending PlaybackSession", "scrobbler", name, "track", t.Title, "artist", t.Artist, err) + continue + } + } +} diff --git a/core/scrobbler/play_tracker.go b/core/scrobbler/play_tracker.go index d1338ca39..bdb261ef2 100644 --- a/core/scrobbler/play_tracker.go +++ b/core/scrobbler/play_tracker.go @@ -3,7 +3,7 @@ package scrobbler import ( "context" "maps" - "sort" + "slices" "sync" "time" @@ -17,13 +17,32 @@ import ( "github.com/navidrome/navidrome/utils/singleton" ) -type NowPlayingInfo struct { - MediaFile model.MediaFile - Start time.Time - Position int - Username string - PlayerId string - PlayerName string +const ( + StateStarting = "starting" + StatePlaying = "playing" + StatePaused = "paused" + StateStopped = "stopped" + StateExpired = "expired" +) + +var ValidStates = map[string]bool{ + StateStarting: true, + StatePlaying: true, + StatePaused: true, + StateStopped: true, +} + +type PlaybackSession struct { + MediaFile model.MediaFile + Start time.Time + UserId string + Username string + PlayerId string + PlayerName string + State string + PositionMs int64 + PlaybackRate float64 + LastReport time.Time } type Submission struct { @@ -31,6 +50,16 @@ type Submission struct { Timestamp time.Time } +type ReportPlaybackParams struct { + MediaId string + PositionMs int64 + State string + PlaybackRate float64 + IgnoreScrobble bool + ClientId string + ClientName string +} + type nowPlayingEntry struct { ctx context.Context userId string @@ -38,10 +67,15 @@ type nowPlayingEntry struct { position int } +type playbackReportEntry struct { + ctx context.Context + info PlaybackSession +} + type PlayTracker interface { - NowPlaying(ctx context.Context, playerId string, playerName string, trackId string, position int) error - GetNowPlaying(ctx context.Context) ([]NowPlayingInfo, error) + GetNowPlaying(ctx context.Context) ([]PlaybackSession, error) Submit(ctx context.Context, submissions []Submission) error + ReportPlayback(ctx context.Context, params ReportPlaybackParams) error } // PluginLoader is a minimal interface for plugin manager usage in PlayTracker @@ -54,7 +88,7 @@ type PluginLoader interface { type playTracker struct { ds model.DataStore broker events.Broker - playMap cache.SimpleCache[string, NowPlayingInfo] + playMap cache.SimpleCache[string, PlaybackSession] builtinScrobblers map[string]Scrobbler pluginScrobblers map[string]Scrobbler pluginLoader PluginLoader @@ -64,6 +98,10 @@ type playTracker struct { npSignal chan struct{} shutdown chan struct{} workerDone chan struct{} + prQueue []playbackReportEntry + prMu sync.Mutex + prSignal chan struct{} + prWorkerDone chan struct{} } func GetPlayTracker(ds model.DataStore, broker events.Broker, pluginManager PluginLoader) PlayTracker { @@ -72,10 +110,14 @@ func GetPlayTracker(ds model.DataStore, broker events.Broker, pluginManager Plug }) } -// This constructor only exists for testing. For normal usage, the PlayTracker has to be a singleton, returned by -// the GetPlayTracker function above +// NewPlayTracker creates a new PlayTracker instance. For normal usage, the PlayTracker has to be a singleton, +// returned by the GetPlayTracker function above. This constructor is exported for testing. +func NewPlayTracker(ds model.DataStore, broker events.Broker, pluginManager PluginLoader) PlayTracker { + return newPlayTracker(ds, broker, pluginManager) +} + func newPlayTracker(ds model.DataStore, broker events.Broker, pluginManager PluginLoader) *playTracker { - m := cache.NewSimpleCache[string, NowPlayingInfo]() + m := cache.NewSimpleCache[string, PlaybackSession]() p := &playTracker{ ds: ds, playMap: m, @@ -87,12 +129,24 @@ func newPlayTracker(ds model.DataStore, broker events.Broker, pluginManager Plug npSignal: make(chan struct{}, 1), shutdown: make(chan struct{}), workerDone: make(chan struct{}), + prSignal: make(chan struct{}, 1), + prWorkerDone: make(chan struct{}), } - if conf.Server.EnableNowPlaying { - m.OnExpiration(func(_ string, _ NowPlayingInfo) { + enableNowPlaying := conf.Server.EnableNowPlaying + m.OnExpiration(func(_ string, info PlaybackSession) { + log.Debug("PlaybackSession expired", "clientId", info.PlayerId, "mediaId", info.MediaFile.ID, "state", + info.State, "username", info.Username, "userId", info.UserId) + if enableNowPlaying { broker.SendBroadcastMessage(context.Background(), &events.NowPlayingCount{Count: m.Len()}) - }) - } + } + ctx := request.WithUser(context.Background(), model.User{ID: info.UserId, UserName: info.Username}) + if info.State != StateStopped { + log.Trace("Enqueueing PlaybackReport for expired session", "session", info) + info.State = StateExpired + info.LastReport = time.Now() + p.enqueuePlaybackReport(ctx, info) + } + }) var enabled []string for name, constructor := range constructors { @@ -107,13 +161,15 @@ func newPlayTracker(ds model.DataStore, broker events.Broker, pluginManager Plug } log.Debug("List of builtin scrobblers enabled", "names", enabled) go p.nowPlayingWorker() + go p.playbackReportWorker() return p } -// stopNowPlayingWorker stops the background worker. This is primarily for testing. -func (p *playTracker) stopNowPlayingWorker() { +// stopBackgroundWorkers stops the background workers. This is primarily for testing. +func (p *playTracker) stopBackgroundWorkers() { close(p.shutdown) - <-p.workerDone // Wait for worker to finish + <-p.workerDone // Wait for nowPlaying worker to finish + <-p.prWorkerDone // Wait for playbackReport worker to finish } // pluginNamesMatchScrobblers returns true if the set of pluginNames matches the keys in pluginScrobblers. @@ -193,112 +249,151 @@ func (p *playTracker) getActiveScrobblers() map[string]Scrobbler { return combined } -func (p *playTracker) NowPlaying(ctx context.Context, playerId string, playerName string, trackId string, position int) error { - mf, err := p.ds.MediaFile(ctx).GetWithParticipants(trackId) - if err != nil { - log.Error(ctx, "Error retrieving mediaFile", "id", trackId, err) - return err +func remainingTTL(durationSec float32, positionMs int64, rate float64) time.Duration { + if rate <= 0 { + rate = 1.0 } + remainingMs := float64(int64(durationSec*1000)-positionMs) / rate + remainingSec := max(int(remainingMs/1000), 0) + return time.Duration(remainingSec+5) * time.Second +} +func (p *playTracker) ReportPlayback(ctx context.Context, params ReportPlaybackParams) error { + player, _ := request.PlayerFrom(ctx) user, _ := request.UserFrom(ctx) - info := NowPlayingInfo{ - MediaFile: *mf, - Start: time.Now(), - Position: position, - Username: user.UserName, - PlayerId: playerId, - PlayerName: playerName, + clientId := params.ClientId + client := params.ClientName + + now := time.Now() + + switch params.State { + case StateStarting: + mf, err := p.ds.MediaFile(ctx).GetWithParticipants(params.MediaId) + if err != nil { + return err + } + info := PlaybackSession{ + MediaFile: *mf, + Start: now, + UserId: user.ID, + Username: user.UserName, + PlayerId: clientId, + PlayerName: client, + State: params.State, + PositionMs: params.PositionMs, + PlaybackRate: params.PlaybackRate, + LastReport: now, + } + err = p.playMap.AddWithTTL(clientId, info, remainingTTL(mf.Duration, params.PositionMs, params.PlaybackRate)) + if err != nil { + log.Warn(ctx, "Error adding PlaybackSession to cache", "clientId", clientId, "mediaId", params.MediaId, "state", params.State, err) + } + p.enqueuePlaybackReport(ctx, info) + + case StatePlaying, StatePaused: + info, getErr := p.playMap.Get(clientId) + if getErr != nil || info.MediaFile.ID != params.MediaId { + mf, err := p.ds.MediaFile(ctx).GetWithParticipants(params.MediaId) + if err != nil { + return err + } + info = PlaybackSession{ + MediaFile: *mf, + Start: now.Add(-time.Duration(params.PositionMs) * time.Millisecond), + UserId: user.ID, + Username: user.UserName, + PlayerId: clientId, + PlayerName: client, + } + } + info.State = params.State + info.PositionMs = params.PositionMs + info.PlaybackRate = params.PlaybackRate + info.LastReport = now + ttl := 30 * time.Minute + if params.State == StatePlaying { + ttl = remainingTTL(info.MediaFile.Duration, params.PositionMs, params.PlaybackRate) + } + log.Trace(ctx, "Updating PlaybackSession in cache", "clientId", clientId, "mediaId", params.MediaId, "state", params.State, "positionMs", params.PositionMs, "playbackRate", params.PlaybackRate, "ttl", ttl) + err := p.playMap.AddWithTTL(clientId, info, ttl) + if err != nil { + log.Warn(ctx, "Error updating PlaybackSession in cache", "clientId", clientId, "mediaId", params.MediaId, "state", params.State, err) + } + p.enqueuePlaybackReport(ctx, info) + + case StateStopped: + var loadedMF *model.MediaFile + if !params.IgnoreScrobble && player.ScrobbleEnabled { + mf, err := p.ds.MediaFile(ctx).GetWithParticipants(params.MediaId) + if err != nil { + return err + } + loadedMF = mf + trackDurationMs := int64(mf.Duration * 1000) + threshold := min(trackDurationMs*50/100, 240_000) + if params.PositionMs >= threshold { + err = p.incPlay(ctx, mf, now) + if err != nil { + log.Warn(ctx, "Error updating play counts", "id", mf.ID, "track", mf.Title, "user", user.UserName, err) + } + p.dispatchScrobble(ctx, mf, now) + } + } + stoppedInfo := PlaybackSession{ + UserId: user.ID, + Username: user.UserName, + PlayerId: clientId, + PlayerName: client, + State: params.State, + PositionMs: params.PositionMs, + PlaybackRate: params.PlaybackRate, + LastReport: now, + } + if info, getErr := p.playMap.Get(clientId); getErr == nil { + stoppedInfo.MediaFile = info.MediaFile + stoppedInfo.Start = info.Start + } else { + mf := loadedMF + if mf == nil { + var mfErr error + mf, mfErr = p.ds.MediaFile(ctx).GetWithParticipants(params.MediaId) + if mfErr != nil { + return mfErr + } + } + stoppedInfo.MediaFile = *mf + } + p.enqueuePlaybackReport(ctx, stoppedInfo) + p.playMap.Remove(clientId) } - // Calculate TTL based on remaining track duration. If position exceeds track duration, - // remaining is set to 0 to avoid negative TTL. - remaining := max(int(mf.Duration)-position, 0) - // Add 5 seconds buffer to ensure the NowPlaying info is available slightly longer than the track duration. - ttl := time.Duration(remaining+5) * time.Second - _ = p.playMap.AddWithTTL(playerId, info, ttl) if conf.Server.EnableNowPlaying { p.broker.SendBroadcastMessage(ctx, &events.NowPlayingCount{Count: p.playMap.Len()}) } - player, _ := request.PlayerFrom(ctx) - if player.ScrobbleEnabled { - p.enqueueNowPlaying(ctx, playerId, user.ID, mf, position) + + if !params.IgnoreScrobble && player.ScrobbleEnabled && + (params.State == StateStarting || params.State == StatePlaying) { + if info, err := p.playMap.Get(clientId); err == nil { + p.enqueueNowPlaying(ctx, clientId, user.ID, &info.MediaFile, int(params.PositionMs/1000)) + } } + return nil } -func (p *playTracker) enqueueNowPlaying(ctx context.Context, playerId string, userId string, track *model.MediaFile, position int) { - p.npMu.Lock() - defer p.npMu.Unlock() - ctx = context.WithoutCancel(ctx) // Prevent cancellation from affecting background processing - p.npQueue[playerId] = nowPlayingEntry{ - ctx: ctx, - userId: userId, - track: track, - position: position, - } - p.sendNowPlayingSignal() -} - -func (p *playTracker) sendNowPlayingSignal() { - // Don't block if the previous signal was not read yet - select { - case p.npSignal <- struct{}{}: - default: - } -} - -func (p *playTracker) nowPlayingWorker() { - defer close(p.workerDone) - for { - select { - case <-p.shutdown: - return - case <-time.After(time.Second): - case <-p.npSignal: - } - - p.npMu.Lock() - if len(p.npQueue) == 0 { - p.npMu.Unlock() - continue - } - - // Keep a copy of the entries to process and clear the queue - entries := p.npQueue - p.npQueue = make(map[string]nowPlayingEntry) - p.npMu.Unlock() - - // Process entries without holding lock - for _, entry := range entries { - p.dispatchNowPlaying(entry.ctx, entry.userId, entry.track, entry.position) - } - } -} - -func (p *playTracker) dispatchNowPlaying(ctx context.Context, userId string, t *model.MediaFile, position int) { - if t.Artist == consts.UnknownArtist { - log.Debug(ctx, "Ignoring external NowPlaying update for track with unknown artist", "track", t.Title, "artist", t.Artist) - return - } - allScrobblers := p.getActiveScrobblers() - for name, s := range allScrobblers { - if !s.IsAuthorized(ctx, userId) { - continue - } - log.Debug(ctx, "Sending NowPlaying update", "scrobbler", name, "track", t.Title, "artist", t.Artist, "position", position) - err := s.NowPlaying(ctx, userId, t, position) - if err != nil { - log.Error(ctx, "Error sending NowPlayingInfo", "scrobbler", name, "track", t.Title, "artist", t.Artist, err) - continue - } - } -} - -func (p *playTracker) GetNowPlaying(_ context.Context) ([]NowPlayingInfo, error) { +func (p *playTracker) GetNowPlaying(_ context.Context) ([]PlaybackSession, error) { res := p.playMap.Values() - sort.Slice(res, func(i, j int) bool { - return res[i].Start.After(res[j].Start) + slices.SortFunc(res, func(a, b PlaybackSession) int { + return b.Start.Compare(a.Start) }) + for i := range res { + if res[i].State == StatePlaying { + elapsed := time.Since(res[i].LastReport).Milliseconds() + estimated := res[i].PositionMs + int64(float64(elapsed)*res[i].PlaybackRate) + trackDurationMs := int64(res[i].MediaFile.Duration * 1000) + res[i].PositionMs = min(estimated, trackDurationMs) + } + } return res, nil } diff --git a/core/scrobbler/play_tracker_test.go b/core/scrobbler/play_tracker_test.go index f7edecdfd..684b887fe 100644 --- a/core/scrobbler/play_tracker_test.go +++ b/core/scrobbler/play_tracker_test.go @@ -20,9 +20,6 @@ import ( . "github.com/onsi/gomega" ) -// mockPluginLoader is a test implementation of PluginLoader for plugin scrobbler tests -// Moved to top-level scope to avoid linter issues - type mockPluginLoader struct { mu sync.RWMutex names []string @@ -51,7 +48,7 @@ func (m *mockPluginLoader) LoadScrobbler(name string) (Scrobbler, bool) { var _ = Describe("PlayTracker", func() { var ctx context.Context var ds model.DataStore - var tracker PlayTracker + var tracker *playTracker var eventBroker *fakeEventBroker var track model.MediaFile var album model.Album @@ -74,7 +71,7 @@ var _ = Describe("PlayTracker", func() { }) eventBroker = &fakeEventBroker{} tracker = newPlayTracker(ds, eventBroker, nil) - tracker.(*playTracker).builtinScrobblers["fake"] = fake // Bypass buffering for tests + tracker.builtinScrobblers["fake"] = fake // Bypass buffering for tests track = model.MediaFile{ ID: "123", @@ -99,88 +96,12 @@ var _ = Describe("PlayTracker", func() { AfterEach(func() { // Stop the worker goroutine to prevent data races between tests - tracker.(*playTracker).stopNowPlayingWorker() + tracker.stopBackgroundWorkers() }) It("does not register disabled scrobblers", func() { - Expect(tracker.(*playTracker).builtinScrobblers).To(HaveKey("fake")) - Expect(tracker.(*playTracker).builtinScrobblers).ToNot(HaveKey("disabled")) - }) - - Describe("NowPlaying", func() { - It("sends track to agent", func() { - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - Expect(err).ToNot(HaveOccurred()) - Eventually(func() bool { return fake.GetNowPlayingCalled() }).Should(BeTrue()) - Expect(fake.GetUserID()).To(Equal("u-1")) - Expect(fake.GetTrack().ID).To(Equal("123")) - Expect(fake.GetTrack().Participants).To(Equal(track.Participants)) - }) - It("does not send track to agent if user has not authorized", func() { - fake.Authorized = false - - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - - Expect(err).ToNot(HaveOccurred()) - Expect(fake.GetNowPlayingCalled()).To(BeFalse()) - }) - It("does not send track to agent if player is not enabled to send scrobbles", func() { - ctx = request.WithPlayer(ctx, model.Player{ScrobbleEnabled: false}) - - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - - Expect(err).ToNot(HaveOccurred()) - Expect(fake.GetNowPlayingCalled()).To(BeFalse()) - }) - It("does not send track to agent if artist is unknown", func() { - track.Artist = consts.UnknownArtist - - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - - Expect(err).ToNot(HaveOccurred()) - Expect(fake.GetNowPlayingCalled()).To(BeFalse()) - }) - - It("stores position when greater than zero", func() { - pos := 42 - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", pos) - Expect(err).ToNot(HaveOccurred()) - - Eventually(func() int { return fake.GetPosition() }).Should(Equal(pos)) - - playing, err := tracker.GetNowPlaying(ctx) - Expect(err).ToNot(HaveOccurred()) - Expect(playing).To(HaveLen(1)) - Expect(playing[0].Position).To(Equal(pos)) - }) - - It("sends event with count", func() { - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - Expect(err).ToNot(HaveOccurred()) - eventList := eventBroker.getEvents() - Expect(eventList).ToNot(BeEmpty()) - evt, ok := eventList[0].(*events.NowPlayingCount) - Expect(ok).To(BeTrue()) - Expect(evt.Count).To(Equal(1)) - }) - - It("does not send event when disabled", func() { - conf.Server.EnableNowPlaying = false - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - Expect(err).ToNot(HaveOccurred()) - Expect(eventBroker.getEvents()).To(BeEmpty()) - }) - - It("passes user to scrobbler via context (fix for issue #4787)", func() { - ctx = request.WithUser(ctx, model.User{ID: "u-1", UserName: "testuser"}) - ctx = request.WithPlayer(ctx, model.Player{ScrobbleEnabled: true}) - - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - Expect(err).ToNot(HaveOccurred()) - Eventually(func() bool { return fake.GetNowPlayingCalled() }).Should(BeTrue()) - // Verify the username was passed through async dispatch via context - Eventually(func() string { return fake.GetUsername() }).Should(Equal("testuser")) - }) + Expect(tracker.builtinScrobblers).To(HaveKey("fake")) + Expect(tracker.builtinScrobblers).ToNot(HaveKey("disabled")) }) Describe("GetNowPlaying", func() { @@ -188,10 +109,16 @@ var _ = Describe("PlayTracker", func() { track2 := track track2.ID = "456" _ = ds.MediaFile(ctx).Put(&track2) - ctx = request.WithUser(GinkgoT().Context(), model.User{UserName: "user-1"}) - _ = tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - ctx = request.WithUser(GinkgoT().Context(), model.User{UserName: "user-2"}) - _ = tracker.NowPlaying(ctx, "player-2", "player-two", "456", 0) + ctx1 := request.WithUser(GinkgoT().Context(), model.User{UserName: "user-1"}) + ctx1 = request.WithPlayer(ctx1, model.Player{ScrobbleEnabled: true}) + _ = tracker.ReportPlayback(ctx1, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-1", ClientName: "player-one", + }) + ctx2 := request.WithUser(GinkgoT().Context(), model.User{UserName: "user-2"}) + ctx2 = request.WithPlayer(ctx2, model.Player{ScrobbleEnabled: true}) + _ = tracker.ReportPlayback(ctx2, ReportPlaybackParams{ + MediaId: "456", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-2", ClientName: "player-two", + }) playing, err := tracker.GetNowPlaying(ctx) @@ -211,8 +138,8 @@ var _ = Describe("PlayTracker", func() { Describe("Expiration events", func() { It("sends event when entry expires", func() { - info := NowPlayingInfo{MediaFile: track, Start: time.Now(), Username: "user"} - _ = tracker.(*playTracker).playMap.AddWithTTL("player-1", info, 10*time.Millisecond) + info := PlaybackSession{MediaFile: track, Start: time.Now(), Username: "user"} + _ = tracker.playMap.AddWithTTL("player-1", info, 10*time.Millisecond) Eventually(func() int { return len(eventBroker.getEvents()) }).Should(BeNumerically(">", 0)) eventList := eventBroker.getEvents() evt, ok := eventList[len(eventList)-1].(*events.NowPlayingCount) @@ -223,10 +150,48 @@ var _ = Describe("PlayTracker", func() { It("does not send event when disabled", func() { conf.Server.EnableNowPlaying = false tracker = newPlayTracker(ds, eventBroker, nil) - info := NowPlayingInfo{MediaFile: track, Start: time.Now(), Username: "user"} - _ = tracker.(*playTracker).playMap.AddWithTTL("player-2", info, 10*time.Millisecond) + info := PlaybackSession{MediaFile: track, Start: time.Now(), Username: "user"} + _ = tracker.playMap.AddWithTTL("player-2", info, 10*time.Millisecond) Consistently(func() int { return len(eventBroker.getEvents()) }).Should(Equal(0)) }) + + It("sends expired playback report when session expires", func() { + info := PlaybackSession{ + MediaFile: track, + Start: time.Now(), + UserId: "u-1", + Username: "user", + PlayerId: "player-3", + PlayerName: "test-player", + State: StatePlaying, + PositionMs: 5000, + } + _ = tracker.playMap.AddWithTTL("player-3", info, 10*time.Millisecond) + Eventually(func() *PlaybackSession { + return fake.LastPlaybackReport.Load() + }).ShouldNot(BeNil()) + report := fake.LastPlaybackReport.Load() + Expect(report.State).To(Equal(StateExpired)) + Expect(report.MediaFile.ID).To(Equal("123")) + Expect(report.PlayerId).To(Equal("player-3")) + }) + + It("does not send expired report when session was already stopped", func() { + info := PlaybackSession{ + MediaFile: track, + Start: time.Now(), + UserId: "u-1", + Username: "user", + PlayerId: "player-4", + PlayerName: "test-player", + State: StateStopped, + PositionMs: 180000, + } + _ = tracker.playMap.AddWithTTL("player-4", info, 10*time.Millisecond) + Consistently(func() *PlaybackSession { + return fake.LastPlaybackReport.Load() + }).Should(BeNil()) + }) }) Describe("Submit", func() { @@ -336,6 +301,532 @@ var _ = Describe("PlayTracker", func() { }) }) + Describe("ReportPlayback", func() { + const defaultClientId = "client-1" + + BeforeEach(func() { + ctx = request.WithPlayer(ctx, model.Player{ID: "p1", ScrobbleEnabled: true}) + }) + + It("creates entry on starting and removes on stopped", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].State).To(Equal("starting")) + Expect(playing[0].MediaFile.ID).To(Equal("123")) + + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + IgnoreScrobble: true, + }) + Expect(err).ToNot(HaveOccurred()) + + playing, err = tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(BeEmpty()) + }) + + It("full lifecycle: starting -> playing -> paused -> playing -> stopped", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].State).To(Equal("playing")) + Expect(playing[0].PositionMs).To(BeNumerically(">=", int64(10000))) + + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 30000, State: "paused", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + playing, err = tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing[0].State).To(Equal("paused")) + Expect(playing[0].PositionMs).To(Equal(int64(30000))) + + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 30000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 100000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + playing, err = tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(BeEmpty()) + }) + + It("starting replaces existing entry for same player", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 50000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].State).To(Equal("starting")) + Expect(playing[0].PositionMs).To(Equal(int64(0))) + }) + + It("multiple players have independent sessions", func() { + ctx1 := request.WithUser(ctx, model.User{ID: "u-1", UserName: "user1"}) + ctx1 = request.WithPlayer(ctx1, model.Player{ID: "p1", ScrobbleEnabled: true}) + + ctx2 := request.WithUser(ctx, model.User{ID: "u-1", UserName: "user1"}) + ctx2 = request.WithPlayer(ctx2, model.Player{ID: "p2", ScrobbleEnabled: true}) + + track2 := track + track2.ID = "456" + _ = ds.MediaFile(ctx).Put(&track2) + + err := tracker.ReportPlayback(ctx1, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "playing", PlaybackRate: 1.0, ClientId: "client-1", + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx2, ReportPlaybackParams{ + MediaId: "456", PositionMs: 0, State: "playing", PlaybackRate: 1.0, ClientId: "client-2", + }) + Expect(err).ToNot(HaveOccurred()) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(2)) + }) + + Describe("SSE broadcast on state change", func() { + BeforeEach(func() { + eventBroker = &fakeEventBroker{} + tracker = newPlayTracker(ds, eventBroker, nil) + tracker.builtinScrobblers["fake"] = fake + }) + + It("broadcasts NowPlayingCount on every state change", func() { + // starting -> count should be 1 + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + evts := eventBroker.getEvents() + Expect(evts).To(HaveLen(1)) + Expect(evts[0].(*events.NowPlayingCount).Count).To(Equal(1)) + + // playing -> count should be 1 + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + evts = eventBroker.getEvents() + Expect(evts).To(HaveLen(2)) + Expect(evts[1].(*events.NowPlayingCount).Count).To(Equal(1)) + + // paused -> count should be 1 + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 30000, State: "paused", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + evts = eventBroker.getEvents() + Expect(evts).To(HaveLen(3)) + Expect(evts[2].(*events.NowPlayingCount).Count).To(Equal(1)) + + // stopped -> count should be 0 + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 30000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + IgnoreScrobble: true, + }) + Expect(err).ToNot(HaveOccurred()) + evts = eventBroker.getEvents() + Expect(evts).To(HaveLen(4)) + Expect(evts[3].(*events.NowPlayingCount).Count).To(Equal(0)) + }) + + It("does NOT broadcast when EnableNowPlaying is false", func() { + conf.Server.EnableNowPlaying = false + tracker = newPlayTracker(ds, eventBroker, nil) + tracker.builtinScrobblers["fake"] = fake + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(eventBroker.getEvents()).To(BeEmpty()) + }) + }) + + Describe("auto-scrobble", func() { + It("scrobbles on stopped when positionMs >= 50% of track", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 90000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(1))) + Expect(album.PlayCount).To(Equal(int64(1))) + Expect(artist1.PlayCount).To(Equal(int64(1))) + }) + + It("scrobbles on stopped when positionMs >= 4 min for long tracks", func() { + longTrack := model.MediaFile{ + ID: "long", Title: "Long Song", Album: "Album", AlbumID: "al-1", + Duration: 600, + Participants: map[model.Role]model.ParticipantList{ + model.RoleArtist: []model.Participant{_p("ar-1", "Artist 1")}, + }, + } + _ = ds.MediaFile(ctx).Put(&longTrack) + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "long", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "long", PositionMs: 240000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(longTrack.PlayCount).To(Equal(int64(1))) + }) + + It("does NOT scrobble when positionMs below threshold", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(0))) + }) + + It("does NOT scrobble when ignoreScrobble=true even if threshold met", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 90000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + IgnoreScrobble: true, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(0))) + }) + + It("does NOT scrobble when player ScrobbleEnabled=false even if threshold met", func() { + ctx = request.WithPlayer(ctx, model.Player{ID: "p1", ScrobbleEnabled: false}) + + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 90000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(0))) + }) + + It("scrobbles twice for two separate sessions of same song", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 90000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 90000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(2))) + }) + + It("dispatches to external scrobblers on auto-scrobble", func() { + fake.ScrobbleCalled.Store(false) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 90000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(fake.ScrobbleCalled.Load()).To(BeTrue()) + }) + }) + + Describe("position estimation", func() { + It("estimates position for playing state based on elapsed time", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + time.Sleep(50 * time.Millisecond) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].PositionMs).To(BeNumerically(">", int64(10000))) + }) + + It("does NOT estimate for paused", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "paused", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + time.Sleep(50 * time.Millisecond) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].PositionMs).To(Equal(int64(10000))) + }) + + It("does NOT estimate for starting", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + time.Sleep(50 * time.Millisecond) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].PositionMs).To(Equal(int64(0))) + }) + + It("respects playbackRate", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "playing", PlaybackRate: 2.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + time.Sleep(100 * time.Millisecond) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + // At 2x speed, 100ms real time = ~200ms playback time + Expect(playing[0].PositionMs).To(BeNumerically(">", int64(10100))) + }) + + It("caps estimated position at track duration", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 179990, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + time.Sleep(50 * time.Millisecond) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].PositionMs).To(Equal(int64(180000))) // track.Duration * 1000 + }) + + }) + + Describe("resilience (no prior starting)", func() { + It("playing without prior starting creates entry with Start approx now - positionMs", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 30000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].State).To(Equal("playing")) + expectedStart := time.Now().Add(-30 * time.Second) + Expect(playing[0].Start).To(BeTemporally("~", expectedStart, 2*time.Second)) + }) + + It("paused without prior starting creates entry", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 30000, State: "paused", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + playing, err := tracker.GetNowPlaying(ctx) + Expect(err).ToNot(HaveOccurred()) + Expect(playing).To(HaveLen(1)) + Expect(playing[0].State).To(Equal("paused")) + }) + + It("stopped without prior starting auto-scrobbles if threshold met", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 90000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(1))) + }) + + It("stopped without prior starting does NOT scrobble if below threshold", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "stopped", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(track.PlayCount).To(Equal(int64(0))) + }) + }) + + Describe("external scrobbler dispatch", func() { + It("dispatches NowPlaying on starting", func() { + fake.nowPlayingCalled.Store(false) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Eventually(func() bool { return fake.GetNowPlayingCalled() }).Should(BeTrue()) + }) + + It("dispatches NowPlaying on playing", func() { + fake.nowPlayingCalled.Store(false) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "playing", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Eventually(func() bool { return fake.GetNowPlayingCalled() }).Should(BeTrue()) + }) + + It("does NOT dispatch on paused", func() { + fake.nowPlayingCalled.Store(false) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 10000, State: "paused", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Consistently(func() bool { return fake.GetNowPlayingCalled() }).Should(BeFalse()) + }) + + It("does NOT dispatch when ignoreScrobble=true", func() { + fake.nowPlayingCalled.Store(false) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + IgnoreScrobble: true, + }) + Expect(err).ToNot(HaveOccurred()) + Consistently(func() bool { return fake.GetNowPlayingCalled() }).Should(BeFalse()) + }) + + It("does NOT dispatch when ScrobbleEnabled=false", func() { + fake.nowPlayingCalled.Store(false) + ctx = request.WithPlayer(ctx, model.Player{ID: "p1", ScrobbleEnabled: false}) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: "starting", PlaybackRate: 1.0, ClientId: defaultClientId, + }) + Expect(err).ToNot(HaveOccurred()) + Consistently(func() bool { return fake.GetNowPlayingCalled() }).Should(BeFalse()) + }) + }) + + Describe("PlaybackReport dispatch", func() { + It("dispatches PlaybackReport for starting state", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StateStarting, PlaybackRate: 1.0, + ClientId: "client-1", ClientName: "Test Player", + }) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() bool { + return fake.PlaybackReportCalled.Load() + }).Should(BeTrue()) + + info := fake.LastPlaybackReport.Load() + Expect(info).ToNot(BeNil()) + Expect(info.MediaFile.ID).To(Equal("123")) + Expect(info.State).To(Equal(StateStarting)) + Expect(info.PositionMs).To(Equal(int64(0))) + Expect(info.PlaybackRate).To(Equal(1.0)) + Expect(info.PlayerId).To(Equal("client-1")) + Expect(info.PlayerName).To(Equal("Test Player")) + }) + + It("dispatches PlaybackReport for playing state", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StateStarting, PlaybackRate: 1.0, + ClientId: "client-1", ClientName: "Test Player", + }) + Expect(err).ToNot(HaveOccurred()) + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + fake.PlaybackReportCalled.Store(false) + fake.LastPlaybackReport.Store(nil) + + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 30000, State: StatePlaying, PlaybackRate: 1.5, + ClientId: "client-1", ClientName: "Test Player", + }) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + info := fake.LastPlaybackReport.Load() + Expect(info.State).To(Equal(StatePlaying)) + Expect(info.PositionMs).To(Equal(int64(30000))) + Expect(info.PlaybackRate).To(Equal(1.5)) + }) + + It("dispatches PlaybackReport for paused state", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StateStarting, PlaybackRate: 1.0, + ClientId: "client-1", ClientName: "Test Player", + }) + Expect(err).ToNot(HaveOccurred()) + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + fake.PlaybackReportCalled.Store(false) + fake.LastPlaybackReport.Store(nil) + + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 45000, State: StatePaused, PlaybackRate: 1.0, + ClientId: "client-1", ClientName: "Test Player", + }) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + info := fake.LastPlaybackReport.Load() + Expect(info.State).To(Equal(StatePaused)) + Expect(info.PositionMs).To(Equal(int64(45000))) + }) + + It("dispatches PlaybackReport for stopped state", func() { + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StateStarting, PlaybackRate: 1.0, + ClientId: "client-1", ClientName: "Test Player", + }) + Expect(err).ToNot(HaveOccurred()) + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + fake.PlaybackReportCalled.Store(false) + fake.LastPlaybackReport.Store(nil) + + err = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 100000, State: StateStopped, PlaybackRate: 1.0, + ClientId: "client-1", ClientName: "Test Player", + }) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() bool { return fake.PlaybackReportCalled.Load() }).Should(BeTrue()) + info := fake.LastPlaybackReport.Load() + Expect(info.State).To(Equal(StateStopped)) + Expect(info.PositionMs).To(Equal(int64(100000))) + }) + }) + }) + Describe("Plugin scrobbler logic", func() { var pluginLoader *mockPluginLoader var pluginFake *fakeScrobbler @@ -349,32 +840,37 @@ var _ = Describe("PlayTracker", func() { tracker = newPlayTracker(ds, events.GetBroker(), pluginLoader) // Bypass buffering for both built-in and plugin scrobblers - tracker.(*playTracker).builtinScrobblers["fake"] = fake - tracker.(*playTracker).pluginScrobblers["plugin1"] = pluginFake + tracker.builtinScrobblers["fake"] = fake + tracker.pluginScrobblers["plugin1"] = pluginFake }) It("registers and uses plugin scrobbler for NowPlaying", func() { - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-1", + }) Expect(err).ToNot(HaveOccurred()) Eventually(func() bool { return pluginFake.GetNowPlayingCalled() }).Should(BeTrue()) }) It("removes plugin scrobbler if not present anymore", func() { - // First call: plugin present - _ = tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) + _ = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-1", + }) Eventually(func() bool { return pluginFake.GetNowPlayingCalled() }).Should(BeTrue()) pluginFake.nowPlayingCalled.Store(false) - // Remove plugin pluginLoader.SetNames([]string{}) - _ = tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) - // Should not be called since plugin was removed + _ = tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-1", + }) Consistently(func() bool { return pluginFake.GetNowPlayingCalled() }).Should(BeFalse()) }) It("calls both builtin and plugin scrobblers for NowPlaying", func() { fake.nowPlayingCalled.Store(false) pluginFake.nowPlayingCalled.Store(false) - err := tracker.NowPlaying(ctx, "player-1", "player-one", "123", 0) + err := tracker.ReportPlayback(ctx, ReportPlaybackParams{ + MediaId: "123", PositionMs: 0, State: StatePlaying, PlaybackRate: 1.0, ClientId: "player-1", + }) Expect(err).ToNot(HaveOccurred()) Eventually(func() bool { return fake.GetNowPlayingCalled() }).Should(BeTrue()) Eventually(func() bool { return pluginFake.GetNowPlayingCalled() }).Should(BeTrue()) @@ -462,7 +958,7 @@ var _ = Describe("PlayTracker", func() { }) AfterEach(func() { - pTracker.stopNowPlayingWorker() + pTracker.stopBackgroundWorkers() }) It("uses the new plugin instance after reload (simulating config update)", func() { @@ -550,16 +1046,36 @@ var _ = Describe("PlayTracker", func() { }) }) +var _ = DescribeTable("remainingTTL", + func(durationSec float32, positionMs int64, rate float64, expected time.Duration) { + Expect(remainingTTL(durationSec, positionMs, rate)).To(Equal(expected)) + }, + Entry("full track at 1x", float32(300), int64(0), 1.0, 305*time.Second), + Entry("halfway through at 1x", float32(300), int64(150000), 1.0, 155*time.Second), + Entry("near end at 1x", float32(300), int64(298000), 1.0, 7*time.Second), + Entry("at end of track", float32(300), int64(300000), 1.0, 5*time.Second), + Entry("past end of track", float32(300), int64(310000), 1.0, 5*time.Second), + Entry("2x speed halves remaining time", float32(300), int64(0), 2.0, 155*time.Second), + Entry("2x speed halfway", float32(300), int64(150000), 2.0, 80*time.Second), + Entry("0.5x speed doubles remaining time", float32(300), int64(0), 0.5, 605*time.Second), + Entry("zero rate defaults to 1x", float32(300), int64(0), 0.0, 305*time.Second), + Entry("negative rate defaults to 1x", float32(300), int64(0), -1.0, 305*time.Second), + Entry("short track", float32(3.5), int64(0), 1.0, 8*time.Second), + Entry("zero duration", float32(0), int64(0), 1.0, 5*time.Second), +) + type fakeScrobbler struct { - Authorized bool - nowPlayingCalled atomic.Bool - ScrobbleCalled atomic.Bool - userID atomic.Pointer[string] - username atomic.Pointer[string] - track atomic.Pointer[model.MediaFile] - position atomic.Int32 - LastScrobble atomic.Pointer[Scrobble] - Error error + Authorized bool + nowPlayingCalled atomic.Bool + ScrobbleCalled atomic.Bool + PlaybackReportCalled atomic.Bool + userID atomic.Pointer[string] + username atomic.Pointer[string] + track atomic.Pointer[model.MediaFile] + position atomic.Int32 + LastScrobble atomic.Pointer[Scrobble] + LastPlaybackReport atomic.Pointer[PlaybackSession] + Error error } func (f *fakeScrobbler) GetNowPlayingCalled() bool { @@ -577,17 +1093,6 @@ func (f *fakeScrobbler) GetTrack() *model.MediaFile { return f.track.Load() } -func (f *fakeScrobbler) GetPosition() int { - return int(f.position.Load()) -} - -func (f *fakeScrobbler) GetUsername() string { - if p := f.username.Load(); p != nil { - return *p - } - return "" -} - func (f *fakeScrobbler) IsAuthorized(ctx context.Context, userId string) bool { return f.Error == nil && f.Authorized } @@ -623,6 +1128,17 @@ func (f *fakeScrobbler) Scrobble(ctx context.Context, userId string, s Scrobble) return nil } +func (f *fakeScrobbler) PlaybackReport(ctx context.Context, info PlaybackSession) error { + f.PlaybackReportCalled.Store(true) + if f.Error != nil { + return f.Error + } + uid := info.UserId + f.userID.Store(&uid) + f.LastPlaybackReport.Store(&info) + return nil +} + func _p(id, name string, sortName ...string) model.Participant { p := model.Participant{Artist: model.Artist{ID: id, Name: name}} if len(sortName) > 0 { @@ -678,3 +1194,7 @@ func (m *mockBufferedScrobbler) NowPlaying(ctx context.Context, userId string, t func (m *mockBufferedScrobbler) Scrobble(ctx context.Context, userId string, s Scrobble) error { return m.wrapped.Scrobble(ctx, userId, s) } + +func (m *mockBufferedScrobbler) PlaybackReport(ctx context.Context, info PlaybackSession) error { + return m.wrapped.PlaybackReport(ctx, info) +} diff --git a/core/scrobbler/playbackreport_worker.go b/core/scrobbler/playbackreport_worker.go new file mode 100644 index 000000000..78ca6e0f7 --- /dev/null +++ b/core/scrobbler/playbackreport_worker.go @@ -0,0 +1,64 @@ +package scrobbler + +import ( + "context" + + "github.com/navidrome/navidrome/log" +) + +func (p *playTracker) enqueuePlaybackReport(ctx context.Context, info PlaybackSession) { + p.prMu.Lock() + defer p.prMu.Unlock() + ctx = context.WithoutCancel(ctx) + p.prQueue = append(p.prQueue, playbackReportEntry{ + ctx: ctx, + info: info, + }) + p.sendPlaybackReportSignal() +} + +func (p *playTracker) sendPlaybackReportSignal() { + select { + case p.prSignal <- struct{}{}: + default: + } +} + +func (p *playTracker) playbackReportWorker() { + defer close(p.prWorkerDone) + for { + select { + case <-p.shutdown: + return + case <-p.prSignal: + } + + p.prMu.Lock() + if len(p.prQueue) == 0 { + p.prMu.Unlock() + continue + } + entries := p.prQueue + p.prQueue = nil + p.prMu.Unlock() + + allScrobblers := p.getActiveScrobblers() + for _, entry := range entries { + p.dispatchPlaybackReport(entry.ctx, entry.info, allScrobblers) + } + } +} + +func (p *playTracker) dispatchPlaybackReport(ctx context.Context, info PlaybackSession, allScrobblers map[string]Scrobbler) { + for name, s := range allScrobblers { + if !s.IsAuthorized(ctx, info.UserId) { + continue + } + log.Debug(ctx, "Sending PlaybackReport", "scrobbler", name, "track", info.MediaFile.Title, "state", info.State, "positionMs", info.PositionMs) + err := s.PlaybackReport(ctx, info) + if err != nil { + log.Error(ctx, "Error sending PlaybackReport", "scrobbler", name, "track", info.MediaFile.Title, "state", info.State, err) + continue + } + } +} diff --git a/core/stream/decider.go b/core/stream/decider.go index cde12f0f3..d6e48497c 100644 --- a/core/stream/decider.go +++ b/core/stream/decider.go @@ -59,18 +59,6 @@ func (s *deciderService) MakeDecision(ctx context.Context, mf *model.MediaFile, decision.SourceStream = buildSourceStream(mf, probe) src := &decision.SourceStream - // Check for server-side player transcoding override - if trc, ok := request.TranscodingFrom(ctx); ok && trc.TargetFormat != "" { - clientInfo = applyServerOverride(ctx, clientInfo, &trc) - } else if player, ok := request.PlayerFrom(ctx); ok && player.MaxBitRate > 0 { - if clientInfo.MaxAudioBitrate == 0 || player.MaxBitRate < clientInfo.MaxAudioBitrate { - modified := *clientInfo - modified.MaxAudioBitrate = player.MaxBitRate - clientInfo = &modified - log.Debug(ctx, "Applied player MaxBitRate cap", "playerMaxBitRate", player.MaxBitRate, "client", clientInfo.Name) - } - } - log.Trace(ctx, "Making transcode decision", "mediaID", mf.ID, "container", src.Container, "codec", src.Codec, "bitrate", src.Bitrate, "channels", src.Channels, "sampleRate", src.SampleRate, "lossless", src.IsLossless, "client", clientInfo.Name) diff --git a/core/stream/decider_test.go b/core/stream/decider_test.go index 8b58f3323..f74953258 100644 --- a/core/stream/decider_test.go +++ b/core/stream/decider_test.go @@ -1042,8 +1042,8 @@ var _ = Describe("Decider", func() { }) }) - Context("Server-side player transcoding override", func() { - It("forces transcoding when override targets a different format", func() { + Context("Server-side context is ignored by MakeDecision", func() { + It("ignores transcoding override in context", func() { mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) ci := &ClientInfo{ Name: "TestClient", @@ -1051,148 +1051,21 @@ var _ = Describe("Decider", func() { {Containers: []string{"flac"}, Protocols: []string{ProtocolHTTP}}, }, } - // Set server override in context overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 192}) - overrideCtx = request.WithPlayer(overrideCtx, model.Player{MaxBitRate: 0}) - decision, err := svc.MakeDecision(overrideCtx, mf, ci, TranscodeOptions{}) Expect(err).ToNot(HaveOccurred()) - Expect(decision.CanDirectPlay).To(BeFalse()) - Expect(decision.CanTranscode).To(BeTrue()) - Expect(decision.TargetFormat).To(Equal("mp3")) - Expect(decision.TargetBitrate).To(Equal(192)) - }) - - It("allows direct play when source matches forced format and bitrate is within cap", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 128, Channels: 2, SampleRate: 44100}) - ci := &ClientInfo{ - Name: "TestClient", - DirectPlayProfiles: []DirectPlayProfile{ - {Containers: []string{"flac"}, Protocols: []string{ProtocolHTTP}}, - }, - } - overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 256}) - - decision, err := svc.MakeDecision(overrideCtx, mf, ci, TranscodeOptions{}) - Expect(err).ToNot(HaveOccurred()) - Expect(decision.CanDirectPlay).To(BeTrue()) - Expect(decision.CanTranscode).To(BeFalse()) - }) - - It("transcodes when source bitrate exceeds the forced cap", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 320, Channels: 2, SampleRate: 44100}) - ci := &ClientInfo{ - Name: "TestClient", - } - overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 192}) - - decision, err := svc.MakeDecision(overrideCtx, mf, ci, TranscodeOptions{}) - Expect(err).ToNot(HaveOccurred()) - Expect(decision.CanDirectPlay).To(BeFalse()) - Expect(decision.CanTranscode).To(BeTrue()) - Expect(decision.TargetFormat).To(Equal("mp3")) - Expect(decision.TargetBitrate).To(Equal(192)) - }) - - It("uses player MaxBitRate over transcoding DefaultBitRate", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) - ci := &ClientInfo{ - Name: "TestClient", - } - overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 192}) - overrideCtx = request.WithPlayer(overrideCtx, model.Player{MaxBitRate: 320}) - - decision, err := svc.MakeDecision(overrideCtx, mf, ci, TranscodeOptions{}) - Expect(err).ToNot(HaveOccurred()) - Expect(decision.CanTranscode).To(BeTrue()) - Expect(decision.TargetFormat).To(Equal("mp3")) - Expect(decision.TargetBitrate).To(Equal(320)) - }) - - It("applies no bitrate cap when both MaxBitRate and DefaultBitRate are 0", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) - ci := &ClientInfo{ - Name: "TestClient", - } - overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 0}) - overrideCtx = request.WithPlayer(overrideCtx, model.Player{MaxBitRate: 0}) - - decision, err := svc.MakeDecision(overrideCtx, mf, ci, TranscodeOptions{}) - Expect(err).ToNot(HaveOccurred()) - Expect(decision.CanTranscode).To(BeTrue()) - Expect(decision.TargetFormat).To(Equal("mp3")) - // With no cap, lossless→lossy uses format default bitrate (160 for mp3 from mock) - Expect(decision.TargetBitrate).To(Equal(160)) - }) - - It("does not apply override when no transcoding is in context", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) - ci := &ClientInfo{ - Name: "TestClient", - DirectPlayProfiles: []DirectPlayProfile{ - {Containers: []string{"flac"}, Protocols: []string{ProtocolHTTP}}, - }, - } - // No override in context — client profiles used as-is - decision, err := svc.MakeDecision(ctx, mf, ci, TranscodeOptions{}) - Expect(err).ToNot(HaveOccurred()) Expect(decision.CanDirectPlay).To(BeTrue()) }) - }) - - Context("Player MaxBitRate cap", func() { - It("applies player MaxBitRate cap when client has no limit", func() { + It("ignores player MaxBitRate in context", func() { mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) ci := &ClientInfo{ Name: "TestClient", DirectPlayProfiles: []DirectPlayProfile{ - {Containers: []string{"flac", "mp3"}, AudioCodecs: []string{"flac", "mp3"}, Protocols: []string{ProtocolHTTP}}, - }, - TranscodingProfiles: []Profile{ - {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, + {Containers: []string{"flac"}, Protocols: []string{ProtocolHTTP}}, }, } playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 320}) - - decision, err := svc.MakeDecision(playerCtx, mf, ci, TranscodeOptions{}) - Expect(err).ToNot(HaveOccurred()) - // Source bitrate 1000 > player cap 320, so direct play is not possible - Expect(decision.CanDirectPlay).To(BeFalse()) - Expect(decision.CanTranscode).To(BeTrue()) - // Lossless→lossy should use MaxAudioBitrate (320) as target, not format default - Expect(decision.TargetBitrate).To(Equal(320)) - }) - - It("uses client limit when it is more restrictive than player MaxBitRate", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) - ci := &ClientInfo{ - Name: "TestClient", - MaxAudioBitrate: 256, - MaxTranscodingAudioBitrate: 256, - TranscodingProfiles: []Profile{ - {Container: "mp3", AudioCodec: "mp3", Protocol: ProtocolHTTP}, - }, - } - playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 500}) - - decision, err := svc.MakeDecision(playerCtx, mf, ci, TranscodeOptions{}) - Expect(err).ToNot(HaveOccurred()) - Expect(decision.CanTranscode).To(BeTrue()) - // Client limit 256 < player cap 500, so player cap doesn't apply; client limit wins - Expect(decision.TargetBitrate).To(Equal(256)) - }) - - It("does not cap when player MaxBitRate is 0", func() { - mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 320, Channels: 2, SampleRate: 44100}) - ci := &ClientInfo{ - Name: "TestClient", - DirectPlayProfiles: []DirectPlayProfile{ - {Containers: []string{"mp3"}, AudioCodecs: []string{"mp3"}, Protocols: []string{ProtocolHTTP}}, - }, - } - playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 0}) - decision, err := svc.MakeDecision(playerCtx, mf, ci, TranscodeOptions{}) Expect(err).ToNot(HaveOccurred()) Expect(decision.CanDirectPlay).To(BeTrue()) diff --git a/core/stream/legacy_client.go b/core/stream/legacy_client.go index d6e929ec8..9dd6179a0 100644 --- a/core/stream/legacy_client.go +++ b/core/stream/legacy_client.go @@ -7,12 +7,11 @@ import ( "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" ) // buildLegacyClientInfo translates legacy Subsonic stream/download parameters // into a ClientInfo for use with MakeDecision. -// It does NOT read request.TranscodingFrom(ctx) — that is handled by -// MakeDecision's applyServerOverride. func buildLegacyClientInfo(mf *model.MediaFile, reqFormat string, reqBitRate int) *ClientInfo { ci := &ClientInfo{Name: "legacy"} @@ -65,6 +64,19 @@ func (s *deciderService) ResolveRequest(ctx context.Context, mf *model.MediaFile } clientInfo := buildLegacyClientInfo(mf, reqFormat, reqBitRate) + + // Apply server-side player transcoding override before making the decision + if trc, ok := request.TranscodingFrom(ctx); ok && trc.TargetFormat != "" { + clientInfo = applyServerOverride(ctx, clientInfo, &trc) + } else if player, ok := request.PlayerFrom(ctx); ok && player.MaxBitRate > 0 { + if clientInfo.MaxAudioBitrate == 0 || player.MaxBitRate < clientInfo.MaxAudioBitrate { + modified := *clientInfo + modified.MaxAudioBitrate = player.MaxBitRate + clientInfo = &modified + log.Debug(ctx, "Applied player MaxBitRate cap", "playerMaxBitRate", player.MaxBitRate, "client", clientInfo.Name) + } + } + decision, err := s.MakeDecision(ctx, mf, clientInfo, TranscodeOptions{SkipProbe: true}) if err != nil { log.Error(ctx, "Error making transcode decision, falling back to raw", "id", mf.ID, err) diff --git a/core/stream/legacy_client_test.go b/core/stream/legacy_client_test.go index de1eb1339..ce7b38650 100644 --- a/core/stream/legacy_client_test.go +++ b/core/stream/legacy_client_test.go @@ -7,6 +7,7 @@ import ( "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -187,6 +188,109 @@ var _ = Describe("ResolveRequest", func() { Expect(req.Offset).To(Equal(30)) }) + Context("Server-side player transcoding override", func() { + It("forces transcoding when override targets a different format", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) + overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 192}) + overrideCtx = request.WithPlayer(overrideCtx, model.Player{MaxBitRate: 0}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(overrideCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("mp3")) + Expect(req.BitRate).To(Equal(192)) + }) + + It("allows direct play when source matches forced format and bitrate is within cap", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 128, Channels: 2, SampleRate: 44100}) + overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 256}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(overrideCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("raw")) + }) + + It("transcodes when source bitrate exceeds the forced cap", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 320, Channels: 2, SampleRate: 44100}) + overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 192}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(overrideCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("mp3")) + Expect(req.BitRate).To(Equal(192)) + }) + + It("uses player MaxBitRate over transcoding DefaultBitRate", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) + overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 192}) + overrideCtx = request.WithPlayer(overrideCtx, model.Player{MaxBitRate: 320}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(overrideCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("mp3")) + Expect(req.BitRate).To(Equal(320)) + }) + + It("applies no bitrate cap when both MaxBitRate and DefaultBitRate are 0", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) + overrideCtx := request.WithTranscoding(ctx, model.Transcoding{TargetFormat: "mp3", DefaultBitRate: 0}) + overrideCtx = request.WithPlayer(overrideCtx, model.Player{MaxBitRate: 0}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(overrideCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("mp3")) + // With no cap, lossless→lossy uses format default bitrate (160 for mp3 from mock) + Expect(req.BitRate).To(Equal(160)) + }) + + It("does not apply override when no transcoding is in context", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(ctx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("raw")) + }) + }) + + Context("Player MaxBitRate cap", func() { + It("applies player MaxBitRate cap when client has no limit", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 320}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(playerCtx, mf, "mp3", 0, 0) + + Expect(req.Format).To(Equal("mp3")) + Expect(req.BitRate).To(Equal(320)) + }) + + It("uses client limit when it is more restrictive than player MaxBitRate", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "flac", Codec: "FLAC", BitRate: 1000, Channels: 2, SampleRate: 44100, BitDepth: 16}) + playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 500}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(playerCtx, mf, "mp3", 256, 0) + + Expect(req.Format).To(Equal("mp3")) + Expect(req.BitRate).To(Equal(256)) + }) + + It("does not cap when player MaxBitRate is 0", func() { + mf := withProbe(&model.MediaFile{ID: "1", Suffix: "mp3", Codec: "MP3", BitRate: 320, Channels: 2, SampleRate: 44100}) + playerCtx := request.WithPlayer(ctx, model.Player{MaxBitRate: 0}) + + decider := svc.(*deciderService) + req := decider.ResolveRequest(playerCtx, mf, "", 0, 0) + + Expect(req.Format).To(Equal("raw")) + }) + }) + Context("fallback for unknown format", func() { It("falls back to DefaultDownsamplingFormat", func() { DeferCleanup(configtest.SetupConfig()) diff --git a/go.mod b/go.mod index a4c0c014b..36218ba6b 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/navidrome/navidrome -go 1.26.0 +go 1.26 // Fork to implement raw tags support replace go.senan.xyz/taglib => github.com/deluan/go-taglib v0.0.0-20260407173416-cf47afbaa67a @@ -35,12 +35,12 @@ require ( github.com/kardianos/service v1.2.4 github.com/kr/pretty v0.3.1 github.com/lestrrat-go/jwx/v3 v3.1.0 - github.com/mattn/go-sqlite3 v1.14.42 + github.com/mattn/go-sqlite3 v1.14.44 github.com/microcosm-cc/bluemonday v1.0.27 github.com/mileusna/useragent v1.3.5 - github.com/onsi/ginkgo/v2 v2.28.2 - github.com/onsi/gomega v1.39.1 - github.com/pelletier/go-toml/v2 v2.3.0 + github.com/onsi/ginkgo/v2 v2.28.3 + github.com/onsi/gomega v1.40.0 + github.com/pelletier/go-toml/v2 v2.3.1 github.com/pmezard/go-difflib v1.0.0 github.com/pocketbase/dbx v1.12.0 github.com/pressly/goose/v3 v3.27.1 @@ -70,7 +70,7 @@ require ( require ( dario.cat/mergo v1.0.2 // indirect - github.com/Masterminds/semver/v3 v3.4.0 // indirect + github.com/Masterminds/semver/v3 v3.5.0 // indirect github.com/atombender/go-jsonschema v0.20.0 // indirect github.com/aymerick/douceur v0.2.0 // indirect github.com/beorn7/perks v1.0.1 // indirect @@ -81,7 +81,7 @@ require ( github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.1 // indirect github.com/dylibso/observe-sdk/go v0.0.0-20240828172851-9145d8ad07e1 // indirect github.com/ebitengine/purego v0.10.0 // indirect - github.com/fsnotify/fsnotify v1.9.0 // indirect + github.com/fsnotify/fsnotify v1.10.0 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-task/slim-sprig/v3 v3.0.0 // indirect github.com/go-viper/mapstructure/v2 v2.5.0 // indirect @@ -93,7 +93,7 @@ require ( github.com/google/subcommands v1.2.0 // indirect github.com/gorilla/css v1.0.1 // indirect github.com/hashicorp/errwrap v1.1.0 // indirect - github.com/ianlancetaylor/demangle v0.0.0-20251118225945-96ee0021ea0f // indirect + github.com/ianlancetaylor/demangle v0.0.0-20260502231528-600b0e508b8c // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect github.com/klauspost/cpuid/v2 v2.3.0 // indirect @@ -138,7 +138,7 @@ require ( golang.org/x/telemetry v0.0.0-20260409153401-be6f6cb8b1fa // indirect golang.org/x/tools v0.44.0 // indirect google.golang.org/protobuf v1.36.11 // indirect - gopkg.in/ini.v1 v1.67.1 // indirect + gopkg.in/ini.v1 v1.67.2 // indirect gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce // indirect ) diff --git a/go.sum b/go.sum index 3e665ba02..90e0ec040 100644 --- a/go.sum +++ b/go.sum @@ -2,8 +2,8 @@ dario.cat/mergo v1.0.2 h1:85+piFYR1tMbRrLcDwR18y4UKJ3aH1Tbzi24VRW1TK8= dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA= filippo.io/edwards25519 v1.2.0 h1:crnVqOiS4jqYleHd9vaKZ+HKtHfllngJIiOpNpoJsjo= filippo.io/edwards25519 v1.2.0/go.mod h1:xzAOLCNug/yB62zG1bQ8uziwrIqIuxhctzJT18Q77mc= -github.com/Masterminds/semver/v3 v3.4.0 h1:Zog+i5UMtVoCU8oKka5P7i9q9HgrJeGzI9SA1Xbatp0= -github.com/Masterminds/semver/v3 v3.4.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= +github.com/Masterminds/semver/v3 v3.5.0 h1:kQceYJfbupGfZOKZQg0kou0DgAKhzDg2NZPAwZ/2OOE= +github.com/Masterminds/semver/v3 v3.5.0/go.mod h1:4V+yj/TJE1HU9XfppCwVMZq3I84lprf4nC11bSS5beM= github.com/Masterminds/squirrel v1.5.4 h1:uUcX/aBc8O7Fg9kaISIUsHXdKuqehiXAMQTYX8afzqM= github.com/Masterminds/squirrel v1.5.4/go.mod h1:NNaOrjSoIDfDA40n7sr2tPNZRfjzjA400rg+riTZj10= github.com/andybalholm/cascadia v1.3.3 h1:AG2YHrzJIm4BZ19iwJ/DAua6Btl3IwJX+VI4kktS1LM= @@ -63,8 +63,8 @@ github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= github.com/frankban/quicktest v1.14.6/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= -github.com/fsnotify/fsnotify v1.9.0 h1:2Ml+OJNzbYCTzsxtv8vKSFD9PbJjmhYF14k/jKC7S9k= -github.com/fsnotify/fsnotify v1.9.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0= +github.com/fsnotify/fsnotify v1.10.0 h1:Xx/5Ydg9CeBDX/wi4VJqStNtohYjitZhhlHt4h3St1M= +github.com/fsnotify/fsnotify v1.10.0/go.mod h1:TLheqan6HD6GBK6PrDWyDPBaEV8LspOxvPSjC+bVfgo= github.com/gen2brain/webp v0.5.5 h1:MvQR75yIPU/9nSqYT5h13k4URaJK3gf9tgz/ksRbyEg= github.com/gen2brain/webp v0.5.5/go.mod h1:xOSMzp4aROt2KFW++9qcK/RBTOVC2S9tJG66ip/9Oc0= github.com/gkampitakis/ciinfo v0.3.2 h1:JcuOPk8ZU7nZQjdUhctuhQofk7BGHuIy0c9Ez8BNhXs= @@ -125,8 +125,8 @@ github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo= github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM= -github.com/ianlancetaylor/demangle v0.0.0-20251118225945-96ee0021ea0f h1:Fnl4pzx8SR7k7JuzyW8lEtSFH6EQ8xgcypgIn8pcGIE= -github.com/ianlancetaylor/demangle v0.0.0-20251118225945-96ee0021ea0f/go.mod h1:gx7rwoVhcfuVKG5uya9Hs3Sxj7EIvldVofAWIUtGouw= +github.com/ianlancetaylor/demangle v0.0.0-20260502231528-600b0e508b8c h1:A1enk+iN8X/J1M/eN4U4NFGQToI51gCvRxEXYrfmqNs= +github.com/ianlancetaylor/demangle v0.0.0-20260502231528-600b0e508b8c/go.mod h1:gx7rwoVhcfuVKG5uya9Hs3Sxj7EIvldVofAWIUtGouw= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/jellydator/ttlcache/v3 v3.4.0 h1:YS4P125qQS0tNhtL6aeYkheEaB/m8HCqdMMP4mnWdTY= @@ -175,8 +175,8 @@ github.com/maruel/natural v1.3.0 h1:VsmCsBmEyrR46RomtgHs5hbKADGRVtliHTyCOLFBpsg= github.com/maruel/natural v1.3.0/go.mod h1:v+Rfd79xlw1AgVBjbO0BEQmptqb5HvL/k9GRHB7ZKEg= github.com/mattn/go-isatty v0.0.21 h1:xYae+lCNBP7QuW4PUnNG61ffM4hVIfm+zUzDuSzYLGs= github.com/mattn/go-isatty v0.0.21/go.mod h1:ZXfXG4SQHsB/w3ZeOYbR0PrPwLy+n6xiMrJlRFqopa4= -github.com/mattn/go-sqlite3 v1.14.42 h1:MigqEP4ZmHw3aIdIT7T+9TLa90Z6smwcthx+Azv4Cgo= -github.com/mattn/go-sqlite3 v1.14.42/go.mod h1:pjEuOr8IwzLJP2MfGeTb0A35jauH+C2kbHKBr7yXKVQ= +github.com/mattn/go-sqlite3 v1.14.44 h1:3VSe+xafpbzsLbdr2AWlAZk9yRHiBhTBakioXaCKTF8= +github.com/mattn/go-sqlite3 v1.14.44/go.mod h1:pjEuOr8IwzLJP2MfGeTb0A35jauH+C2kbHKBr7yXKVQ= github.com/mfridman/interpolate v0.0.2 h1:pnuTK7MQIxxFz1Gr+rjSIx9u7qVjf5VOoM/u6BbAxPY= github.com/mfridman/interpolate v0.0.2/go.mod h1:p+7uk6oE07mpE/Ik1b8EckO0O4ZXiGAfshKBWLUM9Xg= github.com/mfridman/tparse v0.18.0 h1:wh6dzOKaIwkUGyKgOntDW4liXSo37qg5AXbIhkMV3vE= @@ -193,12 +193,12 @@ github.com/ncruces/go-strftime v1.0.0 h1:HMFp8mLCTPp341M/ZnA4qaf7ZlsbTc+miZjCLOF github.com/ncruces/go-strftime v1.0.0/go.mod h1:Fwc5htZGVVkseilnfgOVb9mKy6w1naJmn9CehxcKcls= github.com/ogier/pflag v0.0.1 h1:RW6JSWSu/RkSatfcLtogGfFgpim5p7ARQ10ECk5O750= github.com/ogier/pflag v0.0.1/go.mod h1:zkFki7tvTa0tafRvTBIZTvzYyAu6kQhPZFnshFFPE+g= -github.com/onsi/ginkgo/v2 v2.28.2 h1:DTrMfpqxiNUyQ3Y0zhn1n3cOO2euFgQPYIpkWwxVFps= -github.com/onsi/ginkgo/v2 v2.28.2/go.mod h1:CLtbVInNckU3/+gC8LzkGUb9oF+e8W8TdUsxPwvdOgE= -github.com/onsi/gomega v1.39.1 h1:1IJLAad4zjPn2PsnhH70V4DKRFlrCzGBNrNaru+Vf28= -github.com/onsi/gomega v1.39.1/go.mod h1:hL6yVALoTOxeWudERyfppUcZXjMwIMLnuSfruD2lcfg= -github.com/pelletier/go-toml/v2 v2.3.0 h1:k59bC/lIZREW0/iVaQR8nDHxVq8OVlIzYCOJf421CaM= -github.com/pelletier/go-toml/v2 v2.3.0/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= +github.com/onsi/ginkgo/v2 v2.28.3 h1:4JvMdwtFU0imd8fHx25OJXoDMRexnf8v5NHKYSTTji4= +github.com/onsi/ginkgo/v2 v2.28.3/go.mod h1:+aXOY+vzZ5mu2iI2HpTZUPmM//oQfsNFX6gU9kNcA44= +github.com/onsi/gomega v1.40.0 h1:Vtol0e1MghCD2ZVIilPDIg44XSL9l2QAn8ZNaljWcJc= +github.com/onsi/gomega v1.40.0/go.mod h1:M/Uqpu/8qTjtzCLUA2zJHX9Iilrau25x1PdoSRbWh5A= +github.com/pelletier/go-toml/v2 v2.3.1 h1:MYEvvGnQjeNkRF1qUuGolNtNExTDwct51yp7olPtrEc= +github.com/pelletier/go-toml/v2 v2.3.1/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= @@ -409,8 +409,8 @@ google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q= -gopkg.in/ini.v1 v1.67.1 h1:tVBILHy0R6e4wkYOn3XmiITt/hEVH4TFMYvAX2Ytz6k= -gopkg.in/ini.v1 v1.67.1/go.mod h1:x/cyOwCgZqOkJoDIJ3c1KNHMo10+nLGAhh+kn3Zizss= +gopkg.in/ini.v1 v1.67.2 h1:JtOSMb9OuaCZKr7h5D/h6iii14sK0hLbplTc6frx4Ss= +gopkg.in/ini.v1 v1.67.2/go.mod h1:x/cyOwCgZqOkJoDIJ3c1KNHMo10+nLGAhh+kn3Zizss= gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce h1:+JknDZhAj8YMt7GC73Ei8pv4MzjDUNPHgQWJdtMAaDU= gopkg.in/natefinch/npipe.v2 v2.0.0-20160621034901-c1b8fa8bdcce/go.mod h1:5AcXVHNjg+BDxry382+8OKon8SEWiKktQR07RKPsv1c= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= diff --git a/model/criteria/fields.go b/model/criteria/fields.go index 20c0048b3..9eafff7ab 100644 --- a/model/criteria/fields.go +++ b/model/criteria/fields.go @@ -2,83 +2,95 @@ package criteria import "strings" -// FieldInfo contains semantic metadata about a criteria field +// FieldInfo contains semantic metadata about a criteria field. type FieldInfo struct { - Name string + Alias string // If set, this field is a backward-compat alias for another canonical name IsTag bool IsRole bool Numeric bool - alias string + Boolean bool + + tagAlias string // If set, a tag name from mappings.yml that resolves to this field + name string // Canonical name, populated by LookupField from the map key +} + +// Name returns the canonical field name (the map key used to register this field). +func (f FieldInfo) Name() string { + return f.name } var fieldMap = map[string]FieldInfo{ - "title": {Name: "title"}, - "album": {Name: "album"}, - "hascoverart": {Name: "hascoverart"}, - "tracknumber": {Name: "tracknumber"}, - "discnumber": {Name: "discnumber"}, - "year": {Name: "year"}, - "date": {Name: "date", alias: "recordingdate"}, - "originalyear": {Name: "originalyear"}, - "originaldate": {Name: "originaldate"}, - "releaseyear": {Name: "releaseyear"}, - "releasedate": {Name: "releasedate"}, - "size": {Name: "size"}, - "compilation": {Name: "compilation"}, - "missing": {Name: "missing"}, - "explicitstatus": {Name: "explicitstatus"}, - "dateadded": {Name: "dateadded"}, - "datemodified": {Name: "datemodified"}, - "discsubtitle": {Name: "discsubtitle"}, - "comment": {Name: "comment"}, - "lyrics": {Name: "lyrics"}, - "sorttitle": {Name: "sorttitle"}, - "sortalbum": {Name: "sortalbum"}, - "sortartist": {Name: "sortartist"}, - "sortalbumartist": {Name: "sortalbumartist"}, - "albumcomment": {Name: "albumcomment"}, - "catalognumber": {Name: "catalognumber"}, - "filepath": {Name: "filepath"}, - "filetype": {Name: "filetype"}, - "codec": {Name: "codec"}, - "duration": {Name: "duration"}, - "bitrate": {Name: "bitrate"}, - "bitdepth": {Name: "bitdepth"}, - "samplerate": {Name: "samplerate"}, - "bpm": {Name: "bpm"}, - "channels": {Name: "channels"}, - "loved": {Name: "loved"}, - "dateloved": {Name: "dateloved"}, - "lastplayed": {Name: "lastplayed"}, - "daterated": {Name: "daterated"}, - "playcount": {Name: "playcount"}, - "rating": {Name: "rating"}, - "averagerating": {Name: "averagerating", Numeric: true}, - "albumrating": {Name: "albumrating"}, - "albumloved": {Name: "albumloved"}, - "albumplaycount": {Name: "albumplaycount"}, - "albumlastplayed": {Name: "albumlastplayed"}, - "albumdateloved": {Name: "albumdateloved"}, - "albumdaterated": {Name: "albumdaterated"}, - "artistrating": {Name: "artistrating"}, - "artistloved": {Name: "artistloved"}, - "artistplaycount": {Name: "artistplaycount"}, - "artistlastplayed": {Name: "artistlastplayed"}, - "artistdateloved": {Name: "artistdateloved"}, - "artistdaterated": {Name: "artistdaterated"}, - "mbz_album_id": {Name: "mbz_album_id"}, - "mbz_album_artist_id": {Name: "mbz_album_artist_id"}, - "mbz_artist_id": {Name: "mbz_artist_id"}, - "mbz_recording_id": {Name: "mbz_recording_id"}, - "mbz_release_track_id": {Name: "mbz_release_track_id"}, - "mbz_release_group_id": {Name: "mbz_release_group_id"}, - "library_id": {Name: "library_id", Numeric: true}, + "title": {}, + "album": {}, + "hascoverart": {Boolean: true}, + "tracknumber": {}, + "discnumber": {}, + "year": {}, + "date": {tagAlias: "recordingdate"}, + "originalyear": {}, + "originaldate": {}, + "releaseyear": {}, + "releasedate": {}, + "size": {}, + "compilation": {Boolean: true}, + "missing": {Boolean: true}, + "explicitstatus": {}, + "dateadded": {}, + "datemodified": {}, + "discsubtitle": {}, + "comment": {}, + "lyrics": {}, + "sorttitle": {}, + "sortalbum": {}, + "sortartist": {}, + "sortalbumartist": {}, + "albumcomment": {}, + "catalognumber": {}, + "filepath": {}, + "filetype": {}, + "codec": {}, + "duration": {}, + "bitrate": {}, + "bitdepth": {}, + "samplerate": {}, + "bpm": {}, + "channels": {}, + "loved": {Boolean: true}, + "dateloved": {}, + "lastplayed": {}, + "daterated": {}, + "playcount": {}, + "rating": {}, + "averagerating": {Numeric: true}, + "albumrating": {}, + "albumloved": {Boolean: true}, + "albumplaycount": {}, + "albumlastplayed": {}, + "albumdateloved": {}, + "albumdaterated": {}, + "artistrating": {}, + "artistloved": {Boolean: true}, + "artistplaycount": {}, + "artistlastplayed": {}, + "artistdateloved": {}, + "artistdaterated": {}, + "mbz_album_id": {}, + "mbz_album_artist_id": {}, + "mbz_artist_id": {}, + "mbz_recording_id": {}, + "mbz_release_track_id": {}, + "mbz_release_group_id": {}, + "rgalbumgain": {Numeric: true}, + "rgalbumpeak": {Numeric: true}, + "rgtrackgain": {Numeric: true}, + "rgtrackpeak": {Numeric: true}, + "library_id": {Numeric: true}, // Backward compatibility: albumtype is an alias for the releasetype tag. - "albumtype": {Name: "releasetype", IsTag: true}, + "albumtype": {Alias: "releasetype", IsTag: true}, - "random": {Name: "random"}, - "value": {Name: "value"}, + // Pseudo-field for random sorting + "random": {}, } // AllFieldNames returns the names of all registered criteria fields. @@ -92,7 +104,15 @@ func AllFieldNames() []string { // LookupField returns semantic metadata for a criteria field name. func LookupField(name string) (FieldInfo, bool) { - f, ok := fieldMap[strings.ToLower(name)] + key := strings.ToLower(name) + f, ok := fieldMap[key] + if ok { + if f.Alias != "" { + f.name = f.Alias + } else { + f.name = key + } + } return f, ok } @@ -104,7 +124,7 @@ func AddRoles(roles []string) { if _, ok := fieldMap[name]; ok { continue } - fieldMap[name] = FieldInfo{Name: name, IsRole: true} + fieldMap[name] = FieldInfo{IsRole: true} } } @@ -116,14 +136,16 @@ func AddTagNames(tagNames []string) { if _, ok := fieldMap[name]; ok { continue } - for _, fm := range fieldMap { - if fm.alias == name { + for key, fm := range fieldMap { + if fm.tagAlias == name { + fm.Alias = key + fm.tagAlias = "" fieldMap[name] = fm break } } if _, ok := fieldMap[name]; !ok { - fieldMap[name] = FieldInfo{Name: name, IsTag: true} + fieldMap[name] = FieldInfo{IsTag: true} } } } @@ -136,7 +158,7 @@ func AddNumericTags(tagNames []string) { fm.Numeric = true fieldMap[name] = fm } else { - fieldMap[name] = FieldInfo{Name: name, IsTag: true, Numeric: true} + fieldMap[name] = FieldInfo{IsTag: true, Numeric: true} } } } diff --git a/model/criteria/fields_test.go b/model/criteria/fields_test.go index ecbeb5857..5b6f53341 100644 --- a/model/criteria/fields_test.go +++ b/model/criteria/fields_test.go @@ -11,31 +11,24 @@ var _ = Describe("fields", func() { field, ok := LookupField("Title") gomega.Expect(ok).To(gomega.BeTrue()) - gomega.Expect(field).To(gomega.Equal(FieldInfo{Name: "title"})) + gomega.Expect(field.Name()).To(gomega.Equal("title")) }) - It("resolves aliases to their semantic field name", func() { + It("resolves aliases to their canonical field name", func() { field, ok := LookupField("albumtype") gomega.Expect(ok).To(gomega.BeTrue()) - gomega.Expect(field.Name).To(gomega.Equal("releasetype")) + gomega.Expect(field.Name()).To(gomega.Equal("releasetype")) gomega.Expect(field.IsTag).To(gomega.BeTrue()) }) - It("finds special fields", func() { - field, ok := LookupField("value") - - gomega.Expect(ok).To(gomega.BeTrue()) - gomega.Expect(field.Name).To(gomega.Equal("value")) - }) - It("finds registered tag names", func() { AddTagNames([]string{"task3_mood"}) field, ok := LookupField("task3_mood") gomega.Expect(ok).To(gomega.BeTrue()) - gomega.Expect(field.Name).To(gomega.Equal("task3_mood")) + gomega.Expect(field.Name()).To(gomega.Equal("task3_mood")) gomega.Expect(field.IsTag).To(gomega.BeTrue()) }) @@ -56,8 +49,9 @@ var _ = Describe("fields", func() { field, ok := LookupField("task3_producer") gomega.Expect(ok).To(gomega.BeTrue()) - gomega.Expect(field.Name).To(gomega.Equal("task3_producer")) + gomega.Expect(field.Name()).To(gomega.Equal("task3_producer")) gomega.Expect(field.IsRole).To(gomega.BeTrue()) }) + }) }) diff --git a/model/criteria/json.go b/model/criteria/json.go index f6ab56eda..ca47ceb95 100644 --- a/model/criteria/json.go +++ b/model/criteria/json.go @@ -3,6 +3,7 @@ package criteria import ( "encoding/json" "fmt" + "strconv" "strings" ) @@ -38,6 +39,7 @@ func unmarshalExpression(opName string, rawValue json.RawMessage) Expression { if err != nil { return nil } + normalizeBoolFields(m) switch opName { case "is": return Is(m) @@ -69,10 +71,48 @@ func unmarshalExpression(opName string, rawValue json.RawMessage) Expression { return InPlaylist(m) case "notinplaylist": return NotInPlaylist(m) + case "ismissing": + normalizeAllBoolFields(m) + return IsMissing(m) + case "ispresent": + normalizeAllBoolFields(m) + return IsPresent(m) } return nil } +func normalizeAllBoolFields(m map[string]any) { + for k, v := range m { + m[k] = normalizeBoolValue(v) + } +} + +func normalizeBoolFields(m map[string]any) { + for field, value := range m { + info, ok := LookupField(field) + if ok && info.Boolean { + m[field] = normalizeBoolValue(value) + } + } +} + +func normalizeBoolValue(v any) any { + switch val := v.(type) { + case string: + if b, err := strconv.ParseBool(val); err == nil { + return b + } + case float64: + if val == 1 { + return true + } + if val == 0 { + return false + } + } + return v +} + func unmarshalConjunction(conjName string, rawValue json.RawMessage) Expression { var items unmarshalConjunctionType err := json.Unmarshal(rawValue, &items) diff --git a/model/criteria/operators.go b/model/criteria/operators.go index 983c6aa1a..3ddd77f8b 100644 --- a/model/criteria/operators.go +++ b/model/criteria/operators.go @@ -162,6 +162,22 @@ func (nipl NotInPlaylist) MarshalJSON() ([]byte, error) { func (nipl NotInPlaylist) fields() map[string]any { return nipl } +type IsMissing map[string]any + +func (im IsMissing) MarshalJSON() ([]byte, error) { + return marshalExpression("isMissing", im) +} + +func (im IsMissing) fields() map[string]any { return im } + +type IsPresent map[string]any + +func (ip IsPresent) MarshalJSON() ([]byte, error) { + return marshalExpression("isPresent", ip) +} + +func (ip IsPresent) fields() map[string]any { return ip } + func extractPlaylistIds(inputRule any) (ids []string) { var id string var ok bool diff --git a/model/criteria/operators_test.go b/model/criteria/operators_test.go index c93e8f2b2..17c4272ba 100644 --- a/model/criteria/operators_test.go +++ b/model/criteria/operators_test.go @@ -31,6 +31,7 @@ var _ = Describe("Operators", func() { }, Entry("is [string]", Is{"title": "Low Rider"}, `{"is":{"title":"Low Rider"}}`), Entry("is [bool]", Is{"loved": false}, `{"is":{"loved":false}}`), + Entry("is [string does not coerce non-boolean field]", Is{"title": "true"}, `{"is":{"title":"true"}}`), Entry("isNot", IsNot{"title": "Low Rider"}, `{"isNot":{"title":"Low Rider"}}`), Entry("gt", Gt{"playCount": 10.0}, `{"gt":{"playCount":10}}`), Entry("lt", Lt{"playCount": 10.0}, `{"lt":{"playCount":10}}`), @@ -46,5 +47,83 @@ var _ = Describe("Operators", func() { Entry("notInTheLast", NotInTheLast{"lastPlayed": 30.0}, `{"notInTheLast":{"lastPlayed":30}}`), Entry("inPlaylist", InPlaylist{"id": "deadbeef-dead-beef"}, `{"inPlaylist":{"id":"deadbeef-dead-beef"}}`), Entry("notInPlaylist", NotInPlaylist{"id": "deadbeef-dead-beef"}, `{"notInPlaylist":{"id":"deadbeef-dead-beef"}}`), + Entry("isMissing [true]", IsMissing{"genre": true}, `{"isMissing":{"genre":true}}`), + Entry("isMissing [false]", IsMissing{"genre": false}, `{"isMissing":{"genre":false}}`), + Entry("isPresent [true]", IsPresent{"genre": true}, `{"isPresent":{"genre":true}}`), + Entry("isPresent [false]", IsPresent{"genre": false}, `{"isPresent":{"genre":false}}`), ) + + Describe("Boolean string coercion at unmarshal time (issue #4826)", func() { + It("coerces string 'true' to bool for boolean fields", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"is":{"loved":"true"}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": true})) + }) + + It("coerces string 'false' to bool for boolean fields", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"is":{"loved":"false"}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": false})) + }) + + It("does not coerce string values for non-boolean fields", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"is":{"title":"true"}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(Is{"title": "true"})) + }) + + It("coerces numeric 1 to bool true for boolean fields", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"is":{"loved":1}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": true})) + }) + + It("coerces numeric 0 to bool false for boolean fields", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"is":{"loved":0}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(Is{"loved": false})) + }) + + It("coerces in nested any/all groups", func() { + var c Criteria + err := json.Unmarshal([]byte(`{"all":[{"contains":{"title":"love"}},{"any":[{"is":{"loved":"true"}}]}]}`), &c) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + all := c.Expression.(All) + nested := all[1].(Any) + gomega.Expect(nested[0]).To(gomega.Equal(Is{"loved": true})) + }) + + It("coerces isMissing string 'true' to bool", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"isMissing":{"genre":"true"}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(IsMissing{"genre": true})) + }) + + It("coerces isMissing numeric 0 to bool false", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"isMissing":{"genre":0}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(IsMissing{"genre": false})) + }) + + It("coerces isPresent string 'false' to bool", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"isPresent":{"genre":"false"}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(IsPresent{"genre": false})) + }) + + It("coerces isPresent numeric 1 to bool true", func() { + var obj UnmarshalConjunctionType + err := json.Unmarshal([]byte(`[{"isPresent":{"genre":1}}]`), &obj) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(obj[0]).To(gomega.Equal(IsPresent{"genre": true})) + }) + }) }) diff --git a/model/criteria/sort.go b/model/criteria/sort.go index 05b108cf9..e38fe0551 100644 --- a/model/criteria/sort.go +++ b/model/criteria/sort.go @@ -43,7 +43,7 @@ func (c Criteria) OrderByFields() []SortField { if order == "desc" { desc = !desc } - fields = append(fields, SortField{Field: info.Name, Desc: desc}) + fields = append(fields, SortField{Field: info.Name(), Desc: desc}) } if len(fields) == 0 { log.Warn("No valid sort fields found in 'sort', falling back to 'title'", "sort", sortValue) diff --git a/model/criteria/walk.go b/model/criteria/walk.go index acaf48289..7445c2aef 100644 --- a/model/criteria/walk.go +++ b/model/criteria/walk.go @@ -24,7 +24,7 @@ func Walk(expr Expression, visit Visitor) error { return err } } - case Is, IsNot, Gt, Lt, Before, After, Contains, NotContains, StartsWith, EndsWith, InTheRange, InTheLast, NotInTheLast, InPlaylist, NotInPlaylist: + case Is, IsNot, Gt, Lt, Before, After, Contains, NotContains, StartsWith, EndsWith, InTheRange, InTheLast, NotInTheLast, InPlaylist, NotInPlaylist, IsMissing, IsPresent: return nil default: return fmt.Errorf("unknown criteria expression type %T", expr) diff --git a/persistence/criteria_sql.go b/persistence/criteria_sql.go index 1431f0d0e..a1bae3170 100644 --- a/persistence/criteria_sql.go +++ b/persistence/criteria_sql.go @@ -111,9 +111,12 @@ var smartPlaylistFields = map[string]smartPlaylistField{ "mbz_recording_id": {expr: "media_file.mbz_recording_id"}, "mbz_release_track_id": {expr: "media_file.mbz_release_track_id"}, "mbz_release_group_id": {expr: "media_file.mbz_release_group_id"}, + "rgalbumgain": {expr: "media_file.rg_album_gain"}, + "rgalbumpeak": {expr: "media_file.rg_album_peak"}, + "rgtrackgain": {expr: "media_file.rg_track_gain"}, + "rgtrackpeak": {expr: "media_file.rg_track_peak"}, "library_id": {expr: "media_file.library_id"}, "random": {order: "random()"}, - "value": {expr: "value"}, } func (c smartPlaylistCriteria) Where() (squirrel.Sqlizer, error) { @@ -185,6 +188,10 @@ func (c smartPlaylistCriteria) exprSQL(expr criteria.Expression) (squirrel.Sqliz return c.inList(e, false) case criteria.NotInPlaylist: return c.inList(e, true) + case criteria.IsMissing: + return missingExpr(e, true) + case criteria.IsPresent: + return missingExpr(e, false) default: return nil, fmt.Errorf("unknown criteria expression type %T", expr) } @@ -201,6 +208,26 @@ func isNotExpr(values map[string]any) (squirrel.Sqlizer, error) { return squirrel.NotEq(fields), nil } +func missingExpr(values map[string]any, checkAbsence bool) (squirrel.Sqlizer, error) { + field, value, info, ok := singleField(values) + if !ok { + if len(values) != 1 { + return nil, fmt.Errorf("invalid field in criteria: isMissing/isPresent requires exactly one field") + } + return nil, fmt.Errorf("invalid field in criteria: %s", field) + } + if !info.IsTag && !info.IsRole { + return nil, fmt.Errorf("isMissing/isPresent operator is only supported for tag and role fields, got: %s", field) + } + + b, ok := value.(bool) + if !ok { + return nil, fmt.Errorf("invalid boolean value for 'missing' expression: %s: %v", field, value) + } + negate := checkAbsence == b + return jsonExpr(info, nil, negate), nil +} + func mapExpr(values map[string]any, makeCond func(map[string]any) squirrel.Sqlizer, negateJSON bool) (squirrel.Sqlizer, error) { if _, value, info, ok := singleField(values); ok && (info.IsTag || info.IsRole) { return jsonExpr(info, makeCond(map[string]any{"value": value}), negateJSON), nil @@ -314,9 +341,9 @@ func (c smartPlaylistCriteria) inList(values map[string]any, negate bool) (squir func jsonExpr(info criteria.FieldInfo, cond squirrel.Sqlizer, negate bool) squirrel.Sqlizer { if info.IsRole { - return roleCond{role: info.Name, cond: cond, not: negate} + return roleCond{role: info.Name(), cond: cond, not: negate} } - return tagCond{tag: info.Name, numeric: info.Numeric, cond: cond, not: negate} + return tagCond{tag: info.Name(), numeric: info.Numeric, cond: cond, not: negate} } type tagCond struct { @@ -327,11 +354,18 @@ type tagCond struct { } func (e tagCond) ToSql() (string, []any, error) { - cond, args, err := e.cond.ToSql() - if e.numeric { - cond = strings.ReplaceAll(cond, "value", "CAST(value AS REAL)") + var cond string + var args []any + var err error + if e.cond != nil { + cond, args, err = e.cond.ToSql() + if e.numeric { + cond = strings.ReplaceAll(cond, "value", "CAST(value AS REAL)") + } + cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value' and %s)", e.tag, cond) + } else { + cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value')", e.tag) } - cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.tags, '$.%s') where key='value' and %s)", e.tag, cond) if e.not { cond = "not " + cond } @@ -345,8 +379,15 @@ type roleCond struct { } func (e roleCond) ToSql() (string, []any, error) { - cond, args, err := e.cond.ToSql() - cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.participants, '$.%s') where key='name' and %s)", e.role, cond) + var cond string + var args []any + var err error + if e.cond != nil { + cond, args, err = e.cond.ToSql() + cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.participants, '$.%s') where key='name' and %s)", e.role, cond) + } else { + cond = fmt.Sprintf("exists (select 1 from json_tree(media_file.participants, '$.%s') where key='name')", e.role) + } if e.not { cond = "not " + cond } @@ -374,7 +415,7 @@ func sqlFields(values map[string]any) (map[string]any, error) { if info.IsTag || info.IsRole { return nil, fmt.Errorf("tag and role criteria must contain exactly one field: %s", field) } - sqlField, ok := fieldExpr(info.Name) + sqlField, ok := fieldExpr(info.Name()) if !ok || sqlField == "" { return nil, fmt.Errorf("invalid field in criteria: %s", field) } @@ -393,7 +434,7 @@ func fieldJoinType(name string) smartPlaylistJoinType { if !ok { return smartPlaylistJoinNone } - field, ok := smartPlaylistFields[info.Name] + field, ok := smartPlaylistFields[info.Name()] if !ok { return smartPlaylistJoinNone } @@ -441,17 +482,17 @@ func sortExpr(sortField string) (string, bool) { if !ok { return "", false } - if field, ok := smartPlaylistFields[info.Name]; ok && field.order != "" { + if field, ok := smartPlaylistFields[info.Name()]; ok && field.order != "" { return field.order, true } var mapped string switch { case info.IsTag: - mapped = "COALESCE(json_extract(media_file.tags, '$." + info.Name + "[0].value'), '')" + mapped = "COALESCE(json_extract(media_file.tags, '$." + info.Name() + "[0].value'), '')" case info.IsRole: - mapped = "COALESCE(json_extract(media_file.participants, '$." + info.Name + "[0].name'), '')" + mapped = "COALESCE(json_extract(media_file.participants, '$." + info.Name() + "[0].name'), '')" default: - field, ok := smartPlaylistFields[info.Name] + field, ok := smartPlaylistFields[info.Name()] if !ok || field.expr == "" { return "", false } diff --git a/persistence/criteria_sql_test.go b/persistence/criteria_sql_test.go index e02032d9a..ae2695a4d 100644 --- a/persistence/criteria_sql_test.go +++ b/persistence/criteria_sql_test.go @@ -59,6 +59,30 @@ var _ = Describe("Smart playlist criteria SQL", func() { Entry("role is", criteria.Is{"artist": "u2"}, "exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name' and value = ?)", "u2"), Entry("role contains", criteria.Contains{"composer": "Lennon"}, "exists (select 1 from json_tree(media_file.participants, '$.composer') where key='name' and value LIKE ?)", "%Lennon%"), Entry("role not contains", criteria.NotContains{"artist": "u2"}, "not exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name' and value LIKE ?)", "%u2%"), + // ReplayGain fields + Entry("rgAlbumGain is", criteria.Is{"rgAlbumGain": 0}, "media_file.rg_album_gain = ?", 0), + Entry("rgAlbumGain gt", criteria.Gt{"rgAlbumGain": -6.0}, "media_file.rg_album_gain > ?", -6.0), + Entry("rgTrackPeak lt", criteria.Lt{"rgTrackPeak": 1.0}, "media_file.rg_track_peak < ?", 1.0), + // isMissing — tags + Entry("isMissing tag [true]", criteria.IsMissing{"genre": true}, + "not exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"), + Entry("isMissing tag [false]", criteria.IsMissing{"genre": false}, + "exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"), + // isMissing — roles + Entry("isMissing role [true]", criteria.IsMissing{"artist": true}, + "not exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name')"), + Entry("isMissing role [false]", criteria.IsMissing{"artist": false}, + "exists (select 1 from json_tree(media_file.participants, '$.artist') where key='name')"), + // isPresent — tags + Entry("isPresent tag [true]", criteria.IsPresent{"genre": true}, + "exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"), + Entry("isPresent tag [false]", criteria.IsPresent{"genre": false}, + "not exists (select 1 from json_tree(media_file.tags, '$.genre') where key='value')"), + // isPresent — roles + Entry("isPresent role [true]", criteria.IsPresent{"composer": true}, + "exists (select 1 from json_tree(media_file.participants, '$.composer') where key='name')"), + Entry("isPresent role [false]", criteria.IsPresent{"composer": false}, + "not exists (select 1 from json_tree(media_file.participants, '$.composer') where key='name')"), ) Describe("playlist permissions", func() { @@ -115,6 +139,21 @@ var _ = Describe("Smart playlist criteria SQL", func() { Expect(err).To(MatchError("invalid field in criteria: unknown")) }) + It("returns an error when isMissing is used with a regular field", func() { + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"year": true}}).Where() + Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is only supported for tag and role fields"))) + }) + + It("returns an error when isPresent is used with a regular field", func() { + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsPresent{"title": true}}).Where() + Expect(err).To(MatchError(ContainSubstring("isMissing/isPresent operator is only supported for tag and role fields"))) + }) + + It("returns an error when isMissing has a non-boolean value", func() { + _, err := newSmartPlaylistCriteria(criteria.Criteria{Expression: criteria.IsMissing{"genre": "hello"}}).Where() + Expect(err).To(MatchError(ContainSubstring("invalid boolean value for 'missing' expression"))) + }) + Describe("sort", func() { It("sorts by regular fields", func() { Expect(newSmartPlaylistCriteria(criteria.Criteria{Sort: "title"}).OrderBy()).To(Equal("media_file.title asc")) @@ -160,8 +199,8 @@ var _ = Describe("Smart playlist criteria SQL", func() { if info.IsTag || info.IsRole { continue } - _, hasSQLField := smartPlaylistFields[info.Name] - Expect(hasSQLField).To(BeTrue(), "criteria field %q (name=%q) has no entry in smartPlaylistFields", name, info.Name) + _, hasSQLField := smartPlaylistFields[info.Name()] + Expect(hasSQLField).To(BeTrue(), "criteria field %q (name=%q) has no entry in smartPlaylistFields", name, info.Name()) } }) diff --git a/persistence/e2e/e2e_suite_test.go b/persistence/e2e/e2e_suite_test.go index ea0d0fea8..f42292f02 100644 --- a/persistence/e2e/e2e_suite_test.go +++ b/persistence/e2e/e2e_suite_test.go @@ -116,9 +116,9 @@ func buildTestFS() { fs := storagetest.FakeFS{} fs.SetFiles(fstest.MapFS{ "Rock/The Beatles/Abbey Road/01 - Come Together.mp3": abbeyRoad(track(1, "Come Together", - _t{"genre": "Rock;Blues", "composer": "Lennon/McCartney", "bpm": 120})), + _t{"genre": "Rock;Blues", "composer": "Lennon/McCartney", "bpm": 120, "grouping": "Beatles Tracks"})), "Rock/The Beatles/Abbey Road/02 - Something.mp3": abbeyRoad(track(2, "Something", - _t{"genre": "Rock", "composer": "Harrison", "bpm": 100})), + _t{"genre": "Rock", "composer": "Harrison", "bpm": 100, "grouping": "Beatles Tracks"})), "Rock/Led Zeppelin/IV/01 - Stairway To Heaven.flac": ledZepIV(track(1, "Stairway To Heaven", _t{"genre": "Rock;Folk", "composer": "Page/Plant", "bpm": 82, "suffix": "flac", "bitrate": 900, "samplerate": 44100, "bitdepth": 16})), diff --git a/persistence/e2e/smartplaylist_test.go b/persistence/e2e/smartplaylist_test.go index 086e73703..a844dc982 100644 --- a/persistence/e2e/smartplaylist_test.go +++ b/persistence/e2e/smartplaylist_test.go @@ -330,4 +330,45 @@ var _ = Describe("Smart Playlists", func() { }) }) + + Describe("isMissing/isPresent operators", func() { + It("isMissing finds tracks without grouping tag", func() { + results := evaluateRule(`{"all":[{"isMissing":{"grouping":true}}]}`) + Expect(results).To(ConsistOf("Stairway To Heaven", "Black Dog", "So What", + "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) + }) + + It("isMissing false finds tracks with grouping tag", func() { + results := evaluateRule(`{"all":[{"isMissing":{"grouping":false}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something")) + }) + + It("isPresent finds tracks with grouping tag", func() { + results := evaluateRule(`{"all":[{"isPresent":{"grouping":true}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something")) + }) + + It("isPresent false finds tracks without grouping tag", func() { + results := evaluateRule(`{"all":[{"isPresent":{"grouping":false}}]}`) + Expect(results).To(ConsistOf("Stairway To Heaven", "Black Dog", "So What", + "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) + }) + + It("isMissing returns all tracks for a tag nobody has", func() { + results := evaluateRule(`{"all":[{"isMissing":{"lyricist":true}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven", "Black Dog", + "So What", "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) + }) + + It("isPresent returns all tracks for a role everyone has", func() { + results := evaluateRule(`{"all":[{"isPresent":{"composer":true}}]}`) + Expect(results).To(ConsistOf("Come Together", "Something", "Stairway To Heaven", "Black Dog", + "So What", "Bohemian Rhapsody", "All Along the Watchtower", "We Are the Champions")) + }) + + It("combines isMissing with other operators", func() { + results := evaluateRule(`{"all":[{"isMissing":{"grouping":true}},{"is":{"genre":"Blues"}}]}`) + Expect(results).To(ConsistOf("Black Dog", "All Along the Watchtower")) + }) + }) }) diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 207fe0c36..7bc705385 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -281,6 +281,71 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { Expect(pls.Tracks).To(BeEmpty()) }) + + It("matches loved tracks when loved value is a string in nested group (issue #4826)", func() { + // songComeTogether (ID "1002") is starred in test fixtures + rules := &criteria.Criteria{ + Expression: criteria.All{ + criteria.Any{ + criteria.Is{"loved": "true"}, + }, + }, + } + newPls := model.Playlist{Name: "String Loved Nested", OwnerID: "userid", Rules: rules} + Expect(repo.Put(&newPls)).To(Succeed()) + testPlaylistID = newPls.ID + + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + pls, err := repo.GetWithTracks(newPls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + + trackIDs := make([]string, len(pls.Tracks)) + for i, t := range pls.Tracks { + trackIDs[i] = t.MediaFileID + } + Expect(trackIDs).To(ContainElement("1002")) + Expect(len(pls.Tracks)).To(BeNumerically(">=", 1)) + }) + + It("returns same results for string and bool loved values (issue #4826)", func() { + boolRules := &criteria.Criteria{ + Expression: criteria.All{ + criteria.Any{ + criteria.Is{"loved": true}, + }, + }, + } + boolPls := model.Playlist{Name: "Bool Loved", OwnerID: "userid", Rules: boolRules} + Expect(repo.Put(&boolPls)).To(Succeed()) + DeferCleanup(func() { _ = repo.Delete(boolPls.ID) }) + + stringRules := &criteria.Criteria{ + Expression: criteria.All{ + criteria.Any{ + criteria.Is{"loved": "true"}, + }, + }, + } + stringPls := model.Playlist{Name: "String Loved", OwnerID: "userid", Rules: stringRules} + Expect(repo.Put(&stringPls)).To(Succeed()) + testPlaylistID = stringPls.ID + + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + boolResult, err := repo.GetWithTracks(boolPls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + stringResult, err := repo.GetWithTracks(stringPls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + + boolIDs := make([]string, len(boolResult.Tracks)) + for i, t := range boolResult.Tracks { + boolIDs[i] = t.MediaFileID + } + stringIDs := make([]string, len(stringResult.Tracks)) + for i, t := range stringResult.Tracks { + stringIDs[i] = t.MediaFileID + } + Expect(stringIDs).To(ConsistOf(boolIDs)) + }) }) Describe("Smart Playlists with Tag Criteria", func() { diff --git a/plugins/capabilities/scrobbler.go b/plugins/capabilities/scrobbler.go index ed8a4fb6c..4918d5e8f 100644 --- a/plugins/capabilities/scrobbler.go +++ b/plugins/capabilities/scrobbler.go @@ -5,7 +5,7 @@ package capabilities // ListenBrainz, or custom scrobbling backends. // // All methods are required - plugins implementing this capability must provide -// all three functions: IsAuthorized, NowPlaying, and Scrobble. +// all four functions: IsAuthorized, NowPlaying, Scrobble, and PlaybackReport. // //nd:capability name=scrobbler required=true type Scrobbler interface { @@ -20,6 +20,10 @@ type Scrobbler interface { // Scrobble submits a completed scrobble to the scrobbling service. //nd:export name=nd_scrobbler_scrobble Scrobble(ScrobbleRequest) error + + // PlaybackReport sends a playback state report to the scrobbling service. + //nd:export name=nd_scrobbler_playback_report + PlaybackReport(PlaybackReportRequest) error } // IsAuthorizedRequest is the request for authorization check. @@ -96,6 +100,26 @@ type ScrobbleRequest struct { Timestamp int64 `json:"timestamp"` } +// PlaybackReportRequest is the request for playback report notifications. +type PlaybackReportRequest struct { + // Username is the username of the user. + Username string `json:"username"` + // Track is the track being played. + Track TrackInfo `json:"track"` + // State is the current playback state (starting/playing/paused/stopped/expired). + State string `json:"state"` + // PositionMs is the current playback position in milliseconds. + PositionMs int64 `json:"positionMs"` + // PlaybackRate is the playback speed (1.0 = normal). + PlaybackRate float64 `json:"playbackRate"` + // PlayerId is the unique client identifier. + PlayerId string `json:"playerId"` + // PlayerName is the human-readable player name. + PlayerName string `json:"playerName"` + // Timestamp is the Unix timestamp when this report was generated. + Timestamp int64 `json:"timestamp"` +} + // ScrobblerError represents an error type for scrobbling operations. type ScrobblerError string diff --git a/plugins/capabilities/scrobbler.yaml b/plugins/capabilities/scrobbler.yaml index 8ada5f7e4..9d5cfed30 100644 --- a/plugins/capabilities/scrobbler.yaml +++ b/plugins/capabilities/scrobbler.yaml @@ -18,6 +18,11 @@ exports: input: $ref: '#/components/schemas/ScrobbleRequest' contentType: application/json + nd_scrobbler_playback_report: + description: PlaybackReport sends a playback state report to the scrobbling service. + input: + $ref: '#/components/schemas/PlaybackReportRequest' + contentType: application/json components: schemas: ArtistRef: @@ -59,6 +64,45 @@ components: - username - track - position + PlaybackReportRequest: + description: PlaybackReportRequest is the request for playback report notifications. + properties: + username: + type: string + description: Username is the username of the user. + track: + $ref: '#/components/schemas/TrackInfo' + description: Track is the track being played. + state: + type: string + description: State is the current playback state (starting/playing/paused/stopped/expired). + positionMs: + type: integer + format: int64 + description: PositionMs is the current playback position in milliseconds. + playbackRate: + type: number + format: float + description: PlaybackRate is the playback speed (1.0 = normal). + playerId: + type: string + description: PlayerId is the unique client identifier. + playerName: + type: string + description: PlayerName is the human-readable player name. + timestamp: + type: integer + format: int64 + description: Timestamp is the Unix timestamp when this report was generated. + required: + - username + - track + - state + - positionMs + - playbackRate + - playerId + - playerName + - timestamp ScrobbleRequest: description: ScrobbleRequest is the request for submitting a scrobble. properties: diff --git a/plugins/manager_sync.go b/plugins/manager_sync.go index 2e024ca37..f97069e74 100644 --- a/plugins/manager_sync.go +++ b/plugins/manager_sync.go @@ -138,11 +138,13 @@ func (m *Manager) syncPlugins(ctx context.Context, folder string) error { filesOnDisk := make(map[string]string) // name -> path for _, entry := range entries { if entry.IsDir() || !strings.HasSuffix(entry.Name(), PackageExtension) { + log.Trace(ctx, "Skipping non-plugin entry", "name", entry.Name(), "isDir", entry.IsDir()) continue } name := strings.TrimSuffix(entry.Name(), PackageExtension) filesOnDisk[name] = filepath.Join(folder, entry.Name()) } + log.Debug(ctx, "Plugin sync: scanned folder", "folder", folder, "entriesTotal", len(entries), "pluginsFound", len(filesOnDisk)) // Get all plugins from DB repo := m.ds.Plugin(adminCtx) @@ -154,6 +156,7 @@ func (m *Manager) syncPlugins(ctx context.Context, folder string) error { for i := range dbPlugins { pluginsInDB[dbPlugins[i].ID] = &dbPlugins[i] } + log.Debug(ctx, "Plugin sync: current DB state", "pluginsInDB", len(pluginsInDB)) now := time.Now() diff --git a/plugins/pdk/go/scrobbler/scrobbler.go b/plugins/pdk/go/scrobbler/scrobbler.go index d27ae3a9c..0d045e597 100644 --- a/plugins/pdk/go/scrobbler/scrobbler.go +++ b/plugins/pdk/go/scrobbler/scrobbler.go @@ -52,6 +52,26 @@ type NowPlayingRequest struct { Position int32 `json:"position"` } +// PlaybackReportRequest is the request for playback report notifications. +type PlaybackReportRequest struct { + // Username is the username of the user. + Username string `json:"username"` + // Track is the track being played. + Track TrackInfo `json:"track"` + // State is the current playback state (starting/playing/paused/stopped/expired). + State string `json:"state"` + // PositionMs is the current playback position in milliseconds. + PositionMs int64 `json:"positionMs"` + // PlaybackRate is the playback speed (1.0 = normal). + PlaybackRate float64 `json:"playbackRate"` + // PlayerId is the unique client identifier. + PlayerId string `json:"playerId"` + // PlayerName is the human-readable player name. + PlayerName string `json:"playerName"` + // Timestamp is the Unix timestamp when this report was generated. + Timestamp int64 `json:"timestamp"` +} + // ScrobbleRequest is the request for submitting a scrobble. type ScrobbleRequest struct { // Username is the username of the user. @@ -106,7 +126,7 @@ type TrackInfo struct { // ListenBrainz, or custom scrobbling backends. // // All methods are required - plugins implementing this capability must provide -// all three functions: IsAuthorized, NowPlaying, and Scrobble. +// all four functions: IsAuthorized, NowPlaying, Scrobble, and PlaybackReport. type Scrobbler interface { // IsAuthorized - IsAuthorized checks if a user is authorized to scrobble to this service. IsAuthorized(IsAuthorizedRequest) (bool, error) @@ -114,11 +134,14 @@ type Scrobbler interface { NowPlaying(NowPlayingRequest) error // Scrobble - Scrobble submits a completed scrobble to the scrobbling service. Scrobble(ScrobbleRequest) error + // PlaybackReport - PlaybackReport sends a playback state report to the scrobbling service. + PlaybackReport(PlaybackReportRequest) error } // Internal implementation holders var ( - isAuthorizedImpl func(IsAuthorizedRequest) (bool, error) - nowPlayingImpl func(NowPlayingRequest) error - scrobbleImpl func(ScrobbleRequest) error + isAuthorizedImpl func(IsAuthorizedRequest) (bool, error) + nowPlayingImpl func(NowPlayingRequest) error + scrobbleImpl func(ScrobbleRequest) error + playbackReportImpl func(PlaybackReportRequest) error ) // Register registers a scrobbler implementation. @@ -127,6 +150,7 @@ func Register(impl Scrobbler) { isAuthorizedImpl = impl.IsAuthorized nowPlayingImpl = impl.NowPlaying scrobbleImpl = impl.Scrobble + playbackReportImpl = impl.PlaybackReport } // NotImplementedCode is the standard return code for unimplemented functions. @@ -201,3 +225,24 @@ func _NdScrobblerScrobble() int32 { return 0 } + +//go:wasmexport nd_scrobbler_playback_report +func _NdScrobblerPlaybackReport() int32 { + if playbackReportImpl == nil { + // Return standard code - host will skip this plugin gracefully + return NotImplementedCode + } + + var input PlaybackReportRequest + if err := pdk.InputJSON(&input); err != nil { + pdk.SetError(err) + return -1 + } + + if err := playbackReportImpl(input); err != nil { + pdk.SetError(err) + return -1 + } + + return 0 +} diff --git a/plugins/pdk/go/scrobbler/scrobbler_stub.go b/plugins/pdk/go/scrobbler/scrobbler_stub.go index 9e6f706ac..b35e7c40e 100644 --- a/plugins/pdk/go/scrobbler/scrobbler_stub.go +++ b/plugins/pdk/go/scrobbler/scrobbler_stub.go @@ -49,6 +49,26 @@ type NowPlayingRequest struct { Position int32 `json:"position"` } +// PlaybackReportRequest is the request for playback report notifications. +type PlaybackReportRequest struct { + // Username is the username of the user. + Username string `json:"username"` + // Track is the track being played. + Track TrackInfo `json:"track"` + // State is the current playback state (starting/playing/paused/stopped/expired). + State string `json:"state"` + // PositionMs is the current playback position in milliseconds. + PositionMs int64 `json:"positionMs"` + // PlaybackRate is the playback speed (1.0 = normal). + PlaybackRate float64 `json:"playbackRate"` + // PlayerId is the unique client identifier. + PlayerId string `json:"playerId"` + // PlayerName is the human-readable player name. + PlayerName string `json:"playerName"` + // Timestamp is the Unix timestamp when this report was generated. + Timestamp int64 `json:"timestamp"` +} + // ScrobbleRequest is the request for submitting a scrobble. type ScrobbleRequest struct { // Username is the username of the user. @@ -103,7 +123,7 @@ type TrackInfo struct { // ListenBrainz, or custom scrobbling backends. // // All methods are required - plugins implementing this capability must provide -// all three functions: IsAuthorized, NowPlaying, and Scrobble. +// all four functions: IsAuthorized, NowPlaying, Scrobble, and PlaybackReport. type Scrobbler interface { // IsAuthorized - IsAuthorized checks if a user is authorized to scrobble to this service. IsAuthorized(IsAuthorizedRequest) (bool, error) @@ -111,6 +131,8 @@ type Scrobbler interface { NowPlaying(NowPlayingRequest) error // Scrobble - Scrobble submits a completed scrobble to the scrobbling service. Scrobble(ScrobbleRequest) error + // PlaybackReport - PlaybackReport sends a playback state report to the scrobbling service. + PlaybackReport(PlaybackReportRequest) error } // NotImplementedCode is the standard return code for unimplemented functions. diff --git a/plugins/pdk/rust/nd-pdk-capabilities/src/scrobbler.rs b/plugins/pdk/rust/nd-pdk-capabilities/src/scrobbler.rs index 348460374..1e9c51375 100644 --- a/plugins/pdk/rust/nd-pdk-capabilities/src/scrobbler.rs +++ b/plugins/pdk/rust/nd-pdk-capabilities/src/scrobbler.rs @@ -62,6 +62,35 @@ pub struct NowPlayingRequest { #[serde(default)] pub position: i32, } +/// PlaybackReportRequest is the request for playback report notifications. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct PlaybackReportRequest { + /// Username is the username of the user. + #[serde(default)] + pub username: String, + /// Track is the track being played. + #[serde(default)] + pub track: TrackInfo, + /// State is the current playback state (starting/playing/paused/stopped/expired). + #[serde(default)] + pub state: String, + /// PositionMs is the current playback position in milliseconds. + #[serde(default)] + pub position_ms: i64, + /// PlaybackRate is the playback speed (1.0 = normal). + #[serde(default)] + pub playback_rate: f64, + /// PlayerId is the unique client identifier. + #[serde(default)] + pub player_id: String, + /// PlayerName is the human-readable player name. + #[serde(default)] + pub player_name: String, + /// Timestamp is the Unix timestamp when this report was generated. + #[serde(default)] + pub timestamp: i64, +} /// ScrobbleRequest is the request for submitting a scrobble. #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -158,7 +187,7 @@ impl Error { /// ListenBrainz, or custom scrobbling backends. /// /// All methods are required - plugins implementing this capability must provide -/// all three functions: IsAuthorized, NowPlaying, and Scrobble. +/// all four functions: IsAuthorized, NowPlaying, Scrobble, and PlaybackReport. pub trait Scrobbler { /// IsAuthorized - IsAuthorized checks if a user is authorized to scrobble to this service. fn is_authorized(&self, req: IsAuthorizedRequest) -> Result; @@ -166,6 +195,8 @@ pub trait Scrobbler { fn now_playing(&self, req: NowPlayingRequest) -> Result<(), Error>; /// Scrobble - Scrobble submits a completed scrobble to the scrobbling service. fn scrobble(&self, req: ScrobbleRequest) -> Result<(), Error>; + /// PlaybackReport - PlaybackReport sends a playback state report to the scrobbling service. + fn playback_report(&self, req: PlaybackReportRequest) -> Result<(), Error>; } /// Register all exports for the Scrobbler capability. @@ -197,5 +228,13 @@ macro_rules! register_scrobbler { $crate::scrobbler::Scrobbler::scrobble(&plugin, req.into_inner())?; Ok(()) } + #[extism_pdk::plugin_fn] + pub fn nd_scrobbler_playback_report( + req: extism_pdk::Json<$crate::scrobbler::PlaybackReportRequest> + ) -> extism_pdk::FnResult<()> { + let plugin = <$plugin_type>::default(); + $crate::scrobbler::Scrobbler::playback_report(&plugin, req.into_inner())?; + Ok(()) + } }; } diff --git a/plugins/scrobbler_adapter.go b/plugins/scrobbler_adapter.go index 302f5e1da..8abdccf07 100644 --- a/plugins/scrobbler_adapter.go +++ b/plugins/scrobbler_adapter.go @@ -2,6 +2,7 @@ package plugins import ( "context" + "errors" "strings" "github.com/navidrome/navidrome/core/scrobbler" @@ -16,9 +17,10 @@ const CapabilityScrobbler Capability = "Scrobbler" // Scrobbler function names (snake_case as per design) const ( - FuncScrobblerIsAuthorized = "nd_scrobbler_is_authorized" - FuncScrobblerNowPlaying = "nd_scrobbler_now_playing" - FuncScrobblerScrobble = "nd_scrobbler_scrobble" + FuncScrobblerIsAuthorized = "nd_scrobbler_is_authorized" + FuncScrobblerNowPlaying = "nd_scrobbler_now_playing" + FuncScrobblerScrobble = "nd_scrobbler_scrobble" + FuncScrobblerPlaybackReport = "nd_scrobbler_playback_report" ) func init() { @@ -27,6 +29,7 @@ func init() { FuncScrobblerIsAuthorized, FuncScrobblerNowPlaying, FuncScrobblerScrobble, + FuncScrobblerPlaybackReport, ) } @@ -182,5 +185,25 @@ func mapScrobblerError(err error) error { } } +// PlaybackReport sends a playback state report to the scrobbler +func (s *ScrobblerPlugin) PlaybackReport(ctx context.Context, info scrobbler.PlaybackSession) error { + input := capabilities.PlaybackReportRequest{ + Username: info.Username, + Track: mediaFileToTrackInfo(s.plugin, &info.MediaFile), + State: info.State, + PositionMs: info.PositionMs, + PlaybackRate: info.PlaybackRate, + PlayerId: info.PlayerId, + PlayerName: info.PlayerName, + Timestamp: info.LastReport.Unix(), + } + + err := callPluginFunctionNoOutput(ctx, s.plugin, FuncScrobblerPlaybackReport, input) + if errors.Is(err, errFunctionNotFound) || errors.Is(err, errNotImplemented) { + return nil + } + return mapScrobblerError(err) +} + // Verify interface implementation at compile time var _ scrobbler.Scrobbler = (*ScrobblerPlugin)(nil) diff --git a/plugins/scrobbler_adapter_test.go b/plugins/scrobbler_adapter_test.go index c56d8a900..56a452742 100644 --- a/plugins/scrobbler_adapter_test.go +++ b/plugins/scrobbler_adapter_test.go @@ -229,6 +229,62 @@ var _ = Describe("ScrobblerPlugin", Ordered, func() { }) }) + Describe("PlaybackReport", func() { + It("successfully calls the plugin", func() { + info := scrobbler.PlaybackSession{ + MediaFile: model.MediaFile{ + ID: "track-1", + Title: "Test Song", + Album: "Test Album", + Artist: "Test Artist", + AlbumArtist: "Test Album Artist", + Duration: 180, + TrackNumber: 1, + DiscNumber: 1, + Participants: model.Participants{ + model.RoleArtist: {{Artist: model.Artist{ID: "artist-1", Name: "Test Artist"}}}, + model.RoleAlbumArtist: {{Artist: model.Artist{ID: "album-artist-1", Name: "Test Album Artist"}}}, + }, + }, + Username: "testuser", + PlayerId: "player-1", + PlayerName: "Test Player", + State: "playing", + PositionMs: 30000, + PlaybackRate: 1.0, + LastReport: time.Now(), + } + + err := s.PlaybackReport(ctxWithUser(), info) + Expect(err).ToNot(HaveOccurred()) + }) + + Context("when plugin returns error", Ordered, func() { + var retryScrobbler scrobbler.Scrobbler + + BeforeAll(func() { + mgr, _ := createTestManagerWithPlugins(map[string]map[string]string{ + "test-scrobbler": {"error": "service unavailable", "error_type": "scrobbler(retry_later)"}, + }, "test-scrobbler"+PackageExtension) + + var ok bool + retryScrobbler, ok = mgr.LoadScrobbler("test-scrobbler") + Expect(ok).To(BeTrue()) + }) + + It("returns ErrRetryLater", func() { + info := scrobbler.PlaybackSession{ + MediaFile: model.MediaFile{ID: "track-1", Title: "Test Song"}, + State: "playing", + LastReport: time.Now(), + } + err := retryScrobbler.PlaybackReport(ctxWithUser(), info) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(scrobbler.ErrRetryLater)) + }) + }) + }) + Describe("PluginNames", func() { It("returns plugin names with Scrobbler capability", func() { names := scrobblerManager.PluginNames("Scrobbler") diff --git a/plugins/testdata/test-scrobbler/main.go b/plugins/testdata/test-scrobbler/main.go index d9c142d51..a8cee4a4e 100644 --- a/plugins/testdata/test-scrobbler/main.go +++ b/plugins/testdata/test-scrobbler/main.go @@ -53,6 +53,20 @@ func (t *testScrobbler) Scrobble(input scrobbler.ScrobbleRequest) error { return nil } +// PlaybackReport receives a playback state report. +func (t *testScrobbler) PlaybackReport(input scrobbler.PlaybackReportRequest) error { + if err := checkConfigError(); err != nil { + return err + } + + artistName := "" + if len(input.Track.Artists) > 0 { + artistName = input.Track.Artists[0].Name + } + pdk.Log(pdk.LogInfo, "PlaybackReport: "+input.Track.Title+" by "+artistName+" state="+input.State) + return nil +} + // checkConfigError checks if the plugin is configured to return an error. // If "error" config is set, it returns the appropriate ScrobblerError. // Error types: "not_authorized", "retry_later", "unrecoverable" diff --git a/resources/i18n/zh-Hant.json b/resources/i18n/zh-Hant.json index 93951a311..92b4af3d0 100644 --- a/resources/i18n/zh-Hant.json +++ b/resources/i18n/zh-Hant.json @@ -180,7 +180,7 @@ "name": "名稱", "transcodingId": "轉碼", "maxBitRate": "最大位元率", - "client": "客戶端", + "client": "用戶端", "userName": "使用者名稱", "lastSeen": "上次上線", "reportRealPath": "回報實際路徑", @@ -333,7 +333,7 @@ } }, "plugin": { - "name": "插件 |||| 插件", + "name": "外掛 |||| 外掛", "fields": { "id": "ID", "name": "名稱", @@ -359,7 +359,7 @@ }, "sections": { "status": "狀態", - "info": "插件資訊", + "info": "外掛資訊", "configuration": "設定", "manifest": "資訊清單", "usersPermission": "使用者權限", @@ -379,29 +379,29 @@ "rescan": "重新掃描" }, "notifications": { - "enabled": "插件已啟用", - "disabled": "插件已停用", - "updated": "插件已更新", - "error": "更新插件時發生錯誤" + "enabled": "外掛已啟用", + "disabled": "外掛已停用", + "updated": "外掛已更新", + "error": "更新外掛時發生錯誤" }, "validation": { "invalidJson": "設定必須是有效的 JSON" }, "messages": { - "configHelp": "使用鍵值對設定插件。若插件無需設定則留空。", + "configHelp": "使用鍵值對設定外掛。若外掛無需設定則留空。", "clickPermissions": "點擊權限以查看詳細資訊", "noConfig": "無設定", - "allUsersHelp": "啟用後，插件將可存取所有使用者，包含未來建立的使用者。", + "allUsersHelp": "啟用後，外掛將可存取所有使用者，包含未來建立的使用者。", "noUsers": "未選擇使用者", "permissionReason": "原因", - "usersRequired": "此插件需要存取使用者資訊。請選擇插件可存取的使用者，或啟用「允許所有使用者」。", - "allLibrariesHelp": "啟用後，插件將可存取所有媒體庫，包含未來建立的媒體庫。", + "usersRequired": "此外掛需要存取使用者資訊。請選擇外掛可存取的使用者，或啟用「允許所有使用者」。", + "allLibrariesHelp": "啟用後，外掛將可存取所有媒體庫，包含未來建立的媒體庫。", "noLibraries": "未選擇媒體庫", - "librariesRequired": "此插件需要存取媒體庫資訊。請選擇插件可存取的媒體庫，或啟用「允許所有媒體庫」。", + "librariesRequired": "此外掛需要存取媒體庫資訊。請選擇外掛可存取的媒體庫，或啟用「允許所有媒體庫」。", "requiredHosts": "必要的 Hosts", "configValidationError": "設定驗證失敗：", - "schemaRenderError": "無法顯示設定表單。插件的 schema 可能無效。", - "allowWriteAccessHelp": "啟用後，插件可以修改媒體庫目錄中的檔案。 預設情況下，插件具有唯讀權限。" + "schemaRenderError": "無法顯示設定表單。外掛的 schema 可能無效。", + "allowWriteAccessHelp": "啟用後，外掛可以修改媒體庫目錄中的檔案。 預設情況下，外掛具有唯讀權限。" }, "placeholders": { "configKey": "鍵", @@ -452,7 +452,7 @@ "delete": "刪除", "edit": "編輯", "export": "匯出", - "list": "列表", + "list": "清單", "refresh": "重新整理", "remove_filter": "清除此條件", "remove": "移除", @@ -497,9 +497,9 @@ "upload_single": "拖曳單個圖片上傳或點擊選擇一個" }, "references": { - "all_missing": "未找到參考數據", - "many_missing": "至少有一條參考數據不再可用", - "single_missing": "關聯的參考數據不再可用" + "all_missing": "未找到參考資料", + "many_missing": "至少有一條參考資料不再可用", + "single_missing": "關聯的參考資料不再可用" }, "password": { "toggle_visible": "隱藏密碼", @@ -514,7 +514,7 @@ "delete_content": "您確定要刪除該項目？", "delete_title": "刪除 %{name} #%{id}", "details": "詳細資訊", - "error": "發生客戶端錯誤，您的請求無法完成", + "error": "發生用戶端錯誤，您的請求無法完成", "invalid_form": "提交內容無效，請檢查錯誤", "loading": "正在載入頁面，請稍候", "no": "否", @@ -564,19 +564,19 @@ "delete_user_content": "您確定要刪除此使用者及其所有資料（包括播放清單和偏好設定）嗎？", "notifications_blocked": "您已在瀏覽器設定中封鎖了此網站的通知", "notifications_not_available": "此瀏覽器不支援桌面通知，或您並非透過 HTTPS 存取 Navidrome", - "lastfmLinkSuccess": "已成功連接 Last.fm 並開啟音樂記錄", - "lastfmLinkFailure": "無法連接 Last.fm", - "lastfmUnlinkSuccess": "已取消與 Last.fm 的連接並停用音樂記錄", - "lastfmUnlinkFailure": "無法取消與 Last.fm 的連接", + "lastfmLinkSuccess": "已成功連結 Last.fm 並開啟音樂記錄", + "lastfmLinkFailure": "無法連結 Last.fm", + "lastfmUnlinkSuccess": "已取消與 Last.fm 的連結並停用音樂記錄", + "lastfmUnlinkFailure": "無法取消與 Last.fm 的連結", "openIn": { "lastfm": "在 Last.fm 中開啟", "musicbrainz": "在 MusicBrainz 中開啟" }, "lastfmLink": "查看更多…", - "listenBrainzLinkSuccess": "已成功以 %{user} 的身份連接 ListenBrainz 並開啟音樂記錄", - "listenBrainzLinkFailure": "無法連接 ListenBrainz：%{error}", - "listenBrainzUnlinkSuccess": "已取消與 ListenBrainz 的連接並停用音樂記錄", - "listenBrainzUnlinkFailure": "無法取消與 ListenBrainz 的連接", + "listenBrainzLinkSuccess": "已成功以 %{user} 的身份連結 ListenBrainz 並開啟音樂記錄", + "listenBrainzLinkFailure": "無法連結 ListenBrainz：%{error}", + "listenBrainzUnlinkSuccess": "已取消與 ListenBrainz 的連結並停用音樂記錄", + "listenBrainzUnlinkFailure": "無法取消與 ListenBrainz 的連結", "downloadOriginalFormat": "下載原始格式", "shareOriginalFormat": "分享原始格式", "shareDialogTitle": "分享 %{resource} '%{name}'", @@ -718,4 +718,4 @@ "empty": "無播放內容", "minutesAgo": "1 分鐘前 |||| %{smart_count} 分鐘前" } -} \ No newline at end of file +} diff --git a/scanner/phase_4_playlists.go b/scanner/phase_4_playlists.go index f726343f2..d52743966 100644 --- a/scanner/phase_4_playlists.go +++ b/scanner/phase_4_playlists.go @@ -50,7 +50,7 @@ func (p *phasePlaylists) produce(put func(entry *model.Folder)) error { return nil } u, _ := request.UserFrom(p.ctx) - if !u.IsAdmin { + if !u.IsAdmin || u.ID == "" { log.Warn(p.ctx, "Playlists will not be imported, as there are no admin users yet, "+ "Please create an admin user first, and then update the playlists for them to be imported") return nil diff --git a/scanner/watcher_test.go b/scanner/watcher_test.go index a4016d470..9795129b0 100644 --- a/scanner/watcher_test.go +++ b/scanner/watcher_test.go @@ -74,23 +74,22 @@ var _ = Describe("Watcher", func() { time.Sleep(10 * time.Millisecond) }) - It("creates separate targets for different folders", func() { + It("creates separate targets for different folders", FlakeAttempts(3), func() { // Send notifications for different folders w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist1"} - time.Sleep(10 * time.Millisecond) w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist2"} - // Wait for watcher to process and trigger scan - Eventually(func() int { - return mockScanner.GetScanFoldersCallCount() - }, 200*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) + // Wait for a scan that collected both targets + Eventually(func() []model.ScanTarget { + calls := mockScanner.GetScanFoldersCalls() + if len(calls) == 0 { + return nil + } + return calls[0].Targets + }, 500*time.Millisecond, 10*time.Millisecond).Should(HaveLen(2)) - // Verify two targets + // Verify targets calls := mockScanner.GetScanFoldersCalls() - Expect(calls).To(HaveLen(1)) - Expect(calls[0].Targets).To(HaveLen(2)) - - // Extract folder paths folderPaths := make(map[string]bool) for _, target := range calls[0].Targets { Expect(target.LibraryID).To(Equal(1)) @@ -107,7 +106,7 @@ var _ = Describe("Watcher", func() { // Wait for watcher to process and trigger scan Eventually(func() int { return mockScanner.GetScanFoldersCallCount() - }, 200*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) + }, 500*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) // Verify the target calls := mockScanner.GetScanFoldersCalls() @@ -117,20 +116,15 @@ var _ = Describe("Watcher", func() { }) It("deduplicates folder and file within same folder", func() { - // Send notification for a folder + // Send multiple notifications for the same folder w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist1/album1"} - time.Sleep(10 * time.Millisecond) - // Send notification for same folder (as if file change was detected there) - // In practice, watchLibrary() would walk up from file path to folder w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist1/album1"} - time.Sleep(10 * time.Millisecond) - // Send another for same folder w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist1/album1"} // Wait for watcher to process and trigger scan Eventually(func() int { return mockScanner.GetScanFoldersCallCount() - }, 200*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) + }, 500*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) // Verify only one target despite multiple file/folder changes calls := mockScanner.GetScanFoldersCalls() @@ -151,32 +145,27 @@ var _ = Describe("Watcher", func() { time.Sleep(10 * time.Millisecond) }) - It("resets timer on each change (debouncing)", func() { + It("resets timer on each change (debouncing)", FlakeAttempts(3), func() { // Send first notification w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist1"} - // Wait a bit less than half the watcher wait time to ensure timer doesn't fire - time.Sleep(20 * time.Millisecond) - - // No scan should have been triggered yet - Expect(mockScanner.GetScanFoldersCallCount()).To(Equal(0)) + // Verify no scan fires during a window shorter than the debounce wait + Consistently(func() int { + return mockScanner.GetScanFoldersCallCount() + }, 20*time.Millisecond, 5*time.Millisecond).Should(Equal(0)) // Send another notification (resets timer) w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist1"} - // Wait a bit less than half the watcher wait time again - time.Sleep(20 * time.Millisecond) + // Again, no scan should fire within a short window + Consistently(func() int { + return mockScanner.GetScanFoldersCallCount() + }, 20*time.Millisecond, 5*time.Millisecond).Should(Equal(0)) - // Still no scan - Expect(mockScanner.GetScanFoldersCallCount()).To(Equal(0)) - - // Wait for full timer to expire after last notification (plus margin) - time.Sleep(60 * time.Millisecond) - - // Now scan should have been triggered + // Now wait for the debounce timer to expire and trigger scan Eventually(func() int { return mockScanner.GetScanFoldersCallCount() - }, 100*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) + }, 500*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) }) It("triggers scan after quiet period", func() { @@ -189,7 +178,7 @@ var _ = Describe("Watcher", func() { // Wait for quiet period Eventually(func() int { return mockScanner.GetScanFoldersCallCount() - }, 200*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) + }, 500*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) }) }) @@ -211,7 +200,7 @@ var _ = Describe("Watcher", func() { // Wait for scan Eventually(func() int { return mockScanner.GetScanFoldersCallCount() - }, 200*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) + }, 500*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) // Should scan the library root calls := mockScanner.GetScanFoldersCalls() @@ -223,13 +212,12 @@ var _ = Describe("Watcher", func() { It("deduplicates empty and dot paths", func() { // Send notifications with empty and dot paths w.watcherNotify <- scanNotification{Library: lib, FolderPath: ""} - time.Sleep(10 * time.Millisecond) w.watcherNotify <- scanNotification{Library: lib, FolderPath: ""} // Wait for scan Eventually(func() int { return mockScanner.GetScanFoldersCallCount() - }, 200*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) + }, 500*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) // Should have only one target calls := mockScanner.GetScanFoldersCalls() @@ -264,20 +252,19 @@ var _ = Describe("Watcher", func() { It("creates separate targets for different libraries", func() { // Send notifications for both libraries w.watcherNotify <- scanNotification{Library: lib, FolderPath: "artist1"} - time.Sleep(10 * time.Millisecond) w.watcherNotify <- scanNotification{Library: lib2, FolderPath: "artist2"} - // Wait for scan - Eventually(func() int { - return mockScanner.GetScanFoldersCallCount() - }, 200*time.Millisecond, 10*time.Millisecond).Should(Equal(1)) - - // Verify two targets for different libraries - calls := mockScanner.GetScanFoldersCalls() - Expect(calls).To(HaveLen(1)) - Expect(calls[0].Targets).To(HaveLen(2)) + // Wait for a scan that collected both targets + Eventually(func() []model.ScanTarget { + calls := mockScanner.GetScanFoldersCalls() + if len(calls) == 0 { + return nil + } + return calls[0].Targets + }, 500*time.Millisecond, 10*time.Millisecond).Should(HaveLen(2)) // Verify library IDs are different + calls := mockScanner.GetScanFoldersCalls() libraryIDs := make(map[int]bool) for _, target := range calls[0].Targets { libraryIDs[target.LibraryID] = true diff --git a/scheduler/scheduler_test.go b/scheduler/scheduler_test.go index 1a134a7f3..8ae69c19b 100644 --- a/scheduler/scheduler_test.go +++ b/scheduler/scheduler_test.go @@ -2,6 +2,7 @@ package scheduler import ( "testing" + "time" "github.com/navidrome/navidrome/log" . "github.com/onsi/ginkgo/v2" @@ -28,7 +29,7 @@ var _ = Describe("Scheduler", func() { s.c.Stop() // Stop the scheduler after tests }) - It("adds and executes a job", func() { + It("adds and executes a job", FlakeAttempts(3), func() { done := make(chan struct{}) id, err := s.Add("@every 50ms", func() { @@ -38,7 +39,7 @@ var _ = Describe("Scheduler", func() { Expect(err).ToNot(HaveOccurred()) Expect(id).ToNot(BeZero()) - Eventually(done).Should(BeClosed()) + Eventually(done, 5*time.Second).Should(BeClosed()) }) It("adds a job with random ~ syntax", func() { diff --git a/server/e2e/e2e_suite_test.go b/server/e2e/e2e_suite_test.go index 4b003167a..7ce8de2e6 100644 --- a/server/e2e/e2e_suite_test.go +++ b/server/e2e/e2e_suite_test.go @@ -390,29 +390,13 @@ func (n noopProvider) AlbumImage(context.Context, string) (*url.URL, error) { return nil, model.ErrNotFound } -// noopPlayTracker implements scrobbler.PlayTracker -type noopPlayTracker struct{} - -func (n noopPlayTracker) NowPlaying(context.Context, string, string, string, int) error { - return nil -} - -func (n noopPlayTracker) GetNowPlaying(context.Context) ([]scrobbler.NowPlayingInfo, error) { - return nil, nil -} - -func (n noopPlayTracker) Submit(context.Context, []scrobbler.Submission) error { - return nil -} - // Compile-time interface checks var ( - _ artwork.Artwork = noopArtwork{} - _ stream.MediaStreamer = &spyStreamer{} - _ core.Archiver = noopArchiver{} - _ external.Provider = noopProvider{} - _ scrobbler.PlayTracker = noopPlayTracker{} - _ ffmpeg.FFmpeg = noopFFmpeg{} + _ artwork.Artwork = noopArtwork{} + _ stream.MediaStreamer = &spyStreamer{} + _ core.Archiver = noopArchiver{} + _ external.Provider = noopProvider{} + _ ffmpeg.FFmpeg = noopFFmpeg{} ) var _ = BeforeSuite(func() { @@ -513,7 +497,7 @@ func setupTestDB() { s, events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), - noopPlayTracker{}, + scrobbler.NewPlayTracker(ds, events.NoopBroker(), nil), core.NewShare(ds), playback.PlaybackServer(nil), metrics.NewNoopInstance(), diff --git a/server/e2e/subsonic_media_annotation_test.go b/server/e2e/subsonic_media_annotation_test.go index 0f7e92083..ec9b070de 100644 --- a/server/e2e/subsonic_media_annotation_test.go +++ b/server/e2e/subsonic_media_annotation_test.go @@ -157,4 +157,115 @@ var _ = Describe("Media Annotation Endpoints", Ordered, func() { Expect(resp.Error).ToNot(BeNil()) }) }) + + Describe("ReportPlayback", Ordered, func() { + var songID string + + BeforeAll(func() { + songs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{Max: 1, Sort: "title"}) + Expect(err).ToNot(HaveOccurred()) + Expect(songs).ToNot(BeEmpty()) + songID = songs[0].ID + }) + + It("returns error when required params are missing", func() { + resp := doReq("reportPlayback") + Expect(resp.Status).To(Equal(responses.StatusFailed)) + }) + + It("returns error for invalid state", func() { + resp := doReq("reportPlayback", + "mediaId", songID, + "mediaType", "song", + "positionMs", "0", + "state", "invalid", + ) + Expect(resp.Status).To(Equal(responses.StatusFailed)) + }) + + It("starting report creates a getNowPlaying entry", func() { + resp := doReq("reportPlayback", + "mediaId", songID, + "mediaType", "song", + "positionMs", "0", + "state", "starting", + ) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + np := doReq("getNowPlaying") + Expect(np.Status).To(Equal(responses.StatusOK)) + Expect(np.NowPlaying.Entry).To(HaveLen(1)) + Expect(np.NowPlaying.Entry[0].Id).To(Equal(songID)) + Expect(np.NowPlaying.Entry[0].State).To(Equal("starting")) + }) + + It("playing report updates getNowPlaying state and position", func() { + resp := doReq("reportPlayback", + "mediaId", songID, + "mediaType", "song", + "positionMs", "30000", + "state", "playing", + ) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + np := doReq("getNowPlaying") + Expect(np.NowPlaying.Entry).To(HaveLen(1)) + Expect(np.NowPlaying.Entry[0].State).To(Equal("playing")) + Expect(np.NowPlaying.Entry[0].PositionMs).To(BeNumerically(">=", int64(30000))) + }) + + It("paused report freezes position in getNowPlaying", func() { + resp := doReq("reportPlayback", + "mediaId", songID, + "mediaType", "song", + "positionMs", "30000", + "state", "paused", + ) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + np := doReq("getNowPlaying") + Expect(np.NowPlaying.Entry).To(HaveLen(1)) + Expect(np.NowPlaying.Entry[0].State).To(Equal("paused")) + Expect(np.NowPlaying.Entry[0].PositionMs).To(Equal(int64(30000))) + }) + + It("stopped report removes entry from getNowPlaying", func() { + resp := doReq("reportPlayback", + "mediaId", songID, + "mediaType", "song", + "positionMs", "90000", + "state", "stopped", + ) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + np := doReq("getNowPlaying") + Expect(np.NowPlaying.Entry).To(BeEmpty()) + }) + + It("accepts mediaType=podcast without error", func() { + resp := doReq("reportPlayback", + "mediaId", songID, + "mediaType", "podcast", + "positionMs", "0", + "state", "starting", + ) + Expect(resp.Status).To(Equal(responses.StatusOK)) + }) + + It("accepts optional playbackRate and ignoreScrobble", func() { + resp := doReq("reportPlayback", + "mediaId", songID, + "mediaType", "song", + "positionMs", "5000", + "state", "playing", + "playbackRate", "1.5", + "ignoreScrobble", "true", + ) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + np := doReq("getNowPlaying") + Expect(np.NowPlaying.Entry).To(HaveLen(1)) + Expect(np.NowPlaying.Entry[0].PlaybackRate).To(Equal(1.5)) + }) + }) }) diff --git a/server/e2e/subsonic_playlists_test.go b/server/e2e/subsonic_playlists_test.go index 3468979f4..466e68cf0 100644 --- a/server/e2e/subsonic_playlists_test.go +++ b/server/e2e/subsonic_playlists_test.go @@ -517,4 +517,134 @@ var _ = Describe("Playlist Endpoints", Ordered, func() { Expect(resp.Status).To(Equal(responses.StatusFailed)) }) }) + + Describe("Smart Playlist Boolean String Normalization (issue #4826)", Ordered, func() { + var songID string + var boolPlaylistID, stringPlaylistID, nestedPlaylistID string + + BeforeAll(func() { + setupTestDB() + + songs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{Sort: "title", Max: 1}) + Expect(err).ToNot(HaveOccurred()) + Expect(songs).ToNot(BeEmpty()) + songID = songs[0].ID + + // Star the song via the Subsonic API + resp := doReq("star", "id", songID) + Expect(resp.Status).To(Equal(responses.StatusOK)) + + // Force immediate refresh for all smart playlists + conf.Server.SmartPlaylistRefreshDelay = -1 * time.Second + + // Create smart playlist with boolean true + boolPls := &model.Playlist{ + Name: "Bool Loved", + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.Is{"loved": true}}}, + } + Expect(ds.Playlist(ctx).Put(boolPls)).To(Succeed()) + boolPlaylistID = boolPls.ID + + // Create smart playlist with string "true" + stringPls := &model.Playlist{ + Name: "String Loved", + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.Is{"loved": "true"}}}, + } + Expect(ds.Playlist(ctx).Put(stringPls)).To(Succeed()) + stringPlaylistID = stringPls.ID + + // Create smart playlist with string "true" in nested any group (exact issue #4826 scenario) + nestedPls := &model.Playlist{ + Name: "Nested String Loved", + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{ + criteria.Any{ + criteria.Is{"loved": "true"}, + }, + }}, + } + Expect(ds.Playlist(ctx).Put(nestedPls)).To(Succeed()) + nestedPlaylistID = nestedPls.ID + }) + + It("smart playlist with bool loved=true returns starred song", func() { + resp := doReq("getPlaylist", "id", boolPlaylistID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.SongCount).To(BeNumerically(">=", int32(1))) + entryIDs := make([]string, len(resp.Playlist.Entry)) + for i, e := range resp.Playlist.Entry { + entryIDs[i] = e.Id + } + Expect(entryIDs).To(ContainElement(songID)) + }) + + It("smart playlist with string loved='true' returns same results as bool (issue #4826)", func() { + boolResp := doReq("getPlaylist", "id", boolPlaylistID) + stringResp := doReq("getPlaylist", "id", stringPlaylistID) + + Expect(stringResp.Status).To(Equal(responses.StatusOK)) + Expect(stringResp.Playlist.SongCount).To(Equal(boolResp.Playlist.SongCount)) + }) + + It("nested any group with string loved='true' returns starred song (issue #4826)", func() { + resp := doReq("getPlaylist", "id", nestedPlaylistID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.SongCount).To(BeNumerically(">=", int32(1))) + entryIDs := make([]string, len(resp.Playlist.Entry)) + for i, e := range resp.Playlist.Entry { + entryIDs[i] = e.Id + } + Expect(entryIDs).To(ContainElement(songID)) + }) + + It("isPresent with string 'true' matches songs that have the tag", func() { + pls := &model.Playlist{ + Name: "Genre Present String", + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.IsPresent{"genre": "true"}}}, + } + Expect(ds.Playlist(ctx).Put(pls)).To(Succeed()) + + resp := doReq("getPlaylist", "id", pls.ID) + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.SongCount).To(BeNumerically(">=", int32(1))) + }) + + It("isMissing with string 'true' excludes songs that have the tag", func() { + pls := &model.Playlist{ + Name: "Genre Missing String", + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.IsMissing{"genre": "true"}}}, + } + Expect(ds.Playlist(ctx).Put(pls)).To(Succeed()) + + resp := doReq("getPlaylist", "id", pls.ID) + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.Playlist.SongCount).To(Equal(int32(0))) + }) + + It("isMissing with string 'true' returns same results as bool true", func() { + boolPls := &model.Playlist{ + Name: "Genre Missing Bool", + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.IsMissing{"genre": true}}}, + } + Expect(ds.Playlist(ctx).Put(boolPls)).To(Succeed()) + + stringPls := &model.Playlist{ + Name: "Genre Missing String2", + OwnerID: adminUser.ID, + Rules: &criteria.Criteria{Expression: criteria.All{criteria.IsMissing{"genre": "true"}}}, + } + Expect(ds.Playlist(ctx).Put(stringPls)).To(Succeed()) + + boolResp := doReq("getPlaylist", "id", boolPls.ID) + stringResp := doReq("getPlaylist", "id", stringPls.ID) + Expect(stringResp.Playlist.SongCount).To(Equal(boolResp.Playlist.SongCount)) + }) + }) }) diff --git a/server/e2e/subsonic_sonic_similarity_test.go b/server/e2e/subsonic_sonic_similarity_test.go index 3a010bc67..40161470b 100644 --- a/server/e2e/subsonic_sonic_similarity_test.go +++ b/server/e2e/subsonic_sonic_similarity_test.go @@ -14,6 +14,7 @@ import ( "github.com/navidrome/navidrome/core/metrics" "github.com/navidrome/navidrome/core/playback" "github.com/navidrome/navidrome/core/playlists" + "github.com/navidrome/navidrome/core/scrobbler" "github.com/navidrome/navidrome/core/sonic" "github.com/navidrome/navidrome/core/stream" "github.com/navidrome/navidrome/model" @@ -42,7 +43,7 @@ func buildSonicRouter(provider sonic.Provider) *subsonic.Router { nil, // scanner events.NoopBroker(), playlists.NewPlaylists(ds, core.NewImageUploadService()), - noopPlayTracker{}, + scrobbler.NewPlayTracker(ds, events.NoopBroker(), nil), core.NewShare(ds), playback.PlaybackServer(nil), metrics.NewNoopInstance(), diff --git a/server/e2e/subsonic_transcode_test.go b/server/e2e/subsonic_transcode_test.go index 6041cd013..ae3d6208c 100644 --- a/server/e2e/subsonic_transcode_test.go +++ b/server/e2e/subsonic_transcode_test.go @@ -396,68 +396,30 @@ var _ = Describe("Transcode Endpoints", Ordered, func() { }) }) - Describe("player MaxBitRate cap", func() { - It("forces transcode when source bitrate exceeds player MaxBitRate", func() { + Describe("player MaxBitRate cap is ignored", func() { + It("allows direct play even when source bitrate exceeds player MaxBitRate", func() { setPlayerMaxBitRate(320) // 320 kbps cap - // FLAC is 900kbps, client has no bitrate limit but player cap is 320 + // FLAC is 900kbps, player cap is 320, but getTranscodeDecision + // ignores server-side overrides — client profiles are used as-is resp := doPostReq("getTranscodeDecision", flacAndMp3Client, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) - Expect(resp.TranscodeDecision.CanDirectPlay).To(BeFalse()) - Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) - Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) - Expect(resp.TranscodeDecision.TranscodeStream.Container).To(Equal("mp3")) - // Target bitrate should be capped at player's 320kbps = 320000 bps - Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(320000))) - }) - - It("does not affect direct play when source bitrate is under player MaxBitRate", func() { - setPlayerMaxBitRate(500) // 500 kbps cap - - // MP3 is 320kbps, under the 500kbps player cap → direct play - resp := doPostReq("getTranscodeDecision", mp3OnlyClient, "mediaId", mp3TrackID, "mediaType", "song") - Expect(resp.Status).To(Equal(responses.StatusOK)) - Expect(resp.TranscodeDecision).ToNot(BeNil()) Expect(resp.TranscodeDecision.CanDirectPlay).To(BeTrue()) }) - It("uses client limit when more restrictive than player MaxBitRate", func() { - setPlayerMaxBitRate(500) // 500 kbps player cap - - // Client caps at 320kbps (bitrateCapClient), which is more restrictive than 500 - // FLAC is 900kbps → exceeds both limits → transcode - resp := doPostReq("getTranscodeDecision", bitrateCapClient, "mediaId", flacTrackID, "mediaType", "song") - Expect(resp.Status).To(Equal(responses.StatusOK)) - Expect(resp.TranscodeDecision).ToNot(BeNil()) - Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) - Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) - // Client limit (320kbps) is more restrictive → 320000 bps - Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(320000))) - }) - - It("uses player MaxBitRate when more restrictive than client limit", func() { + It("uses only client limit, not player MaxBitRate", func() { setPlayerMaxBitRate(192) // 192 kbps player cap // Client caps at 320kbps (bitrateCapClient), player is more restrictive at 192 - // FLAC is 900kbps → transcode at 192kbps + // but getTranscodeDecision ignores player cap → client limit (320kbps) applies resp := doPostReq("getTranscodeDecision", bitrateCapClient, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) - // Player limit (192kbps) is more restrictive → 192000 bps - Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(192000))) - }) - - It("has no effect when player MaxBitRate is 0", func() { - setPlayerMaxBitRate(0) // No player cap - - // FLAC with flac+mp3 client → direct play (no bitrate constraint) - resp := doPostReq("getTranscodeDecision", flacAndMp3Client, "mediaId", flacTrackID, "mediaType", "song") - Expect(resp.Status).To(Equal(responses.StatusOK)) - Expect(resp.TranscodeDecision).ToNot(BeNil()) - Expect(resp.TranscodeDecision.CanDirectPlay).To(BeTrue()) + // Only client limit (320kbps) applies → 320000 bps + Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(320000))) }) }) @@ -513,56 +475,37 @@ var _ = Describe("Transcode Endpoints", Ordered, func() { }) }) - Describe("player MaxBitRate + client limits combined", func() { - It("player MaxBitRate injects maxAudioBitrate, format default used for transcode target", func() { + Describe("player MaxBitRate is ignored by getTranscodeDecision", func() { + It("does not inject maxAudioBitrate from player cap", func() { setPlayerMaxBitRate(320) // opusTranscodeClient has no client bitrate limits - // Player cap injects maxAudioBitrate=320 - // FLAC (900kbps) → exceeds 320 → transcode to opus - // Lossless→lossy: maxTranscodingAudioBitrate=0, so falls back to maxAudioBitrate=320 + // Player cap is 320, but getTranscodeDecision ignores it + // FLAC (900kbps) → can't direct play → transcode to opus using format default resp := doPostReq("getTranscodeDecision", opusTranscodeClient, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) Expect(resp.TranscodeDecision.TranscodeStream.Codec).To(Equal("opus")) - // maxAudioBitrate=320 used as fallback → 320000 bps - Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(320000))) + // Bitrate should be opus format default (128kbps), not player cap (320kbps) + Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(128000))) }) - It("player MaxBitRate + client maxTranscodingAudioBitrate work together", func() { + It("uses only client maxTranscodingAudioBitrate, ignoring player cap", func() { setPlayerMaxBitRate(320) - // maxTranscodeBitrateClient: maxTranscodingAudioBitrate=192000 (192kbps), no maxAudioBitrate - // Player cap injects maxAudioBitrate=320 - // FLAC (900kbps) → exceeds 320 → transcode to mp3 - // Lossless→lossy: maxTranscodingAudioBitrate=192 takes priority + // maxTranscodeBitrateClient: maxTranscodingAudioBitrate=192000 (192kbps) + // Player cap is 320, but getTranscodeDecision ignores it + // Only client maxTranscodingAudioBitrate=192 applies resp := doPostReq("getTranscodeDecision", maxTranscodeBitrateClient, "mediaId", flacTrackID, "mediaType", "song") Expect(resp.Status).To(Equal(responses.StatusOK)) Expect(resp.TranscodeDecision).ToNot(BeNil()) Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) Expect(resp.TranscodeDecision.TranscodeStream).ToNot(BeNil()) - // maxTranscodingAudioBitrate=192 is preferred → 192000 bps + // maxTranscodingAudioBitrate=192 → 192000 bps Expect(resp.TranscodeDecision.TranscodeStream.AudioBitrate).To(Equal(int32(192000))) }) - - It("streams with correct bitrate after player MaxBitRate-triggered transcode", func() { - setPlayerMaxBitRate(128) - - // Get decision: FLAC (900kbps) with player cap 128 → transcode - resp := doPostReq("getTranscodeDecision", mp3OnlyClient, "mediaId", flacTrackID, "mediaType", "song") - Expect(resp.Status).To(Equal(responses.StatusOK)) - Expect(resp.TranscodeDecision.CanTranscode).To(BeTrue()) - token := resp.TranscodeDecision.TranscodeParams - Expect(token).ToNot(BeEmpty()) - - // Stream using the token - w := doRawReq("getTranscodeStream", "mediaId", flacTrackID, "mediaType", "song", "transcodeParams", token) - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(streamerSpy.LastRequest.Format).To(Equal("mp3")) - Expect(streamerSpy.LastRequest.BitRate).To(Equal(128)) - }) }) }) diff --git a/server/public/public.go b/server/public/public.go index 5e3407c19..18867e1c4 100644 --- a/server/public/public.go +++ b/server/public/public.go @@ -5,14 +5,12 @@ import ( "path" "github.com/go-chi/chi/v5" - "github.com/go-chi/chi/v5/middleware" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/publicurl" "github.com/navidrome/navidrome/core/stream" - "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/server" "github.com/navidrome/navidrome/ui" @@ -43,13 +41,8 @@ func (pub *Router) routes() http.Handler { r.Group(func(r chi.Router) { r.Use(server.URLParamsMiddleware) r.Group(func(r chi.Router) { - if conf.Server.DevArtworkMaxRequests > 0 { - log.Debug("Throttling public images endpoint", "maxRequests", conf.Server.DevArtworkMaxRequests, - "backlogLimit", conf.Server.DevArtworkThrottleBacklogLimit, "backlogTimeout", - conf.Server.DevArtworkThrottleBacklogTimeout) - r.Use(middleware.ThrottleBacklog(conf.Server.DevArtworkMaxRequests, conf.Server.DevArtworkThrottleBacklogLimit, - conf.Server.DevArtworkThrottleBacklogTimeout)) - } + r.Use(server.ThrottleBacklog(conf.Server.DevArtworkMaxRequests, conf.Server.DevArtworkThrottleBacklogLimit, + conf.Server.DevArtworkThrottleBacklogTimeout)) r.HandleFunc("/img/{id}", pub.handleImages) }) if conf.Server.EnableSharing { diff --git a/server/serve_index.go b/server/serve_index.go index 734aabc70..13fa4a9ce 100644 --- a/server/serve_index.go +++ b/server/serve_index.go @@ -58,6 +58,7 @@ func serveIndex(ds model.DataStore, fs fs.FS, shareInfo *model.Share) http.Handl "uiCoverArtSize": conf.Server.UICoverArtSize, "enableCoverAnimation": conf.Server.EnableCoverAnimation, "enableNowPlaying": conf.Server.EnableNowPlaying, + "playbackReportIntervalMs": conf.Server.UIPlaybackReportInterval.Milliseconds(), "gaTrackingId": conf.Server.GATrackingID, "losslessFormats": strings.ToUpper(strings.Join(mime.LosslessFormats, ",")), "devActivityPanel": conf.Server.DevActivityPanel, diff --git a/server/serve_index_test.go b/server/serve_index_test.go index 31bca02cf..78f3873b8 100644 --- a/server/serve_index_test.go +++ b/server/serve_index_test.go @@ -106,6 +106,7 @@ var _ = Describe("serveIndex", func() { Entry("enableSharing", func() { conf.Server.EnableSharing = true }, "enableSharing", true), Entry("devNewEventStream", func() { conf.Server.DevNewEventStream = true }, "devNewEventStream", true), Entry("extAuthLogoutURL", func() { conf.Server.ExtAuth.LogoutURL = "https://auth.example.com/logout" }, "extAuthLogoutURL", "https://auth.example.com/logout"), + Entry("playbackReportIntervalMs", func() { conf.Server.UIPlaybackReportInterval = 30 * time.Second }, "playbackReportIntervalMs", float64(30000)), ) It("sanitizes entity-encoded welcomeMessage as html", func() { diff --git a/server/subsonic/album_lists.go b/server/subsonic/album_lists.go index 56cf469c5..0d82c8be9 100644 --- a/server/subsonic/album_lists.go +++ b/server/subsonic/album_lists.go @@ -212,13 +212,17 @@ func (api *Router) GetNowPlaying(r *http.Request) (*responses.Subsonic, error) { response := newResponse() response.NowPlaying = &responses.NowPlaying{} var i int32 - response.NowPlaying.Entry = slice.Map(npInfo, func(np scrobbler.NowPlayingInfo) responses.NowPlayingEntry { + response.NowPlaying.Entry = slice.Map(npInfo, func(np scrobbler.PlaybackSession) responses.NowPlayingEntry { + i++ return responses.NowPlayingEntry{ - Child: childFromMediaFile(ctx, np.MediaFile), - UserName: np.Username, - MinutesAgo: int32(time.Since(np.Start).Minutes()), - PlayerId: i + 1, // Fake numeric playerId, it does not seem to be used for anything - PlayerName: np.PlayerName, + Child: childFromMediaFile(ctx, np.MediaFile), + UserName: np.Username, + MinutesAgo: int32(time.Since(np.Start).Minutes()), + PlayerId: i, + PlayerName: np.PlayerName, + State: np.State, + PositionMs: np.PositionMs, + PlaybackRate: np.PlaybackRate, } }) return response, nil diff --git a/server/subsonic/api.go b/server/subsonic/api.go index 5b6565260..1ca364449 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -9,7 +9,6 @@ import ( "regexp" "github.com/go-chi/chi/v5" - "github.com/go-chi/chi/v5/middleware" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/artwork" @@ -144,6 +143,7 @@ func (api *Router) routes() http.Handler { h(r, "star", api.Star) h(r, "unstar", api.Unstar) h(r, "scrobble", api.Scrobble) + h(r, "reportPlayback", api.ReportPlayback) }) r.Group(func(r chi.Router) { r.Use(getPlayer(api.players)) @@ -189,14 +189,8 @@ func (api *Router) routes() http.Handler { hr(r, "getTranscodeStream", api.GetTranscodeStream) }) r.Group(func(r chi.Router) { - // configure request throttling - if conf.Server.DevArtworkMaxRequests > 0 { - log.Debug("Throttling Subsonic getCoverArt endpoint", "maxRequests", conf.Server.DevArtworkMaxRequests, - "backlogLimit", conf.Server.DevArtworkThrottleBacklogLimit, "backlogTimeout", - conf.Server.DevArtworkThrottleBacklogTimeout) - r.Use(middleware.ThrottleBacklog(conf.Server.DevArtworkMaxRequests, conf.Server.DevArtworkThrottleBacklogLimit, - conf.Server.DevArtworkThrottleBacklogTimeout)) - } + r.Use(server.ThrottleBacklog(conf.Server.DevArtworkMaxRequests, conf.Server.DevArtworkThrottleBacklogLimit, + conf.Server.DevArtworkThrottleBacklogTimeout)) hr(r, "getCoverArt", api.GetCoverArt) }) r.Group(func(r chi.Router) { diff --git a/server/subsonic/api_test.go b/server/subsonic/api_test.go index 3b88704b1..f3053c8af 100644 --- a/server/subsonic/api_test.go +++ b/server/subsonic/api_test.go @@ -156,7 +156,7 @@ var _ = Describe("sendResponse", func() { It("updates status pointer when an error occurs", func() { pointer := int32(0) - ctx := context.WithValue(r.Context(), subsonicErrorPointer, &pointer) + ctx := context.WithValue(r.Context(), subsonicErrorPointer, &pointer) //nolint:govet r = r.WithContext(ctx) payload.Status = responses.StatusFailed diff --git a/server/subsonic/media_annotation.go b/server/subsonic/media_annotation.go index 39bc83fa9..e8b0278c1 100644 --- a/server/subsonic/media_annotation.go +++ b/server/subsonic/media_annotation.go @@ -3,6 +3,7 @@ package subsonic import ( "context" "fmt" + "math" "net/http" "time" @@ -217,6 +218,73 @@ func (api *Router) scrobblerNowPlaying(ctx context.Context, trackId string, posi } log.Info(ctx, "Now Playing", "title", mf.Title, "artist", mf.Artist, "user", username, "player", player.Name, "position", position) - err = api.scrobbler.NowPlaying(ctx, clientId, client, trackId, position) - return err + return api.scrobbler.ReportPlayback(ctx, scrobbler.ReportPlaybackParams{ + MediaId: trackId, + PositionMs: int64(position) * 1000, + State: scrobbler.StatePlaying, + PlaybackRate: 1.0, + ClientId: clientId, + ClientName: client, + }) +} + +func (api *Router) ReportPlayback(r *http.Request) (*responses.Subsonic, error) { + p := req.Params(r) + mediaId, err := p.String("mediaId") + if err != nil { + return nil, err + } + mediaType, err := p.String("mediaType") + if err != nil { + return nil, err + } + positionMs, err := p.Int64("positionMs") + if err != nil { + return nil, err + } + if positionMs < 0 { + return nil, newError(responses.ErrorGeneric, "positionMs must be non-negative") + } + state, err := p.String("state") + if err != nil { + return nil, err + } + + if !scrobbler.ValidStates[state] { + return nil, newError(responses.ErrorGeneric, "Invalid state: %s", state) + } + + playbackRate := p.Float64Or("playbackRate", 1.0) + if math.IsNaN(playbackRate) || math.IsInf(playbackRate, 0) || playbackRate <= 0 { + return nil, newError(responses.ErrorGeneric, "playbackRate must be a finite positive number") + } + ignoreScrobble := p.BoolOr("ignoreScrobble", false) + + ctx := r.Context() + if mediaType != "song" { + log.Warn(ctx, "reportPlayback received unsupported mediaType", "mediaType", mediaType, "mediaId", mediaId) + } + + player, _ := request.PlayerFrom(ctx) + client, _ := request.ClientFrom(ctx) + clientId, ok := request.ClientUniqueIdFrom(ctx) + if !ok { + clientId = player.ID + } + + err = api.scrobbler.ReportPlayback(ctx, scrobbler.ReportPlaybackParams{ + MediaId: mediaId, + PositionMs: positionMs, + State: state, + PlaybackRate: playbackRate, + IgnoreScrobble: ignoreScrobble, + ClientId: clientId, + ClientName: client, + }) + if err != nil { + log.Error(ctx, "Error in ReportPlayback", "mediaId", mediaId, "state", state, err) + return nil, err + } + + return newResponse(), nil } diff --git a/server/subsonic/media_annotation_test.go b/server/subsonic/media_annotation_test.go index 1c513edf4..487335d1a 100644 --- a/server/subsonic/media_annotation_test.go +++ b/server/subsonic/media_annotation_test.go @@ -10,6 +10,7 @@ import ( "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/events" + "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -89,35 +90,110 @@ var _ = Describe("MediaAnnotationController", func() { Expect(playTracker.Submissions).To(BeEmpty()) }) - It("registers a NowPlaying", func() { + It("registers a NowPlaying via ReportPlayback", func() { _, err := router.Scrobble(req) Expect(err).ToNot(HaveOccurred()) - Expect(playTracker.Playing).To(HaveLen(1)) - Expect(playTracker.Playing).To(HaveKey("player-1")) + Expect(playTracker.ReportedPlayback).To(HaveLen(1)) + Expect(playTracker.ReportedPlayback[0].MediaId).To(Equal("12")) + Expect(playTracker.ReportedPlayback[0].State).To(Equal(scrobbler.StatePlaying)) + Expect(playTracker.ReportedPlayback[0].ClientId).To(Equal("player-1")) }) }) }) + + Describe("ReportPlayback", func() { + It("returns error when mediaId is missing", func() { + r := newGetRequest("mediaType=song", "positionMs=0", "state=playing") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error when mediaType is missing", func() { + r := newGetRequest("mediaId=123", "positionMs=0", "state=playing") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error when positionMs is missing", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "state=playing") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error when state is missing", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error for invalid state value", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=invalid") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error for negative positionMs", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=-1", "state=playing") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error for NaN playbackRate", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=NaN") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error for Inf playbackRate", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=Inf") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error for negative playbackRate", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=-1.0") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("returns error for zero playbackRate", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=0", "state=playing", "playbackRate=0") + _, err := router.ReportPlayback(r) + Expect(err).To(HaveOccurred()) + }) + + It("accepts mediaType=podcast without error", func() { + r := newGetRequest("mediaId=123", "mediaType=podcast", "positionMs=0", "state=playing") + ctx := request.WithPlayer(r.Context(), model.Player{ID: "p1"}) + r = r.WithContext(ctx) + resp, err := router.ReportPlayback(r) + Expect(err).ToNot(HaveOccurred()) + Expect(resp.Status).To(Equal(responses.StatusOK)) + }) + + It("defaults playbackRate to 1.0 and ignoreScrobble to false", func() { + r := newGetRequest("mediaId=123", "mediaType=song", "positionMs=5000", "state=playing") + ctx := request.WithPlayer(r.Context(), model.Player{ID: "p1"}) + r = r.WithContext(ctx) + _, err := router.ReportPlayback(r) + Expect(err).ToNot(HaveOccurred()) + Expect(playTracker.ReportedPlayback).To(HaveLen(1)) + Expect(playTracker.ReportedPlayback[0].PlaybackRate).To(Equal(1.0)) + Expect(playTracker.ReportedPlayback[0].IgnoreScrobble).To(BeFalse()) + Expect(playTracker.ReportedPlayback[0].ClientId).To(Equal("p1")) + Expect(playTracker.ReportedPlayback[0].ClientName).To(BeEmpty()) + }) + }) }) type fakePlayTracker struct { - Submissions []scrobbler.Submission - Playing map[string]string - Error error + Submissions []scrobbler.Submission + ReportedPlayback []scrobbler.ReportPlaybackParams + Error error } -func (f *fakePlayTracker) NowPlaying(_ context.Context, playerId string, _ string, trackId string, position int) error { - if f.Error != nil { - return f.Error - } - if f.Playing == nil { - f.Playing = make(map[string]string) - } - f.Playing[playerId] = trackId - return nil -} - -func (f *fakePlayTracker) GetNowPlaying(_ context.Context) ([]scrobbler.NowPlayingInfo, error) { +func (f *fakePlayTracker) GetNowPlaying(_ context.Context) ([]scrobbler.PlaybackSession, error) { return nil, f.Error } @@ -129,6 +205,14 @@ func (f *fakePlayTracker) Submit(_ context.Context, submissions []scrobbler.Subm return nil } +func (f *fakePlayTracker) ReportPlayback(_ context.Context, params scrobbler.ReportPlaybackParams) error { + if f.Error != nil { + return f.Error + } + f.ReportedPlayback = append(f.ReportedPlayback, params) + return nil +} + var _ scrobbler.PlayTracker = (*fakePlayTracker)(nil) type fakeEventBroker struct { diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index c3c6d98ea..8461723ed 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -40,7 +40,7 @@ func (api *Router) GetAvatar(w http.ResponseWriter, r *http.Request) (*responses log.Warn(ctx, "User needs an email for gravatar to work", "username", username) return api.getPlaceHolderAvatar(w, r) } - http.Redirect(w, r, gravatar.Url(u.Email, 0), http.StatusFound) + http.Redirect(w, r, gravatar.Url(u.Email, 0), http.StatusFound) //nolint:gosec // URL is not constructed from user input return nil, nil } diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index 45a475da6..0bde18121 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -78,16 +78,13 @@ var _ = Describe("MediaRetrievalController", func() { When("client disconnects (context is cancelled)", func() { It("should not call the service if cancelled before the call", func() { - // Create a request ctx, cancel := context.WithCancel(context.Background()) r := newGetRequest("id=34", "size=128", "square=true") r = r.WithContext(ctx) - cancel() // Cancel the context before the call + cancel() - // Call the GetCoverArt method _, err := router.GetCoverArt(w, r) - // Expect no error and no call to the artwork service Expect(err).ToNot(HaveOccurred()) Expect(artwork.recvId).To(Equal("")) Expect(artwork.recvSize).To(Equal(0)) @@ -96,17 +93,14 @@ var _ = Describe("MediaRetrievalController", func() { }) It("should not return data if cancelled during the call", func() { - // Create a request with a context that will be cancelled ctx, cancel := context.WithCancel(context.Background()) - defer cancel() // Ensure the context is cancelled after the test (best practices) + defer cancel() r := newGetRequest("id=34", "size=128", "square=true") r = r.WithContext(ctx) - artwork.ctxCancelFunc = cancel // Set the cancel function to simulate cancellation in the service + artwork.ctxCancelFunc = cancel - // Call the GetCoverArt method _, err := router.GetCoverArt(w, r) - // Expect no error and the service to have been called Expect(err).ToNot(HaveOccurred()) Expect(artwork.recvId).To(Equal("34")) Expect(artwork.recvSize).To(Equal(128)) @@ -834,7 +828,7 @@ func (c *fakeArtwork) GetOrPlaceholder(_ context.Context, id string, size int, s c.recvSize = size c.recvSquare = square if c.ctxCancelFunc != nil { - c.ctxCancelFunc() // Simulate context cancellation + c.ctxCancelFunc() return nil, time.Time{}, context.Canceled } return io.NopCloser(bytes.NewReader([]byte(c.data))), time.Time{}, nil @@ -853,9 +847,7 @@ func (m *mockedMediaFile) GetAll(opts ...model.QueryOptions) (model.MediaFiles, return data, nil } - // Hardcoded support for lyrics sorting result := slices.Clone(data) - // Sort by presence of lyrics, then by updated_at. Respect the order specified in opts. slices.SortFunc(result, func(a, b model.MediaFile) int { diff := cmp.Or( cmp.Compare(a.Lyrics, b.Lyrics), diff --git a/server/subsonic/middlewares.go b/server/subsonic/middlewares.go index 2d8b1fd94..5832bb1de 100644 --- a/server/subsonic/middlewares.go +++ b/server/subsonic/middlewares.go @@ -199,7 +199,7 @@ func getPlayer(players core.Players) func(next http.Handler) http.Handler { } r = r.WithContext(ctx) - cookie := &http.Cookie{ + cookie := &http.Cookie{ //nolint:gosec // Secure omitted: Navidrome may run over plain HTTP Name: playerIDCookieName(userName), Value: player.ID, MaxAge: consts.CookieExpiry, @@ -239,7 +239,9 @@ func playerIDCookieName(userName string) string { return cookieName } -const subsonicErrorPointer = "subsonicErrorPointer" +type contextKey string + +const subsonicErrorPointer contextKey = "subsonicErrorPointer" func recordStats(metrics metrics.Metrics) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { diff --git a/server/subsonic/opensubsonic.go b/server/subsonic/opensubsonic.go index 16bd6805f..97b3cafcc 100644 --- a/server/subsonic/opensubsonic.go +++ b/server/subsonic/opensubsonic.go @@ -14,6 +14,7 @@ func (api *Router) GetOpenSubsonicExtensions(_ *http.Request) (*responses.Subson {Name: "songLyrics", Versions: []int32{1, 2}}, {Name: "indexBasedQueue", Versions: []int32{1}}, {Name: "transcoding", Versions: []int32{1}}, + {Name: "playbackReport", Versions: []int32{1}}, } if api.sonic != nil && api.sonic.HasProvider() { extensions = append(extensions, responses.OpenSubsonicExtension{ diff --git a/server/subsonic/opensubsonic_test.go b/server/subsonic/opensubsonic_test.go index 5031f1c69..e4217303f 100644 --- a/server/subsonic/opensubsonic_test.go +++ b/server/subsonic/opensubsonic_test.go @@ -44,42 +44,13 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { router = subsonic.New(nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil) }) - It("should return the base 5 OpenSubsonicExtensions without sonicSimilarity", func() { + It("should return the base 6 OpenSubsonicExtensions without sonicSimilarity", func() { router.ServeHTTP(w, r) // Make sure the endpoint is public, by not passing any authentication Expect(w.Code).To(Equal(http.StatusOK)) Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) - var response responses.JsonWrapper - err := json.Unmarshal(w.Body.Bytes(), &response) - Expect(err).NotTo(HaveOccurred()) - Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( - HaveLen(5), - ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), - ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), - ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), - )) - Expect(*response.Subsonic.OpenSubsonicExtensions).NotTo( - ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), - ) - }) - }) - - Context("with sonic similarity plugin", func() { - BeforeEach(func() { - sonicService := sonicsvc.New(nil, &mockSonicPluginLoader{names: []string{"test-plugin"}}, nil) - router = subsonic.New(nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, sonicService) - }) - - It("should return 6 extensions including sonicSimilarity", func() { - router.ServeHTTP(w, r) - - Expect(w.Code).To(Equal(http.StatusOK)) - Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) - var response responses.JsonWrapper err := json.Unmarshal(w.Body.Bytes(), &response) Expect(err).NotTo(HaveOccurred()) @@ -90,6 +61,37 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), + )) + Expect(*response.Subsonic.OpenSubsonicExtensions).NotTo( + ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), + ) + }) + }) + + Context("with sonic similarity plugin", func() { + BeforeEach(func() { + sonicService := sonicsvc.New(nil, &mockSonicPluginLoader{names: []string{"test-plugin"}}, nil) + router = subsonic.New(nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, sonicService) + }) + + It("should return 7 extensions including sonicSimilarity", func() { + router.ServeHTTP(w, r) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Content-Type")).To(Equal("application/json")) + + var response responses.JsonWrapper + err := json.Unmarshal(w.Body.Bytes(), &response) + Expect(err).NotTo(HaveOccurred()) + Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( + HaveLen(7), + ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), + ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), )) }) diff --git a/server/subsonic/responses/.snapshots/Responses NowPlaying with data should match .JSON b/server/subsonic/responses/.snapshots/Responses NowPlaying with data should match .JSON new file mode 100644 index 000000000..f99cb62af --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses NowPlaying with data should match .JSON @@ -0,0 +1,23 @@ +{ + "status": "ok", + "version": "1.16.1", + "type": "navidrome", + "serverVersion": "v0.55.0", + "openSubsonic": true, + "nowPlaying": { + "entry": [ + { + "id": "1", + "isDir": false, + "title": "Song", + "username": "testuser", + "minutesAgo": 2, + "playerId": 1, + "playerName": "TestPlayer", + "state": "playing", + "positionMs": 120000, + "playbackRate": 1.5 + } + ] + } +} diff --git a/server/subsonic/responses/.snapshots/Responses NowPlaying with data should match .XML b/server/subsonic/responses/.snapshots/Responses NowPlaying with data should match .XML new file mode 100644 index 000000000..ae05b68d0 --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses NowPlaying with data should match .XML @@ -0,0 +1,5 @@ + + + + + diff --git a/server/subsonic/responses/.snapshots/Responses NowPlaying without data should match .JSON b/server/subsonic/responses/.snapshots/Responses NowPlaying without data should match .JSON new file mode 100644 index 000000000..ab8f793fa --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses NowPlaying without data should match .JSON @@ -0,0 +1,8 @@ +{ + "status": "ok", + "version": "1.16.1", + "type": "navidrome", + "serverVersion": "v0.55.0", + "openSubsonic": true, + "nowPlaying": {} +} diff --git a/server/subsonic/responses/.snapshots/Responses NowPlaying without data should match .XML b/server/subsonic/responses/.snapshots/Responses NowPlaying without data should match .XML new file mode 100644 index 000000000..763b4b583 --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses NowPlaying without data should match .XML @@ -0,0 +1,3 @@ + + + diff --git a/server/subsonic/responses/responses.go b/server/subsonic/responses/responses.go index 8d3279d87..b8b5a03cd 100644 --- a/server/subsonic/responses/responses.go +++ b/server/subsonic/responses/responses.go @@ -358,10 +358,13 @@ type Starred2 struct { type NowPlayingEntry struct { Child - UserName string `xml:"username,attr" json:"username"` - MinutesAgo int32 `xml:"minutesAgo,attr" json:"minutesAgo"` - PlayerId int32 `xml:"playerId,attr" json:"playerId"` - PlayerName string `xml:"playerName,attr" json:"playerName,omitempty"` + UserName string `xml:"username,attr" json:"username"` + MinutesAgo int32 `xml:"minutesAgo,attr" json:"minutesAgo"` + PlayerId int32 `xml:"playerId,attr" json:"playerId"` + PlayerName string `xml:"playerName,attr" json:"playerName,omitempty"` + State string `xml:"state,attr" json:"state"` + PositionMs int64 `xml:"positionMs,attr" json:"positionMs"` + PlaybackRate float64 `xml:"playbackRate,attr" json:"playbackRate"` } type NowPlaying struct { diff --git a/server/subsonic/responses/responses_test.go b/server/subsonic/responses/responses_test.go index ba86deaf2..ee98a3daa 100644 --- a/server/subsonic/responses/responses_test.go +++ b/server/subsonic/responses/responses_test.go @@ -1109,6 +1109,42 @@ var _ = Describe("Responses", func() { }) }) + Describe("NowPlaying", func() { + BeforeEach(func() { + response.NowPlaying = &NowPlaying{} + }) + + Describe("without data", func() { + It("should match .XML", func() { + Expect(xml.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + It("should match .JSON", func() { + Expect(json.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + }) + + Describe("with data", func() { + BeforeEach(func() { + response.NowPlaying.Entry = []NowPlayingEntry{{ + Child: Child{Id: "1", Title: "Song", IsDir: false}, + UserName: "testuser", + MinutesAgo: 2, + PlayerId: 1, + PlayerName: "TestPlayer", + State: "playing", + PositionMs: 120000, + PlaybackRate: 1.5, + }} + }) + It("should match .XML", func() { + Expect(xml.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + It("should match .JSON", func() { + Expect(json.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + }) + }) + Describe("SonicMatches", func() { Context("without data", func() { BeforeEach(func() { diff --git a/server/throttle_backlog.go b/server/throttle_backlog.go new file mode 100644 index 000000000..c3672fd1e --- /dev/null +++ b/server/throttle_backlog.go @@ -0,0 +1,150 @@ +package server + +import ( + "bytes" + "context" + "errors" + "net/http" + "sync" + "time" + + "github.com/go-chi/chi/v5/middleware" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/log" +) + +var ( + ErrThrottleCapacityExceeded = errors.New("throttle: capacity exceeded") + ErrThrottleTimeout = errors.New("throttle: backlog timeout") +) + +type requestThrottle struct { + tokens chan struct{} + backlogTokens chan struct{} + backlogTimeout time.Duration +} + +// ThrottleBacklog creates a Chi-compatible middleware that limits concurrent +// request processing. Unlike Chi's ThrottleBacklog, it buffers the handler's +// response while holding the token, releases it, then flushes the buffer to +// the client with a write deadline. This prevents slow clients from holding +// throttle capacity. +// +// Because it buffers the entire response in memory, this middleware should only +// be used for endpoints that return small responses (e.g., artwork images). Do +// not use it for audio streaming or download endpoints. +func ThrottleBacklog(limit, backlogLimit int, backlogTimeout time.Duration) func(http.Handler) http.Handler { + if limit <= 0 { + return func(next http.Handler) http.Handler { return next } + } + if !conf.Server.DevArtworkThrottleBuffered { + return middleware.ThrottleBacklog(limit, backlogLimit, backlogTimeout) + } + t := &requestThrottle{ + tokens: make(chan struct{}, limit), + backlogTokens: make(chan struct{}, limit+backlogLimit), + backlogTimeout: backlogTimeout, + } + for range limit { + t.tokens <- struct{}{} + } + for range limit + backlogLimit { + t.backlogTokens <- struct{}{} + } + return t.handler +} + +func (t *requestThrottle) handler(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + + release, err := t.acquire(ctx) + if err != nil { + switch { + case errors.Is(err, ErrThrottleCapacityExceeded): + log.Warn(ctx, "Request throttle capacity exceeded", "path", r.URL.Path) + case errors.Is(err, ErrThrottleTimeout): + log.Warn(ctx, "Request throttle backlog timeout", "path", r.URL.Path) + } + http.Error(w, http.StatusText(http.StatusTooManyRequests), http.StatusTooManyRequests) + return + } + + buf := &bufferedResponseWriter{header: make(http.Header)} + func() { + defer release() + next.ServeHTTP(buf, r) + }() + + for k, v := range buf.header { + w.Header()[k] = v + } + if buf.code > 0 { + w.WriteHeader(buf.code) + } + if _, err := w.Write(buf.body.Bytes()); err != nil { + log.Warn(ctx, "Error writing throttled response", err) + } + }) +} + +func (t *requestThrottle) acquire(ctx context.Context) (release func(), err error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + case <-t.backlogTokens: + default: + return nil, ErrThrottleCapacityExceeded + } + + select { + case <-t.tokens: + return t.releaseFunc(), nil + default: + } + + timer := time.NewTimer(t.backlogTimeout) + select { + case <-timer.C: + t.backlogTokens <- struct{}{} + return nil, ErrThrottleTimeout + case <-ctx.Done(): + timer.Stop() + t.backlogTokens <- struct{}{} + return nil, ctx.Err() + case <-t.tokens: + timer.Stop() + return t.releaseFunc(), nil + } +} + +func (t *requestThrottle) releaseFunc() func() { + var once sync.Once + return func() { + once.Do(func() { + t.tokens <- struct{}{} + t.backlogTokens <- struct{}{} + }) + } +} + +type bufferedResponseWriter struct { + header http.Header + body bytes.Buffer + code int +} + +func (w *bufferedResponseWriter) Header() http.Header { + return w.header +} + +func (w *bufferedResponseWriter) Write(b []byte) (int, error) { + return w.body.Write(b) +} + +func (w *bufferedResponseWriter) WriteHeader(code int) { + if w.code != 0 { + return + } + w.code = code +} diff --git a/server/throttle_backlog_test.go b/server/throttle_backlog_test.go new file mode 100644 index 000000000..4d8567db5 --- /dev/null +++ b/server/throttle_backlog_test.go @@ -0,0 +1,266 @@ +package server + +import ( + "bytes" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "time" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ThrottleBacklog", func() { + It("is a passthrough when limit is 0", func() { + m := ThrottleBacklog(0, 10, time.Second) + r := chi.NewRouter() + r.Use(m) + r.Get("/test", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("ok")) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + r.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Body.String()).To(Equal("ok")) + }) + + It("returns 429 when capacity is exceeded", func() { + _, secondStatus := runTwoRequests(ThrottleBacklog(1, 0, time.Second)) + Expect(secondStatus).To(Equal(http.StatusTooManyRequests)) + }) + + It("returns 429 when backlog times out", func() { + _, secondStatus := runTwoRequests(ThrottleBacklog(1, 1, 50*time.Millisecond)) + Expect(secondStatus).To(Equal(http.StatusTooManyRequests)) + }) + + It("releases capacity when the handler panics", func() { + m := ThrottleBacklog(1, 0, time.Second) + r := chi.NewRouter() + r.Use(middleware.Recoverer) + r.Use(m) + r.Get("/panic", func(w http.ResponseWriter, r *http.Request) { + panic("boom") + }) + r.Get("/test", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("ok")) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/panic", nil) + r.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusInternalServerError)) + + w = httptest.NewRecorder() + req, _ = http.NewRequest("GET", "/test", nil) + r.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Body.String()).To(Equal("ok")) + }) + + It("preserves response headers and status code", func() { + m := ThrottleBacklog(2, 0, time.Second) + r := chi.NewRouter() + r.Use(m) + r.Get("/test", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "image/jpeg") + w.Header().Set("Cache-Control", "public") + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte("body")) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + r.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusCreated)) + Expect(w.Header().Get("Content-Type")).To(Equal("image/jpeg")) + Expect(w.Header().Get("Cache-Control")).To(Equal("public")) + Expect(w.Body.String()).To(Equal("body")) + }) + + It("uses the first response status code", func() { + m := ThrottleBacklog(2, 0, time.Second) + r := chi.NewRouter() + r.Use(m) + r.Get("/test", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte("body")) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + r.ServeHTTP(w, req) + + Expect(w.Code).To(Equal(http.StatusCreated)) + Expect(w.Body.String()).To(Equal("body")) + }) + + It("never exceeds the concurrency limit", func() { + const limit = 3 + const goroutines = 20 + m := ThrottleBacklog(limit, goroutines, 5*time.Second) + + var concurrent atomic.Int32 + var maxConcurrent atomic.Int32 + + r := chi.NewRouter() + r.Use(m) + r.Get("/test", func(w http.ResponseWriter, r *http.Request) { + cur := concurrent.Add(1) + for { + old := maxConcurrent.Load() + if cur <= old || maxConcurrent.CompareAndSwap(old, cur) { + break + } + } + time.Sleep(5 * time.Millisecond) + concurrent.Add(-1) + _, _ = w.Write([]byte("ok")) + }) + + var wg sync.WaitGroup + for range goroutines { + wg.Go(func() { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + r.ServeHTTP(w, req) + }) + } + + wg.Wait() + Expect(maxConcurrent.Load()).To(BeNumerically("<=", limit)) + }) + + // Regression: with only 1 token, a slow client blocking during response + // writing must NOT prevent other requests from being served. Chi's original + // ThrottleBacklog holds the token for the entire handler lifecycle including + // io.Copy, causing starvation. The buffered implementation releases it first. + Context("when a client is slow to read the response", func() { + slowClientTest := func(m func(http.Handler) http.Handler) (*chi.Mux, chan struct{}, chan struct{}) { + handlerReached := make(chan struct{}, 1) + router := chi.NewRouter() + router.Use(m) + router.Get("/test", func(w http.ResponseWriter, r *http.Request) { + select { + case handlerReached <- struct{}{}: + default: + } + _, _ = io.Copy(w, strings.NewReader("image data")) + }) + + unblocked := make(chan struct{}) + slow := newSlowTestWriter(unblocked) + + reqDone := make(chan struct{}) + go func() { + defer close(reqDone) + req, _ := http.NewRequest("GET", "/test", nil) + router.ServeHTTP(slow, req) + }() + <-handlerReached + + return router, unblocked, reqDone + } + + It("does not starve concurrent requests with buffered middleware", func() { + router, unblocked, reqDone := slowClientTest(ThrottleBacklog(1, 1, 500*time.Millisecond)) + + Eventually(func() int { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + router.ServeHTTP(w, req) + return w.Code + }, 2*time.Second, 10*time.Millisecond).Should(Equal(http.StatusOK)) + + close(unblocked) + Eventually(reqDone, 2*time.Second).Should(BeClosed()) + }) + + It("starves concurrent requests with Chi's original middleware", func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.DevArtworkThrottleBuffered = false + + router, unblocked, reqDone := slowClientTest(ThrottleBacklog(1, 1, 500*time.Millisecond)) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + router.ServeHTTP(w, req) + Expect(w.Code).To(Equal(http.StatusTooManyRequests)) + + close(unblocked) + Eventually(reqDone, 2*time.Second).Should(BeClosed()) + }) + }) +}) + +// runTwoRequests sends two concurrent requests through a throttled router. The +// first request holds the token until the second has been dispatched. +func runTwoRequests(m func(http.Handler) http.Handler) (firstStatus, secondStatus int) { + held := make(chan struct{}, 1) + release := make(chan struct{}) + r := chi.NewRouter() + r.Use(m) + r.Get("/test", func(w http.ResponseWriter, r *http.Request) { + select { + case held <- struct{}{}: + default: + } + <-release + _, _ = w.Write([]byte("ok")) + }) + + done := make(chan int) + go func() { + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + r.ServeHTTP(w, req) + done <- w.Code + }() + <-held + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/test", nil) + r.ServeHTTP(w, req) + secondStatus = w.Code + + close(release) + firstStatus = <-done + return firstStatus, secondStatus +} + +// slowTestWriter implements http.ResponseWriter without embedding +// httptest.ResponseRecorder. This is necessary because ResponseRecorder +// promotes io.ReaderFrom, which io.Copy prefers over Write — bypassing +// our blocking Write and defeating the slow-client simulation. +type slowTestWriter struct { + header http.Header + body bytes.Buffer + code int + unblocked chan struct{} +} + +func newSlowTestWriter(unblocked chan struct{}) *slowTestWriter { + return &slowTestWriter{header: make(http.Header), unblocked: unblocked} +} + +func (w *slowTestWriter) Header() http.Header { return w.header } + +func (w *slowTestWriter) WriteHeader(code int) { w.code = code } + +func (w *slowTestWriter) Write(p []byte) (int, error) { + <-w.unblocked + return w.body.Write(p) +} diff --git a/ui/src/actions/serverEvents.js b/ui/src/actions/serverEvents.js index 995534550..892f2e27e 100644 --- a/ui/src/actions/serverEvents.js +++ b/ui/src/actions/serverEvents.js @@ -2,6 +2,7 @@ export const EVENT_SCAN_STATUS = 'scanStatus' export const EVENT_SERVER_START = 'serverStart' export const EVENT_REFRESH_RESOURCE = 'refreshResource' export const EVENT_NOW_PLAYING_COUNT = 'nowPlayingCount' +export const EVENT_NOW_PLAYING_COUNT_SYNC = 'nowPlayingCountSync' export const EVENT_STREAM_RECONNECTED = 'streamReconnected' export const processEvent = (type, data) => ({ @@ -18,6 +19,11 @@ export const nowPlayingCountUpdate = (data) => ({ data: data, }) +export const nowPlayingCountSync = (data) => ({ + type: EVENT_NOW_PLAYING_COUNT_SYNC, + data: data, +}) + export const serverDown = () => ({ type: EVENT_SERVER_START, data: {}, diff --git a/ui/src/album/AlbumGridView.jsx b/ui/src/album/AlbumGridView.jsx index 9717618fa..33db9e40f 100644 --- a/ui/src/album/AlbumGridView.jsx +++ b/ui/src/album/AlbumGridView.jsx @@ -34,6 +34,7 @@ const useStyles = makeStyles( tileBar: { transition: 'all 150ms ease-out', opacity: 0, + pointerEvents: 'none', textAlign: 'left', background: 'linear-gradient(to top, rgba(0,0,0,0.7) 0%,rgba(0,0,0,0.4) 70%,rgba(0,0,0,0) 100%)', @@ -78,8 +79,9 @@ const useStyles = makeStyles( position: 'relative', display: 'block', textDecoration: 'none', - '&:hover $tileBar': { + '&:hover $tileBar, &:focus-within $tileBar': { opacity: 1, + pointerEvents: 'auto', }, }, albumLink: { diff --git a/ui/src/audioplayer/Player.jsx b/ui/src/audioplayer/Player.jsx index 3603e263e..f1538c1d6 100644 --- a/ui/src/audioplayer/Player.jsx +++ b/ui/src/audioplayer/Player.jsx @@ -1,4 +1,5 @@ import React, { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { useInterval } from '../common' import { useDispatch, useSelector } from 'react-redux' import { useMediaQuery } from '@material-ui/core' import { ThemeProvider } from '@material-ui/core/styles' @@ -66,9 +67,11 @@ const Player = () => { const dataProvider = useDataProvider() const playerState = useSelector((state) => state.player) const dispatch = useDispatch() - const [startTime, setStartTime] = useState(null) - const [scrobbled, setScrobbled] = useState(false) - const [preloaded, setPreload] = useState(false) + const [currentTrackId, setCurrentTrackId] = useState(null) + const [heartbeatTrackId, setHeartbeatTrackId] = useState(null) + const lastPositionMsRef = useRef(0) + const currentTrackIdRef = useRef(null) + const stoppedRef = useRef(false) const [audioInstance, setAudioInstance] = useState(null) const isDesktop = useMediaQuery('(min-width:810px)') const isMobilePlayer = @@ -83,6 +86,21 @@ const Player = () => { const playerStateRef = useRef(playerState) playerStateRef.current = playerState + currentTrackIdRef.current = currentTrackId + + useInterval( + () => { + if (heartbeatTrackId && !stoppedRef.current) { + subsonic.reportPlayback( + heartbeatTrackId, + lastPositionMsRef.current, + 'playing', + ) + } + }, + heartbeatTrackId ? config.playbackReportIntervalMs : null, + ) + // Detect browser codec profile and eagerly resolve transcode URLs for the // persisted queue once on mount (e.g. after a browser refresh) useEffect(() => { @@ -257,15 +275,33 @@ const Player = () => { useEffect(() => { const handleBeforeUnload = (e) => { - // Check there's a current track and is actually playing/not paused if (playerState.current?.uuid && audioInstance && !audioInstance.paused) { e.preventDefault() - e.returnValue = '' // Chrome requires returnValue to be set + e.returnValue = '' + } + } + + const handlePageHide = () => { + if (currentTrackIdRef.current && !playerState.current?.isRadio) { + stoppedRef.current = true + try { + subsonic.reportPlaybackKeepalive( + currentTrackIdRef.current, + lastPositionMsRef.current, + 'stopped', + ) + } catch { + // fetch/sendBeacon may throw; ignore + } } } window.addEventListener('beforeunload', handleBeforeUnload) - return () => window.removeEventListener('beforeunload', handleBeforeUnload) + window.addEventListener('pagehide', handlePageHide) + return () => { + window.removeEventListener('beforeunload', handleBeforeUnload) + window.removeEventListener('pagehide', handlePageHide) + } }, [playerState, audioInstance]) useEffect(() => { @@ -425,44 +461,25 @@ const Player = () => { [dispatch], ) - const nextSong = useCallback(() => { - const idx = playerState.queue.findIndex( - (item) => item.uuid === playerState.current.uuid, - ) - return idx !== null ? playerState.queue[idx + 1] : null - }, [playerState]) + const onAudioProgress = useCallback((info) => { + if (info.ended) { + document.title = 'Navidrome' + } + if (!info.isRadio && info.currentTime != null) { + lastPositionMsRef.current = Math.floor(info.currentTime * 1000) + } + }, []) - const onAudioProgress = useCallback( + const onAudioSeeked = useCallback( (info) => { - if (info.ended) { - document.title = 'Navidrome' - } - - const progress = (info.currentTime / info.duration) * 100 - if (isNaN(info.duration) || (progress < 50 && info.currentTime < 240)) { - return - } - - if (info.isRadio) { - return - } - - if (!preloaded) { - const next = nextSong() - if (next != null && !next.isRadio) { - // Trigger decision pre-fetch (this also warms the cache) - decisionService.prefetchDecisions([next.trackId]) - } - setPreload(true) - return - } - - if (!scrobbled) { - info.trackId && subsonic.scrobble(info.trackId, startTime) - setScrobbled(true) + if (!info.isRadio && currentTrackId) { + const posMs = Math.floor(info.currentTime * 1000) + lastPositionMsRef.current = posMs + const state = audioInstance?.paused ? 'paused' : 'playing' + subsonic.reportPlayback(currentTrackId, posMs, state) } }, - [startTime, scrobbled, nextSong, preloaded], + [currentTrackId, audioInstance], ) const onAudioVolumeChange = useCallback( @@ -473,24 +490,30 @@ const Player = () => { const onAudioPlay = useCallback( (info) => { - // Do this to start the context; on chrome-based browsers, the context - // will start paused since it is created prior to user interaction if (context && context.state !== 'running') { context.resume() } dispatch(currentPlaying(info)) - if (startTime === null) { - setStartTime(Date.now()) - } if (info.duration) { const song = info.song document.title = `${song.title} - ${song.artist} - Navidrome` if (!info.isRadio) { - const pos = startTime === null ? null : Math.floor(info.currentTime) - subsonic.nowPlaying(info.trackId, pos) + const posMs = Math.floor(info.currentTime * 1000) + lastPositionMsRef.current = posMs + const isNewTrack = info.trackId !== currentTrackId + if (isNewTrack) { + subsonic + .reportPlayback(info.trackId, posMs, 'starting') + .then(() => + subsonic.reportPlayback(info.trackId, posMs, 'playing'), + ) + setCurrentTrackId(info.trackId) + } else { + subsonic.reportPlayback(info.trackId, posMs, 'playing') + } + setHeartbeatTrackId(info.trackId) } - setPreload(false) if (config.gaTrackingId) { ReactGA.event({ category: 'Player', @@ -507,34 +530,49 @@ const Player = () => { } } }, - [context, dispatch, showNotifications, startTime], + [context, dispatch, showNotifications, currentTrackId], ) const onAudioPlayTrackChange = useCallback(() => { - if (scrobbled) { - setScrobbled(false) + if (currentTrackId) { + subsonic.reportPlayback( + currentTrackId, + lastPositionMsRef.current, + 'stopped', + ) } - if (startTime !== null) { - setStartTime(null) - } - }, [scrobbled, startTime]) + setHeartbeatTrackId(null) + setCurrentTrackId(null) + }, [currentTrackId]) const onAudioPause = useCallback( - (info) => dispatch(currentPlaying(info)), - [dispatch], + (info) => { + dispatch(currentPlaying(info)) + if (!info.isRadio && currentTrackId) { + const posMs = Math.floor(info.currentTime * 1000) + lastPositionMsRef.current = posMs + subsonic.reportPlayback(currentTrackId, posMs, 'paused') + } + setHeartbeatTrackId(null) + }, + [dispatch, currentTrackId], ) const onAudioEnded = useCallback( (currentPlayId, audioLists, info) => { - setScrobbled(false) - setStartTime(null) + if (currentTrackId && !info.isRadio) { + const posMs = Math.floor((info.duration || 0) * 1000) + subsonic.reportPlayback(currentTrackId, posMs, 'stopped') + } + setHeartbeatTrackId(null) + setCurrentTrackId(null) dispatch(currentPlaying(info)) dataProvider .getOne('keepalive', { id: info.trackId }) // eslint-disable-next-line no-console .catch((e) => console.log('Keepalive error:', e)) }, - [dispatch, dataProvider], + [dispatch, dataProvider, currentTrackId], ) const onCoverClick = useCallback((mode, audioLists, audioInfo) => { @@ -570,10 +608,19 @@ const Player = () => { const onBeforeDestroy = useCallback(() => { return new Promise((resolve, reject) => { + if (currentTrackId && !playerStateRef.current?.current?.isRadio) { + subsonic.reportPlayback( + currentTrackId, + lastPositionMsRef.current, + 'stopped', + ) + } + setHeartbeatTrackId(null) + setCurrentTrackId(null) dispatch(clearQueue()) reject() }) - }, [dispatch]) + }, [dispatch, currentTrackId]) if (!visible) { document.title = 'Navidrome' @@ -599,6 +646,7 @@ const Player = () => { onAudioListsChange={onAudioListsChange} onAudioVolumeChange={onAudioVolumeChange} onAudioProgress={onAudioProgress} + onAudioSeeked={onAudioSeeked} onAudioPlay={onAudioPlay} onAudioPlayTrackChange={onAudioPlayTrackChange} onAudioPause={onAudioPause} diff --git a/ui/src/config.js b/ui/src/config.js index f4d60dfba..39f0cd467 100644 --- a/ui/src/config.js +++ b/ui/src/config.js @@ -33,6 +33,7 @@ const defaultConfig = { enableExternalServices: true, enableCoverAnimation: true, enableNowPlaying: true, + playbackReportIntervalMs: 60000, devShowArtistPage: true, devUIShowConfig: true, devNewEventStream: false, diff --git a/ui/src/dataProvider/httpClient.js b/ui/src/dataProvider/httpClient.js index a8897aef8..3820f658d 100644 --- a/ui/src/dataProvider/httpClient.js +++ b/ui/src/dataProvider/httpClient.js @@ -6,8 +6,8 @@ import { jwtDecode } from 'jwt-decode' import { removeHomeCache } from '../utils/removeHomeCache' const customAuthorizationHeader = 'X-ND-Authorization' -const clientUniqueIdHeader = 'X-ND-Client-Unique-Id' -const clientUniqueId = uuidv4() +export const clientUniqueIdHeader = 'X-ND-Client-Unique-Id' +export const clientUniqueId = uuidv4() const httpClient = (url, options = {}) => { url = baseUrl(url) diff --git a/ui/src/dataProvider/index.js b/ui/src/dataProvider/index.js index e9ceea761..059023378 100644 --- a/ui/src/dataProvider/index.js +++ b/ui/src/dataProvider/index.js @@ -1,6 +1,6 @@ -import httpClient from './httpClient' +import httpClient, { clientUniqueId, clientUniqueIdHeader } from './httpClient' import wrapperDataProvider from './wrapperDataProvider' -export { httpClient } +export { httpClient, clientUniqueId, clientUniqueIdHeader } export default wrapperDataProvider diff --git a/ui/src/layout/NowPlayingPanel.jsx b/ui/src/layout/NowPlayingPanel.jsx index 4aaee1bee..509263b42 100644 --- a/ui/src/layout/NowPlayingPanel.jsx +++ b/ui/src/layout/NowPlayingPanel.jsx @@ -1,4 +1,4 @@ -import React, { useState, useEffect, useCallback } from 'react' +import React, { useState, useEffect, useCallback, useRef } from 'react' import PropTypes from 'prop-types' import { useSelector, useDispatch } from 'react-redux' import { useTranslate, Link, useNotify } from 'react-admin' @@ -9,30 +9,29 @@ import { Tooltip, List, ListItem, - ListItemText, - ListItemAvatar, Avatar, Badge, Card, CardContent, Typography, + LinearProgress, useTheme, useMediaQuery, } from '@material-ui/core' -import { FaRegCirclePlay } from 'react-icons/fa6' +import { FaRegCirclePlay, FaPause } from 'react-icons/fa6' import subsonic from '../subsonic' import { useInterval } from '../common' -import { nowPlayingCountUpdate } from '../actions' +import { nowPlayingCountSync } from '../actions' +import { formatDuration } from '../utils' import config from '../config' const useStyles = makeStyles((theme) => ({ button: { color: 'inherit' }, list: { - width: '30em', + width: '26em', maxHeight: (props) => { - // Calculate height for up to 4 entries before scrolling - const entryHeight = 80 - const maxEntries = Math.min(props.entryCount || 0, 4) + const entryHeight = 120 + const maxEntries = Math.min(props.entryCount || 0, 3) return maxEntries > 0 ? `${maxEntries * entryHeight}px` : '12em' }, overflowY: 'auto', @@ -42,42 +41,111 @@ const useStyles = makeStyles((theme) => ({ padding: 0, }, cardContent: { - padding: `${theme.spacing(1)}px !important`, // Minimal padding, override default + padding: `${theme.spacing(1)}px !important`, '&:last-child': { - paddingBottom: `${theme.spacing(1)}px !important`, // Override Material-UI's last-child padding + paddingBottom: `${theme.spacing(1)}px !important`, }, }, listItem: { - paddingTop: theme.spacing(0.5), - paddingBottom: theme.spacing(0.5), - paddingLeft: theme.spacing(1), - paddingRight: theme.spacing(1), + display: 'flex', + alignItems: 'flex-start', + gap: theme.spacing(1.5), + padding: theme.spacing(1), + }, + avatarContainer: { + position: 'relative', + flexShrink: 0, + width: theme.spacing(8), + height: theme.spacing(8), }, avatar: { - width: theme.spacing(6), - height: theme.spacing(6), + width: '100%', + height: '100%', cursor: 'pointer', + borderRadius: theme.spacing(0.5), '&:hover': { opacity: 0.8, }, }, + stateOverlay: { + position: 'absolute', + top: 0, + left: 0, + width: '100%', + height: '100%', + display: 'flex', + alignItems: 'center', + justifyContent: 'center', + backgroundColor: 'rgba(0, 0, 0, 0.45)', + borderRadius: theme.spacing(0.5), + pointerEvents: 'none', + }, + stateIcon: { + color: 'rgba(255, 255, 255, 0.85)', + fontSize: 18, + }, + entryContent: { + flex: 1, + minWidth: 0, + display: 'flex', + flexDirection: 'column', + gap: theme.spacing(0.25), + }, + trackTitle: { + fontWeight: 600, + fontSize: '0.875rem', + lineHeight: 1.3, + overflow: 'hidden', + textOverflow: 'ellipsis', + whiteSpace: 'nowrap', + }, + trackDetail: { + fontSize: '0.75rem', + color: theme.palette.text.secondary, + overflow: 'hidden', + textOverflow: 'ellipsis', + whiteSpace: 'nowrap', + }, + artistLink: { + cursor: 'pointer', + color: theme.palette.text.secondary, + fontSize: '0.75rem', + '&:hover': { + textDecoration: 'underline', + }, + }, + progressRow: { + display: 'flex', + alignItems: 'center', + gap: theme.spacing(0.75), + marginTop: theme.spacing(0.5), + }, + progressTime: { + fontSize: '0.65rem', + color: theme.palette.text.secondary, + fontVariantNumeric: 'tabular-nums', + flexShrink: 0, + }, + progressBar: { + flex: 1, + height: 3, + borderRadius: 2, + backgroundColor: theme.palette.action.disabledBackground, + '& .MuiLinearProgress-bar': { + borderRadius: 2, + }, + }, + userInfo: { + fontSize: '0.65rem', + color: theme.palette.text.disabled, + marginTop: theme.spacing(0.25), + }, badge: { '& .MuiBadge-badge': { backgroundColor: theme.palette.primary.main, color: theme.palette.primary.contrastText, }, }, - artistLink: { - cursor: 'pointer', - '&:hover': { - textDecoration: 'underline', - }, - }, - primaryText: { - display: 'flex', - alignItems: 'center', - flexWrap: 'wrap', - }, })) // NowPlayingButton component - handles the button with badge @@ -113,15 +181,32 @@ NowPlayingButton.propTypes = { onClick: PropTypes.func.isRequired, } -// NowPlayingItem component - individual list item const NowPlayingItem = React.memo( - ({ nowPlayingEntry, onLinkClick, getArtistLink }) => { + ({ nowPlayingEntry, onLinkClick, getArtistLink, now }) => { const classes = useStyles() - const translate = useTranslate() + const isPaused = nowPlayingEntry.state === 'paused' + const isPlaying = + nowPlayingEntry.state === 'playing' || + nowPlayingEntry.state === 'starting' + const basePositionMs = nowPlayingEntry.positionMs || 0 + const rate = nowPlayingEntry.playbackRate || 1 + const elapsedSinceFetch = now - (nowPlayingEntry._fetchedAt || now) + const interpolatedMs = isPlaying + ? basePositionMs + elapsedSinceFetch * rate + : basePositionMs + const durationMs = (nowPlayingEntry.duration || 0) * 1000 + const clampedMs = Math.max(0, interpolatedMs) + const positionMs = + durationMs > 0 ? Math.min(clampedMs, durationMs) : clampedMs + const positionSec = positionMs / 1000 + const durationSec = nowPlayingEntry.duration || 0 + const progress = durationSec > 0 ? (positionSec / durationSec) * 100 : 0 + const artistId = nowPlayingEntry.albumArtistId || nowPlayingEntry.artistId + const artistName = nowPlayingEntry.albumArtist || nowPlayingEntry.artist return ( - - + +
- - - {nowPlayingEntry.albumArtistId || nowPlayingEntry.artistId ? ( - - {nowPlayingEntry.albumArtist || nowPlayingEntry.artist} - - ) : ( - - {nowPlayingEntry.albumArtist || nowPlayingEntry.artist} - - )} -  - {nowPlayingEntry.title} + {isPaused && ( +
+
- } - secondary={`${nowPlayingEntry.username}${nowPlayingEntry.playerName ? ` (${nowPlayingEntry.playerName})` : ''} • ${translate('nowPlaying.minutesAgo', { smart_count: nowPlayingEntry.minutesAgo })}`} - /> + )} +
+
+ + {nowPlayingEntry.title} + + {artistId ? ( + + {artistName} + + ) : ( + + {artistName} + + )} + + {nowPlayingEntry.album} + +
+ + {formatDuration(positionSec)} + + + + {formatDuration(durationSec)} + +
+ + {nowPlayingEntry.username} + {nowPlayingEntry.playerName + ? ` (${nowPlayingEntry.playerName})` + : ''} + +
) }, @@ -178,16 +291,19 @@ NowPlayingItem.propTypes = { title: PropTypes.string.isRequired, username: PropTypes.string.isRequired, playerName: PropTypes.string, - minutesAgo: PropTypes.number.isRequired, album: PropTypes.string, + state: PropTypes.string, + positionMs: PropTypes.number, + duration: PropTypes.number, }).isRequired, onLinkClick: PropTypes.func.isRequired, getArtistLink: PropTypes.func.isRequired, + now: PropTypes.number.isRequired, } // NowPlayingList component - handles the popover content const NowPlayingList = React.memo( - ({ anchorEl, open, onClose, entries, onLinkClick, getArtistLink }) => { + ({ anchorEl, open, onClose, entries, onLinkClick, getArtistLink, now }) => { const classes = useStyles({ entryCount: entries.length }) const translate = useTranslate() @@ -215,10 +331,11 @@ const NowPlayingList = React.memo( > {entries.map((nowPlayingEntry) => ( ))} @@ -239,12 +356,14 @@ NowPlayingList.propTypes = { entries: PropTypes.arrayOf(PropTypes.object).isRequired, onLinkClick: PropTypes.func.isRequired, getArtistLink: PropTypes.func.isRequired, + now: PropTypes.number.isRequired, } // Main NowPlayingPanel component const NowPlayingPanel = () => { const dispatch = useDispatch() const count = useSelector((state) => state.activity.nowPlayingCount) + const lastUpdate = useSelector((state) => state.activity.nowPlayingLastUpdate) const streamReconnected = useSelector( (state) => state.activity.streamReconnected, ) @@ -258,6 +377,7 @@ const NowPlayingPanel = () => { const [anchorEl, setAnchorEl] = useState(null) const [entries, setEntries] = useState([]) + const [now, setNow] = useState(Date.now()) const open = Boolean(anchorEl) const handleMenuOpen = useCallback((event) => { @@ -282,40 +402,57 @@ const NowPlayingPanel = () => { : `/album?filter={"artist_id":"${artistId}"}&order=ASC&sort=max_year&displayedFilters={"compilation":true}&perPage=15` }, []) - const fetchList = useCallback( - () => - subsonic - .getNowPlaying() - .then((resp) => resp.json['subsonic-response']) - .then((data) => { - if (data.status === 'ok') { - const nowPlayingEntries = data.nowPlaying?.entry || [] - setEntries(nowPlayingEntries) - // Also update the count in Redux store - dispatch(nowPlayingCountUpdate({ count: nowPlayingEntries.length })) - } else { - throw new Error( - data.error?.message || 'Failed to fetch now playing data', - ) - } + const fetchTimerRef = useRef(null) + const doFetchRef = useRef() + doFetchRef.current = () => + subsonic + .getNowPlaying() + .then((resp) => resp.json['subsonic-response']) + .then((data) => { + if (data.status === 'ok') { + const nowPlayingEntries = data.nowPlaying?.entry || [] + const fetchTime = Date.now() + setEntries( + nowPlayingEntries.map((e) => ({ ...e, _fetchedAt: fetchTime })), + ) + dispatch(nowPlayingCountSync({ count: nowPlayingEntries.length })) + } else { + throw new Error( + data.error?.message || 'Failed to fetch now playing data', + ) + } + }) + .catch((error) => { + notify('ra.page.error', 'warning', { + messageArgs: { error: error.message || 'Unknown error' }, }) - .catch((error) => { - notify('ra.page.error', 'warning', { - messageArgs: { error: error.message || 'Unknown error' }, - }) - }), - [dispatch, notify], - ) + }) + const fetchList = useCallback(() => { + if (fetchTimerRef.current) clearTimeout(fetchTimerRef.current) + fetchTimerRef.current = setTimeout(() => { + fetchTimerRef.current = null + doFetchRef.current() + }, 300) + }, []) + + useEffect(() => { + return () => { + if (fetchTimerRef.current) clearTimeout(fetchTimerRef.current) + } + }, []) // Initialize count and entries on mount, and refresh on server/stream changes useEffect(() => { if (serverUp) fetchList() }, [fetchList, serverUp, streamReconnected]) - // Refresh when count changes from WebSocket events (if panel is open) + // Refresh when NowPlaying updates from SSE events (if panel is open) useEffect(() => { if (open && serverUp) fetchList() - }, [count, open, fetchList, serverUp]) + }, [lastUpdate, open, fetchList, serverUp]) + + // Update current time every second when open to animate progress bars + useInterval(() => setNow(Date.now()), open ? 1000 : null) // Periodic refresh when panel is open (10 seconds) useInterval( @@ -341,6 +478,7 @@ const NowPlayingPanel = () => { open={open} onClose={handleMenuClose} entries={entries} + now={now} onLinkClick={handleLinkClick} getArtistLink={getArtistLink} /> diff --git a/ui/src/layout/NowPlayingPanel.test.jsx b/ui/src/layout/NowPlayingPanel.test.jsx index 4dd5dac8b..ea4a3568b 100644 --- a/ui/src/layout/NowPlayingPanel.test.jsx +++ b/ui/src/layout/NowPlayingPanel.test.jsx @@ -70,7 +70,12 @@ describe('', () => { ) } + afterEach(() => { + vi.useRealTimers() + }) + beforeEach(() => { + vi.useFakeTimers() vi.clearAllMocks() mockUseMediaQuery.mockReturnValue(false) // Default to large screen @@ -105,10 +110,8 @@ describe('', () => { , ) - // Wait for initial fetch to complete - await waitFor(() => { - expect(subsonic.getNowPlaying).toHaveBeenCalled() - }) + // Advance past debounce and flush promises + await vi.advanceTimersByTimeAsync(500) fireEvent.click(screen.getByRole('button')) await waitFor(() => { @@ -128,21 +131,16 @@ describe('', () => { , ) - // Wait for initial fetch to complete - await waitFor(() => { - expect(subsonic.getNowPlaying).toHaveBeenCalled() - }) + await vi.advanceTimersByTimeAsync(500) fireEvent.click(screen.getByRole('button')) await waitFor(() => { - expect( - screen.getByText('u1 (Chrome Browser) • nowPlaying.minutesAgo'), - ).toBeInTheDocument() + expect(screen.getByText('u1 (Chrome Browser)')).toBeInTheDocument() }) }) it('handles entries without player name', async () => { - subsonic.getNowPlaying.mockResolvedValueOnce({ + subsonic.getNowPlaying.mockResolvedValue({ json: { 'subsonic-response': { status: 'ok', @@ -170,19 +168,16 @@ describe('', () => { , ) - // Wait for initial fetch to complete - await waitFor(() => { - expect(subsonic.getNowPlaying).toHaveBeenCalled() - }) + await vi.advanceTimersByTimeAsync(500) fireEvent.click(screen.getByRole('button')) await waitFor(() => { - expect(screen.getByText('u1 • nowPlaying.minutesAgo')).toBeInTheDocument() + expect(screen.getByText('u1')).toBeInTheDocument() }) }) it('shows empty message when no entries', async () => { - subsonic.getNowPlaying.mockResolvedValueOnce({ + subsonic.getNowPlaying.mockResolvedValue({ json: { 'subsonic-response': { status: 'ok', nowPlaying: { entry: [] } }, }, @@ -194,10 +189,7 @@ describe('', () => { , ) - // Wait for initial fetch - await waitFor(() => { - expect(subsonic.getNowPlaying).toHaveBeenCalled() - }) + await vi.advanceTimersByTimeAsync(500) fireEvent.click(screen.getByRole('button')) await waitFor(() => { @@ -215,10 +207,7 @@ describe('', () => { , ) - // Wait for initial fetch to complete - await waitFor(() => { - expect(subsonic.getNowPlaying).toHaveBeenCalled() - }) + await vi.advanceTimersByTimeAsync(500) // Open the panel fireEvent.click(screen.getByRole('button')) @@ -268,7 +257,9 @@ describe('', () => { expect(subsonic.getNowPlaying).not.toHaveBeenCalled() }) - it('does not double-fetch on server reconnection', () => { + it('does not double-fetch on server reconnection', async () => { + vi.useFakeTimers() + const initialStore = createMockStore({ nowPlayingCount: 1, serverStart: { startTime: null }, // Server initially down @@ -295,8 +286,13 @@ describe('', () => { , ) + // Advance past the debounce window + vi.advanceTimersByTime(500) + // Should only make one call despite both serverUp and streamReconnected changing expect(subsonic.getNowPlaying).toHaveBeenCalledTimes(1) + + vi.useRealTimers() }) it('skips polling when server is down', () => { diff --git a/ui/src/plugin/PluginList.jsx b/ui/src/plugin/PluginList.jsx index 67af85b81..346e1ceae 100644 --- a/ui/src/plugin/PluginList.jsx +++ b/ui/src/plugin/PluginList.jsx @@ -2,6 +2,7 @@ import React, { useMemo, useState, useCallback } from 'react' import { Button, Datagrid, + Empty, TextField, TopToolbar, useNotify, @@ -10,7 +11,13 @@ import { useTranslate, } from 'react-admin' import { makeStyles } from '@material-ui/core/styles' -import { useMediaQuery, Tooltip, Chip, Typography } from '@material-ui/core' +import { + useMediaQuery, + Tooltip, + Chip, + Typography, + Box, +} from '@material-ui/core' import { MdError, MdRefresh } from 'react-icons/md' import { List, DateField, SimpleList, useResourceRefresh } from '../common' import { httpClient } from '../dataProvider' @@ -72,8 +79,7 @@ const ManifestField = ({ source }) => { return {manifest[source] || '-'} } -const PluginListActions = () => { - const translate = useTranslate() +const RescanButton = () => { const notify = useNotify() const refresh = useRefresh() const [loading, setLoading] = useState(false) @@ -92,20 +98,37 @@ const PluginListActions = () => { }) }, [notify, refresh]) + return ( + + ) +} + +const PluginListActions = () => { return ( - + ) } +const PluginEmpty = () => { + return ( + <> + + + + + + ) +} + const PluginList = (props) => { const isXsmall = useMediaQuery((theme) => theme.breakpoints.down('xs')) const translate = useTranslate() @@ -118,6 +141,7 @@ const PluginList = (props) => { exporter={false} bulkActionButtons={false} actions={} + empty={} > {isXsmall ? ( { TopToolbar: ({ children }) => (
{children}
), + Empty: () =>
No resources
, Datagrid: ({ children }) =>
{children}
, TextField: ({ source }) => , } @@ -42,9 +49,10 @@ vi.mock('react-admin', async () => { // Mock common components vi.mock('../common', async () => { return { - List: ({ children, actions, ...props }) => ( + List: ({ children, actions, empty, ...props }) => (
{actions} + {empty &&
{empty}
} {children}
), @@ -94,14 +102,16 @@ describe('PluginList', () => { expect(screen.getByTestId('datagrid')).toBeInTheDocument() }) - it('renders the rescan button', () => { + it('renders the rescan button in the toolbar', () => { render() - expect(screen.getByTestId('rescan-button')).toBeInTheDocument() + const toolbar = screen.getByTestId('top-toolbar') + expect(within(toolbar).getByTestId('rescan-button')).toBeInTheDocument() }) it('calls rescan endpoint when rescan button is clicked', async () => { render() - const rescanButton = screen.getByTestId('rescan-button') + const toolbar = screen.getByTestId('top-toolbar') + const rescanButton = within(toolbar).getByTestId('rescan-button') fireEvent.click(rescanButton) @@ -114,7 +124,8 @@ describe('PluginList', () => { it('calls refresh after successful rescan', async () => { render() - const rescanButton = screen.getByTestId('rescan-button') + const toolbar = screen.getByTestId('top-toolbar') + const rescanButton = within(toolbar).getByTestId('rescan-button') fireEvent.click(rescanButton) @@ -127,7 +138,8 @@ describe('PluginList', () => { mockHttpClient.mockRejectedValue(new Error('Network error')) render() - const rescanButton = screen.getByTestId('rescan-button') + const toolbar = screen.getByTestId('top-toolbar') + const rescanButton = within(toolbar).getByTestId('rescan-button') fireEvent.click(rescanButton) @@ -137,4 +149,25 @@ describe('PluginList', () => { }) }) }) + + it('renders a rescan button in the empty state', () => { + render() + const emptyState = screen.getByTestId('empty-state') + expect(emptyState).toBeInTheDocument() + expect(within(emptyState).getByTestId('rescan-button')).toBeInTheDocument() + }) + + it('empty state rescan button triggers rescan', async () => { + render() + const emptyState = screen.getByTestId('empty-state') + const rescanButton = within(emptyState).getByTestId('rescan-button') + + fireEvent.click(rescanButton) + + await waitFor(() => { + expect(mockHttpClient).toHaveBeenCalledWith('/api/plugin/rescan', { + method: 'POST', + }) + }) + }) }) diff --git a/ui/src/reducers/activityReducer.js b/ui/src/reducers/activityReducer.js index 8238e395a..d61ed6018 100644 --- a/ui/src/reducers/activityReducer.js +++ b/ui/src/reducers/activityReducer.js @@ -3,6 +3,7 @@ import { EVENT_SCAN_STATUS, EVENT_SERVER_START, EVENT_NOW_PLAYING_COUNT, + EVENT_NOW_PLAYING_COUNT_SYNC, EVENT_STREAM_RECONNECTED, } from '../actions' import config from '../config' @@ -17,6 +18,7 @@ const initialState = { }, serverStart: { version: config.version }, nowPlayingCount: 0, + nowPlayingLastUpdate: 0, streamReconnected: 0, // Timestamp of last reconnection } @@ -45,6 +47,12 @@ export const activityReducer = (previousState = initialState, payload) => { }, } case EVENT_NOW_PLAYING_COUNT: + return { + ...previousState, + nowPlayingCount: data.count, + nowPlayingLastUpdate: Date.now(), + } + case EVENT_NOW_PLAYING_COUNT_SYNC: return { ...previousState, nowPlayingCount: data.count } case EVENT_STREAM_RECONNECTED: return { ...previousState, streamReconnected: Date.now() } diff --git a/ui/src/reducers/activityReducer.test.js b/ui/src/reducers/activityReducer.test.js index c9db38dbb..e52a262c1 100644 --- a/ui/src/reducers/activityReducer.test.js +++ b/ui/src/reducers/activityReducer.test.js @@ -18,6 +18,7 @@ describe('activityReducer', () => { }, serverStart: { version: config.version }, nowPlayingCount: 0, + nowPlayingLastUpdate: 0, streamReconnected: 0, } @@ -133,6 +134,22 @@ describe('activityReducer', () => { expect(newState.nowPlayingCount).toEqual(5) }) + it('handles EVENT_NOW_PLAYING_COUNT with nowPlayingLastUpdate', () => { + const action = { + type: EVENT_NOW_PLAYING_COUNT, + data: { count: 3 }, + } + const beforeTimestamp = Date.now() + const newState = activityReducer(initialState, action) + const afterTimestamp = Date.now() + + expect(newState.nowPlayingCount).toEqual(3) + expect(newState.nowPlayingLastUpdate).toBeGreaterThanOrEqual( + beforeTimestamp, + ) + expect(newState.nowPlayingLastUpdate).toBeLessThanOrEqual(afterTimestamp) + }) + it('handles EVENT_STREAM_RECONNECTED', () => { const action = { type: EVENT_STREAM_RECONNECTED, diff --git a/ui/src/reducers/playerReducer.js b/ui/src/reducers/playerReducer.js index 2c3b2ba7a..449dcd294 100644 --- a/ui/src/reducers/playerReducer.js +++ b/ui/src/reducers/playerReducer.js @@ -169,13 +169,15 @@ const reduceSetVolume = (state, { data: { volume } }) => { } const reduceSyncQueue = (state, { data: { audioInfo, audioLists } }) => { - // Only keep clear and playIndex alive when there is an actual pending - // track switch (playIndex differs from savedPlayIndex). This lets - // PLAYER_PLAY_TRACKS selections survive the sync, while allowing - // PLAYER_PLAY_NEXT (which sets playIndex to the current track) to - // reset immediately and avoid restarting playback. + // Keep clear and playIndex alive when there is a pending track switch. + // A switch is pending when playIndex is set AND either: + // - playIndex differs from savedPlayIndex, OR + // - clear is true (a new queue was loaded, e.g. after clearQueue + playTracks) + // The clear check handles the edge case where both playIndex and + // savedPlayIndex are 0 (close player then play a new album from track 1). const hasPendingSwitch = - state.playIndex != null && state.playIndex !== state.savedPlayIndex + state.playIndex != null && + (state.clear || state.playIndex !== state.savedPlayIndex) return { ...state, queue: audioLists, diff --git a/ui/src/reducers/playerReducer.test.js b/ui/src/reducers/playerReducer.test.js index 1c399859d..43f24ec55 100644 --- a/ui/src/reducers/playerReducer.test.js +++ b/ui/src/reducers/playerReducer.test.js @@ -106,6 +106,88 @@ describe('playerReducer', () => { }) }) + describe('play new album after closing player (issue #5440)', () => { + it('SYNC_QUEUE preserves pending playIndex=0 after clearQueue', () => { + // Scenario: user plays album A, advances to track 3, closes player, + // then plays album B. After clearQueue, savedPlayIndex=0. + // PLAYER_PLAY_TRACKS sets playIndex=0. SYNC_QUEUE must NOT clear it. + const stateAfterClearThenPlay = { + queue: [ + { trackId: 'b1', uuid: 'u1', name: 'B Song 1' }, + { trackId: 'b2', uuid: 'u2', name: 'B Song 2' }, + { trackId: 'b3', uuid: 'u3', name: 'B Song 3' }, + ], + current: {}, + playIndex: 0, + savedPlayIndex: 0, // reset by clearQueue + clear: true, + volume: 1, + } + + const action = { + type: PLAYER_SYNC_QUEUE, + data: { + audioInfo: {}, + audioLists: stateAfterClearThenPlay.queue, + }, + } + const result = playerReducer(stateAfterClearThenPlay, action) + expect(result.playIndex).toBe(0) + expect(result.clear).toBe(true) + }) + + it('CURRENT for wrong track preserves pending playIndex=0 after clearQueue', () => { + // The music player fires onAudioPlay for the old track (at index 3) + // before switching to the new track at index 0. + const stateAfterClearThenPlay = { + queue: [ + { trackId: 'b1', uuid: 'u1', name: 'B Song 1' }, + { trackId: 'b2', uuid: 'u2', name: 'B Song 2' }, + { trackId: 'b3', uuid: 'u3', name: 'B Song 3' }, + { trackId: 'b4', uuid: 'u4', name: 'B Song 4' }, + ], + current: {}, + playIndex: 0, + savedPlayIndex: 0, + clear: true, + volume: 1, + } + + // Player reports track at index 3 as current (stale callback) + const action = { + type: PLAYER_CURRENT, + data: { uuid: 'u4', name: 'B Song 4', volume: 1 }, + } + const result = playerReducer(stateAfterClearThenPlay, action) + expect(result.playIndex).toBe(0) + expect(result.clear).toBe(true) + }) + + it('CURRENT for correct track consumes pending playIndex=0', () => { + const stateAfterClearThenPlay = { + queue: [ + { trackId: 'b1', uuid: 'u1', name: 'B Song 1' }, + { trackId: 'b2', uuid: 'u2', name: 'B Song 2' }, + ], + current: {}, + playIndex: 0, + savedPlayIndex: 0, + clear: true, + volume: 1, + } + + // Player confirms it switched to track at index 0 + const action = { + type: PLAYER_CURRENT, + data: { uuid: 'u1', name: 'B Song 1', volume: 1 }, + } + const result = playerReducer(stateAfterClearThenPlay, action) + expect(result.playIndex).toBeUndefined() + expect(result.clear).toBe(false) + expect(result.savedPlayIndex).toBe(0) + }) + }) + describe('PLAYER_REFRESH_QUEUE', () => { it('clamps negative savedPlayIndex to 0', () => { const state = { diff --git a/ui/src/subsonic/index.js b/ui/src/subsonic/index.js index 47ebabe99..ae9a47a30 100644 --- a/ui/src/subsonic/index.js +++ b/ui/src/subsonic/index.js @@ -1,5 +1,9 @@ -import { httpClient } from '../dataProvider' import { baseUrl } from '../utils' +import { + httpClient, + clientUniqueId, + clientUniqueIdHeader, +} from '../dataProvider' const url = (command, id, options) => { const username = localStorage.getItem('username') @@ -37,16 +41,21 @@ const url = (command, id, options) => { const ping = () => httpClient(url('ping')) -const scrobble = (id, time, submission = true, position = null) => - httpClient( - url('scrobble', id, { - ...(submission && time && { time }), - submission, - ...(!submission && position !== null && { position }), - }), - ) +const reportPlaybackUrl = (mediaId, positionMs, state) => + url('reportPlayback', null, { mediaId, mediaType: 'song', positionMs, state }) -const nowPlaying = (id, position = null) => scrobble(id, null, false, position) +const reportPlayback = (mediaId, positionMs, state) => + httpClient(reportPlaybackUrl(mediaId, positionMs, state)) + +const reportPlaybackKeepalive = (mediaId, positionMs, state) => { + const u = reportPlaybackUrl(mediaId, positionMs, state) + if (u) { + fetch(baseUrl(u), { + keepalive: true, + headers: { [clientUniqueIdHeader]: clientUniqueId }, + }) + } +} const star = (id) => httpClient(url('star', id)) @@ -136,8 +145,8 @@ const streamUrl = (id, options) => { export default { url, ping, - scrobble, - nowPlaying, + reportPlayback, + reportPlaybackKeepalive, download, star, unstar, diff --git a/ui/src/subsonic/index.test.js b/ui/src/subsonic/index.test.js index 6bd5e08ee..b433c3402 100644 --- a/ui/src/subsonic/index.test.js +++ b/ui/src/subsonic/index.test.js @@ -1,13 +1,14 @@ import { vi } from 'vitest' +import config from '../config' import { httpClient } from '../dataProvider' import subsonic from './index' vi.mock('../dataProvider', () => ({ httpClient: vi.fn(() => Promise.resolve({})), + clientUniqueId: 'test-client-id', + clientUniqueIdHeader: 'X-ND-Client-Unique-Id', })) -const COVER_ART_SIZE = 600 - describe('getCoverArtUrl', () => { beforeEach(() => { // Mock window.location @@ -37,7 +38,11 @@ describe('getCoverArtUrl', () => { updatedAt: '2023-01-01T00:00:00Z', } - const url = subsonic.getCoverArtUrl(playlistRecord, COVER_ART_SIZE, true) + const url = subsonic.getCoverArtUrl( + playlistRecord, + config.uiCoverArtSize, + true, + ) expect(url).toContain('pl-playlist-123') expect(url).toContain('size=600') @@ -51,7 +56,11 @@ describe('getCoverArtUrl', () => { sync: true, } - const url = subsonic.getCoverArtUrl(playlistRecord, COVER_ART_SIZE, true) + const url = subsonic.getCoverArtUrl( + playlistRecord, + config.uiCoverArtSize, + true, + ) expect(url).toContain('pl-playlist-123') expect(url).toContain('size=600') @@ -66,7 +75,11 @@ describe('getCoverArtUrl', () => { updatedAt: '2023-01-01T00:00:00Z', } - const url = subsonic.getCoverArtUrl(albumRecord, COVER_ART_SIZE, true) + const url = subsonic.getCoverArtUrl( + albumRecord, + config.uiCoverArtSize, + true, + ) expect(url).toContain('al-album-123') expect(url).toContain('size=600') @@ -80,7 +93,7 @@ describe('getCoverArtUrl', () => { updatedAt: '2023-01-01T00:00:00Z', } - const url = subsonic.getCoverArtUrl(songRecord, COVER_ART_SIZE, true) + const url = subsonic.getCoverArtUrl(songRecord, config.uiCoverArtSize, true) expect(url).toContain('mf-song-123') expect(url).toContain('size=600') @@ -93,7 +106,11 @@ describe('getCoverArtUrl', () => { updatedAt: '2023-01-01T00:00:00Z', } - const url = subsonic.getCoverArtUrl(artistRecord, COVER_ART_SIZE, true) + const url = subsonic.getCoverArtUrl( + artistRecord, + config.uiCoverArtSize, + true, + ) expect(url).toContain('ar-artist-123') expect(url).toContain('size=600') @@ -211,3 +228,33 @@ describe('getLyricsBySongId', () => { expect(calledUrl).toContain('enhanced=true') }) }) + +describe('reportPlayback', () => { + beforeEach(() => { + const localStorageMock = { + getItem: vi.fn((key) => { + const values = { + username: 'testuser', + 'subsonic-token': 'testtoken', + 'subsonic-salt': 'testsalt', + } + return values[key] || null + }), + } + Object.defineProperty(window, 'localStorage', { value: localStorageMock }) + }) + + it('should construct reportPlayback URL with correct parameters', () => { + const url = subsonic.url('reportPlayback', null, { + mediaId: 'song-123', + mediaType: 'song', + positionMs: 5000, + state: 'playing', + }) + expect(url).toContain('reportPlayback') + expect(url).toContain('mediaId=song-123') + expect(url).toContain('mediaType=song') + expect(url).toContain('positionMs=5000') + expect(url).toContain('state=playing') + }) +}) diff --git a/utils/cache/simple_cache.go b/utils/cache/simple_cache.go index cac41be7b..2f0ff4764 100644 --- a/utils/cache/simple_cache.go +++ b/utils/cache/simple_cache.go @@ -17,6 +17,7 @@ type SimpleCache[K comparable, V any] interface { AddWithTTL(key K, value V, ttl time.Duration) error Get(key K) (V, error) GetWithLoader(key K, loader func(key K) (V, time.Duration, error)) (V, error) + Remove(key K) Keys() []K Values() []V Len() int @@ -77,6 +78,10 @@ func (c *simpleCache[K, V]) AddWithTTL(key K, value V, ttl time.Duration) error return nil } +func (c *simpleCache[K, V]) Remove(key K) { + c.data.Delete(key) +} + func (c *simpleCache[K, V]) Get(key K) (V, error) { item := c.data.Get(key) if item == nil { diff --git a/utils/req/req.go b/utils/req/req.go index f9fa5724b..6b6135e1a 100644 --- a/utils/req/req.go +++ b/utils/req/req.go @@ -170,3 +170,15 @@ func (r *Values) BoolOr(param string, def bool) bool { } return v } + +func (r *Values) Float64Or(param string, def float64) float64 { + v, err := r.String(param) + if err != nil { + return def + } + f, err := strconv.ParseFloat(v, 64) + if err != nil { + return def + } + return f +} diff --git a/utils/req/req_test.go b/utils/req/req_test.go index e710365bd..d76b3b934 100644 --- a/utils/req/req_test.go +++ b/utils/req/req_test.go @@ -244,6 +244,23 @@ var _ = Describe("Request Helpers", func() { }) }) + Describe("Float64Or", func() { + It("returns parsed float value", func() { + r := req.Params(httptest.NewRequest("GET", "/test?rate=1.5", nil)) + Expect(r.Float64Or("rate", 1.0)).To(Equal(1.5)) + }) + + It("returns default when param is missing", func() { + r := req.Params(httptest.NewRequest("GET", "/test", nil)) + Expect(r.Float64Or("rate", 1.0)).To(Equal(1.0)) + }) + + It("returns default when param is not a valid float", func() { + r := req.Params(httptest.NewRequest("GET", "/test?rate=abc", nil)) + Expect(r.Float64Or("rate", 1.0)).To(Equal(1.0)) + }) + }) + Describe("ParamBoolPtr", func() { Context("value is true", func() { BeforeEach(func() {