search

Serial-ATA / lofty-rs

Audio metadata library
Apache License 2.0
184 stars 34 forks source link

ID3v2: URIs in APIC frames are not parsed #247

Open FriederHannenheim opened 1 year ago

FriederHannenheim commented 1 year ago

ID3v2 allows for URIs to the image in the APIC tag. This is currently ignored and will not parse the image. https://mutagen-specs.readthedocs.io/en/latest/id3/id3v2.4.0-frames.html#apic

I thought about implementing this functionality myself but the URI could point to a file or a image on the web. Since I don't think lofty should connect to the internet on it's own I would suggest to just return the URI.

Serial-ATA commented 1 year ago

I've always put off supporting those, didn't think anyone even used them, especially with them being discouraged by both the ID3v2 and FLAC picture specs.

To support this Picture could be made into an enum to support both embedded and external images. The docs for the external variant should just instruct the caller to use Picture::from_reader after looking it up. We of course will not do that ourselves.

It'd be nice to only make a special case on AttachedPictureFrame, but if we're going to support it at all, it should also extend to FLAC pictures as well since it (sadly) took after ID3v2 as well.

I don't have any time to work on this at the moment. Would you be interested in working on this approach?

uklotzde commented 1 year ago

The URI is the actual metadata. Fetching any content referenced by an URI is out of scope.

Serial-ATA commented 1 year ago

The URI is the actual metadata. Fetching any content referenced by an URI is out of scope.

It may or may not be worth it to have a special case for the external images to make them nicer to work with. They work currently, but with MimeType::Unknown("-->") and the URI as a Vec<u8>, making it no more than a Vec -> String conversion.

uklotzde commented 1 year ago

I agree that mapping URI data from Vec<u8> to String might be more convenient. But this is the maximum lofty should try to do.