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 ability for chapters [feature request] #3

Closed
kour1er opened this issue Jan 13, 2022 · 24 comments
Closed

Add ability for chapters [feature request] #3

kour1er opened this issue Jan 13, 2022 · 24 comments
Assignees
Labels
enhancement New feature or request

Comments

@kour1er
Copy link

kour1er commented Jan 13, 2022

Would it be possible to add the ability to make chapter markers for merging mp3 files when working with audiobooks? For example, if I have a directory of mp3 files that I'm merging, to make a new chapter marker for every file in the final outputted single mp3 file?

Thanks for the consideration

@crra
Copy link
Owner

crra commented Jan 15, 2022

Hey @kour1er,

thanks for reaching out. You mean: https://id3.org/id3v2-chapters-1.0?
I'll look into it.

@crra crra added the enhancement New feature or request label Jan 15, 2022
@kour1er
Copy link
Author

kour1er commented Jan 15, 2022

Yes exactly that - thanks very much!

@crra
Copy link
Owner

crra commented Jan 17, 2022

Hello @kour1er,

I've created a pre-release with the support of the chapter markers. Would you be so kind to check if the feature works as expected?

@kour1er
Copy link
Author

kour1er commented Jan 17, 2022

Gosh that was quick work!

So I've tested a couple different sets of mp3 files to merge into a single chapterised mp3 file. When I use the --verbose flag I can see the chapter timestamp being generated but in the final file I'm not seeing any chapter data. As a side issue, if I use the --cover flag, that information doesn't appear to be added either, so I wonder if the additional data just isn't being written to the final file? I checked with the previous release and --cover works there for me but not in the new release.

For testing, I'm using Bound on iOS, ffprobe on the command line with -show_chapters flag and IINA for macOS (a front end for mpv).

Am I missing a setting somewhere? This is with version 4.1.0+pre1

@crra
Copy link
Owner

crra commented Jan 18, 2022

Sorry that it didn't work. Thanks for the ffprobe link for inspecting chapters.

As for the covers, this is very strange. It works on my machine (TM) :) with magic discovery and --cover foo.jpg. Can you share a link to the cover?

with_cover

@kour1er
Copy link
Author

kour1er commented Jan 18, 2022

re: ffprobe/chapters - please let me know if I can provide further information on that.

re: cover - so let's take something popular as an example: how about The Martian. It's a 500x500 pixel image so I don't think that should be outside of spec.

If I try using that jpg on the pre-release the cover isn't added to the mp3 on my machine (doesn't work TM?). But using version 4.0.0 it does. I'm validating that using Meta on macOS. I don't know 'magic discovery' and couldn't find an obvious match from a search, could you link to it and I can test here with that tool too?

Cheers

@crra
Copy link
Owner

crra commented Jan 19, 2022

Oh shoot. kid3 (which I used for testing) shows the meta data, but any other tool (e.g. id3editor) shows nothing. Maybe it's chapter related or due to the update of id3v2 to the latest v2. However, an id3v2 frame was written "ID3":

hex

@kour1er
Copy link
Author

kour1er commented Jan 20, 2022

Interesting - I checked with kid3 on my system and it also shows the correct image in both the old and prerelease versions. But other tools (IINA, VLC, Meta, Finder, Bound etc) don't show the cover art on mp3s generated by the prerelease version. If I inspect the hex of an mp3 created by the prerelease and the current release I see the same ID3 information you found too. So that's weird :)

Also, on the original matter of the chapter embedding, I noticed in kid3, that although it does show embedded chapter information it doesn't appear to have the "Table of Contents" "toc" tag. I think that's a required element? Haven't been able to find a good link for that. Just wonder if that might be it?

@crra
Copy link
Owner

crra commented Jan 20, 2022

Baby steps: merging two mp3 files without id3 tags yield the same result in 4.0.0 and head.

@crra
Copy link
Owner

crra commented Jan 20, 2022

After adding one tag, the position of the ID3 tag in reference to the Xing tag is different. 4.0.0 shows the tack number in the editor, head doesn't.

4.0.0 (left) head (right)

onetag

@crra
Copy link
Owner

crra commented Jan 20, 2022

I've changed the order when the id3 tags is written, because I wanted to parse each file once for the duration:

  • 4.0.0: write metadata, bind
  • head: bind, write metadata

My guess is that the file offset of the output file is not properly set (stateful) and I'm corrupting the file with a random write.

@crra
Copy link
Owner

crra commented Jan 20, 2022

Mhh, the id3 tag is now written at the end, but still not working. Next check is id3v2.4 (UTF-8) vs id3v2.3 (UTF-8). I've also noticed that any other id3 editor rewrites the file so that the id3 tag is always at the beginning. I know that this was a requirement for id3v1.

@crra
Copy link
Owner

crra commented Jan 20, 2022

If I write the ID3 tag at the beginning of the file, any editor recognizes the tag. I always thought it would be better so that music players don't need to parse the whole file. But I discarded it with the chapter change as "premature optimization", but I want to keep the concept of parsing the files only once (IO) and not holding the whole files in memory.

BTW: the underlying library for ID3 follows the idea of putting the ID3 tag at the beginning of the file. "They" also pay the IO price of copying the data into a temp file: https://github.com/bogem/id3v2/blob/main/v2/tag.go#L321

