internal/library/genre.go

@@ -32,11 +32,25 @@ func extractGenres(meta tag.Metadata, rs io.ReadSeeker) (genres []string, fellBa
 		// No frame at all is the common case for untagged files, and
 		// dhowden/tag will have nothing either — not worth flagging.
 		fellBack = meta.Genre() != ""
-	default:
+	case tag.VORBIS, tag.MP4:
-		// Vorbis comments (FLAC/OGG/Opus) and MP4 atoms don't go through
+		// dhowden/tag keeps only the last of a repeated field here (#2500):
-		// dhowden's welding path, so its value is already a faithful read of
+		// see vorbisgenre.go and mp4genre.go.
-		// the primary genre. Multi-value handling for those containers is a
+		var values []string
-		// separate, unproven concern — see #2500.
+		var err error
+		switch {
+		case meta.Format() == tag.MP4:
+			values, err = readMP4GenreValues(rs)
+		case meta.FileType() == tag.OGG:
+			values, err = readOggGenreValues(rs)
+		default:
+			values, err = readFLACGenreValues(rs)
+		}
+		if err == nil && len(values) > 0 {
+			return normaliseGenres(values), false
+		}
+		// err == nil with no values: the file declares no genre of its own
+		// (or, for MP4, only the numeric gnre atom, which dhowden resolves).
+		fellBack = err != nil && meta.Genre() != ""
 	}
 	return normaliseGenres([]string{meta.Genre()}), fellBack
 }

internal/library/mp4genre.go

@@ -0,0 +1,168 @@
+package library
+
+import (
+	"encoding/binary"
+	"errors"
+	"io"
+	"strings"
+)
+
+// MP4 has the same gap as Vorbis comments (#2500). A tag writer stores several
+// genres as several "data" atoms inside one ©gen atom (mutagen and Picard do),
+// or occasionally as repeated ©gen atoms. dhowden/tag keeps atoms in a map keyed
+// by name (mp4.go: m.data[name] = data), so one value survives either way.
+//
+// Path to the values: moov > udta > meta > ilst > ©gen > data. A file that
+// carries only the numeric "gnre" atom has no ©gen; this reader returns
+// (nil, nil) for it, and extractGenres falls back to dhowden/tag, which resolves
+// gnre to a name.
+
+// maxMP4GenreAtomSize caps how much of one ©gen atom is buffered. Genre text is
+// bytes; the cap only stops a corrupt size from making the scanner allocate.
+const maxMP4GenreAtomSize = 1 << 20
+
+var errMalformedAtom = errors.New("library: malformed MP4 atom")
+
+type mp4Atom struct {
+	kind        string
+	body, limit int64 // body start and end offsets in the file
+}
+
+// readMP4Atom reads the atom header at pos, which must lie within [pos, limit).
+func readMP4Atom(rs io.ReadSeeker, pos, limit int64) (mp4Atom, error) {
+	if _, err := rs.Seek(pos, io.SeekStart); err != nil {
+		return mp4Atom{}, err
+	}
+	var hdr [8]byte
+	if _, err := io.ReadFull(rs, hdr[:]); err != nil {
+		return mp4Atom{}, errMalformedAtom
+	}
+	size := int64(binary.BigEndian.Uint32(hdr[0:4]))
+	hdrLen := int64(8)
+	switch size {
+	case 0: // runs to the end of the enclosing atom
+		size = limit - pos
+	case 1: // 64-bit size follows the type
+		var large [8]byte
+		if _, err := io.ReadFull(rs, large[:]); err != nil {
+			return mp4Atom{}, errMalformedAtom
+		}
+		size = int64(binary.BigEndian.Uint64(large[:]))
+		hdrLen = 16
+	}
+	if size < hdrLen || size > limit-pos {
+		return mp4Atom{}, errMalformedAtom
+	}
+	return mp4Atom{kind: string(hdr[4:8]), body: pos + hdrLen, limit: pos + size}, nil
+}
+
+// findMP4Child returns the first child of the given kind in [start, end).
+func findMP4Child(rs io.ReadSeeker, start, end int64, kind string) (mp4Atom, bool, error) {
+	for pos := start; pos+8 <= end; {
+		a, err := readMP4Atom(rs, pos, end)
+		if err != nil {
+			return mp4Atom{}, false, err
+		}
+		if a.kind == kind {
+			return a, true, nil
+		}
+		pos = a.limit
+	}
+	return mp4Atom{}, false, nil
+}
+
+// readMP4GenreValues returns every ©gen value, in file order. rs is seeked
+// as needed, so it is safe to call after dhowden/tag has consumed the reader.
+func readMP4GenreValues(rs io.ReadSeeker) ([]string, error) {
+	end, err := rs.Seek(0, io.SeekEnd)
+	if err != nil {
+		return nil, err
+	}
+	// moov may sit after mdat; walking headers and seeking past bodies finds
+	// it either way without reading the audio.
+	cur := mp4Atom{body: 0, limit: end}
+	for _, kind := range []string{"moov", "udta", "meta"} {
+		next, found, err := findMP4Child(rs, cur.body, cur.limit, kind)
+		if err != nil {
+			return nil, errNoGenreFrame
+		}
+		if !found {
+			return nil, nil // no iTunes metadata at all
+		}
+		cur = next
+	}
+	// meta is a "full box" with four bytes of version and flags before its
+	// children in the ISO layout. QuickTime-style files omit them, so the
+	// first child (hdlr) starts straight away. Look before skipping.
+	if _, err := rs.Seek(cur.body, io.SeekStart); err != nil {
+		return nil, errNoGenreFrame
+	}
+	var peek [8]byte
+	if _, err := io.ReadFull(rs, peek[:]); err != nil {
+		return nil, errNoGenreFrame
+	}
+	if string(peek[4:8]) != "hdlr" {
+		cur.body += 4
+	}
+	ilst, found, err := findMP4Child(rs, cur.body, cur.limit, "ilst")
+	if err != nil {
+		return nil, errNoGenreFrame
+	}
+	if !found {
+		return nil, nil
+	}
+
+	var out []string
+	for pos := ilst.body; pos+8 <= ilst.limit; {
+		a, err := readMP4Atom(rs, pos, ilst.limit)
+		if err != nil {
+			return nil, errNoGenreFrame
+		}
+		pos = a.limit
+		if a.kind != "\xa9gen" {
+			continue
+		}
+		n := a.limit - a.body
+		if n > maxMP4GenreAtomSize {
+			return nil, errNoGenreFrame
+		}
+		if _, err := rs.Seek(a.body, io.SeekStart); err != nil {
+			return nil, errNoGenreFrame
+		}
+		body := make([]byte, n)
+		if _, err := io.ReadFull(rs, body); err != nil {
+			return nil, errNoGenreFrame
+		}
+		values, ok := mp4DataValues(body)
+		if !ok {
+			return nil, errNoGenreFrame
+		}
+		out = append(out, values...)
+	}
+	return out, nil
+}
+
+// mp4DataValues reads the "data" atoms in a ©gen body. Each is: size, "data",
+// a 4-byte type indicator (1 = UTF-8 text), a 4-byte locale, then the value.
+// Atoms of other types carry no text and are skipped.
+func mp4DataValues(b []byte) ([]string, bool) {
+	var out []string
+	for len(b) >= 8 {
+		size := binary.BigEndian.Uint32(b[0:4])
+		if size < 8 || uint64(size) > uint64(len(b)) {
+			return nil, false
+		}
+		atom := b[:size]
+		b = b[size:]
+		if string(atom[4:8]) != "data" || len(atom) < 16 {
+			continue
+		}
+		if binary.BigEndian.Uint32(atom[8:12])&0x00FFFFFF != 1 {
+			continue
+		}
+		if v := strings.TrimSpace(string(atom[16:])); v != "" {
+			out = append(out, v)
+		}
+	}
+	return out, true
+}

internal/library/scanner.go

