Martchus / tagparser

C++ library for reading and writing MP4/M4A/AAC (iTunes), ID3, Vorbis, Opus, FLAC and Matroska tags
GNU General Public License v2.0
127 stars 17 forks source link

Adding multiple TXXX frames to an ID3 tag #15

Closed Grzzlwmpf closed 2 years ago

Grzzlwmpf commented 3 years ago

I'm currently trying to add ReplayGain fields to the library as outlined in adding-new-fields.md. However, as far as I can tell, there's no way to handle multiple TXXX frames in an ID3 tag, as internallyGetKnownField returns only one value even though there may be several such fields (differing only in their description) according to the ID3 specs.

Am I missing something or does the library currently simply not implement the reading/writing of more than one arbitrary TXXX frame?

Martchus commented 3 years ago

If a mapping from one native tag field to a specific known field is not possible you could of course just try to omit it. It is generally possible to have multiple native fields with the same ID. It is just the question whether it can be nicely mapped to the generic fields. Note that the special handling of text frames in general might get in your way and you might need to touch a little bit more code than mentioned in that documentation. Also mind the difference between the multiple versions of ID3 which especially affects how multiple text frames are handled.

I already have a v10 branch for API/ABI breaking changes so if you need to change something, feel free to do so but try to keep it to a minimum.

Martchus commented 3 years ago

Maybe you can implement it in the same way the different cover types are handled? There I just have one known field for all types of covers. The different covers need then be distinguished by the application which is unfortunately not very generic but is still good enough. (Also checkout the code of my tag editor for how I do this for covers.)

Martchus commented 3 years ago

I've just had a quick look myself and apparently this is literally about the "TXXX" frame (the "XXX" is not a placeholder like I initially assumed; https://id3.org/id3v2.4.0-frames, section 4.2.6.). That means the ReplayGain isn't really like the cover and my suggestion to handle it like the cover would mean that there would be only a mapping to e.g. KnownField::UserData which would be not very convenient to use.

I also had a quick look at my code and there's no handling of the "TXXX" frame at all besides the exclusion from usual text frames. That means the frame data is read/stored/passed as-is and one has to deal with the binary structure defined in section 4.2.6 manually.

So I suppose as a first step parsing of "TXXX" frames should be implemented. I'm also wondering how the mapping should look like. Should there be one KnownField::ReplayGain and a newly added struct ReplayGain which would be returned as TagValue which needed to be extended to support it? Or we add a KnownField for every key and simply return the string?

Note that it is generally possible to override the functions to get and set values like it is already done for MP4, e.g. Mp4Tag::values(KnownField field). And speaking of other formats: Other formats should be considered as well (at least from the design perspective).

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.