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

When Writing to a File That Uses Id3v2 Using Tag, It Erases All Items that Have Multiple Values #507

Closed
Lepidopteran opened this issue Jan 28, 2025 · 6 comments · Fixed by #508
Labels
bug Something isn't working

Comments

@Lepidopteran
Copy link
Contributor

Lepidopteran commented Jan 28, 2025

Reproducer

I tried the following code:

let path = PathBuf::from("path/to/file.mp3");

let tagged_file = match Probe::open(path) {
    Ok(tag) => tag.read().map_err(|err| err.to_string())?,
    Err(err) => return Err(err.to_string()),
};

let tag = match tagged_file.primary_tag() {
    Some(tag) => tag,
    None => return Err("No tag found".to_string()),
};

let tag_type = tag.tag_type();

for (key, value) in &[
    (ItemKey::TrackTitle, String::from("Title")),
    (ItemKey::TrackArtist, String::from("Artist")),
    (ItemKey::AlbumTitle, String::from("Album")),
    (ItemKey::AlbumArtist, String::from("Album Artist")),
    (ItemKey::TrackNumber, String::from("1")),
    (ItemKey::DiscNumber, String::from("1")),
    (ItemKey::Year, String::from("2022")),
] {
    if let Some(value) = value {
        let item = TagItem::new_checked(
            tag_type.clone(),
            key.clone(),
            ItemValue::Text(value.to_string()),
        );

        if let Some(item) = item {
            tag.insert(item);
        }
    }
}

let genre = String::from("Jazz, Vaporwave, Soundtrack")

let item = TagItem::new_checked(
    tag_type.into(),
    ItemKey::Genre,
    ItemValue::Text(genre.replace(", ", "\0")),
);

if let Some(item) = item {
    tag.insert(item);
}

tag.save_to_path(&self.path, WriteOptions::default())
    .map_err(|err| err.to_string())?;

Note: This is a simplified example of my code, The actual code uses variables that assign the tag values.

Summary

When writing to Id3v2 Tags using the generic Tag struct, it seems to be erasing tags that have multiple values and making them singular that I don't even specify.

Another odd side effect I didn't mention in the title is that when writing using Tag it also erases everything in the MusicBrainzRecordingId for some odd reason as well.

To make sure it was just Id3v2, I tried the same code with a file with Vorbis Comments, and another with Ilst and they were fine.

This seems similar to the issue #349,
I did read the workaround but I wasn't sure how to implement it with how I'm using the ItemKey,
because the Id3v2Tag expects a Frame instead of an Item.

I've generated examples of what I'm talking about:

Idv2

Original
TrackTitle:                    Spaceman the Vulnerable
AlbumArtist:                   Disasterpeace & David Peacock
TrackArtist:                   David Peacock & Disasterpeace
TrackArtists:                  David Peacock; Disasterpeace
Genre:                         Chiptune; Classical; Electronic; Instrumental
MusicBrainzReleaseId:          c04b6a89-abe3-482e-ac11-92508ce15ec8
MusicBrainzReleaseArtistId:    8a562589-9650-45f6-ab82-332625c0bf2d; 0f7d6189-7426-4454-8964-fb1327960dea
MusicBrainzReleaseGroupId:     4a0e2367-e9c6-4192-a91c-64dff5bba907
MusicBrainzRecordingId:        e6be50f5-c54c-43c1-9087-ab8e5c83b5fd
MusicBrainzTrackId:            e0f39524-dfce-4e19-a2d1-3aa11700ebb9
MusicBrainzArtistId:           0f7d6189-7426-4454-8964-fb1327960dea; 8a562589-9650-45f6-ab82-332625c0bf2d

Written with Lofty

