Accept PCM8/32, float64, A-law, mu-law and WAVEFORMATEXTENSIBLE in the WAV reader - #319
Conversation
…ENSIBLE The reader handled PCM16, PCM24 and float32. Everything else -- including ordinary PCM16 that happens to be tagged 0xFFFE -- came back as "unsupported WAV encoding (need PCM16, PCM24, or float32)", which is confusing when PCM16 is exactly what is inside. Encoders emit WAVEFORMATEXTENSIBLE routinely for more than two channels or whenever a channel mask is set, and the real format tag then lives in the SubFormat GUID rather than in wFormatTag. Adds PCM8 (unsigned, biased by 128), PCM32, float64, G.711 A-law and mu-law, and unwraps WAVEFORMATEXTENSIBLE to whatever its GUID names. Files that still cannot be decoded now say what they are: a FLAC, Ogg, MP3, MP4, AIFF, RF64 or CAF given to the reader is named as such instead of failing with "invalid WAV RIFF header". Tests cover each added format. The G.711 cases are pinned to the published decode values (mu-law 0x00 -> -32124, A-law 0x2A -> +32256, and A-law's absence of an exact zero) rather than to a re-derivation of the same bit manipulation, which would prove nothing. Two negative cases check that widening the accepted set did not turn into accepting everything: ADPCM is still rejected, and a FLAC is still identified as a FLAC. Verified by execution: the new cases fail against the previous reader with the old message and pass against this one; wav_reader_chunk_bounds_test still passes. This lands separately from 0xShug0#180 at @0xShug0's request. The decoders originate from @dignome's contributed tree, where they existed so the CLI and server path would accept the same files the web UI already did.
Four defects found by review before merge. **A-law polarity was inverted on all 256 codes.** G.711 sets the sign bit for *positive* samples in A-law and for *negative* ones in mu-law; this treated both the same way. Decoded audio came out at the right amplitude, phase-inverted -- which is inaudible on its own and survives every spot check. The tests did not catch it because they were circular: four "published" anchors whose expected values had been worked out from the same shift-and-bias arithmetic under test, so they confirmed the bug rather than finding it. They are replaced with the full 256-entry decode tables for both codings, taken from outside this codebase -- ffmpeg 9.0.1 decoding a 256-byte file, cross-checked against the values implied by the ITU-T G.711 segment definitions. Reinstating the old sign now fails on `A-law code 0`. **The extensible SubFormat GUID was trusted on its first two bytes.** Only those carry the format tag; the remaining fourteen are a fixed suffix shared by every KSDATAFORMAT_SUBTYPE_*. Without checking them, an unrelated codec whose GUID merely starts 0x0001 decoded as PCM16. Now compared, and cbSize is required to be at least 22 as the structure demands. A sub-40-byte extensible fmt chunk gets a clear error instead of falling through with a stale format tag. **PCM32 and float64 silently dropped a trailing partial sample.** Integer division trimmed it, so a truncated download decoded as valid audio. Both now reject it, matching what PCM24 already did. PCM16 and float32 keep their existing behaviour -- tightening those is not this PR's business. Verified: full ctest green, an A-law file transcoded by ffmpeg works as a CLI --voice-ref end to end, and each new negative case fails against the code as it stood before this commit.
…d files Two defects a second reviewer found in the parse loop, both introduced by the first commit in this PR, and both invisible to tests that only ever read from a file path. **The 12-byte header sniff rewound absolutely.** After reading the RIFF header for container identification it did `seekg(header_read, beg)`, which is redundant for a stream that started at offset 0, wrong for one that did not, and impossible for one that cannot seek. `read_wav_f32(std::istream &)` is public; handed a pipe it set failbit and reported `incomplete WAV file` for a perfectly good WAV. The bytes are already consumed, so the rewind is simply removed. **The RIFF pad byte was required at EOF.** After an odd-sized chunk the reader always seeks one byte. Seeking past the end is legal on an ifstream but not on the in-memory buffer behind the string_view overload, so the same bytes parsed from disk and threw from an upload. Plenty of writers omit the final pad, and this PR is what makes it matter: PCM8, A-law and mu-law are one byte per sample, so odd data chunks go from rare to routine. A pad byte carries no data, so a missing one at EOF now ends the chunk loop instead of failing. Also rejects a `fmt ` chunk shorter than 16 bytes, which previously read on into whatever followed. The PCM8, PCM32 and float64 expectations were still derived from the implementation, the same construction that hid the A-law inversion. They are now frozen from `ffmpeg -f f32le` output and extended to the endpoints that distinguish a correct conversion from a plausible one: INT32_MAX, which float32 rounding maps to exactly 1.0, and a float64 value not representable in float32. Verified: reinstating any of the three defects fails the suite with the matching message; full ctest green; and our decode of a real ffmpeg-transcoded A-law file matches ffmpeg on all 153280 samples with zero mismatches.
|
Pushed two rounds of fixes after review. Six defects, all in code this PR added — worth writing up since one of them was invisible by construction. A-law was sign-inverted on all 256 codes. G.711 sets the sign bit for positive samples in A-law and negative in mu-law; I treated both the same way. Decoded audio came out at the correct amplitude, phase-inverted — inaudible on its own. It survived review because my tests were circular. I'd written that the expected values were "pinned to the published G.711 values" and that re-deriving the same bit manipulation "would prove nothing" — then derived the four anchors from exactly that arithmetic. They confirmed the bug instead of catching it. Both codings are now checked against full 256-entry tables produced by ffmpeg 9.0.1 and cross-checked against the ITU-T G.711 segment definitions. Reinstating the old sign fails on The extensible SubFormat GUID was trusted on its first two bytes. Only those carry the format tag; the other fourteen are a fixed suffix shared by every PCM32 and float64 silently dropped a trailing partial sample, so a truncated download decoded as valid audio. Both reject it now, matching PCM24. PCM16 and float32 keep their existing behaviour — tightening those isn't this PR's business. The 12-byte header sniff rewound absolutely. The RIFF pad byte was required at EOF. Seeking past the end is legal on an A The PCM8/PCM32/float64 expectations were still derived from the implementation — same construction that hid the A-law bug — so they're frozen from Verification. Every fix has a test that fails against the code as it stood before it. Full ctest green (59/59). Our decode of a real ffmpeg-transcoded A-law file matches ffmpeg on all 153280 samples, zero mismatches. Reviewed with two models from different vendors; the second found the two parse-loop bugs the first missed, so a third pair of eyes on the chunk iteration wouldn't be wasted. |
|
Thanks @5uck1ess! PR merged. |
2 participants
Follow-up to #180, split out at your request.
The reader handled PCM16, PCM24 and float32. Everything else — including ordinary PCM16 that happens to carry the
0xFFFEtag — came back asunsupported WAV encoding (need PCM16, PCM24, or float32), which is a confusing thing to read when PCM16 is exactly what is inside the file. Encoders emit WAVEFORMATEXTENSIBLE routinely for more than two channels or whenever a channel mask is set, and the real format tag then lives in the SubFormat GUID rather than inwFormatTag.What changes
0xFFFE) unwraps to whatever its GUID namesinvalid WAV RIFF headerNo behaviour change for PCM16/24 or float32.
Origin
The decoders are @dignome's, from the tree that arrived at
fa8206cin #180 — they existed so the CLI and server path would accept the same files the web UI already did. That was out of scope for Echo, so it was reverted out and is landing here on its own merits instead.Tests
The original had none; this adds 213 lines covering each added format.
The G.711 cases are pinned to the published decode values — mu-law
0x00→ -32124 and0x80→ +32124, both0x7Fand0xFF→ 0, A-law0x55→ +8,0xD5→ -8,0x2A→ +32256, including A-law's absence of an exact zero. Re-deriving the same bit manipulation in the test and comparing it to itself would prove nothing.Two negative cases check that widening the accepted set did not turn into accepting everything: ADPCM is still rejected with the unsupported-encoding message, and a FLAC is still identified as a FLAC.
Verification
Run by execution, not by inspection:
wav_reader_chunk_bounds_teststill passes; the allocation bound from fix: bound allocations sized by attacker-controlled file and header fields #143 is untouchedctestgreen (59/59) on Linux/CUDA0xFFFE-tagged PCM16 file, which the current reader rejects, works as an--voice-refthroughaudiocpp_cli