search

mono / taglib-sharp

Library for reading and writing metadata in media files
GNU Lesser General Public License v2.1
1.27k stars 312 forks source link

Id3v2, AttachmentFrame and Pictures - Are all Attachment's Pictures? #199

Open seankearney opened 4 years ago

seankearney commented 4 years ago

I believe I've found an issue with ID3V2, AttachmentFrame and specifically the Pictures property of an ID3v2 object.

I have an ID3v2 tag with images (APIC) as well as multiple GEOB frames comprising various binary ("application/octet-stream") and text data ("application/json"). When I access ID3v2.Pictures I am provided with all frames, not just picture frames. However, looking at StandardTests.cs (https://github.com/mono/taglib-sharp/blob/master/src/TaglibSharp.Tests/FileFormats/StandardTests.cs#L94) it appears that this is intended behavior since you are expecting an .m4a attachment to be returned in Pictures.

The bigger problem occurs when I attempt to set Pictures. When I attempt to set the Pictures property on the ID3v2 to a new IPicture[] all the GEOB frames are removed!

I'm curious what the intent is here. Changing https://github.com/mono/taglib-sharp/blob/master/src/TaglibSharp/Id3v2/Tag.cs#L2295 to limit Pictures to APIC only frames is a simple change, but it does break a good number of exists tests. You can take a look at my branch https://github.com/digitaldjpool/taglib-sharp/tree/199-id3v2-pictures. If I am on the right path here I can update the failing tests and send a PR.

Starwer commented 4 years ago

Hi @seankearney,

Seems like what you describe is an expected behavior.

Fact is the Tag.Pictures property doesn't represent only the pictures, but all the attachments (pictures and raw files). I realize the name "Pictures" is confusing here, but for backward compatibility it hasn't been renamed to "Attachments". Similarly, an IPicture object may represent any kind of file, picture or not. You can differentiate between the different file-types by using the IPicture.Type property: a possible value is PictureType.NotAPicture.

As a result, when you set Pictures property with an array of, strictly-speaking, pictures, you overwrite all attachements of any kind in the tags. What you should do is to assign an array containing all the files, pictures and non-pictures, to be embedded in the tag. Under the hood, the Id3v2.Tag will sort it out and store these in the file as APIC or GEOB (when Type == PictureType.NotAPicture).

I hope this clarifies.

seankearney commented 4 years ago

@Starwer I figured it was expected since tests were pretty explicit, but it is confusing as you pointed out. I also recognize backwards compatibility would be concern and this would be a breaking change.

Not sure if there is a roadmap for this project, being it is so mature, but perhaps a rename of these objects/properties is v3 worthy?