Serial-ATA / lofty-rs

Audio metadata library
Apache License 2.0
193 stars 36 forks source link

Generic chapter representation #249

Open Serial-ATA opened 1 year ago

Serial-ATA commented 1 year ago

Similar to Picture, once chapters are supported in ID3v2 (#189), EBML, Vorbis Comments, and MP4, it may be worth creating a generic representation of chapters that can be converted to and from the concrete implementations.

Not yet sure if this would be easy (or possible) to accomplish, since there's a pretty big difference in available information between the formats.

The basic idea would be:

pub struct Chapter {
    pub name: String,
    pub start_time: u32,
    pub end_time: u32,
}

This seems to cover the bare minimum required information for chapters in all formats (It could be expanded with additional optional information).

However, with MP4 (at least in one of many chapter formats available) and Vorbis Comments, there is only a name and start time available. end_time may need to be optional, making the conversion between Chapter and the ID3v2/EBML chapters fallible.

sandreas commented 1 year ago

@Serial-ATA Hey thank you for creating this library. I would love to see generic chapter support and there is an implementation of a similar approach in atldotnet, which I use for my little side project tone.

Here is a good writeup about chapters from the atldotnet developer: https://github.com/Zeugma440/atldotnet/wiki/Focus-on-Chapter-metadata

And a rust ID3 chapters implementation: https://docs.rs/chapters/0.4.2/chapters/fn.from_mp3_file.html

I had the following thoughts:

Serial-ATA commented 1 year ago

Hello @sandreas!

Just letting you know, this is pretty far out. Currently Lofty does not support chapters in any format (they'll just be exposed as binary items). Support for the individual chapter formats (ID3v2, EBML, Vorbis Comments, and MP4) will have to come before the generic implementation.

If there is no length (like in Vorbis or MP4), you could use a whole ChapterSet type including the track duration to recalculate the lengths - especially of the last chapter

So we could read the chapters, and as new ones come into the list, we update the previous chapter's end time with the current one's start time. But...

Calculating the EXACT track duration in a generic way https://github.com/enzo1982/mp4v2/issues/18 - ffmpeg for example is not reliable on this!

What would be done in the case of read_properties(false)? Since Lofty lets you skip reading of the audio properties, we can't rely on having valid length information present.

The best solution I can think of would be to make the end time Option<u32>, which is unfortunate since every chapter except the last one would have an end time.

I'm no rust expert, but for C# / atldotnet I would have loved a Chapter implementation with TimeSpan for start_time and length instead of a simple uint - it would have made things much easier and less low level in most of my use cases - maybe this could also prevent the _time suffix, if there is a TimeSpan type or similar in rust

Do you mean have start_time and a length field of type std::time::Duration?

sandreas commented 1 year ago

Thank you for this detailed response.

Support for the individual chapter formats (ID3v2, EBML, Vorbis Comments, and MP4) will have to come before the generic implementation.

Absolutely. If I could help besides providing resources and links specs, I would. Unfortunately I hardly have any experience in Rust, so my code would probably not have the quality to be ever released :-)

What would be done in the case of read_properties(false)? Since Lofty lets you skip reading of the audio properties, we can't rely on having valid length information present.

I would consider the total duration as optional but helpful if present. If it is not present, the last chapter would not have a length if it cannot be determined otherwise. You could go 0 to represent that the real length is not available, instead of making the whole thing Option<u32>. A zero length chapter makes no sense in most cases, on most apps, zero length chapters cannot be skipped previous, so they should be prohibited anyway.

Do you mean have start_time and a length field of type std::time::Duration?

That could be a good decision, but as I said, I'm no rust professional. I leave this up to the experts (e.g. you) - but I would like the idea, that the struct also has kind of a real type + unit / precision. Otherwise it could be milliseconds or nanoseconds. The _time suffix would be obsolete and even a suffix like _ms is no longer needed, if Duration is used as type... Of course Duration may have lower performance, but I cannot estimate if it is worth.

Serial-ATA commented 1 year ago

You could go 0 to represent that the real length is not available, instead of making the whole thing Option.

Yeah, that's a better solution. We could simply error if someone tries to write a zero length chapter (if it's a format that even needs an end time).

Of course Duration may have lower performance

There's no performance penalty for using Duration. It's just a wrapper type for two integers representing seconds and nanoseconds. It'll allow you to get the chapter length as_secs, as_millis, and as_nanos, so using it instead of end_time does seem like a good idea. 🙂

Thanks for your input on the issue. Having never used chapters myself (hence the current lack of support), its good to know how they could be represented in a way that is actually useful.

sandreas commented 1 year ago

If you would like to experiment with chapters and are looking for testfiles, atldotnet has some.

And tone as an atldotnet command line implementation may be a useful tool for your experiments with different formats. Be sure to use 0.1.5, the readme is a bit outdated 😊