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.3 multi-value field separator is always "/" #21

Open
Moonbase59 opened this issue Oct 10, 2019 · 3 comments
Open

ID3v2.3 multi-value field separator is always "/" #21

Moonbase59 opened this issue Oct 10, 2019 · 3 comments

Comments

@Moonbase59
Copy link

Moonbase59 commented Oct 10, 2019

For some libraries, users might decide to store ID3v2.3 tags for greater compatibility with older software (as opposed to the ID3v2.4 default).

Now ID3v2.4 allows storing multiple tags of the same name (for multi-value entries like artists, genres and so on) and ID3v2.3 traditionally used a slash "/" character (which brought us the "AC/DC problem").

People (and tagging software like MusicBrainz Picard) have started to support other separators to store multiple values on one ID3v2.3 field. Typical separators used today are the semicolon ;, the vertical bar |, a comma , the NULL character \0 and even character sequences like semicolon blank ; for better readability. The underlying Mutagen supports all of these by specifying v23_sep.

This is a missing feature in MediaFile (and beets) and I’d like to add it to the MediaFile class so it could also be supported via a beets config entry:

class MediaFile(object):
    """Represents a multimedia file on disk and provides access to its
    metadata.
    """
    def __init__(self, path, id3v23=False, id3v23_sep='/'):
        """Constructs a new `MediaFile` reflecting the file at path. May
        throw `UnreadableFileError`.

        By default, MP3 files are saved with ID3v2.4 tags. You can use
        the older ID3v2.3 standard by specifying the `id3v23` option
        and (optionally) the desired separator `id3v23_sep` for
        multi-value fields (defaults to a slash character).

        To use a NULL character as terminator, pass `id3v23_sep=None`.
        """

Objections?
Would it need a u'/' instead?

N.B.: Making this a core config entry in beets should also help some plugins who currently specify their own separators.

@sampsyo
Copy link
Member

sampsyo commented Oct 12, 2019

Sounds pretty reasonable to me, despite my general resistance to additional configuration options. Because Mutagen has first-class support, I can see this being effective. Here are a few miscellaneous comments:

  • MediaFile and certainly beets currently don't have great support for multi-valued tags in the first place, so realizing the full potential (e.g., superseding those plugin configuration options) will not happen automatically.
  • Any particular reason to have None stand in for the null byte? It seems more predictable to let users specify '\0' if they want to use the null byte.
  • One general question: how much does ID3v2.3 really matter? There are certainly tools that are still stuck on ID3v2.3, but I don't currently have a clear picture of which ones are really important. Windows Explorer, for example, finally added v2.4 support in 2017, AFAICT.

@Moonbase59
Copy link
Author

  • Yeah, multi-value tags in MP3s are very badly supported—everywhere I look. Maybe due to all these separator problems in ID3v2.3. ID3v2.4 was supposed to better this (and UTF-8), but it came too late so nobody cared too much anymore.

  • I’m also not too happy with None for the NULL byte, but that’s how Mutagen decided to do it.

  • You’d think everybody uses ID3v2.4 now, but actually the overwhelming majority of the files out there are still ID3v2.3, for many different reasons. My own library of about 150k tracks also still has some 120k MP3s tagged using ID3v2.3, for compatibility reasons.

Would it break anything if we started out with the extra param in MediaFile and then later care about it in beets?

JuniorJPDJ added a commit to JuniorJPDJ/mediafile that referenced this issue May 11, 2021
fixes rest of problems in beetbox#43

Shouldn't be enabled on tags like `artists` or other without confidence that values will not contain separator (eg. `artists: ['AC/DC']`).
Format of mbid is known and I'm sure we can enable it here.

When solving beetbox#21 this separator value in `el.split('/')` also should be taken into account.
JuniorJPDJ added a commit to JuniorJPDJ/mediafile that referenced this issue May 11, 2021
fixes rest of problems in beetbox#43

Shouldn't be enabled on tags like `artists` or other without confidence that values will not contain separator (eg. `artists: ['AC/DC']`).
Format of mbid is known and I'm sure we can enable it here.

When solving beetbox#21 this separator value in `el.split('/')` also should be taken into account.
@JuniorJPDJ
Copy link
Contributor

Would it break anything if we started out with the extra param in MediaFile and then later care about it in beets?

It's the idea I would like to see as it's the most reasonable IMO.

JuniorJPDJ added a commit to JuniorJPDJ/mediafile that referenced this issue May 11, 2021
…StorageStyle

fixes rest of problems in beetbox#43

Shouldn't be enabled on tags like `artists` or other without confidence that values will not contain separator (eg. `artists: ['AC/DC']`).
Format of mbid is known and I'm sure we can enable it here.

When solving beetbox#21 this separator value in `el.split('/')` also should be taken into account.
JuniorJPDJ added a commit to JuniorJPDJ/mediafile that referenced this issue May 12, 2021
…StorageStyle

fixes rest of problems in beetbox#43

Shouldn't be enabled on tags like `artists` or other without confidence that values will not contain separator (eg. `artists: ['AC/DC']`).
Format of mbid is known and I'm sure we can enable it here.

When solving beetbox#21 this separator value in `el.split('/')` also should be taken into account.
JuniorJPDJ added a commit to JuniorJPDJ/mediafile that referenced this issue May 13, 2021
…StorageStyle

fixes rest of problems in beetbox#43

Shouldn't be enabled on tags like `artists` or other without confidence that values will not contain separator (eg. `artists: ['AC/DC']`).
Format of mbid is known and I'm sure we can enable it here.

When solving beetbox#21 this separator value in `el.split('/')` also should be taken into account.
JuniorJPDJ added a commit to JuniorJPDJ/mediafile that referenced this issue May 15, 2021
…StorageStyle

fixes rest of problems in beetbox#43

Shouldn't be enabled on tags like `artists` or other without confidence that values will not contain separator (eg. `artists: ['AC/DC']`).
Format of mbid is known and I'm sure we can enable it here.

When solving beetbox#21 this separator value in `el.split('/')` also should be taken into account.
JuniorJPDJ added a commit to JuniorJPDJ/mediafile that referenced this issue May 16, 2021
…StorageStyle

fixes rest of problems in beetbox#43

Shouldn't be enabled on tags like `artists` or other without confidence that values will not contain separator (eg. `artists: ['AC/DC']`).
Format of mbid is known and I'm sure we can enable it here.

When solving beetbox#21 this separator value in `el.split('/')` also should be taken into account.
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

No branches or pull requests

3 participants