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

[Feature Request] Lyric support #338

Open
LordZeuss opened this issue Sep 26, 2022 · 20 comments · Fixed by #693
Open

[Feature Request] Lyric support #338

LordZeuss opened this issue Sep 26, 2022 · 20 comments · Fixed by #693
Assignees
Labels
enhancement Improves an existing feature redesign-beta Issues available for Hacktoberfest

Comments

@LordZeuss
Copy link

LordZeuss commented Sep 26, 2022

Finamp is a fantastic project. Not sure how difficult it is to implement, but to be able to read downloaded lyrics files, or somehow have them pull from an external source on the fly, would be amazing.

@koollylee
Copy link

Embedded lyrics display is very necessary for music library。

@iPaulTech
Copy link

Great news in this topic!!!
Last month Jellyfin has merged lyric support in their official API!
The feature should be available in the next releases.
jellyfin/jellyfin#8381
I've build Jellyfin from Source and the API is there!!!

This means that we could implement this feature now.
Maybe I'll take a look at it, I've already forked Finamp.

However, I have no experience in Flutter yet. Could therefore also become nothing.

@Chaphasilor
Copy link
Collaborator

Awesome! I guess combined with this new API and the fact that any files that have embedded lyrics tags can also be used to acquire lyrics, this should cover almost all cases.

The implementation could first test if there are (mp3) tags embedded with lyrics info, and if there are try to parse them with an LRC parser. If there are no tags or parsing fails, the API could be tried next.

Ideally, this should probably be done when a song is fetched for playback, or downloaded for offline use.

@iPaulTech if you need any help with the UI stuff just ask me. I'd be happy to design some mockups for you :)
But getting the lyrics data into the app somehow, even without displaying them, would already be a massive leap forward!

@iPaulTech
Copy link

Oh you've right... There is a download function 😄.
I mostly streamt directly, because this works so good :)

I think it would be better, if the lyric comes only from the Jellyfin API. And would be saved for offline playback in the Finamp database (I think this is SQLite).

I hope i can experiment with flutter soon. Because last weekend i've had build problems... But this should be fixed in the next days. I wait for the commits :).

And for the UI, i like the implementation from the App "musicolet" (not really Opensource).

@Chaphasilor
Copy link
Collaborator

I think it would be better, if the lyric comes only from the Jellyfin API

The potential problem with that is that is only exposes .lrc and .txt files and not any embedded tags. The embedded tags solution already works in other apps and would be great to have here as well, otherwise some people might be missing out, depening on how their media is organized :)

@iPaulTech
Copy link

Mh... Thats not good. But i think this should not be a "Finamp problem"... It would be better if the user would use some script for extract the lyrics to seperated files on the server side... I also do not know how this would be with transcoded streams... If the metadata are not in the transcoded files you would habe a Problem...

People should do the backend rightly and not a simple client... What is with the Web Client for Jellyfin... This would be bad if only Finamp has the lyrics from inside the .mp3 or .FLAC files...

@Chaphasilor
Copy link
Collaborator

You're right of course, the API should ideally support embedded tags as well. I'd suggest to start with using just the API and then seeing if we can also add client-side support for embedded tags until the backend catches up.
Removing code once it's not needed anymore shouldn't be a problem.

Also, I'm curious what @jmshrv's opinion is on this 🤔

@jmshrv
Copy link
Owner

jmshrv commented Nov 16, 2022

This is really exciting! Awesome to see that its becoming part of Jellyfin.

just_audio currently doesn't support viewing a song's metadata tags, but it should be possible to add (although it would require me writing Objective-C 🤢) - ryanheise/just_audio#88

Ideally, Jellyfin itself should handle this. I'd assume it's possible as Jellyfin already looks at tags to organise the library. It also looks like the code is pretty easy to extend as LRC and TXT implementations are nicely split into providers, so it'll probably be easier to add a tag provider than to add tag extraction support inside Finamp/just_audio.

