Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Synced lyrics support #87

wants to merge 7 commits into from

Conversation

xxxserxxx
Copy link
Collaborator

@xxxserxxx xxxserxxx commented Dec 18, 2024

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.

capture.webm

logger/logger.go Outdated Show resolved Hide resolved
Copy link
Collaborator Author

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.

stmps.go Outdated Show resolved Hide resolved
…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]>
@xxxserxxx
Copy link
Collaborator Author

I finally set up commit signing. ¯\_(ツ)_/¯

…ren't updating from song to song unless the album art also changed.
@mahmed2000
Copy link

mahmed2000 commented Dec 24, 2024

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.

@xxxserxxx
Copy link
Collaborator Author

xxxserxxx commented Dec 25, 2024

One thing this is missing is checking if the server actually supports lyrics.

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 .lrc files for calls to getLyricsBySongId -- or return anything, from what I can tell. However, this guard should prevent missing endpoint errors for this one endpoint, and it also entirely hides the lyrics widget if the server doesn't support the lyrics extension.

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.

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) subsonic/api.go#399 (and later line 441, where it's populated). If you're seenig lags on every song in the same album, I wonder if it's because your server is giving the cover art for every song a different ID regardless of whether it's the same image? I can see this happening with cover art embedded in songs; servers are probably not bothering to deduplicate cover art.

In any case, cover art issues are unrelated to lyrics support, right?

Edit

Incidentally, 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.

@mahmed2000
Copy link

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 .lrc files for calls to getLyricsBySongId -- or return anything,

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?

Even without the concurrent branch, every movement shouldn't cause a fetch; it fetches only if the album art hasn't already been fetched.

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".

I'm thinking that maybe the difference is that my server is in the basement, and I'm ethernetted

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants