ogg: support comment header packets spanning pages (#57)

* ogg: support comment header packets spanning pages

* ogg: add test for multipage comment header

* ogg: cleaner buffer initialization

Co-Authored-By: David Howden <dhowden@gmail.com>

ogg.go

@@ -5,6 +5,7 @@
 package tag
 
 import (
+	"bytes"
 	"errors"
 	"io"
 )
@@ -61,34 +62,17 @@ func ReadOGGTags(r io.ReadSeeker) (Metadata, error) {
 		return nil, err
 	}
 
-	// Beginning of a new page. Comment packet is on a separate page
-	// See http://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-132000A.2
-	oggs, err = readString(r, 4)
+	// Read comment header packet. May include setup header packet, if it is on the
+	// same page. First audio packet is guaranteed to be on the separate page.
+	// See https://www.xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-132000A.2
+	ch, err := readPackets(r)
 	if err != nil {
 		return nil, err
 	}
-	if oggs != "OggS" {
-		return nil, errors.New("expected 'OggS'")
-	}
+	chr := bytes.NewReader(ch)
 
-	// Skip page 2 header, same as line 30
-	_, err = r.Seek(22, io.SeekCurrent)
-	if err != nil {
-		return nil, err
-	}
-
-	nS, err = readInt(r, 1)
-	if err != nil {
-		return nil, err
-	}
-
-	_, err = r.Seek(int64(nS), io.SeekCurrent)
-	if err != nil {
-		return nil, err
-	}
-
-	// Packet type is comment, type 3
-	t, err = readInt(r, 1)
+	// First packet type is comment, type 3
+	t, err = readInt(chr, 1)
 	if err != nil {
 		return nil, err
 	}
@@ -97,7 +81,7 @@ func ReadOGGTags(r io.ReadSeeker) (Metadata, error) {
 	}
 
 	// Seek and discard 6 bytes from common header
-	_, err = r.Seek(6, io.SeekCurrent)
+	_, err = chr.Seek(6, io.SeekCurrent)
 	if err != nil {
 		return nil, err
 	}
@@ -106,10 +90,74 @@ func ReadOGGTags(r io.ReadSeeker) (Metadata, error) {
 		newMetadataVorbis(),
 	}
 
-	err = m.readVorbisComment(r)
+	err = m.readVorbisComment(chr)
 	return m, err
 }
 
+// readPackets reads vorbis header packets from contiguous ogg pages in ReadSeeker.
+// The pages are considered contiguous, if the first lacing value in second
+// page's segment table continues rather than begins a packet. This is indicated
+// by setting header_type_flag 0x1 (continued packet).
+// See https://www.xiph.org/ogg/doc/framing.html on packets spanning pages.
+func readPackets(r io.ReadSeeker) ([]byte, error) {
+	buf := &bytes.Buffer{}
+
+	firstPage := true
+	for {
+		// Read capture pattern
+		oggs, err := readString(r, 4)
+		if err != nil {
+			return nil, err
+		}
+		if oggs != "OggS" {
+			return nil, errors.New("expected 'OggS'")
+		}
+
+		// Read page header
+		head, err := readBytes(r, 22)
+		if err != nil {
+			return nil, err
+		}
+		headerTypeFlag := head[1]
+
+		continuation := headerTypeFlag&0x1 > 0
+		if !(firstPage || continuation) {
+			// Rewind to the beginning of the page
+			_, err = r.Seek(-26, io.SeekCurrent)
+			if err != nil {
+				return nil, err
+			}
+			break
+		}
+		firstPage = false
+
+		// Read the number of segments
+		nS, err := readInt(r, 1)
+		if err != nil {
+			return nil, err
+		}
+
+		// Read segment table
+		segments, err := readBytes(r, nS)
+		if err != nil {
+			return nil, err
+		}
+
+		// Calculate remaining page size
+		pageSize := 0
+		for i := 0; i < nS; i++ {
+			pageSize += int(segments[i])
+		}
+
+		_, err = io.CopyN(buf, r, int64(pageSize))
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return buf.Bytes(), nil
+}
+
 type metadataOGG struct {
 	*metadataVorbis
 }

tag_test.go

@@ -54,20 +54,21 @@ var mp3id3v11Metadata = testMetadata{
 
 func TestReadFrom(t *testing.T) {
 	testdata := map[string]testMetadata{
-		"with_tags/sample.flac":       fullMetadata,
-		"with_tags/sample.id3v11.mp3": mp3id3v11Metadata,
-		"with_tags/sample.id3v22.mp3": fullMetadata,
-		"with_tags/sample.id3v23.mp3": fullMetadata,
-		"with_tags/sample.id3v24.mp3": fullMetadata,
-		"with_tags/sample.m4a":        fullMetadata,
-		"with_tags/sample.mp4":        fullMetadata,
-		"with_tags/sample.ogg":        fullMetadata,
-		"with_tags/sample.dsf":        fullMetadata,
-		"without_tags/sample.flac":    emptyMetadata,
-		"without_tags/sample.m4a":     emptyMetadata,
-		"without_tags/sample.mp3":     emptyMetadata,
-		"without_tags/sample.mp4":     emptyMetadata,
-		"without_tags/sample.ogg":     emptyMetadata,
+		"with_tags/sample.flac":          fullMetadata,
+		"with_tags/sample.id3v11.mp3":    mp3id3v11Metadata,
+		"with_tags/sample.id3v22.mp3":    fullMetadata,
+		"with_tags/sample.id3v23.mp3":    fullMetadata,
+		"with_tags/sample.id3v24.mp3":    fullMetadata,
+		"with_tags/sample.m4a":           fullMetadata,
+		"with_tags/sample.mp4":           fullMetadata,
+		"with_tags/sample.ogg":           fullMetadata,
+		"with_tags/sample.multipage.ogg": fullMetadata,
+		"with_tags/sample.dsf":           fullMetadata,
+		"without_tags/sample.flac":       emptyMetadata,
+		"without_tags/sample.m4a":        emptyMetadata,
+		"without_tags/sample.mp3":        emptyMetadata,
+		"without_tags/sample.mp4":        emptyMetadata,
+		"without_tags/sample.ogg":        emptyMetadata,
 	}
 
 	for path, metadata := range testdata {