Rework slskd search + download reliability - #212
Conversation
LumePart
left a comment
LumePart
left a comment
There was a problem hiding this comment.
Some good changes here, especially the retry functionality, left a few comments that should be looked over
| streams := []*ffmpeg.Stream{ | ||
| ffmpeg.Input(srcFile), | ||
| "c": "copy", | ||
| "map_metadata": "-1", |
There was a problem hiding this comment.
I think erasing all the file metadata is wrong at the moment, I'd prefer just overwriting the tags Explo has metadata for, the file itself could have some useful metadata already embedded (i.e custom playlist track could already have a MBID set) or some tracks might have lyrics embedded.
| return err | ||
| func splitArtists(artist string) []string { | ||
| norm := artist | ||
| for _, sep := range []string{" featuring ", " feat.", " feat ", " ft.", " ft ", " with ", " & ", " x ", ",", "/", "+", "&"} { |
There was a problem hiding this comment.
Using "," or ", " will create invalid artists, same goes for "/", "&", "+" and " with ".
Some examples:
Tyler, the Creator
&ME
Mumford & Sons
+44
Brick + Mortar
AC/DC
They can return vastly different search results which means wrong tracks being downloaded
| return nil | ||
| add(fmt.Sprintf("%s - %s", track.CleanTitle, track.Artist)) | ||
| var names []string | ||
| for _, n := range splitArtists(track.MainArtist) { |
There was a problem hiding this comment.
MainArtist should always be a single artist. Splitting should be done at playlist level (if needed)
There was a problem hiding this comment.
If track.Artist was meant to be used, I don't think searching by each featured artist is safe. A featured artist can:
- have released another track with the same name themselves
- be featured on a completely different artist's track with the same name
In both cases, a query like <track title> <featured artist> can return an unrelated track and increase the chance of downloading the wrong file (especially with the matching changes).
| if a.rank != b.rank { | ||
| return a.rank - b.rank | ||
| } | ||
| return b.file.BitRate - a.file.BitRate |
There was a problem hiding this comment.
should probably add BitDepth ranking as well, as lossless formats don't have BitRate, and lossy formats don't have bitdepth defined (in slskd atleast)
| return true | ||
| } | ||
| for _, tok := range artistTokens { | ||
| if containsLower(sanitizedFilename, tok) { |
There was a problem hiding this comment.
This will increase the chance of downloading the wrong track. In my opinion, the current matching is already fairly loose (checking only the main artist), so matching against featured artists as well makes it even more likely that an unrelated version of the track will pass.
2 participants
This PR adds some reliability fixes and file matching that VASTLY improves download success, and improves auto-tagging for downloaded tracks! A 25 track playlist will have 25/25 tracks consistently being ripped now with the correct tags, unless the song literally does NOT exist on slskd!
slskd.go
•
searchStatuswas a recursive retry that bailed before slskd had even finished searching.SLSKD_RETRY=2is only 30s, but slskd routinely takes 60s+ to wrap up a search, so we were giving up on tracks that were about to return results. Instead of waiting around for a set time, it now polls on a deadline and actually wait for the search to reportIsComplete. This one change took me from ~9/25 tracks, to 12/25 downloading by itself.• Added
searchQueries, instead of searching one and done, it tries a few combinations in order:So now there's more than one chance to actually pick up a song, and I set them up so 99% of tracks get picked up by the end of the list.
•
GetTracknow retries the /responses endpoint a few times. It lags behind the search summary, so we were occasionally reading an empty response and marking a track dead right when the files were about to show up.CollectFilesis where most of the matching improvements are:• Artist matching is now token based, so a file credited to just one artist of a collab still matches (ex.
Moon - Daniel Caesar feat. Bon Ivernow matches a file likeDaniel Caesar - Mooninstead of getting skipped for not having both names)• Titles are now matched against the files base name, instead of the entire path, so a title word can't accidentally be marked as a successful grab incorrectly.
• Files from busy peers are kept as fallbacks now instead of being thrown away/skipped
monitor.go
•
monitor.goused to just skip a track the moment its download errored. Now it tries the next candidate source viaRetryDownloadand only gives up once there's literally NONE that match. One dead download no longer kills the whole track!downloader.go + metadata.go
• I merged the finalize path into
FinalizeDownloadindownloader.goso tags get written regardless if files are migrated. I saw some people having issues with that too.• Downloaded tracks have their albums grouped under the real lead artist now, so they don't end being imported as "Various Artists". That bug is effectively dead now :)
SO many changes I know, but after all of this, almost all of the playlists I tested ACTUALLY came back with 25/25 tracks, with their proper artist info tagged and everything.
heads up most of these metadata changes only apply with Auto-tag songs is enabled in settings! But EVERY playlist now benefits from these changes, LB, Apple Music AND Spotify. :) Lmk if I can answer any questions. Theres a second PR that will be pushed soon that improves tagging and searching for custom playlists soon. I just split these both up because otherwise it'd be a massive PR lol.