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

First implementation of chapter TOC #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ func (tag *Tag) AddAttachedPicture(pf PictureFrame) {
tag.AddFrame(tag.CommonID("Attached picture"), pf)
}

// AddChapterFrame adds the chapter frame to tag.
func (tag *Tag) AddChapterFrame(cf ChapterFrame) {
tag.AddFrame(tag.CommonID("Chapters"), cf)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, AddChapterFrame is used in V1:chapter_frame_test.go. Either remove the test or the implementation.


// AddCommentFrame adds the comment frame to tag.
func (tag *Tag) AddCommentFrame(cf CommentFrame) {
tag.AddFrame(tag.CommonID("Comments"), cf)
Expand Down
141 changes: 141 additions & 0 deletions v2/chapter_toc_frame.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package id3v2

import (
"encoding/binary"
"errors"
"fmt"
"io"
)

const (
maskOrdered = byte(1 << 0)
maskToplevel = byte(1 << 1)
)

var ErrUnexpectedId = errors.New("unexpected ID")

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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

TopLevel bool
// This provides a hint as to whether the elements should be played as a continuous ordered sequence or played individually.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

Ordered bool
ChapterIds []string
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's better to name properties the same as they named in the spec. Also good idea to provide comment for it:

Suggested change
ChapterIds []string
// Zero or more CHAP/CTOC Child element IDs.
ChildElementIds []string

Description *TextFrame
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be named as DescriptiveData like in spec:

Suggested change
Description *TextFrame
DescriptiveData *TextFrame

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func (ctf ChapterTocFrame) Size() int {
size := encodedSize(ctf.ElementID, EncodingISO)
size += 1 // trailing zero after ElementID
size += 1 // CTOC Flags
// The Entry count is the number of entries in the Child Element ID
// list that follows and must be greater than zero.
size += 1 // Entrycount

// entries
for _, id := range ctf.ChapterIds {
size += encodedSize(id, EncodingISO)
size += 1 // trailing zero after ID
}

// (optional) descriptive data
if ctf.Description != nil {
size += frameHeaderSize // Description frame header size
size += ctf.Description.Size()
}

return size
}

func (ctf ChapterTocFrame) UniqueIdentifier() string {
return ctf.ElementID
}

func (ctf ChapterTocFrame) WriteTo(w io.Writer) (n int64, err error) {
return useBufWriter(w, func(bw *bufWriter) {
bw.EncodeAndWriteText(ctf.ElementID, EncodingISO)
bw.WriteByte(0)

ctocFlags := byte(0)
if ctf.TopLevel {
ctocFlags |= maskToplevel
}
if ctf.Ordered {
ctocFlags |= maskOrdered
}

binary.Write(bw, binary.BigEndian, ctocFlags)

binary.Write(bw, binary.BigEndian, uint8(len(ctf.ChapterIds)))

for _, id := range ctf.ChapterIds {
bw.EncodeAndWriteText(id, EncodingISO)
bw.WriteByte(0)
}

if ctf.Description != nil {
writeFrame(bw, "TIT2", *ctf.Description, true)
}
})
}

func parseChapterTocFrame(br *bufReader, version byte) (Framer, error) {
elementID := string(br.ReadText(EncodingISO))
synchSafe := version == 4
var ctocFlags byte
if err := binary.Read(br, binary.BigEndian, &ctocFlags); err != nil {
return nil, err
}

var elements uint8
if err := binary.Read(br, binary.BigEndian, &elements); err != nil {
return nil, err
}

chaptersIDs := make([]string, elements)
for i := uint8(0); i < elements; i++ {
chaptersIDs[i] = string(br.ReadText(EncodingISO))
}

var description TextFrame

// borrowed from parse.go
buf := getByteSlice(32 * 1024)
defer putByteSlice(buf)

for {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

header, err := parseFrameHeader(buf, br, synchSafe)
if err == io.EOF || err == errBlankFrame || err == ErrInvalidSizeFormat {
break
}

if err != nil {
return nil, err
}

if header.ID != "TIT2" {
return nil, fmt.Errorf("expected: '%s', got: '%s' : %w", "TIT2", header.ID, ErrUnexpectedId)
}

bodyRd := getLimitedReader(br, header.BodySize)
br := newBufReader(bodyRd)
frame, err := parseTextFrame(br)
if err != nil {
putLimitedReader(bodyRd)
return nil, err
}
description = frame.(TextFrame)

putLimitedReader(bodyRd)
}

tocFrame := ChapterTocFrame{
ElementID: elementID,
TopLevel: (ctocFlags & maskToplevel) == maskToplevel,
Ordered: (ctocFlags & maskOrdered) == maskOrdered,
ChapterIds: chaptersIDs,
Description: &description,
}

return tocFrame, nil
}
116 changes: 116 additions & 0 deletions v2/chapter_toc_frame_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package id3v2

import (
"bytes"
"fmt"
"log"
"testing"
"time"
)

const (
testChapterTocSampleTitle = "Chapter TOC title"
)

func newChapterFrames(noOfChapters int) []ChapterFrame {
var start time.Duration
offset := time.Duration(1000 * nanosInMillis)

chapters := make([]ChapterFrame, noOfChapters)

for i := 0; i < noOfChapters; i++ {
end := start + offset

chapters[i] = ChapterFrame{
ElementID: fmt.Sprintf("ch%d", i),
StartTime: start,
EndTime: end,
StartOffset: IgnoredOffset,
EndOffset: IgnoredOffset,
Title: &TextFrame{
Encoding: EncodingUTF8,
Text: fmt.Sprintf("Chapter %d", i),
},
}

start = end
}

return chapters
}

func TestAddChapterTocFrame(t *testing.T) {
const noOfChapters = 5
buf := &bytes.Buffer{}
tag := NewEmptyTag()

chapters := newChapterFrames(noOfChapters)

chapterIds := make([]string, len(chapters))
for i, c := range chapters {
tag.AddChapterFrame(c)

chapterIds[i] = c.ElementID
}

chapterToc := ChapterTocFrame{
ElementID: "Main TOC",
TopLevel: true,
Ordered: true,
ChapterIds: chapterIds,
Description: &TextFrame{
Encoding: EncodingUTF8,
Text: testChapterTocSampleTitle,
},
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
Can you please add a test for ChapterTocFrame without Description? 🙂

}

tag.AddChapterTocFrame(chapterToc)
tag.WriteTo(buf)

// Read back

tagBack, err := ParseReader(buf, Options{Parse: true})
if err != nil {
log.Fatal("Error parsing mp3 content: ", err)
}

if !tagBack.HasFrames() {
log.Fatal("No tags in content in mp3 content")
}

chapterTocBackFrame := tag.GetLastFrame("CTOC")
if chapterTocBackFrame == nil {
log.Fatal("Error getting chapter TOC frame: ", err)
}

chapterTocBack, ok := chapterTocBackFrame.(ChapterTocFrame)
if !ok {
log.Fatal("Error casting chapter TOC frame")
}

if chapterToc.ElementID != chapterTocBack.ElementID {
t.Errorf("Expected element ID: %s, but got %s", chapterToc.ElementID, chapterTocBack.ElementID)
}

if chapterToc.TopLevel != chapterTocBack.TopLevel {
t.Errorf("Expected top level: %v, but got %v", chapterToc.TopLevel, chapterTocBack.TopLevel)
}

if chapterToc.Ordered != chapterTocBack.Ordered {
t.Errorf("Expected ordered: %v, but got %v", chapterToc.Ordered, chapterTocBack.Ordered)
}

if expected, actual := len(chapterToc.ChapterIds), len(chapterTocBack.ChapterIds); expected != actual {
t.Errorf("Expected ordered: %v, but got %v", expected, actual)
}

for i := 0; i < len(chapterToc.ChapterIds); i++ {
if expected, actual := chapterToc.ChapterIds[i], chapterTocBack.ChapterIds[i]; expected != actual {
t.Errorf("Expected chapter reference at index: %d: %s, but got %s", i, expected, actual)
}
}

if chapterToc.Description != nil && chapterToc.Description.Text != chapterTocBack.Description.Text {
t.Errorf("Expected description: %s, but got %s", chapterToc.Description.Text, chapterTocBack.Description.Text)
}
}
3 changes: 3 additions & 0 deletions v2/common_ids.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ var (
V23CommonIDs = map[string]string{
"Attached picture": "APIC",
"Chapters": "CHAP",
"Chapters TOC": "CTOC",
"Comments": "COMM",
"Album/Movie/Show title": "TALB",
"BPM": "TBPM",
Expand Down Expand Up @@ -64,6 +65,7 @@ var (
V24CommonIDs = map[string]string{
"Attached picture": "APIC",
"Chapters": "CHAP",
"Chapters TOC": "CTOC",
"Comments": "COMM",
"Album/Movie/Show title": "TALB",
"BPM": "TBPM",
Expand Down Expand Up @@ -140,6 +142,7 @@ var (
var parsers = map[string]func(*bufReader, byte) (Framer, error){
"APIC": parsePictureFrame,
"CHAP": parseChapterFrame,
"CTOC": parseChapterTocFrame,
"COMM": parseCommentFrame,
"POPM": parsePopularimeterFrame,
"TXXX": parseUserDefinedTextFrame,
Expand Down
4 changes: 4 additions & 0 deletions v2/tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ func (tag *Tag) AddChapterFrame(cf ChapterFrame) {
tag.AddFrame(tag.CommonID("Chapters"), cf)
}

func (tag *Tag) AddChapterTocFrame(ctf ChapterTocFrame) {
tag.AddFrame(tag.CommonID("Chapters TOC"), ctf)
}

// AddCommentFrame adds the comment frame to tag.
func (tag *Tag) AddCommentFrame(cf CommentFrame) {
tag.AddFrame(tag.CommonID("Comments"), cf)
Expand Down