Conversation
LumePart
left a comment
LumePart
left a comment
There was a problem hiding this comment.
I think there are some good ideas here, especially around improving track matching.
I've left some comments on areas where the PR needs to be improved. Some of the changes make sense in isolation, but some functions need to be refactored to better fit the existing codebase.
Feel free to address those changes, or alternatively I can take some of these ideas and implement them in a way that fits better with the existing architecture.
| if track.File != "" && artistMatch && pathMatch { | ||
| targetAlnumTitle := util.NormalizeTitle(track.CleanTitle) | ||
| rawIncomingArtist := strings.ToLower(track.MainArtist) | ||
| normalizedMainArtist := util.NormalizeArtist(util.StripFeat(track.MainArtist)) |
There was a problem hiding this comment.
mainArtist doesn't need NormalizeArtist() or StripFeat() here. It is always a single primary artist that released the track.
| for _, individualArtist := range item.Artists { | ||
| normalizedIndiv := util.NormalizeArtist(individualArtist) | ||
| if normalizedIndiv == normalizedMainArtist || | ||
| strings.Contains(normalizedMainArtist, normalizedIndiv) || | ||
| util.ContainsFold(rawIncomingArtist, individualArtist) { | ||
| artistMatch = true | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Could probably be simplified with slices.ContainsFunc()
| 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 | ||
| } |
There was a problem hiding this comment.
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 using the official API, it could consume API tokens quickly.
- More requests to youtube will result in more often rate-limiting
If the issue is mainly timeout-related, maybe increasing DOWNLOAD_LIMITER by a second or two would be a better first step (default is currently 1). It could be that multiple frequent operations are causing the timeouts.
| // 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) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
2 participants
After hitting my head against a wall trying to get track matching to work nicely in Jellyfin, ran this through Gemini several times to get the hit rate up to 100%.
Improves Jellyfin client track searching mechanism, resolving issues where Explo repeatedly logs [jellyfin] failed to find X for songs that are already present in the media library.
Changes (mainly jellyfin.go):
Alphanumeric Matching:
Replaced string matches with alphanumeric-only normalization comparisons on both sides (currentAlnumItemTitle == targetAlnumTitle) to bypass typography differences (e.g., curly vs. straight apostrophes like Where’d and Where'd).
Array/Substring Multi-Artist Tracking:
Upgraded the artist matching verification block to evaluate elements against Jellyfin’s discrete split-string JSON array slices (item.Artists). This handles collaborations cleanly (e.g., Artist A & Artist B vs Artist A feat. Artist B).
Result Pool Expansion:
Increased the API query fetch scope to Limit=300 to prevent Jellyfin from truncating matching tracks inside generic search pools.
Fuzzy Root-Word Fallback Strategy:
Implemented a non-alphanumeric split fail-safe. If an exact database query fails due to complex brackets or parentheses syntax breaking the index parser, the code automatically falls back to searching Jellyfin by the track's first continuous block of alphanumeric characters (e.g., matching Rollin' (Air Raid Vehicle) seamlessly via a root search for Rollin).
Changes to downloader.go:
Adding retry attempts due to YT time outs. 49/50 success rater, which was good enough for me to move on to dealing with all of the above items.
Change to config.go:
Just adding attempts value.
Change to sanitize.go:
Changes to string normalization. This is where the first changes were attempted, but eventually the lion's share ended up in jellyfin.go.