NickvisionApps / Tagger

Tag your music
https://flathub.org/apps/details/org.nickvision.tagger
MIT License
225 stars 14 forks source link

Lyrics Support #225

Closed SpicyWasab closed 1 year ago

SpicyWasab commented 1 year ago

Hi, first of all thanks for the awesome app and the fact that you're actively maintaining it !

Second is, I don't know much about audio tags, I think I learned a few things while trying to understand what was wrong.

So here's the story : I'm using Oto Music on Android to listen to my .flac songs, and also to fetch the synced lyrics. I thought the lyrics would be shown afterwards on Tagger, but it's not the case. I tried to understand where the lyrics were stored, and apparently they're stored in a lyrics custom tag. I saw that you did an update 2 weeks ago to add support for custom tags, and indeed they're here. Still, the lyrics tag doesn't show up. I found out while using Ex Falso (another tag editor) that apparently the issue could be that the lyrics are (obviously) stored in some sort of "multi-line" field, because on Ex Falso there's a checkbox to "unsee multi-lines tags", which makes the "lyrics" tag disappear. It may not be the issue, but if this is indeed the issue, is there any way to add support for "Multi-Lines" custom tags ?

nlogozzo commented 1 year ago

Yes I'll look into multi-line custom tags...probably just need to let the row expand vertically to show it.

Also, I'm thinking we could also add proper lyrics support to Tagger as they have some special features (could probably download lyrics from web too)

SpicyWasab commented 1 year ago

Wow, thanks for the fast answer.

add proper lyrics support to Tagger

I wasn't thinking about that in the first place ... that would be awesome ! Maybe some sort of helper for editing .LRC data as well ? :thinking: That's not necessary though, just being able to edit the lyrics would be cool.

nlogozzo commented 1 year ago

Please try out the feature in this release: https://github.com/NickvisionApps/Tagger/releases/tag/2023.8.2-rc1

SpicyWasab commented 1 year ago

Hi ! Sorry I only saw that yesterday before I went sleeping. I already installed it through flathub, I'm gonna do some tests to provide a feedback. The synced lyrics editor looks cool though, and the update arrived quite quickly, that's very nice !

SpicyWasab commented 1 year ago

@nlogozzo Hi again !

EDIT : Warning, there's a wall of text ahead. I tried to describe the issues and what I tried as precisely as possible, but english isn't my native language and I tend to write complicated stuff. I hope it would help understand more precisely what are the issues though.

I tried with 2 .flac files and one blank .mp3 (by "blank" I mean a song which does not currently have embedded lyrics).

Unfortunately I have some issues.

The first one may not be Tagger related, it's just that my synced lyrics are stored in the LYRICS tag, where Tagger gets the unsynced lyrics, so it's displayed as plain text. (which is still better than no lyrics at all though)

I got another issue where the lyrics were just not displaying on one of my .flac song, even though they were on other apps (such as NTag or Ex Falso). The file was a .flac file and the lyrics were in the LYRICS tag. As they were displayed on the first file, I opened the file in NTag and tried some things, such as shortening the lyrics to match the first files' length (I thought that it may be a string length issue ...). I even tried to replace the lyrics with the ones from the other working song, but they weren't displaying when I opened the file in tagger. It could be a pretty bad issue because when I edit some things in tagger with this file (like the song name), tagger overwrites the lyrics tag, and deletes it since it sees it as empty.

I tried setting the lyrics with tagger on this file, and they display properly on other softwares (as NTag and Ex Falso). However, when I edit them with NTag and close every apps (Tagger, NTag, Ex Falso), when I open the file with NTag and Ex Falso, the lyrics are as they were when I edited them, but with Tagger they stayed as they were before the edit. Which is really weird. If I do other changes to the file with tagger, the lyrics are overwritten to the before-edit-state. I tried with the other "lyrics-working-at-first" file as well, and it's the same issue :thinking:. And yes, I tried both refreshing and closing Tagger, as well as closing Tagger before editing with another software. I tried copying more music files to my Tagger folder and the lyrics aren't displaying on some of them. It's like 2 or 3 lyrics-missing files on 19 songs.

As for the other tabs, I don't seem to see where the informations are stored. When I try to add synced lyrics there does not seem to be any change to the file when I lookup on other softwares. Same goes for the "language" and the "description" fields in the configure tab. The datas are obviously there, because I tried doing some md5sum on the files after each edit, and checking the sum after reverting the edits to see if it matches the previous one. I I just don't know where they are x)

(and edit : of course feel free to ask anything !)

SpicyWasab commented 1 year ago

@nlogozzo 2 other things I just realized :

  1. I think users should not be allowed to create a lyrics custom tag since that's what the "Unsynced lyrics" tab is basically for. And it create 2 lyrics tag which is pretty weird.
  2. In the "Synced lyrics" tab it's said that "lyrics are stored as a Dictionnary of strings identified by their timestamp in milliseconds." I don't know if there's some sort of specification for this type of embedded lyrics, but - for me using Oto Music at least - the lyrics are in .lrc format (the same as the file format, just embedded). (not sure if it should be stored in the lyrics or if this is some Oto Music specific thing.

Here's a link about LRC, the wikipedia article is pretty good at describing how the format works and what is allowed : https://en.wikipedia.org/wiki/LRC_(file_format)

nlogozzo commented 1 year ago

Thanks for all the information!!

It's sounds more like an issue in the tagging backend Tagger uses (atldotnet)...so I ask if you can send me an mp3 file and a flac file with lyrics that were NOT edited in Tagger....I'll pass them along to atldotnet's developer so we can see how this other software stores tags and then parse them correctly so they work in Tagger.

Feel free to email them to me @ nlogozzo225@gmail.com

SpicyWasab commented 1 year ago

(just to say that : I'm not sure if you saw the second message I just sent since I sent it while you were on the page. Heard github isn't really good as live updating the page sometimes)

I can easily send you some different .flac file in a few hours. As for the .mp3 I don't have one right now but I can easily generate one, just a matter of minutes. (why .mp3 though ?)

And thanks for the fast reply as well c:

nlogozzo commented 1 year ago

I think users should not be allowed to create a lyrics custom tag since that's what the "Unsynced lyrics" tab is basically for. And it create 2 lyrics tag which is pretty weird.

Agreed!

I don't know if there's some sort of specification for this type of embedded lyrics, but - for me using Oto Music at least - the lyrics are in .lrc format (the same as the file format, just embedded). (not sure if it should be stored in the lyrics or if this is some Oto Music specific thing.

Well LRC is just timestamps and lines so it's basically stored all the same way. But I'll make sure to look at LRC and make sure we parse that correctly for synchronized lyrics...and maybe add an option to Tagger to import and edit that directly.

why .mp3 though ?

Each music file type uses a different tag container...MP3 uses what's called ID3V2 and Flac generally uses what's known as XIPH Comment. Tagger and atldotnet abstract all these differences to make all files work the same together...but behind the scenes we are dealing with different tags and specifications per file type.

nlogozzo commented 1 year ago

I can easily send you some different .flac file in a few hours.

Let me know here once you send the email as I'll be working all day and I'll see the github notification before the email one 😅

SpicyWasab commented 1 year ago

I sent you an email. The .flac files were too heavy to be sent alongside the email, so there's a Proton Drive link at the end. The rest of the email contains some infos about what I did with each file (where does the lyrics got applied, etc...).

By the way, I didn't mentionned it earlier but whenever I open the Lyrics dialog, even though I didn't changed anything Tagger considers the file as edited, and will prompt to apply the changes.

nlogozzo commented 1 year ago

I sent you an email.

Perfect, thanks!

Will work to get a fix hopefully sometime this weekend.

By the way, I didn't mentionned it earlier but whenever I open the Lyrics dialog, even though I didn't changed anything Tagger considers the file as edited, and will prompt to apply the changes.

Will look into it :)

SpicyWasab commented 1 year ago

Fine ! I'll check for the new beta version. However, I'm going to go on vacation this sunday, I'l probably be able to try the new version but I'll also probably be less there.

nlogozzo commented 1 year ago

Fine ! I'll check for the new beta version. However, I'm going to go on vacation this sunday, I'l probably be able to try the new version but I'll also probably be less there.

Alrighty no worries...I should be able to test it with the files you sent me as well

Enjoy the vacation 🙂

nlogozzo commented 1 year ago

Please try the beta and let me know how it goes: https://github.com/NickvisionApps/Tagger/releases/tag/2023.8.3-beta1 :)

SpicyWasab commented 1 year ago

Hi again !

I tried. It's really nice ! I like the fact that it doesn't overwrites all the lyrics, but keeps the existing tags even though tagger don't use them.

Still some issues though : First of all, you can't just edit the synced lyrics. If you only edit the synced lyrics - not talking about adding a line but rather editing an existing one - the "apply" button will still greyed-out. I checked with another software as well : not edited. You need to change another metadata in order to enable the "apply button", then apply the changes. Then, the lyrics are edited.

Can't save language and description, but I don't have any idea what those values are. I just know I can't edit them, like if I add a value it disappears after an apply.

Weird behaviour when having both synced and unsynced lyrics. Which is understandable since it's just not possible lol. Basically, adding unsynced lyrics wipes out the synced lyrics. Adding synced lyrics is not possible if there's already unsynced lyrics. Not a bug, but it would be nice to ... I don't really know, maybe greying out one of the tabs if the other one is the one currently in use. But we should find a way to explain to the user why the tab is greyed out and how to switch. Or maybe making both tabs overwrite the lyrics field if and only if it's not the current used tab, and adding a modal dialog to warn the user that this change will be destructive.

