Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic chapter representation #249

Open
Serial-ATA opened this issue Aug 21, 2023 · 5 comments
Open

Generic chapter representation #249

Serial-ATA opened this issue Aug 21, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@Serial-ATA
Copy link
Owner

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.

@Serial-ATA Serial-ATA added the enhancement New feature or request label Aug 21, 2023
@sandreas
Copy link

sandreas commented Nov 6, 2023

@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:

  • Instead of end_position you could store length, which should be a smaller number, more intuitive (having the length makes it easier to find length issues within dumps) and you're still able calculate the end_position
  • 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
  • Calculating the EXACT track duration in a generic way can be tricky - ffmpeg for example is not reliable on this!
  • There are two chapter formats in MP4 (Nero and Quicktime, see the atldotnet writeup)
  • 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

@Serial-ATA
Copy link
Owner Author

Serial-ATA commented Nov 7, 2023

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 enzo1982/mp4v2#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
Copy link

sandreas commented Nov 8, 2023

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
Copy link
Owner Author

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
Copy link

sandreas commented Nov 9, 2023

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 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants