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 for APEv2 cover tags #1042

Merged
merged 2 commits into from
Oct 28, 2014
Merged

Conversation

kiefermat
Copy link
Contributor

I have added support for APEv2 cover tags. There seems to be no official specifications for these tags but they are supported by several tagging tools. The origin of the tags can be found here: http://www.hydrogenaud.io/forums/index.php?showtopic=40603, sample files here: http://www.hydrogenaud.io/forums/index.php?showtopic=40860

I have created a separate APEv2ImageStorageStyle class for the APEv2 related formats. However, this results in APEv2 tags containing vorbis comment like covers no longer being recognized. I am not sure if this is a valid use case. If you want, I can change the implementation to support both formats.

@@ -868,7 +868,7 @@ class VorbisImageStorageStyle(ListStorageStyle):
base64-encoded. Values are `Image` objects.
"""
formats = ['OggOpus', 'OggTheora', 'OggSpeex', 'OggVorbis',
'OggFlac', 'APEv2File', 'WavPack', 'Musepack', 'MonkeysAudio']
'OggFlac']
Copy link
Member

Choose a reason for hiding this comment

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

Cool—I think the answer is "yes", but just to confirm: all three formats almost always use APE tags, right (WavPack, Musepack, and Monkey's Audio)? I've never run across any with Vorbis comments, but it would be great to have a second opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, all these formats use ape tags as primary metadata format. The mutagen classes also inherit from APEv2File.

@sampsyo
Copy link
Member

sampsyo commented Oct 27, 2014

Awesome! Thank you for looking into this—and for the links to the background discussion. ✨ 😃 ❗

This implementation looks solid, and I'm not to worried about losing compatibility with Vorbis-style images in APE tags. Before we hit merge, can I have your help fitting in with beets style—namely, just wrapping to 79-character lines should do well.

@sampsyo
Copy link
Member

sampsyo commented Oct 28, 2014

Fantastic! Thanks so much for fiddling with the style. I'll merge this now. ✨

@sampsyo sampsyo merged commit 4a33fe7 into beetbox:master Oct 28, 2014
sampsyo added a commit that referenced this pull request Oct 28, 2014
sampsyo added a commit that referenced this pull request Oct 28, 2014
sampsyo added a commit that referenced this pull request Oct 28, 2014
@kiefermat
Copy link
Contributor Author

Cool. Thanks for merging!

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