SpicyWasab commented 1 year ago

Here's an image of what it could look like if the second "modal" suggestion is not clear enough. It's really late so I don't really know. image

Good night c:

nlogozzo commented 1 year ago

First of all, you can't just edit the synced lyrics. If you only edit the synced lyrics - not talking about adding a line but rather editing an existing one - the "apply" button will still greyed-out. I checked with another software as well : not edited. You need to change another metadata in order to enable the "apply button", then apply the changes. Then, the lyrics are edited.

Ah this issue came when we refactored the code...I know the fix (see #269)

Can't save language and description, but I don't have any idea what those values are. I just know I can't edit them, like if I add a value it disappears after an apply.

Will look into

Weird behaviour when having both synced and unsynced lyrics. Which is understandable since it's just not possible lol. Basically, adding unsynced lyrics wipes out the synced lyrics. Adding synced lyrics is not possible if there's already unsynced lyrics. Not a bug, but it would be nice to ... I don't really know, maybe greying out one of the tabs if the other one is the one currently in use. But we should find a way to explain to the user why the tab is greyed out and how to switch. Or maybe making both tabs overwrite the lyrics field if and only if it's not the current used tab, and adding a modal dialog to warn the user that this change will be destructive.

It should be possible to have both unsync and sync lyrics actually...will investigate

nlogozzo commented 1 year ago

It should be possible to have both unsync and sync lyrics actually...will investigate

It works for me...can't reproduce: Screencast from 2023-08-22 00-37-05.webm

Could you email me the file that's giving you trouble and see if it happens with different files and different file types (flac vs mp3 vs m4a)

nlogozzo commented 1 year ago

Can't save language and description, but I don't have any idea what those values are. I just know I can't edit them, like if I add a value it disappears after an apply.

From our backend's dev:

I simply assumed setting a description or a language code without setting the lyrics themselves is pointless. Plus in ID3v2, language code and description cannot float in outer space - they need to be linked to other metadata.

Which makes sense. Therefore I'll probably gray those fields out in the UI until unsync or sync lyrics are set.

nlogozzo commented 1 year ago

Ok got all of this fixed, just waiting for an updated package of our backend and I'll make a new release

SpicyWasab commented 1 year ago

(sorry for the late reply as well)

It should be possible to have both unsync and sync lyrics actually...will investigate

Oh ... I just saw your screencast. How does it work ? :sob: I mean, both of them are stored in the "lyrics" field :thinking:

Could you email me the file that's giving you trouble and see if it happens with different files and different file types (.flac vs .mp3 vs .m4a)

According to your screencast you're using an mp3 file. I just tried with an .mp3 file and it works, but not with my .flac ones. I think your guess is correct. The .flac files I sent you a few days ago should work. (or, "not work" more precisely. I mean ... it should work to not work.).

*tbh I guess you probably already did some tests since it's been 3 days and apparently "you got all of this fixed", but I can still provide you other files if you want.


not really an "edit" but more of a "I'm adding something" since I tend to do testings along writing my comments. I just managed to add a language code and a description to the lyrics with a .flac file. I didn't manage to reproduce with the other one I'm doing my tests with. I don't have any idea why. Same goes for the ability to have both types of lyrics. Maybe it's just an issue with this particular file ... I don't have the time to try more intensively right now and since we're waiting for a future release it may have been fixed. Anyways, ask me if you still want that file.

nlogozzo commented 1 year ago

Anyways, ask me if you still want that file.

Yeah please send me that specific flac file that's giving you issues...

SpicyWasab commented 1 year ago

I emailed you the files (still a Proton Drive link, one file is working, the other one isn't).

Here's a video of me trying to edit both files as well.

Capture vidéo du 2023-08-25 21-03-40.webm

nlogozzo commented 1 year ago

I emailed you the files (still a Proton Drive link, one file is working, the other one isn't).

Here's a video of me trying to edit both files as well.

Capture vidéo du 2023-08-25 21-03-40.webm

Thank you!

nlogozzo commented 1 year ago

Alright so everything should be fixed in https://github.com/NickvisionApps/Tagger/releases/tag/2023.8.3 expect having both unsync and sync for flac files...going to work on that for next version :)

nlogozzo commented 1 year ago

Just note for lang code and description will be disabled now until you add an unsync or sync lyrics as tags required for it to be saved...might move the config tab to the last one as i think that'll make more sense

nlogozzo commented 1 year ago

Use #279 to track further

nlogozzo commented 1 year ago

https://github.com/NickvisionApps/Tagger/releases/tag/2023.9.0-beta1