TrackTitle:                    Spaceman the Vulnerable
AlbumArtist:                   Disasterpeace & David Peacock
TrackArtist:                   David Peacock & Disasterpeace
TrackArtists:                  Disasterpeace
Genre:                         Chiptune; Classical; Electronic; Instrumental
MusicBrainzReleaseId:          c04b6a89-abe3-482e-ac11-92508ce15ec8
MusicBrainzReleaseArtistId:    0f7d6189-7426-4454-8964-fb1327960dea
MusicBrainzReleaseGroupId:     4a0e2367-e9c6-4192-a91c-64dff5bba907
MusicBrainzRecordingId:        None
MusicBrainzTrackId:            e0f39524-dfce-4e19-a2d1-3aa11700ebb9
MusicBrainzArtistId:           8a562589-9650-45f6-ab82-332625c0bf2d

Note: The reason why genre is kept is because it is rewritten everytime.

Vorbis Comments

Original
TrackTitle:                    Holographic Harmony
AlbumArtist:                   haircuts for men
TrackArtist:                   haircuts for men
TrackArtists:                  haircuts for men
Genre:                         Chillwave; Electronic; Experimental; Experimental Electronic; Instrumental
MusicBrainzReleaseId:          4532b60f-7f1a-4db4-ac00-0fb798af797d
MusicBrainzReleaseArtistId:    87ca0b80-5c62-4316-bc23-c5d2a296a281
MusicBrainzReleaseGroupId:     376c01c2-e1dc-48d2-83ad-ca8f4871a63b
MusicBrainzRecordingId:        6a32eb94-1e07-4e9e-b879-c1fd095cc794
MusicBrainzTrackId:            dd1c4b5d-52b5-4607-a342-51a46d9ed080
MusicBrainzArtistId:           87ca0b80-5c62-4316-bc23-c5d2a296a281
Written with Lofty
TrackTitle:                    Holographic Harmony
AlbumArtist:                   haircuts for men
TrackArtist:                   haircuts for men
TrackArtists:                  haircuts for men
Genre:                         Chillwave\0Electronic\0Experimental\0Experimental Electronic\0Instrumental
MusicBrainzReleaseId:          4532b60f-7f1a-4db4-ac00-0fb798af797d
MusicBrainzReleaseArtistId:    87ca0b80-5c62-4316-bc23-c5d2a296a281
MusicBrainzReleaseGroupId:     376c01c2-e1dc-48d2-83ad-ca8f4871a63b
MusicBrainzRecordingId:        6a32eb94-1e07-4e9e-b879-c1fd095cc794
MusicBrainzTrackId:            dd1c4b5d-52b5-4607-a342-51a46d9ed080
MusicBrainzArtistId:           87ca0b80-5c62-4316-bc23-c5d2a296a281

Note: Corruption of genre tag is caused by me rewriting it using the null separator \0.

Ilst

Original
TrackTitle:                    VHS Vortex
AlbumArtist:                   haircuts for men
TrackArtist:                   haircuts for men
TrackArtists:                  haircuts for men
Genre:                         Chillwave 
MusicBrainzReleaseId:          4532b60f-7f1a-4db4-ac00-0fb798af797d
MusicBrainzReleaseArtistId:    87ca0b80-5c62-4316-bc23-c5d2a296a281
MusicBrainzReleaseGroupId:     376c01c2-e1dc-48d2-83ad-ca8f4871a63b
MusicBrainzRecordingId:        cc3d1560-e249-4cc2-aaf8-740730a4d27c
MusicBrainzTrackId:            527e8b2c-a88d-482d-a5d5-cb8fb95c12ab
MusicBrainzArtistId:           87ca0b80-5c62-4316-bc23-c5d2a296a281
Written with Lofty
TrackTitle:                    VHS Vortex
AlbumArtist:                   haircuts for men
TrackArtist:                   haircuts for men
TrackArtists:                  haircuts for men
Genre:                         Chillwave 
MusicBrainzReleaseId:          4532b60f-7f1a-4db4-ac00-0fb798af797d
MusicBrainzReleaseArtistId:    87ca0b80-5c62-4316-bc23-c5d2a296a281
MusicBrainzReleaseGroupId:     376c01c2-e1dc-48d2-83ad-ca8f4871a63b
MusicBrainzRecordingId:        cc3d1560-e249-4cc2-aaf8-740730a4d27c
MusicBrainzTrackId:            527e8b2c-a88d-482d-a5d5-cb8fb95c12ab
MusicBrainzArtistId:           87ca0b80-5c62-4316-bc23-c5d2a296a281

