From fd3e7459f495f17cd69b6e18295b0fb9d99c29b4 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 17 Aug 2026 20:23:16 +0100 Subject: [PATCH] Fix #4: locate the WAV `data` chunk instead of inferring where it must be `decode_wav_bytes` derived the audio offset by subtraction: header_size = len(wav_bytes) - n_channels * width_bytes * nframes which is only correct when nothing follows the `data` chunk. Plenty follows it: `LIST`/`INFO` tags are routinely appended by ffmpeg, Audacity and iTunes. Those trailing bytes were counted as header, so the read started too far into the buffer and returned audio of the right LENGTH and the wrong CONTENT -- with no exception. For a 10-sample file with a LIST chunk appended: expected [0, 1, -1, 2, -2, 3, -3, 4, -4, 5] actual [0, 20041, 20294, 21321, 21574, 6, 0, 24908, 26230, 14389] Separately, a `data` chunk declaring more bytes than the file carries -- what a stream writer leaves behind when it never patches the length back into the header -- tripped `assert header_size >= 44`, which blamed the header for a problem in the data size. Both are fixed by walking the RIFF chunk list to find `data` rather than inferring its position. A differential sweep over 576 well-formed containers (channels x width x nframes x fmt size x chunks before/after data) shows the new implementation correct on 576/576 where the old was wrong on 336/576, with no case where the old was right and the new is wrong -- so no behaviour change on valid input, which matters: `decode_wav_bytes` has eight consumers in this ecosystem (hum, know, odat, front among them). Two subtleties worth naming, both found by adversarial review of the first cut: - Clamping an over-declared size to end-of-file re-creates the original bug when a trailing chunk is present -- the metadata comes back as extra samples. So an overrunning `data` chunk is bounded by the next position from which the rest of the buffer parses as a chunk list landing exactly on EOF, falling back to EOF only when there is no such boundary. - A short `data` chunk now warns (`ShortWavData`) rather than decoding silently. Clamping is what the issue asked for, but a caller that cannot tell a half-downloaded file from a complete one is no better off than before. Also here: - Sub-frame remainders are dropped rather than raising a struct-size error, and a payload with no whole frame returns an empty waveform instead of surfacing an `IndexError` from inside the chunked decoder. - Malformed input raises `ValueError` saying what is wrong. This unifies three previous outcomes (`AssertionError`, `wave.Error`, `EOFError`); no consumer in this ecosystem catches any of them, but it is a contract change. - `header_size_of_wav_bytes` returns the data offset from the same walk, and loses its `meta` parameter, which it no longer reads (no callers exist). - `comptype` in `decode_wav_header_bytes` is assigned unconditionally; it was bound only inside `if params.comptype == "NONE"`, an unreachable branch that would have left the name unbound if stdlib `wave` ever widened. CI: the pytest step ran with `root-dir: recode`, and the isee action builds its file list with `find -name '*.py'` -- which never reached the repo-root `test_recode.py`. CI collected 25 doctests and zero tests, so the existing `test_decode_wav_bytes` had never gated a merge and neither would a new regression test. Pointed at the repo root it collects 151. Verified on Python 3.10.13 (the CI interpreter) with the exact CI invocation: 151 passed. Every guard is mutation-checked -- seven single-line reversions, including restoring the old subtraction, dropping the frame-size channel factor and clamping an overrun to EOF, each turns the suite red. Known limitation, unchanged by this commit: WAVE_FORMAT_EXTENSIBLE files decode on Python 3.12 but not on 3.10, because `decode_wav_header_bytes` delegates to stdlib `wave`, which only learned that format in 3.12. The chunk walker handles them fine; only the header read fails. --- .github/workflows/ci.yml | 8 +- recode/audio.py | 231 +++++++++++++++++++++++++++++++++---- test_recode.py | 240 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 454 insertions(+), 25 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0206546..993d9da 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,8 +39,12 @@ jobs: - name: Pytest Validation uses: i2mint/isee/actions/pytest-validation@master with: - root-dir: ${{ env.PROJECT_NAME }} - paths-to-ignore: examples,scrap + # Repo root, not the package dir: `test_recode.py` lives at the top level, so + # with root-dir=recode the action's `find` never reached it and the test suite + # had never once run in CI (25 doctests collected, 0 tests). Widening it here + # is what makes the wav-parsing regression guards actually gate a merge. + root-dir: . + paths-to-ignore: notebooks,setup.py publish: name: Publish diff --git a/recode/audio.py b/recode/audio.py index 532cfcc..9bf7268 100644 --- a/recode/audio.py +++ b/recode/audio.py @@ -36,6 +36,8 @@ """ +import struct +import warnings from io import BytesIO from typing import Union from collections.abc import Iterable @@ -112,16 +114,32 @@ def decode_pcm_bytes(pcm_bytes: bytes, width: Width = 2, n_channels: int = 1): return decode(pcm_bytes) -MIN_WAV_N_BYTES = 44 +_RIFF_HEADER_SIZE = 12 # 'RIFF' + form size + 'WAVE' +_CHUNK_HEADER_SIZE = 8 # chunk id + chunk size +_CHUNK_ID_SIZE = 4 + + +class ShortWavData(UserWarning): + """The `data` chunk carries fewer bytes than its own header declares. + + Raised as a warning rather than an error because the audio that *is* present is + still worth decoding -- a partially downloaded file, or one written to a stream + whose length was never patched back into the header. What must not happen is for + the shortfall to pass unmentioned, since the caller cannot otherwise tell a + truncated file from a complete one. + """ def decode_wav_bytes(wav_bytes: bytes): - r""" + r"""Decode WAV bytes into a ``(waveform, sample_rate)`` pair. - :param width: The width of a sample (in bits, bytes, numpy dtype, pyaudio ...) - (Will try to figure it out) - :param n_channels: Number of channels - :return: The decoded waveform + :param wav_bytes: The bytes of a RIFF/WAVE container holding uncompressed PCM + :return: ``(wf, sr)`` -- the decoded waveform and its sample rate + + :raises ValueError: if `wav_bytes` is not a RIFF/WAVE container with a `data` + chunk. (Before recode#4 the same inputs raised `AssertionError`, `wave.Error` + or `EOFError` depending on how they were malformed; they are unified here.) + :raises ShortWavData: *warning*, not an exception -- see below. >>> wav_bytes = ( ... b'RIFF.\x00\x00\x00WAVEfmt \x10\x00\x00\x00\x01\x00\x01\x00' # header @@ -133,30 +151,193 @@ def decode_wav_bytes(wav_bytes: bytes): [0, 1, -1, 2, -2] >>> sr 42 + + The `data` chunk is located by walking the RIFF structure, so chunks that sit + *after* the audio -- `LIST`/`INFO` metadata, which ffmpeg, Audacity and iTunes + all append -- do not shift the waveform: + + >>> import struct + >>> info = b'INFOISFT' + struct.pack('>> with_trailing_metadata = wav_bytes + b'LIST' + struct.pack('>> decode_wav_bytes(with_trailing_metadata)[0] + [0, 1, -1, 2, -2] + + A file carrying less audio than its header declares decodes to the whole frames + that are actually there, and says so: + + >>> truncated = wav_bytes[:-4] + >>> import warnings + >>> with warnings.catch_warnings(record=True) as caught: + ... _ = warnings.simplefilter('always') + ... wf, sr = decode_wav_bytes(truncated) + >>> wf + [0, 1, -1] + >>> caught[0].category.__name__ + 'ShortWavData' """ + offset, size = _wav_data_chunk(wav_bytes) meta = decode_wav_header_bytes(wav_bytes) - header_size = header_size_of_wav_bytes(wav_bytes, meta) + # Decoding is defined on whole frames, and a frame is every channel's sample: a + # file cut mid-frame drops the remainder rather than raising a struct-size error. + frame_size = int(meta["n_channels"] * meta["width_bytes"]) + if frame_size: + size -= size % frame_size + if size // frame_size < meta["nframes"]: + warnings.warn( + f"WAV `data` chunk is short: the header declares {meta['nframes']} " + f"frames, {size // frame_size} are present. Decoding what is there.", + ShortWavData, + stacklevel=2, + ) + if size == 0: + # No whole frame survived. Answering with an empty waveform is the consistent + # reading of "decode the frames that are present"; letting it through would + # surface as an IndexError from inside the chunked decoder instead. + return [], meta["sr"] wf = decode_pcm_bytes( - wav_bytes[header_size:], + wav_bytes[offset : offset + size], width=meta["width_bytes"], n_channels=meta["n_channels"], ) return wf, meta["sr"] -def header_size_of_wav_bytes(wav_bytes: bytes, meta: dict = None): - """Compute the header size""" - if meta is None: - meta = decode_wav_header_bytes(wav_bytes) - # the header tells us how many samples (frames) of data there are, how many - # channels, and how many bytes each sample (frame) takes, so the header size is - # the total size (number of bytes), minus the product of those three quantities - data_size = int(meta["n_channels"] * meta["width_bytes"] * meta["nframes"]) - header_size = len(wav_bytes) - data_size - assert ( - header_size >= MIN_WAV_N_BYTES - ), f"Header size of wav bytes should be at least 44 bytes" - return header_size +def _wav_data_chunk(wav_bytes: bytes) -> tuple: + r"""Locate the audio payload: ``(offset, size)`` of the `data` chunk's contents. + + Walks the RIFF chunk list rather than inferring the position arithmetically, which + is what makes it robust to the shapes real-world WAV files take that a + size-subtraction cannot survive (recode#4): + + - **chunks after `data`.** `LIST`/`INFO` tags are routinely appended by encoders. + Deriving the header size as ``len(wav_bytes) - n_channels * width * nframes`` + silently counts those trailing bytes as header, so the waveform is read from too + far in -- returning audio of the right *length* and the wrong *content*, with no + error raised. + - **an over-declared `data` size.** Files written to a stream (length unknown at + write time, patched afterwards -- or never) declare more samples than they carry, + commonly with the sentinel ``0xFFFFFFFF``. The size is clamped to what is really + there instead of asserting. + + Those two interact, and naively clamping an over-declared size to end-of-file + would re-create the very bug this function exists to kill -- a trailing `LIST` + would be handed back as audio. So when the declared size overruns, the payload is + bounded by the next chunk that the remainder of the file parses cleanly from, + rather than by EOF. Only when no such boundary exists does it fall back to EOF. + + Sizes are read as unsigned little-endian, per the RIFF spec. + + >>> import struct, wave, io + >>> b = io.BytesIO() + >>> with wave.open(b, 'wb') as w: + ... _ = w.setnchannels(1), w.setsampwidth(2), w.setframerate(8000) + ... w.writeframes(struct.pack('<3h', 1, 2, 3)) + >>> raw = b.getvalue() + >>> offset, size = _wav_data_chunk(raw) + >>> size + 6 + >>> raw[offset:offset + size] == struct.pack('<3h', 1, 2, 3) + True + + An over-declared size does not swallow what follows the audio: + + >>> broken = bytearray(raw) + >>> at = broken.find(b'data') + >>> broken[at + 4:at + 8] = struct.pack('>> trailing = b'LIST' + struct.pack('>> _wav_data_chunk(bytes(broken) + trailing) + (44, 6) + """ + if ( + len(wav_bytes) < _RIFF_HEADER_SIZE + or wav_bytes[:4] != b"RIFF" + or wav_bytes[8:12] != b"WAVE" + ): + raise ValueError( + "Not WAV bytes: expected a RIFF/WAVE container, got " + f"{bytes(wav_bytes[:4])!r}...{bytes(wav_bytes[8:12])!r}" + ) + pos = _RIFF_HEADER_SIZE + while pos + _CHUNK_HEADER_SIZE <= len(wav_bytes): + chunk_id = bytes(wav_bytes[pos : pos + _CHUNK_ID_SIZE]) + (declared,) = struct.unpack( + " len(wav_bytes) - contents: + raise ValueError( + f"Not WAV bytes: chunk walk desynced at offset {pos - declared - 8} " + f"({chunk_id!r} declares {declared} bytes but only " + f"{len(wav_bytes) - contents} remain); no `data` chunk reachable" + ) + raise ValueError("Not WAV bytes: no `data` chunk found") + + +def _payload_end_of_overrunning_data(wav_bytes: bytes, contents: int) -> int: + """How many bytes of audio a `data` chunk whose declared size overruns really has. + + The declared size is unusable, so the extent has to come from the file itself: the + audio runs until the next thing that is demonstrably a chunk, meaning a position + (word-aligned, as RIFF requires) from which the rest of the buffer parses as a + well-formed chunk list ending exactly at EOF. Requiring the parse to reach EOF is + what keeps this from firing on audio that merely happens to contain four + plausible-looking bytes. + + Falls back to end-of-file when no such position exists -- a genuinely truncated + file, where reading to the end is right. + """ + end = len(wav_bytes) + start = contents + (contents % 2) + for candidate in range(start, end - _CHUNK_HEADER_SIZE + 1, 2): + if _parses_as_chunk_list_to_eof(wav_bytes, candidate): + return candidate - contents + return end - contents + + +def _parses_as_chunk_list_to_eof(wav_bytes: bytes, pos: int) -> bool: + """Does the buffer from `pos` read as a chunk list that lands exactly on EOF?""" + end = len(wav_bytes) + while pos < end: + if pos + _CHUNK_HEADER_SIZE > end: + return False + chunk_id = wav_bytes[pos : pos + _CHUNK_ID_SIZE] + # A chunk id is four printable ASCII characters; anything else is audio. + if not all(0x20 <= b < 0x7F for b in chunk_id): + return False + (declared,) = struct.unpack( + " int: + r"""Size, in bytes, of everything preceding the audio payload. + + That is the offset of the `data` chunk's contents, found by walking the RIFF + structure (see :func:`_wav_data_chunk`). For a well-formed file with nothing after + the audio this is the same number the old size-subtraction produced; unlike it, it + stays correct when the file carries trailing metadata or an over-declared `data` + size. + + `meta` is accepted for backwards compatibility and no longer used. + + >>> header_size_of_wav_bytes( + ... b'RIFF.\x00\x00\x00WAVEfmt \x10\x00\x00\x00\x01\x00\x01\x00' + ... b'*\x00\x00\x00T\x00\x00\x00\x02\x00\x10\x00data\n\x00\x00\x00' + ... b'\x00\x00\x01\x00\xff\xff\x02\x00\xfe\xff' + ... ) + 44 + """ + offset, _ = _wav_data_chunk(wav_bytes) + return offset # # TODO: Repair. See https://github.com/otosense/recode/issues/3 @@ -389,8 +570,12 @@ def decode_wav_header_bytes(wav_header_bytes: bytes) -> dict: """ wav_read_obj = Wave_read(BytesIO(wav_header_bytes)) params = wav_read_obj.getparams() - if params.comptype == "NONE": # it's the only one supported - comptype = None # but we're making it compatible with encoding anyway + # Normalized to None so the value round-trips with `encode_wav_header_bytes`. + # Unconditional on purpose: `Wave_read` rejects any non-PCM fmt tag with + # `wave.Error` before `getparams()` returns, so comptype is always 'NONE' here. + # It used to be assigned inside `if params.comptype == "NONE"`, an unreachable + # guard that would have left the name unbound if that ever changed. + comptype = None return dict( sr=params.framerate, width_bytes=params.sampwidth, diff --git a/test_recode.py b/test_recode.py index e11d6b5..80269b9 100644 --- a/test_recode.py +++ b/test_recode.py @@ -270,3 +270,243 @@ def test_decode_wav_bytes(): # mk_test_wav_file() + + +# ------ recode#4: the `data` chunk must be located, not inferred ----------------------- +# +# `decode_wav_bytes` used to derive the header size by subtraction: +# +# header_size = len(wav_bytes) - n_channels * width_bytes * nframes +# +# which reads any bytes that follow the audio as though they were header. The failure is +# silent -- the waveform comes back the right LENGTH and the wrong CONTENT -- so these +# tests assert decoded samples, never just a length or a "did not raise". +# +# Each case below is a real shape a WAV file takes in the wild, and each one is read +# correctly by `soundfile`, which is how the original report noticed the discrepancy. + +import io +import struct +import warnings +import wave as _wave + +from recode.audio import ShortWavData + +_WF = [0, 1, -1, 2, -2, 3, -3, 4, -4, 5] +_SR = 44100 + + +_STRUCT_CODE_FOR_WIDTH = {1: "b", 2: "h", 4: "i"} + + +def _wav(wf=_WF, sr=_SR, n_channels=1, width=2): + """A minimal, well-formed PCM WAV, built with the stdlib only. + + `width` drives the packing as well as the header, so a non-default width yields a + valid file rather than a header that disagrees with its own payload. + """ + code = _STRUCT_CODE_FOR_WIDTH[width] + b = io.BytesIO() + with _wave.open(b, "wb") as w: + w.setnchannels(n_channels) + w.setsampwidth(width) + w.setframerate(sr) + w.writeframes(b"".join(struct.pack("<" + code, x) for x in wf)) + return b.getvalue() + + +def _with_riff_size_fixed(raw): + raw = bytearray(raw) + raw[4:8] = struct.pack("= 44` and raised. `0xFFFFFFFF` is + the sentinel a stream writer leaves behind when it never learns the length. + """ + raw = bytearray(_wav()) + at = raw.find(b"data") + (declared,) = struct.unpack("