diff --git a/backend/download/candidate_shape_test.go b/backend/download/candidate_shape_test.go new file mode 100644 index 0000000..6b979a0 --- /dev/null +++ b/backend/download/candidate_shape_test.go @@ -0,0 +1,248 @@ +package download + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +// Multi-disc rips, single-track results and coverage counted in tracks +// rather than files (#270). + +func TestParsePathReadsTheDiscFromItsFolder(t *testing.T) { + t.Parallel() + + cases := []struct { + path string + disc int + track int + folder string + }{ + {`\share\Pink Floyd - The Wall (1979)\CD2\03 Hey You.flac`, 2, 3, "The Wall"}, + {`\share\The Wall\Disc 1\01 In The Flesh.flac`, 1, 1, "The Wall"}, + {`\share\The Wall\[Disk-2]\01 Hey You.flac`, 2, 1, "The Wall"}, + {`\share\The Wall\CD1 - Live\04 Mother.flac`, 1, 4, "The Wall"}, + // The filename's own disc number is more specific than the folder. + {`\share\The Wall\CD1\2-05 Comfortably Numb.flac`, 2, 5, "The Wall"}, + // Not a disc folder: a number is required. + {`\share\CDs\The Wall\01 In The Flesh.flac`, 0, 1, "The Wall"}, + } + + for _, tc := range cases { + t.Run(tc.path, func(t *testing.T) { + t.Parallel() + + got := ParsePath(tc.path) + if got.Disc != tc.disc || got.Track != tc.track || got.Folder != tc.folder { + t.Errorf( + "ParsePath = disc %d track %d folder %q, want %d %d %q", + got.Disc, got.Track, got.Folder, tc.disc, tc.track, tc.folder, + ) + } + }) + } +} + +func TestAlbumDir(t *testing.T) { + t.Parallel() + + cases := map[string]string{ + `\share\Album\CD1\01 A.flac`: "/share/Album", + `\share\Album\01 A.flac`: "/share/Album", + `CD1\01 A.flac`: "CD1", + `\share\CD Collection\01.mp3`: "/share/CD Collection", + } + + for in, want := range cases { + if got := AlbumDir(in); got != want { + t.Errorf("AlbumDir(%q) = %q, want %q", in, got, want) + } + } +} + +// One album shared as CD1/CD2 is one candidate, named after the album. +func TestSlskdGroupsDiscFoldersIntoOneCandidate(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + stub.responses = []slskdResponse{{ + Username: "peer", + Files: []slskdFile{ + {Filename: `\m\The Wall\CD1\01 In The Flesh.flac`, Size: 1}, + {Filename: `\m\The Wall\CD1\02 The Thin Ice.flac`, Size: 1}, + {Filename: `\m\The Wall\CD2\01 Hey You.flac`, Size: 1}, + {Filename: `\m\The Wall\CD2\02 Is There Anybody Out There.flac`, Size: 1}, + }, + }} + + s, _ := newStubSlskd(t, stub) + + got, err := s.Search(context.Background(), Download{Query: "the wall"}) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(got) != 1 { + t.Fatalf("got %d candidates, want the two discs as one", len(got)) + } + + if got[0].Title != "The Wall" || len(got[0].Files) != 4 { + t.Errorf( + "candidate = %q with %d files, want \"The Wall\" with 4", + got[0].Title, len(got[0].Files), + ) + } +} + +// A track search matches one file per folder, so a single-track request +// must accept a one-file folder that an album request rightly drops. +func TestSlskdKeepsASingleFileForATrackRequest(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + stub.responses = []slskdResponse{{ + Username: "peer", + Files: []slskdFile{ + {Filename: `\m\OK Computer\02 Paranoid Android.flac`, Size: 1}, + }, + }} + + s, _ := newStubSlskd(t, stub) + + track, err := s.Search(context.Background(), Download{ + RecordingMBID: "rec-1", Artist: "Radiohead", Album: "Paranoid Android", + }) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(track) != 1 { + t.Errorf("track request: got %d candidates, want 1", len(track)) + } + + album, err := s.Search(context.Background(), Download{ + ReleaseMBID: "rel-1", Artist: "Radiohead", Album: "OK Computer", + }) + if err != nil { + t.Fatalf("Search: %v", err) + } + + if len(album) != 0 { + t.Errorf("album request: got %d candidates, want the one-file folder dropped", len(album)) + } +} + +// Two discs with a file of the same name both reach staging, each under +// its disc folder, where the importer reads the disc number from. +func TestSlskdCollectKeepsDiscFolders(t *testing.T) { + t.Parallel() + + stub := newSlskdStub(t) + s, downloads := newStubSlskd(t, stub) + + for _, disc := range []string{"CD1", "CD2"} { + dir := filepath.Join(downloads, disc) + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + + if err := os.WriteFile( + filepath.Join(dir, "01 Intro.flac"), []byte(disc), 0o600, + ); err != nil { + t.Fatalf("write: %v", err) + } + } + + dst := t.TempDir() + + got, err := s.collect(Candidate{Files: []CandidateFile{ + {Path: `\m\Album\CD1\01 Intro.flac`, IsAudio: true}, + {Path: `\m\Album\CD2\01 Intro.flac`, IsAudio: true}, + }}, dst) + if err != nil { + t.Fatalf("collect: %v", err) + } + + if len(got.Files) != 2 { + t.Fatalf("collected %d files, want 2", len(got.Files)) + } + + for _, disc := range []string{"CD1", "CD2"} { + data, err := os.ReadFile(filepath.Join(dst, disc, "01 Intro.flac")) + if err != nil || string(data) != disc { + t.Errorf("%s's file missing or overwritten: %q, %v", disc, data, err) + } + + if hint := ParsePath(filepath.Join(dst, disc, "01 Intro.flac")); hint.Disc == 0 { + t.Errorf("staged %s file lost its disc number", disc) + } + } +} + +// A two-disc release whose discs both number from 01 aligns completely +// once the disc comes from the folder; before, disc 2's 01 collided with +// disc 1's. +func TestMultiDiscCandidateAlignsEveryTrack(t *testing.T) { + t.Parallel() + + dl := Download{ + ReleaseMBID: "the-wall", + Artist: "Pink Floyd", + Album: "The Wall", + Expected: []ExpectedTrack{ + {DiscNumber: 1, Position: 1, Title: "In the Flesh?"}, + {DiscNumber: 1, Position: 2, Title: "The Thin Ice"}, + {DiscNumber: 2, Position: 1, Title: "Hey You"}, + {DiscNumber: 2, Position: 2, Title: "Is There Anybody Out There?"}, + }, + } + + c := Candidate{ + Title: "The Wall", + Files: []CandidateFile{ + {Path: `\m\Pink Floyd - The Wall\CD1\01 In the Flesh.flac`, Size: 1}, + {Path: `\m\Pink Floyd - The Wall\CD1\02 The Thin Ice.flac`, Size: 1}, + {Path: `\m\Pink Floyd - The Wall\CD2\01 Hey You.flac`, Size: 1}, + {Path: `\m\Pink Floyd - The Wall\CD2\02 Is There Anybody Out There.flac`, Size: 1}, + }, + } + + got := Score(dl, c, 50, AutoDownloadPrefs{}) + + if got.Match.Completeness != 1 { + t.Errorf("completeness = %f, want 1", got.Match.Completeness) + } + + if got.Match.AlbumFit < 0.99 { + t.Errorf("album fit = %f, want the album's own name to match", got.Match.AlbumFit) + } + + if got.Match.Overall < minMatch { + t.Errorf("match = %f, want it to clear the auto-pick bar %f", got.Match.Overall, minMatch) + } +} + +// Ten files against a ten-track album is not a complete album when only +// three of them are its tracks. +func TestCompletenessCountsTracksNotFiles(t *testing.T) { + t.Parallel() + + dl := okComputer() + + c := Candidate{Title: "OK Computer", Files: []CandidateFile{ + {Path: `\m\Radiohead - OK Computer\Airbag.flac`, Size: 1}, + {Path: `\m\Radiohead - OK Computer\Paranoid Android.flac`, Size: 1}, + {Path: `\m\Radiohead - OK Computer\Exit Music (For a Film).flac`, Size: 1}, + {Path: `\m\Radiohead - OK Computer\Creep.flac`, Size: 1}, + }} + + got := Score(dl, c, 50, AutoDownloadPrefs{}) + + if got.Match.Completeness > 0.76 { + t.Errorf( + "completeness = %f with 3 of 4 tracks present, want at most 0.75", + got.Match.Completeness, + ) + } +} diff --git a/backend/download/pathmatch.go b/backend/download/pathmatch.go index 78a2249..a9149e6 100644 --- a/backend/download/pathmatch.go +++ b/backend/download/pathmatch.go @@ -66,6 +66,14 @@ var ( // separatorPattern splits "Artist - Album" style folder names. separatorPattern = regexp.MustCompile(`\s+[-–—]\s+`) + + // discFolderPattern matches a directory that holds one disc of an + // album rather than the album: "CD1", "CD 2", "Disc 3", "Disk-1", + // "[Disc 2]", "CD1 - The Early Years". A number is required, so a + // folder merely called "CDs" is not one. + discFolderPattern = regexp.MustCompile( + `(?i)^\s*[\[(]?\s*(?:cd|disc|disk)\s*[-_.#]?\s*(\d{1,2})\b`, + ) ) // FormatForPath returns the audio format implied by a path's extension, @@ -94,16 +102,63 @@ type TrackHint struct { Folder string } +// discFolder reports whether a directory name is one disc of an album, +// and which. +func discFolder(name string) (int, bool) { + m := discFolderPattern.FindStringSubmatch(name) + if m == nil { + return 0, false + } + + n, err := strconv.Atoi(m[1]) + if err != nil || n == 0 { + return 0, false + } + + return n, true +} + +// AlbumDir is the directory that holds a file's *album*: its parent, +// or its grandparent when the parent is a disc folder. +// +// Multi-disc rips are shared as `Album/CD1/…` and `Album/CD2/…`, and +// grouping candidates by the immediate parent split one album into two +// half-albums, each titled "CD1". Neither could clear the completeness +// or album-title bars, so a multi-disc release could not be auto-picked +// at all. A disc folder at the root has no album above it and is +// returned as it is. +func AlbumDir(p string) string { + dir := path.Dir(strings.ReplaceAll(p, `\`, "/")) + + if _, ok := discFolder(path.Base(dir)); !ok { + return dir + } + + parent := path.Dir(dir) + if parent == "." || parent == "/" || parent == "" { + return dir + } + + return parent +} + // ParsePath extracts what it can from one candidate file path. func ParsePath(p string) TrackHint { // Soulseek paths are Windows-style; normalize before splitting. norm := strings.ReplaceAll(p, `\`, "/") base := path.Base(norm) - folder := path.Base(path.Dir(norm)) name := strings.TrimSuffix(base, path.Ext(base)) - hint := TrackHint{Folder: cleanAlbumName(folder)} + // The album's name is the album directory's, not a disc folder's, + // and the disc folder is where a multi-disc rip says which disc a + // file is on. A disc number in the filename ("2-01 …") is more + // specific and overrides it below. + hint := TrackHint{Folder: cleanAlbumName(path.Base(AlbumDir(norm)))} + + if disc, ok := discFolder(path.Base(path.Dir(norm))); ok { + hint.Disc = disc + } if m := trackNumPattern.FindStringSubmatch(name); m != nil { if m[1] != "" { diff --git a/backend/download/provider_slskd.go b/backend/download/provider_slskd.go index ffe0542..ae7f5f4 100644 --- a/backend/download/provider_slskd.go +++ b/backend/download/provider_slskd.go @@ -70,8 +70,9 @@ const ( slskdTransferPoll = 3 * time.Second // slskdMinFiles is the fewest audio files a folder needs before it - // is offered as a candidate. Soulseek returns a lot of one-file - // noise for common queries. + // is offered as a candidate for an album. Soulseek returns a lot of + // one-file noise for common queries. A single-track request takes + // one (see minFilesFor). slskdMinFiles = 2 // slskdHTTPTimeout bounds one API call. @@ -330,7 +331,24 @@ func (s *slskd) Search(ctx context.Context, dl Download) ([]Candidate, error) { ) }() - return s.candidatesFrom(search), nil + return s.candidatesFrom(search, minFilesFor(dl)), nil +} + +// minFilesFor is the fewest audio files a folder must offer to be a +// candidate for this request. +// +// Soulseek answers a search with the files that match it, not with the +// folders they sit in. An album query matches every file in the album's +// folder, because the folder name carries the terms; a *track* query +// usually matches one file per folder. The two-file floor that filters +// out one-file noise for an album therefore filtered out every result +// for a track, and a single-track request could never be served here. +func minFilesFor(dl Download) int { + if dl.RecordingMBID != "" { + return 1 + } + + return slskdMinFiles } // awaitSearch polls until the search completes or the budget runs out. @@ -371,8 +389,9 @@ func (s *slskd) awaitSearch( return last, nil } -// candidatesFrom groups a search's responses into candidates. -func (s *slskd) candidatesFrom(search slskdSearch) []Candidate { +// candidatesFrom groups a search's responses into candidates, dropping +// folders with fewer than minFiles audio files. +func (s *slskd) candidatesFrom(search slskdSearch, minFiles int) []Candidate { out := make([]Candidate, 0, len(search.Responses)) for _, resp := range search.Responses { @@ -400,7 +419,7 @@ func (s *slskd) candidatesFrom(search slskdSearch) []Candidate { total += f.Size } - if audio < slskdMinFiles { + if audio < minFiles { continue } @@ -421,13 +440,15 @@ func (s *slskd) candidatesFrom(search slskdSearch) []Candidate { return out } -// groupByFolder buckets a peer's files by their containing directory. +// groupByFolder buckets a peer's files by the album directory they sit +// in — the containing directory, or the one above it for a disc folder +// (see AlbumDir), so a multi-disc rip is one candidate and not two. func groupByFolder(files []slskdFile) map[string][]slskdFile { out := map[string][]slskdFile{} for _, f := range files { - norm := strings.ReplaceAll(f.Filename, `\`, "/") - out[path.Dir(norm)] = append(out[path.Dir(norm)], f) + dir := AlbumDir(f.Filename) + out[dir] = append(out[dir], f) } return out @@ -817,7 +838,13 @@ func (s *slskd) collect(c Candidate, dst string) (Result, error) { continue } + // A multi-disc candidate keeps its disc folders in staging. + // Flattened, disc 2's "01 Intro.flac" overwrites disc 1's, and + // the importer loses the folder it reads the disc number from. target := filepath.Join(dst, base) + if _, ok := discFolder(folder); ok { + target = filepath.Join(dst, folder, base) + } if err := movePath(src, target); err != nil { return Result{}, fmt.Errorf("collect %s: %w", base, err) diff --git a/backend/download/rank.go b/backend/download/rank.go index 731f9a2..ff90f46 100644 --- a/backend/download/rank.go +++ b/backend/download/rank.go @@ -347,7 +347,9 @@ func scoreMatch( TitleFit: titleFit, } - m.Completeness = completeness(len(audio), len(dl.Expected)) + m.Completeness = completeness( + alignedCount(c.Files), len(audio), len(dl.Expected), + ) // The candidate's own title, and the folder its files sit in, are // two independent guesses at the album name. Take the better one: @@ -415,30 +417,54 @@ func artistFit(want string, c Candidate) float64 { return best } -// completeness scores audio file count against the expected track -// count. Extra files are penalized far more gently than missing ones: +// completeness scores how much of the expected tracklist a candidate +// covers. Extra files are penalized far more gently than missing ones: // a folder with bonus tracks or a stray intro is still the album, while // a folder missing half the tracks is not. -func completeness(got, want int) float64 { +// +// **Coverage is counted in aligned tracks, not in files.** It used to +// be the audio file count, so any ten files scored full marks against +// a ten-track album whether or not they were its tracks — and since +// title fit is the mean over the files that *did* align, a folder where +// three titles matched read as a near-perfect candidate on both counts. +// `aligned` is how many files matchFiles assigned to an expected track; +// `audio` still sets the penalty for extras, because a folder of thirty +// files holding the ten wanted is a worse copy than one holding ten. +func completeness(aligned, audio, want int) float64 { if want == 0 { - if got > 0 { + if audio > 0 { return 0.5 } return 0 } - if got == 0 { + if aligned == 0 { return 0 } - if got >= want { - extra := float64(got-want) / float64(want) + cover := float64(min(aligned, want)) / float64(want) - return math.Max(0.75, 1.0-0.25*extra) + if audio > want { + extra := float64(audio-want) / float64(want) + cover *= math.Max(0.75, 1.0-0.25*extra) } - return float64(got) / float64(want) + return cover +} + +// alignedCount is how many audio files were assigned to an expected +// track. +func alignedCount(files []CandidateFile) int { + n := 0 + + for _, f := range files { + if f.IsAudio && f.MatchedTo != 0 { + n++ + } + } + + return n } // scoreQuality answers whether this is a good copy. diff --git a/backend/download/rank_test.go b/backend/download/rank_test.go index 9c0a2cf..4aeb93f 100644 --- a/backend/download/rank_test.go +++ b/backend/download/rank_test.go @@ -282,28 +282,32 @@ func TestCompleteness(t *testing.T) { tests := []struct { name string - got int + aligned int + audio int want int minScore float64 maxScore float64 }{ - {"exact", 10, 10, 1.0, 1.0}, - {"half missing", 5, 10, 0.49, 0.51}, - {"one bonus track", 11, 10, 0.95, 1.0}, - {"double", 20, 10, 0.74, 0.76}, - {"nothing", 0, 10, 0, 0}, - {"no expectation", 5, 0, 0.5, 0.5}, + {"exact", 10, 10, 10, 1.0, 1.0}, + {"half missing", 5, 5, 10, 0.49, 0.51}, + {"one bonus track", 10, 11, 10, 0.95, 1.0}, + {"double", 10, 20, 10, 0.74, 0.76}, + {"nothing", 0, 0, 10, 0, 0}, + {"no expectation", 0, 5, 0, 0.5, 0.5}, + // Ten files are not ten tracks: three that align are three. + {"right count, wrong tracks", 3, 10, 10, 0.29, 0.31}, + {"files that align to nothing", 0, 10, 10, 0, 0}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := completeness(tt.got, tt.want) + got := completeness(tt.aligned, tt.audio, tt.want) if got < tt.minScore || got > tt.maxScore { t.Errorf( - "completeness(%d, %d) = %f, want in [%f, %f]", - tt.got, tt.want, got, tt.minScore, tt.maxScore, + "completeness(%d, %d, %d) = %f, want in [%f, %f]", + tt.aligned, tt.audio, tt.want, got, tt.minScore, tt.maxScore, ) } })