-
Notifications
You must be signed in to change notification settings - Fork 51
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
support CHAP frames #62
Conversation
@BoGeM Hello, could you review this PR? |
Just passing by; excellent work on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @takaishi,
thank you for the PR and sorry for so big delay. It's awesome functionality you implemented! Please take a look at my comments :)
chapter_frame.go
Outdated
buf := getByteSlice(32 * 1024) | ||
defer putByteSlice(buf) | ||
for { | ||
// no way to determine whether this should be true or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synchSafe is true when tag version is 4. You have to add an argument synchSafe bool
to all parser functions and provide tag.Version() === 4
to them like here
- test with TIT2 and TIT3 pattern - improve error message
@BoGeM Hello, I added commit. Could you review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @takaishi,
I'm super sorry for so huge delay. I left some simple and minor suggestions. Please resolve them before merging :)
Also one important thing: did you test the MP3 file with written CHAPs via id3v2 in a major media player (e.g. Apple Music, WinAmp)? Does a player recognise such CHAPs? If yes, please send me a screenshot 🙏 |
More clarity message Co-authored-by: Albert Nigmatzianov <[email protected]>
More clarity message Co-authored-by: Albert Nigmatzianov <[email protected]>
More clarity message Co-authored-by: Albert Nigmatzianov <[email protected]>
More clarity message Co-authored-by: Albert Nigmatzianov <[email protected]>
More clarity message Co-authored-by: Albert Nigmatzianov <[email protected]>
More clarity message Co-authored-by: Albert Nigmatzianov <[email protected]>
More clarity message Co-authored-by: Albert Nigmatzianov <[email protected]>
@BoGeM I'm so sorry for late, I added commits for your comment. And I added results of test chapters written by id3v2 package:
I tried test in a Apple Music or QuickTime Player, but they may not support chapters. I don't know major player supports chapter, so I tested by 2 software ( Forecast and IINA ). First, I wrote chapters to test.mp3 by following code: Code to write 3 chapters to copied file from testdata/test.mp3package main
import (
"fmt"
"github.com/bogem/id3v2"
"os"
"time"
)
func main() {
if err := _main(); err != nil {
fmt.Errorf(err.Error())
os.Exit(1)
}
}
func _main() error {
fmt.Println("AAA")
f, err := os.Open("./test_copied.mp3")
if err != nil {
return err
}
defer f.Close()
tag, err := id3v2.Open(f.Name(), id3v2.Options{Parse: true})
if tag == nil || err != nil {
return fmt.Errorf("error while opening mp3 file: ", err)
}
defer tag.Close()
cfs := []id3v2.ChapterFrame{
{
ElementID: "0",
StartTime: 0,
EndTime: time.Duration(60000 * 1000000),
StartOffset: 0,
EndOffset: 0,
Title: &id3v2.TextFrame{
Text: "chap0",
Encoding: id3v2.EncodingUTF8,
},
Description: &id3v2.TextFrame{
Text: "description0",
Encoding: id3v2.EncodingUTF8,
},
},
{
ElementID: "1",
StartTime: time.Duration(60000 * 1000000),
EndTime: time.Duration(120000 * 1000000),
StartOffset: 0,
EndOffset: 0,
Title: &id3v2.TextFrame{
Text: "chap1",
Encoding: id3v2.EncodingUTF8,
},
Description: &id3v2.TextFrame{
Text: "description1",
Encoding: id3v2.EncodingUTF8,
},
},
{
ElementID: "2",
StartTime: time.Duration(120000 * 1000000),
EndTime: time.Duration(180000 * 1000000),
StartOffset: 0,
EndOffset: 0,
Title: &id3v2.TextFrame{
Text: "chap2",
Encoding: id3v2.EncodingUTF8,
},
Description: &id3v2.TextFrame{
Text: "description2",
Encoding: id3v2.EncodingUTF8,
},
},
}
for _, cf := range cfs {
tag.AddChapterFrame(cf)
}
if err := tag.Save(); err != nil {
return err
}
return nil
} Second, I opened mp3 file by Forecast that is chapter editor of mp3. It has 3 chapters with same start time, duration and title. Finally, I played mp3 file by IINA because IINA supports chapters. In following gif movie, I select chapter 1 and 2, player jumps selected point (Sorry menu text is japanese) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome feature! Huge thanks to you for making this 👍
Super excited for this. Thanks for all the hard work! |
@takaishi - I have tried to add chapters, and it worked the same way as you show in Forecast screenshot above. The issue (well, at least I think it is an issue) is that players don't show chapters. For example, Quick View doesn't show any. I think the reason can be seen on your screenshot - as you can see, the third column ("Include in chapters list") is unselected for each chapter. I have the same mp3 tagged with Python's eyed3, and all the chapters are marked as included and showed just fine by any player I have tried. I'm not an expert in mp3 tags, but this may be due to a missing CTOC frame. I have tried to add it manually, like this, but it didn't change the result. Any bright ideas or suggestions on how to fix it?
|
Support to add and parse CHAP frames according to https://id3.org/id3v2-chapters-1.0 .
Similar PullRequest already opened (#40), but it looks like doesn't proceed.
I respect it's commit and update in this PullRequest.