Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
H0tCh0colat3 opened this issue May 15, 2024 · 4 comments
Assignees

Comments

@H0tCh0colat3
Copy link

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:

  • YEAR: 2021
  • YEAR: 2021-10-10

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

Environment

  • Tested with source code from commit 7c73e60, and with nuget package version 5.23
  • Windows 10.0.19045

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

Zeugma440 commented May 19, 2024

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants