music-metadata icon indicating copy to clipboard operation
music-metadata copied to clipboard

Add chapter.start & chapter.timescale

Open jigzahoy opened this issue 4 years ago • 7 comments

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.

jigzahoy avatar Jan 14 '21 11:01 jigzahoy

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 avatar Jan 14 '21 21:01 Borewit

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

jigzahoy avatar Jan 15 '21 07:01 jigzahoy

Coverage Status

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

coveralls avatar Jan 15 '21 21:01 coveralls

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 avatar Jan 15 '21 21:01 Borewit

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

Can please convert the issues to actual issues @jigzahoy?

Borewit avatar Jan 18 '21 20:01 Borewit

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 avatar Jan 19 '21 11:01 jigzahoy

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 avatar Jan 19 '21 19:01 Borewit