@@ -60,7 +60,11 @@ var audioExtensions = map[string]bool{
 //	   (#5241). Lidarr names albums by release group, not by the release id
 //	   albums.mbid holds, so re-acquisition and request completion need it for
 //	   the albums already in the library, not only for files added from now on.
-const tagReadVersion int16 = 3
+//	4: every value of a repeated genre field is read for FLAC, Ogg Vorbis, Opus
+//	   and MP4 (#2500). dhowden/tag kept only the last, and 3,658 of the
+//	   operator's 4,092 FLACs declare more than one, so most of their genres
+//	   were never stored.
+const tagReadVersion int16 = 4
 
 type Stats struct {
 	Scanned int `json:"scanned"`

internal/library/vorbisgenre.go

@@ -0,0 +1,214 @@
+package library
+
+import (
+	"bufio"
+	"bytes"
+	"encoding/binary"
+	"errors"
+	"io"
+	"strings"
+)
+
+// Why this file exists: Vorbis comments (FLAC, Ogg Vorbis, Opus) say a field
+// has several values by repeating it:
+//
+//	GENRE=Boom Bap
+//	GENRE=Downtempo
+//
+// dhowden/tag stores comments in a map keyed by field name
+// (vorbis.go: m.c[strings.ToLower(k)] = v), so each repeat overwrites the last
+// and only the final value survives. On the operator's library that was 3,658
+// of 4,092 FLAC files: Kupla's Life Forms declares eight genres and was stored
+// as "Instrumental Hip Hop" alone (#2500). Nothing looked corrupt; the other
+// seven were simply never seen by browse or the taste profile.
+//
+// The comment block is read here directly, as the ID3v2 genre frame is in
+// id3v2genre.go. Every other field still comes from dhowden/tag.
+
+// maxVorbisCommentSize caps a comment block or packet. Comments are kilobytes;
+// an embedded METADATA_BLOCK_PICTURE can take them to a few megabytes. The cap
+// only stops a corrupt length from making the scanner allocate wildly.
+const maxVorbisCommentSize = 16 << 20
+
+var errMalformedComment = errors.New("library: malformed Vorbis comment")
+
+// readFLACGenreValues returns every GENRE value in a FLAC file's comment block.
+// (nil, nil) means the file is readable and declares no genre. rs is seeked to
+// the start, so it is safe to call after dhowden/tag has consumed the reader.
+func readFLACGenreValues(rs io.ReadSeeker) ([]string, error) {
+	if _, err := rs.Seek(0, io.SeekStart); err != nil {
+		return nil, err
+	}
+	var magic [4]byte
+	if _, err := io.ReadFull(rs, magic[:]); err != nil {
+		return nil, errNoGenreFrame
+	}
+	// Some taggers prepend an ID3v2 tag to a FLAC. The FLAC stream starts after it.
+	if string(magic[:3]) == "ID3" {
+		var rest [6]byte
+		if _, err := io.ReadFull(rs, rest[:]); err != nil {
+			return nil, errNoGenreFrame
+		}
+		size := syncsafeInt(rest[2:6])
+		if size < 0 {
+			return nil, errNoGenreFrame
+		}
+		skip := int64(10 + size)
+		if rest[1]&0x10 != 0 {
+			skip += 10 // 2.4 footer
+		}
+		if _, err := rs.Seek(skip, io.SeekStart); err != nil {
+			return nil, errNoGenreFrame
+		}
+		if _, err := io.ReadFull(rs, magic[:]); err != nil {
+			return nil, errNoGenreFrame
+		}
+	}
+	if string(magic[:]) != "fLaC" {
+		return nil, errNoGenreFrame
+	}
+
+	for {
+		var hdr [4]byte
+		if _, err := io.ReadFull(rs, hdr[:]); err != nil {
+			return nil, errNoGenreFrame
+		}
+		last := hdr[0]&0x80 != 0
+		blockType := hdr[0] & 0x7F
+		size := int64(hdr[1])<<16 | int64(hdr[2])<<8 | int64(hdr[3])
+		switch blockType {
+		case 4: // VORBIS_COMMENT
+			body := make([]byte, size)
+			if _, err := io.ReadFull(rs, body); err != nil {
+				return nil, errNoGenreFrame
+			}
+			return vorbisCommentGenres(body)
+		case 127: // invalid by spec; nothing after it can be trusted
+			return nil, errNoGenreFrame
+		}
+		if last {
+			return nil, nil // no comment block at all
+		}
+		if _, err := rs.Seek(size, io.SeekCurrent); err != nil {
+			return nil, errNoGenreFrame
+		}
+	}
+}
+
+// readOggGenreValues returns every GENRE value in an Ogg Vorbis or Opus file.
+// The comments are the stream's second packet, which can span several pages
+// when it carries cover art, so pages are reassembled until it is complete.
+// Only the first logical stream is read; pages of any other are skipped.
+func readOggGenreValues(rs io.ReadSeeker) ([]string, error) {
+	if _, err := rs.Seek(0, io.SeekStart); err != nil {
+		return nil, err
+	}
+	br := bufio.NewReader(rs)
+	var (
+		serial    uint32
+		haveFirst bool
+		packet    []byte
+		packetIdx int
+	)
+	for {
+		var hdr [27]byte
+		if _, err := io.ReadFull(br, hdr[:]); err != nil {
+			return nil, errNoGenreFrame
+		}
+		if string(hdr[0:4]) != "OggS" {
+			return nil, errNoGenreFrame
+		}
+		pageSerial := binary.LittleEndian.Uint32(hdr[14:18])
+		segments := make([]byte, hdr[26])
+		if _, err := io.ReadFull(br, segments); err != nil {
+			return nil, errNoGenreFrame
+		}
+		if !haveFirst {
+			serial, haveFirst = pageSerial, true
+		}
+		if pageSerial != serial {
+			var n int64
+			for _, l := range segments {
+				n += int64(l)
+			}
+			if _, err := io.CopyN(io.Discard, br, n); err != nil {
+				return nil, errNoGenreFrame
+			}
+			continue
+		}
+		for _, l := range segments {
+			if len(packet)+int(l) > maxVorbisCommentSize {
+				return nil, errNoGenreFrame
+			}
+			seg := make([]byte, l)
+			if _, err := io.ReadFull(br, seg); err != nil {
+				return nil, errNoGenreFrame
+			}
+			packet = append(packet, seg...)
+			if l == 255 {
+				continue // the packet carries on in the next segment
+			}
+			if packetIdx == 1 {
+				return oggCommentGenres(packet)
+			}
+			packetIdx++
+			packet = packet[:0]
+		}
+	}
+}
+
+// oggCommentGenres strips the codec's comment-packet prefix. Ogg FLAC and Speex
+// are rare enough to leave to dhowden/tag.
+func oggCommentGenres(packet []byte) ([]string, error) {
+	switch {
+	case bytes.HasPrefix(packet, []byte("\x03vorbis")):
+		return vorbisCommentGenres(packet[7:])
+	case bytes.HasPrefix(packet, []byte("OpusTags")):
+		return vorbisCommentGenres(packet[8:])
+	}
+	return nil, errNoGenreFrame
+}
+
+// vorbisCommentGenres reads a comment block (vendor string, then a count of
+// length-prefixed "KEY=value" entries, little-endian lengths throughout) and
+// returns the GENRE values in file order. Field names are case-insensitive by
+// spec. Anything after the last entry, such as Vorbis's framing bit, is ignored.
+func vorbisCommentGenres(b []byte) ([]string, error) {
+	next := func() ([]byte, bool) {
+		if len(b) < 4 {
+			return nil, false
+		}
+		n := binary.LittleEndian.Uint32(b)
+		if uint64(n) > uint64(len(b)-4) {
+			return nil, false
+		}
+		field := b[4 : 4+n]
+		b = b[4+n:]
+		return field, true
+	}
+	if _, ok := next(); !ok { // vendor string
+		return nil, errMalformedComment
+	}
+	if len(b) < 4 {
+		return nil, errMalformedComment
+	}
+	count := binary.LittleEndian.Uint32(b)
+	b = b[4:]
+	var out []string
+	// Each entry costs at least four bytes, so a corrupt count runs out of
+	// input long before it runs up a loop.
+	for i := uint32(0); i < count; i++ {
+		field, ok := next()
+		if !ok {
+			return nil, errMalformedComment
+		}
+		key, value, found := strings.Cut(string(field), "=")
+		if !found || !strings.EqualFold(key, "GENRE") {
+			continue
+		}
+		if value = strings.TrimSpace(value); value != "" {
+			out = append(out, value)
+		}
+	}
+	return out, nil
+}

internal/library/vorbisgenre_test.go

@@ -0,0 +1,288 @@
+package library
+
+import (
+	"bytes"
+	"encoding/binary"
+	"strings"
+	"testing"
+
+	"github.com/dhowden/tag"
+)
+
+// vorbisComment builds a comment block: vendor, count, then each "KEY=value".
+func vorbisComment(fields ...string) []byte {
+	var b bytes.Buffer
+	vendor := "minstrel-test"
+	_ = binary.Write(&b, binary.LittleEndian, uint32(len(vendor)))
+	b.WriteString(vendor)
+	_ = binary.Write(&b, binary.LittleEndian, uint32(len(fields)))
+	for _, f := range fields {
+		_ = binary.Write(&b, binary.LittleEndian, uint32(len(f)))
+		b.WriteString(f)
+	}
+	return b.Bytes()
+}
+
+// buildFLAC assembles a STREAMINFO block and a comment block, optionally
+// behind an ID3v2 tag the way some taggers write them.
+func buildFLAC(withID3 bool, fields ...string) []byte {
+	var b bytes.Buffer
+	if withID3 {
+		pad := make([]byte, 20)
+		b.WriteString("ID3")
+		b.Write([]byte{4, 0, 0})
+		b.Write(synchsafeBytes(len(pad)))
+		b.Write(pad)
+	}
+	b.WriteString("fLaC")
+	b.Write([]byte{0x00, 0, 0, 34}) // STREAMINFO, not last
+	b.Write(make([]byte, 34))
+	vc := vorbisComment(fields...)
+	b.Write([]byte{0x84, byte(len(vc) >> 16), byte(len(vc) >> 8), byte(len(vc))}) // last, type 4
+	b.Write(vc)
+	return b.Bytes()
+}
+
+// oggCRC is the Ogg page checksum: CRC-32, polynomial 0x04c11db7, no
+// reflection, zero initial value. dhowden/tag verifies it, so the end-to-end
+// tests need real pages.
+func oggCRC(p []byte) uint32 {
+	var crc uint32
+	for _, c := range p {
+		crc ^= uint32(c) << 24
+		for i := 0; i < 8; i++ {
+			if crc&0x80000000 != 0 {
+				crc = crc<<1 ^ 0x04c11db7
+			} else {
+				crc <<= 1
+			}
+		}
+	}
+	return crc
+}
+
+// buildOgg lays packets out as Ogg pages for one logical stream, splitting a
+// packet across pages when it needs more than 255 lacing values.
+func buildOgg(serial uint32, packets ...[]byte) []byte {
+	var out bytes.Buffer
+	var seq uint32
+	for _, p := range packets {
+		var lacing []byte
+		for n := len(p); ; n -= 255 {
+			if n >= 255 {
+				lacing = append(lacing, 255)
+				continue
+			}
+			lacing = append(lacing, byte(n))
+			break
+		}
+		continued := false
+		for off := 0; len(lacing) > 0; {
+			segs := lacing
+			if len(segs) > 255 {
+				segs = segs[:255]
+			}
+			lacing = lacing[len(segs):]
+			n := 0
+			for _, s := range segs {
+				n += int(s)
+			}
+			var page bytes.Buffer
+			page.WriteString("OggS")
+			page.WriteByte(0)
+			flags := byte(0)
+			if continued {
+				flags |= 0x01
+			}
+			if seq == 0 {
+				flags |= 0x02
+			}
+			page.WriteByte(flags)
+			page.Write(make([]byte, 8)) // granule position
+			_ = binary.Write(&page, binary.LittleEndian, serial)
+			_ = binary.Write(&page, binary.LittleEndian, seq)
+			page.Write(make([]byte, 4)) // checksum, filled below
+			page.WriteByte(byte(len(segs)))
+			page.Write(segs)
+			page.Write(p[off : off+n])
+			raw := page.Bytes()
+			binary.LittleEndian.PutUint32(raw[22:26], oggCRC(raw))
+			out.Write(raw)
+			off += n
+			seq++
+			continued = true
+		}
+	}
+	return out.Bytes()
+}
+
+func vorbisIdent() []byte { return append([]byte("\x01vorbis"), make([]byte, 23)...) }
+
+// mp4Box wraps children in an atom of the given kind.
+func mp4Box(kind string, children ...[]byte) []byte {
+	body := bytes.Join(children, nil)
+	b := make([]byte, 8, 8+len(body))
+	binary.BigEndian.PutUint32(b, uint32(8+len(body)))
+	copy(b[4:], kind)
+	return append(b, body...)
+}
+
+func mp4Text(value string) []byte {
+	return mp4Box("data", []byte{0, 0, 0, 1, 0, 0, 0, 0}, []byte(value))
+}
+
+// buildMP4 places the ©gen atoms under moov/udta/meta/ilst. isoMeta adds the
+// four version/flags bytes ISO files carry before meta's children.
+func buildMP4(isoMeta bool, genAtoms ...[]byte) []byte {
+	ilst := mp4Box("ilst", append([][]byte{mp4Box("\xa9nam", mp4Text("A Song"))}, genAtoms...)...)
+	hdlr := mp4Box("hdlr", make([]byte, 25))
+	var meta []byte
+	if isoMeta {
+		meta = mp4Box("meta", []byte{0, 0, 0, 0}, hdlr, ilst)
+	} else {
+		meta = mp4Box("meta", hdlr, ilst)
+	}
+	ftyp := mp4Box("ftyp", []byte("M4A \x00\x00\x00\x00M4A mp42isom"))
+	mdat := mp4Box("mdat", make([]byte, 64))
+	// moov after mdat, as many encoders write it.
+	return bytes.Join([][]byte{ftyp, mdat, mp4Box("moov", mp4Box("udta", meta))}, nil)
+}
+
+var lifeForms = []string{
+	"Boom Bap", "Downtempo", "Hip Hop", "Instrumental",
+	"Lo-Fi", "Lo-Fi Hip Hop", "Chillwave", "Instrumental Hip Hop",
+}
+
+func genreFields(values ...string) []string {
+	out := make([]string, len(values))
+	for i, v := range values {
+		out[i] = "GENRE=" + v
+	}
+	return out
+}
+
+// The #2500 regression, shaped like the operator's Kupla - Life Forms files:
+// eight GENRE fields, of which dhowden/tag keeps the last.
+func TestExtractGenres_FLACRepeatedFields(t *testing.T) {
+	data := buildFLAC(false, append([]string{"TITLE=Eons"}, genreFields(lifeForms...)...)...)
+	rs := bytes.NewReader(data)
+	meta, err := tag.ReadFrom(rs)
+	if err != nil {
+		t.Fatalf("tag.ReadFrom: %v", err)
+	}
+	// Confirm the upstream behaviour this fix exists for is still present.
+	if got := meta.Genre(); got != "Instrumental Hip Hop" {
+		t.Logf("note: dhowden/tag no longer keeps only the last GENRE (got %q)", got)
+	}
+	genres, fellBack := extractGenres(meta, rs)
+	if fellBack {
+		t.Error("fellBack = true, want false")
+	}
+	if !equalStrings(genres, lifeForms) {
+		t.Errorf("genres = %q, want %q", genres, lifeForms)
+	}
+}
+
+func TestReadFLACGenreValues(t *testing.T) {
+	tests := []struct {
+		name string
+		data []byte
+		want []string
+		err  bool
+	}{
+		{"field names are case-insensitive", buildFLAC(false, "genre=Shoegaze", "Genre=Dream Pop"), []string{"Shoegaze", "Dream Pop"}, false},
+		{"behind an ID3v2 tag", buildFLAC(true, "GENRE=Jazz", "GENRE=Bop"), []string{"Jazz", "Bop"}, false},
+		{"no genre is not an error", buildFLAC(false, "TITLE=x"), nil, false},
+		{"empty values dropped", buildFLAC(false, "GENRE= ", "GENRE=Rock"), []string{"Rock"}, false},
+		{"a key that only starts with GENRE", buildFLAC(false, "GENRES=Rock", "GENRE=Jazz"), []string{"Jazz"}, false},
+		{"not a FLAC", []byte("RIFF....WAVEfmt "), nil, true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := readFLACGenreValues(bytes.NewReader(tc.data))
+			if (err != nil) != tc.err {
+				t.Fatalf("err = %v, want error: %v", err, tc.err)
+			}
+			if !equalStrings(got, tc.want) {
+				t.Errorf("got %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestReadFLACGenreValues_TruncatedCommentIsAnError(t *testing.T) {
+	data := buildFLAC(false, "GENRE=Jazz", "GENRE=Bop")
+	if _, err := readFLACGenreValues(bytes.NewReader(data[:len(data)-2])); err == nil {
+		t.Error("err = nil for a cut-off comment block")
+	}
+}
+
+func TestExtractGenres_OggVorbisRepeatedFields(t *testing.T) {
+	comment := append([]byte("\x03vorbis"), vorbisComment(genreFields("Shoegaze", "Dream Pop")...)...)
+	comment = append(comment, 1) // framing bit
+	rs := bytes.NewReader(buildOgg(7, vorbisIdent(), comment))
+	meta, err := tag.ReadFrom(rs)
+	if err != nil {
+		t.Fatalf("tag.ReadFrom: %v", err)
+	}
+	genres, fellBack := extractGenres(meta, rs)
+	if fellBack || !equalStrings(genres, []string{"Shoegaze", "Dream Pop"}) {
+		t.Errorf("genres = %q (fellBack %v), want [Shoegaze Dream Pop]", genres, fellBack)
+	}
+}
+
+func TestReadOggGenreValues_Opus(t *testing.T) {
+	head := append([]byte("OpusHead"), make([]byte, 11)...)
+	tags := append([]byte("OpusTags"), vorbisComment(genreFields("Ambient", "Drone")...)...)
+	got, err := readOggGenreValues(bytes.NewReader(buildOgg(3, head, tags)))
+	if err != nil || !equalStrings(got, []string{"Ambient", "Drone"}) {
+		t.Errorf("got %q, %v; want [Ambient Drone]", got, err)
+	}
+}
+
+// Cover art makes the comment packet span pages. A reader that stopped at the
+// first page boundary would read a truncated block.
+func TestReadOggGenreValues_PacketSpansPages(t *testing.T) {
+	fields := append([]string{"METADATA_BLOCK_PICTURE=" + strings.Repeat("A", 70000)}, genreFields("Jazz", "Bop")...)
+	comment := append([]byte("\x03vorbis"), vorbisComment(fields...)...)
+	got, err := readOggGenreValues(bytes.NewReader(buildOgg(9, vorbisIdent(), comment)))
+	if err != nil || !equalStrings(got, []string{"Jazz", "Bop"}) {
+		t.Errorf("got %q, %v; want [Jazz Bop]", got, err)
+	}
+}
+
+func TestReadMP4GenreValues(t *testing.T) {
+	tests := []struct {
+		name string
+		data []byte
+		want []string
+	}{
+		{"several data atoms in one ©gen", buildMP4(true, mp4Box("\xa9gen", mp4Text("Hip Hop"), mp4Text("Lo-Fi"))), []string{"Hip Hop", "Lo-Fi"}},
+		{"repeated ©gen atoms", buildMP4(true, mp4Box("\xa9gen", mp4Text("Hip Hop")), mp4Box("\xa9gen", mp4Text("Lo-Fi"))), []string{"Hip Hop", "Lo-Fi"}},
+		{"QuickTime meta without version bytes", buildMP4(false, mp4Box("\xa9gen", mp4Text("Jazz"), mp4Text("Bop"))), []string{"Jazz", "Bop"}},
+		{"no ©gen is not an error", buildMP4(true), nil},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := readMP4GenreValues(bytes.NewReader(tc.data))
+			if err != nil {
+				t.Fatalf("err = %v", err)
+			}
+			if !equalStrings(got, tc.want) {
+				t.Errorf("got %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestExtractGenres_MP4MultiValue(t *testing.T) {
+	rs := bytes.NewReader(buildMP4(true, mp4Box("\xa9gen", mp4Text("Hip Hop"), mp4Text("Lo-Fi"))))
+	meta, err := tag.ReadFrom(rs)
+	if err != nil {
+		t.Fatalf("tag.ReadFrom: %v", err)
+	}
+	genres, fellBack := extractGenres(meta, rs)
+	if fellBack || !equalStrings(genres, []string{"Hip Hop", "Lo-Fi"}) {
+		t.Errorf("genres = %q (fellBack %v), want [Hip Hop Lo-Fi]", genres, fellBack)
+	}
+}