backend/tagwriter writes a WAV's tags into a RIFF id3 chunk, and metadata.ExtractTags is built on dhowden/tag, which has no RIFF
reader at all. So the app could not read tags it had written itself:
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. WAV is in SupportedFileExtensions, so those files scan and play —
they just arrive with whatever the filename fallback gives and stay
that way.
What changed
Commit
Issue
c56eae2 fix(metadata): read a WAV's tags out of its RIFF id3 chunk
backend/riff is a new package, not a move into either half: tagwriter already imports metadata, so reaching back for parseRIFF is an import cycle rather than merely the wrong direction. backend/tagtotals is the precedent for a small package that exists so
two callers need not import each other.
Three things in it are load-bearing:
The two readers are deliberately different.Parse (the writer's
half, moved verbatim) holds every chunk in memory, which is what
rewriting a file needs — and a WAV's audio is a chunk, so using it
on the scan path would read every WAV in the library in full. ID3Chunk seeks over what it is not looking for, and copies the tag
with io.CopyN rather than allocating the size the header declares.
The container is asked before tag.ReadFrom, not after it fails.
That library's last resort is an ID3v1 trailer, which a WAV is free
to carry, and it would otherwise outrank the chunk the app wrote.
An untagged WAV is a file with no tags, not a file with a
problem. No chunk, an RF64 container, or a tag with every frame
cleared all read as empty metadata with noTagReadWarning — 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 a test that said so. TestWAVTagsAreNotReadableYet failed the moment the reader learned and
carried in its own comment what to update, so it is deleted, TestFixturesMatchManifest no longer skips wav, and totals_test.go's WAV case now reads through metadata.ExtractTags
like the other three formats. That last one is the seam the issue's
"why nothing caught it" names: a round trip asserted through the
writer's own parser is a test of the writer.
Verification
make test — clean, all three build configurations. New cases in backend/riff/riff_test.go (the tag located past an odd-length
chunk, uppercase ID3 , no chunk, not RIFF, not WAVE, RF64, two
truncations) and backend/tagwriter/wav_test.go (the issue's own
reproduction through metadata.ExtractTags, and an untagged WAV
reading empty without a warning).
make lint — 0 issues, all three build configurations.
Proved non-vacuous. With the one dispatch line in ExtractTagsFromReader disabled, the new round trip fails with every field zero and the WAV totals case fails on all four numbers
— which is exactly the symptom #104 was filed from.
Measured in the running app, on a rebuilt default seed: both
fixture WAVs now import as Tone A / Tone B, artist Field Recordings, album Test Tones, genre Field Recording,
2024, with cover art. They used to arrive untitled with no album.
make e2e — 239 passed against make dev-headless SEED=default
(chromium; WebKit is CI's half). Run because this changes what the fixture library contains, which is what every seeded tier sees and
what CI rebuilds its seed from. Port 34115 was free, so this is not
somebody else's app.
No .sql, no .templ, no bound signature: make generate and make bindings were not needed (bindings-check runs pre-commit and
passed). No frontend change, so make ui-test was not run.
Deliberately not done
The issue's suggestion to measure how much WAV is realistically in a
library. It would decide the priority, and the priority is already Low.
#216 — riff.Parse still allocates whatever size a chunk header
claims, which a malformed WAV can make 4 GB. It is the writer's path
only, needs a deliberate tag write, and predates this; the read path
added here does not have that shape. Filed rather than folded in.
#217 — e2e/specs/queue-selection.spec.ts's comment names 01 Tone A and 02 Tone B as the fixture library's albumless
tracks, and they are not any more. The spec still passes and its
filter is still needed (two untagged mp3s remain albumless); the
comment is wrong. Left alone on purpose: seven open PRs already edit
that surface and this branch is backend Go.
readWavID3Tags stays in wav_test.go for the two cases that are
about the bytes rather than the scan — a tag with every frame
cleared, which no reader reports as anything, and the chunk
preservation test, which parses the container anyway.
## The issue
`backend/tagwriter` writes a WAV's tags into a RIFF `id3 ` chunk, and
`metadata.ExtractTags` is built on `dhowden/tag`, which has **no RIFF
reader at all**. So the app could not read tags it had written itself:
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. WAV is in `SupportedFileExtensions`, so those files scan and play —
they just arrive with whatever the filename fallback gives and stay
that way.
## What changed
| Commit | Issue |
| --- | --- |
| `c56eae2` fix(metadata): read a WAV's tags out of its RIFF id3 chunk | #104 |
**`backend/riff` is a new package**, not a move into either half:
`tagwriter` already imports `metadata`, so reaching back for
`parseRIFF` is an import cycle rather than merely the wrong direction.
`backend/tagtotals` is the precedent for a small package that exists so
two callers need not import each other.
Three things in it are load-bearing:
- **The two readers are deliberately different.** `Parse` (the writer's
half, moved verbatim) holds every chunk in memory, which is what
rewriting a file needs — and a WAV's audio *is* a chunk, so using it
on the scan path would read every WAV in the library in full.
`ID3Chunk` seeks over what it is not looking for, and copies the tag
with `io.CopyN` rather than allocating the size the header declares.
- **The container is asked before `tag.ReadFrom`, not after it fails.**
That library's last resort is an ID3v1 trailer, which a WAV is free
to carry, and it would otherwise outrank the chunk the app wrote.
- **An untagged WAV is a file with no tags, not a file with a
problem.** No chunk, an RF64 container, or a tag with every frame
cleared all read 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 a test that said so.**
`TestWAVTagsAreNotReadableYet` failed the moment the reader learned and
carried in its own comment what to update, so it is deleted,
`TestFixturesMatchManifest` no longer skips `wav`, and
`totals_test.go`'s WAV case now reads through `metadata.ExtractTags`
like the other three formats. That last one is the seam the issue's
"why nothing caught it" names: a round trip asserted through the
writer's own parser is a test of the writer.
## Verification
- **`make test`** — clean, all three build configurations. New cases in
`backend/riff/riff_test.go` (the tag located past an odd-length
chunk, uppercase `ID3 `, no chunk, not RIFF, not WAVE, RF64, two
truncations) and `backend/tagwriter/wav_test.go` (the issue's own
reproduction through `metadata.ExtractTags`, and an untagged WAV
reading empty without a warning).
- **`make lint`** — 0 issues, all three build configurations.
- **Proved non-vacuous.** With the one dispatch line in
`ExtractTagsFromReader` disabled, the new round trip fails with
*every* field zero and the WAV totals case fails on all four numbers
— which is exactly the symptom #104 was filed from.
- **Measured in the running app**, on a rebuilt `default` seed: both
fixture WAVs now import as `Tone A` / `Tone B`, artist
`Field Recordings`, album `Test Tones`, genre `Field Recording`,
2024, with cover art. They used to arrive untitled with no album.
- **`make e2e`** — 239 passed against `make dev-headless SEED=default`
(chromium; WebKit is CI's half). Run because this changes what the
*fixture library* contains, which is what every seeded tier sees and
what CI rebuilds its seed from. Port 34115 was free, so this is not
somebody else's app.
- No `.sql`, no `.templ`, no bound signature: `make generate` and
`make bindings` were not needed (`bindings-check` runs pre-commit and
passed). No frontend change, so `make ui-test` was not run.
## Deliberately not done
- The issue's suggestion to measure how much WAV is realistically in a
library. It would decide the priority, and the priority is already
`Low`.
- **#216** — `riff.Parse` still allocates whatever size a chunk header
claims, which a malformed WAV can make 4 GB. It is the writer's path
only, needs a deliberate tag write, and predates this; the read path
added here does not have that shape. Filed rather than folded in.
- **#217** — `e2e/specs/queue-selection.spec.ts`'s comment names
`01 Tone A` and `02 Tone B` as the fixture library's albumless
tracks, and they are not any more. The spec still passes and its
filter is still needed (two untagged mp3s remain albumless); the
comment is wrong. Left alone on purpose: seven open PRs already edit
that surface and this branch is backend Go.
- `readWavID3Tags` stays in `wav_test.go` for the two cases that are
about the bytes rather than the scan — a tag with every frame
cleared, which no reader reports as anything, and the chunk
preservation test, which parses the container anyway.
Closes #104
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
CI: green — run 553, attempt 2. check and e2e both pass
(e2e covers chromium and WebKit, which is the half I cannot run
locally).
Attempt 1 failed, and not on this change: the Component and store suite step (make ui-test) failed on
FAIL test/components/play-in-context.test.ts > double-clicking a row in the track list
AssertionError: expected [ 12, '/music/track-11.mp3' ] to deeply equal [ 12, '/music/track-3.mp3' ]
which is #138 verbatim — same spec, same assertion, same pair of
paths, a known Reviewed/Confirmed flake at ~1 in 3 full-suite runs
whose lead is track-list's sort preference leaking through localStorage between files. This branch touches no frontend code at
all: the diff is backend/riff, backend/metadata, backend/tagwriter and their tests. e2e was skipped on that attempt
because it needs check, so attempt 2 is the first run that exercised
it here — and it is green.
Noted on #138 as another occurrence, since the rate is the useful thing
about that issue.
**CI: green** — run 553, attempt 2. `check` and `e2e` both pass
(`e2e` covers chromium *and* WebKit, which is the half I cannot run
locally).
Attempt 1 failed, and **not on this change**: the `Component and store
suite` step (`make ui-test`) failed on
```
FAIL test/components/play-in-context.test.ts > double-clicking a row in the track list
AssertionError: expected [ 12, '/music/track-11.mp3' ] to deeply equal [ 12, '/music/track-3.mp3' ]
```
which is **#138** verbatim — same spec, same assertion, same pair of
paths, a known `Reviewed/Confirmed` flake at ~1 in 3 full-suite runs
whose lead is `track-list`'s sort preference leaking through
`localStorage` between files. This branch touches no frontend code at
all: the diff is `backend/riff`, `backend/metadata`,
`backend/tagwriter` and their tests. `e2e` was skipped on that attempt
because it needs `check`, so attempt 2 is the first run that exercised
it here — and it is green.
Noted on #138 as another occurrence, since the rate is the useful thing
about that issue.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The issue
backend/tagwriterwrites a WAV's tags into a RIFFid3chunk, andmetadata.ExtractTagsis built ondhowden/tag, which has no RIFFreader at all. So the app could not read tags it had written itself:
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. WAV is in
SupportedFileExtensions, so those files scan and play —they just arrive with whatever the filename fallback gives and stay
that way.
What changed
c56eae2fix(metadata): read a WAV's tags out of its RIFF id3 chunkbackend/riffis a new package, not a move into either half:tagwriteralready importsmetadata, so reaching back forparseRIFFis an import cycle rather than merely the wrong direction.backend/tagtotalsis the precedent for a small package that exists sotwo callers need not import each other.
Three things in it are load-bearing:
Parse(the writer'shalf, moved verbatim) holds every chunk in memory, which is what
rewriting a file needs — and a WAV's audio is a chunk, so using it
on the scan path would read every WAV in the library in full.
ID3Chunkseeks over what it is not looking for, and copies the tagwith
io.CopyNrather than allocating the size the header declares.tag.ReadFrom, not after it fails.That library's last resort is an ID3v1 trailer, which a WAV is free
to carry, and it would otherwise outrank the chunk the app wrote.
problem. No chunk, an RF64 container, or a tag with every frame
cleared all read as empty metadata with no
TagReadWarning— thescanner's filename fallback is the right answer there, and a warning
would report a fault on a healthy file.
The gap was pinned by a test that said so.
TestWAVTagsAreNotReadableYetfailed the moment the reader learned andcarried in its own comment what to update, so it is deleted,
TestFixturesMatchManifestno longer skipswav, andtotals_test.go's WAV case now reads throughmetadata.ExtractTagslike the other three formats. That last one is the seam the issue's
"why nothing caught it" names: a round trip asserted through the
writer's own parser is a test of the writer.
Verification
make test— clean, all three build configurations. New cases inbackend/riff/riff_test.go(the tag located past an odd-lengthchunk, uppercase
ID3, no chunk, not RIFF, not WAVE, RF64, twotruncations) and
backend/tagwriter/wav_test.go(the issue's ownreproduction through
metadata.ExtractTags, and an untagged WAVreading empty without a warning).
make lint— 0 issues, all three build configurations.ExtractTagsFromReaderdisabled, the new round trip fails withevery field zero and the WAV totals case fails on all four numbers
— which is exactly the symptom #104 was filed from.
defaultseed: bothfixture WAVs now import as
Tone A/Tone B, artistField Recordings, albumTest Tones, genreField Recording,2024, with cover art. They used to arrive untitled with no album.
make e2e— 239 passed againstmake dev-headless SEED=default(chromium; WebKit is CI's half). Run because this changes what the
fixture library contains, which is what every seeded tier sees and
what CI rebuilds its seed from. Port 34115 was free, so this is not
somebody else's app.
.sql, no.templ, no bound signature:make generateandmake bindingswere not needed (bindings-checkruns pre-commit andpassed). No frontend change, so
make ui-testwas not run.Deliberately not done
library. It would decide the priority, and the priority is already
Low.riff.Parsestill allocates whatever size a chunk headerclaims, which a malformed WAV can make 4 GB. It is the writer's path
only, needs a deliberate tag write, and predates this; the read path
added here does not have that shape. Filed rather than folded in.
e2e/specs/queue-selection.spec.ts's comment names01 Tone Aand02 Tone Bas the fixture library's albumlesstracks, and they are not any more. The spec still passes and its
filter is still needed (two untagged mp3s remain albumless); the
comment is wrong. Left alone on purpose: seven open PRs already edit
that surface and this branch is backend Go.
readWavID3Tagsstays inwav_test.gofor the two cases that areabout the bytes rather than the scan — a tag with every frame
cleared, which no reader reports as anything, and the chunk
preservation test, which parses the container anyway.
Closes #104
CI: green — run 553, attempt 2.
checkande2eboth pass(
e2ecovers chromium and WebKit, which is the half I cannot runlocally).
Attempt 1 failed, and not on this change: the
Component and store suitestep (make ui-test) failed onwhich is #138 verbatim — same spec, same assertion, same pair of
paths, a known
Reviewed/Confirmedflake at ~1 in 3 full-suite runswhose lead is
track-list's sort preference leaking throughlocalStoragebetween files. This branch touches no frontend code atall: the diff is
backend/riff,backend/metadata,backend/tagwriterand their tests.e2ewas skipped on that attemptbecause it needs
check, so attempt 2 is the first run that exercisedit here — and it is green.
Noted on #138 as another occurrence, since the rate is the useful thing
about that issue.