diff --git a/README.md b/README.md index 4a413e3..9cdbf04 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,10 @@ Credentials you need to set, per source: | Beatport | `username` + `password` | | SoundCloud | none; handled via `yt-dlp` | +Lyrics (plain and synced `.lrc`-style) are fetched for Qobuz, Tidal and Deezer. +Each costs one extra API request per track, so `metadata.exclude = ["lyrics"]` +turns them off for a faster rip. + Each provider section also has vendor `app_client_id`-style keys. Leave them empty: they default to values built into the binary. They exist so that if a provider rotates one, you can patch your install with a config edit instead of diff --git a/config.toml.example b/config.toml.example index 47015f7..3b504c8 100644 --- a/config.toml.example +++ b/config.toml.example @@ -10,8 +10,13 @@ concurrency = true # The maximum number of tracks to download at once # Set to -1 for no limit max_connections = 6 -# Max number of API requests per source per minute -# Set to -1 for no limit +# Max number of API requests per source per minute. Set to -1 for no limit. +# +# This is the usual cause of a large album feeling slow: every track needs a +# stream-URL lookup, so a 68-track album is ~69 requests and at 60/minute that +# is a minute of waiting before the audio is even reached. Requests are +# allowed to burst up to max_connections, so raising this is what lets several +# tracks download at once. Raise it if your provider tolerates it. requests_per_minute = 60 # Verify SSL certificates for API connections # Set to false only if certificate verification fails (not recommended) @@ -169,7 +174,13 @@ set_playlist_to_album = true renumber_playlist_tracks = true # Separator used when a provider exposes multiple artists as separate values artist_separator = "; " -# Metadata fields to exclude from tagging +# Metadata fields to exclude from tagging. +# +# "lyrics" is worth knowing about: Qobuz, Tidal and Deezer each serve lyrics +# from a separate endpoint, so fetching them costs one extra API request per +# track and is subject to requests_per_minute above. Set +# exclude = ["lyrics"] if you do not want lyrics and would rather have the +# faster rip. exclude = [] [filepaths] diff --git a/internal/app/app.go b/internal/app/app.go index a4a9162..a3920bc 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -53,6 +53,7 @@ type ripTrackOptions struct { albumEmbedCover string albumArtist string prefetched *provider.Downloadable + prefetchedMeta map[string]any index int total int albumDiscTotal int @@ -91,6 +92,28 @@ type trackTagger interface { TagFLAC(ctx context.Context, path string, meta tag.Metadata, coverPath string) error } +// trackLyricsProvider is implemented by providers that can fetch lyrics for a +// track independently of its metadata lookup. +// +// Tidal and Deezer attach lyrics inside GetMetadata, which album rips skip +// when they reuse the album payload's track objects. Qobuz exposes this +// instead, so lyrics work on both paths. +type trackLyricsProvider interface { + TrackLyrics(ctx context.Context, trackID string) (plain string, synced string, err error) +} + +// albumTracksSelfDescribing is implemented by providers whose album payload +// embeds track objects complete enough to tag from, making the per-track +// metadata call redundant. +// +// This is opt-in per provider rather than assumed, because it is only true +// where it has been checked against the live API, and because some providers +// do extra work inside their track lookup: Tidal, for instance, fetches lyrics +// there, which an album payload does not carry. +type albumTracksSelfDescribing interface { + AlbumTracksAreComplete() bool +} + type videoDownloadableProvider interface { GetVideoDownloadable(ctx context.Context, videoID string) (*provider.Downloadable, error) } @@ -253,6 +276,100 @@ func (m *Main) runTrackWorkers(ctx context.Context, ids []string, rip func(pos i return failures } +// metadataExcluded reports whether a tag was opted out via +// metadata.exclude in the config. +func (m *Main) metadataExcluded(field string) bool { + for _, k := range m.Config.Session.Metadata.Exclude { + if strings.EqualFold(strings.TrimSpace(k), field) { + return true + } + } + return false +} + +// attachLyrics fills in lyrics for providers that fetch them separately from +// track metadata. It is a no-op when the metadata already carries lyrics (as +// Tidal's and Deezer's does), when the provider has no lyrics support, or when +// the user excluded them. +func (m *Main) attachLyrics(ctx context.Context, p provider.Client, meta map[string]any, trackID string) { + if meta == nil || m.metadataExcluded("lyrics") { + return + } + if jsonutil.StringFromAny(meta["lyrics"]) != "" || jsonutil.StringFromAny(meta["lyrics_synced"]) != "" { + return + } + lp, ok := p.(trackLyricsProvider) + if !ok { + return + } + plain, synced, err := lp.TrackLyrics(ctx, trackID) + if err != nil { + // Lyrics are cosmetic; never fail a download over them. + m.logf("lyrics unavailable for id=%s: %v\n", trackID, err) + return + } + if plain != "" { + meta["lyrics"] = plain + } + if synced != "" { + meta["lyrics_synced"] = synced + } +} + +// trackMetaUsable reports whether a track object carried over from an album +// payload is complete enough to tag and name a file from. +// +// It is a belt-and-braces check on top of the provider opting in: title and +// track number determine the output filename, so if either is missing the +// caller refetches rather than writing a wrongly named file. +func trackMetaUsable(meta map[string]any) bool { + if len(meta) == 0 { + return false + } + if strings.TrimSpace(jsonutil.StringFromAny(meta["title"])) == "" { + return false + } + return jsonutil.IntFromAny(meta["track_number"]) > 0 +} + +// albumTrackMetadata builds the per-track metadata for an album rip from the +// track objects already embedded in the album payload. +// +// Verified against the live Qobuz API: the only field a dedicated track +// lookup adds that this app reads is the nested album object, which is the +// parent album we already have. The album copy has its own track list removed, +// so attaching it to every track does not duplicate the whole album n times. +func albumTrackMetadata(albumMeta map[string]any, rawItems []any) map[string]map[string]any { + albumForTrack := make(map[string]any, len(albumMeta)) + for k, v := range albumMeta { + if k == "tracks" { + continue + } + albumForTrack[k] = v + } + + out := make(map[string]map[string]any, len(rawItems)) + for _, raw := range rawItems { + item, ok := raw.(map[string]any) + if !ok { + continue + } + id := jsonutil.StringFromAny(item["id"]) + if id == "" { + continue + } + meta := make(map[string]any, len(item)+1) + for k, v := range item { + meta[k] = v + } + if _, has := meta["album"]; !has { + meta["album"] = albumForTrack + } + out[id] = meta + } + return out +} + // anySlice normalizes a JSON array that may have been decoded as either // []any or []map[string]any, which differs per provider because some clients // hand-build their metadata maps instead of round-tripping through encoding/json. @@ -760,6 +877,13 @@ func (m *Main) ripAlbum(ctx context.Context, p provider.Client, source, albumID if discTotal == 0 { discTotal = jsonutil.IntFromAny(albumMeta["numberOfVolumes"]) } + // Providers that say their album payload embeds complete track objects let + // us skip one throttled API call per track. + var embeddedMeta map[string]map[string]any + if sd, ok := p.(albumTracksSelfDescribing); ok && sd.AlbumTracksAreComplete() { + embeddedMeta = albumTrackMetadata(albumMeta, rawItems) + } + m.logf("Album: %s (%d tracks)\n", albumTitle, total) failures := m.runTrackWorkers(ctx, trackIDs, func(pos int, trackID string) error { opts := ripTrackOptions{ @@ -769,6 +893,7 @@ func (m *Main) ripAlbum(ctx context.Context, p provider.Client, source, albumID index: pos, total: total, albumDiscTotal: discTotal, + prefetchedMeta: embeddedMeta[trackID], } // The first track's downloadable was already fetched to decide the // album folder's audio values, so reuse it instead of refetching. @@ -937,12 +1062,22 @@ func (m *Main) ripTrack(ctx context.Context, p provider.Client, source, id, fall } } - meta, err := p.GetMetadata(ctx, id, "track") - if err != nil { - m.markFailed(ctx, source, "track", id) - return fmt.Errorf("id=%s metadata: %w", id, err) + // A prefetched track object from the album payload saves one API call per + // track. Every call goes through the request-per-minute limiter, so on a + // large album this is the difference between the rip being gated by the + // API and being gated by the network. + meta := opts.prefetchedMeta + var err error + if !trackMetaUsable(meta) { + meta, err = p.GetMetadata(ctx, id, "track") + if err != nil { + m.markFailed(ctx, source, "track", id) + return fmt.Errorf("id=%s metadata: %w", id, err) + } } + m.attachLyrics(ctx, p, meta, id) + title := titleFromMetadata(meta, id) if title == id && fallbackTitle != "" { title = fallbackTitle diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 99115cc..2a7a233 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -1138,3 +1138,405 @@ func TestBookkeepingSurvivesCancellation(t *testing.T) { t.Fatal("failure record not written under a cancelled context") } } + +// countingMetaProvider records how many per-track metadata lookups happen, so +// the album-payload optimization can be asserted rather than assumed. +type countingMetaProvider struct { + url string + complete bool + + mu sync.Mutex + trackMeta int + albumMeta int + downloads int +} + +func (f *countingMetaProvider) Source() string { return "qobuz" } +func (f *countingMetaProvider) Login(context.Context) error { return nil } +func (f *countingMetaProvider) LoggedIn() bool { return true } +func (f *countingMetaProvider) Close() error { return nil } +func (f *countingMetaProvider) Search(context.Context, string, string, int) ([]map[string]any, error) { + return nil, nil +} + +// AlbumTracksAreComplete is only consulted when complete is true; the false +// case still satisfies the interface, which mirrors a provider that opts out. +func (f *countingMetaProvider) AlbumTracksAreComplete() bool { return f.complete } + +func (f *countingMetaProvider) GetMetadata(_ context.Context, id string, mediaType string) (map[string]any, error) { + f.mu.Lock() + if mediaType == "album" { + f.albumMeta++ + } else { + f.trackMeta++ + } + f.mu.Unlock() + + if mediaType == "album" { + items := make([]any, 0, 3) + for i := 1; i <= 3; i++ { + items = append(items, map[string]any{ + "id": fmt.Sprintf("t%d", i), + "title": fmt.Sprintf("Track %d", i), + "track_number": float64(i), + "media_number": float64(1), + "isrc": fmt.Sprintf("ISRC%d", i), + "copyright": "(C) Label", + "performer": map[string]any{"name": "Fleetwood Mac"}, + }) + } + return map[string]any{ + "title": "Rumours", + "release_date_original": "1977-02-04", + "artist": map[string]any{"name": "Fleetwood Mac"}, + "tracks": map[string]any{"items": items}, + }, nil + } + + return map[string]any{ + "title": "Fetched " + id, + "track_number": float64(1), + "performer": map[string]any{"name": "Fleetwood Mac"}, + "album": map[string]any{"title": "Rumours"}, + }, nil +} + +func (f *countingMetaProvider) GetDownloadable(_ context.Context, _ string, _ int) (*provider.Downloadable, error) { + f.mu.Lock() + f.downloads++ + f.mu.Unlock() + return &provider.Downloadable{URL: f.url, Extension: "flac", Source: "qobuz"}, nil +} + +func (f *countingMetaProvider) counts() (album, track, downloads int) { + f.mu.Lock() + defer f.mu.Unlock() + return f.albumMeta, f.trackMeta, f.downloads +} + +func runAlbumRip(t *testing.T, prov *countingMetaProvider) string { + t.Helper() + tmp := t.TempDir() + d := config.DefaultConfigData() + d.Downloads.Folder = tmp + d.Downloads.SourceSubdirectories = false + d.Downloads.Concurrency = false + d.Artwork.Embed = false + d.Artwork.SaveArtwork = false + cfg := &config.Config{File: d, Session: d} + + m := &Main{ + Config: cfg, + Providers: map[string]provider.Client{"qobuz": prov}, + Store: store.NewDummy(), + DL: download.New(), + Tagger: noopTagger{}, + } + + ctx := context.Background() + if err := m.AddByID(ctx, "qobuz", "album", "a1"); err != nil { + t.Fatalf("AddByID() error = %v", err) + } + if err := m.Resolve(ctx); err != nil { + t.Fatalf("Resolve() error = %v", err) + } + if err := m.Rip(ctx); err != nil { + t.Fatalf("Rip() error = %v", err) + } + return tmp +} + +// TestAlbumRipSkipsPerTrackMetadataWhenEmbedded is the performance regression +// test: a provider that embeds complete track objects in its album payload +// must not be asked for each track again. Every such call goes through the +// requests-per-minute limiter, so on a 68-track album the redundant lookups +// were ~68 seconds of pure waiting. +func TestAlbumRipSkipsPerTrackMetadataWhenEmbedded(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("fLaC")) + })) + defer ts.Close() + + prov := &countingMetaProvider{url: ts.URL, complete: true} + tmp := runAlbumRip(t, prov) + + album, track, _ := prov.counts() + if album != 1 { + t.Errorf("album metadata fetches = %d, want 1", album) + } + if track != 0 { + t.Errorf("per-track metadata fetches = %d, want 0 (album payload is complete)", track) + } + + // The embedded titles must be what lands on disk, not the fetched ones. + albumDir := soleSubdir(t, tmp) + for i := 1; i <= 3; i++ { + name := fmt.Sprintf("%02d. Fleetwood Mac - Track %d.flac", i, i) + if _, err := os.Stat(filepath.Join(albumDir, name)); err != nil { + entries, _ := os.ReadDir(albumDir) + var got []string + for _, e := range entries { + got = append(got, e.Name()) + } + t.Fatalf("expected %q from the embedded metadata; album dir contains %v", name, got) + } + } +} + +// TestAlbumRipFetchesPerTrackMetadataWhenNotEmbedded pins the opt-out path, so +// providers like Tidal (whose track lookup also fetches lyrics) keep working. +func TestAlbumRipFetchesPerTrackMetadataWhenNotEmbedded(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("fLaC")) + })) + defer ts.Close() + + prov := &countingMetaProvider{url: ts.URL, complete: false} + runAlbumRip(t, prov) + + _, track, _ := prov.counts() + if track != 3 { + t.Errorf("per-track metadata fetches = %d, want 3 (provider opted out)", track) + } +} + +// TestAlbumTrackMetadataAttachesParentAlbum covers the one field the live +// Qobuz check showed track/get adds: the nested album object. +func TestAlbumTrackMetadataAttachesParentAlbum(t *testing.T) { + albumMeta := map[string]any{ + "title": "Rumours", + "replaygain_album_gain": -7.5, + "tracks": map[string]any{"items": []any{ + map[string]any{"id": "t1", "title": "Dreams", "track_number": float64(1)}, + }}, + } + items, _ := anySlice(albumMeta["tracks"].(map[string]any)["items"]) + + got := albumTrackMetadata(albumMeta, items) + meta, ok := got["t1"] + if !ok { + t.Fatal("no metadata built for t1") + } + alb, ok := meta["album"].(map[string]any) + if !ok { + t.Fatal("parent album not attached") + } + if alb["title"] != "Rumours" { + t.Errorf("album title = %v", alb["title"]) + } + // Album-level ReplayGain is read through trackMeta["album"]. + if alb["replaygain_album_gain"] != -7.5 { + t.Errorf("album replaygain lost: %v", alb["replaygain_album_gain"]) + } + // The attached album must not carry the whole track list back in. + if _, has := alb["tracks"]; has { + t.Error("attached album still has its track list; that is O(n^2) memory on a large album") + } +} + +func TestTrackMetaUsable(t *testing.T) { + tests := []struct { + name string + meta map[string]any + want bool + }{ + {"nil", nil, false}, + {"empty", map[string]any{}, false}, + {"complete", map[string]any{"title": "A", "track_number": float64(1)}, true}, + {"no title", map[string]any{"track_number": float64(1)}, false}, + {"blank title", map[string]any{"title": " ", "track_number": float64(1)}, false}, + {"no track number", map[string]any{"title": "A"}, false}, + {"zero track number", map[string]any{"title": "A", "track_number": float64(0)}, false}, + {"string track number", map[string]any{"title": "A", "track_number": "2"}, true}, + } + for _, tc := range tests { + if got := trackMetaUsable(tc.meta); got != tc.want { + t.Errorf("%s: trackMetaUsable = %v, want %v", tc.name, got, tc.want) + } + } +} + +// soleSubdir returns the single directory inside dir, which for an album rip +// is the generated album folder. +func soleSubdir(t *testing.T, dir string) string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + var dirs []string + for _, e := range entries { + if e.IsDir() { + dirs = append(dirs, filepath.Join(dir, e.Name())) + } + } + if len(dirs) != 1 { + t.Fatalf("expected exactly one album folder in %s, got %v", dir, dirs) + } + return dirs[0] +} + +// lyricsProvider is a countingMetaProvider that also serves lyrics, to prove +// lyrics still reach the tagger on the album fast path, where the per-track +// metadata call is skipped. +type lyricsProvider struct { + *countingMetaProvider + plain string + synced string + calls int + err error +} + +func (f *lyricsProvider) TrackLyrics(context.Context, string) (string, string, error) { + f.mu.Lock() + f.calls++ + f.mu.Unlock() + if f.err != nil { + return "", "", f.err + } + return f.plain, f.synced, nil +} + +type capturingTagger struct { + mu sync.Mutex + seen []tag.Metadata +} + +func (c *capturingTagger) TagFLAC(_ context.Context, _ string, meta tag.Metadata, _ string) error { + c.mu.Lock() + defer c.mu.Unlock() + c.seen = append(c.seen, meta) + return nil +} + +func (c *capturingTagger) lyrics() []string { + c.mu.Lock() + defer c.mu.Unlock() + out := make([]string, 0, len(c.seen)) + for _, m := range c.seen { + out = append(out, m.Lyrics) + } + return out +} + +func runAlbumRipWithTagger(t *testing.T, prov provider.Client, tagger trackTagger, exclude ...string) { + t.Helper() + d := config.DefaultConfigData() + d.Downloads.Folder = t.TempDir() + d.Downloads.Concurrency = false + d.Artwork.Embed = false + d.Artwork.SaveArtwork = false + d.Metadata.Exclude = exclude + cfg := &config.Config{File: d, Session: d} + + m := &Main{ + Config: cfg, + Providers: map[string]provider.Client{"qobuz": prov}, + Store: store.NewDummy(), + DL: download.New(), + Tagger: tagger, + } + ctx := context.Background() + if err := m.AddByID(ctx, "qobuz", "album", "a1"); err != nil { + t.Fatalf("AddByID() error = %v", err) + } + if err := m.Resolve(ctx); err != nil { + t.Fatalf("Resolve() error = %v", err) + } + if err := m.Rip(ctx); err != nil { + t.Fatalf("Rip() error = %v", err) + } +} + +func TestAlbumRipAttachesLyricsOnFastPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("fLaC")) + })) + defer ts.Close() + + prov := &lyricsProvider{ + countingMetaProvider: &countingMetaProvider{url: ts.URL, complete: true}, + plain: "line one\nline two", + synced: "[00:01.00]line one", + } + tagger := &capturingTagger{} + runAlbumRipWithTagger(t, prov, tagger) + + _, trackMeta, _ := prov.counts() + if trackMeta != 0 { + t.Errorf("per-track metadata fetches = %d, want 0 (fast path must be preserved)", trackMeta) + } + if prov.calls != 3 { + t.Errorf("TrackLyrics calls = %d, want 3 (one per track)", prov.calls) + } + + got := tagger.lyrics() + if len(got) != 3 { + t.Fatalf("tagged %d tracks, want 3", len(got)) + } + for i, l := range got { + // Synced lyrics win over plain, matching the existing behaviour. + if l != "[00:01.00]line one" { + t.Errorf("track %d lyrics = %q", i, l) + } + } +} + +func TestAlbumRipSkipsLyricsWhenExcluded(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("fLaC")) + })) + defer ts.Close() + + prov := &lyricsProvider{ + countingMetaProvider: &countingMetaProvider{url: ts.URL, complete: true}, + plain: "should not be fetched", + } + runAlbumRipWithTagger(t, prov, noopTagger{}, "lyrics") + + if prov.calls != 0 { + t.Errorf("TrackLyrics calls = %d, want 0 when metadata.exclude contains lyrics", prov.calls) + } +} + +// A lyrics failure is cosmetic and must never fail the download. +func TestAlbumRipSurvivesLyricsFailure(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("fLaC")) + })) + defer ts.Close() + + prov := &lyricsProvider{ + countingMetaProvider: &countingMetaProvider{url: ts.URL, complete: true}, + err: errors.New("lyrics service down"), + } + tagger := &capturingTagger{} + runAlbumRipWithTagger(t, prov, tagger) + + if n := len(tagger.lyrics()); n != 3 { + t.Fatalf("tagged %d tracks, want 3 despite the lyrics failure", n) + } + for i, l := range tagger.lyrics() { + if l != "" { + t.Errorf("track %d should have no lyrics, got %q", i, l) + } + } +} + +// Providers that already attach lyrics in their metadata (Tidal, Deezer) must +// not be asked again. +func TestLyricsNotRefetchedWhenMetadataAlreadyHasThem(t *testing.T) { + d := config.DefaultConfigData() + m := &Main{Config: &config.Config{File: d, Session: d}, DL: download.New()} + + prov := &lyricsProvider{countingMetaProvider: &countingMetaProvider{}, plain: "fresh"} + meta := map[string]any{"lyrics_synced": "[00:00.00]already here"} + m.attachLyrics(context.Background(), prov, meta, "t1") + + if prov.calls != 0 { + t.Errorf("TrackLyrics calls = %d, want 0 when metadata already carries lyrics", prov.calls) + } + if meta["lyrics_synced"] != "[00:00.00]already here" { + t.Errorf("existing lyrics overwritten: %v", meta["lyrics_synced"]) + } +} diff --git a/internal/provider/beatport/client.go b/internal/provider/beatport/client.go index 75045cd..f451db4 100644 --- a/internal/provider/beatport/client.go +++ b/internal/provider/beatport/client.go @@ -54,7 +54,7 @@ func New(cfg *config.Config) *Client { return &Client{ cfg: cfg, http: h, - limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute), + limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute, cfg.Session.Downloads.MaxConnections), baseURL: baseURL, } } diff --git a/internal/provider/deezer/client.go b/internal/provider/deezer/client.go index db2511a..dbf89f3 100644 --- a/internal/provider/deezer/client.go +++ b/internal/provider/deezer/client.go @@ -72,7 +72,7 @@ func New(cfg *config.Config) *Client { return &Client{ cfg: cfg, http: httpClient, - limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute), + limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute, cfg.Session.Downloads.MaxConnections), ua: randomDeezerUA(), deviceID: randomHexN(32), arl: strings.TrimSpace(cfg.Session.Deezer.ARL), diff --git a/internal/provider/qobuz/client.go b/internal/provider/qobuz/client.go index 2aed919..5e656b8 100644 --- a/internal/provider/qobuz/client.go +++ b/internal/provider/qobuz/client.go @@ -97,7 +97,7 @@ func New(cfg *config.Config) *Client { // read, so the 30s API client aborted any track that took longer than // 30s to stream. internal/download does the same for this reason. streamHTTP: netutil.NewHTTPClient(0, cfg.Session.Downloads.VerifySSL, cfg.Session.Downloads.MaxConnections), - limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute), + limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute, cfg.Session.Downloads.MaxConnections), baseURL: baseURL, fetchCfg: nil, } @@ -1507,3 +1507,164 @@ func (c *Client) mobileAppSecret() string { } return config.DefaultQobuzMobileAppSecret } + +// AlbumTracksAreComplete reports that Qobuz's album/get response embeds track +// objects complete enough to tag from, so internal/app can skip the per-track +// track/get call. +// +// Checked against the live API for a 68-track release: the embedded items +// carry title, version, track_number, media_number, parental_warning, isrc, +// copyright, performer, performers, composer, duration, audio_info, +// maximum_bit_depth, maximum_sampling_rate and release_date_original. The only +// field track/get adds that this app reads is the nested album object, and the +// caller attaches the parent album itself. Everything else track/get returns +// (articles, created_at, has_lyrics, indexed_at) is unused. +func (c *Client) AlbumTracksAreComplete() bool { return true } + +// Qobuz serves lyrics in two steps, mirroring its Android app: track/lyricsUrl +// returns a short-lived signed CloudFront URL, and that URL serves the lyrics +// document itself. +// +// Verified against the live API: +// +// GET track/lyricsUrl?track_id=&app_id=&request_ts=&request_sig= +// -> {"track_id","album_id","lyrics_url"} +// GET +// -> {"original":{"type","lang","lines":[{"line","start","end"}]}, +// "translation_langs":[...],"writers","publishers":[...]} +// +// "type" is plain, lsync (line-synced) or wsync (word-synced); start and end +// are milliseconds. Passing a language parameter returns a translation +// document instead of the original, and only for languages that exist for the +// track, so this deliberately omits it and always takes the original. +const lyricsURLEndpoint = "track/lyricsUrl" + +// lyricsExcluded reports whether the user opted out via +// metadata.exclude = ["lyrics"]. +func (c *Client) lyricsExcluded() bool { + for _, k := range c.cfg.Session.Metadata.Exclude { + if strings.EqualFold(strings.TrimSpace(k), "lyrics") { + return true + } + } + return false +} + +// TrackLyrics returns the plain and synced (LRC) lyrics for a track, or empty +// strings when Qobuz has none. It is exposed separately from GetMetadata so +// that album rips, which reuse the track objects embedded in the album +// payload, can still fetch lyrics without a full per-track metadata call. +func (c *Client) TrackLyrics(ctx context.Context, trackID string) (string, string, error) { + trackID = strings.TrimSpace(trackID) + if trackID == "" || c.lyricsExcluded() { + return "", "", nil + } + if !c.LoggedIn() { + return "", "", errNotLoggedIn + } + + requestTS := strconv.FormatInt(time.Now().Unix(), 10) + sigRaw := "tracklyricsUrltrack_id" + trackID + requestTS + c.secret + sum := md5.Sum([]byte(sigRaw)) + + params := url.Values{} + params.Set("app_id", c.cfg.Session.Qobuz.AppID) + params.Set("track_id", trackID) + params.Set("request_ts", requestTS) + params.Set("request_sig", hex.EncodeToString(sum[:])) + + resp, status, err := c.apiRequest(ctx, lyricsURLEndpoint, params, c.authHeaders()) + if err != nil { + return "", "", err + } + if status != http.StatusOK { + // A track without lyrics is not an error worth failing a rip over. + verbose.Printf(verbose.V, "qobuz: no lyrics for track %s (status=%d)\n", trackID, status) + return "", "", nil + } + docURL := strings.TrimSpace(jsonutil.StringFromAny(resp["lyrics_url"])) + if docURL == "" { + return "", "", nil + } + + return c.fetchLyricsDocument(ctx, trackID, docURL) +} + +// fetchLyricsDocument retrieves the signed lyrics document. The URL points at +// a CDN rather than the Qobuz API, so it deliberately bypasses the API rate +// limiter: it is a different host and does not count against the provider's +// request budget. +func (c *Client) fetchLyricsDocument(ctx context.Context, trackID, docURL string) (string, string, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, docURL, nil) + if err != nil { + return "", "", err + } + resp, err := c.http.Do(req) + if err != nil { + return "", "", err + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + // The signed URL 403s when the document does not exist, which is how + // Qobuz represents "no lyrics in this language". + verbose.Printf(verbose.V, "qobuz: lyrics document for track %s unavailable (status=%d)\n", trackID, resp.StatusCode) + return "", "", nil + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", "", err + } + var doc map[string]any + if err = json.Unmarshal(body, &doc); err != nil { + return "", "", fmt.Errorf("qobuz lyrics document: %w", err) + } + + plain, synced := parseQobuzLyrics(jsonutil.NestedMap(doc, "original")) + return plain, synced, nil +} + +// parseQobuzLyrics turns a lyrics object into plain text and, when the lines +// carry timings, an LRC transcript. +func parseQobuzLyrics(obj map[string]any) (string, string) { + lines, ok := obj["lines"].([]any) + if !ok || len(lines) == 0 { + return "", "" + } + // wsync documents are line objects that additionally carry per-word + // timings; the line text and start are in the same place, so both synced + // variants are handled identically. + synced := strings.EqualFold(jsonutil.StringFromAny(obj["type"]), "lsync") || + strings.EqualFold(jsonutil.StringFromAny(obj["type"]), "wsync") + + plainParts := make([]string, 0, len(lines)) + lrcParts := make([]string, 0, len(lines)) + timed := false + for _, raw := range lines { + m, ok := raw.(map[string]any) + if !ok { + continue + } + text := strings.TrimSpace(jsonutil.StringFromAny(m["line"])) + if text == "" { + continue + } + plainParts = append(plainParts, text) + if synced { + if start, has := m["start"]; has { + ms := jsonutil.IntFromAny(start) + lrcParts = append(lrcParts, fmt.Sprintf("[%02d:%05.2f]%s", ms/60000, float64(ms%60000)/1000.0, text)) + timed = true + } + } + } + if len(plainParts) == 0 { + return "", "" + } + lrc := "" + if timed { + lrc = strings.Join(lrcParts, "\n") + } + return strings.Join(plainParts, "\n"), lrc +} diff --git a/internal/provider/qobuz/client_test.go b/internal/provider/qobuz/client_test.go index cb30cad..52037ad 100644 --- a/internal/provider/qobuz/client_test.go +++ b/internal/provider/qobuz/client_test.go @@ -416,3 +416,165 @@ func testConfigData() config.ConfigData { d.Downloads.RequestsPerMinute = -1 return d } + +// newLyricsServer serves the two-step Qobuz lyrics flow: track/lyricsUrl +// returns a URL on this same server, which then serves the document. +func newLyricsServer(t *testing.T, doc any, docStatus int) *httptest.Server { + t.Helper() + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/track/lyricsUrl": + if r.URL.Query().Get("request_sig") == "" { + t.Error("track/lyricsUrl called without a request signature") + } + if r.URL.Query().Get("language") != "" { + t.Error("language must be omitted so the original lyrics are returned, not a translation") + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "track_id": r.URL.Query().Get("track_id"), + "album_id": "a1", + "lyrics_url": srv.URL + "/doc.json?Signature=x", + }) + case "/doc.json": + if docStatus != http.StatusOK { + w.WriteHeader(docStatus) + _, _ = w.Write([]byte(`AccessDenied`)) + return + } + _ = json.NewEncoder(w).Encode(doc) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(srv.Close) + return srv +} + +func lyricsClient(t *testing.T, srv *httptest.Server, exclude ...string) *Client { + t.Helper() + d := testConfigData() + d.Metadata.Exclude = exclude + c := New(&config.Config{File: d, Session: d}) + c.baseURL = srv.URL + c.loggedIn = true + c.secret = "secret" + return c +} + +func TestTrackLyricsLineSynced(t *testing.T) { + srv := newLyricsServer(t, map[string]any{ + "track_id": "42", + "translation_langs": []any{"fr", "de"}, + "original": map[string]any{ + "type": "lsync", + "lang": "en", + "lines": []any{ + map[string]any{"line": "First line", "start": 5780, "end": 8710}, + map[string]any{"line": "", "start": 9000, "end": 9100}, + map[string]any{"line": "Second line", "start": 65250, "end": 68000}, + }, + }, + }, http.StatusOK) + + plain, lrc, err := lyricsClient(t, srv).TrackLyrics(context.Background(), "42") + if err != nil { + t.Fatalf("TrackLyrics() error = %v", err) + } + if plain != "First line\nSecond line" { + t.Errorf("plain = %q", plain) + } + want := "[00:05.78]First line\n[01:05.25]Second line" + if lrc != want { + t.Errorf("lrc = %q, want %q", lrc, want) + } +} + +func TestTrackLyricsPlainHasNoTimestamps(t *testing.T) { + srv := newLyricsServer(t, map[string]any{ + "original": map[string]any{ + "type": "plain", + "lines": []any{ + map[string]any{"line": "Just text"}, + map[string]any{"line": "More text"}, + }, + }, + }, http.StatusOK) + + plain, lrc, err := lyricsClient(t, srv).TrackLyrics(context.Background(), "42") + if err != nil { + t.Fatal(err) + } + if plain != "Just text\nMore text" { + t.Errorf("plain = %q", plain) + } + if lrc != "" { + t.Errorf("plain lyrics must not produce an LRC transcript, got %q", lrc) + } +} + +func TestTrackLyricsWordSyncedUsesLineTimings(t *testing.T) { + srv := newLyricsServer(t, map[string]any{ + "original": map[string]any{ + "type": "wsync", + "lines": []any{ + map[string]any{ + "line": "Hello world", + "start": 1500, + "words": []any{ + map[string]any{"text": "Hello", "start": 1500, "end": 1800}, + map[string]any{"text": "world", "start": 1800, "end": 2100}, + }, + }, + }, + }, + }, http.StatusOK) + + plain, lrc, err := lyricsClient(t, srv).TrackLyrics(context.Background(), "42") + if err != nil { + t.Fatal(err) + } + if plain != "Hello world" { + t.Errorf("plain = %q", plain) + } + if lrc != "[00:01.50]Hello world" { + t.Errorf("lrc = %q", lrc) + } +} + +// A missing document 403s on the signed URL; that is how Qobuz says "no +// lyrics", so it must not fail the rip. +func TestTrackLyricsMissingDocumentIsNotAnError(t *testing.T) { + srv := newLyricsServer(t, nil, http.StatusForbidden) + + plain, lrc, err := lyricsClient(t, srv).TrackLyrics(context.Background(), "42") + if err != nil { + t.Fatalf("a missing lyrics document must not be an error, got %v", err) + } + if plain != "" || lrc != "" { + t.Errorf("expected no lyrics, got plain=%q lrc=%q", plain, lrc) + } +} + +func TestTrackLyricsRespectsMetadataExclude(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("no request should be made when lyrics are excluded, got %s", r.URL.Path) + })) + defer srv.Close() + + plain, lrc, err := lyricsClient(t, srv, "lyrics").TrackLyrics(context.Background(), "42") + if err != nil || plain != "" || lrc != "" { + t.Fatalf("exclude should short-circuit; got plain=%q lrc=%q err=%v", plain, lrc, err) + } +} + +func TestTrackLyricsEmptyIDIsNoOp(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("no request expected for an empty track id, got %s", r.URL.Path) + })) + defer srv.Close() + + if plain, lrc, err := lyricsClient(t, srv).TrackLyrics(context.Background(), " "); err != nil || plain != "" || lrc != "" { + t.Fatalf("got plain=%q lrc=%q err=%v", plain, lrc, err) + } +} diff --git a/internal/provider/soundcloud/client.go b/internal/provider/soundcloud/client.go index 593178c..ba374c9 100644 --- a/internal/provider/soundcloud/client.go +++ b/internal/provider/soundcloud/client.go @@ -52,7 +52,7 @@ func New(cfg *config.Config) *Client { // downloads.max_connections, and bypassed the transport that logs // requests at -vv with credentials redacted. http: netutil.NewHTTPClient(20*time.Second, cfg.Session.Downloads.VerifySSL, cfg.Session.Downloads.MaxConnections), - limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute), + limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute, cfg.Session.Downloads.MaxConnections), cache: map[string]map[string]any{}, } } diff --git a/internal/provider/tidal/client.go b/internal/provider/tidal/client.go index 33709b9..92af5ce 100644 --- a/internal/provider/tidal/client.go +++ b/internal/provider/tidal/client.go @@ -68,7 +68,7 @@ func New(cfg *config.Config) *Client { return &Client{ cfg: cfg, http: netutil.NewHTTPClient(30*time.Second, cfg.Session.Downloads.VerifySSL, cfg.Session.Downloads.MaxConnections), - limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute), + limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute, cfg.Session.Downloads.MaxConnections), baseURL: baseURL, lyricsAPI: lyricsAPIv1, openAPI: openAPIV2, diff --git a/internal/provider/yandex/client.go b/internal/provider/yandex/client.go index a20b20e..d3ea6f9 100644 --- a/internal/provider/yandex/client.go +++ b/internal/provider/yandex/client.go @@ -54,7 +54,7 @@ func New(cfg *config.Config) *Client { return &Client{ cfg: cfg, http: netutil.NewHTTPClient(30*time.Second, cfg.Session.Downloads.VerifySSL, cfg.Session.Downloads.MaxConnections), - limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute), + limiter: ratelimit.New(cfg.Session.Downloads.RequestsPerMinute, cfg.Session.Downloads.MaxConnections), baseURL: baseURL, userID: strings.TrimSpace(cfg.Session.Yandex.UserID), } diff --git a/internal/ratelimit/limiter.go b/internal/ratelimit/limiter.go index 460dbcc..1b27fbf 100644 --- a/internal/ratelimit/limiter.go +++ b/internal/ratelimit/limiter.go @@ -1,3 +1,5 @@ +// Package ratelimit provides the API request throttle shared by the provider +// clients. package ratelimit import ( @@ -6,20 +8,54 @@ import ( "time" ) +// Limiter is a token bucket: it enforces a sustained rate of +// requestsPerMinute while allowing up to burst requests to proceed +// immediately when budget has accrued. +// +// It used to space every request exactly interval apart, with no burst. That +// made max_connections meaningless for anything that goes through the API: +// with the default 60 requests/minute, six download workers each needing a +// metadata call and a stream-URL call could only start one track per two +// seconds, so an album appeared to download serially even though the transfers +// themselves were fast. type Limiter struct { interval time.Duration + burst int - mu sync.Mutex + mu sync.Mutex + // next is the time the next token becomes available. It is allowed to + // trail now by up to (burst-1)*interval, which is what accrues burst + // budget while no requests are being made. next time.Time } -func New(requestsPerMinute int) *Limiter { +// New returns a limiter allowing requestsPerMinute sustained, with burst +// requests permitted back-to-back. A non-positive requestsPerMinute disables +// throttling entirely. +// +// burst is clamped to at least 1 and to at most one minute's worth of +// requests, so a very low rate cannot be bypassed by a large burst. +func New(requestsPerMinute int, burst int) *Limiter { if requestsPerMinute <= 0 { return &Limiter{} } - return &Limiter{interval: time.Minute / time.Duration(requestsPerMinute)} + if burst < 1 { + burst = 1 + } + if burst > requestsPerMinute { + burst = requestsPerMinute + } + return &Limiter{ + interval: time.Minute / time.Duration(requestsPerMinute), + burst: burst, + } } +// Wait blocks until the caller may issue a request, or until ctx is done. +// +// Budget is reserved at call time, so a cancelled Wait still consumes its +// slot. That keeps the sustained rate honest under cancellation, at the cost +// of briefly under-using the budget. func (l *Limiter) Wait(ctx context.Context) error { if l.interval <= 0 { return nil @@ -27,22 +63,21 @@ func (l *Limiter) Wait(ctx context.Context) error { l.mu.Lock() now := time.Now() - if l.next.IsZero() { - l.next = now.Add(l.interval) - l.mu.Unlock() - return nil + // Accrue budget for idle time, capped at burst tokens. + earliest := now.Add(-time.Duration(l.burst-1) * l.interval) + if l.next.Before(earliest) { + l.next = earliest } - wake := l.next - if now.After(wake) { - l.next = now.Add(l.interval) - l.mu.Unlock() - return nil - } l.next = l.next.Add(l.interval) l.mu.Unlock() - timer := time.NewTimer(time.Until(wake)) + delay := time.Until(wake) + if delay <= 0 { + return nil + } + + timer := time.NewTimer(delay) defer timer.Stop() select { diff --git a/internal/ratelimit/limiter_test.go b/internal/ratelimit/limiter_test.go index d995545..a589469 100644 --- a/internal/ratelimit/limiter_test.go +++ b/internal/ratelimit/limiter_test.go @@ -2,19 +2,25 @@ package ratelimit import ( "context" + "sync" "testing" "time" ) func TestLimiterDisabled(t *testing.T) { - l := New(-1) - if err := l.Wait(context.Background()); err != nil { - t.Fatalf("Wait() error = %v", err) + for _, rpm := range []int{-1, 0} { + l := New(rpm, 6) + for i := 0; i < 10; i++ { + if err := l.Wait(context.Background()); err != nil { + t.Fatalf("New(%d).Wait() error = %v", rpm, err) + } + } } } func TestLimiterContextCancel(t *testing.T) { - l := New(1) + // burst 1 means strict spacing, so the second request must block. + l := New(1, 1) if err := l.Wait(context.Background()); err != nil { t.Fatalf("first Wait() error = %v", err) } @@ -26,3 +32,85 @@ func TestLimiterContextCancel(t *testing.T) { t.Fatalf("expected context error") } } + +// TestLimiterAllowsBurst is the regression test for album rips appearing +// serial: with the old strict-spacing limiter, max_connections workers could +// not get their API calls through concurrently, so only one track downloaded +// at a time. +func TestLimiterAllowsBurst(t *testing.T) { + const burst = 6 + l := New(60, burst) // the shipped defaults + + start := time.Now() + for i := 0; i < burst; i++ { + if err := l.Wait(context.Background()); err != nil { + t.Fatalf("Wait() %d error = %v", i, err) + } + } + elapsed := time.Since(start) + + // 60 rpm is one token per second; the burst must not have waited. + if elapsed > 200*time.Millisecond { + t.Fatalf("%d burst requests took %v, want them to proceed immediately", burst, elapsed) + } +} + +// TestLimiterEnforcesSustainedRate checks the burst does not become a way to +// exceed the configured rate: once the bucket is drained, requests are spaced +// by the interval again. +func TestLimiterEnforcesSustainedRate(t *testing.T) { + // 6000 rpm = 10ms per token, so this stays fast. + const burst = 3 + l := New(6000, burst) + + ctx := context.Background() + for i := 0; i < burst; i++ { + if err := l.Wait(ctx); err != nil { + t.Fatal(err) + } + } + + // The bucket is drained; the next two must each wait about one interval. + start := time.Now() + for i := 0; i < 2; i++ { + if err := l.Wait(ctx); err != nil { + t.Fatal(err) + } + } + elapsed := time.Since(start) + if elapsed < 15*time.Millisecond { + t.Fatalf("post-burst requests took %v, want them throttled to ~10ms each", elapsed) + } +} + +// TestLimiterBurstCappedByRate stops a large max_connections from letting a +// deliberately tiny rate through in one go. +func TestLimiterBurstCappedByRate(t *testing.T) { + l := New(2, 64) + if l.burst != 2 { + t.Fatalf("burst = %d, want it clamped to the rate (2)", l.burst) + } +} + +func TestLimiterConcurrentCallersShareTheRate(t *testing.T) { + const burst = 8 + l := New(6000, burst) + + var wg sync.WaitGroup + start := time.Now() + for i := 0; i < burst; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if err := l.Wait(context.Background()); err != nil { + t.Errorf("Wait() error = %v", err) + } + }() + } + wg.Wait() + + // All within the burst, so none should have blocked meaningfully. + if elapsed := time.Since(start); elapsed > 200*time.Millisecond { + t.Fatalf("%d concurrent burst requests took %v", burst, elapsed) + } +}