-
Notifications
You must be signed in to change notification settings - Fork 7
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
Synced lyrics support #87
base: main
Are you sure you want to change the base?
Conversation
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.
This fixes a bug in the playlist fetching code, where the response can sometimes be nil by the time this is updated -- heavy network congestion, mostly. It doesn't belong in this PR, per se, but if this PR is merged, it addresses an issue. The change should be made regardless.
…n aborted start at support. Signed-off-by: Sean E. Russell <[email protected]>
adds: ability to also write logs to a log file Signed-off-by: Sean E. Russell <[email protected]>
… that makes up for the inaccuracy of mpv Signed-off-by: Sean E. Russell <[email protected]>
Signed-off-by: Sean E. Russell <[email protected]>
I finally set up commit signing. ¯\_(ツ)_/¯ |
…ren't updating from song to song unless the album art also changed.
One thing this is missing is checking if the server actually supports lyrics. Lyrics are an extension, not baked into the base API as specified here. A situation like it is now for gonic where the view doesn't exist is still a "valid" implementation of the spec. I haven't gotten around to testing gonic with lyrics, but without it, browsing the queue gets sluggish (latency dependent). Every movement already triggers a cover art fetch (dealt with by the concurrent album art branch and caching), and also a lyrics fetch with these changes now. Gonic (currently on main, at least), reports an error 70: view not found for lyrics. Because that response is invalid, nothing gets cached and stmps keeps retrying on each re-selection. This will be fixed when that gonic PR gets merged, but any servers running prior versions, and other servers that don't support the extension will have this issue. |
…ntains the OpenSubsonic lyrics extension
I've pushed changes that account for this into the xxxserxxx branch, and backported them to this branch in fa85bb0. One thing to note is that just because a server reports that it has the endpoints doesn't mean that the endpoints work: Navidrome claims to support lyrics, and it has the endpoints, but it does not return
Even without the concurrent branch, every movement shouldn't cause a fetch; it fetches only if the album art hasn't already been fetched. What the concurrent branch does is fetch the art in the background, so that browsing doesn't affect the latency of moving through the list. Yes, if the art isn't loaded, there might be a lag, but if you're browsing an entire album, there should be no lag after the first. This is around (depending on your branch) In any case, cover art issues are unrelated to lyrics support, right? EditIncidentally, it occurs to me that it may be useful to include in the issue reporting the network layout of the reporter. As I've mentioned elsewhere, I'm running both gonic and Navidrome on a several-year-old weenie AMD A10 Micro-6700T with a bunch of other self-hosted services (mpd, MyMPD, Home Assistant, snapcast server, Jellyfin, a mosquitto server, caddy), and the lag I see on the lyrics branch with cover art and lyrics is a slight hesitation if I hold down the "down" arrow when browsing a large queue. I'm thinking that maybe the difference is that my server is in the basement, and I'm ethernetted (over a coaxial converter, and I get ca. 8Mbps R/W between desktop & server). You (@mahmed2000) and @spezifisch are both reporting much more lag, and I wonder if that's entirely network related or some other issue. In any case, the issue template might include a "profile" attribute, where the reporter says whether the server is accessed through a LAN or WAN. |
That would be strange. afaik, subsonic isn't supposed to return lrc files. I checked navidrome's source, and that endpoint should be making the structured response. Is the server returning no lyrics in the StructuredLyrics array specifically, or literally just an empty json object at the lyricsList level?
Right, I'm realizing I could have phrased that a lot better. By fetch I meant just calling GetCoverArt here, so fetching is both from the server or from the cache. There's no lag caused by the cover art after the first time it gets cached. The only issue with cover art is when there's 50 songs from different albums. That's solved by the concurrent album branch, and has nothing to do with lyrics. I mentioned covers more so as "empty lyrics can fail to cache, unlike cover art".
That would the case, as far as I can tell. I access mine over 2 locations. One is when I'm accessing it over LAN over wifi. The other is over a VPN tunnel with 200-300 ms of latency, and spotty bandwidth anywhere in 100-1k kbps. Over LAN there's basically zero issues. Over the tunnel, its a hitch-fest. |
Implements #49
This was rebased onto main.
This adds support for synchronized lyrics.
It's functional, if not beautiful. mpv gives us ticks every second, on the second, and synchronzed lyrcs rarely line up with the second marks. I've found that a sweet spot is changing lyrics about a half-second in advance, as it nearly always means that at least the right line is highlighted when the vocals happen.
Navidrome doesn't have synchronized lyrics support, that I can tell; @danielepintore has a PR for gonic in the queue waiting for @sentriz to pull it -- that's the fork I'm working and testing with.
As usual, here's the teaser; it's long, but near the end shows me jumping around and having the lyrics match up:
Edit sigh jump forward at the start to 15s; it was apparently recording while waiting for me to respond to a prompt.