Note: Looks like get_strings does not work with Ilst. :/

I have more examples but this summary is getting long enough.

TLDR

When writing to Id3v2 Files with Tag it removes multiple strings from all items not just the ones I specify and replaces it with the first string.
How would you recommend me go forward to fix this issue?

Expected behavior

When writing a Id3v2 files using Tag, keep the items that have multiple values.

Assets

No response

@Lepidopteran Lepidopteran added the bug Something isn't working label Jan 28, 2025
@Serial-ATA
Copy link
Owner

Thanks for all the details.

I haven't looked into it yet, but some observations:

let item = TagItem::new_checked(
    tag_type.into(),
    ItemKey::Genre,
    ItemValue::Text(genre.replace(", ", "\0")),
);

For multi-value items, you should make a new TagItem for each and use Tag::push():

for genre in genre.split(", ") {
    tag.push(TagItem::new(
        ItemKey::Genre,
        ItemValue::Text(genre.to_string())
    ));
}

When merging a Tag back into an Id3v2Tag, it expects that multiple items exist, rather than a single item with separators:

let mut iter = tag.take_strings(key);
let Some(first) = iter.next() else {
continue;
};

I did read the workaround but I wasn't sure how to implement it

Every concrete tag type implements From<Tag>, so you can do:

let tag = match tagged_file.primary_tag() {
    Some(tag) => tag,
    None => return Err("No tag found".to_string()),
};

// ...

if tag.tag_type() == TagType::Id3v2 {
	let id3_tag: Id3v2Tag = tag.into();
	id3_tag.save_to_path(/* ... */)?;
} else {
	tag.save_to_path(/* ... */)?;
}

Not sure if the workaround applies here or not, I'll have to look into it.

Another odd side effect I didn't mention in the title is that when writing using Tag it also erases everything in the MusicBrainzRecordingId for some odd reason as well.

That's weird. All of the MusicBrainz tags use the same logic, so if one disappears then they all should.

@Lepidopteran
Copy link
Contributor Author

Lepidopteran commented Jan 28, 2025

Thank you for the quick reply!

I tried your suggestions, specifically the converting the tag to a Id3v2 and pushing the genres instead of inserting them using a separator, and they worked partially.

Id3v2 doesn't erase everything in MusicBrainzRecordingId and TrackArtists preserves the multiple artists instead of replacing it with the first artist.

However It still makes every MusicBrainz tag into a singular value.

I generated the test result with the Id3v2 tag, I removed the Vorbis Comments because your suggested fix for genres fixed the problem from what I can see, and Ilst produced no difference.

I'll create more results with different files with Vorbis Comments and Ilst with multiple MusicBrainz tags and or multiple artists when I find some.

Id3v2

Original
TrackTitle:                    Spaceman the Vulnerable
AlbumArtist:                   Disasterpeace & David Peacock
TrackArtist:                   David Peacock & Disasterpeace
TrackArtists:                  David Peacock; Disasterpeace
Genre:                         Chiptune; Classical; Electronic; Instrumental
MusicBrainzReleaseId:          c04b6a89-abe3-482e-ac11-92508ce15ec8
MusicBrainzReleaseArtistId:    8a562589-9650-45f6-ab82-332625c0bf2d; 0f7d6189-7426-4454-8964-fb1327960dea
MusicBrainzReleaseGroupId:     4a0e2367-e9c6-4192-a91c-64dff5bba907
MusicBrainzRecordingId:        e6be50f5-c54c-43c1-9087-ab8e5c83b5fd
MusicBrainzTrackId:            e0f39524-dfce-4e19-a2d1-3aa11700ebb9
MusicBrainzArtistId:           0f7d6189-7426-4454-8964-fb1327960dea; 8a562589-9650-45f6-ab82-332625c0bf2d
Written using lofty
TrackTitle:                    Spaceman the Vulnerable
AlbumArtist:                   Disasterpeace & David Peacock
TrackArtist:                   David Peacock & Disasterpeace
TrackArtists:                  David Peacock; Disasterpeace
Genre:                         Chiptune; Classical; Electronic; Instrumental
MusicBrainzReleaseId:          c04b6a89-abe3-482e-ac11-92508ce15ec8
MusicBrainzReleaseArtistId:    0f7d6189-7426-4454-8964-fb1327960dea
MusicBrainzReleaseGroupId:     4a0e2367-e9c6-4192-a91c-64dff5bba907
MusicBrainzRecordingId:        e6be50f5-c54c-43c1-9087-ab8e5c83b5fd
MusicBrainzTrackId:            e0f39524-dfce-4e19-a2d1-3aa11700ebb9
MusicBrainzArtistId:           8a562589-9650-45f6-ab82-332625c0bf2d

