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

id3v2: Do not replace non-empty frames with duplicate, empty frames #351

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Feb 22, 2024

I have found an MP3 file with 2 comment frames. The first frame contains the actual value and the second frame is empty.

Quod Libet and mediainfo only show the first comment, while exiftool and lofty pick the second, empty comment. Many applications like Mixxx also only read the first non-empty frame and ignore any subsequent, empty frames. Even though TagLib provides access to all of them.

This fix tries to avoid loss of information and behaves like most other applications would handle those conflicts.

@Serial-ATA
Copy link
Owner

So the tag has 2 comments with the same language and description but one of them is empty? That's strange. Do you know where these files are coming from? Just to document why such a hack is necessary.

@uklotzde
Copy link
Contributor Author

So the tag has 2 comments with the same language and description but one of them is empty? That's strange. Do you know where these files are coming from? Just to document why such a hack is necessary.

No idea how this tag was created. But duplicate frames could always occur, even if they are not permitted. The on-disk-format allows this and we have to handle it somehow.

Currently, only the last frame was preserved, regardless of any preceding frames. But many applications will probably only read the first frame and ignore the rest. Picking the first non-empty frame is probably a good strategy.

@Serial-ATA
Copy link
Owner

Makes sense, thanks!

@Serial-ATA Serial-ATA merged commit e66aaab into Serial-ATA:main Feb 22, 2024
12 checks passed
@uklotzde
Copy link
Contributor Author

MusicBrainz Picard seems to add duplicate and empty comment frames to MP3 files. Maybe not always and only under certain cirumstances.

@Serial-ATA
Copy link
Owner

Is that a known issue? For an app as big as Picard something like this should be looked into.

@uklotzde
Copy link
Contributor Author

uklotzde commented Feb 25, 2024

I didn't check the details, but this issue might be related: https://tickets.metabrainz.org/browse/PICARD-2468

The files that were affected were rejected by Lofty with BadFrameLength. Removing all the tags added by MusicBrainz Picard a long time ago fixed that issue. But trying to re-add them again in MusicBrainz Picard (current version) then generated those duplicate comment tags (~4 empty + 2 with the actual comment).

@Serial-ATA
Copy link
Owner

Does your file have a comment with language="XXX"? Just tried out that issue reproducer and Picard cannot handle those comments correctly at all.

@uklotzde
Copy link
Contributor Author

Does your file have a comment with language="XXX"? Just tried out that issue reproducer and Picard cannot handle those comments correctly at all.

I didn't check. After fixing those files (using Quod Libet) that Lofty rejected I try to avoid MB Picard.

@Serial-ATA
Copy link
Owner

I went ahead and fixed that issue in Picard and now they're thinking of ways to better handle comments overall. Hopefully this issue will be a thing of the past.

@uklotzde uklotzde deleted the id3v2-duplicate-empty-frames branch March 28, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants