Some Required Header Information Shown as Additional Fields

[nlogozzo]

flac/opus/vorbis files contain a required "VORBIS-VENDOR" field in the header as required by the structure of the tag: https://xiph.org/vorbis/doc/v-comment.html

wav files similarly contain an "info.ISFT" as part of the RIFF spec: https://exiftool.org/TagNames/RIFF.html

These fields become a part of Track.AdditionalFields however they are unable to be removed as upon doing:

var track = new Track(Path); //flac file
track.AdditionalFields.Remove("VORBIS-VENDOR");
track.Save();

seems to remove the field but upon reloading the file (track = new Track(Path)), AdditionalFields contains the "VORBIS-VENDOR" field still, as again it's required by the tag so it's expected.

My proposal is to exclude "VORBIS-VENDOR" and "info.ISFT" from AdditionalFields as 1) they are required and 2) require specific encoding formats that are usually set when the file is first created and are not expected to change.

Correct me if I'm wrong about anything.

[Zeugma440]

I agree about VORBIS-VENDOR.

However, I don't see how or why info.ISFT would be mandatory or unmovable. Did you confirm that using ATL?

[nlogozzo]

However, I don't see how or why info.ISFT would be mandatory or unmovable. Did you confirm that using ATL?

Yes same code as with VORBIS-VENDOR, just with loading a WAV file instead and obviously removing info.ISFT instead.

info.ISFT field contains Lavf60.3.100 (codec information) just like VORBIS-VENDOR does and therefore shouldn't be considered an Additional Field but mandatory instead.

[nlogozzo]

Could instead of hiding them from AdditionalFields, we have a ReadOnlyAdditionalFields list that would contains these properties? That why they will still be available for us to show to user but ensure that they aren't altered.

[nlogozzo]

Hi, any update on this?

[Zeugma440]

Sorry, I've been working on my other project all week long. I'm gonna take a look ASAP 😅

[nlogozzo]

Sorry, I've been working on my other project all week long. I'm gonna take a look ASAP 😅

No worries, I've been swamped with midterms and school work this week so I understand 😆

[Zeugma440]

Here are my first thoughts on the subject :

• I understand the link you've made between VORBIS-VENDOR and info.ISFT as they are similar in the information they carry (referring to properties of the physical audio data). However, there's a structural difference between them : the VORBIS-VENDOR field is built in the Vorbis format and is completely mandatory, whereas info.ISFT is an optional field that is absent from many WAV files

That also means track.AdditionalFields.Remove("info.ISFT") should work perfectly 😉

• I think it wouldn't be wise to remove this information from Track.AdditionalFields as it is as useful as any other metadata

• Marking specific metadata as read only or undeletable would be the correct way to go theoretically, but embedding that information would require to change the type of AdditionalFields from its current IDictionary<string, string> to something more elaborate... and less intuitive to use. As we're talking about just one metadata, I'm not convinced the case is big enough to change this widely-used data structure.

Maybe transmitting a second IDictionary<string, MetadataProperties> would do the trick?

[nlogozzo]

That also means track.AdditionalFields.Remove("info.ISFT") should work perfectly 😉

I'm going to do some more testing and provide you with the file that it wasn't working on if I can still reproduce it...

from its current IDictionary<string, string> to something more elaborate... and less intuitive to use. As we're talking about just one metadata, I'm not convinced the case is big enough to change this widely-used data structure.

maybe IDictionary<string, (string Value, bool ReadOnly)> as a tuple.

Maybe transmitting a second IDictionary<string, MetadataProperties> would do the trick?

I feel like having a second Dictionary here with a same information is a waste of space.

Maybe a IReadOnlyList<(string Title, string Value)> AdditionalReadOnlyFields list with just the read only properties to avoid having to modify the original AdditionalFields list?

I feel like making the original AdditionalFields list a IDictionary<string, (string Value, bool ReadOnly)> is the most intuitive solution. Yes it breaks API but it's still just one list/dictionary instead of requiring extra memory space.

[Zeugma440]

Breaking an important part of the API and making it harder to use for just one single metadata still feels overkill to me. I'm thinking of all the other apps that use the library. Such a change would be far from neutral to them... plus you can't be serious about extra memory, we're talking bytes 😅

I'm hesitating between :

A/ Values in IDictionary<string, string> AdditionalFields; properties in IDictionary<string, MetadataProperties> AdditionalFieldsProperties

• Pros : Best retrocompatibility + can enrich MetadataProperties when we need to document more properties than "read only"
• Cons : Weird design : having to synch two data structures to get a piece of information has never been a good sign

B/ R/W values in IDictionary<string, string> AdditionalFields; Readonly values in IDictionary<string, string> AdditionalReadOnlyFields

• Pros : Crystal clear purpose for each Dictionary + allows to easily ignore read-only fields at write-time
• Cons : Silent retrocompatibilty issue; minor code adaptation is required to get all additional fields again + we're stuck if we want to add more properties to our metadata

My personal preference would go to B anyway. How about you?

[nlogozzo]

My personal preference would go to B anyway. How about you?

I agree that B is the best solution to go with.

[nlogozzo]

That also means track.AdditionalFields.Remove("info.ISFT") should work perfectly 😉

I'm going to do some more testing and provide you with the file that it wasn't working on if I can still reproduce it...

Here's the WAV file that contains info.ISFT and in which the code does not work: https://go.wetransfer.com/t-EvR5H2NAYQ

Again the code used is:

var track = new Track(Path); //wav file
track.AdditionalFields.Remove("info.ISFT");
track.Save();
track = new Track(Path) //reload
var x = track.AdditionalFields.Contains("info.ISFT"); //x is true

[nlogozzo]

Readonly values in IDictionary<string, string> AdditionalReadOnlyFields

Also I'd make the type IReadOnlyDictionary

[Zeugma440]

The WAV thing is a coincidence - you actually discovered a bug where removing an AdditionalField has no effect on some WAV chunks. The fix will ship with next release ✅

I still have to implement "solution B". Will do that during the weekend.

[Zeugma440]

I just realized that, even though "VORBIS-VENDOR" is technically mandatory (in the sense that the field has to be in the file structure), the Vorbis spec doesn't forbid :

• to change its initial value
• to set it to "" (i.e. empty string), which would count as a functional delete

Right now, the latter doesn't work by calling track.AdditionalFields.Remove("VORBIS-VENDOR") but I can fix this easily. Wouldn't that fix the whole issue?

[nlogozzo]

Right now, the latter doesn't work by calling track.AdditionalFields.Remove("VORBIS-VENDOR") but I can fix this easily. Wouldn't that fix the whole issue?

Yes I get that would work too.

[Zeugma440]

Done! Available on today's v5.12 👍

[nlogozzo]

Yay! Thanks :)

[Zeugma440]

you're welcome. Sorry for the two-weeks lag 😋

[nlogozzo]

you're welcome. Sorry for the two-weeks lag 😋

No worries...I've been lagging with all my school work too so no rush 😅

[nlogozzo]

Can confirm it's fixed. Thanks again!