@Serial-ATA
Copy link
Owner

Serial-ATA commented Jan 28, 2025

Ah, so for the MusicBrainz keys, they were just never added here:

// Multi-valued TXXX key-to-frame mappings
for item_key in [
&ItemKey::TrackArtists,
&ItemKey::Director,
&ItemKey::CatalogNumber,
] {

So what ends up happening is they fall through to this:

// Insert all remaining items as single frames and deduplicate as needed
for item in tag.items {
merged.insert_item(item);
}

Which doesn't join them together, it inserts them one by one. That means that only the last ID will actually be saved. That's a simple fix.

Did the workaround for #349 fix anything? I'm not sure if that's a related issue here.

@Lepidopteran
Copy link
Contributor Author

Which doesn't join them together, it inserts them one by one. That means that only the last ID will actually be saved. That's a simple fix.

That's great! Should I create a PR to change that or would you like to handle it?

Did the workaround for #349 fix anything? I'm not sure if that's a related issue here.

Yes actually, The TrackArtists are preserved by converting the Tag into an Id3v2Tag!
But as for the MusicBrainz tags I do not believe that is a related issue.

@Serial-ATA
Copy link
Owner

Should I create a PR to change that or would you like to handle it?

If you could, that'd be great! All the variants are here:

///////////////////////////////////////////////////////////////
// MusicBrainz Identifiers
/// MusicBrainz Recording ID
///
/// Textual representation of the UUID.
///
/// Reference: <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#id21>
MusicBrainzRecordingId,
/// MusicBrainz Track ID
///
/// Textual representation of the UUID.
///
/// Reference: <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#id24>
MusicBrainzTrackId,
/// MusicBrainz Release ID
///
/// Textual representation of the UUID.
///
/// Reference: <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#id23>
MusicBrainzReleaseId,
/// MusicBrainz Release Group ID
///
/// Textual representation of the UUID.
///
/// Reference: <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#musicbrainz-release-group-id>
MusicBrainzReleaseGroupId,
/// MusicBrainz Artist ID
///
/// Textual representation of the UUID.
///
/// Reference: <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#id17>
MusicBrainzArtistId,
/// MusicBrainz Release Artist ID
///
/// Textual representation of the UUID.
///
/// Reference: <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#id22>
MusicBrainzReleaseArtistId,
/// MusicBrainz Work ID
///
/// Textual representation of the UUID.
///
/// Reference: <https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#musicbrainz-work-id>
MusicBrainzWorkId,
///////////////////////////////////////////////////////////////

With the exception of MusicBrainzRecordingId. That one has special handling in ID3v2 (it becomes a UFID frame).

The TrackArtists are preserved by converting the Tag into an Id3v2Tag!

I haven't looked into that issue in awhile, but I think it's a little more involved. You may need to keep using that workaround for a bit until I can get around to it.

@Lepidopteran
Copy link
Contributor Author

Lepidopteran commented Jan 30, 2025

You may need to keep using that workaround for a bit until I can get around to it.

That's fine! It only adds a couple of lines of code for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants