diff --git a/backend/config/config.go b/backend/config/config.go index f0ff93b..2f6430e 100644 --- a/backend/config/config.go +++ b/backend/config/config.go @@ -411,9 +411,10 @@ func (c *Config) SetDownloadPreferences(prefs download.AutoDownloadPrefs) error formats = append(formats, string(f)) } - c.Downloads.MinFileSizeMB = prefs.MinSizeMB + c.Downloads.MinKbps = prefs.MinKbps + c.Downloads.MaxKbps = prefs.MaxKbps + c.Downloads.PreferredKbps = prefs.PreferredKbps c.Downloads.MaxFileSizeMB = prefs.MaxSizeMB - c.Downloads.PreferredFileSizeMB = prefs.PreferredSizeMB c.Downloads.AllowedFormats = formats if err := c.Save(); err != nil { diff --git a/backend/download/config.go b/backend/download/config.go index bd3e7b2..b176247 100644 --- a/backend/download/config.go +++ b/backend/download/config.go @@ -34,13 +34,29 @@ type UserConfig struct { // in one burst that every provider sees as a flood. WantedBatch int `toml:"WantedBatch"` - // MinFileSizeMB, MaxFileSizeMB and PreferredFileSizeMB bound and - // nudge what auto-pick (interactive or via the request list) may - // grab without asking. Zero on any of them is permissive: see - // AutoDownloadPrefs. - MinFileSizeMB int `toml:"MinFileSizeMB"` - MaxFileSizeMB int `toml:"MaxFileSizeMB"` - PreferredFileSizeMB int `toml:"PreferredFileSizeMB"` + // MinKbps, MaxKbps and PreferredKbps bound and nudge what auto-pick + // (interactive or via the request list) may grab without asking. + // Zero on any of them is permissive: see AutoDownloadPrefs. + // + // They replaced MinFileSizeMB / MaxFileSizeMB / + // PreferredFileSizeMB, which were megabytes and so said nothing + // without knowing how long the release was. The old keys are + // deliberately *not* read back: a number that meant "300 MB" cannot + // be reinterpreted as a bitrate without knowing the album it was + // aimed at, so migrating it would be inventing an intent the user + // never expressed. An existing config falls back to no window, + // which is the permissive default and matches a fresh install — + // and MaxFileSizeMB is the one that does carry over, because a + // ceiling on total bytes still means exactly what it did. + MinKbps int `toml:"MinKbps"` + MaxKbps int `toml:"MaxKbps"` + PreferredKbps int `toml:"PreferredKbps"` + + // MaxFileSizeMB is a hard ceiling on a candidate's total size, kept + // in megabytes on purpose — it is a question about disk space, not + // about quality, and it has to apply to a candidate whose bitrate + // cannot be worked out at all. + MaxFileSizeMB int `toml:"MaxFileSizeMB"` // AllowedFormats restricts auto-pick to these formats. Empty means // no restriction. Values are Format strings ("flac", "mp3", ...). @@ -56,10 +72,11 @@ func (c *UserConfig) AutoDownloadPrefs() AutoDownloadPrefs { } return AutoDownloadPrefs{ - MinSizeMB: c.MinFileSizeMB, - MaxSizeMB: c.MaxFileSizeMB, - PreferredSizeMB: c.PreferredFileSizeMB, - AllowedFormats: formats, + MinKbps: c.MinKbps, + MaxKbps: c.MaxKbps, + PreferredKbps: c.PreferredKbps, + MaxSizeMB: c.MaxFileSizeMB, + AllowedFormats: formats, } } diff --git a/backend/download/manager.go b/backend/download/manager.go index f9eb618..2b284b5 100644 --- a/backend/download/manager.go +++ b/backend/download/manager.go @@ -236,6 +236,12 @@ func (m *Manager) AutoPickable(dl Download, ranked []Candidate) bool { return AutoPickable(dl, ranked, m.preferences()) } +// AutoPickVeto wraps the package function the same way, and is what the +// request list quotes back to the user. +func (m *Manager) AutoPickVeto(dl Download, ranked []Candidate) string { + return AutoPickVeto(dl, ranked, m.preferences()) +} + // Reload rebuilds every provider from stored config. Called at startup // and after any provider settings change. // @@ -612,16 +618,8 @@ func (m *Manager) Attempt( return false, "", err } - if !m.AutoPickable(dl, ranked) { - best := ranked[0] - - return false, fmt.Sprintf( - "best of %d found is not a confident enough match "+ - "(match %.0f%%, quality %.0f%%)", - len(ranked), - best.Match.Overall*100, //nolint:mnd // percent - best.Quality.Overall*100, - ), nil + if veto := m.AutoPickVeto(dl, ranked); veto != "" { + return false, veto, nil } if err := m.store.CreateDownload(ctx, dl); err != nil { diff --git a/backend/download/manager_test.go b/backend/download/manager_test.go index 72d504e..c120699 100644 --- a/backend/download/manager_test.go +++ b/backend/download/manager_test.go @@ -218,8 +218,17 @@ func TestManagerEndToEndAutoPick(t *testing.T) { }, "staging was never released, or the library was never rescanned") } -// An ambiguous result set must park for the user rather than guess. -func TestManagerWaitsWhenAmbiguous(t *testing.T) { +// Two equally good copies are not an ambiguity — they are a spare. +// +// This asserted the opposite for as long as auto-pick required 0.08 of +// daylight over the runner-up, and that rule was wrong in exactly the +// case it fired hardest: a popular album turns up several *correct* +// copies, all matching the tracklist, differing only in format and +// seeders. There is no question there about what to fetch, only about +// which copy, and the ranking already answers that — closest to the +// preferred bitrate first. A candidate does not have to be better than +// the field, only good enough on its own terms. +func TestManagerAutoPicksAmongEquallyGoodCopies(t *testing.T) { t.Parallel() f := newManagerFixture(t) @@ -237,11 +246,41 @@ func TestManagerWaitsWhenAmbiguous(t *testing.T) { t.Fatalf("Start: %v", err) } - if f.manager.AutoPickable(dl, ranked) { - t.Fatal("two equivalent candidates must not auto-pick") + if veto := f.manager.AutoPickVeto(dl, ranked); veto != "" { + t.Fatalf("two equally good copies must auto-pick, got veto: %s", veto) + } + + waitForDownloadState(t, f.store, dl.ID, StateComplete) + + // Exactly one of them was fetched, not both. + if grabs := a.GrabCalls + b.GrabCalls; grabs != 1 { + t.Errorf("grabs = %d, want exactly 1", grabs) + } +} + +// The user can still pick explicitly when auto-pick is not what +// happened — a candidate the ranking did not choose is still grabbable. +func TestManagerPickIsExplicit(t *testing.T) { + t.Parallel() + + f := newManagerFixture(t) + + a := fakeWithAlbum(1, "source-a", ".flac") + b := fakeWithAlbum(2, "source-b", ".flac") + + f.manager.installProvider(Config{ID: 1, Priority: 50}, a) + f.manager.installProvider(Config{ID: 2, Priority: 50}, b) + + // No tracklist: never auto-picks, so the result set parks for the + // user and Pick is the only way anything is fetched. + dl := fourTrackDownload() + dl.Expected = nil + + ranked, err := f.manager.Start(context.Background(), dl) + if err != nil { + t.Fatalf("Start: %v", err) } - // Nothing was grabbed while waiting for the user. if a.GrabCalls != 0 || b.GrabCalls != 0 { t.Errorf( "grabs happened without a pick: a=%d b=%d", @@ -258,7 +297,6 @@ func TestManagerWaitsWhenAmbiguous(t *testing.T) { t.Errorf("stored request id = %s, want %s", stored.ID, dl.ID) } - // The user picks the second one explicitly. if err := f.manager.Pick( context.Background(), dl.ID, ranked[1].ID, ); err != nil { diff --git a/backend/download/rank.go b/backend/download/rank.go index e094372..ecaa53c 100644 --- a/backend/download/rank.go +++ b/backend/download/rank.go @@ -1,6 +1,7 @@ package download import ( + "fmt" "math" "sort" "strings" @@ -34,38 +35,102 @@ const ( weightArtistFit = 0.12 ) -// Quality sub-weights. They sum to 1.0 along with weightSizeFit below. +// Quality sub-weights. Each set sums to 1.0. +// +// There are two of them because a stated preference changes what the +// other numbers are *for*. `formatRank` and `bitrateScore` are the +// app guessing at how good a copy is — FLAC over MP3, 320 over 128 — +// and that guess exists precisely because the user has not said. Once +// they have, the guess should not outvote them: with the old single set +// a preference of 320 kbps moved a candidate's score by at most 0.05 +// against the 0.42 riding on format, so asking for 320 and being handed +// a FLAC every time was the *designed* behaviour. That is the same +// fault the megabyte window had — a preference the user can express and +// the ranking can ignore. const ( - weightFormat = 0.42 - weightBitrate = 0.23 - weightHealth = 0.20 - weightPriority = 0.10 - weightSizeFit = 0.05 + weightFormat = 0.42 + weightBitrate = 0.23 + weightHealth = 0.20 + weightPriority = 0.10 + weightBitrateFit = 0.05 ) +// Quality sub-weights when the user has named a preferred bitrate. +// The weight comes off format and bitrate — the two proxies the +// preference replaces — and health and priority are untouched, since +// neither is a stand-in for anything the user just said. +const ( + statedWeightFormat = 0.20 + statedWeightBitrate = 0.10 + statedWeightHealth = 0.20 + statedWeightPriority = 0.10 + statedWeightBitrateFit = 0.40 +) + +// qualityWeights picks the set, in the order scoreQuality applies them. +func qualityWeights(p AutoDownloadPrefs) ( + format, bitrate, health, priority, fit float64, +) { + if p.PreferredKbps > 0 { + return statedWeightFormat, + statedWeightBitrate, + statedWeightHealth, + statedWeightPriority, + statedWeightBitrateFit + } + + return weightFormat, + weightBitrate, + weightHealth, + weightPriority, + weightBitrateFit +} + // unanchoredCap bounds the match score of a free-text request. Without // an MBID there is no tracklist to be right about, so a confident- // looking score would be a lie — and auto-pick keys off this. const unanchoredCap = 0.65 // AutoDownloadPrefs gates and scores what AutoPickable may choose -// without asking. Zero values are permissive: no size window and no -// format restriction. +// without asking. Zero values are permissive: no bitrate window, no +// size ceiling and no format restriction. +// +// **The window is a rate, not a size.** It used to be three numbers in +// megabytes, which cannot mean anything on their own: 300 MB is a +// generous FLAC single and a suspiciously small boxset, and the user +// setting the number has no idea which release the pipeline will +// eventually apply it to. A bitrate is the same statement normalised +// by how long the music is, so one number holds across a 9-minute EP +// and a 3-hour opera — and it is the unit the thing being described is +// actually measured in. The runtime is known for every request +// auto-pick can act on (`Download.Expected` carries per-track lengths, +// and an anchored request is the only kind that reaches here), so this +// costs no extra lookup. type AutoDownloadPrefs struct { - // MinSizeMB and MaxSizeMB bound what auto-pick will grab. Zero - // means no bound on that side. A candidate outside the window is - // filtered out of auto-pick entirely, not merely scored down — a - // tiny "sampler" torrent or a boxset ten times the expected size is - // usually the wrong thing entirely, not a worse copy of the right - // thing. - MinSizeMB int `json:"minSizeMb"` - MaxSizeMB int `json:"maxSizeMb"` + // MinKbps and MaxKbps bound the average bitrate auto-pick will + // grab. Zero means no bound on that side. A candidate outside the + // window is filtered out of auto-pick entirely, not merely scored + // down — a 96 kbps rip of the right album is not a worse copy the + // user might accept, it is one they said not to take unattended. + // + // For reference: 320 is the top of MP3, ~500–1000 is FLAC depending + // on the material, and anything under ~128 is a transcode. + MinKbps int `json:"minKbps"` + MaxKbps int `json:"maxKbps"` - // PreferredSizeMB nudges the score toward a target size within the - // min/max window (a lossless rip and a heavily-padded lossless rip - // can both pass the window). Zero disables the nudge; sizeFit then - // returns a neutral value that does not affect ranking. - PreferredSizeMB int `json:"preferredSizeMb"` + // PreferredKbps nudges the score toward a target rate within the + // window, and breaks the tie when several candidates are equally + // good matches. Zero disables the nudge; bitrateFit then returns a + // neutral value that does not affect ranking. + PreferredKbps int `json:"preferredKbps"` + + // MaxSizeMB is a hard ceiling on the whole candidate, and it is + // deliberately still a size. It answers a different question from + // the window above — not "is this the quality I want" but "is this + // going to fill the disk" — and it has to hold even for a candidate + // whose bitrate cannot be worked out, which is exactly the shape a + // mislabelled boxset arrives in. Zero means no ceiling. + MaxSizeMB int `json:"maxSizeMb"` // AllowedFormats restricts auto-pick to candidates whose audio // files are all in one of these formats. Empty means no @@ -74,19 +139,33 @@ type AutoDownloadPrefs struct { } // eligible reports whether a candidate may be auto-picked under these -// preferences: within the size window (when set) and, when a format -// list is given, every audio file in an allowed format. -func (p AutoDownloadPrefs) eligible(c Candidate) bool { +// preferences: inside the bitrate window and the size ceiling (when +// set) and, when a format list is given, every audio file in an +// allowed format. +// +// `runtimeMillis` is how long the requested release is, and 0 means +// nobody knows. An unknown runtime **passes** the bitrate window +// rather than failing it: the window is a statement about quality, and +// refusing everything the moment a tracklist is missing a length would +// turn a gap in MusicBrainz into a silent embargo. The size ceiling +// still applies, which is why it exists separately. +func (p AutoDownloadPrefs) eligible(c Candidate, runtimeMillis int64) bool { const bytesPerMB = 1 << 20 - if p.MinSizeMB > 0 && c.TotalSize < int64(p.MinSizeMB)*bytesPerMB { - return false - } - if p.MaxSizeMB > 0 && c.TotalSize > int64(p.MaxSizeMB)*bytesPerMB { return false } + if kbps := candidateKbps(c, runtimeMillis); kbps > 0 { + if p.MinKbps > 0 && kbps < float64(p.MinKbps) { + return false + } + + if p.MaxKbps > 0 && kbps > float64(p.MaxKbps) { + return false + } + } + if len(p.AllowedFormats) == 0 { return true } @@ -107,11 +186,14 @@ func (p AutoDownloadPrefs) eligible(c Candidate) bool { // filter returns only the candidates these preferences allow to be // auto-picked, in the same (already ranked) order. -func (p AutoDownloadPrefs) filter(ranked []Candidate) []Candidate { +func (p AutoDownloadPrefs) filter( + ranked []Candidate, + runtimeMillis int64, +) []Candidate { out := make([]Candidate, 0, len(ranked)) for _, c := range ranked { - if p.eligible(c) { + if p.eligible(c, runtimeMillis) { out = append(out, c) } } @@ -119,32 +201,116 @@ func (p AutoDownloadPrefs) filter(ranked []Candidate) []Candidate { return out } -// sizeFit scores how close totalSize is to PreferredSizeMB, 0..1, -// falling off linearly as the size doubles or halves away from it. -// Returns a neutral 0.5 when no preference is set, so the absence of a -// preference does not bias ranking. -func (p AutoDownloadPrefs) sizeFit(totalSize int64) float64 { +// bitrateFit scores how close a candidate's average bitrate is to +// PreferredKbps, falling off linearly as it doubles or halves away +// from it. +// +// The range is **0.5 to 1.0, not 0 to 1**, and the floor is the point. +// This carries 0.40 of the quality score once a preference is set, so a +// span down to zero would let a preference of 320 kbps push a perfectly +// good FLAC under `minQuality` and out of auto-pick altogether — +// turning "I like 320" into "never take anything else", silently. A +// preference may promote the copy that matches it; it may not +// disqualify the others. That is what `MinKbps`/`MaxKbps` are for, and +// they say so out loud. +// +// Returns the neutral floor when no preference is set or the rate +// cannot be worked out, so neither an absent preference nor an absent +// runtime biases ranking. +func (p AutoDownloadPrefs) bitrateFit( + c Candidate, + runtimeMillis int64, +) float64 { const ( - bytesPerMB = 1 << 20 - neutral = 0.5 + neutral = 0.5 + span = 0.5 ) - if p.PreferredSizeMB <= 0 || totalSize <= 0 { + if p.PreferredKbps <= 0 { return neutral } - preferred := float64(p.PreferredSizeMB) * bytesPerMB - ratio := float64(totalSize) / preferred + kbps := candidateKbps(c, runtimeMillis) + if kbps <= 0 { + return neutral + } + ratio := kbps / float64(p.PreferredKbps) if ratio < 1 { ratio = 1 / ratio } // ratio is now >= 1: 1.0 is an exact match, 2.0 is double or half - // the preferred size. Falls to 0 at 2x away and beyond. - fit := 1 - (ratio - 1) + // the preferred rate, where the closeness term reaches 0. + return neutral + span*clamp01(1-(ratio-1)) +} - return clamp01(fit) +// candidateKbps is a candidate's average audio bitrate, or 0 when it +// cannot be worked out. +// +// Two sources, in this order, and the order matters: +// +// - **Derived from bytes over runtime**, which is the honest one. It +// covers lossless (where a stated bitrate rarely exists), it cannot +// be lied to by a filename, and it is what the user's window means. +// Only the *audio* files count: cover scans and a log file are not +// part of the bitrate, and a folder with 30 MB of artwork would +// otherwise read as a better rip than the same music without it. +// - **The mean stated bitrate**, when the runtime is unknown. Weaker +// — a provider that parses it from an MP3 header states it and one +// that guesses from the filename also "states" it — but a number +// from the file itself beats no number at all. +func candidateKbps(c Candidate, runtimeMillis int64) float64 { + const bitsPerByte = 8 + + audio := c.AudioFiles() + if len(audio) == 0 { + return 0 + } + + if runtimeMillis > 0 { + var bytes int64 + for _, f := range audio { + bytes += f.Size + } + + if bytes > 0 { + // bytes×8 bits over seconds, expressed in kbps: the two + // factors of 1000 (millis→seconds, bits→kilobits) cancel. + return float64(bytes) * bitsPerByte / + float64(runtimeMillis) + } + } + + var ( + sum int + count int + ) + + for _, f := range audio { + if f.Bitrate > 0 { + sum += f.Bitrate + count++ + } + } + + if count == 0 { + return 0 + } + + return float64(sum) / float64(count) +} + +// runtimeMillis is how long the requested release is, summed over its +// expected tracklist. Zero when the tracklist is absent or carries no +// lengths, which is what every caller here treats as "unknown". +func (d Download) runtimeMillis() int64 { + var total int64 + for _, t := range d.Expected { + total += t.LengthMillis + } + + return total } // Score fills a candidate's Match, Quality and Score fields. @@ -160,7 +326,9 @@ func Score(dl Download, c Candidate, priority int, prefs AutoDownloadPrefs) Cand c.Files = mergeMatched(c.Files, matched) c.Match = scoreMatch(dl, c, audio, titleFit) - c.Quality = scoreQuality(c, audio, priority, prefs) + c.Quality = scoreQuality( + c, audio, priority, prefs, dl.runtimeMillis(), + ) c.Score = weightMatch*c.Match.Overall + weightQuality*c.Quality.Overall @@ -279,11 +447,12 @@ func scoreQuality( audio []CandidateFile, priority int, prefs AutoDownloadPrefs, + runtimeMillis int64, ) QualityScore { q := QualityScore{ - Health: clamp01(c.Health), - Priority: clamp01(float64(priority) / 100.0), - SizeFit: prefs.sizeFit(c.TotalSize), + Health: clamp01(c.Health), + Priority: clamp01(float64(priority) / 100.0), + BitrateFit: prefs.bitrateFit(c, runtimeMillis), } if len(audio) == 0 { @@ -310,11 +479,13 @@ func scoreQuality( q.FormatRank = worst q.Bitrate = bitrateScore(audio) - q.Overall = weightFormat*q.FormatRank + - weightBitrate*q.Bitrate + - weightHealth*q.Health + - weightPriority*q.Priority + - weightSizeFit*q.SizeFit + wFormat, wBitrate, wHealth, wPriority, wFit := qualityWeights(prefs) + + q.Overall = wFormat*q.FormatRank + + wBitrate*q.Bitrate + + wHealth*q.Health + + wPriority*q.Priority + + wFit*q.BitrateFit if q.Mixed { q.Overall *= 0.9 @@ -444,6 +615,19 @@ func Rank( return out[i].Match.Overall > out[j].Match.Overall } + // Closest to the preferred bitrate wins the tie. + // + // This is what decides which copy is taken now that auto-pick + // no longer requires the winner to be clear of the field: when + // several candidates are equally good matches of equal overall + // quality, the one the user said they wanted the shape of is + // the answer, ahead of provider priority. With no preference + // set every BitrateFit is the same neutral value and this + // falls through, exactly as before. + if out[i].Quality.BitrateFit != out[j].Quality.BitrateFit { + return out[i].Quality.BitrateFit > out[j].Quality.BitrateFit + } + if out[i].Quality.Priority != out[j].Quality.Priority { return out[i].Quality.Priority > out[j].Quality.Priority } @@ -454,19 +638,58 @@ func Rank( return out } -// AutoPickable reports whether a ranked list has a clear enough winner -// to grab without asking. It demands an anchored request, a high match, -// decent quality, and daylight between first and second place — if two -// candidates are close, the choice is the user's. -func AutoPickable(dl Download, ranked []Candidate, prefs AutoDownloadPrefs) bool { - const ( - minMatch = 0.85 - minQuality = 0.5 - minLead = 0.08 - ) +// Auto-pick gates. Named rather than inlined because AutoPickVeto +// reports which of them refused, and a number in a sentence the user +// reads should be the same number the decision used. +const ( + minMatch = 0.85 + minQuality = 0.5 +) - if !dl.Anchored() || len(ranked) == 0 { - return false +// AutoPickable reports whether a ranked list has a candidate worth +// grabbing without asking: an anchored request with a tracklist behind +// it, and a candidate that clears the match and quality bars inside the +// user's guardrails. +// +// **It does not require the winner to be better than the runner-up.** +// It used to demand 0.08 of daylight on the combined score, which meant +// the check fired hardest in the case it was never written for: a +// popular album turns up five *correct* copies, all matching the +// tracklist at 95%+ and differing only in format and seeders, their +// scores land within a point of each other, and auto-pick refused +// forever on the grounds that the choice was the user's. It was not. +// There was no question about *what* to fetch, only about which copy — +// and abundance is the one condition under which that question matters +// least. A candidate does not need to be the best one, only one that +// meets the criteria; where several do, `Rank` puts the one closest to +// the preferred bitrate first. +func AutoPickable(dl Download, ranked []Candidate, prefs AutoDownloadPrefs) bool { + return AutoPickVeto(dl, ranked, prefs) == "" +} + +// AutoPickVeto returns the reason auto-pick declined, or "" when it +// would go ahead. +// +// It exists because "it rejected all of them" was indistinguishable +// from "it found nothing good". The request list's message was built +// from `ranked[0]` — the best candidate *before* the size and format +// guardrails, and before the lead check — so a request refused because +// the user's maximum size excluded every copy, or because three equally +// good copies were found, reported "best of 12 found is not a confident +// enough match (match 96%, quality 88%)". Numbers that clear both +// thresholds, beside a refusal, is a message that teaches the user the +// matcher is broken. Each gate names itself now. +func AutoPickVeto( + dl Download, + ranked []Candidate, + prefs AutoDownloadPrefs, +) string { + if len(ranked) == 0 { + return "nothing found" + } + + if !dl.Anchored() { + return "the request is free text, so there is no release to be right about" } // An anchor with no tracklist behind it is an anchor in name only: @@ -474,29 +697,42 @@ func AutoPickable(dl Download, ranked []Candidate, prefs AutoDownloadPrefs) bool // is exactly the evidence a wrong-album candidate also has. This // matters most for the request list, where nobody is watching. if len(dl.Expected) == 0 { - return false + return "no tracklist for this release is known yet, so a candidate cannot be checked against it" } - // The guardrails apply before the match/quality/lead checks: a - // candidate outside the allowed size or format is not a worse - // choice, it is not a choice auto-pick may make at all, so it must - // not count as "the winner" nor as "second place" for the lead - // check below. - eligible := prefs.filter(ranked) + // The guardrails apply before the match and quality checks: a + // candidate outside the allowed bitrate, size or format is not a + // worse choice, it is not a choice auto-pick may make at all, so it + // must not count as "the winner" either. + eligible := prefs.filter(ranked, dl.runtimeMillis()) if len(eligible) == 0 { - return false + return fmt.Sprintf( + "all %d found are outside the auto-download bitrate, size or format limits", + len(ranked), + ) } best := eligible[0] - if best.Match.Overall < minMatch || best.Quality.Overall < minQuality { - return false + + if best.Match.Overall < minMatch { + return fmt.Sprintf( + "best of %d found matches this release only %.0f%% (needs %.0f%%)", + len(ranked), + best.Match.Overall*100, //nolint:mnd // percent + minMatch*100, //nolint:mnd // percent + ) } - if len(eligible) > 1 && best.Score-eligible[1].Score < minLead { - return false + if best.Quality.Overall < minQuality { + return fmt.Sprintf( + "best of %d found is the right release but scores %.0f%% on quality (needs %.0f%%)", + len(ranked), + best.Quality.Overall*100, //nolint:mnd // percent + minQuality*100, //nolint:mnd // percent + ) } - return true + return "" } // mergeMatched copies MatchedTo assignments from the audio-only slice diff --git a/backend/download/rank_test.go b/backend/download/rank_test.go index 9c7915b..9c0a2cf 100644 --- a/backend/download/rank_test.go +++ b/backend/download/rank_test.go @@ -1,6 +1,34 @@ package download -import "testing" +import ( + "strings" + "testing" +) + +// trackMillis is five minutes; okComputer's four of them make a +// twenty-minute release, which is what turns a candidate's byte count +// into a bitrate the assertions below can name. +const trackMillis = 5 * 60 * 1000 + +// okComputerRuntime is that release's runtime, for the helpers that +// need it directly. +const okComputerRuntime = 4 * trackMillis + +// kbpsCandidate builds an annotated candidate whose audio adds up to +// the given average bitrate over okComputer's runtime. +func kbpsCandidate(id, ext string, kbps int) Candidate { + // bits = kbps × 1000 × (runtimeMillis / 1000), so the thousands + // cancel and the byte count is kbps × runtimeMillis / 8. + const bitsPerByte = 8 + + total := int64(kbps) * okComputerRuntime / bitsPerByte + + c := candidateFor(id, allTitles(), ext, total/int64(len(allTitles()))) + c.Files = AnnotateFiles(c.Files) + c.TotalSize = total + + return c +} // okComputer is the reference request used across ranking tests. func okComputer() Download { @@ -8,11 +36,15 @@ func okComputer() Download { ReleaseMBID: "mbid-ok-computer", Artist: "Radiohead", Album: "OK Computer", + // Four five-minute tracks: twenty minutes, so a candidate's + // bitrate is a number these tests can state exactly. Without + // lengths there is no runtime and the bitrate window has + // nothing to divide by. Expected: []ExpectedTrack{ - {Position: 1, Title: "Airbag"}, - {Position: 2, Title: "Paranoid Android"}, - {Position: 3, Title: "Subterranean Homesick Alien"}, - {Position: 4, Title: "Exit Music (For a Film)"}, + {Position: 1, Title: "Airbag", LengthMillis: trackMillis}, + {Position: 2, Title: "Paranoid Android", LengthMillis: trackMillis}, + {Position: 3, Title: "Subterranean Homesick Alien", LengthMillis: trackMillis}, + {Position: 4, Title: "Exit Music (For a Film)", LengthMillis: trackMillis}, }, } } @@ -187,7 +219,7 @@ func TestUnanchoredMatchIsCapped(t *testing.T) { } } -func TestAutoPickableRequiresAnchorAndLead(t *testing.T) { +func TestAutoPickableRequiresAnchorAndTracklist(t *testing.T) { t.Parallel() dl := okComputer() @@ -211,14 +243,18 @@ func TestAutoPickableRequiresAnchorAndLead(t *testing.T) { } }) - t.Run("two close candidates are not", func(t *testing.T) { + // Two identical copies are a spare, not an ambiguity. This + // asserted the opposite while auto-pick required daylight over the + // runner-up — a rule that made abundance the thing that stopped a + // request being satisfied, which is backwards. + t.Run("two equally good candidates still are", func(t *testing.T) { t.Parallel() twin := best twin.ID = "twin" - if AutoPickable(dl, []Candidate{best, twin}, AutoDownloadPrefs{}) { - t.Error("identical candidates must not auto-pick") + if !AutoPickable(dl, []Candidate{best, twin}, AutoDownloadPrefs{}) { + t.Error("identical good candidates must auto-pick") } }) @@ -300,18 +336,11 @@ func TestProviderPriorityBreaksTies(t *testing.T) { } } -const mb = 1 << 20 - func TestAutoDownloadPrefsEligible(t *testing.T) { t.Parallel() - flacCandidate := candidateFor("c", allTitles(), ".flac", 30_000_000) - flacCandidate.Files = AnnotateFiles(flacCandidate.Files) - flacCandidate.TotalSize = 300 * mb - - mp3Candidate := candidateFor("c", allTitles(), ".mp3", 3_000_000) - mp3Candidate.Files = AnnotateFiles(mp3Candidate.Files) - mp3Candidate.TotalSize = 30 * mb + flacCandidate := kbpsCandidate("c", ".flac", 900) + mp3Candidate := kbpsCandidate("c", ".mp3", 128) tests := []struct { name string @@ -321,18 +350,25 @@ func TestAutoDownloadPrefsEligible(t *testing.T) { }{ {"zero value is permissive", AutoDownloadPrefs{}, flacCandidate, true}, { - "within min/max window", - AutoDownloadPrefs{MinSizeMB: 100, MaxSizeMB: 500}, + "within the bitrate window", + AutoDownloadPrefs{MinKbps: 320, MaxKbps: 1200}, flacCandidate, true, }, { - "below minimum", - AutoDownloadPrefs{MinSizeMB: 400}, + "below the minimum bitrate", + AutoDownloadPrefs{MinKbps: 500}, + mp3Candidate, false, + }, + { + "above the maximum bitrate", + AutoDownloadPrefs{MaxKbps: 500}, flacCandidate, false, }, { - "above maximum", - AutoDownloadPrefs{MaxSizeMB: 200}, + // The ceiling is bytes, not a rate, and it is the guard + // that still works when the bitrate cannot be worked out. + "above the hard size ceiling", + AutoDownloadPrefs{MaxSizeMB: 50}, flacCandidate, false, }, { @@ -351,57 +387,131 @@ func TestAutoDownloadPrefsEligible(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - if got := tt.prefs.eligible(tt.c); got != tt.want { + got := tt.prefs.eligible(tt.c, okComputerRuntime) + if got != tt.want { t.Errorf("eligible() = %v, want %v", got, tt.want) } }) } } +// A release nobody knows the length of cannot be judged on bitrate, and +// the window must not become a silent embargo because MusicBrainz is +// missing a track length. The size ceiling still applies — that is why +// it is a separate field. +func TestBitrateWindowPassesAnUnknownRuntime(t *testing.T) { + t.Parallel() + + c := kbpsCandidate("c", ".mp3", 128) + prefs := AutoDownloadPrefs{MinKbps: 900} + + if !prefs.eligible(c, 0) { + t.Error("an unknown runtime must pass the bitrate window") + } + + if prefs.eligible(c, okComputerRuntime) { + t.Error("a known runtime must still be judged") + } + + ceiling := AutoDownloadPrefs{MaxSizeMB: 1} + if ceiling.eligible(c, 0) { + t.Error("the size ceiling must apply even with no runtime") + } +} + +// Artwork is not part of the bitrate. A folder carrying 30 MB of +// scans would otherwise read as a better rip than the same music +// without them, which is backwards. +func TestBitrateIgnoresNonAudioFiles(t *testing.T) { + t.Parallel() + + c := kbpsCandidate("c", ".mp3", 320) + bare := candidateKbps(c, okComputerRuntime) + + c.Files = append(c.Files, CandidateFile{ + Path: "Radiohead - OK Computer/cover.jpg", + Size: 30 << 20, + }) + c.Files = AnnotateFiles(c.Files) + + if got := candidateKbps(c, okComputerRuntime); got != bare { + t.Errorf("bitrate with artwork = %f, want %f", got, bare) + } +} + +// Where no runtime is known, a stated per-file bitrate is better than +// no answer at all. +func TestBitrateFallsBackToTheStatedRate(t *testing.T) { + t.Parallel() + + c := candidateFor("c", allTitles(), ".mp3", 3_000_000) + for i := range c.Files { + c.Files[i].Bitrate = 192 + } + + c.Files = AnnotateFiles(c.Files) + + if got := candidateKbps(c, 0); got != 192 { + t.Errorf("stated bitrate = %f, want 192", got) + } +} + func TestAutoDownloadPrefsFilter(t *testing.T) { t.Parallel() - small := candidateFor("small", allTitles(), ".flac", 10_000_000) - small.TotalSize = 50 * mb + lossy := kbpsCandidate("lossy", ".mp3", 128) + lossless := kbpsCandidate("lossless", ".flac", 900) - big := candidateFor("big", allTitles(), ".flac", 30_000_000) - big.TotalSize = 500 * mb + prefs := AutoDownloadPrefs{MinKbps: 500} - prefs := AutoDownloadPrefs{MinSizeMB: 100, MaxSizeMB: 600} + filtered := prefs.filter( + []Candidate{lossy, lossless}, okComputerRuntime, + ) - filtered := prefs.filter([]Candidate{small, big}) - - if len(filtered) != 1 || filtered[0].ID != "big" { + if len(filtered) != 1 || filtered[0].ID != "lossless" { t.Errorf("filter() = %v, want only the in-window candidate", filtered) } } -func TestAutoDownloadPrefsSizeFit(t *testing.T) { +func TestAutoDownloadPrefsBitrateFit(t *testing.T) { t.Parallel() const neutral = 0.5 tests := []struct { - name string - prefs AutoDownloadPrefs - totalSize int64 - want float64 + name string + prefs AutoDownloadPrefs + c Candidate + want float64 }{ - {"no preference is neutral", AutoDownloadPrefs{}, 300 * mb, neutral}, + { + "no preference is neutral", + AutoDownloadPrefs{}, + kbpsCandidate("c", ".flac", 900), neutral, + }, { "exact match scores 1", - AutoDownloadPrefs{PreferredSizeMB: 300}, - 300 * mb, 1.0, + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".mp3", 320), 1.0, }, { - "double the preferred size scores 0", - AutoDownloadPrefs{PreferredSizeMB: 300}, - 600 * mb, 0.0, + // The floor is neutral, not zero: this term carries 0.40 + // of the quality score once a preference is set, and a + // span to zero would let "I like 320" quietly disqualify + // every FLAC from auto-pick. + "double the preferred rate falls to the neutral floor", + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".flac", 640), neutral, }, { - "half the preferred size scores 0", - AutoDownloadPrefs{PreferredSizeMB: 300}, - 150 * mb, 0.0, + "half the preferred rate falls to the neutral floor", + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".mp3", 160), neutral, + }, + { + "an unknowable rate is neutral", + AutoDownloadPrefs{PreferredKbps: 320}, + kbpsCandidate("c", ".mp3", 320), neutral, }, } @@ -409,30 +519,223 @@ func TestAutoDownloadPrefsSizeFit(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - if got := tt.prefs.sizeFit(tt.totalSize); got != tt.want { - t.Errorf("sizeFit(%d) = %f, want %f", tt.totalSize, got, tt.want) + // The last case deliberately withholds the runtime. + runtime := int64(okComputerRuntime) + if tt.name == "an unknowable rate is neutral" { + runtime = 0 + } + + if got := tt.prefs.bitrateFit(tt.c, runtime); got != tt.want { + t.Errorf("bitrateFit() = %f, want %f", got, tt.want) } }) } } // An otherwise-perfect candidate must not auto-pick when it falls -// outside the configured size guard: the guardrail applies before the -// match/quality/lead checks, not as one more input averaged into them. -func TestAutoPickableRejectsCandidateOutsideSizeGuard(t *testing.T) { +// outside the configured guardrails: they apply before the match and +// quality checks, not as one more input averaged into them. +func TestAutoPickableRejectsCandidateOutsideTheGuardrails(t *testing.T) { t.Parallel() dl := okComputer() - best := Score(dl, candidateFor("a", allTitles(), ".flac", 30_000_000), 50, AutoDownloadPrefs{}) - best.TotalSize = 500 * mb + best := Score(dl, kbpsCandidate("a", ".flac", 900), 50, AutoDownloadPrefs{}) if !AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{}) { t.Fatal("expected this candidate to be auto-pickable with no guardrails") } - tight := AutoDownloadPrefs{MinSizeMB: 10, MaxSizeMB: 100} + if AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{MaxKbps: 320}) { + t.Error("candidate above the bitrate window must not auto-pick") + } - if AutoPickable(dl, []Candidate{best}, tight) { - t.Error("candidate outside the size guard must not auto-pick") + if AutoPickable(dl, []Candidate{best}, AutoDownloadPrefs{MaxSizeMB: 1}) { + t.Error("candidate above the size ceiling must not auto-pick") + } +} + +// The refusal has to name the gate that refused. +// +// Before AutoPickVeto, every one of these came back as the same +// sentence built from `ranked[0]` — the best candidate before the size +// and format guardrails — so a request refused because the user's size +// window excluded every copy reported a match and a quality that both +// cleared their thresholds. A refusal quoting numbers that pass is +// what made the matcher look broken from outside. +func TestAutoPickVetoNamesTheGate(t *testing.T) { + t.Parallel() + + dl := okComputer() + best := Score( + dl, + candidateFor("a", allTitles(), ".flac", 30_000_000), + 50, + AutoDownloadPrefs{}, + ) + + // candidateFor sizes the files and leaves TotalSize at 0, which is + // what the guardrails read. + sized := func(c Candidate, total int64) Candidate { + c.TotalSize = total + + return c + } + + tests := []struct { + name string + dl Download + ranked []Candidate + prefs AutoDownloadPrefs + wantSub string + }{ + { + name: "nothing found", + dl: dl, + ranked: nil, + wantSub: "nothing found", + }, + { + name: "free text", + dl: Download{Artist: "Radiohead", Album: "OK Computer"}, + ranked: []Candidate{best}, + wantSub: "free text", + }, + { + name: "no tracklist behind the anchor", + dl: Download{ + ReleaseMBID: "mbid-ok-computer", + Artist: "Radiohead", + Album: "OK Computer", + }, + ranked: []Candidate{best}, + wantSub: "no tracklist", + }, + { + // The candidate is 120 MB and the window tops out at 1 MB: + // the old message reported its match and quality instead. + name: "outside the size window", + dl: dl, + ranked: []Candidate{sized(best, 120<<20)}, + prefs: AutoDownloadPrefs{MaxSizeMB: 1}, + wantSub: "bitrate, size or format limits", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := AutoPickVeto(tt.dl, tt.ranked, tt.prefs) + if !strings.Contains(got, tt.wantSub) { + t.Errorf("veto = %q, want it to mention %q", got, tt.wantSub) + } + }) + } +} + +// A clear winner has no veto at all — the sentence is empty, which is +// what AutoPickable reads. +func TestAutoPickVetoIsEmptyForAClearWinner(t *testing.T) { + t.Parallel() + + dl := okComputer() + best := Score( + dl, + candidateFor("a", allTitles(), ".flac", 30_000_000), + 50, + AutoDownloadPrefs{}, + ) + weak := Score( + dl, + candidateFor("b", allTitles()[:2], ".mp3", 1_000_000), + 50, + AutoDownloadPrefs{}, + ) + + if got := AutoPickVeto(dl, []Candidate{best, weak}, AutoDownloadPrefs{}); got != "" { + t.Errorf("veto = %q, want none", got) + } +} + +// With several candidates that all clear the bar, the preferred +// bitrate decides which one is taken. +// +// This is what replaced the daylight requirement. Auto-pick no longer +// refuses when the field is close; it takes the copy nearest the shape +// the user asked for, which is the question they actually answered in +// Settings. +func TestPreferredBitrateBreaksTheTie(t *testing.T) { + t.Parallel() + + dl := okComputer() + prefs := AutoDownloadPrefs{PreferredKbps: 320} + + // Same album, same completeness, same health, same provider — the + // only difference between them is the rate. + lossless := kbpsCandidate("lossless", ".flac", 900) + perfect := kbpsCandidate("perfect", ".mp3", 320) + + ranked := Rank( + dl, []Candidate{lossless, perfect}, nil, prefs, + ) + + if ranked[0].ID != "perfect" { + t.Errorf( + "winner = %q (fit %f) over %q (fit %f), want the 320 kbps copy", + ranked[0].ID, ranked[0].Quality.BitrateFit, + ranked[1].ID, ranked[1].Quality.BitrateFit, + ) + } + + if AutoPickVeto(dl, ranked, prefs) != "" { + t.Error("a close field must still auto-pick") + } +} + +// With no preference set, nothing changes: BitrateFit is the same +// neutral value for every candidate and the older tie-breaks decide. +func TestNoPreferredBitrateLeavesRankingAlone(t *testing.T) { + t.Parallel() + + dl := okComputer() + + lossless := kbpsCandidate("lossless", ".flac", 900) + lossy := kbpsCandidate("lossy", ".mp3", 320) + + ranked := Rank( + dl, []Candidate{lossy, lossless}, nil, AutoDownloadPrefs{}, + ) + + if ranked[0].ID != "lossless" { + t.Errorf( + "winner = %q, want the lossless copy on format alone", + ranked[0].ID, + ) + } +} + +// A preferred bitrate promotes the copy that matches it and must never +// disqualify the ones that do not. It carries 0.40 of the quality +// score, so a fit spanning down to zero would put a perfectly good FLAC +// under minQuality and out of auto-pick — turning a preference into a +// prohibition without saying so. MinKbps and MaxKbps are how a user +// says that on purpose. +func TestAPreferredBitrateNeverDisqualifies(t *testing.T) { + t.Parallel() + + dl := okComputer() + far := AutoDownloadPrefs{PreferredKbps: 128} + + lossless := Score(dl, kbpsCandidate("flac", ".flac", 900), 50, far) + + if lossless.Quality.Overall < minQuality { + t.Errorf( + "quality = %f under a far-off preference, want >= %f", + lossless.Quality.Overall, minQuality, + ) + } + + if veto := AutoPickVeto(dl, []Candidate{lossless}, far); veto != "" { + t.Errorf("a far-off preference vetoed the candidate: %s", veto) } } diff --git a/backend/download/types.go b/backend/download/types.go index c4e7886..d76c2ec 100644 --- a/backend/download/types.go +++ b/backend/download/types.go @@ -302,7 +302,12 @@ type QualityScore struct { Bitrate float64 `json:"bitrate"` Health float64 `json:"health"` // seeders, free slots Priority float64 `json:"priority"` // user's per-provider preference - SizeFit float64 `json:"sizeFit"` // closeness to the preferred download size + // BitrateFit is closeness to the preferred *rate*, which is what + // the auto-download window is expressed in. It replaced a + // `SizeFit` measured in megabytes: a size means nothing without + // knowing how long the music is, so the same number described a + // generous single and a suspiciously small boxset. + BitrateFit float64 `json:"bitrateFit"` // Mixed marks a candidate whose files are not all the same format, // which usually means a hand-assembled folder rather than a rip. diff --git a/frontend/bindings/yellowjacket/backend/download/models.ts b/frontend/bindings/yellowjacket/backend/download/models.ts index 4f2a904..fbe539a 100644 --- a/frontend/bindings/yellowjacket/backend/download/models.ts +++ b/frontend/bindings/yellowjacket/backend/download/models.ts @@ -3,28 +3,52 @@ /** * AutoDownloadPrefs gates and scores what AutoPickable may choose - * without asking. Zero values are permissive: no size window and no - * format restriction. + * without asking. Zero values are permissive: no bitrate window, no + * size ceiling and no format restriction. + * + * **The window is a rate, not a size.** It used to be three numbers in + * megabytes, which cannot mean anything on their own: 300 MB is a + * generous FLAC single and a suspiciously small boxset, and the user + * setting the number has no idea which release the pipeline will + * eventually apply it to. A bitrate is the same statement normalised + * by how long the music is, so one number holds across a 9-minute EP + * and a 3-hour opera — and it is the unit the thing being described is + * actually measured in. The runtime is known for every request + * auto-pick can act on (`Download.Expected` carries per-track lengths, + * and an anchored request is the only kind that reaches here), so this + * costs no extra lookup. */ export interface AutoDownloadPrefs { /** - * MinSizeMB and MaxSizeMB bound what auto-pick will grab. Zero - * means no bound on that side. A candidate outside the window is - * filtered out of auto-pick entirely, not merely scored down — a - * tiny "sampler" torrent or a boxset ten times the expected size is - * usually the wrong thing entirely, not a worse copy of the right - * thing. + * MinKbps and MaxKbps bound the average bitrate auto-pick will + * grab. Zero means no bound on that side. A candidate outside the + * window is filtered out of auto-pick entirely, not merely scored + * down — a 96 kbps rip of the right album is not a worse copy the + * user might accept, it is one they said not to take unattended. + * + * For reference: 320 is the top of MP3, ~500–1000 is FLAC depending + * on the material, and anything under ~128 is a transcode. */ - "minSizeMb": number; - "maxSizeMb": number; + "minKbps": number; + "maxKbps": number; /** - * PreferredSizeMB nudges the score toward a target size within the - * min/max window (a lossless rip and a heavily-padded lossless rip - * can both pass the window). Zero disables the nudge; sizeFit then - * returns a neutral value that does not affect ranking. + * PreferredKbps nudges the score toward a target rate within the + * window, and breaks the tie when several candidates are equally + * good matches. Zero disables the nudge; bitrateFit then returns a + * neutral value that does not affect ranking. */ - "preferredSizeMb": number; + "preferredKbps": number; + + /** + * MaxSizeMB is a hard ceiling on the whole candidate, and it is + * deliberately still a size. It answers a different question from + * the window above — not "is this the quality I want" but "is this + * going to fill the disk" — and it has to hold even for a candidate + * whose bitrate cannot be worked out, which is exactly the shape a + * mislabelled boxset arrives in. Zero means no ceiling. + */ + "maxSizeMb": number; /** * AllowedFormats restricts auto-pick to candidates whose audio @@ -487,9 +511,13 @@ export interface QualityScore { "priority": number; /** - * closeness to the preferred download size + * BitrateFit is closeness to the preferred *rate*, which is what + * the auto-download window is expressed in. It replaced a + * `SizeFit` measured in megabytes: a size means nothing without + * knowing how long the music is, so the same number described a + * generous single and a suspiciously small boxset. */ - "sizeFit": number; + "bitrateFit": number; /** * Mixed marks a candidate whose files are not all the same format, diff --git a/frontend/src/components/config-page/download-clients.ts b/frontend/src/components/config-page/download-clients.ts index cf36c44..0fb7052 100644 --- a/frontend/src/components/config-page/download-clients.ts +++ b/frontend/src/components/config-page/download-clients.ts @@ -86,9 +86,10 @@ export class DownloadClients extends LitElement { /** Working copy of the auto-download guardrails. */ @state() private prefs: download.AutoDownloadPrefs = { - minSizeMb: 0, + minKbps: 0, + maxKbps: 0, + preferredKbps: 0, maxSizeMb: 0, - preferredSizeMb: 0, allowedFormats: [], } as download.AutoDownloadPrefs; @@ -284,25 +285,72 @@ export class DownloadClients extends LitElement { : nothing}
+
{ this.prefs = { ...this.prefs, - minSizeMb: Number((e.target as HTMLInputElement).value) || 0, + minKbps: Number((e.target as HTMLInputElement).value) || 0, }; }} > { + this.prefs = { + ...this.prefs, + maxKbps: Number((e.target as HTMLInputElement).value) || 0, + }; + }} + > + { + this.prefs = { + ...this.prefs, + preferredKbps: + Number((e.target as HTMLInputElement).value) || 0, + }; + }} + > +
+ +
+ 320 is the top of MP3; a FLAC rip is usually + 500–1000 depending on the music. Preferred + decides between copies that are otherwise equally + good — it never rules one out, which is what the + minimum and maximum are for. +
+ +
+ { this.prefs = { @@ -311,22 +359,14 @@ export class DownloadClients extends LitElement { }; }} > - { - this.prefs = { - ...this.prefs, - preferredSizeMb: - Number((e.target as HTMLInputElement).value) || 0, - }; - }} - > +
+ +
+ A ceiling on the download itself, in case a + mislabelled boxset gets through. Still a size + because it is a question about disk space, and + because it has to apply to a candidate whose + bitrate cannot be worked out at all.