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

Add chapter.start & chapter.timescale #731

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jigzahoy
Copy link

Hello, I added the starting time and timescale values of chapters from audiobook file .m4b.
Unit test updated and matches values using ffprobe.

Issues Found

  • .m4b with isom/iso2/mp41 container only returns one chapter but all chapter durations are present.

    • In the parseChapterTrack function, chapterTrack.chunkOffsetTable only contains only one value, and chapterTrack.sampleSizeTable is empty. Which I believe they're needed to extract the chapter titles.
  • .m4b with M4A/isom/iso2, it doesn't return any chapter lists.

I can provide the sample files I used if there's a way I can send them privately as they are copyrighted material.

Reading the buffer/token(?) values is not my currently strong suit but I hope this PR helps.

@Borewit
Copy link
Owner

Borewit commented Jan 14, 2021

Thanks for your contribution!
You can send files to GitHub user @xs4all.nl.

Can you please rebase? I hope that triggers the GitHub action integration tests.

@Borewit Borewit self-requested a review January 14, 2021 21:04
@jigzahoy
Copy link
Author

Hello! Feature branch rebased to latest master commit and I also fixed linting issues.
Audio files were sent to the given email.

@coveralls
Copy link

coveralls commented Jan 15, 2021

Coverage Status

Coverage increased (+0.004%) to 91.921% when pulling f382743 on jigzahoy:m4b-chapter into d47097b on Borewit:master.

@Borewit
Copy link
Owner

Borewit commented Jan 15, 2021

I think something went wrong on your side with rebasing (you need to force push after a rebase). I would not have helped because I failed to fix the CI tests.
But now it is fixed, and I rebased the branch.

Now CI tests are running and green, and merge conflicts have been resolved.
Side effect is that the branch on you local dev machine will now be in conflict with origin/m4b-chapter. Not sure how experienced you are with things, but the easiest way to resolve that is:

  1. Switch (checkout) to master branch
  2. Delete m4b-chapter branch (not on GitHub, but only on your local machine)
  3. checkout origin/m4b-chapter

I got the files you send me 🙏, I review your work soon.

@Borewit
Copy link
Owner

Borewit commented Jan 18, 2021

So this PR is about adding starting time and timescale values.

Can please convert the issues to actual issues @jigzahoy?

Copy link
Owner

@Borewit Borewit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are format.chapter[n].sampleOffset / format.sampleRate & format.chapter[n].start / format.timeScale different?

@Borewit Borewit changed the title Update m4b chapter metadata Add chapter.start & chapter.timescale Jan 19, 2021
lib/type.ts Outdated Show resolved Hide resolved
lib/type.ts Outdated
*/
start: number;
/**
* Chapter track timescale
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a bit vague

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type descriptions are updated on latest commit.

@jigzahoy
Copy link
Author

All I had to do was to force push it instead of merging it! Clearly it is my first time to do a PR 😅. Really appreciate the help, thank you very much!

Why are format.chapter[n].sampleOffset / format.sampleRate & format.chapter[n].start / format.timeScale different?

I've notice that format.chapter[n].sampleOffset and format.chapter[n].start are relatively close and they have small difference between them. I assumed every first chapters starts at 00:00 and using the files I tested, format.chapter[0].sampleOffset is a non-zero value (may be small and negligible when divided by format.sampleRate). Was format.chapter[n].sampleOffset has the same intended use as format.chapter[n].start?

Some files I've used sometimes gives format.chapter[n].timeScale a different value from format.sampleRate. I only added it just in case. And I am honestly not sure if timeScale would be the proper name for it. ffpmeg calls it time_base and it's the reciprocal of timescale (1/timeScale). I will update the typing descriptions. But what do you think?

@Borewit
Copy link
Owner

Borewit commented Jan 19, 2021

Clearly it is my first time to do a PR 😅.

You are doing just fine. Nice work.

Regarding the timeScale. I suspect there is reason for both. If we make those values part of the music-metadata.common API it should be document what these values represent. Maybe start and timescale should replace sampleOffset, I honestly don't know. If you say that works better, I have no problem doing that. A formula how the timescale relates the start is required, otherwise it is not clear what these values represent.

@Borewit Borewit force-pushed the master branch 3 times, most recently from 0d5910e to be7a11a Compare January 5, 2023 11:30
@Borewit Borewit force-pushed the master branch 4 times, most recently from 3a2947a to 526cc3a Compare July 19, 2024 16:51
@Borewit Borewit force-pushed the master branch 2 times, most recently from 315cc91 to b7b408b Compare August 2, 2024 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants