Zeugma440 / atldotnet

Fully managed, portable and easy-to-use C# library to read and edit audio data and metadata (tags) from various audio formats, playlists and CUE sheets
MIT License
466 stars 63 forks source link

Library fails to correctly parse date for flac files with more than one "Year" tag #265

Closed H0tCh0colat3 closed 6 months ago

H0tCh0colat3 commented 6 months ago

The problem

I have several flac files with more than one "Year" tag. One with the actual year, and one with a date as follows:

When loading a track with such tags, the library fails to correctly parse the second tag with the full date.

Environment

Details

After a bit of digging, I think I've tracked the problem to the SetMetaField method in VorbisTag.cs.

original

The hasKey method converts the private TagData.Fields to a map and potentially adds additional fields based on RECORDING_DATE_OR_YEAR before checking for the existence of the key. Meanwhile, the indexer simply returns the value from TagData.Fields directly. This results in a situation where tagData.hasKey(Field.RECORDING_DATE_OR_YEAR) returns true, but actually accessing the value returns null. In this case, due to the internal value separator, targetData is set to ˵2021-10-10 instead of 2021-10-10. In later code when the date value is set, the value ˵2021-10-10 fails to parse and defaults to a year only.

Potential solution

After some fiddling, it seems that ensuring the value returned from the dictionary is non null before performing a concatenation solves the issue. With the following code, the date is correctly parsed from the file I tested on.

if (tagData.hasKey(supportedMetaID))
{
    string existingValue = tagData[supportedMetaID];
    if (existingValue != null)
    {
        targetData = existingValue + Settings.InternalValueSeparator + data;
    }
}

However, I'm not familiar enough with the library to say how this change would potentially affect and/or break parsing of other meta data fields. I also don't know if the same issue affects other file types. My first instinct is to suggest changing the hasKey method and the indexer to be consistent with one another, but I think this would result in values like 2021˵2021-10-10 which would also fail to properly parse into a date. And so I leave it as a suggestion for further testing instead of a PR.

Code To Reproduce Issue

var track = new TagTrack("the file.flac");
var date = track.Date; //incorrectly returns January 1st of whatever year was in the file, regardless of actual month and day

Tested on the following flac file:

test.zip

(I truncated this file with ffmpeg to make a smaller file, though I did something wrong and it did not truncate the length, resulting in incorrect values for things like bitrate. However, it still reproduces the date issue. I can provide the original flac file if needed)

Zeugma440 commented 6 months ago

Thanks for your feedback and for taking time to write a suggestion.

The core issue was that the library didn't expect the Date/Year field to have multiple values, which can indeed happen in the case of Vorbis tags (as we can see below).

image

That case is now properly handled at read-time. However, at write-time, metadata is updated not with both values as in the original file, but using the richest value, that is 2021-10-15.

I personally don't see that as an issue, as no data has been lost; but I'd like to have your take on this.

Is it acceptable from where you stand?

H0tCh0colat3 commented 6 months ago

using the richest value, that is 2021-10-15.

For files which originally had a full date value in addition to a year, this is okay.

In the case the original file did not have a full date, but only a year, does writing back to the file cause the value to end up being a full date like 2021-01-01 (where the month and day default to 1), or will it still only write a year in this case? I would prefer not to write additional info which is untrue, if possible.

Zeugma440 commented 6 months ago

Yeah, I was referring to the case where the initial file has both a single year and a full date value, like the sample file you provided.

When the file only contains a single year, it will be necessarily updated by keeping the single year format. There's a specific unit test to ensure that.

The fix will be available in next release. I'll keep you updated.

Zeugma440 commented 6 months ago

Fix is available in today's v5.24. Please close the issue if it works on your side.