✨Lyrics Support

[Chaphasilor]

This adds support for lyrics, which the Jellyfin server supports starting with Jellyfin 10.9.
Since 10.9 is getting close to a release now (planned for end of April), this PR aims to get lyrics support ready for the redesign beta before that release.

The fetching of lyrics, rendering, UI, and things like auto-scrolling are already mostly working. What's still missing is:

TODO

• Pre-fetch lyrics for previous and upcoming tracks
  • Seems like this is a bit more complicated than pre-fetching album covers, since that used helper classes provided by Flutter made explicitly for image fetching
• Look into using JellyfinApiHelper.getPlaybackInfo() in the metadata provider, that might require everything we need for now and is better suited for the task
• Properly highlight lyrics starting at 0:00
• Re-fetch lyrics when toggling offline mode
• Include lyrics with song downloads
• Handle errors when fetching lyrics (e.g. connection timeouts)
• Add a gesture (probably swiping to the left on the controls section) for opening the lyrics screen
• Use a different appbar button for closing the lyrics. The current arrow doesn't fit
• Fix scrolling being locked until the second line is highlighted for synced lyrics
• Use proper localization tokens
• Disable auto-scroll (possibly only temporarily) when the user manually scrolls
• Add some configuration options, like hiding timestamps or centering lyrics
• Allow tapping on a lyric line to jump to that position in the track
• Allow the user to disable auto-scroll
• Use the player's sizing controller to reduce the height of the control section
• Optional: Make the lyrics view part of the player screen, allowing the user to toggle between lyrics view and regular player view. That would have the benefit of allowing users to minimize the lyrics screen immediately without first going back to the player screen, and also re-open to the lyrics view directly

[Komodo5197]

Regarding precaching, I think all you need to do is swap out all the ref.read calls on the metadataProvider future in current_track_metadata_provider to calls to ref.watch. The provider initialized in the prefetch is seeing that no-one is watching and disposing itself before we get to that track. If you use watch, it should stay alive and the value should be available instantly when the track switch occurs.

[Chaphasilor]

Hmm, it seems like the types are not compatible for some reason. It won't let me watch() the future, nor the request.
It appears like ref.watch can't be used with an auto-disposable provider since it requires an AlwaysAliveProviderListenable.
Getting rid of the auto disposing works just fine, but I'm not sure of the implications of that. Seems like it might cause some unnecessary requests (not a huge deal) and also that it won't retry requests after an error when re-opening the player screen.

I've pushed the changes in c1c78ca, maybe you can take a quick look?

[Chaphasilor]

I also forgot to add one TODO yesterday, we should probably also download the lyric data along with the tracks and images for offline lyrics. What would be your approach to do this? You're already downloading track metadata somewhere after all (to show the grayed-out tracks), so maybe we can sneak the LyricDto in there somewhere?

Or instead of downloading the regular BaseItemDto, we use the new MetadataProvider instead (possibly after renaming it and moving it into finamp_models), since that one includes both the BaseItemDto and LyricDto (with the possibility of adding additional data in the future)?
Let me know your thoughts!

[Komodo5197]

We want metadataProvider to autoDispose because otherwise the lyrics will never be moved out of memory. In general, I don't think having a non-autoDispose family provider is good. I think the best option is something like this:

/// Provider to handle pre-fetching metadata for upcoming tracks
final currentTrackMetadataProvider =
    AutoDisposeFutureProvider<MetadataProvider?>((ref) async {
  final List<FinampQueueItem> precacheItems =
      GetIt.instance<QueueService>().peekQueue(next: 3, previous: 1);
  for (final itemToPrecache in precacheItems) {
    BaseItemDto? base = itemToPrecache.baseItem;
    if (base != null) {
      final request = MetadataRequest(item: base, includeLyrics: true);
      unawaited(ref.watch(metadataProvider(request).future));
    }
  }

  final currentTrack = ref.watch(currentSongProvider).value?.baseItem;
  if (currentTrack != null) {
    final request = MetadataRequest(
      item: currentTrack,
      includeLyrics: true,
    );
    var out = ref.watch(metadataProvider(request)).valueOrNull;
    if (out != null) {
      ref.keepAlive();
    }
    return out;
  }
  return null;
});

Regarding downloading lyrics, I'm not completely sure what the best way to go about it is. We don't want to download for all synced songs, only ones that are actually downloaded, both to reduce stored data and to reduce unneeded server requests. I don't see any way to batch lyrics or attach them to the BaseItemDto or anything, so that means an individual server request for every downloaded song with lyrics. The nastiest problem is that none of the lyrics endpoints have any sort of ID or version, so the only way to tell if the lyrics have changed is to re-fetch them for every song. I guess we re-fetch on full sync, skip on quick-sync, and do the actual downloading inside the sync step. I'm thinking the best way to actually store this info is probably in a separate Isar database, but using the same isarId as the parent song.

[Chaphasilor]

Thanks for the suggestions, they seem to work. I think I was missing replacing the FutureProvider type with a AutoDisposeFutureProvider to get the ref.watch to work.
Right now the metadata is only being requested while the player or lyrics screens are open, but I'd like for them to be requested as long as there is a queue to make the app more snappy. Compared to fetching the actual tracks it shouldn't cause any overhead anyway. Do you have any experience with that? Should I just add some kind of global listener for the provider? Or is fetching metadata in the background a bad idea for some reason?

We could create an issue for adding the needed info in the server repo. It probably isn't a big change. I'm also not a fan that lyrics are now split up, but it does makes sense to have them as media streams, just like subtitles.
Maybe we can also get the server to add an endpoint that returns all information about a track at once, but I think it's unlikely, since a media stream usually isn't text-based, and I don't think they'd add an edge case for that. Same for batched lyric / media stream fetching.

What would be the benefit of a separate database for lyrics? More flexibility for handling BaseItemDtos and LyricDtos independently?

[Komodo5197]

Regarding fetching the lyrics in the background, I believe just adding some sort of global watcher is the way to go about it. Maybe just put it into the now playing bar to match the album image, just try not to accidentally cause unneeded rebuilds on metadata change. This all somewhat conflicts with the other idea you had earlier of wanting to re-fetch failed requests when closing and re-opening the player screen.

Putting the lyrics in a separate database is just a bit more flexible, and helps separate any lyric related logic from the core DownloadItem syncing/updating logic, which is already complex enough as-is. It allows managing the lyrics in a way closer to how we manage downloaded files than how we manage the core metadata, which matches better with the behavior we want.

[Chaphasilor]

Okay, I'll see what I can do about it. I wasn't really aiming to re-fetch lyrics when re-opening the player screen, I was just trying to figure out the downsides a non-auto-disposed provider would have.
Errors should be handled explicitly if possible, but I haven't dealt with that yet. Right now errors still result in layout errors and overflow, so it needs additional work anyway ^^

I'll try to figure out how the download system works and how to add the lyrics downloading. I'd be happy about any pointers or guidance you could give me! :)

[Komodo5197]

Yeah, without autoDispose the provider will never get disposed, ever, unless one of it's dependencies refreshes. The cached value will just be stored forever. This makes sense for things with a single provider, like currentAlbumImageProvider, unless they are very large, and even makes sense for things with a small family like playerScreenThemeProvider, but is pretty bad for something with a large family like metadataProvider that creates a new provider per song.

For the downloads, you want to turn the LyricDto into an Isar collection and register it in main. Then inside _downloadSong() you fetch the lyrics, if needed, outside the isar write transaction. Then you actually write it to the new collection inside the same final write transaction in that function, using the same isarId as the parent DownloadItem. Isar transactions cover all collections at once, so this means that it we can never get desync where a notDownloaded song has lyrics downloaded. Finally, add a delete on the lyrics collection using the song's isarId to the transaction in deleteDownload()

[Chaphasilor]

@Komodo5197 I've taken a shot at implementing lyrics downloading now in bd3ec69. I'm sure there's something I did wrong or more complicated than necessary, especially w.r.t. the LyricsStub and LyricsItem. Would be great if you could take a look and maybe leave a review or something!

Edit: I'm also not sure if this update will migrate properly. I'm guessing people will need to re-download all tracks to get the lyrics?

[Chaphasilor]

Okay, that should be all of it. Main features should be fully working now:

• Fetching metadata and lyrics and displaying them nicely
• Pre-fetching metadata for upcoming tracks
• Downloading lyrics and more metadata
• Repairing existing downloads to include lyrics and metadata

I think I'll leave this as it is now for the 10.9 update, and then use the initial feedback for gauging which configuration options are wanted by users.
Would appreciate another round of testing by others!

[Komodo5197]

While looking at the volume PR, I noticed that there’s another spot in Downloditem.copyWith you should add some mediastream stuff to prevent accidentally removing it when updating metadata.

[Chaphasilor]

Was the thing I just added in the latest commit what you were referring to? I'm honestly not sure what the point of that assert was there (just to make sure the list of MediaSources is not empty?), but tried to add similar checks for the MediaStreams. Would that work, or do I have to do something else?

[Komodo5197]

Yeah, that’s what I was referring to. Looking at the assert, I’m honestly not entirely sure why it was put in either. Regardless, it’s probably best to update it to make sure both are null or non-empty, vs the current logic which checks if either is.

[Chaphasilor]

Okay, that should be it then. I'll merge this tomorrow morning, unless something else comes up...

[Chaphasilor]

Just noticed I forgot to update the the assert. I'll throw that in tomorrow

[jmshrv]

This all looks good to me :)

[Chaphasilor]

🎉

[barolo]

This all looks good to me :)

Can we have a build?

[Chaphasilor]

@barolo it was just released ^^

But the feature will only work with a Jellyfin 10.9 server!

[barolo]

@barolo it was just released ^^

But the feature will only work with a Jellyfin 10.9 server!

Great!. I know, I'm already on 10.9

[barolo]

It's kind of weird that it's a separate screen [instead appearing in and expanding from within the current one], could we have an option to set it as default? Or auto-trigger when lyrics are available?
It looks and works nice though.

[Chaphasilor]

Yeah, I agree that the current "layered" behavior isn't great, especially because it doesn't allow quickly collapsing the player screen and e.g. searching for a track. I can try making it so that it actually switches between the screen instead of layering them, and that it remembers the default.
Not sure when I'll get to that though, I'll probably wait for additional feedback after 10.9 has officially dropped...