@kour1er
Copy link
Author

kour1er commented Jan 21, 2022

Interesting - the 'root of all evil' strikes again :)

@crra
Copy link
Owner

crra commented Jan 24, 2022

Interesting - the 'root of all evil' strikes again :)

Thanks for the working title of the next pre-release. You may want to give it a try. I'm not sure if I want to keep this specific implementation (using a temporary file to ensure the frame order) but any other solution this is more complicated and more error prone (e.g. own parser for frame offsets and delay the actual read till the end). Maybe a temporary file is not so bad at all.

@crra crra self-assigned this Jan 24, 2022
@kour1er
Copy link
Author

kour1er commented Jan 25, 2022

Congrats on the progress! Love the pre-release name :) Seems like a totally reasonable approach to me. I really love how fast your tool is. Crazy how much faster than some other tools I've used.

I've now run some tests in my environment. The cover art problem is solved 👍

Chapter information is odd. I can see the chapter information correctly added on the final file in IINA (mpv frontend). But the merged file is not showing the chapter information on iOS in Bound or BookPlayer nor on macOS in the Apple books player. Interestingly, on PocketCast (I know it's a podcast player but I know it supports chapters) chapters do appear.

So then I was reading the ID3v2 Chapter Frame Addendum and saw this bit:

The Start offset is a zero-based count of bytes from the beginning of the file to the first byte of the first audio frame in the chapter. If these bytes are all set to 0xFF then the value should be ignored and the start time value should be utilized.
The End offset is a zero-based count of bytes from the beginning of the file to the first byte of the audio frame following the end of the chapter. If these bytes are all set to 0xFF then the value should be ignored and the end time value should be utilized.

Following that, I added by hand in kid3 'FFFFFFFF' to each chapter start offset and end offset. Then I saw this bit in the same spec:

The purpose of "CTOC" frames is to allow a table of contents to be defined. In the simplest case, a single "CTOC" frame can be used to provide a flat (single-level) table of contents. However, multiple "CTOC" frames can also be used to define a hierarchical (multi-level) table of contents.

Then also in kid3 I added a Table of Contents. Following the structure of another mp3 with chapter marks, I changed the chapter Identifier from 'chap-0'... to 'ch0'... and used those as the Items of in the TOC. I also made the TOC 'top level' and 'ordered' enabled in kid3.

Retransmitted the altered file and chapters work everywhere! So it "appears" that:

  • TOC is essential
  • Some apps are picky about the Identifier of the Chapter value
  • And start/end offset is required in some players

This is obviously just experimental - love to know your thoughts

@crra
Copy link
Owner

crra commented Jan 26, 2022

  • TOC is essential (currently not supported by the used library, needs investigation)
  • Some apps are picky about the Identifier of the Chapter value
  • start/end offset is required in some players (should be possible)

@crra
Copy link
Owner

crra commented Jan 29, 2022

@kour1er it seams like you have the wider variety of tools to check the chapter support. Would you be so kind to test it with the new beta? I extended the underlying library because it didn't support chapter TOCs.

@kour1er
Copy link
Author

kour1er commented Jan 29, 2022

Just to give you quick feedback - progress! Chapters are now working in IINA, Books (Apple desktop app), Bound (iOS) and BookPlayer (iOS). Nice work on the library work. 👍

One regression, the artist, title and album (so author and book as it were) don't appear to be written to the final file. I don't see those tags in Kid3 either. This is only from testing one book running it through your tool a few times. Cover is working fine.

@crra
Copy link
Owner

crra commented Jan 29, 2022

Now is the time to add those missing tests :)

@crra
Copy link
Owner

crra commented Jan 30, 2022

One regression, the artist, title and album (so author and book as it were) don't appear to be written to the final file.

Can you post the command line here?

I assume it contains spaces or special chars (,, =,:) and then the string must be quoted. For example: --tapply TIT2=foo,TALB=bar results in:
unquoted
and --tapply 'TIT2="Foo foo",TALB=bar' results in:
quoted

UPDATE: In the next beta the value can contain spaces with or without surrounding quotes (as long as there is no,, =,:)
Notice the surrounding quotes '.

Without surrounding quotes is currently unsupported and leads to broken results: --tapply TIT2="Foo foo",TALB=bar

The shell removes the quotes before it reaches the program and the key/value parser removes any spaces outside of quotes. Maybe there is an easy fix, tracked in: #5.

@crra
Copy link
Owner

crra commented Jan 30, 2022

With #5 closed, I think it becomes more user-friendly if one uses --tapply TIT2="Foo foo".

@kour1er
Copy link
Author

kour1er commented Jan 30, 2022

I assume it contains spaces or special chars (,, =,:) and then the string must be quoted.

Doh! You're entirely correct. Can't believe I didn't think of that.

So I've tested three more books using the most recent beta you sent and all seems to be working correctly! I will continue to test some more and of course with the new version with the more 'user-friendly' quote version too. Congrats on the solution.

(I still can't get over how fast the tool is)

@crra
Copy link
Owner

crra commented Jan 31, 2022

I consider the current head stable enough for new official release. Please feel free to open new issues any time.

@crra crra closed this as completed Jan 31, 2022
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