-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for the APIC frame type #16
base: master
Are you sure you want to change the base?
Conversation
e5b11d1
to
c92ed20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malor Thank you very much for this patch! This is a great step forward. Something I wanted implement long ago, but never dared to. I feel like we can do it this time.
src/id3manager/formats/toml.py
Outdated
mime = frame.get("mime") | ||
description = frame.get("description") | ||
type = getattr(id3.PictureType, frame.get("type", "COVER_FRONT")) | ||
|
||
frame = utils.parse_apic(frame["data"], mime, type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So TOML, in a way, becomes superior to plain text because it allows to to specify mimetype and apic type. If possible, I'd prefer us to remain compatible and express this via plain/text too.
Can we come up with some sort of custom formatting for the value?
APIC = image/png;https://example.com/logo.png;COVER_FRONT
I'm not sure that ;
is a good delimiter though. We probably should pick one that is rarely used in URLs. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. TOML is definitely more expressive, and plain text would restrict our options here... But I see your point.
What if we keep the MIME type only for "data" URLs that allow embedding images (https://developer.mozilla.org/en-US/docs/Web/URI/Schemes/data#syntax), but completely ignore it for file://
and http://
style URLs? That would leave us with two values - the URL and the image type. We could then make the latter optional (with the default value COVER_FRONT
), and separate them by
(space)?
The thinking here is that the MIME type is already ignored for "remote" URLs (the spec says to set it to -->
), and we can deduct its value for a local file.
tests/test_set.py
Outdated
text = "Podcast" | ||
|
||
[[APIC]] | ||
data = "data:image/jpeg;base64,ZHNhZmFzZmFkcw==" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. It concerns me that we may end up in a situation with two mimetypes specified. E.g.
[[APIC]]
mime = image/png
data = "data:image/jpeg;base64,ZHNhZmFzZmFkcw=="
I feel that we should probably parse in unto three independent components:
- mimetype
- data
- cover_type
And then represent those components separately in TOML, and somehow as a single line in plain/text 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can see how this can be confusing.
So the story here is that I started with the plain text format and I was looking for the most intuitive and/or "standard" way to configure this. I stumbled upon https://developer.mozilla.org/en-US/docs/Web/URI/Schemes/data#syntax, and that seemed like a great fit for embedding image data which had the look and feel of URLs and could easily be substituted with a normal http://
or a file://
URL to give people more options.
Now with TOML, we don't need to go through all that trouble, and we can simply use separate attributes. Perhaps, something like:
[[APIC]]
mime = "image/png"
data = "ZHNhZmFzZmFkcw=="
type = "COVER_FRONT"
[[APIC]]
mime = "image/png"
uri = "file://path/to/a/local/file.png"
type = "COVER_FRONT"
[[APIC]]
uri = "https://path.to/a/remote/file.jpg"
with the restriction that you can only use one of data
or uri
at the same time, and the former always expects a Base64 encoded payload and requires mime
to be specified, while the URI version only requires the location. I didn't go for this option because I wanted to make the TOML configuration follow the plain text configuration as close as possible, but, perhaps, that creates more issues than it solves...
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@malor wrote:
and the former always expects a Base64 encoded payload and requires mime to be specified
Is mimetype required in ID3 APIC frame? The ID3v2 spec sounds unclear to me:
In the event that the MIME media type name is omitted, "image/" will be implied.
What does image/
mean? Is this some kind of generic mimetype for any image type? Or do they imply that some random image mimetype should be used instead?
It's important for me to understand in order to make a concisious decision on how to proceed with mime type.
Oh.. are you suggesting mime
type to be required in order to render it in plain text format? According to Data URLs, the mime type there is optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does image/ mean? Is this some kind of generic mimetype for any image type? Or do they imply that some random image mimetype should be used instead?
https://developer.mozilla.org/en-US/docs/Web/HTTP/MIME_types claims that:
Each type has its own set of possible subtypes. A MIME type always has both a type and a subtype, never just one or the other.
So, I think, image/
is useless because it essentially means that the consumer either need to guess the correct format or just try all supported formats one by one until it finds the implementation that can successfully interpret the payload.
Oh.. are you suggesting mime type to be required in order to render it in plain text format? According to Data URLs, the mime type there is optional.
Yes. My suggestion would be to make it required as we need to put a valid MIME type value in the APIC frame to make it usable for players. I tried not setting the MIME type, and, as expected, mpv
refuses to display the cover image:
> mpv output.mp3
[ffmpeg] Unknown attached picture mimetype: , skipping.
● Audio --aid=1 (mp3 1ch 48000 Hz 115 kbps)
We can make it required, and then use data URLs in plain text (that combine the MIME type and the payload), and separate attributes in TOML.
In the plain text implementation, we would still need a way to specify the image type somehow, so my suggestion from the comment thread above is to supplement the data URL with an optional image type value (either the string name, or the hexadecimal number from https://id3.org/id3v2.3.0#Attached_picture) separated by a space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make it required, and then use data URLs in plain text (that combine the MIME type and the payload), and separate attributes in TOML.
While I agree that it's useless to have APIC w/o mimetype, I still don't understand why to enforce it. I mean, the data URLs do not require a mimetype. It's optional there:
data:;base64,ZHNhZmFzZmFkcw==
So I'm trying to understand why to enforce that?
What if are to use this project with a media file that misses mime type? It'd be unfortunate if you won't be able to set() what you previously get().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm trying to understand why to enforce that?
What if are to use this project with a media file that misses mime type? It'd be unfortunate if you won't be able to set() what you previously get().
That's a good point! We can make it optional. The default value in Mutagen is an empty string. Updated the code to not require the value to be set.
99f1e48
to
21643fb
Compare
Specification: https://id3.org/id3v2.3.0#Attached_picture ID3v2 allows storing arbitrary URLs to remote locations and directly embedding image data in MP3 files. The suggested text interface for setting the value of APIC frames is as follows: * Remote URLs (the image data will **not** be embedded) ```text APIC = http://path.to/some/image.png COVER_FRONT ``` ```toml [[APIC]] url = "http://path.to/some/image.png" picture_type = "COVER_FRONT" ``` * Local file URLs (the image data **will** be embedded) ```text APIC = file:///path/to/some/image.png COVER_FRONT ``` ```toml [[APIC]] url = "file:///path/to/some/image.png" picture_type = "COVER_FRONT" ``` * Base64 encoded image payload (the image data **will** be embedded) ```text APIC = data:image/jpeg;base64,ZHNhZmFzZmFkcw== COVER_FRONT ``` ```toml [[APIC]] data = ZHNhZmFzZmFkcw== mime_type = "image/jpeg" picture_type = "COVER_FRONT" ``` To display the data, we will: * Display remote URLs as is ```text APIC = http://path.to/some/image.png COVER_FRONT ``` ```toml [[APIC]] url = "http://path.to/some/image.png" picture_type = "COVER_FRONT" mime_type = "-->" ``` * Display embedded data as a Base64 encoded value ```text APIC = data:image/jpeg;base64,ZHNhZmFzZmFkcw== COVER_FRONT ``` ```toml [[APIC]] data = "ZHNhZmFzZmFkcw==" picture_type = "COVER_FRONT" mime_type = "image/jpeg" ```
Specification: https://id3.org/id3v2.3.0#Attached_picture
ID3v2 allows storing arbitrary URLs to remote locations and directly embedding image data in MP3 files. The suggested text interface for setting the value of APIC frames is as follows:
To display the data, we will: