Conversation
|
Hello @BoGeM, did you find the chance to look at the pull request? If it's not to your liking, please help me to improve it. I want to get rid of my fork. |
|
Hi @crra, |
|
Sure, take your time. You can check the live application of the chapters TOCs with mp3binder. The chapters are enabled by default. |
| // AddChapterFrame adds the chapter frame to tag. | ||
| func (tag *Tag) AddChapterFrame(cf ChapterFrame) { | ||
| tag.AddFrame(tag.CommonID("Chapters"), cf) | ||
| } |
There was a problem hiding this comment.
As far as I see this change is in the v1 file, but actually all changes should go to v2 folder and you already made them in v2/tag.go, so please delete it :)
There was a problem hiding this comment.
However, AddChapterFrame is used in V1:chapter_frame_test.go. Either remove the test or the implementation.
|
|
||
| type ChapterTocFrame struct { | ||
| ElementID string | ||
| // This frame is the root of the Table of Contents tree and is not a child of any other "CTOC" frame. |
There was a problem hiding this comment.
| // This frame is the root of the Table of Contents tree and is not a child of any other "CTOC" frame. | |
| // TopLevel defines if a frame is the root of the Table of Contents tree and is not a child of any other "CTOC" frame. |
| ElementID string | ||
| // This frame is the root of the Table of Contents tree and is not a child of any other "CTOC" frame. | ||
| TopLevel bool | ||
| // This provides a hint as to whether the elements should be played as a continuous ordered sequence or played individually. |
There was a problem hiding this comment.
| // This provides a hint as to whether the elements should be played as a continuous ordered sequence or played individually. | |
| // Ordered defines if the entries in the Chapters IDs are ordered. |
| TopLevel bool | ||
| // This provides a hint as to whether the elements should be played as a continuous ordered sequence or played individually. | ||
| Ordered bool | ||
| ChapterIds []string |
There was a problem hiding this comment.
IMO it's better to name properties the same as they named in the spec. Also good idea to provide comment for it:
| ChapterIds []string | |
| // Zero or more CHAP/CTOC Child element IDs. | |
| ChildElementIds []string |
| // This provides a hint as to whether the elements should be played as a continuous ordered sequence or played individually. | ||
| Ordered bool | ||
| ChapterIds []string | ||
| Description *TextFrame |
There was a problem hiding this comment.
I think it should be named as DescriptiveData like in spec:
| Description *TextFrame | |
| DescriptiveData *TextFrame |
There was a problem hiding this comment.
I think my implementation is wrong. After reading the spec again: TIT2 and TIT3 can be added, so it's a collection of text frames rather than one single text frame.
| buf := getByteSlice(32 * 1024) | ||
| defer putByteSlice(buf) | ||
|
|
||
| for { |
There was a problem hiding this comment.
As far as I understand, you're parsing DescriptiveData here. You're using for-loop, but AFAIU there can be only one DescriptiveData, so you don't need loop, right?
| Description: &TextFrame{ | ||
| Encoding: EncodingUTF8, | ||
| Text: testChapterTocSampleTitle, | ||
| }, |
There was a problem hiding this comment.
Nice!
Can you please add a test for ChapterTocFrame without Description? 🙂
|
Hello @BoGeM, How do you read?
Does this mean any frame can be embedded? So it's like a new container for any id3v2 tags listed in |
|
Hey @crra, That's good point actually, but it's so confusing (as a lot of places in id3v2 spec). When I read this, I also have the same understanding as you because of the sentence IMO it's a rare edge case that will require a lot of work and code to implement. IMO for now we can skip the whole descriptive data |
2 participants
Hello Albert,
it seams like that some mp3 players need chapter TOCs in addition to chapter frames to function properly. I've introduced
AddChapterTocFrameaccording to https://id3.org/id3v2-chapters-1.0. I kept the spirit of a 'low-level' API without renaming the boolean values in reverse to support default values (values must be explicitly set). The fields are also named according to the specification to avoid confusion.