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

Support reading differently named catalog number file tags #61

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

JOJ0
Copy link
Member

@JOJ0 JOJ0 commented Mar 21, 2022

Eg mp3tag was using DISCOGS_CATALOG a couple of years ago and is using CATALOGID, which my current tests using a trial version of mp3tag for macOS revealed.

This feature enables beets make use of these potentially available on-disk fields when importing and having musicbrainz.extra_fields: ['catalognum'] set.

on-disk fields other tagging software is using.
@JOJ0
Copy link
Member Author

JOJ0 commented Mar 21, 2022

Out of curiosity I also got in contact with the dev of mp3tag and asked how he came to the naming decision of CATALOGID. Reply pending.
The same question I would like to ask you @sampsyo for CATALOGNUM :-))) Sorry I did not do my homework of googling for what the more widely used name might be. Just asking around first ;-)

@sampsyo
Copy link
Member

sampsyo commented Mar 21, 2022

All looks good, if this is a common thing we should support! Does mp3tag have some documentation we could link to here?

Just to clarify, catalognum is an internal name, so it doesn't reflect any decision about on-disk tags. It's just an arbitrary column name for the Python side of things. The current set of on-disk tags, such as CATALOGNUMBER in Vorbis Comments, comes from Picard:
https://picard-docs.musicbrainz.org/en/appendices/tag_mapping.html#id7

@JOJ0
Copy link
Member Author

JOJ0 commented Mar 22, 2022

Hi,
sorry there was a typo in my post, I actually wanted to write CATALOGNUMBER, thus the misunderstanding. Yes catalognum is just the attributes name to access the on-disk tags, got that :-)

Unfortunately mp3tag's documentation does not include that CATALOGID field: https://docs.mp3tag.de/mapping/

But here is a screenshot of a fresh and uncustomized mp3tag for macOS installation, tagging an mp3 file from MusicBrainz:

Screenshot 2022-03-22 at 12 30 38

  • I tested mp3 files and flac files
  • It equally writes such a tag with either MusicBrainz or Discogs as the metadata source.

Regarding the "old" behaviour of mp3tag with Discogs catno's I find the term DISCOGS_CATALOG quite often on the mp3tag community forum: https://community.mp3tag.de/search?q=DISCOGS_CATALOG

These days no additional plugin/script/whatever is needed. Both MusicBrainz and Discogs are supported OOTB, which is stated here: https://docs.mp3tag.de/tag-sources/import/

@JOJ0
Copy link
Member Author

JOJ0 commented Mar 22, 2022

Something else: I had an mp3 file that had a TXXX tag named CATALOG #, and it turned out that it was read and used somehow in the import, because I saw the catalog number that was saved in that tag in the "searching musicbrainz for ......" verbose log output. I even tried it importing it again with the original file restored from backup. And it worked again :-o

Is that by any means possible? I can't find such a tag name variation in mediafile's code.

@sampsyo
Copy link
Member

sampsyo commented Mar 23, 2022

Hmm; I'd be pretty surprised if we somehow magically found a TXXX tag with description CATALOG #! I'm not sure how that would work…

Comment on lines +1972 to +1973
ASFStorageStyle('CATALOGID', read_only=True),
ASFStorageStyle('DISCOGS_CATALOG', read_only=True),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know these show up in Windows Media files too?

Copy link
Member Author

@JOJ0 JOJ0 Mar 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in contact with the mp3tag developer and asked him how exactly the WMA files CATALOGID tag looks like. He clarified that there is no prefix like WM/ and it's just the pure name. He also pointed me to the docs here, which also clarifies how exactly mp4 tags will look like: https://docs.mp3tag.de/mapping/#other-fields

As a sidenote, I also learned that WMA is actually only supported by the Windows version of mp3tag, not the macOS version.

So, yes, the tag definitions are correct like that :-)

Comment on lines +1969 to +1970
ListStorageStyle('CATALOGID', read_only=True),
ListStorageStyle('DISCOGS_CATALOG', read_only=True),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking whether we're being overzealous here… I understand that these strings might appear in the free-form tag formats, but do we actually know whether other software is putting analogous tags in stuff like MP4 frames? It seems best to be conservative and only include tags that we know are actually in use…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mp3tag supports MP4, this is how an mp4 frame looks like when tagged with it:

----:com.apple.iTunes:CATALOGID=MP4FreeForm(b'M.PM18', <AtomDataType.UTF8: 1>)

I might have understood something wrong here: I thought ListStorageStyle is ment for FLAC and Vorbis files, analogous to StorageStyle. That not the case?

@JOJ0
Copy link
Member Author

JOJ0 commented Mar 25, 2022

Furthermore, I think, great news :-)

mp3tag devloper Florian Heidenreich just told me that due to our conversation about this specific tag name, he changed the default to CATALOGNUMBER. The change is included in the latest release which just came out for Windows, the macOS release will follow: https://community.mp3tag.de/t/mp3tag-v3-13-released/56785

Also he included it in the docs: https://docs.mp3tag.de/mapping/

and it's talked about it in the forums already: https://community.mp3tag.de/t/do-we-need-to-change-catalogid-to-catalognumber-in-existing-tags/56792

👍

Since this PR was about to improve importing behaviour of older files and this changed just now in mp3tag, IMHO it certainly still is a feature addition that makes sense.

@sampsyo
Copy link
Member

sampsyo commented Mar 25, 2022

Wow; great development! Thanks for doing the homework. 😃 Can you please throw a changelog entry onto this PR and we'll call it done?

@JOJ0
Copy link
Member Author

JOJ0 commented Mar 30, 2022

Changelog added :-) Thanks!

@sampsyo sampsyo merged commit 246098b into beetbox:master Mar 30, 2022
sampsyo added a commit that referenced this pull request Mar 30, 2022
@sampsyo
Copy link
Member

sampsyo commented Mar 30, 2022

Great; thank you for your diligent work on this!! Merged! ✨

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.

2 participants