Serial-ATA / lofty-rs

Audio metadata library
Apache License 2.0
185 stars 34 forks source link

ID3v2: Missing owner in UFID frame #204

Closed jokorone closed 1 year ago

jokorone commented 1 year ago

Hi, firstly thanks for the easy-to-use library, it's been a breeze so far! Symphonia is just too vast for my use case.

I'm trying to read the metadata from a music library with all sorts of file types, I find all working pretty well out of the box.

I encountered this error ERROR: Failed to read file!: Id3v2(ID3v2: MissingUfidOwner) while reading from a .mp3. The file is not currupted, plays well and when reading tags with symphonia I get the following:

|     [01] Codec:           MPEG Audio Layer 3 (mp3)
|          Sample Rate:     44100
|          Duration:        0:06:40.039 (17641728)
|          Time Base:       1/44100
|          Encoder Delay:   1105
|          Encoder Padding: 623
|          Channel(s):      2
|          Channel Map:     0b000000000000000000000000000011

The part which is failing me:

let tagged_file = Probe::open(path)
    .expect("ERROR: Bad path provided!")
    .read()
    .expect("ERROR: Failed to read file!"); // <---- 

When browsing the documentation, I found a piece about registering a custom resolver, but don't really know how to go about it in this case, as I dont have a custom type at hand.

At this point I dont really know what to try next, as I feel like it's a really bad idea to create the TaggedFile myself.

Any hints or things to try? Happy to share the audio file (copyrighted...) via zippy, trying first without.

PS: I saw a call for more documentation, I'm happy to create digest of whatever comes out of this for future reference.

Serial-ATA commented 1 year ago

Hello!

That error comes from your file having an incomplete UFID frame, which according to the spec should have an owner identifier.

Looking at Symphonia, it looks like the UFID frame is ignored entirely: https://github.com/pdeljanov/Symphonia/blob/c6a14a0405281af555edca98fa56f3c51a46d459/symphonia-metadata/src/id3v2/frames.rs#L338.

Since Lofty is purely a metadata crate, it tries to stick a lot closer to the spec.

Is this a common issue in your library? It may be worth it to add an exception for this with ParsingMode::Relaxed.

jokorone commented 1 year ago

Thanks for the blazing quick response!

I wasn't aware of the spec, thanks for the ref. Metadata is such a complex beast!

The library is composed of all filetypes of different origins, but I couldn't reproduce exactly this error with no other file I have.

While looking at the docs for ParsingMode::Relaxed I didn't get a clear grasp on where and how exactly to implement it. Could you provide me with an example use?

Thanks 🙏

Serial-ATA commented 1 year ago

Yeah, that's definitely a weak spot in the docs. :)

You use it in ParseOptions which you can then pass to your Probe.

Like this:

use lofty::{Probe, ParseOptions, ParsingMode};

let options = ParseOptions::new().parsing_mode(ParsingMode::Relaxed);
let tagged_file = Probe::open(path)?.options(options).read()?;

For now ParsingMode::Relaxed will only really affect the output of property readings, but it could be expanded to cover malformed items in otherwise valid tags.

jokorone commented 1 year ago

I added ParsingMode::Relaxed to my snippet, but the error persists.

For now ParsingMode::Relaxed will only really affect the output of property readings, but it could be expanded to cover malformed items in otherwise valid tags.

I know too little about the inner workings of Strict vs Relaxed when dealing with malformed items. Your suggestion however seems very logical to me.

The reading of the probe to not panic when a key, which is required by the spec but just isn't there, would be a nice addition when working with organically grown libraries of questionable origin 😆

Thanks a lot for the help.

Serial-ATA commented 1 year ago

I know too little about the inner workings of Strict vs Relaxed when dealing with malformed items.

Yeah, no one really knows unless they read the code. I really need to get to giving ParsingMode some actual documentation. :)

Lofty is a lot stricter than other tag reading libraries by default. I'll see about expanding ParsingMode::Relaxed to cover malformed tag items. It's unfortunate that the entire file is regarded as invalid due to a single frame.

For now, you should either fix or remove the UFID frame in a tag editor.

uklotzde commented 1 year ago

https://en.wikipedia.org/wiki/Robustness_principle

Probably we need 3 modes of operation:

Serial-ATA commented 1 year ago

In 0.14.0 the default parsing mode will be the new ParsingMode::BestAttempt, which will have no problem reading your UFID frame without an owner. :)