Add support for Lyrics

[X-Ryl669]

This is a large PR, and probably need some guidance to fix my errors.

This adds support for capturing lyrics and displaying them in the web interface, to play in sync (or not) with a song being played.
It appears like this:

The PR is split in 3 commits that are completely independent of each other.

First commit

The first commit adds a key in the file table in the database to store the lyrics.
I'm using ffmpeg's metadata API here, so the lyrics are available in keys lyrics_eng or lyrics_XXX, etc (the key name contains the language for the lyric). This works with audio files with embedded LRC lyrics and non synchronized lyrics.

Since the current implementation used to look for fixed keys' name, I've inverted the key searching logic so that the code enumerate all metadata keys from the file and match them with the metadata key Owntone is able to handle. This avoid doing 2 pass over the metadata (one for usual metadata from Owntone's keys), and another for searching any lyrics.
My algorithm is doing a single O(N) search like before, so it's not changing the speed of Owntone's file parsing.

If it proves too slow anyway, we could force the Owntone's metadata keys array to be sorted by name and perform a binary search on them.

I've tested all the parsing and database code.
Please check the db upgrade code, I'm not 100% sure it's correct and I wasn't able to test it correctly, since I don't know why it doesn't upgrade.

Second commit

I think I got this right. This adds a /api/library/lyrics route to fetch a track's lyrics. The api/library/track route was modified to add a "lyrics": 1 if lyrics are available for a track, with the idea to optimize a call to /api/library/lyrics if they aren't. The web interface doesn't use this route anyway, so it would only be useful for any other client.

I've tested the API completely

Third commit

This adds the required changes to the web interface to display lyrics and add a lyrics pane.
I've added 2 components (Lyrics.vue and PlayerButtonLyrics.vue). The latter allow to toggle the visibility of the former.
I've modified the store to support lyrics too and the main application to synchronize fetching lyrics when the player status is updated. This means that an additional request is made each time a new track is started (to fetch the lyrics).

I had to modify the bottom navbar code too to deal with the lyrics toggling button. I've removed the hack used to center the play control icons (with auto margin left/margin right) and instead used the flex properties and made 3 groups (the left group contains the queue button, the center group contains the player control icons, and the right group contains the lyric toggle and output selection button).

What I'm not sure of:

1. In the Lyrics.vue, I've embedded a lot of style code that's required for animations and displaying the pane. The other components don't do this, I'm not sure what is the policy here or if I need to move it elsewhere.
2. Currently, the color is hardcoded in this embedded stylesheet. So any dark mode switching would require some tweaking. I don't know how you intend to deal with this.

If required, I can send you songs with the 2 kinds of embedded lyrics if you want to test.

[ejurgensen]

Thanks for the PR. A general question from me would be if it is normal to have the lyrics part of the embedded metadata? If you adding those tags yourself from some online source, might it then be an idea to get the lyrics directly from there?

Anyway, as you say, the PR is large, so it might take some time to review it. I suggest you start by reviewing it yourself - check that code style is consistent with the rest of OwnTone, typos stuff like that. Check that MODULE_RAUTH also.

Afterwards, I think both @hacketiwack and I will need to review.

[hacketiwack]

Yes, indeed, I need a bit of time to check that PR.

To answer your question @ejurgensen it can/may be normal to have lyrics embedded into the metadata.
At least, there is an extension for MusicBrainz Picard to do it and there are "official" tags for it.
The problem here is more: what is a reliable source of lyrics as there are not that many freely available nowadays.

[X-Ryl669]

Is it normal to have the lyrics part of the embedded metadata? If you adding those tags yourself from some online source, might it then be an idea to get the lyrics directly from there?

There's 2 kind of lyrics: synchronized one and non synchronized lyrics. The former is usually using the LRC format. You can use LRCGet to fetch all the lyrics for your track automatically. The latter was easier to find and you could run beets that'd fetch them for you.

To embed any kind of lyrics you can use mutagen library via a CLI tool like eyeD3 or a GUI tool like Ex Falso.

I haven't added support for checking if a .LRC file exists as a sibling of a .MP3 file in a folder. Some player do that, but it's much easier to embed them inside a single file instead.

[X-Ryl669]

The problem here is more: what is a reliable source of lyrics as there are not that many freely available nowadays.

There's LyricsWiki where you'll find almost all (unsynchronized) lyrics (they have more than 300 000 lyrics in their database). When using beets, you can set many source to fetch lyrics (google, lyricswiki, musicmatch, genius, etc...)
With LRCGet, I don't know what's the source used, but it does find a lot of them (but not all).

[X-Ryl669]

@ejurgensen I've a lot of issue with the indentation rules used in Owntone. It's a mix of tab and spaces and no matter what I set in my editor, it never feel right. What are the indentation rules and, better, do you have a tool to indent with this rules ?
I've tried to follow the coding style as much as possible already (for C files I meant).

I have no idea what "MODULE_RAUTH" is. What do you mean ?

[ejurgensen]

I have no idea what "MODULE_RAUTH" is. What do you mean ?

If you reviewed the PR you would notice https://github.com/owntone/owntone-server/pull/1655/files#diff-b0abab8d34e46a738f9efe71f8d6d9ae7a8a58c48b45f178a6346f277efc25b2R104

The indentation logic in OwnTone is pretty special, it is indeed mixed tabs + spaces. Basically 8 spaces become a tab. If you check the other code you will get it.

[X-Ryl669]

I've written this python script that follow this indentation rules:

#!/bin/python
import sys


def replaceTab(line):
    spaceLine = line.rstrip().replace('\t', '        ')
    meat = spaceLine.lstrip()
    # Then convert left spaces to tab following the rules:
    indentation = len(spaceLine) - len(meat)
    tabCount = int(indentation / 8)
    return '\t' * tabCount + ' ' * int(indentation % 8) + meat

def main():
    dryRun = False
    if len(sys.argv) < 2:
        print("Usage is {} [-n] fileToFix".format(sys.argv[0]))
        return 0

    file = sys.argv[1]
    if (file == '-n'):
        dryRun = True
        file = sys.argv[2]

    print("Processing: {} {}".format(file, "dry run" if dryRun else ""))
    outLines = []
    with open(file, 'r') as f:
        lines = f.read().split('\n')
        for line in lines:
            outLines.append(replaceTab(line))

    if dryRun:
        for line in outLines:
            print(line)
    else:
        with open(file, 'w') as f:
            f.write('\n'.join(outLines))
    
if __name__ == '__main__':
    main()

And applied to main.c. There are multiple place in the source that doesn't follow this rule (off by 1 space or 2).
I haven't run this indentation tool on all files, it's probably out of sync in multiple places.

It's probably another PR request, but would you accept to switch to a more conventional indentation rule (like all spaces, 2 space per level) ?
I've written a clang-format config file but obviously, this kind of mixed indentation doesn't exist. And the indent opening brace on their own line requires the latest clang version.

Anyway, I've removed the RAUTH line, I don't remember putting it here. I think I've followed the other coding convention I'm seeing around.
I've rebased the commit, so it's easier to review commit by commit since they are all independent.

[X-Ryl669]

Any update on the review process?

[ejurgensen]

Reviewing isn't the most fun, but I'll try to get it done soon. I have the C stuff, @hacketiwack has the web UI.

[hacketiwack]

I will do the review in the next few days (over the weekend). Sorry for the delay.

[ejurgensen]

Here's a first review covering the db and json changes. I haven't done the filescanner yet.

[X-Ryl669]

I think it's resolved all remarks (except for design remark). I've also renamed the commit to fit the other commits name. For example files with lyrics, please follow this link it's valid for 7 days. You'll find both synchronized, unsynchronized and tracks with no lyric.

[hacketiwack]

Thank you for taking into account the comments and make the according changes.

[X-Ryl669]

Ok, I've reworked the commits with all your requested change (API + Web changes).

@hacketiwack I've squashed your commits in the last one so it's semantically only one with the "convention" naming for the commit message. Feel free to check if I haven't miss anything. Thank you!

[hacketiwack]

All good for me 👍🏼

[X-Ryl669]

I have applied all changes requested. Please review for the potential bug I've mentioned, if it's correct, I'll remove the comment.

[X-Ryl669]

Ok, I think it's the last shot, everything should be correct now. I've reinstantiated av_dict_get since it's the most compatible function to use. I've fixed the lyrics index so it's optimal. I've fixed the mdcount bug as well.

[ejurgensen]

All my requested changes don't seem to have been made. I will do them and then merge.

[X-Ryl669]

What did I miss?

[X-Ryl669]

I'm sorry I haven't understood your last message. What change did I miss? How can I help you better?

[ejurgensen]

I'll add a few things and then merge, but it will be later this week.

[ejurgensen]

Merged it now, thanks for the contribution. As you can see, the parts I was missing was using av_dict_iterate and not doing the pre-search for the lyrics index. I also fixed up a few other things (e.g., there was a potential bug in the old code where the value from ffmpeg dictionary was being modified, but the docs say not to do that).

Hope it works, I have to admit I didn't actually test it with a lyrics file.

[X-Ryl669]

Testing right now. Please wait.

[ejurgensen]

I notice now that the lyrics icon is showing for all tracks, even if they don't have lyrics. Can you change it so it only shows if there are lyrics?

[X-Ryl669]

It's working for me.

Just a remark: the systemd service file isn't working well for me. It should include a User= and Group= rule, since without this, it'll run as root and then setuid to whatever user you've set up in the owntone.conf. Yet the group is then the (low privilege) user that doesn't have read access to the song library.

If I understand POSIX user management correctly, the songs in the library should be owned by a "music" group whose users are allowed to read and/or write to them. I want to be able to set up this user in either the conf file (or in the service file).

[X-Ryl669]

Can you change it so it only shows if there are lyrics?

This starts an usability issue: If you use the lyric mode (by clicking on the button) and song A in queue has lyrics, then song B hasn't and then song C has too. You don't want to disable the lyrics mode when reaching song B because then you'd have to enable it again in song C.

The other player, like spotify, amazon music, works the same. When you have toggled the lyrics, it stays on even for tracks without lyrics.

What do you think, should we act differently?

[ejurgensen]

I think it's fine to keep it active and visible if it has been pressed. My main concern is that I think very few users have lyrics, so for them the icon has no use.

[X-Ryl669]

Solution 1: Leave like this, experience replicate other music players
Solution 2: We can grey/disable the button if the song hasn't any lyrics (and remember the lyrics panel state outside of the button visibility). You can only click it if the tracks has lyrics
Solution 3: Hide the lyrics button if the song hasn't any lyrics. This also means hiding the lyrics panel also. As soon as a track has lyrics, show both button and panels (this means tracking the lyrics panel state outside of the button). If the lyrics panel was enabled before, it automatically display when the track with lyrics is played
Solution 4: Same as solution 3, except that the user has to click to re-show the lyrics panel for each track, it's not done automatically.

Both solution 3 and 4 show a usability/discoverability issue; the user will never know there's a feature to display the lyrics until she both wait for a track with one and happen to be in "currently playing state".

Your preference is for solution 3, right ?

[ejurgensen]

The current solution is not an option in view. I personally would prefer to have as few buttons on the interface as possible to keep it clean. Would there be a fifth option to not have any button, but just auto-display the lyrics if they are available? With a "close" button if they user doesn't want to see them.

@hacketiwack what do you think?

[ejurgensen]

I've reverted the UI changes, in addition to the button issue I noticed that the linter was had some issues with them:

/home/runner/work/owntone-ghatest/owntone-ghatest/web-src/src/components/Lyrics.vue
  11:7  error  Elements in iteration expect to have 'v-bind:key' directives  vue/require-v-for-key
  23:9  error  Component name "Lyrics" should always be multi-word           vue/multi-word-component-names
  57:9  error  Unexpected side effect in "lyricIndex" computed property      vue/no-side-effects-in-computed-properties
  58:9  error  Unexpected side effect in "lyricIndex" computed property      vue/no-side-effects-in-computed-properties

/home/runner/work/owntone-ghatest/owntone-ghatest/web-src/src/components/PlayerButtonLyrics.vue
  19:8  error  'webapi' is defined but never used  no-unused-vars

Please fix and submit a new PR

[hacketiwack]

I went through the last version of the source code for the web part. Two main points:

1. It still needs to be prettified with the command npm run format.
2. The bottom navigation bar is not working properly:
   • On mobile devices, the title of the track is not truncated anymore when the size is too small (see below).
     
   • The buttons are now too large (see below).
     

For point 2, I highly recommend to keep the old navigation bar and add the lyrics button in the menu where there is the volume. We will improve the navigation bar in another release.
This is how it should look like:

[X-Ryl669]

The current master gives this (on constrained width):

But also this (on unconstrained width):

Which is disturbing as, as soon as you click on the "now playing icon", the control shifts position (in addition to also changing color):

I think it's wrong (buttons position should never move, it's bad for UX and the color shouldn't toggle either).

In fact the current master solution is trying to fit a 2 opposite models with the current layout.

There are 2 cases:

1. The song title fits in the bottom left of the screen (viewport width is large enough): in that case, the player buttons and volume pane button should be respectively centered and on the bottom right.
2. The song title doesn't fit in the bottom left of the screen (viewport is too small): in that case, the player button and volume button should be moved to the bottom right of the pane

In case 1, the current PR fixes it since the player button never shift place or jump from right to center.
In case 2, I think the design should be reworked to move the player buttons on the bottom right (or even better, remove the previous track and next track button and use drag movement on the album art to skip to the next/previous track).

This means setting up a responsive layout with 2 rules, and a breaking rule set to when the title size is below 70% of the viewport width (as you can see in the screenshot above).

The first layout would keep the player controls on the center (whatever the state : queue / now playing)
The second layout would remove the flex rules to center the player controls to align them on the bottom right.

I agree about your idea to move the lyrics button on the volume pane, it fits perfectly with the expected behavior.
Let me know what you think about the responsive rule here.

The buttons are now too large (see below).

It's not the button that is too large (notice that you can't click out of the button), it's the navbar-item hover effect that darken the area around. I think I can fix this.