moehmeni / syncedlyrics

Get an LRC format (synchronized) lyrics for your music
MIT License
188 stars 18 forks source link

LRClib using first result to have lyrics, not best match #46

Closed jmaximusix closed 3 weeks ago

jmaximusix commented 1 month ago

This part of the code puzzled me, because this seems clearly flawed / leads to unexpected behaviour.

https://github.com/moehmeni/syncedlyrics/blob/738f0157b2dbca5080657bd6d8121779beebab48/syncedlyrics/providers/lrclib.py#L38-L44

Why choose the first track that has synced lyrics, which very likely may not be the track you're looking for. Especially, when there's still the option to search on other providers.

Besides, I'd rather get no lyrics for a song than wrong ones. Apparently this was a deliberate change. Why? Am I missing something?

moehmeni commented 3 weeks ago

I remember there were some lyrics without synced version. Maybe a better option would be defining a min_score for sort to remove items under that score and then return. After that we can just grab the first one that has a synced lyrics like this loop.

jmaximusix commented 3 weeks ago

Okay, but does LRClib actually return multiple different results for the same song? With the examples I was testing I got one result for the correct song and the next one was a different song (therefore shouldn't be considered) But I didn't test too many songs yet

moehmeni commented 3 weeks ago

Yeah and indeed it does not have any search sorting method it seem. For example I put a log to print the result and if I search with:

syncedlyrics "clan of xymox spider on the wall" -p lrclib

It lists:

Clan of Xymox - She
Clan of Xymox - Lovers
Clan of Xymox - I Don't Like Myself
Clan of Xymox - Spider On the Wall
Clan of Xymox - Black Mirror
Clan of Xymox - Into the Unknown
Clan of Xymox - All I Ever Know
Clan of Xymox - When We Were Young
Clan of Xymox - My New Lows
Clan of Xymox - See You On The Other Side

while we want the fourth one.

tranxuanthang commented 3 weeks ago

@moehmeni

Yeah and indeed it does not have any search sorting method it seem.

LRCLIB author here 👋 In this case, there are many matches because spider on the wall is also the album name. You can get better results by specifying that the keyword must be in the track_name, like this:

https://lrclib.net/api/search?artist_name=clan+of+xymox&track_name=spider+on+the+wall

Edit: The results is now much better after commit https://github.com/tranxuanthang/lrclib/commit/317986eef318e8e64a5a85283692004f7e61ce94, even with query q search:

https://lrclib.net/api/search?q=clan%20of%20xymox%20spider%20on%20the%20wall

jmaximusix commented 3 weeks ago

@tranxuanthang oh thanks, yeah I was so confused right now since I literally couldn't reproduce @moehmeni 's example, in that particular case it already appears to be fixed, and I haven't encountered any issues myself when using a somewhat precise query.

I would still strongly suggest merging #48 for now, even if there are still issues, this method of grabbing the first song with synced lyrics seems pretty arbitrary to me, it would almost be a coincidence if that happens to be the one you were looking for (and as I said, I'd prefer not to get lyrics over wrong ones) Then, if this is still should still be an issue (maybe it'll work just fine), we could look into solutions for getting better results, like @tranxuanthang mentioned there are specific query parameters for track_name or artist but since other providers don't support that it would mean significant refactoring.