-
Notifications
You must be signed in to change notification settings - Fork 42
Updates for improving Jellyfin track matching and downloading #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
3fa7ba4
2800b65
4076d2f
9a06abb
b15e075
3122c75
28db79c
f4b0ea6
1ad0260
8637cee
2902fbc
d13209a
d249485
884e29e
fdbb276
5b4e924
85f4f0b
3176d5d
434945c
316e406
70570ca
32bd951
9b6996e
86593b1
cb6a5d1
152704d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,8 @@ type Audios struct { | |
| } | ||
|
|
||
| type ProviderIds struct { | ||
| MusicBrainzTrack string `json:"MusicBrainzTrack"` | ||
| MusicBrainzTrack string `json:"MusicBrainzTrack"` | ||
| MusicBrainzRecording string `json:"MusicBrainzRecording"` | ||
| } | ||
|
|
||
| type Items struct { | ||
|
|
@@ -150,8 +151,13 @@ func (c *Jellyfin) CheckRefreshState() bool { | |
|
|
||
| func (c *Jellyfin) SearchSongs(tracks []*models.Track) error { | ||
| for _, track := range tracks { | ||
| reqParam := fmt.Sprintf("/Items?IncludeMediaTypes=Audio&SearchTerm=%s&Recursive=true&Fields=Path,ProviderIDs", url.QueryEscape(util.CleanSearchTitle(track.CleanTitle))) | ||
| // Clean typography drift from the search parameters | ||
| cleanSearchTitle := strings.ReplaceAll(track.CleanTitle, "’", "'") | ||
| cleanSearchTitle = strings.ReplaceAll(cleanSearchTitle, "`", "'") | ||
| cleanSearchTitle = strings.TrimSpace(cleanSearchTitle) | ||
|
|
||
| // 1. Fetch candidates using the standard search term | ||
| reqParam := fmt.Sprintf("/Items?IncludeMediaTypes=Audio&SearchTerm=%s&Recursive=true&Limit=300&Fields=Path,ProviderIDs", url.QueryEscape(cleanSearchTitle)) | ||
| body, err := c.HttpClient.MakeRequest("GET", c.Cfg.URL+reqParam, nil, c.Cfg.Creds.Headers) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -161,23 +167,67 @@ func (c *Jellyfin) SearchSongs(tracks []*models.Track) error { | |
| if err = util.ParseResp(body, &results); err != nil { | ||
| return err | ||
| } | ||
| normalizedCleanTitle := util.NormalizeTitle(track.CleanTitle) | ||
| for _, item := range results.Items { | ||
|
|
||
| normalizedItemTitle := util.NormalizeTitle(item.Name) | ||
|
|
||
| musicBrainzMatch := track.MusicBrainzTrackID != "" && item.ProviderIds.MusicBrainzTrack == track.MusicBrainzTrackID | ||
| titleMatch := normalizedItemTitle == normalizedCleanTitle | ||
| artistMatch := strings.EqualFold(item.AlbumArtist, track.MainArtist) || (len(item.Artists) > 0 && strings.EqualFold(item.Artists[0], track.MainArtist)) | ||
| pathMatch := util.ContainsFold(item.Path,track.File) | ||
| // 2. Robust Root-Word Fallback: If primary search returns 0 results, | ||
| // isolate the very first continuous block of alphanumeric characters (stops at spaces, apostrophes, etc.) | ||
| if len(results.Items) == 0 && len(cleanSearchTitle) > 0 { | ||
| endIdx := 0 | ||
| for i, r := range cleanSearchTitle { | ||
| if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') { | ||
| endIdx = i + 1 | ||
| } else if endIdx > 0 { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if musicBrainzMatch || (titleMatch && artistMatch) { | ||
| track.ID = item.ID | ||
| track.Present = true | ||
| break | ||
| if endIdx > 0 { | ||
| firstWord := cleanSearchTitle[:endIdx] | ||
| broadParam := fmt.Sprintf("/Items?IncludeMediaTypes=Audio&SearchTerm=%s&Recursive=true&Limit=300&Fields=Path,ProviderIDs", url.QueryEscape(firstWord)) | ||
| broadBody, err := c.HttpClient.MakeRequest("GET", c.Cfg.URL+broadParam, nil, c.Cfg.Creds.Headers) | ||
| if err == nil { | ||
| _ = util.ParseResp(broadBody, &results) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if track.File != "" && artistMatch && pathMatch { | ||
| targetAlnumTitle := util.NormalizeTitle(track.CleanTitle) | ||
| rawIncomingArtist := strings.ToLower(track.MainArtist) | ||
| normalizedMainArtist := util.NormalizeArtist(util.StripFeat(track.MainArtist)) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mainArtist doesn't need NormalizeArtist() or StripFeat() here. It is always a single primary artist that released the track. |
||
|
|
||
| for _, item := range results.Items { | ||
| currentAlnumItemTitle := util.NormalizeTitle(item.Name) | ||
|
|
||
| // 1. Strict MusicBrainz Recording/Track ID Match | ||
| musicBrainzMatch := track.MusicBrainzTrackID != "" && | ||
| (item.ProviderIds.MusicBrainzRecording == track.MusicBrainzTrackID || | ||
| item.ProviderIds.MusicBrainzTrack == track.MusicBrainzTrackID) | ||
|
|
||
| // 2. Pure Alphanumeric Title Match | ||
| titleMatch := currentAlnumItemTitle == targetAlnumTitle | ||
|
|
||
| // 3. Substring-Aware Artist Matching | ||
| artistMatch := false | ||
| if normalizedMainArtist != "" { | ||
| normalizedAlbumArtist := util.NormalizeArtist(item.AlbumArtist) | ||
|
|
||
| if normalizedAlbumArtist == normalizedMainArtist || | ||
| strings.Contains(normalizedMainArtist, normalizedAlbumArtist) || | ||
| strings.Contains(normalizedAlbumArtist, normalizedMainArtist) { | ||
| artistMatch = true | ||
| } else { | ||
| for _, individualArtist := range item.Artists { | ||
| normalizedIndiv := util.NormalizeArtist(individualArtist) | ||
| if normalizedIndiv == normalizedMainArtist || | ||
| strings.Contains(normalizedMainArtist, normalizedIndiv) || | ||
| util.ContainsFold(rawIncomingArtist, individualArtist) { | ||
| artistMatch = true | ||
| break | ||
| } | ||
| } | ||
|
Comment on lines
+218
to
+226
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could probably be simplified with slices.ContainsFunc() |
||
| } | ||
| } | ||
|
|
||
| if musicBrainzMatch || (titleMatch && artistMatch) { | ||
| track.ID = item.ID | ||
| track.Present = true | ||
| break | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,23 @@ type Downloader interface { | |
| Monitor | ||
| } | ||
|
|
||
| // retry runs fn up to attempts times with linear backoff, returning nil on the | ||
| // first success. Used for transient network failures on query/download. | ||
| func retry(attempts int, baseDelay time.Duration, label string, fn func() error) error { | ||
| var err error | ||
| for i := 1; i <= attempts; i++ { | ||
| if err = fn(); err == nil { | ||
| return nil | ||
| } | ||
| if i < attempts { | ||
| slog.Warn("retrying after failure", | ||
| "step", label, "attempt", i, "max", attempts, "context", err.Error()) | ||
| time.Sleep(time.Duration(i) * baseDelay) | ||
| } | ||
| } | ||
| return err | ||
| } | ||
|
Comment on lines
+38
to
+51
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think retrying should be handled globally for all downloaders. For example, slskd already has its own retry logic for search and download attempts, and different providers may need different retry functions. For YouTube specifically, multiple retries could have some downsides:
If the issue is mainly timeout-related, maybe increasing |
||
|
|
||
| // get download services from config and append them to DownloadClient | ||
| func NewDownloader(cfg *cfg.DownloadConfig, httpClient *util.HttpClient, filterLocal bool) (*DownloadClient, error) { | ||
| var downloader []Downloader | ||
|
|
@@ -86,20 +103,31 @@ func (c *DownloadClient) StartDownload(tracks *[]*models.Track) { | |
| return err | ||
| } | ||
|
|
||
| if err := d.QueryTrack(track); err != nil { | ||
| attempts := c.Cfg.DownloadAttempts | ||
| if attempts < 1 { | ||
| attempts = 1 | ||
| } | ||
|
|
||
| if err := retry(attempts, 3*time.Second, "query", func() error { | ||
| if werr := limiter.Wait(ctx); werr != nil { | ||
| return werr | ||
| } | ||
| return d.QueryTrack(track) | ||
| }); err != nil { | ||
| slog.Warn(err.Error()) | ||
| return nil | ||
| } | ||
|
|
||
| if err := limiter.Wait(ctx); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := d.GetTrack(track); err != nil { | ||
|
|
||
| if err := retry(attempts, 3*time.Second, "download", func() error { | ||
| if werr := limiter.Wait(ctx); werr != nil { | ||
| return werr | ||
| } | ||
| return d.GetTrack(track) | ||
| }); err != nil { | ||
| slog.Warn(err.Error()) | ||
| return nil | ||
| } | ||
|
|
||
| return nil | ||
| }) | ||
| } | ||
|
|
@@ -382,4 +410,4 @@ func isDirEmpty(path string) (bool, error) { | |
| return true, nil // no entries | ||
| } | ||
| return false, err | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having a fuzzy fallback, but I'd rather keep it generic instead of implementing it only for jellyfin. It feels like something that could be shared across clients via a helper, with each client only performing the actual search.