Tags with multiple values

[geigerzaehler]

These are first steps towards multivalued tags (see #505). As of now, the branch includes

• Refactoring of tests
• Refactoring of the StorageStyle implementation
• Experimental support for genre lists in MP3 files.

It does not break the current Mediafile API. It also doesn't interface the additions with other parts of beets*, e.g. the library.

Rationale

Lists are of prime importance for genres (see #119, #437). In addition, there are several fields not currently in the beets core that would benefit from lists. Most specifically MusicBrainz relations like "performer", "composer", etc.

I am pretty sure there are many more use cases out there and I would be glad to hear about them to inform the implementation.

Storing Lists in Tags

I am not yet sure how to implement this and there are many points that factor into the decision. Comments are highly appreciated.

• Does a format support lists natively? I surveyed this for ID3 and Ogg below. But I don't know anything about the other formats. Input would be appreciated
• How does *mutagen treat lists?* I think mutagen generally treats tags as lists. It is however not clear if correct handling is supported in all formats, or whether the handling is idempotent.
• How does other software treat lists? I am not aware of any player that show lists of some metadata. For lists in ID3 text frames (see below) most players display them as space seperated strings and destroy this information when writing tags.

As an example, I implemented genre lists for MP3 files. There is a new genres property in the MediaFile API. It allows for getting and setting lists of genres. Tests can be found in the MP3Test class. At the moment we use 0x00 seperated strings to store lists (see below).

List of tags for ID3

ID3 has two different ways of storing lists. First, text frames (frames whose identifier starts with "T", excluding "TXXX", see Frames,4.2) may contain lists of strings seperated by the terminal character of the encoding (0x00 for UTF8). Secondly, there are several frames that can appear multiple times, but have to be unique up to an additional parameter (e.g. description for "TXXX"). This restricts storing lists severly.

I know how mutagen handles this so writing code for this should not be an issue.

List of tags for VorbisComment

Storing lists is well defined for Vorbis comments. In fact storing single values is just a special case of storing a list with one element. Hence, we also should expect no issues here.

Other Formats

I have not yet looked into other tagging schemes. Any input would be appreciated.

Implementation details

Test refactoring

The tests in test_mediafile_basic.py were refactored to be more manageable. Each file type has its own test case with some general tests mixed in from ReadWriteTestBase and possibly PartialTestMixin. I also added more files with empty tags to test against.

Mediafile refactoring

Previously, all the code responsible for interacting with mutagen was contained in the MediaField class. This was moved into the StorageStyle class. Then, instead of checking fields like style.id3_desc, I used subclasses to implement the file type specific behaviour. I am not sure whether this refactoring did change the behaviour. If so, this is not covered by the test cases. Help in spotting bugs would be appreciated.

[geigerzaehler]

Thanks for the fireworks 😊.

I've implemented genre lists for all formats and it seems to work smoothly. Eeven for APE tags he tests work fine. Do you know why? I also started work on refactoring the image storage, with the goal to support multiple covers with descriptions.

As for the questions

Do we want to provide a list version of every field, just to cover our bases? If so, is there a more elegant way to do this than creating a plural name for each field in turn? If not, how do we decide which fields to provide list version of?

I think we should restrict ourselves to what is needed or actually requested from users. So I think at the moment genres and images should suffice.

We should add list access to the album art fields as well (eventually).

Did not think of this initially but I'm working on it.

I think the cleanest option is just to return the first element of the list, as it does now...

I agree.

...but that raises questions about how we should change the beets database—I assume we should not attempt to store genre but only genres.

Again, I agree. When we start using the list interface in other parts of beets we should try to avoid the singular property by all means and let the caller handle any kind of normalization. We can then deprecate singular access.

One alternative would be to use something friendlier, like commas, which we normalize across all format types, but that has its own obvious issues.

I'm not quite sure what you have in mind. If you want to persist lists as comma separated values, then obtaining the list would involve some custom (de)serialization logic. I don't think that's a viable option. I could imagine that storing text data as null-separated strings in the database might work.

[geigerzaehler]

There is a new feature in town 👢. With 1f01997 media files are only written it the tags actually change

[sampsyo]

Eeven for APE tags he tests work fine. Do you know why?

Looking closer at the implementation, it looks like Mutagen will transparently insert null bytes to separate lists in APE tags.

>>> f['GENRE'] = ['a', 'b']
>>> f['GENRE']
APETextValue('a\x00b', 0)

So we may need to be careful here about stripping these when accessing the plain genre field (maybe you're already doing this).

I think we should restrict ourselves to what is needed or actually requested from users. So I think at the moment genres and images should suffice.

That sounds reasonable. FWIW, #505 notes that artist (and mb_artistid) could also benefit from multiple values.

We should add list access to the album art fields as well (eventually).

Did not think of this initially but I'm working on it.

Cool. We can also take care of album art fields in a future update too (if you want to just focus on the core functionality for now).

When we start using the list interface in other parts of beets we should try to avoid the singular property by all means and let the caller handle any kind of normalization. We can then deprecate singular access.

Sounds good. Let's worry about this later, though, and focus on the MediaFile stuff in this PR.

I don't think that's a viable option.

You're right; that proposal didn't really make sense. 😃

[geigerzaehler]

So we may need to be careful here about stripping these when accessing the plain genre field (maybe you're already doing this).

Mutagen already does this for us.

That sounds reasonable. FWIW, #505 notes that artist (and mb_artistid) could also benefit from multiple values.

For the artist field this is a bit tricky since musicbrainz uses join phrases to generate a string for display; mb_artistid should work though. The composer field would be easier to handle. However right now i think we should add list fields only if code actually uses them. Otherwise we might get bloated code that is not usable.

Cool. We can also take care of album art fields in a future update too (if you want to just focus on the core functionality for now).

I migrated the ImageField to the refactored code. It should be easy to extend to support lists and more stuff (thinking about mime, description, cover type, etc.)

I also removed the LazySave feature which sure belongs in a separate PR. I will open it as soon as this PR is merged as the feature depends on the rewritten unittests.

If there are no objections I think we are ready to merge this!

[sampsyo]

All sounds good, and the PR is looking great!

I agree that this is ready to merge. Would you mind, however, if we held off until after the 1.3.3 release? There are already lots of changes in this release and I'd like to finish things off and get it out the door in the next couple of days. If we can avoid rocking the boat with another major refactoring until after then, I think we can give this feature the love that it needs.

I'll try to redouble my efforts to get 1.3.3 finished as soon as possible.

[geigerzaehler]

Would you mind, however, if we held off until after the 1.3.3 release?

That's fine by me.

[sampsyo]

Merged for v1.3.4! Thanks again for all your effort on this.

Here's what's still on my to-do list in this department:

• Some documentation for the new StorageStyle hierarchy that you've added. This was a great idea and vastly simplifies the handling of different file types. I'd like to flesh out some docstrings in this department to make the code easier to read & digest. (This is especially relevant since I think it's getting close to time for MediaFile to leave the nest and be a separate, reusable library—in which case documentation will be particularly important.)
• I'm still interested in dreaming up an elegant API solution for providing both single- and multiple-valued versions of arbitrary fields. I'd like to avoid touching too much code whenever we find a need for another list-valued version.

I'll hack away on both of these over the next week or so.