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.

Sign up for GitHub

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

Jump to bottom

Add chapter.start & chapter.timescale #731

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

Add chapter.start & chapter.timescale #731

wants to merge 4 commits into 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
@Borewit Borewit added the enhancement label Jan 14, 2021
@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.

@Borewit Borewit force-pushed the m4b-chapter branch from c4eafbf to 2a8ae57 Compare January 15, 2021 21:28
@Borewit Borewit force-pushed the master branch from bf88e9d to d47097b Compare January 15, 2021 21:34
@Borewit Borewit force-pushed the m4b-chapter branch from 2a8ae57 to 305a4a3 Compare January 15, 2021 21:36
@coveralls
Copy link

coveralls commented Jan 15, 2021 •
edited
Loading

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 •
edited
Loading

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

Can please convert the issues to actual issues @jigzahoy?

Borewit
Borewit reviewed Jan 19, 2021
View reviewed changes
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
Borewit
Borewit reviewed Jan 19, 2021
View reviewed changes
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?

@jigzahoy jigzahoy mentioned this pull request Jan 19, 2021
Open
@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 5 times, most recently from 707e04f to cfa8a81 Compare July 11, 2021 06:16
@Borewit Borewit force-pushed the master branch 3 times, most recently from 36fde3e to 97a24a3 Compare January 27, 2022 14:35
@Borewit Borewit force-pushed the master branch from 3232785 to 94421a0 Compare August 3, 2022 10:50
@Borewit Borewit force-pushed the master branch from 1be603e to 13111d4 Compare January 5, 2023 11:16
@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
Reviewers

@Borewit Borewit Borewit left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jigzahoy @Borewit @coveralls
  NODES
COMMUNITY 2
Project 5
USERS 1