Adding metadata fields / tags maybe is too complex

[sandreas]

Hello,

I took a look into #121 and what would be needed to add the following metadata fields:

• string SortAlbum
• string SortAlbumArtist
• string SortArtist
• string SortTitle
• string Group
• string MovementName (or SeriesTitle)
• string MovementIndex (or SeriesPart - string because of part 2.1, etc.)
• string LongDescription

Now I noticed, that there is much work to do per field. Let's say I would like to add Group, here is the list of things to do:

• class TagData
  • add public const byte TAG_FIELD_GROUP = 30;
  • add public string Group = null;
  • extend public void IntegrateValue(byte key, string value) to contain a case case TAG_FIELD_GROUP: Group = value; break;
  • extend public IDictionary<byte, string> ToMap() with addIfConsistent(Group, TAG_FIELD_GROUP, result);
  • extend public void Clear() with Group = null;
• class Track
  • add public string Group { get; set; }
  • extend protected void Update(bool onlyReadEmbeddedPictures = false) with Group = Utils.ProtectValue(fileIO.Group);
  • extend private TagData toTagData() with result.Group = Group;
• interface IMetaDataIO
  • extend with string Group { get; }
• class AudioFileIO
  • extend with public string Group { get { return processString(metaData.Group); } }
• CrossMetadataReader
  • add public String Group
• AND THE MOST WORK: extend all files in directory IO with a valid frameMapping for group (depending on which key is valid for every supported metadata format)

That seems to be a lot of code for one extra field. After reading through other parts of the code, I came up with the following ideas, to improve some of the duplicated parts:

• Extract new Interface IMetadata from IMetadataIO, keeping only the properties, not the methods
• Extract IAudioData from IAudioDataIO, keeping only the properties
• Make IAudioFile extending from IMetadata and IAudioData
• Make TagData implement IMetadata
  • Problem: TagData is string-only typed (e.g. PublishingDate is string, not DateTime )
  • Suggestion: Use typed properties from IMetadata, implement <Property>AsString to be compatible (e.g. ReleaseDateAsString => ReleaseDate.ToString())
• Make Track extend TagData and implement IAudioFile (instead of toTagData etc.) OR use a Track.TagData field containg the whole Metadata container

This way there would be less duplication and complexity, adding Group only to TagData and integrating it into the mapping logic.

I also thought about instead Mapping things in EVERY IO class (e.g. MP4.cs) just reading the additional fields and converting all properties directly in Track.cs (this way the whole mapping would be done in Track.cs and you would only have to change one file):

class Track {

    private Format _format { get; set; }
    public IDictionary<string, string> AdditionalFields { get; set; }

    public string? Group
    {
        get => GetAdditionalField(_format);
        set => SetAdditionalField(_format, value); // if _format == MP4, key for Group = ©nam
    }

    private void SetAdditionalField<T>(Format format, T? value, [CallerMemberName] string key=null)
    {
        var mappedKey = MapAdditionalField(format, key);
        if (mappedKey == null)
        {
            return;
        }

        if (value == null)
        {
            if (AdditionalFields.ContainsKey(mappedKey))
            {
                AdditionalFields.Remove(mappedKey);
            }

            return;
        }
        AdditionalFields[mappedKey] = Stringify(value);
    }

    private string Stringify<T>(T value)
    {
        if (value is string s)
        {
            return s;
        }

        if (value is DateTime d)
        {
            return d.ToString();
        }

        if (value is int i)
        {
            return i.ToString();
        }
        return "";
    }

    private string? GetAdditionalField(Format format, [CallerMemberName] string key=null)
    {
        var mappedKey = MapAdditionalField(format, key);
        if (mappedKey == null)
        {
            return null;
        }
        return AdditionalFields.ContainsKey(mappedKey) ? AdditionalFields[mappedKey] : null;
    }

    // this is new
    public string? MapAdditionalField(Format format, string key) => format.ID switch
    {
        // Format is not really appropriate, because it should be the TaggingFormat (id3v2, etc.), not the audio file format
        AudioDataIOFactory.CID_MP3 => MapMp3(key),
        AudioDataIOFactory.CID_MP4 => MapMp4(key),
        _ => null
    };
    
    private static string? MapMp3(string key) => key switch
    {
        nameof(Group)    => "TIT1",
        _ => null,
    };
    
    private static string? MapMp4(string key) => key switch
    {
        nameof(Group)    => "©grp",
        _ => null,
    };
    // ...
}

Other ideas / questions:

• Make public const byte TAG_FIELD_GENERAL_DESCRIPTION = 0; in TagData an extra enum TagFields
• Why is there Track.EmbeddedPictures AND Track.PictureTokens in IMetadataIO (same type, different purpose)?
• Why Track.Duration is not a TimeSpan?

Sorry for this huge post, I hope I'm not wasting your time.

[Zeugma440]

You surely aren't wasting my time, don't worry ^^

I've been developing ATL as a pet project since 2006, and while I've tried to keep things as clean as possible, there are inevitably :

• Things that I didn't see... or didn't want to see 😅
• Things that are still there as legacy from the past, for good... and no-so-good reasons

That being said, let's start with the easy ones :

Why is there Track.EmbeddedPictures AND Track.PictureTokens in IMetadataIO (same type, different purpose)?

That's because back when it started, ATL didn't support embedded pictures and was just signaling their presence in the file.

That part's still there now, even though I'm not sure it's used at all. Deprecating it to drop it in the next major release might be a good idea for clarity's sake.

Why Track.Duration is not a TimeSpan?

Because it didn't occur to me to do that. Representing a duration with a long is straightforward enough, and no user has expressed the need to use TimeSpan so far, as millisecond precision is more than enough to handle audio file management use cases. I'm not certain changing that will make such a difference.

Make public const byte TAG_FIELD_GENERAL_DESCRIPTION = 0; in TagData an extra enum TagFields

Lists of constants that are not enums are typically legacy code from the past I never bothered to refactor. Upgrading that is a good idea.

---

Now for the main dish 😉

I do share your impression that there are too many steps to take to add one single field. I'd love to find a solution to simplify that while keeping everything else intact functionally 👍

However, regarding mapping things in every IO class, that is completely intended, as low-level specifics for a given format should be handled in one single place by design. I really want to keep most of the code as generic as possible while isolating format-specific stuff into the IO classes exclusively.

So far, the only way I see where we could reach an agreement would be to move these mapping to a single JSON file that every IO class would have access to at class load time.

Before we develop all that, there are two more importants things to consider, architecture-wise :

1/ A given audio file can be tagged with multiple tagging systems (e.g. an MP3 file can host both an APE tag and an ID3v2 tag, an AIFF file can host both its own native tag plus an ID3v2 tag).

Consequently, ATL has to support the following use cases :

• Reading data from one specific tag
• Reading all data from all tags, keeping any field that is has a value in one tag or the other
• Modifying/removing just one tag and keeping the others as is

2/ ATL can be used in two ways by client apps :

• The simple way, using just Track
• The surgical way, using AudioDataIOFactory and MetaDataIOFactory to instanciate classes to handle a specific type of data

I'll write down reactions and refactoring ideas in an upcoming post !

[sandreas]

Thank you for this detailed response and the good explanations.

I'm not certain changing that [Duration as TimeSpan] will make such a difference.

Agreed. For debugging and formatting purposes it would be nice to have, but definetely not a must have

So far, the only way I see where we could reach an agreement would be to move these mapping to a single JSON file that every IO class would have access to at class load time.

While a single json file seems to be a good idea, in my opinion it should not be shipped and parsed at runtime but used to autogenerate the mapping code in C#, which is faster and easier to ship. Maybe this json file could also be RFCed in the future for all commonly used tag mappings and made available for other projects to generate code in other programming languages in their libraries. That way there would be kind of a default way to do mappings.

Before we develop all that, there are two more importants things to consider, architecture-wise [addressing multiple metadata formats per file]

That is a problem I stumbled over while extending Track for my app, too. The property AudioFormat does not represent the metadata format being used to read or write, but the binary file format. So a valid mapping of AdditionalFields cannot be fully determined. Example:

        switch (AudioFormat.ID)
        {
            case AudioDataIOFactory.CID_MP3:
                // should I read / write APE, ID3v2 or both to additional properties?
                AdditionalFields["TIT1"] = "myGroup"
                break;
            // ...
        }
		

Maybe it would be more accurate to have MetadataContainer Metadata; like this:

enum MetadataFormat
{
    Native,
    Id3v23,
    Ape,
    // ...
}

class Metadata : IMetadata // !!! Use IMetadata to ensure every class uses the same base properties
{
    public MetadataFormat Format { get; set; } = MetadataFormat.Native;

    public string? Group { get; set; }
    // ... other IMetadata properties
}

class MetadataContainer: IMetadata { // !!! Use IMetadata to ensure every class uses the same base properties
    public IList<Metadata> Formats { get; set; }
	
    public Metadata Id3v23 => Formats.FirstOrDefault(f => f.Id = MetadataFormat.Id3v23);

    public string? Group
    {
        get => Formats.FirstOrDefault(f => !string.IsNullOrEmpty(f.Group))?.Group;
        set
        {
            foreach (var f in Formats)
            {
                f.Group = value;
            }
        }
    }
}

class Track: IMetadata  // !!! Use IMetadata (or a derived interface) to ensure every class uses the same base properties
{
    public MetadataContainer Metadata { get; set; }
}

That way it would be possible to access specific formats while still being able to wrap things in all desired classes... but this is stuff for the future - and maybe overengineering.

next steps...

Deprecating it [PictureTokens] to drop it in the next major release might be a good idea for clarity's sake.

Lists of constants that are not enums are typically legacy code from the past I never bothered to refactor. Upgrading that [TagData.TAG_FIELD_* => enum] is a good idea.

What would be good to have I think is an extensive listing of internal property names, that represent information and could be mapped to tag properties. To my knowledge the best match for this is currently IMetadataIO - but it also has methods and non-metadata things, so a little extraction would be needed.

Why IMetadata with only properites?
I think for other projects using atldotnet as library it would be very good to have such an interface. I implemented this myself and use it everywhere, especially in a concept of IMetadataImprover where I run several metadata improvers in a composite over an IMetadata instance to add tagging information from different sources (like Internet APIs, files beneath the audio file, command line parameters and even defaults, if something is a property combined out of others).

Like Elvis said, I would now take these actions instead of having more conversation:

• Create IMetadata as the all internal properties for mapping container from IMetadataIO without:
  • Exists
  • MetadataFormats
  • PaddingSize
  • PictureTokens - deprecated in favor of `EmbeddedPictures
  • Size
  • SetEmbedder(..) - method
  • Read(..) - method
  • Write(..) - method
  • Remove(..) - method
  • Clear(..) - method
• Extend IMetadataIO from IMetadata containing everthing that has not been extracted, so everthing would work fine
• Make Track implement IMetadata
• Refactor TagData.TAG_FIELD_* as enum
• Submit a PR with these changes

Are you ok with this?

[Zeugma440]

Just pushed something nice to the new tagdata-refactor branch 😁

The only difference with what you suggested is Track is still standalone, not implementing any interface, as it publishes nullable fields while most of the app doesn't work with those.

Apart from that, it all comes nicely, and reduces the parts to edit when adding new fields, that are now :

• New method signature in IMetaData
• New method in CrossMetaDataReader
• New value in the TagData.Field enum
• New property in MetadataHolder (= mapping with TagData)
• New property in TagHolder <= WIP; will probably be merged with MetadataHolder in the near future
• New property in Track
• New lines in Track.Update (= formatting what comes out of IMetaData)
• New lines in Track.toTagData (= mapping properties to TagData)

[sandreas]

Ok, then I would take my hands off for now. BTW, did you see this?
https://dustri.org/b/horrible-edge-cases-to-consider-when-dealing-with-music.html
Discussion: https://news.ycombinator.com/item?id=30892811

I thought this might be interesting for improving the automated tests :-)

[Zeugma440]

Ok, then I would take my hands off for now

Yeah, the TagHolder vs. MetadataHolder thingy might look trivial but it isn't when you ask yourself what needs to be tested, and on which level of detail. Please hold until I greenlight this branch.

BTW, did you see this?

I'm fairly confident with ATL being able to handle most of what's described in that article, except maybe the empty album names / titles, which brings us back to the "nullable vs. empty" debate. These cases have more value to test a music organizer software's data quality rules imho.

The discussion also has much value. The topic that invariably comes when talking about complex tags / formatting is classical music. I've put that in the back of my mind for quite a while, as I'm also interested in data modeling. That doesn't quite fit into ATL's main goal but could be published as an extension or companion library.

[sandreas]

which brings us back to the "nullable vs. empty" debate

Well, in my opinion <empty string> should lead to an existing value with an empty string, while <null> should lead to removing the value completely. If the value is mandatory, <null> must be converted to the only sane default <empty string>. The last part is important. If the binary format does not specify a value being removed completely, <null> must lead to fallback value. That is also the case for other types. Handling <null> values with other types (int, DateTime, etc.) should lead to removal, if specified. If not, a sane default. Thats also why I think, that IMetadata should allow null values for EVERY property, except Listings (Chapters, AdditionalFields, etc.). Just my 2 cents :-)

Regarding atldotnet, I will now focus on implementing a class MetaDataTrack : Track in my project that contains a valid mapping for all the AdditionalFields, that are not already mapped by atldotnet itself and I need for tone to work like expected. Therefore I'm gonna leave issue #121 up to you, until you say something else. If you need help with any other things, feel free to ask. Otherwise may I thank you for this really meaningful discussion and maintaining atldotnet.

[sandreas]

What do you think about the null vs <empty string> situation? I think IMetadata should have all scalar fields nullable, and if so, null means, remove this tag. Wouldn't that be better than having to use <empty string> to remove a field?

[Zeugma440]

You are totally right about that. However, this requires yet another major overhaul as the whole library and test suite is not really what I would call null-proof 😅

I'm gonna handle that date formatting suggestion you just made, release a new version and get to work on #121 after that. What we're discussing right now is less important imho, as it doesn't really add any feature for the end user, just better handling of some edge cases.

[sandreas]

I'm gonna handle that date formatting suggestion you just made, release a new version and get to work on #121 after that. What we're discussing right now is less important imho, as it doesn't really add any feature for the end user, just better handling of some edge cases.

You are really kind. Thank you very much. I really don't wanna distract you, but your library inspires me to have some great (or not so great) ideas :-) Maybe I should tell you, WHY I think that null would be an improvement. I plan to make tone be able to remove tags. I did not find this documented but I saw your comment in an issue, that setting a field to (or in case of DateTime to DateTime.MinValue will remove the field. So maybe I show the the class, that I have been working on to show you the idea:

public class MetadataTrack : Track, IMetadata
{
    // meet IMetadata interface requirements
    public new string? Path => base.Path;

    public DateTime? RecordingDate
    {
        get => Date;
        set => Date = value;
    }

    // be able to set totalDuration, if it has not been detected or its a dummy track
    private TimeSpan? _totalDuration;

    public TimeSpan TotalDuration
    {
        get => _totalDuration ?? TimeSpan.FromMilliseconds(DurationMs);
        set => _totalDuration = value;
    }
    
    private static readonly List<(string, Action<MetadataTrack>)> RemoveFieldCallbacks = new()
    {
        (nameof(Bpm), track => track.Bpm = null),
        (nameof(EncodedBy), track => track.EncodedBy = null),
        (nameof(EncoderSettings), track => track.EncoderSettings = null),
        (nameof(Subtitle), track => track.Subtitle = null),
        (nameof(ItunesCompilation), track => track.ItunesCompilation = null),
        (nameof(ItunesMediaType), track => track.ItunesMediaType = null),
        (nameof(ItunesPlayGap), track => track.ItunesPlayGap = null),
        (nameof(DiscNumber), track => track.DiscNumber = null),
        (nameof(DiscTotal), track => track.DiscTotal = null),
        (nameof(TrackNumber), track => track.TrackNumber = null),
        (nameof(TrackTotal), track => track.TrackTotal = null),
        (nameof(Popularity), track => track.Popularity = null),
        (nameof(Title), track => track.Title = null),
        (nameof(Artist), track => track.Artist = null),
        (nameof(Composer), track => track.Composer = null),
        (nameof(Comment), track => track.Comment = null),
        (nameof(Genre), track => track.Genre = null),
        (nameof(Album), track => track.Album = null),
        (nameof(OriginalAlbum), track => track.OriginalAlbum = null),
        (nameof(OriginalArtist), track => track.OriginalArtist = null),
        (nameof(Copyright), track => track.Copyright = null),
        (nameof(Description), track => track.Description = null),
        (nameof(Publisher), track => track.Publisher = null),
        (nameof(AlbumArtist), track => track.AlbumArtist = null),
        (nameof(Conductor), track => track.Conductor = null),
        (nameof(Group), track => track.Group = null),
        (nameof(SortTitle), track => track.SortTitle = null),
        (nameof(SortAlbum), track => track.SortAlbum = null),
        (nameof(SortArtist), track => track.SortArtist = null),
        (nameof(SortAlbumArtist), track => track.SortAlbumArtist = null),
        (nameof(SortComposer), track => track.SortComposer = null),
        (nameof(LongDescription), track => track.LongDescription = null),
        (nameof(EncodingTool), track => track.EncodingTool = null),
        (nameof(ChaptersTableDescription), track => track.ChaptersTableDescription = null),
        (nameof(Narrator), track => track.Narrator = null),
        (nameof(MovementName), track => track.MovementName = null),
        (nameof(Movement), track => track.Movement = null),
        (nameof(PublishingDate), track => track.PublishingDate = null),
        (nameof(RecordingDate), track => track.RecordingDate = null),
        (nameof(PurchaseDate), track => track.PurchaseDate = null),
        (nameof(Lyrics), track => track.Lyrics = null),
        (nameof(Chapters), track => track.Chapters.Clear()),
        (nameof(EmbeddedPictures), track => track.EmbeddedPictures.Clear()),
    };

    private static Dictionary<string, (string ID3v23, string ID3v24, string MP4, string Matroska, string
        ASF_WindowsMedia, string RiffInfo)> TagMapping { get; } = new()
    {
        // ("ID3v2.3","ID3v2.4","MP4","Matroska","ASF/Windows Media","RIFF INFO")
        { nameof(Bpm), ("TBPM", "TBPM", "tmpo", "T=30", "WM/BeatsPerMinute", "") },
        { nameof(EncodedBy), ("TENC", "TENC", "", "T=30 ENCODED_BY", "WM/EncodedBy", "") },
        { nameof(EncoderSettings), ("TSSE", "TSSE", "©enc", "T=30", "WM/EncodingSettings", "") },
        { nameof(EncodingTool), ("", "", "©too", "", "WM/ToolName", "") },
        { nameof(Group), ("TIT1", "TIT1", "©grp", "T=30", "WM/ArtistSortOrder", "") },
        { nameof(ItunesCompilation), ("TCMP", "TCMP", "cpil", "T=30", "", "") },
        { nameof(ItunesMediaType), ("", "", "stik", "", "", "") },
        { nameof(ItunesPlayGap), ("", "", "pgap", "", "", "") },
        { nameof(LongDescription), ("TDES", "TDES", "ldes", "T=30", "", "") },
        { nameof(Movement), ("MVIN", "MVIN", "©mvi", "T=20 PART_NUMBER", "", "") },
        { nameof(MovementName), ("MVNM", "MVNM", "©mvn", "T=20 TITLE", "", "") },
        // {nameof(MovementTotal), ("MVIN","MVIN","©mvc","T=30","","")}, // special case: MVIN has to be appended, not replaced
        { nameof(Narrator), ("", "", "©nrt", "T=30", "", "") },
        { nameof(PurchaseDate), ("", "", "purd", "", "", "") },
        { nameof(SortAlbum), ("TSOA", "TSOA", "soal", "T=50 SORT_WITH", "WM/AlbumSortOrder", "") },
        { nameof(SortAlbumArtist), ("TSO2", "TSO2", "soaa", "T=30", "", "") },
        { nameof(SortArtist), ("TSOP", "TSOP", "soar", "T=30", "WM/ArtistSortOrder", "") },
        { nameof(SortComposer), ("TSOC", "TSOC", "soco", "T=30", "", "") },
        { nameof(SortTitle), ("TSOT", "TSOT", "sonm", "T=30 SORT_WITH", "WM/TitleSortOrder", "") },
        /*mp4 => ©st3 ? */
        { nameof(Subtitle), ("TIT3", "TIT3", "----:com.apple.iTunes:SUBTITLE", "T=30", "WM/SubTitle", "") },
    };

    // todo: for fields that are combined multiple values (e.g. MVIN)
    // idea: store internal name as key and original value (e.g. nameof(Group))
    // private Dictionary<string, string> TagMappingOriginalValues = new();
    // then build mapper Func build value for write and parser Func to parse existing value to internal fields
    // MVIN, () => { return $"{Movement}/{MovementTotal}"}, (additionalFieldValue) => {additionalFieldValue.Split("/")...}

    public int? Bpm
    {
        get => GetAdditionalField(IntField);
        set => SetAdditionalField(value);
    }

    public string? EncodedBy
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? EncoderSettings
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? EncodingTool
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? Group
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public ItunesCompilation? ItunesCompilation
    {
        get => GetAdditionalField(EnumField<ItunesCompilation?>);
        set => SetAdditionalField(value);
    }

    public ItunesMediaType? ItunesMediaType
    {
        get => GetAdditionalField(EnumField<ItunesMediaType?>);
        set => SetAdditionalField(value);
    }

    public ItunesPlayGap? ItunesPlayGap
    {
        get => GetAdditionalField(EnumField<ItunesPlayGap?>);
        set => SetAdditionalField(value);
    }

    public string? LongDescription
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? Movement
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? MovementName
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? Narrator
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public DateTime? PurchaseDate
    {
        get => GetAdditionalField(DateTimeField);
        set => SetAdditionalField(value);
    }

    public string? SortAlbum
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? SortAlbumArtist
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? SortArtist
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? SortComposer
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }

    public string? SortTitle
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }


    public string? Subtitle
    {
        get => GetAdditionalField(StringField);
        set => SetAdditionalField(value);
    }


    public MetadataTrack()
    {
        // https://pastebin.com/DQTZFE6H

        // json spec for tag field mapping
        // Fields => (preferredField,writeTemplate,parseCallback), (alternate1, template, parseCallback), (alternate2,template)

        // possible improvements for the future (unclear/unspecified mapping):
        // ACOUSTID_ID
        // ACOUSTID_FINGERPRINT
        // BARCODE
        // CATALOGNUMBER
        // INITIALKEY
        // INVOLVEDPEOPLE
        // MUSICBRAINZ_*
        // PODCAST*

        // Remove is indeed to remove an entire tag type (e.g. ID3v2 or APE) from a file
        // If you want to remove a field, just assign an empty value "" to it and save
    }

    public MetadataTrack(string path, IProgress<float>? writeProgress = null, bool load = true)
        : base(path, writeProgress, load)
    {
    }

    public MetadataTrack(IFileSystemInfo fileInfo, IProgress<float>? writeProgress = null, bool load = true)
        : base(fileInfo.FullName, writeProgress, load)
    {
    }


    private static string? StringField(string? value) => value;
    private static int? IntField(string? value) => int.TryParse(value, out var result) ? result : null;

    private static DateTime? DateTimeField(string? value) =>
        DateTime.TryParse(value, out var result) ? result : null;

    private static T? EnumField<T>(string? value) =>
        Enum.TryParse(typeof(T), value, out var result) && result != null ? (T)result : default;

    private T? GetAdditionalField<T>(Func<string, T?> converter, [CallerMemberName] string key = "")
    {
        var mappedKey = MapAdditionalField(AudioFormat, key);
        if (mappedKey == "" || !AdditionalFields.ContainsKey(mappedKey) || AdditionalFields[mappedKey] == null)
        {
            return default;
        }

        return converter(AdditionalFields[mappedKey]);
    }

    private void SetAdditionalField<T>(T? value, [CallerMemberName] string key = "")
    {
        var mappedKey = MapAdditionalField(AudioFormat, key);
        if (mappedKey == "")
        {
            return;
        }

        if (value == null)
        {
            if (AdditionalFields.ContainsKey(mappedKey))
            {
                AdditionalFields.Remove(mappedKey);
            }

            return;
        }

        /*
For ID3v2.3 and older, you'd combine the TYER tag (YYYY) with the TDAT tag (MMDD) and TIME tag (HHMM).
For ID3v2.4, you'd use TDRC or TDRA (or any of the other timestamp frames), with any level of accuracy you want, up to: YYYYMMDDTHHMMSS. Include Year, throw in month, throw in day, add the literal T and throw in hour, minute, second.
Vorbis: ISO8601 
         */
        // var formatString = AudioFormat.ID switch
        // {
        //     AudioDataIOFactory.CID_MP3
        // };
        AdditionalFields[mappedKey] = value switch
        {
            DateTime d => d.ToString("yyyy-MM-dd"),
            _ => value.ToString() ?? ""
        };

        // todo store / remove original value unmapped for 
        // TagMappingValues[key] = AdditionalFields[mappedKey];
    }


    private static string MapAdditionalField(Format format, string key) => format.ID switch
    {
        // Format is not really appropriate, because it should be the TaggingFormat (id3v2, etc.), not the audio file format
        // TODO: Map others
        AudioDataIOFactory.CID_MP3 => TagMapping.ContainsKey(key) ? TagMapping[key].ID3v23 : "",
        AudioDataIOFactory.CID_MP4 => TagMapping.ContainsKey(key) ? TagMapping[key].MP4 : "",
        _ => ""
    };

    public string[] RemoveFields(params string[] fieldNames)
    {
        var lcFieldNames = fieldNames.Select(f => f.ToLowerInvariant()).ToArray();
        var removedFieldNames = new Stack<string>();

        foreach (var (fieldName, callback) in RemoveFieldCallbacks)
        {
            if (!lcFieldNames.Contains(fieldName.ToLowerInvariant()))
            {
                continue;
            }

            callback(this);
            removedFieldNames.Push(fieldName);
        }

        return removedFieldNames.ToArray();
    }
}