As for downloads, Finamp's download system is a mess that I've been meaning to strip out for a while. I don't have the time to do it now, so I guess we can't wait for that to happen. It's currently spread out across 5 key value databases (+ the SQLite DB that flutter_downloader creates), and its a horrible blob of functions that just have to manage everything themselves with no security if anything goes wrong - I'm amazed I don't get any bug reports mentioning downloads going missing or something. Since the lyric endpoint returns its own type instead of adding it to BaseItemDto (which is a way better solution - BaseItemDto is silly), we'd have to get it separately when adding downloads. It could probably be added into DownloadedSong pretty easily, and addDownloads already needs to get other item metadata so we could do it there. I'll be happy to handle the download stuff since it's pretty impossible to understand.

As for implementing the API bit, these are the relevant files:

  • services/jellyfin_api.dart - This actually handles sending HTTP requests to and from Jellyfin. It's pretty much just a simple Chopper thing that returns JSON.
  • services/jellyfin_api_helper.dart - This is what the whole app uses to interact with Jellyfin (and by extension the offline stuff, but that's all obscured). In this, you'll call the method you make in jellyfin_api.dart and return a nice class instead of some random JSON
  • models/jellyfin_models.dart - This is where we store classes from the Jellyfin API. The Hive stuff is related to offline storage, so it's not essential for basic API stuff. I usually just hand write classes from the API documentation, which is extremely fun and not draining at all :)

@DomiStyle
Copy link

Ideally, Jellyfin itself should handle this. I'd assume it's possible as Jellyfin already looks at tags to organise the library. It also looks like the code is pretty easy to extend as LRC and TXT implementations are nicely split into providers, so it'll probably be easier to add a tag provider than to add tag extraction support inside Finamp/just_audio.

I added support for embedded lyrics in this PR: jellyfin/jellyfin#9220

Works transparently for the client so just calling the one lyrics endpoint is fine.

@jmshrv
Copy link
Owner

jmshrv commented Feb 3, 2023

That's awesome, saves me a lot of work. Thanks!

@ryanwwest
Copy link

Thanks @DomiStyle! Just wondering, does this PR work for both downloaded and streamed songs? And pardon my lack of understanding—to get the lyrics in Jellyfin Server is that enabled by default, or something you need to set up before this PR would read them from the server?

@jmshrv
Copy link
Owner

jmshrv commented Mar 7, 2023

That PR is for Jellyfin itself, which previously had no way for asking for lyrics. Finamp will need to add support for the new endpoint, which I plan to do in the redesign :)

@ryanwwest
Copy link

Ha, right, I should have clicked the link to see where the PR originated from! Thanks for letting me know.

@Chaphasilor
Copy link
Collaborator

I'll probably start working on this once I've finished implementing the new queueing system. This will happen in the redesign though, and I probably won't backport it myself. But backporting should be possible, if someone wants lyrics asap.
ETA is probably... in a month? 😅

@jmshrv
Copy link
Owner

jmshrv commented Jul 26, 2023

Jellyfin releases are super slow so the redesign will probably be out by time lyrics are available in the server lol

@Chaphasilor
Copy link
Collaborator

That's what I'm hoping for. Still haven't started, because the queue is driving me insane...
I'm looking into setting up a Jellyfin beta server for testing though, and as soon as the queue stuff is done I'm tackling lyrics!

@stefan1983
Copy link

+1 :)

@Chaphasilor
Copy link
Collaborator

ETA is probably... in a month?

Well that didn't work out so well...
I'm keeping up with @jmshrv's traditions it seems 😁

Now that the beta is out, I'll first focus on the imminent issues, and then see when I have some time to tackle this. There's still no ETA for Jellyfin 10.9, but I'd like to make sure Finamp is ready for that release!

@Chaphasilor Chaphasilor self-assigned this Feb 29, 2024
@Chaphasilor Chaphasilor added the enhancement Improves an existing feature label Feb 29, 2024
@jmshrv
Copy link
Owner

jmshrv commented Feb 29, 2024

I'm keeping up with @jmshrv's traditions it seems 😁

hey you actually deliver on most of your promises :)

@Chaphasilor Chaphasilor linked a pull request Apr 28, 2024 that will close this issue
16 tasks
@Chaphasilor
Copy link
Collaborator

Lyrics support is basically done at this point, just in time for the Jellyfin 10.9 update next week! It will be part of the next beta update

@Chaphasilor Chaphasilor added the redesign-beta Issues available for Hacktoberfest label May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves an existing feature redesign-beta Issues available for Hacktoberfest
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants