search

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
440 stars 60 forks source link

The BPM field won't be extracted if the value contains a BOM #241

Closed seankearney closed 6 months ago

seankearney commented 6 months ago

The problem

If the value in the BPM field contains a BOM in the string value it will fail to parse to an int. This sets the BPM to 0. Moreover, the BPM field is removed from Additional Fields and we can't extract it ourselves.

Environment

Details

We are actively working on updating from version 4.33 to version 5.13. However we are seeing some issues with the way BPM values are handled since #209. In our experience, we can see a Byte order mark in these BPM values. Currently BPMs will fail to extract a BPM if the value has a BOM. Additionally, since the field is removed from the "Additional Fields" collection I don't see a way that we can extract it ourselves.

The commit: https://github.com/Zeugma440/atldotnet/commit/f74132c5ca35ff8c22646f5f39c7d8dc55854201

I have a 27 MB AIF file that exhibits the issue; I can't attach here.

I was able to hack a fix in the MetaDataIO class. https://github.com/seankearney/atldotnet/commit/d407db729ca03bf47f9988a61798d805283350d7?diff=split&w=0. I suspect this is not the ideal solution, but I'm willing to adjust and send a PR.

Zeugma440 commented 6 months ago

First of all, thanks for your feedback :)

It's much easier to diagnose and solve these kind of issues with the file itself. Could you please send a link to a 3rd party host (e.g. mega, wetransfer...) to [my github username]@hotmail . com ?

Zeugma440 commented 6 months ago

Got it, thanks. It has been fixed for next release.

The problem was ATL didn't expect BOMs into ID3v2.2 frames, as the specs don't explicitly mention them (as opposed to ID3v2.3+ where BOMs are part of the spec). As a consequence, the BOM was read into the value, which prevented its conversion to integer.

NB : That issue was already there on v4.35 and before, but was invisible as the BPM appeared as a text field, not a numeric field

Zeugma440 commented 6 months ago

The fix is available in today's v5.14~

Please close the issue if you confirm the fix.

seankearney commented 6 months ago

Looks great! Closing. Thank you!!!