Failed to read file!: NotAPicture

dannyglover commented 11 months ago

Reproducer

I ran the example command from the documentation in the readme.

RUST_BACKTRACE=1 cargo run --example tag_reader "/home/user/Downloads/11 - Howling Halls.flac"
<code>

Summary

Lofty fails to read metadata when there is an issue with the embedded picture contained within the audio files metadata. VLC and MPV are both able to play the file in question, and display the embedded image (front cover).

Here is the trace:

[user@pc lofty-rs-0.15.0]$ RUST_BACKTRACE=1 cargo run --example tag_reader "/home/user/Downloads/11 - Howling Halls.flac"
    Finished dev [unoptimized + debuginfo] target(s) in 0.02s
     Running `target/debug/examples/tag_reader '/home/user/Downloads/11 - Howling Halls.flac'`
thread 'main' panicked at 'ERROR: Failed to read file!: NotAPicture', examples/tag_reader.rs:15:10
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/panicking.rs:67:14
   2: core::result::unwrap_failed
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/result.rs:1651:5
   3: core::result::Result<T,E>::expect
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/result.rs:1033:23
   4: tag_reader::main
             at ./examples/tag_reader.rs:12:20
   5: core::ops::function::FnOnce::call_once
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/ops/function.rs:250:5

Expected behavior

Lofty should (ideally) be able to read the image data from the metadata. It also really shouldn't prevent retrieval of the rest of the metadata (song title, album etc) if it fails to read an image from said metadata. It would be preferable if lofty would return all valid metadata, and if it is unable to read certain metadata fields, it should just return null/an empty value for the metadata field in question.

I can't predict if users audio files will contain metadata (mainly images) that are not compatible with lofty, so being able to at least retrieve the metadata lofty can read, would allow me to use a placeholder for any data it can't read, so I can alert the user to problematic metadata, without having to forsake the entirety of the metadata due to one problematic field.

Thank you.

Assets

You will need this audio file to reproduce: audio file link

This is the image that is embedded in the metadata: Image link

Serial-ATA commented 11 months ago

Thanks for the report!

So this file actually had two issues:

It had a COVERART field in the Vorbis Comments, which I never got around to supporting since I've never seen it in the wild. So that's finally been addressed.
It had an invalid picture block with a picture type of 0xFFFFFFFF. This is again something I've never seen to indicate an invalid picture type. We now support any picture type by default, with the original error still being around for ParsingMode::Strict.

so I can alert the user to problematic metadata, without having to forsake the entirety of the metadata due to one problematic field.

This has been a goal for awhile. Lofty was originally made with the specifications in mind, but I quickly found out that many applications simply do not write metadata correctly at all. That's why ParsingMode was introduced. It's not used everywhere yet, as it was introduced (relatively) recently, which is why these types of errors pop up from time to time.

dannyglover commented 11 months ago

Thanks for the report!

So this file actually had two issues:
* It had a COVERART field in the Vorbis Comments, which I never got around to supporting since I've never seen it in the wild. So that's finally been addressed.

* It had an invalid picture block with a picture type of `0xFFFFFFFF`. This is again something I've never seen to indicate an invalid picture type. We now support any picture type by default, with the original error still being around for `ParsingMode::Strict`.
so I can alert the user to problematic metadata, without having to forsake the entirety of the metadata due to one problematic field.

This has been a goal for awhile. Lofty was originally made with the specifications in mind, but I quickly found out that many applications simply do not write metadata correctly at all. That's why ParsingMode was introduced. It's not used everywhere yet, as it was introduced (relatively) recently, which is why these types of errors pop up from time to time.

Thank you so much! You work fast :)

So I guess I can pull the latest changes and test again? Cheers!

Serial-ATA commented 11 months ago

Could you try out #254 to see if it works for you? Thanks!

dannyglover commented 11 months ago

Could you try out #254 to see if it works for you? Thanks!

I get the following data returned now, so it looks good.

--- Tag Information ---
Title: Howling Halls
Artist: Russell Shaw
Album: Fable II
Genre: Soundtrack
Album Artist: 
--- Audio Properties ---
Bitrate (Audio): 615
Bitrate (Overall): 621
Sample Rate: 44100
Bit depth: 16
Channels: 2
Duration: 04:29

Thank you for the rapid response on this. It's most appreciated.

Serial-ATA commented 11 months ago

Awesome! I'll get 0.16.0 out tomorrow.

Serial-ATA / lofty-rs