Lyrics in ID3 tag are deleted, Content descriptor is cleared and the language is set to XXX

[stifi]

When using the metadata-editor to modify lyrics stored in the ID3 tag two most likely linked issues appear:

• If there are more than one lyrics stored in a file all lyrics are deleted except the one shown in the metadata-editor
• If lyrics are modified, the Content descriptor is cleared and the language is set to XXX.

This features have been added with pull request #4890

[M-Bab]

Hmmm ... I have no idea why one should store more than one static lyrics text in an MP3 file. After all there is actually only one song text. ;-)

Concerning the technical aspects: The taglib method "setTextFrame" is protected so the way Clementine goes to remove all frames of a certain tag type before the updated text is added again. As well only the "front" element of all tag frames are read so Clementine is not optimized to read/write multilayered frames at all. Therefore if you want to edit multiframed mp3-files I wouldnt recommend the Clementine metadata-editor at all. You can still VIEW the contents without risk if you dont save the file.

I am not sure how the Contant descriptor and the language tags are affected because they are not touched by "TagReader::SetUnsyncLyricsFrame". Are you sure this is a new behavior introduced with pull request https://github.com/clementine-player/Clementine/pull/4890 or might this happened already before whenever you edited & saved an MP3-file with the metadata-editor?

[stifi]

According to the id3v2.4 specification more than one USLT frame is allowed (I use this feature for instance for translations of the lyrics). The different frames are distinguished by the language and content descriptor. However, you are right using this feature is not a common use-case. I do not expect Clementine to support showing or editing multiple lyrics in one file. Deleting them is, however, an issue we should solve. Additionally, changing the language and content descriptor is a bad behavior in my opinion.

I had a look at the code. As I have hardly an C++ knowledge, I hope I got it right. Two things draw my attention:

• https://github.com/clementine-player/Clementine/pull/4890/files#diff-0069a64354ef11891db4567e5a88a4c3R868

    while (tag->frameListMap().contains(id_vector) &&
        tag->frameListMap()[id_vector].size() != 0) {
        tag->removeFrame(tag->frameListMap()[id_vector].front());
    }

The while loop loops over all USLT frames and deletes them. So all lyrics in the file are deleted.

• https://github.com/clementine-player/Clementine/pull/4890/files#diff-0069a64354ef11891db4567e5a88a4c3R876

frame->setText(StdStringToTaglibString(value));
tag->addFrame(frame);

The text of the frame is set. However, language and content descriptor are not set (frame->setLanguage and frame->setDescription are not called). I assume TagLib uses default values then. As all frames have been deleted priorly, the information is lost.

How the code should behave in my opinion: Fetch the language and content descriptor of the displayed lyrics in the metadata-editor. After editing delete the USLT frame having these descriptors. Afterwards save the new lyrics using the old descriptors.

Regarding your question if this issue was introduced with pull request #4890. Unfortunately, I cannot answer this. However, its most likely when I look at the code fragments discussed above.

To debug the ID3 tag after modification in Clementine KID3 (preferable) or my recently published USLT Manager are quite handy.

[M-Bab]

Okay I think I got the issue now. It's a bit tricky because taglib does not like low-level editing of frame lists nor sorting the frame lists. There are several warnings about this in the documentation.

Yes so far the metadata-editor removes all frames and only sets the modified one. Not only at lyrics but also at all other frames except the ones that can be accessed directly (compare https://taglib.github.io/api/classTagLib_1_1ID3v2_1_1Tag.html).

But the following solution should be feasible only using add and remove frames:

1. Iterate through the frames and cache all existing frames as bytevectors or as lyricsframes.
2. Delete all frames.
3. Update the first cached frame 0 with the new text.
4. Add the frames again in the correct order 0 .... x (frameAdd is an append operation)

That should maintain the lyrics metadata and all other lyrics texts. When I find the time I will implement it and you can test it in my forked branch before I submit the next pull request.

[M-Bab]

Hey @stifi ,

I tried to fix the issue by caching the metadata into a byte vector. Can you test it with the code in my forked repo (https://github.com/M-Bab/Clementine) and comment if it helped?

It will still not show ALL the lyrics frames but at least it should not delete other frames anymore and also maintain existing metadata (Language ...) of the first editable frame. Further features might come in the future ;-)

When you approve that the fix resolves the problem I will do a pull request.

[stifi]

Hey @M-Bab

Tried your fix successfully -- great work! From my point of view you can create a pull request.

Regarding features: Displaying more than one lyrics is really no common use case. However, what would be nice is, if you set the Content Descriptor to Clementine Metadata-Editor or similar if new lyrics are entered. In this case it is clear which tool initially stored the lyrics in the file.