From c56eae2959cbc57e57873cc3724ca9fdc05c4891 Mon Sep 17 00:00:00 2001 From: Logan Date: Mon, 24 Aug 2026 05:44:39 -0400 Subject: [PATCH] fix(metadata): read a WAV's tags out of its RIFF id3 chunk tagwriter has always written a WAV's tags into a RIFF "id3 " chunk correctly, and dhowden/tag -- which metadata.ExtractTags is built on -- has no RIFF reader at all. So the app could not see tags it had just written: editing tags on a WAV, autotagging a WAV folder or importing a WAV download all appeared to succeed and changed nothing the library could show, while the file on disk really was tagged and other players read it. backend/riff is a new package rather than a move into either half, because tagwriter already imports metadata: reaching back for parseRIFF is an import cycle, not merely the wrong direction. backend/tagtotals is the precedent. Its two readers are deliberately different. Parse holds every chunk in memory, which is what rewriting a file needs -- and a WAV's audio *is* a chunk, so doing that on the scan path would read every WAV in the library in full. ID3Chunk seeks over what it is not looking for. The container is asked before tag.ReadFrom rather than after it fails, because that library's last resort is an ID3v1 trailer and a WAV carrying both would otherwise be read by the wrong one. An untagged WAV -- no chunk, an RF64 container, a tag with every frame cleared -- reads as empty metadata with no TagReadWarning: the scanner's filename fallback is the right answer there, and a warning would report a fault on a healthy file. The gap was pinned by TestWAVTagsAreNotReadableYet, which failed the moment the reader learned and said in its own comment what to update. So it goes, TestFixturesMatchManifest no longer skips wav, and totals_test.go's WAV case reads through metadata.ExtractTags like the other three formats -- a round trip asserted through the writer's own parser was a test of the writer, which is why nothing caught this. Closes #104 --- CLAUDE.md | 37 ++++- backend/metadata/tags.go | 8 + backend/metadata/tags_wav.go | 68 ++++++++ backend/riff/riff.go | 183 +++++++++++++++++++++ backend/riff/riff_test.go | 211 +++++++++++++++++++++++++ backend/tagwriter/totals_test.go | 9 +- backend/tagwriter/wav.go | 116 ++------------ backend/tagwriter/wav_test.go | 120 ++++++++++++-- internal/testfixtures/fixtures_test.go | 39 ----- 9 files changed, 621 insertions(+), 170 deletions(-) create mode 100644 backend/metadata/tags_wav.go create mode 100644 backend/riff/riff.go create mode 100644 backend/riff/riff_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 97eb7a5..5628c6f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2549,11 +2549,38 @@ Five things about it are load-bearing, and four of them fail silently: correctly. Confidently wrong is worse than absent here, which is the same rule `Known` exists for. -One gap this did not close, and it is older: **`dhowden/tag` has no -RIFF reader**, so nothing the tag writer puts in a WAV's `id3 ` chunk -is visible to `metadata.ExtractTags` — not the totals and not the title -either. `wav_test.go` reads that chunk itself, which is why no test -ever noticed. +One gap this did not close and #104 did: **`dhowden/tag` has no RIFF +reader**, so nothing the tag writer put in a WAV's `id3 ` chunk was +visible to `metadata.ExtractTags` — not the totals and not the title +either, on files the app itself had just tagged. `wav_test.go` read +that chunk itself, which is why no test noticed: a round trip asserted +through the writer's own parser is a test of the writer. + +`backend/riff` is where the container is now read, and it is its own +package because the alternative is an import cycle — `tagwriter` +imports `metadata`, so `metadata` cannot reach back for `parseRIFF`. +`backend/tagtotals` is the precedent. + +Three things about it are load-bearing. **The two readers are +deliberately different**: `Parse` holds every chunk in memory, which is +what rewriting a file needs, and a WAV's audio *is* a chunk — so the +scan path uses `ID3Chunk`, which seeks over what it is not looking for. +**The container decides, before `tag.ReadFrom` rather than after it +fails**, because that library's last resort is an ID3v1 trailer and a +WAV carrying both would otherwise be read by the wrong one. And **an +untagged WAV is a file with no tags, not a file with a problem**: no +chunk, an RF64 container or a tag holding no frames all read as empty +metadata with no `TagReadWarning`, since the scanner's filename +fallback is the right answer and a warning would put a fault on a file +that has none. + +The gap was pinned by a test that said so, which failed the moment the +reader learned and carried the instructions for what to update in its +own comment. So it is deleted, `TestFixturesMatchManifest` no longer +skips `wav`, and `totals_test.go`'s WAV case goes through +`metadata.ExtractTags` like the other three formats. The fixture +library's two WAV tracks scan with their tags and their cover now, +which is a change to what every seeded tier sees. **The absence is what gets marked, not the presence.** The tracklist put a green tick against every owned track and a legend underneath diff --git a/backend/metadata/tags.go b/backend/metadata/tags.go index c734848..ed3e68d 100644 --- a/backend/metadata/tags.go +++ b/backend/metadata/tags.go @@ -74,6 +74,14 @@ func ExtractTags(path string) (*TrackMetadata, error) { // ExtractTagsFromReader reads metadata from an io.ReadSeeker. func ExtractTagsFromReader(r io.ReadSeeker) (*TrackMetadata, error) { + // The container decides, so this is asked before tag.ReadFrom and + // not after its failure: a WAV's tags live in a RIFF chunk that + // dhowden/tag cannot see, and its fallback -- an ID3v1 trailer -- + // would otherwise outrank them. + if meta, ok := wavTags(r); ok { + return meta, nil + } + m, err := tag.ReadFrom(r) if err != nil { // No tags found is not necessarily an error - return empty metadata diff --git a/backend/metadata/tags_wav.go b/backend/metadata/tags_wav.go new file mode 100644 index 0000000..56767f5 --- /dev/null +++ b/backend/metadata/tags_wav.go @@ -0,0 +1,68 @@ +package metadata + +import ( + "bytes" + "errors" + "fmt" + "io" + "strings" + + "yellowjacket/backend/riff" +) + +// wavTags reads the ID3v2 tag a WAV carries in its RIFF "id3 " chunk, +// which is where backend/tagwriter puts it and where dhowden/tag -- +// having no RIFF reader at all -- cannot look. Without this a WAV +// scans as an untagged file however carefully it was tagged. +// +// ok is false when r is not a RIFF/WAVE container, and the read +// position is restored either way so the caller can carry on. +func wavTags(r io.ReadSeeker) (*TrackMetadata, bool) { + start, err := r.Seek(0, io.SeekCurrent) + if err != nil { + return nil, false + } + + id3Data, chunkErr := riff.ID3Chunk(r) + + if _, err := r.Seek(start, io.SeekStart); err != nil { + return nil, false + } + + switch { + case chunkErr == nil: + return wavTagsFrom(id3Data), true + + // Not ours to read: let the ordinary dispatch have the file. + case errors.Is(chunkErr, riff.ErrNotRIFF), errors.Is(chunkErr, riff.ErrNotWAVE): + return nil, false + + // A RIFF container we cannot get a tag out of -- no chunk, an RF64 + // file, a truncated header. That is a file with no readable tags, + // which is what the scanner's filename fallback is for. + default: + return &TrackMetadata{}, true + } +} + +// wavTagsFrom parses the bytes of a WAV's ID3v2 chunk. +func wavTagsFrom(id3Data []byte) *TrackMetadata { + meta, err := extractID3v2Lenient(bytes.NewReader(id3Data)) + if err != nil { + // A tag holding no frames is not a damaged tag: writing every + // field back out empty leaves one, and warning about it would + // put a fault on a file that has none. + if errors.Is(err, ErrTagsUnreadable) { + return &TrackMetadata{} + } + + return &TrackMetadata{ + TagReadWarning: fmt.Errorf("%w: %w", ErrTagsUnreadable, err), + } + } + + // extractID3v2Lenient names MP3, being the recovery path for one. + meta.FileFormat = strings.ToUpper(strings.TrimPrefix(string(WAV), ".")) + + return meta +} diff --git a/backend/riff/riff.go b/backend/riff/riff.go new file mode 100644 index 0000000..e60d265 --- /dev/null +++ b/backend/riff/riff.go @@ -0,0 +1,183 @@ +// Package riff reads the chunk layout of a RIFF/WAVE container. +// +// It exists because both halves of WAV tagging need it and neither can +// import the other: backend/tagwriter writes a WAV's tags into a RIFF +// "id3 " chunk and already imports backend/metadata, which is what has +// to read them back out. backend/tagtotals is the precedent. +// +// The two readers here are deliberately different. Parse holds every +// chunk's data in memory, which is what rewriting a file needs; a WAV's +// audio *is* the "data" chunk, so doing that on the scan path would +// read every library file in full. ID3Chunk seeks over what it is not +// looking for instead. Both walk the same headers. +package riff + +import ( + "bytes" + "encoding/binary" + "errors" + "fmt" + "io" + "strings" +) + +// Sentinel errors describing a container this package will not read. +var ( + ErrRF64NotSupported = errors.New("RF64 files are not yet supported") + ErrNotRIFF = errors.New("not a RIFF file") + ErrNotWAVE = errors.New("not a WAVE file") + ErrNoID3Chunk = errors.New("no ID3 chunk in RIFF file") +) + +// Chunk holds a single RIFF sub-chunk (ID + raw data). +type Chunk struct { + ID [4]byte + Data []byte +} + +// IsID3 reports whether id is that of an ID3v2 RIFF chunk. Both +// lowercase "id3 " and uppercase "ID3 " are accepted. +func IsID3(id [4]byte) bool { + return strings.ToLower(string(id[:3])) == "id3" +} + +// Parse reads every RIFF sub-chunk from r, in order, starting at the +// reader's current position. It rejects RF64 files and non-WAVE +// containers with descriptive errors. The parser is lenient: it +// tolerates a missing final padding byte and ignores the declared +// RIFF size. +func Parse(r io.Reader) ([]Chunk, error) { + if err := readContainer(r); err != nil { + return nil, err + } + + var chunks []Chunk + + for { + id, size, err := nextHeader(r) + if errors.Is(err, io.EOF) { + break + } + + if err != nil { + return nil, err + } + + data := make([]byte, size) + if _, err := io.ReadFull(r, data); err != nil { + return nil, fmt.Errorf("read chunk data for %q: %w", id, err) + } + + chunks = append(chunks, Chunk{ID: id, Data: data}) + + // Odd-length chunks have a padding byte. Lenient: if the + // read fails (e.g. EOF), just break rather than error. + if size%2 != 0 { + var pad [1]byte + + if _, err := r.Read(pad[:]); err != nil { + break + } + } + } + + return chunks, nil +} + +// ID3Chunk returns the payload of the ID3v2 chunk of the RIFF/WAVE +// container at the reader's current position, seeking over every other +// chunk rather than reading it. It returns ErrNoID3Chunk when the +// container carries no such chunk, and leaves the read position +// unspecified either way. +func ID3Chunk(r io.ReadSeeker) ([]byte, error) { + if err := readContainer(r); err != nil { + return nil, err + } + + for { + id, size, err := nextHeader(r) + if errors.Is(err, io.EOF) { + return nil, ErrNoID3Chunk + } + + if err != nil { + return nil, err + } + + if !IsID3(id) { + // Odd-length chunks carry a padding byte. Seeking past + // the end of the file is not an error; the next header + // read is what reports the end. + if _, err := r.Seek(int64(size)+int64(size%2), io.SeekCurrent); err != nil { + return nil, fmt.Errorf("skip chunk %q: %w", id, err) + } + + continue + } + + // Copied rather than allocated up front: a truncated file is + // free to declare a chunk larger than the whole of itself. + var data bytes.Buffer + if _, err := io.CopyN(&data, r, int64(size)); err != nil { + return nil, fmt.Errorf("read chunk data for %q: %w", id, err) + } + + return data.Bytes(), nil + } +} + +// readContainer consumes the 12-byte RIFF/WAVE header at the reader's +// current position. +func readContainer(r io.Reader) error { + var magic [4]byte + if _, err := io.ReadFull(r, magic[:]); err != nil { + return fmt.Errorf("read RIFF magic: %w", err) + } + + if string(magic[:]) == "RF64" { + return ErrRF64NotSupported + } + + if string(magic[:]) != "RIFF" { + return fmt.Errorf("%w: got %q", ErrNotRIFF, magic) + } + + // Read (and discard) RIFF size — lenient, do not enforce. + var riffSize uint32 + if err := binary.Read(r, binary.LittleEndian, &riffSize); err != nil { + return fmt.Errorf("read RIFF size: %w", err) + } + + var form [4]byte + if _, err := io.ReadFull(r, form[:]); err != nil { + return fmt.Errorf("read WAVE form type: %w", err) + } + + if string(form[:]) != "WAVE" { + return fmt.Errorf("%w: got %q", ErrNotWAVE, form) + } + + return nil +} + +// nextHeader reads one sub-chunk header. It returns io.EOF once the +// chunks are exhausted, including for a header cut short. +func nextHeader(r io.Reader) ([4]byte, uint32, error) { + var id [4]byte + + _, err := io.ReadFull(r, id[:]) + if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { + return id, 0, io.EOF + } + + if err != nil { + return id, 0, fmt.Errorf("read chunk ID: %w", err) + } + + var size uint32 + if err := binary.Read(r, binary.LittleEndian, &size); err != nil { + return id, 0, fmt.Errorf("read chunk size for %q: %w", id, err) + } + + return id, size, nil +} diff --git a/backend/riff/riff_test.go b/backend/riff/riff_test.go new file mode 100644 index 0000000..19295ff --- /dev/null +++ b/backend/riff/riff_test.go @@ -0,0 +1,211 @@ +package riff_test + +import ( + "bytes" + "encoding/binary" + "errors" + "testing" + + "yellowjacket/backend/riff" +) + +// chunk is one sub-chunk to put in a test container. +type chunk struct { + id string + data []byte +} + +// buildRIFF assembles a container from magic, form type and chunks, +// padding odd-length chunks the way a writer must. +func buildRIFF(magic, form string, chunks []chunk) []byte { + var body bytes.Buffer + + body.WriteString(form) + + for _, c := range chunks { + body.WriteString(c.id) + _ = binary.Write(&body, binary.LittleEndian, uint32(len(c.data))) + body.Write(c.data) + + if len(c.data)%2 != 0 { + body.WriteByte(0) + } + } + + var out bytes.Buffer + + out.WriteString(magic) + _ = binary.Write(&out, binary.LittleEndian, uint32(body.Len())) + out.Write(body.Bytes()) + + return out.Bytes() +} + +func TestID3Chunk_FindsTheTagPastTheAudio(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + chunks []chunk + want string + }{ + { + name: "after an odd-length chunk", + chunks: []chunk{ + {id: "fmt ", data: make([]byte, 16)}, + {id: "LIST", data: []byte("INFOodd")}, + {id: "data", data: make([]byte, 200)}, + {id: "id3 ", data: []byte("ID3vTAG")}, + }, + want: "ID3vTAG", + }, + { + // The chunk ID is written both ways in the wild, and the + // writer accepts either, so the reader must too. + name: "uppercase ID3", + chunks: []chunk{ + {id: "data", data: make([]byte, 8)}, + {id: "ID3 ", data: []byte("upper")}, + }, + want: "upper", + }, + { + name: "first chunk", + chunks: []chunk{ + {id: "id3 ", data: []byte("first")}, + {id: "data", data: make([]byte, 8)}, + }, + want: "first", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + r := bytes.NewReader(buildRIFF("RIFF", "WAVE", tc.chunks)) + + got, err := riff.ID3Chunk(r) + if err != nil { + t.Fatalf("ID3Chunk: %v", err) + } + + if string(got) != tc.want { + t.Errorf("chunk data: got %q, want %q", got, tc.want) + } + }) + } +} + +func TestID3Chunk_RejectsWhatItCannotRead(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + bytes []byte + want error + }{ + { + name: "no ID3 chunk", + bytes: buildRIFF("RIFF", "WAVE", []chunk{{id: "data", data: []byte{1, 2}}}), + want: riff.ErrNoID3Chunk, + }, + { + name: "no chunks at all", + bytes: buildRIFF("RIFF", "WAVE", nil), + want: riff.ErrNoID3Chunk, + }, + { + name: "not RIFF", + bytes: []byte("ID3\x03\x00\x00\x00\x00\x00\x00\x00\x00"), + want: riff.ErrNotRIFF, + }, + { + name: "not WAVE", + bytes: buildRIFF("RIFF", "AVI ", []chunk{{id: "id3 ", data: []byte("x")}}), + want: riff.ErrNotWAVE, + }, + { + name: "RF64", + bytes: buildRIFF("RF64", "WAVE", []chunk{{id: "id3 ", data: []byte("x")}}), + want: riff.ErrRF64NotSupported, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + _, err := riff.ID3Chunk(bytes.NewReader(tc.bytes)) + if !errors.Is(err, tc.want) { + t.Errorf("ID3Chunk error: got %v, want %v", err, tc.want) + } + }) + } +} + +// A file cut short mid-chunk is a file with no tag, not a reason to +// allocate the size it claims: the declared size is four bytes any +// truncation can leave saying 4 GB. +func TestID3Chunk_ToleratesATruncatedFile(t *testing.T) { + t.Parallel() + + full := buildRIFF("RIFF", "WAVE", []chunk{ + {id: "data", data: make([]byte, 64)}, + {id: "id3 ", data: []byte("tag")}, + }) + + t.Run("cut inside the audio", func(t *testing.T) { + t.Parallel() + + _, err := riff.ID3Chunk(bytes.NewReader(full[:32])) + if !errors.Is(err, riff.ErrNoID3Chunk) { + t.Errorf("ID3Chunk error: got %v, want %v", err, riff.ErrNoID3Chunk) + } + }) + + t.Run("cut inside the tag", func(t *testing.T) { + t.Parallel() + + if _, err := riff.ID3Chunk(bytes.NewReader(full[:len(full)-2])); err == nil { + t.Error("ID3Chunk: got nil error for a truncated tag chunk") + } + }) +} + +// Parse is the writer's half and reads every chunk into memory, which +// is what preserving them needs. +func TestParse_ReadsEveryChunkInOrder(t *testing.T) { + t.Parallel() + + raw := buildRIFF("RIFF", "WAVE", []chunk{ + {id: "fmt ", data: make([]byte, 16)}, + {id: "LIST", data: []byte("INFOodd")}, + {id: "id3 ", data: []byte("tag")}, + }) + + chunks, err := riff.Parse(bytes.NewReader(raw)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + + want := []string{"fmt ", "LIST", "id3 "} + if len(chunks) != len(want) { + t.Fatalf("chunk count: got %d, want %d", len(chunks), len(want)) + } + + for i, id := range want { + if got := string(chunks[i].ID[:]); got != id { + t.Errorf("chunk %d: got %q, want %q", i, got, id) + } + } + + if !riff.IsID3(chunks[2].ID) || string(chunks[2].Data) != "tag" { + t.Errorf("id3 chunk: got %q", chunks[2].Data) + } + + // The padding byte after an odd chunk is not part of its data. + if string(chunks[1].Data) != "INFOodd" { + t.Errorf("odd chunk data: got %q, want %q", chunks[1].Data, "INFOodd") + } +} diff --git a/backend/tagwriter/totals_test.go b/backend/tagwriter/totals_test.go index 860b4ea..c40b993 100644 --- a/backend/tagwriter/totals_test.go +++ b/backend/tagwriter/totals_test.go @@ -13,9 +13,10 @@ import ( // indistinguishable from never having written one. So these assert the // round trip through the *reader the scan uses*, not the bytes. // -// WAV is the exception and it is not this change's: dhowden/tag has no -// RIFF reader at all, so metadata.ExtractTags cannot see a WAV's ID3 -// chunk -- which is why every other test here reads that chunk itself. +// WAV was the exception until #104 -- dhowden/tag has no RIFF reader, +// so metadata.ExtractTags could not see a WAV's ID3 chunk and this +// case read the chunk itself, which is a test of the writer wearing +// the shape of a round trip. All four go through the scanner now. func TestWriteTotals_RoundTripsInEveryFormat(t *testing.T) { t.Parallel() @@ -91,7 +92,7 @@ func TestWriteTotals_RoundTripsInEveryFormat(t *testing.T) { }, { name: "wav", - read: readWavID3Tags, + read: viaScanner, write: func(t *testing.T, dir string) string { t.Helper() diff --git a/backend/tagwriter/wav.go b/backend/tagwriter/wav.go index 583324f..215bd15 100644 --- a/backend/tagwriter/wav.go +++ b/backend/tagwriter/wav.go @@ -8,116 +8,22 @@ import ( "io" "log/slog" "os" - "strings" id3v2 "github.com/bogem/id3v2/v2" "yellowjacket/backend/fileutil" + "yellowjacket/backend/riff" ) -// Sentinel errors for WAV RIFF operations. -var ( - errRF64NotSupported = errors.New("RF64 files are not yet supported") - errNotRIFF = errors.New("not a RIFF file") - errNotWAVE = errors.New("not a WAVE file") - errFileTooLargeForWAV = errors.New("file too large for WAV format (>4GB)") -) - -// riffChunk holds a single RIFF sub-chunk (ID + raw data). -type riffChunk struct { - id [4]byte - data []byte -} - -// parseRIFF reads all RIFF sub-chunks from r. It rejects RF64 files -// and non-WAVE containers with descriptive errors. The parser is -// lenient on read: it tolerates missing padding bytes and ignores -// the declared RIFF size. -func parseRIFF(r io.ReadSeeker) ([]riffChunk, error) { - // Read 4-byte container magic. - var magic [4]byte - if _, err := io.ReadFull(r, magic[:]); err != nil { - return nil, fmt.Errorf("read RIFF magic: %w", err) - } - - if string(magic[:]) == "RF64" { - return nil, errRF64NotSupported - } - - if string(magic[:]) != "RIFF" { - return nil, fmt.Errorf("%w: got %q", errNotRIFF, magic) - } - - // Read (and discard) RIFF size — lenient, do not enforce. - var riffSize uint32 - if err := binary.Read(r, binary.LittleEndian, &riffSize); err != nil { - return nil, fmt.Errorf("read RIFF size: %w", err) - } - - // Read 4-byte form type. - var form [4]byte - if _, err := io.ReadFull(r, form[:]); err != nil { - return nil, fmt.Errorf("read WAVE form type: %w", err) - } - - if string(form[:]) != "WAVE" { - return nil, fmt.Errorf("%w: got %q", errNotWAVE, form) - } - - // Read sub-chunks until EOF. - var chunks []riffChunk - - for { - var chunkID [4]byte - - _, err := io.ReadFull(r, chunkID[:]) - if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { - break - } - - if err != nil { - return nil, fmt.Errorf("read chunk ID: %w", err) - } - - var chunkSize uint32 - if err := binary.Read(r, binary.LittleEndian, &chunkSize); err != nil { - return nil, fmt.Errorf("read chunk size for %q: %w", chunkID, err) - } - - data := make([]byte, chunkSize) - if _, err := io.ReadFull(r, data); err != nil { - return nil, fmt.Errorf("read chunk data for %q: %w", chunkID, err) - } - - chunks = append(chunks, riffChunk{id: chunkID, data: data}) - - // Odd-length chunks have a padding byte. Lenient: if the - // read fails (e.g. EOF), just break rather than error. - if chunkSize%2 != 0 { - var pad [1]byte - - if _, err := r.Read(pad[:]); err != nil { - break - } - } - } - - return chunks, nil -} - -// isID3ChunkID returns true if id represents an ID3v2 RIFF chunk. -// Both lowercase "id3 " and uppercase "ID3 " are accepted. -func isID3ChunkID(id [4]byte) bool { - s := strings.ToLower(string(id[:3])) - - return s == "id3" -} +// errFileTooLargeForWAV is the one RIFF error that belongs to the +// writer; reading rejects a container in backend/riff. +var errFileTooLargeForWAV = errors.New("file too large for WAV format (>4GB)") // writeRIFF writes a complete RIFF/WAVE container to w, preserving // the given chunks in order and appending the id3Data as the final // "id3 " chunk. Returns errFileTooLargeForWAV if the result would // exceed the 4 GB RIFF limit. -func writeRIFF(w io.Writer, chunks []riffChunk, id3Data []byte) error { +func writeRIFF(w io.Writer, chunks []riff.Chunk, id3Data []byte) error { // Calculate total RIFF payload size: // 4 bytes (WAVE form type) // + for each preserved chunk: 8 (header) + len(data) + padding @@ -125,7 +31,7 @@ func writeRIFF(w io.Writer, chunks []riffChunk, id3Data []byte) error { riffPayload := uint64(4) for _, c := range chunks { - sz := uint64(len(c.data)) + sz := uint64(len(c.Data)) riffPayload += 8 + sz if sz%2 != 0 { @@ -162,7 +68,7 @@ func writeRIFF(w io.Writer, chunks []riffChunk, id3Data []byte) error { // Write each preserved chunk. for _, c := range chunks { - if err := writeChunk(w, c.id, c.data); err != nil { + if err := writeChunk(w, c.ID, c.Data); err != nil { return err } } @@ -225,7 +131,7 @@ func writeWavTags( return fmt.Errorf("open wav for reading: %w", err) } - allChunks, err := parseRIFF(f) + allChunks, err := riff.Parse(f) // Close immediately — we need the handle released before // AtomicWrite creates the replacement file. @@ -237,13 +143,13 @@ func writeWavTags( // Separate preserved chunks from existing ID3 data. var ( - preserved []riffChunk + preserved []riff.Chunk existingID3 []byte ) for _, c := range allChunks { - if isID3ChunkID(c.id) { - existingID3 = c.data + if riff.IsID3(c.ID) { + existingID3 = c.Data } else { preserved = append(preserved, c) } diff --git a/backend/tagwriter/wav_test.go b/backend/tagwriter/wav_test.go index e8fa853..5a57cba 100644 --- a/backend/tagwriter/wav_test.go +++ b/backend/tagwriter/wav_test.go @@ -12,6 +12,7 @@ import ( id3v2 "github.com/bogem/id3v2/v2" "yellowjacket/backend/metadata" + "yellowjacket/backend/riff" ) // createTestWAV builds a minimal valid WAV file with an optional @@ -270,6 +271,88 @@ func TestWriteWavTags_PartialUpdate(t *testing.T) { assertStrField(t, "Composer", meta.Composer, "Original Composer") } +// The writer has always been correct and the reader could not see it: +// a WAV tagged by this app scanned as an untagged file, so editing +// tags, autotagging a folder or importing a WAV download all appeared +// to work and changed nothing the library could show (#104). So this +// asserts the write through metadata.ExtractTags -- the reader the +// scan uses -- rather than through the id3 chunk. +func TestWriteWavTags_ReadBackByTheScanner(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := createTestWAV(t, dir, "scanner.wav", nil) + art := tinyJPEG(t) + + changes := TagChanges{ + FieldTitle: "Some Song", + FieldArtist: "Some Artist", + FieldAlbum: "Some Album", + FieldAlbumArtist: "Some Album Artist", + FieldGenre: "Rock", + FieldYear: 2024, + FieldTrackNumber: 3, + FieldComposer: "Some Composer", + FieldCoverArt: art, + } + + if err := writeWavTags(testLogger(), path, changes); err != nil { + t.Fatalf("writeWavTags: %v", err) + } + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + if meta.TagReadWarning != nil { + t.Errorf("TagReadWarning: %v", meta.TagReadWarning) + } + + assertStrField(t, "Title", meta.Title, "Some Song") + assertStrField(t, "Artist", meta.Artist, "Some Artist") + assertStrField(t, "Album", meta.Album, "Some Album") + assertStrField(t, "AlbumArtist", meta.AlbumArtist, "Some Album Artist") + assertStrField(t, "Genre", meta.Genre, "Rock") + assertStrField(t, "Composer", meta.Composer, "Some Composer") + assertStrField(t, "FileFormat", meta.FileFormat, "WAV") + assertIntField(t, "Year", meta.Year, 2024) + assertIntField(t, "TrackNumber", meta.TrackNumber, 3) + + if !strings.HasPrefix(meta.TagFormat, "ID3v2") { + t.Errorf("TagFormat: got %q, want an ID3v2 version", meta.TagFormat) + } + + if meta.Picture == nil { + t.Fatal("expected cover art, got nil") + } + + if !bytes.Equal(meta.Picture.Data, art) { + t.Errorf("picture data mismatch: got %d bytes, want %d", + len(meta.Picture.Data), len(art)) + } +} + +// An untagged WAV is a file with no tags, not a file with a problem: +// the scanner falls back to the filename and must not be handed a +// warning to surface about it. +func TestUntaggedWav_ReadsAsEmptyWithoutAWarning(t *testing.T) { + t.Parallel() + + path := createTestWAV(t, t.TempDir(), "bare.wav", nil) + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + if meta.TagReadWarning != nil { + t.Errorf("TagReadWarning: %v", meta.TagReadWarning) + } + + assertStrField(t, "Title", meta.Title, "") +} + func TestWriteWavTags_ChunkPreservation(t *testing.T) { t.Parallel() @@ -282,7 +365,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { t.Fatalf("open original: %v", err) } - origChunks, err := parseRIFF(origFile) + origChunks, err := riff.Parse(origFile) _ = origFile.Close() if err != nil { @@ -292,7 +375,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { // Record original chunk data by ID string. origData := map[string][]byte{} for _, c := range origChunks { - origData[string(c.id[:])] = c.data + origData[string(c.ID[:])] = c.Data } // Write a tag to trigger RIFF rewrite. @@ -309,7 +392,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { t.Fatalf("open after write: %v", err) } - newChunks, err := parseRIFF(newFile) + newChunks, err := riff.Parse(newFile) _ = newFile.Close() if err != nil { @@ -320,7 +403,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { origNonID3 := 0 for _, c := range origChunks { - if !isID3ChunkID(c.id) { + if !riff.IsID3(c.ID) { origNonID3++ } } @@ -328,7 +411,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { newNonID3 := 0 for _, c := range newChunks { - if !isID3ChunkID(c.id) { + if !riff.IsID3(c.ID) { newNonID3++ } } @@ -359,17 +442,17 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { // in chunks and its data matches want byte-for-byte. func checkChunkPreserved( t *testing.T, - chunks []riffChunk, + chunks []riff.Chunk, idStr string, want []byte, ) { t.Helper() for _, c := range chunks { - if string(c.id[:]) == idStr { - if !bytes.Equal(c.data, want) { + if string(c.ID[:]) == idStr { + if !bytes.Equal(c.Data, want) { t.Errorf("chunk %q data changed: got %d bytes, want %d", - idStr, len(c.data), len(want)) + idStr, len(c.Data), len(want)) } return @@ -430,7 +513,7 @@ func TestWriteWavTags_RejectsRF64(t *testing.T) { buf.WriteString("WAVE") // Minimal ds64 chunk (required for RF64 but we just need - // enough bytes for parseRIFF to hit the RF64 rejection). + // enough bytes for riff.Parse to hit the RF64 rejection). buf.WriteString("ds64") _ = binary.Write(&buf, binary.LittleEndian, uint32(28)) //nolint:mnd buf.Write(make([]byte, 28)) //nolint:mnd @@ -454,9 +537,12 @@ func TestWriteWavTags_RejectsRF64(t *testing.T) { // readWavID3Tags extracts ID3v2 metadata from a WAV file by parsing // the RIFF structure and reading the id3 chunk with bogem/id3v2. -// dhowden/tag's ReadFrom does not support WAV files, and its -// ReadID3v2Tags fails on empty tags (after clearing all frames). -// Using bogem/id3v2.ParseReader handles all cases correctly. +// +// metadata.ExtractTags reads a WAV since #104 and is what the round +// trips assert through. This stays for the two cases that are about +// the bytes rather than about the scan: a tag with every frame +// cleared, which no reader reports as anything, and the chunk +// preservation test, which is already parsing the container itself. func readWavID3Tags( t *testing.T, path string, @@ -470,17 +556,17 @@ func readWavID3Tags( defer func() { _ = f.Close() }() - chunks, err := parseRIFF(f) + chunks, err := riff.Parse(f) if err != nil { - t.Fatalf("parseRIFF: %v", err) + t.Fatalf("riff.Parse: %v", err) } // Find the id3 chunk. var id3Data []byte for _, c := range chunks { - if isID3ChunkID(c.id) { - id3Data = c.data + if riff.IsID3(c.ID) { + id3Data = c.Data break } diff --git a/internal/testfixtures/fixtures_test.go b/internal/testfixtures/fixtures_test.go index ba181f8..b440c8c 100644 --- a/internal/testfixtures/fixtures_test.go +++ b/internal/testfixtures/fixtures_test.go @@ -30,12 +30,6 @@ func TestFixturesMatchManifest(t *testing.T) { m := testfixtures.Load(t) for _, want := range m.Tracks { - // WAV tags are write-only today; see - // TestWAVTagsAreNotReadableYet. - if want.Format == "wav" { - continue - } - t.Run(want.Path, func(t *testing.T) { t.Parallel() @@ -161,39 +155,6 @@ func TestDuplicateFixturesAreIndistinguishable(t *testing.T) { } } -// TestWAVTagsAreNotReadableYet pins a known gap rather than hiding it. -// -// backend/tagwriter writes WAV tags into a RIFF "id3 " chunk, but -// backend/metadata reads through dhowden/tag, which recognises MP3, -// FLAC, OGG, MP4 and DSF and has no RIFF parser at all. So every tag -// the app writes to a WAV is invisible to the app that wrote it, and -// WAV tracks always scan in as untitled. -// -// The fixtures are tagged correctly on disk, so when the reader learns -// to unwrap the RIFF chunk this test starts failing — which is the -// point. Delete it then and drop the "wav" skip in -// TestFixturesMatchManifest. -func TestWAVTagsAreNotReadableYet(t *testing.T) { - t.Parallel() - - m := testfixtures.Load(t) - - for _, path := range m.Case(t, testfixtures.CaseWAVTracks) { - got, err := metadata.ExtractTags(path) - if err != nil { - t.Fatalf("extract tags from %s: %v", path, err) - } - - if got.Title != "" { - t.Errorf( - "%s: WAV tags are now readable (%q) — good news; "+ - "see this test's comment for what to update", - filepath.Base(path), got.Title, - ) - } - } -} - func assertTag(t *testing.T, field string, want testfixtures.Track, got string) { t.Helper()