Add PERFORMER & COMPOSER metadata tags to audio tracks (if available)

[ABCbum]

Submitted as a part of GCI competition

Description
Composer(s) and performer(s) will be extracted from MusicBrainz recording metadata by new _getComposers and _getPerformers functions then there will be new properties added to each track metadata. If those data are present it will be tagged as new tags PERFORMER and COMPOSER.

[Freso]

I feel like the underlying logic here needs to be improved. The code itself seems fine at a glance, but some of the presumptions seem shaky… :)

[Freso]

Looking better, but still some hairy bits. I’d really recommend you try and actually edit a classical release to see it from the data entry side. :)

[Freso]

Also, looking at #191 now, it actually asks:

Maybe introduce a switch for a non-default/alternative tagging that reflects this.

What you’re doing here is just adding more tags (which, IMHO, is perfectly fine), not actually changing what’s being tagged normally. I’m writing a question on that issue to ask for clarification of what it actually is they would like to see.

[Freso]

Besides the actual code comments, I think a couple of things need to happen:

1. This needs to be unlinked from Alternative tagging for classical releases #191 as what is being done in this PR is not really what that issue asks for. I don’t think you should abandon this PR, but this PR will not solve Alternative tagging for classical releases #191, and at this point I don’t think it should aim to. It’s doing something else, and in the interest of keeping separate changes separate, Alternative tagging for classical releases #191 should be solved in a separate PR.
2. Actually, I think 1) is it. :)

I don’t think you should abandon your GCI task. I still think your work here is GCI task worthy, it just doesn’t resolve #191.

Edit: And actually, based on this, I’d say the whole logic part with --classical can probably be taken out.

[Freso]

I feel like the logic of this function makes whipper start being too bloated and going too much into tagger territory. People are vastly different in how they want they PERFORMER tags (e.g., look up some of the discussions on the MeB forums regarding this). And even then, I reckon there are one or two additional edge cases I could probably think of.

If we want this in whipper, I’d say it should be extremely simple. Maybe just get the performers and don’t capture anything about what it is they perform?

[Freso]

@JoeLametta and @MerlijnWajer input here would be appreciated though.

[JoeLametta]

As written elsewhere: I fully trust Freso's judgement about this. 😉

[Freso]

@MerlijnWajer What do you think?

[JoeLametta]

I've skimmed the code of this pull request but my knowledge about how metadata is structured in MusicBrainz is quite poor so I need to read on tomorrow (sorry).
Regarding to the performers thing I fully trust Freso's judgement.

BTW: What's the purpose of these checks (aren't these expressions always True)?

if len(composers) > 0:
...
if len(performers) > 0:
...

[ABCbum]

I reckon those checks are for cases when no work, no composer or no performer found then whipper will not add new tag just similar to the existing MUSICBRAINZ_WORKID check.

Edit: Don't worry we all are busy :D .

[ABCbum]

I don’t think you should abandon your GCI task. I still think your work here is GCI task worthy, it just doesn’t resolve #191.

@Freso then what should i do next to complete the task ?

[Freso]

BTW: What's the purpose of these checks (aren't these expressions always True)?

They’re not. E.g., https://musicbrainz.org/recording/69f6f250-e26f-4ce5-95a1-12129671ed7a will not have any performers (but will have composers) and https://musicbrainz.org/recording/a2a4f595-0201-4053-9c65-673e44c0aaa0 will have three performers (but no composers).

[Freso]

@Freso then what should i do next to complete the task ?

I mean, @MerlijnWajer is the mentor for the task. My take would be that you complete the PR for adding COMPOSER and PERFORMER tags and once that’s ready for merging, that’s the GCI task completed… and maybe we can make another task for actually doing #191. But it’s @MerlijnWajer’s call in the end.

[JoeLametta]

They’re not. E.g., https://musicbrainz.org/recording/69f6f250-e26f-4ce5-95a1-12129671ed7a will not have any performers (but will have composers) and https://musicbrainz.org/recording/a2a4f595-0201-4053-9c65-673e44c0aaa0 will have three performers (but no composers).

But in the source code of whipper/common/program.py, a few lines before those if statements, there's this:

if len(track.composers) == 0:
    track.composers = track.artist
if len(track.performers) == 0:
    track.performers = [track.recordingArtist]
composers = track.composers
performers = track.performers

Given that, to me it seems both expressions (if len(composers) > 0: and if len(performers) > 0:) will always be True, am I wrong?

[Freso]

But in the source code of whipper/common/program.py

Ah, I missed that. I think I meant for that code to be taken out, since that should be behind a classical check if used as all. (And I don’t think it should be used. Set COMPOSER and PERFORMER as per MB relationship data, and if MB has no info, then don’t include them in the tags.)

Also, that code sets composers to a string which I argued against in another comment.

[MerlijnWajer]

I'm going to rely on Freso here, I don't know enough about classical metadata.

[ABCbum]

The tests failed because the order of composers is changed after converting from a set to a list. Should i look for a way to maintain the order? (i don't think maintaining order is needed and it seems to be unnescessarily convoluted tho).

[Freso]

I’m generally happy with the code at this point. The metadata handling seems to be solid. Only one question about the amount of handling we want to be doing in whipper remains. I’ll leave it to @JoeLametta and/or @MerlijnWajer whether to merge or not from here on out.

[JoeLametta]

I think this pull request is now ready to be merged! 🎆

[JoeLametta]

@Freso Shall we merge this?

[JoeLametta]

Merged, thanks!