Fix ESP32 ROM miniz symbol collision; report FLAC OOM correctly - #179
Closed
rtissera wants to merge 33 commits into
Closed
Fix ESP32 ROM miniz symbol collision; report FLAC OOM correctly#179rtissera wants to merge 33 commits into
rtissera wants to merge 33 commits into
Conversation
chd_get_metadata()'s faux hard-disk metadata snprintf() passed uint32_t header fields against a %d format string (the paired sscanf use already took int* correctly) - harmless on LP64 desktop builds but a real -Werror=format= build failure on ILP32 targets. Cast at the call site; the on-disk MAME metadata text format is untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Espressif's ROMs bake in an older miniz and export its tinfl entry points
from the target's ROM linker script as *absolute* symbols (see the "Group
miniz" block in esp_rom/<target>/ld/<target>.rom.ld, e.g.
tinfl_decompress = 0x4fc000f8). A linker-script assignment outranks an
ordinary object definition, so an ESP-IDF link silently bound those names
to ROM and dropped the copies compiled from deps/miniz-3.1.2/miniz.c -
even though both were present in the archive.
The result was a split decoder: mz_inflateInit2()/mz_inflate() from miniz
3.1.2 built and interpreted a 3.1.2-layout tinfl_decompressor, then handed
it to a ROM tinfl_decompress() laying that struct out differently (miniz
3.0 reworked the Huffman tables from tinfl_huff_table m_tables[3] to the
flattened m_look_up/m_tree_N form, changing field offsets and total size).
The ROM decoder overran the smaller m_decomp into the enclosing
inflate_state, corrupting m_window_bits - which sits just before
m_dict[32768]. mz_inflate() then saw m_window_bits > 0, set
TINFL_FLAG_PARSE_ZLIB_HEADER on a raw-deflate stream opened with
inflateInit2(..., -MAX_WBITS), consumed exactly 2 bytes on the CMF/FLG
check and returned MZ_DATA_ERROR.
It surfaced as CHDERR_DECOMPRESSION_ERROR on real hardware only, and
looked indistinguishable from corrupt input or a silicon/codegen bug:
desktop x86-64, x86-32, vanilla RV32 GCC under QEMU and Espressif's own
GCC 14.2.0 freestanding under QEMU all passed, because none of them links
ESP-IDF's ROM linker scripts. CONFIG_HEAP_POISONING_COMPREHENSIVE was
blind to it too - the corruption is intra-block, so it never reaches a
canary.
Six names collide: tinfl_decompress, tinfl_decompress_mem_to_{heap,mem,
callback}, mz_adler32 and mz_free. mz_free matters independently of the
decoder mismatch: bound to ROM it would hand ESP-IDF-heap pointers to the
ROM allocator.
Renames are applied as compile definitions to whichever target compiles
miniz.c, and are a no-op off ESP-IDF. Kept in cmake/ rather than patched
into deps/miniz-3.1.2/miniz.h: that tree is vendored verbatim so it can be
re-synced from upstream, and an edit there would be silently dropped by
the next miniz bump - resurrecting this bug with no diff to point at. Only
miniz.c references these names, so target-scoped defines suffice.
Measured on a Waveshare ESP32-P4-NANO (chip rev v3.1, ESP-IDF v5.5.5)
against a 128-file CHD corpus on SD plus a 24-file flash corpus:
16 decompression errors -> 0; flash 17/24 -> 20/24 files (4.78 -> 10.63 MB
decoded); SD 18/128 -> 31/128.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
zlib_codec_decompress()'s diagnostic dump previously printed on every call. Over a 128-file real corpus that is tens of KB of blocking printf() per hunk, and it destabilized a full run on its own - so the dump is now emitted only on the failure path, where it costs nothing until something actually breaks. Also tag which stage failed. cd_codec_decompress() (shared by cdzl/cdlz/ cdzs) reports base vs subcode. Without that, a CD codec failure only says "decompression error" and gives no way to tell a main-sector-data problem from a subcode one - which is what separated two independent bugs during the ESP32-P4 investigation: 13 cdzl failures were all stage=base, and the remaining CD-FLAC ones turned out not to be a zlib problem at all. The failure dump prints zerr, total_out, avail_in/avail_out and the full compressed input as hex. avail_in is the useful one: every failure in that investigation consumed exactly 2 bytes with total_out=0, which is the signature of the zlib-header check firing on a raw-deflate stream rather than a genuine decode failure. That single number is what identified the root cause after several days of repros that had been looking elsewhere. Off by default; no effect on a normal build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
flac_decoder_reset() calls drflac_open_with_metadata() once per hunk, which allocates a fresh decoder plus a decoded-sample buffer sized from STREAMINFO - about 40KB for a CD-FLAC hunk - and frees it again on the next hunk. On a small-RAM target that allocation, not the stream, is the most likely thing to fail there. It was reported as CHDERR_DECOMPRESSION_ERROR, which is indistinguishable from genuinely corrupt audio data. Route drflac through allocation callbacks that record a failure, so cdfl/flac can return CHDERR_OUT_OF_MEMORY instead. drflac copies the callbacks struct into the drflac object by value, so passing a local is safe. The decode call is covered as well as the reset: alloc_failed is cleared by the preceding reset(), so it can only be set there by an allocation the decode itself attempted. Found on an ESP32-P4-NANO, where 3 pcenginecd titles failed at their first CD-FLAC hunk and looked like a second instance of an unrelated zlib bug that was under investigation at the time. They are exactly the three largest files in that corpus by hunk count (11055, 11271, 11825) while the largest passing one is 7312 - a monotone boundary, i.e. a headroom wall rather than a data-dependent decode fault. Each hunk of rawmap costs 12 bytes, so ~142KB at 11825 hunks, and past that the 40KB no longer fits. All three decode fully on desktop in both LP64 and ILP32. Confirmed on hardware after this change: alloc_failed=1 on all three. Reusing one drflac instance across hunks rather than rebuilding it every time would remove the failure and the per-hunk malloc/free churn; not attempted here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
An ESP-IDF app that actually flashes and runs on a Waveshare ESP32-P4-NANO, rather than the compile-and-link smoke test already in contrib/esp32p4. It runs two corpora back to back: the synthetic one embedded in flash, and every *.chd found on an SD card, read through the real FATFS/SDMMC stack - the actual deployment path for this board, and the thing that distinguishes a genuine decode bug from a read-path one. Every hunk is poison-filled before the read, so a decode that silently writes nothing is visible rather than passing. Failures print the codec stage and the heap state at the point of failure. Notes worth keeping for anyone repeating this: - Chip rev v3.1 requires ESP-IDF v5.5.3+ or v6.0+; earlier bootloaders reject the part outright. - The onboard TF slot is native SDMMC slot 0, CLK=GPIO43, CMD=GPIO44, D0-D3=GPIO39-42, powered from on-chip LDO channel 4. - ESP-IDF's bundled FatFs has FF_LBA64=0, which disables GPT parsing entirely, and no exFAT support at all. The card must be MBR + FAT32. - chd_open_core_file_callbacks() takes ownership of the file handle on every failure path past its first malloc, so the caller must not close it again. Doing so is harmless against an in-memory backend but is a real double-free of the FATFS file object on SD, which corrupts a FreeRTOS queue inside the VFS layer and resets the board. - The SD sweep caps at 600 hunks per file. Uncapped runs took hours once files started decoding in full instead of bailing out at hunk 0; a cap can only hide failures past that point, and the failures this was built to find were all at hunks 0-272. README.md carries the measured results, the throughput table, and full write-ups of the two bugs this benchmark found: the ESP ROM miniz symbol collision, and drflac being reallocated once per hunk. It also documents the AVHuff RAM wall, which is unchanged and still argues for the streaming-decode redesign. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Every run before this used LOWRAM_TARGET=0, which made the board look far more limited than it is. With the eager map, 76 of 128 SD files failed at chd_open() with CHDERR_OUT_OF_MEMORY, plus 3 CD-FLAC and 2 AVHuff failures further in. The boundary was sharp: the largest file that opened had a 164KB rawmap, the smallest that failed needed 176KB, against 582KB free / 524KB largest block. Sizes ran to a 3.1MB rawmap, which cannot fit in this part's SRAM at all under an eager map. LOWRAM_TARGET=1 takes all of that to zero - lazy CHDv5 map decode, `compressed` grown on demand instead of to the worst case, and codec init() deferred until a hunk needs that slot. The last one also clears the AVHuff and CD-FLAC failures as a side effect: a CHDv5 header lists up to 4 candidate codecs and an eager build pays for all of them at once, so deferring means AVHuff's ~500KB working set and drflac's ~40KB per-hunk buffer no longer compete with dictionaries that hunk never uses. Flash corpus 20/24 -> 24/24 (10.63 -> 14.16 MB decoded). SD corpus 31/128 -> 112/128, which is every valid file on the card: the remaining 16 are broken source files, verified as 13 at size 0 and 3 whose header mapoffset points past their own EOF, and they fail identically on desktop. Zero decompression errors, zero OOM, one reset (power-on). The cost is not measurable here. Across the 20 flash-corpus files that pass in both configurations - uncapped, identical content - total decode time is 3826ms vs 3822ms, or 1.00x. The three real CD titles, where the measurement is least noisy at 762-1490ms each, are 1.00x individually. Per-file spread stays within about +/-10% and is confined to sub-20ms files where noise dominates. README rewritten accordingly: the AVHuff and CD-FLAC sections previously described those as blockers and are now scoped to "why this is tight even when it works". The drflac per-hunk reallocation is still a real inefficiency worth fixing on its own merits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Owner
Author
Update:
|
| flash corpus | SD corpus | decompression errors | OOM | |
|---|---|---|---|---|
| baseline | 17/24, 4.78 MB out | 18/128 | 16 | 81 |
| + ROM-collision fix | 20/24, 10.63 MB out | 31/128 | 0 | 81 |
+ LOWRAM_TARGET=1 |
24/24, 14.16 MB out | 112/128 | 0 | 0 |
112/128 is every valid file on the card. The remaining 16 are broken source files — 13 at size 0, 3 whose header mapoffset points past their own EOF — and they fail identically on desktop x86-64, with libchdr returning the correct error.
What LOWRAM_TARGET=0 looked like: 76/128 failed at chd_open(), plus 3 CD-FLAC and 2 AVHuff further in. Sharp boundary — largest file that opened had a 164KB rawmap, smallest that failed needed 176KB, against 582KB free / 524KB largest block. Rawmap is 12 bytes/hunk and the corpus ran to a 3.1MB rawmap, which cannot fit in this part's SRAM under an eager map at all.
Deferring codec init() per slot is why this also clears the AVHuff and CD-FLAC failures: a CHDv5 header lists up to 4 candidate codecs and an eager build allocates all of them at once, so AVHuff's ~500KB working set and drflac's ~40KB per-hunk buffer stop competing with dictionaries the hunk never touches.
Cost is not measurable. Across the 20 flash-corpus files passing in both configurations — uncapped, identical content — total decode time is 3826 ms vs 3822 ms, i.e. 1.00x. The three real CD titles (762–1490 ms each, where measurement is least noisy) are 1.00x individually. Per-file spread stays within ~±10% and only on sub-20 ms files.
Consequences for this PR:
- The AVHuff and CD-FLAC sections of the README previously described blockers; they are now scoped to "why this is tight even when it works". The AVHuff streaming redesign is a headroom/throughput improvement, not a blocker.
- The drflac-per-hunk reallocation fixed in
351ac80no longer causes failures here, but the inefficiency (~40 KB malloc+free every hunk) and the error-reporting fix both stand on their own merits. - Worth considering whether
LOWRAM_TARGETshould default on for embedded targets generally, rather than being opt-in.
huffman_build_lookup_table()'s LOWRAM_TARGET two-level path allocates a per-prefix subtable for each code longer than l1bits, tracking them with decoder->subtable_count. It rebuilds the whole lookup from scratch on entry, so the arena starts empty - but the count was never reset. A decoder is created once per codec instance (huff_codec_init) and reused for every hunk, so the count grew monotonically across hunks. Once it passed the 2048 guard, every subsequent huffman hunk failed with HUFFERR_TOO_MANY_CONTEXTS, surfacing as CHDERR_DECOMPRESSION_ERROR. The failure is history-dependent, which made it look like data corruption rather than a counter bug: byte-identical compressed input decoded correctly or failed depending only on how many huffman-coded hunks had been read before it. Reading the failing hunk on its own succeeded; reading it after a full sequential walk did not. The map entry, the compressed bytes, the block CRC and the codec pointer were all verified identical between the passing and failing cases, and ASan/UBSan were clean, which ruled out the map, the I/O path and memory corruption. Only reachable with LOWRAM_TARGET=1 - the non-LOWRAM build uses a single full 2^maxbits table with no subtables - and only on CHDs that actually use CHD_CODEC_HUFFMAN, which is why it went unnoticed: huffman is rare in real content (0.2% of hunks in the file it was found on). Also fixes a memory regression in the same path. The arena grew by realloc on every hunk and was only freed at codec teardown, reaching 2048 * 64 entries * 2 bytes = 256KB before the guard fired - a significant leak in the configuration whose entire purpose is to save memory. Measured on x86-64: peak RSS drops ~100-400KB over a full read, with decode time unchanged (2.23/2.26/2.27s before, 2.29/2.26/2.24s after). Found on an ESP32-P4 against a real MAME disk CHD (kinst2.chd, 111737 hunks, 57.3% self-referencing, huffman for 0.2% of hunks), reproducing identically on desktop x86-64. Before: decompression error at hunk 20939. After: all 111737 hunks decode, and kinst.chd's 32002 likewise. All 14 files of the ESP32-P4 characterized sample pass under LOWRAM_TARGET=1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Owner
Author
Correctness hazard found in
|
A self-referencing hunk means "identical to hunk N", and the read path
recursed into a full re-read *and* re-decode of hunk N every time. There
was no decoded-hunk cache anywhere in libchdr.
Measured across a 14-CHD sample (a 289-file corpus characterized first,
then sampled to cover every geometry, codec and self-reference density
present): 100% of self-references point backwards. On FAT-backed storage
that means each one also forces a backward seek, which restarts the
filesystem's cluster-chain walk.
Sized from a simulation of the real access order rather than guessed. A
single entry already captures 97-100% of self-references wherever they
are clustered at all (kinst2 97.2%, Sensible Soccer 97.3%, Ikaruga and
gds-0019 100%), and further entries buy tenths of a percent: kinst2 goes
97.2% -> 97.8% for 64x the memory. Only one file in the corpus rewards a
deep cache (simpbowl, 23.1% -> 88.8% at 64 entries).
The budget is therefore expressed in bytes, not entries, because
hunkbytes varies 8x across real content - 2448 for a raw-sector CD image,
4096 for a hard disk, 19584 for a normal CD. A fixed entry count would
mean 4KB on one file and 1.2MB on another; 64 entries of CD hunks is
1224KB, more than twice the largest free block on the target this was
developed against. Entries = max(1, budget / hunkbytes), and
LOWRAM_TARGET gets a single entry.
Allocated lazily on the first self-reference actually encountered, so
CHDs with none - 3 of the 14 sampled - never pay for it.
Measured on ESP32-P4, whole-file decode, versus the same build without
the cache. The speedup tracks self-reference density, and the two files
with no self-references come in at exactly 1.00x:
self-refs speedup
0.0% (x2) 1.00x
1.4% 1.02x
4.1% 1.05x
7.0% 1.10x
7.6% 1.12x
7.8% 1.07x
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
cdfl costs about 3.6x cdlz per hunk at identical geometry - measured on the synthetic corpus, 10 hunks of 19584 bytes each, RAM-sourced so there is no I/O in the number: cdfl 5.087 ms/hunk vs cdlz 2.303, or 3.869 vs 1.085 once cd_none's 1.218 ms of memcpy/CRC/map overhead is subtracted. That is the opposite of what raw FLAC versus LZMA decode cost predicts, so the natural suspicion was libchdr's own plumbing: flac_decoder_reset() calls drflac_open_with_metadata() once per hunk, tearing down and rebuilding the whole decoder including a ~40KB allocation and a STREAMINFO reparse, where cdlz simply reuses its LZMA state. This splits the three stages so the question can be answered with a measurement instead of a plausible story. On ESP32-P4 it says the suspicion was wrong: reset 0.7 - 7.9% (~0.03 ms/hunk) decode 58.6 - 95.6% (0.245 - 3.622 ms/hunk) subcode 3.7 - 33.5% (~0.14 ms/hunk) The per-hunk rebuild is about 1% of cdfl's time. The cost is the audio decode itself, so reusing the drflac instance is a memory optimisation - worth doing, since that 40KB per-hunk allocation is what produced CHDERR_OUT_OF_MEMORY on the three largest pcenginecd titles - but not a throughput one. Off by default and free when disabled: the macros compile to nothing. The host supplies the clock via chdr_prof_now_us(), so the library keeps no platform dependency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Turns the benchmark from "how fast is it" into "where does the time go", after the first uncapped run appeared to hang and there was no way to tell a stall from a crawl. Attribution. The storage callbacks are the only path from the decoder to the card, so timing inside them splits each hunk's wall time into waiting on SD versus everything else, with no instrumentation inside libchdr. That immediately showed seek time equalling or exceeding read time on every large file - on one, 318s of seek against 185s of read. FATFS fast seek. FatFs walks the FAT cluster chain on every f_lseek and restarts from the first cluster on any *backward* seek. Every COMPRESSION_SELF reference is backward (measured: 100%, all 14 sample files), so a self-reference-heavy CHD pays a full chain walk per hunk. On a 271328-hunk, 29.2%-self-ref file this took reads from 2.7 to 85 ms/hunk and made the run look hung. CONFIG_FATFS_USE_FASTSEEK builds a cluster link map table and makes lseek O(1): in the same 14 minutes the run then reached hunk 245000 instead of 25000, about 10x. Every file on the card is a single fragment, so the default 64-word table is roughly 20x larger than needed here - but note ESP-IDF silently falls back to the slow path when a file needs more, with no error and no log line, so a fragmented card would quietly get nothing. Seek elision was implemented and measured alongside, and does nothing (23.9/85.2/37.6 vs 23.9/84.9/37.4 ms/hunk) because the backward jumps target genuinely different offsets. Kept behind a default-off flag rather than carrying a change that buys zero. Latency as a distribution, not a mean. A mean hunk time says nothing about whether a CD read glitches audio, so hunk times accumulate into a log-scale histogram and report p50/p95/p99/max. Pass B measures request granularity. libchdr reads whole hunks only - there is no sub-hunk API and no decoded-hunk cache - so a sub-hunk request costs a full hunk decode. Reported as read amplification, since "latency versus request size" is a flat line: at 8 units per hunk a 1-unit read moves 8x the bytes and takes the same time as 8 units. Unaligned requests are worse than they look - an unaligned 8-unit read touches 2 hunks. Random versus sequential access is measured beside it, quantifying v5_resume_cache for the first time at 1.8-2.6x. Heap accounting. An earlier version used heap_caps_get_minimum_free_size(), which is the low-water mark *since boot*: it never rises again, so after the first corpus file every delta read as zero and the measurement was silently useless. Free-size deltas instead, sampled during the read loop. With LOWRAM_TARGET=1 opening a 271328-hunk CHD costs 5KB and libchdr's resident footprint is 5-7KB regardless of file size. Config, from a cumulative lever sweep on a 4-file subset: -O2 is worth 1.28x on pure decode and 1.10x end-to-end from SD, heap poisoning off adds 1.01x, and raising the SD clock from 20 to 40MHz adds only 1.05x. Poisoning was a diagnostic for the ROM-miniz collision and is no longer needed. Also adds BENCH_ONE_FILE, BENCH_HUNK_CAP, BENCH_PROGRESS_EVERY and BENCH_FSINFO. The progress counter matters: without it a stalled run and a run that is merely 30x slower than it started are indistinguishable, and that distinction is what identified the seek problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Three sections were stating conclusions that later measurement contradicted. FLAC. The section claimed the per-hunk drflac rebuild explained cdfl's 3.6x cost over cdlz. CHDR_PROFILE_CDFL says the rebuild is 0.7-7.9% and the decode is 58.6-95.6%, so it is a memory problem, not a throughput one. Rewritten, and kept as a worked example of why: the rebuild is real, obviously wasteful, and sits in the hot path, which makes the wrong conclusion easy to reach. LOWRAM_TARGET. The section presented it as an unqualified win. It is the right default, but it selects a two-level huffman lookup table the default build never uses, and that path had a correctness bug and a 256KB leak. Also replaces the RAM projection - which was extrapolated from the eager-map failure boundary - with measured numbers: opening a 271328-hunk CHD costs 5KB and libchdr's resident footprint is 5-7KB regardless of file size, so hunk count no longer drives RAM at all. Adds a storage section, because that is where the time actually goes: 16.1x slower than x86-64 end to end but only 6.0-8.6x on decode alone, the FATFS fast-seek result (~10x, with the silent-fallback caveat), read amplification (a 1-unit read costs 8x the bytes and the same latency as a whole hunk), and random-vs-sequential at 1.8-2.6x. Both near-misses are recorded rather than tidied away: the 600-hunk cap that made the sweep tractable is exactly what hid a failure occurring 20000 hunks in, and the first heap instrumentation used heap_caps_get_minimum_free_size(), a since-boot low-water mark, so it reported a plausible-looking 0 KB instead of an obvious error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
The progress-print block still referenced heap_before and heap_caps_get_minimum_free_size(), both replaced when the heap instrumentation was corrected to use free-size deltas. The block is inside #if BENCH_PROGRESS_EVERY and the run that exercised the new accounting used BENCH_PROGRESS_EVERY=0, so it compiled out and the breakage shipped unnoticed. Builds verified in both configurations this time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
libchdr issues one seek plus one read per hunk, and compressed hunks are small - measured across a 14-CHD sample, 426 to 15223 bytes, a few KB typical - so a large title costs hundreds of thousands of transactions. Their fixed per-call cost (VFS dispatch, filesystem bookkeeping, controller command setup, DMA, interrupt) has nothing to do with their size. On an ESP32-P4 reading from SD that showed up as under 15% of the available bus bandwidth, with I/O accounting for 10-85% of wall time depending on the file. This is only worth doing because hunk payloads are laid out strictly sequentially. Checked before building anything: across that sample, 100% of hunks that touch the file begin exactly where the previous one ended, in a single contiguous run spanning the whole file, on every one of the 14. So one larger read serves many hunks. Off unless the caller sets a budget. chd_set_cache_budget() takes bytes; 0 is the default and reproduces the previous behaviour exactly. The library deliberately does not size this itself - how much memory is available is a property of the embedding system (a desktop, an ESP32 with or without PSRAM, an RP2350) and not something a library can portably discover. Trying to discover it in-library is how the ESP ROM symbol collision fixed earlier in this branch happened. Two details matter for it to be a pure win rather than a trade: A miss keeps whatever it already holds at or after the requested offset, slides it to the front, and reads only what is genuinely new. Without that a window refill re-reads bytes it already had. Only a forward-progressing miss refills. A backward read - in practice a COMPRESSION_SELF reference reaching back to an earlier hunk - is served directly and leaves the window intact, so an excursion cannot discard data prefetched for the sequential stream it is about to return to. Without this, files with many self-references transferred up to 44% more bytes than they needed. Uncompressed hunks consume the window too. They share the same sequential layout, and reading them directly while the window had already prefetched their bytes cost kinst2 (9.3% uncompressed) an extra 9.5% of transfer. Measured, LOWRAM_TARGET=1, transactions issued for a full decode, with bytes transferred unchanged from the uncached case in every configuration: Insanity 11474 -> 472 (24x fewer, 87.9MB either way) kinst2 55058 -> 14580 (3.8x, 154.4 -> 154.7MB) Shadowrun 15876 -> 3915 (4.1x, 167.7MB either way) kinst2 gains least because ~7300 of its baseline reads are LOWRAM lazy-map window reads, which bypass this path entirely; the map is also sequential, so the same treatment would apply, but that is a separate change. Note this trades a higher worst-case single-read latency (a refill transfers the window, not one hunk) for far fewer reads. Which way p99 moves on real storage is not yet measured; the budget is the knob for callers that care. Verified: decoded output byte-identical across budgets 0/16K/32K/64K/256K on every file tested; 14/14 files identical between the pre-change library and this one at budget 0, LOWRAM_TARGET=1; clean under ASan and UBSan with leak detection on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
zlib_codec_init() went through mz_inflateInit2(), which allocates miniz's inflate_state: 41168 bytes, of which 32768 is a fixed m_dict LZ window. That window was never used. libchdr decompresses a hunk as one complete raw-deflate stream into a buffer big enough for all of it - avail_out = destlen, one Z_FINISH call, then a total_out == destlen check - so mz_inflate() was already passing TINFL_FLAG_USING_NON_WRAPPING_OUTPUT_BUF and tinfl was using the caller's output buffer as its dictionary. m_dict was allocated on every instance and never touched. miniz also rejects any window size other than +/-15, so the mz_inflate* wrappers offer no way to ask for a smaller one, and patching the vendored copy is not an option. Calling tinfl_decompress() directly sidesteps both problems and drops the struct from 41168 to 8376 bytes. Worth noting the encoder cannot force the larger window either: a deflate back-reference can never point before the start of its own stream, and each hunk is an independent stream. The subcode streams decode 768 bytes per hunk, so they could never need more than a 1KB window whatever chdman chose. This is purely a decoder-side allocation. It compounds because a CD-flavoured CHD instantiates zlib three or four times - cdzl needs one for sector data and one for subcode, cdlz and cdfl one each for subcode. Peak heap measured with massif, LOWRAM_TARGET=1: Pyramid Plunder (cdlz+cdzl, 3 instances) 254.0 -> 149.9 KB (-41%) Hawiian Island Girls (cdlz, 1 instance) 151.5 -> 113.1 KB (-25%) That matters on the small-RAM targets this branch is aimed at: on a BL616 with 320KB OCRAM, a three-codec CD CHD went from consuming most of the part to leaving real headroom. The system-zlib path is unchanged and keeps the z_stream plus its custom allocator, since real zlib does honour smaller windows and the allocator exists to serve its zalloc hook. One build-system consequence: libchdr_codec_zlib.c now calls tinfl_decompress() itself, so the ESP ROM symbol rename has to cover libchdr's own targets and not just the miniz target. Without that this call would bind to the ROM's older tinfl and reintroduce the split decoder fixed earlier in this branch. Verified: the ESP-IDF component links libchdr_tinfl_decompress and the ROM-collision check comes back empty. Verified: 14/14 sample files decode identically to the previous implementation under LOWRAM_TARGET=1; output byte-identical across read-ahead budgets; clean under ASan and UBSan with leak detection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
…WRAM cdzs_codec_init() built two ZSTD_DCtx, one per stream. A DCtx is ~94KB - the single largest allocation left in libchdr - so a cdzs-coded CHD spent 187KB on decompression contexts alone, most of a 320KB part's memory before anything else. Sharing one is safe. The two streams are decoded strictly in sequence by cd_codec_decompress(): base to completion, error returns early, then subcode, into disjoint halves of the same buffer, never nested and never concurrently. zstd_codec_decompress() calls ZSTD_initDStream() on entry, so nothing carries between them. And zstd_codec_init() ignores its size argument, so the two contexts were identical objects to begin with. It is not free, which is why it is conditional. Alternating two differently-shaped streams through one context rebuilds its working set each way: measured on x86-64 over a whole file, +3.9%. 94KB for 3.9% is a good trade on a memory-constrained part and a bad one on a desktop, so it is made only under LOWRAM_TARGET - the switch that exists to make exactly this choice. A default build keeps two contexts and its previous speed. Ikaruga (92.9% cdzs), peak heap under LOWRAM_TARGET=1: 251.1 -> 157.4KB. The same sharing was tried for cdzl and reverted. Once the unused 32KB miniz dictionary is gone an inflate context is only ~8KB, and it measured +2.6% for that - not worth it. Also worth recording what did not work, since the reasoning looked sound: switching the zstd codec from the streaming API to one-shot ZSTD_decompressDCtx() saves nothing at all. ZSTD_DStream is a typedef for ZSTD_DCtx, so the memory is the context itself and not streaming staging buffers; measured identical peak and marginally slower. The equivalent change for miniz worked only because miniz allocates a genuinely separate and, in libchdr's usage, entirely unused 32KB dictionary. NOTE for future threading work (see PR #162): codec state is already shared across concurrent chd_read() calls, but this removes even the accidental separation between base and subcode. Per-thread codec state has to mean per-thread codec instances. Verified: 14/14 sample files decode identically to separate contexts under LOWRAM_TARGET=1, block CRCs verified throughout; both LOWRAM_TARGET=1 and =0 build and run; cdzl speed confirmed back at its baseline after the revert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Adds a quick 5-file validation subset, a read-ahead budget knob, and two
probes that answer where the storage time actually goes. Also restores the
40MHz SD clock, which an earlier bisect script had silently reset to the
20MHz default - several runs were measured at the wrong clock.
The probes exist because a hypothesis needed testing and the benchmark had
no way to test it. Read-ahead cut fread() calls by 6-16x on real files and
bought 1.2% of throughput, which only makes sense if the path is not
transaction-bound. Measuring directly:
fread() through FATFS+VFS+stdio, 40MHz:
4KB block 2.25 MB/s 32KB 2.26 64KB 2.26 128KB 2.26
sdmmc_read_sectors(), same card, same clock:
4KB 11.28 MB/s 32KB 16.49 64KB 17.80 256KB 18.96
The card sustains 19 MB/s - essentially the full 40MHz 4-bit bus - and
scales with transfer size. Everything above it delivers a flat 2.26 MB/s
regardless of request size. That is an 8.4x loss between the card and
fread(), and it is software, not hardware.
It also explains the read-ahead result. Fewer, larger reads cannot help
when the cost is per byte inside the filesystem layer rather than per call
beneath it, so 16x fewer transactions moved throughput by 1.2% and made p99
up to 22x worse (a refill transfers the whole window rather than one hunk).
Halving the SD clock to 20MHz only costs 13% (2.05 vs 2.26 MB/s), which is
further confirmation that the bus is not the constraint.
Heap sampling now runs every 64 hunks rather than every 1024. Under
LOWRAM_TARGET codecs are initialised lazily during decode, so the old
interval missed the allocations entirely and reported a plausible-looking
5-7 KB where massif measured 150-254 KB. It now reports 63-172 KB on
target, matching massif's scale.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Owner
Author
Status update: hardware validation, and where the throughput actually goesEverything below is measured on a Waveshare ESP32-P4-NANO against 14 real CHDs on SD. The finding that reframes the restSame card, same 40 MHz clock, no libchdr involved:
The card sustains ~19 MB/s and scales with transfer size. Everything above it delivers a flat 2.26 MB/s regardless of request size — an 8.4× loss in software above the driver. Halving the SD clock costs only 13%, so the bus is not the constraint either. Practical consequence: a raw-sector-backed Negative result, reported in fullThe read-ahead window ( The premise was wrong: I inferred transaction-boundedness from getting <15% of the bus with small transfers, without checking whether large transfers did better. They do not. Kept default-off — budget 0 reproduces the previous behaviour exactly, verified — because the mechanism is still sound where transaction cost genuinely dominates, but nobody should enable it on this evidence expecting a win. What did work
Peak heap for a three-codec CD CHD: 254 → 141.7 KB. Also measured
Dead ends, recorded so they are not retried
|
…ange Layering the storage path three ways on the same card at the same clock showed where the time goes: sdmmc_read_sectors() 18.96 MB/s f_read() (FatFs direct) 10.58 MB/s fread() (VFS + newlib stdio) 2.30 MB/s The VFS and newlib stdio wrapper costs 4.6x on its own. libchdr never has to care: core_file_callbacks already lets the embedder supply any reader, so a FatFs-backed backend is a drop-in alternative to the stdio one and needs no change to the library at all. Measured over the 5-file validation subset, 3000 hunks each, identical read counts in both configurations - the access pattern is unchanged, only the cost per byte: file stdio FatFs io% stdio -> FatFs Castlevania X 2.24 5.06 88.1 -> 72.8 kinst2 1.70 2.54 61.9 -> 42.8 Ikaruga 8.34 11.58 42.5 -> 20.1 Shadowrun 1.58 2.04 35.1 -> 16.4 Bonk III 2.03 2.31 43.5 -> 35.3 aggregate 2.47 3.37 MB/s (60.5s -> 44.4s) Throughput while actually reading went from 2.03 to 6.06 MB/s. This is worth more than everything else measured on this branch put together, and it is an integrator-side change: anyone running libchdr on ESP-IDF over FATFS should implement core_file_callbacks over f_read rather than fopen/fread. The stdio backend is kept as the default so the two can be compared, selected with -DBENCH_FATFS_BACKEND=1. With I/O no longer dominant the balance shifts: Shadowrun and Ikaruga are now 84% and 80% CPU, so further gains have to come from decode or from avoiding decode, not from storage. Also fixes a stack protection fault in the f_read probe - FIL embeds a sector buffer and is far too large for app_main's frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Records the three-layer measurement that localises where storage time goes - driver 18.96 MB/s, FatFs 10.58, fread through VFS+stdio 2.30 - and the resulting advice, which is the most useful thing on this branch: implement core_file_callbacks over f_read rather than fopen/fread. That is an integrator-side change requiring nothing from libchdr, and it is worth more than every library change measured here combined. Also records the read-ahead negative result in full, including why its premise was wrong: throughput is flat across a 32x range of request sizes, so the path is bandwidth-bound in software rather than transaction-bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
The FatFs-backed core_file added in the previous commit went straight to f_open/f_read, which skips ESP-IDF's VFS - and the VFS is what normally allocates cltbl and runs f_lseek(CREATE_LINKMAP) (vfs_fat.c:420-422). So the backend silently had no fast seek at all. That matters because every COMPRESSION_SELF reference is a backward seek (measured: 100% of them, all 14 sample files), and without a cluster link map FatFs restarts its chain walk from the first cluster on every one. On Castlevania X - 271328 hunks, 29.2% self-references - the uncapped sweep was still on that single file after 47 minutes and had to be killed. With the map built at open: stdio backend 844.5 s 1.32 MB/s FatFs backend, no cluster map >47 min, did not finish FatFs backend, cluster map 410.2 s 2.71 MB/s The table is a fair description of how this was nearly shipped: the 5-file comparison that justified the FatFs backend was capped at 3000 hunks, and near the start of a file the chain walks are short enough that the missing map costs almost nothing. Capping a sweep to make it quick also makes it blind past the cap - the same way a 600-hunk cap hid the huffman bug earlier on this branch. Sized at 512 words (2KB per open file) and reported when a file is too fragmented to map, in which case it runs without fast seek rather than failing. Every file on this card is a single fragment, so 3 words would do; the headroom is for cards that are not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
…he backend Replaces the capped 5-file figures with the full 14-file uncapped sweeps, 7.32 GB each, 14/14 files decoding and CRC-verifying in every configuration: stdio backend 3641 s 2.01 MB/s FatFs + cluster map 2549 s 2.87 MB/s 1.43x FatFs + cluster map + 64KB read-ahead 2307 s 3.17 MB/s 1.58x Records the cluster-link-map requirement prominently, because the backend is silently broken without it and a short run cannot see that: going straight to FatFs skips ESP-IDF's VFS, which is what normally allocates cltbl and runs f_lseek(CREATE_LINKMAP), so every backward seek - and every COMPRESSION_SELF reference is one - restarts the cluster-chain walk. Also corrects the read-ahead section, which previously said it does not pay. That was true of the backend it was measured against and false of the one people should use: backend read-ahead gain p99 effect stdio 1.01x up to 22x worse FatFs + cluster map 1.10x mostly better than baseline On stdio the per-byte cost inside the VFS dominates, so cutting call count 6-16x changes almost nothing while a window-sized refill wrecks tail latency. With that per-byte cost gone, per-call cost is a real fraction and the same change earns 10%. It is the clearest case on this branch of an optimisation being right or wrong because of a layer beneath it rather than on its own merits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Owner
Author
Final numbers — full 14-file uncapped sweeps7.32 GB decoded per configuration, 14/14 files OK with zero failures in all three.
Per file, with the gain tracking how I/O-bound each one was:
Ikaruga moves least because it was already ~90% CPU-bound; Castlevania X moves most because it was 88% I/O. None of that 1.58× is a libchdr change. It is Correcting an earlier comment in this threadI previously reported read-ahead as a near-worthless +1.2% with p99 up to 22× worse, and asked whether to revert it. That was measured on the stdio backend, and it is wrong for the backend anyone should actually use:
On stdio the per-byte cost inside the VFS dominates, so cutting call count 6–16× changes almost nothing while a window-sized refill wrecks tail latency. Once that per-byte cost is gone, per-call cost is a real fraction and the same change earns 10%. Keeping it, still default-off. One regression caught after it was pushed
It was invisible to the 3000-hunk comparison that justified the backend, because near the start of a file the chain walks are cheap. That is the second time on this branch a cap made a sweep quick and blind at the same time; the first hid the huffman bug for hours. Summary of the branchThree real libchdr bugs fixed — ESP ROM miniz symbol collision, huffman |
ecc_generate() passes val1/val2 as pointers into the sector it is also reading from, so the compiler has to spill and reload both accumulators on every component in case a source read aliases a destination write. It never can: the P rows read at most byte 2075 and write 2076..2247, the Q rows read at most 2247 and write 2248..2351, and sector[MODE_OFFSET] is never written. Keep the accumulators in locals and store once at the end, read each source byte once instead of twice, and hoist the mode-2 test out of the loop. Output is unchanged - verified byte-identical over 144 CHDs, and VERIFY_BLOCK_CRC checks the regenerated ECC against chdman's own CRC on every hunk. Measured with callgrind: ecc_compute_bytes drops 1.60x, which is 15.9% of total decode cost on an LZMA-heavy disc and 56.8% on a zstd-heavy one, where the fixed per-sector work dominates the codec itself. Also refuse CHDR_WANT_RAW_DATA_SECTOR=OFF with CHDR_VERIFY_BLOCK_CRC=ON. The stored CRC covers the reconstituted hunk, so skipping ECC regeneration cannot match it, and the failure is content-dependent - a hunk holding only audio frames has no ECC to regenerate and still verifies - so it reads as sporadic file corruption rather than a build misconfiguration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
dr_flac's LPC prediction for CD audio always takes its 64-bit path (bitsPerSample + precision + ilog2(order) = 16+15+5 > 32), and a 32x32->64 multiply-accumulate costs mul plus mulh on RV32 where x86-64 spends one imul. That was a candidate explanation for FLAC costing more per instruction on the P4 than LZMA does, so measure it instead of assuming. idf.py -DBENCH_MULPROBE=1 times dependent chains and returns without touching the corpus. On ESP32-P4 at 400 MHz: add 1.25, xor 1.25, mul 2.13, mulh 2.13 cycles/op. At 12.6% multiply density in dr_flac's rice loop against 2.9% in LzmaDec, that accounts for ~6.7% - so the multiplier is not the explanation. For the record, there was nothing to explain: profiling the seed CHDs the board actually runs shows x86 instruction counts predict P4 wall clock across all five codecs with one slope, 1.30 cycles/instruction. cdfl costs 3.72x cdlz in instructions and 3.57x in wall clock. FLAC is not penalised on RISC-V, it simply does ~127 instructions per audio sample. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Taken from dr_libs master; upstream has not tagged 0.13.4 yet, so the header
still reads "v0.13.4 - TBD". The fixes are worth having ahead of the tag
because libchdr feeds dr_flac attacker-supplied data.
- drflac__realloc_from_callbacks() copied szOld bytes into the new buffer
even when shrinking, overflowing the smaller allocation. Now copies
DRFLAC_MIN(szNew, szOld).
- drflac__decode_subframe()'s result was discarded, so a subframe that
failed to decode still reported DRFLAC_TRUE. It now propagates, and the
frame error path zeroes currentFLACFrame.subframes and clears
pcmFramesRemaining so stale samples cannot be handed back after a failure.
libchdr only caught this downstream, and only with VERIFY_BLOCK_CRC on.
- Bounds checking when parsing metadata, and a validation check at init that
rejects a 32-bit overflow from a malformed file.
- Seeking fixes: handle the case where binary search cannot narrow further
than two adjacent byte offsets, and use double rather than float for the
approximate compression ratio.
The public API is unchanged - the onTell additions are optional and
drflac_open_with_metadata() still passes NULL for it, so src/libchdr_flac.c
needs no change.
Revalidated: decoded output is byte-identical to 0.13.3 over 287 CHDs
(228,054 hunks sampled strided across each disc, so audio tracks after the
data track are actually reached), zero differences and zero decode failures.
All 17 corpus seeds identical, including cd_cdfl (100% FLAC), and the AVHuff
regression suite passes 4/4 including the FLAC-audio case.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
The x86 instruction counts we model P4 wall clock against include glibc's malloc, but not ESP-IDF's capability-tagged multi-heap allocator, so the allocator was a live candidate for FLAC's cost that the model could not see. dr_flac allocates roughly one 32-64KB block per FLAC hunk - measured on desktop with a malloc interposer at 1.9 allocs/hunk for the cd_cdfl seed and 0.82 allocs plus 34.7KB per cdfl hunk on a real disc, against 0.017 allocs and 714 bytes for cdlz, so ~39x the allocation rate. Measured on ESP32-P4 at 400 MHz: heap_caps_malloc + heap_caps_free costs ~4.08 us and is flat from 4KB to 224KB (it does not zero), while memset of 32KB costs 30.9 us. That puts the allocator at 0.15% of a cdfl hunk, or 0.28% of the whole cdfl-to-cdlz gap - 7.7 us against 2.78 ms. Including zeroing the full 34.7KB the upper bound is 0.8%. Ruled out. This also settles the reverted drflac arena: it was a RAM fix, and a perfect one would return under 8 us per hunk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Two exact algebraic identities in libchdr's ECC, both verified over their full domain rather than sampled: ecclow[x] == xtime(x) in GF(2^8) with poly 0x11d, all 256 entries poffsets[r][c] == r + 86c qoffsets[r][c] == (86*(r>>1) + (r&1) + 88c) mod 2236 So all three tables - 8856 bytes of .rodata - are computable, and the P offsets being contiguous across rows means P's 86 independent rows can be the inner loop over consecutive bytes. x86 instruction counts say the arithmetic forms are worse (+17.6% Ir) and an in-order single-issue core says the opposite of a wide out-of-order one, so measure instead of reason. ESP32-P4 at 400 MHz, cycles per 2352-byte sector, all five producing identical parity: A table off + table xtime (ships) 47757 1.000x B table off + arith xtime 64635 0.739x C closed off + table xtime 50543 0.945x D closed off + arith xtime 73493 0.650x E P row-inner vectorisable + Q as-is 50084 0.954x The shipped version wins on this core: one lbu from a hot 256-byte table beats six ALU ops, and the closed-form offset walk costs more than the offset load it removes. Nothing to change here. Variant E is the interesting one because it is target-divergent. The same restructure auto-vectorises elsewhere - 9.7x on x86-64 SSE2, 17.7x with AVX2, and NEON on aarch64 - but costs 4.6% on the P4, which has no vector unit GCC can target (PIE is assembler-only, no intrinsics). It also needs -O3; -ftree-vectorize is not on at -O2, where the win drops to 1.08x. Q cannot be vectorised either way: its offsets are a diagonal, contiguous in neither dimension, and it is 2236 of the 4300 components per sector. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
The 86 P rows are independent of one another and, for a fixed component, read consecutive bytes - poffsets[row][comp] is exactly row + 86*comp. So four rows fit in one 32-bit accumulator and the per-component step goes word-wide. The table lookup cannot come along, but it does not need to: ecclow[x] is exactly multiplication by x in GF(2^8) with polynomial 0x11d, verified over all 256 entries. Masking the bits that would cross a byte boundary lets one word carry four independent products. This is SWAR, not SIMD - no vector ISA, no intrinsics, no target-specific code, and it is a win on any machine with 32-bit registers. That matters because the obvious alternative does not port: restructuring the loop so a compiler can auto-vectorise it is 9.7x on x86-64 SSE2 and 17.7x with AVX2, but needs -O3 (1.08x at -O2) and *loses* 4.6% on ESP32-P4 and 22% on aarch64 at -O2, because without a vector unit the accumulators spill to memory. Bytes are assembled from four loads rather than read as a word. The P stride is 86 and 86 % 4 == 2, so consecutive components alternate 4-aligned and 2-aligned whatever the caller's buffer alignment - it cannot be padded away. Measured on ESP32-P4, the byte-built word beats both an lw/lhu split by alignment parity and a padded aligned copy, and it is correct everywhere. Measured, ecc_generate over a 2352-byte sector on ESP32-P4 at 400 MHz: shipped scalar 47776 cycles 1.000x P SWAR 4x, byte-built word 32513 cycles 1.469x End to end on real discs on the board, where ECC is only regenerated for the frames whose parity chdman stripped: Pyramid Plunder (22.5% ECC) 468.58 -> 444.41 ms 1.054x Hawiian Island Girls 916.64 -> 888.11 ms 1.032x Local Girls of Hawaii (11.9% ECC) 1079.33 -> 1046.37 ms 1.031x whole 24-file flash corpus 2.87 -> 2.78 s 1.032x On a disc whose data sectors dominate the P parity is a bigger share - 56.8% of decode CPU on Ikaruga - and x86 instruction counts drop 240.2M to 210.9M there, with the P half alone going 48.5M to 21.2M. Output is unchanged: byte-identical over 287 CHDs, with VERIFY_BLOCK_CRC checking the regenerated parity against chdman's own CRC on every hunk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
crc16_update() was a byte-at-a-time table walk, which GCC turns into twelve instructions per byte on RV32 - two of them purely to truncate the accumulator back to uint16_t. It runs over every decoded hunk under VERIFY_BLOCK_CRC, so on an ESP32-P4 it cost about 0.76 ms per 19584-byte hunk: more than the zstd decode it was checking. Slice-by-4 takes that to 6.25 instructions per byte. s_table1/2/3 are s_table advanced by one, two and three byte positions, so four lookups XOR together into one 16-bit result. Costs 1536 bytes of extra rodata. Verified identical to the byte-at-a-time result for every length 0..4096 and for 256 different starting CRCs - the latter matters because the CHD v5 map CRC chains a running value rather than restarting from 0xffff. Decoded output is byte-identical over 287 CHDs, which also exercises the check itself: a wrong CRC would reject every hunk. Measured on ESP32-P4 at 400 MHz, ms/hunk on identical content, against the state before this branch's ECC work: codec before +ECC SWAR +this total cd_cdzl 1.945 1.926 1.656 1.175x cd_cdzs 1.901 1.907 1.645 1.156x cd_cdlz 2.303 2.304 2.038 1.130x cd_cdfl 5.087 5.018 4.753 1.070x cd_none 1.218 1.220 1.223 0.996x Pyramid Plunder 468.58 -> 409.54 ms 1.144x Hawiian Island Girls 916.64 -> 847.72 ms 1.081x cd_none is unchanged because an uncompressed CHD takes the early path that never reaches the CRC. Everything else gains, because unlike the ECC work this runs on every compressed hunk of every codec on every platform. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Writes up what was measured on the board, what shipped, and - more usefully - the things that look obviously correct on paper and are losses in practice: six of the seven ECC rewrites tried, and seven hypotheses for FLAC's cost including three I asserted before measuring them properly. Also records the two levers still open (DR_FLAC_NO_CRC, the cdfl triple-copy) with their risks, and a methodology section listing the measurement mistakes that produced wrong conclusions along the way: anchoring a fit on the point under test, comparing different discs, trusting x86 instruction counts for a load-latency question on an in-order core, counting static instructions in a function full of dead specialisations, and using synthetic seeds that turn out to do 0% of the work being measured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
get_bigendian_uint32_t shifted base[0] left by 24 without casting first. A
uint8_t promotes to int, so any byte >= 0x80 made that shift signed overflow -
undefined behaviour - in the header and map parser, on data that comes straight
from the file.
Real files never hit it: every field read through this helper holds either a
small count or a four-character codec tag, and those tags are ASCII, so the
high byte is always below 0x80. A malformed file only has to set one high bit.
Found by fuzzing mutated headers under UBSan:
libchdr_chd.c:763:18: runtime error: left shift of 228 by 24 places
cannot be represented in type 'int'
#0 get_bigendian_uint32_t
#1 header_read
#2 chd_open_core_file_callbacks
get_bigendian_uint48 and get_bigendian_uint64_t already cast for exactly this
reason; this one was inconsistent with them.
Output is unchanged for well-formed files. Verified against 561 malformed CHDs
generated from all 17 corpus seed codecs - truncations and header and body
mutations - which now run clean under ASan and UBSan with no hangs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
dr_flac CRC-checks every FLAC frame it decodes. When VERIFY_BLOCK_CRC is on, libchdr then checks the whole decoded hunk against the CRC chdman stored - the same data verified twice. Defining DR_FLAC_NO_CRC drops the inner check: a corrupt frame that dr_flac would have rejected instead decodes to garbage, and the hunk CRC rejects it one level up with the same CHDERR_DECOMPRESSION_ERROR. Tied to VERIFY_BLOCK_CRC rather than to any "small target" switch, because VERIFY_BLOCK_CRC is precisely what makes it safe. Without it the frame CRC is the only integrity check FLAC data gets, so the define must not appear. Built both ways on RV32 to confirm the gate works: text 51765 bytes with VERIFY_BLOCK_CRC=1, 64859 with it off. That is 13094 bytes of text (-20%) on ESP32-P4, because the compiler can then discard dr_flac's CRC-8 and CRC-16 tables entirely, and rice__scalar shrinks 23206 -> 19056. Worth roughly 8% of a CD-FLAC hunk. DR_FLAC_NO_CRC also disables binary-search seeking, which libchdr never uses: each hunk is opened as a complete stream and read straight through, and drflac_seek_to_pcm_frame() is never called. Detection is unaffected. Over 25 independent corruptions of a 93.6%-cdfl disc, the build with dr_flac's CRC and the build without agree on every case, with zero instances of corrupt output escaping detection. Decoded output is byte-identical over 287 CHDs. chdconfig.h is now included so VERIFY_BLOCK_CRC takes the same default here as in libchdr_chd.c when the build system does not define it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
The CHD v5 compressed map header carries three bit widths - lengthbits,
selfbits and parentbits - as raw bytes, and they become the width argument to
bitstream_read() for every map entry. Nothing validated them, so a malformed
file could make bitstream_peek() evaluate
bitstream->buffer >> (32 - numbits)
with numbits above 32, shifting by a negative amount. Found by fuzzing:
libchdr_bitstream.c:62:27: runtime error: shift exponent -36 is negative
#0 bitstream_peek
#1 bitstream_read
#2 build_v5_map_checkpoints
#3 decompress_v5_map
#4 chd_open_core_file_callbacks
chdman derives all three from hunkbytes and the hunk count, so they never
legitimately exceed 32; anything larger means the file is corrupt. Reject it as
CHDERR_INVALID_FILE at parse time, before the value reaches the bitstream.
bitstream_remove() is hardened separately: consuming all 32 bits is a legitimate
request that peek() already serves, but the matching `buffer <<= 32` on a
uint32_t was undefined too.
Well-formed files are unaffected - decoded output is byte-identical over the
CHD corpus. 3281 malformed inputs, generated from all 17 corpus seed codecs
across header, map-region, whole-file and truncation mutations, now run clean
under ASan and UBSan with no hangs, on both 64-bit and 32-bit builds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
bitstream_remove() subtracts unconditionally, so a stream that malformed input
has over-consumed ends up with a negative bit count. bitstream_peek() then
refills with
buffer |= read[doffset] << (24 - bits)
and once bits drops below -8 that shift reaches 32 on a 32-bit type, which is
undefined. Reached through the huffman decoder while parsing a corrupt v5 map:
libchdr_bitstream.c:55:72: runtime error: shift exponent 32 is too large
#0 bitstream_peek
#1 huffman_decode_one
#2 decompress_v5_map
Skipping the byte in that case is not just safe but arithmetically correct: a
byte shifted 32 or more places lands entirely above bit 31 and contributes
nothing to a 32-bit accumulator. Well-formed streams keep bits >= 0, so the
shift stays at 24 or below and the guard never fires.
This one only showed up with CHDR_LOWRAM_TARGET=OFF - the lazy checkpointed map
reaches the huffman decoder differently - so it had been missed by fuzzing that
only covered the low-RAM path. Both map implementations are now fuzzed.
3412 malformed inputs (mutations of all 17 corpus seed codecs, plus
structure-aware header-field cases) run clean under ASan and UBSan against both
LOWRAM_TARGET=ON and OFF. Decoded output for well-formed files is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Adds the end-to-end number the CPU work is worth - 2307 s to 1924.7 s over the same 7.32 GB, 1.20x - with the per-file breakdown and how each disc compares to the drive it shipped on. Records the three latent parser bugs the fuzzing found, all pre-existing, and the robustness bar now covering them: 3412 malformed inputs across both map implementations and both word sizes, metadata-chain cycles, API misuse, access order equivalence, and the config matrix. Also the self-reference locality measurement that closes the decoded-hunk cache question, and the measured portability matrix for the two perf changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
Owner
Author
|
Superseded by #180, which is the same work rebuilt on a clean branch: 32 commits squashed to 13 topical ones, with the investigation scaffolding removed (per-stage FLAC profiling, the debug-printf diagnostics, and five one-off benchmark probes - about 700 lines). #180 also carries three fixes found after this PR was opened: two undefined shifts reachable from a malformed CHD (unvalidated v5 map bit widths, and a bitstream refill shifting by 32), and the dr_flac 0.13.4 bump for its heap-overflow fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Real-hardware validation on a Waveshare ESP32-P4-NANO (chip rev v3.1, ESP-IDF v5.5.5) turned up two independent bugs. One is a genuine correctness bug affecting every ESP-IDF target; the other is an error-reporting bug that made the first one much harder to find.
1. ESP ROM exports its own older miniz, and it wins the link
Espressif's ROMs bake in an older miniz and export its tinfl entry points from the target's ROM linker script as absolute symbols — see the "Group miniz" block in
esp_rom/<target>/ld/<target>.rom.ld:A linker-script assignment outranks an ordinary object definition, so an ESP-IDF link silently binds that name to ROM and drops the copy compiled from
deps/miniz-3.1.2/miniz.c— even though both are in the archive:The result is a split decoder:
mz_inflateInit2()/mz_inflate()from miniz 3.1.2 build and interpret a 3.1.2-layouttinfl_decompressor, then hand it to a ROM decoder that lays that struct out differently (miniz 3.0 reworked the Huffman tables fromtinfl_huff_table m_tables[3]to the flattenedm_look_up/m_tree_Nform). The ROM decoder overruns the smallerm_decompinto the enclosinginflate_state.How it presents:
CHDERR_DECOMPRESSION_ERROR, hardware only, deterministic, data-dependent. The overrun lands oninflate_state::m_window_bits(which sits just beforem_dict[32768]), so the first inflate of a stream mostly survives and every later one fails.The tell: all 16 failures consumed exactly 2 bytes with
total_out=0,zerr=-3:That is
TINFL_FLAG_PARSE_ZLIB_HEADER's 2-byte CMF/FLG check firing on a raw-deflate stream opened withinflateInit2(…, -MAX_WBITS)— i.e. corruptedm_window_bits, not bad data.Why it looked like silicon. Desktop x86-64, x86-32, vanilla RV32 GCC under QEMU, and Espressif's own GCC 14.2.0 freestanding under QEMU all passed — none of them links ESP-IDF's ROM linker scripts, so all used real miniz.
CONFIG_HEAP_POISONING_COMPREHENSIVEwas blind too: the corruption is intra-block, so it never reaches a canary.Fix. Six names collide:
tinfl_decompress,tinfl_decompress_mem_to_{heap,mem,callback},mz_adler32,mz_free. (mz_freematters on its own — bound to ROM it hands ESP-IDF-heap pointers to the ROM allocator.) They're renamed via compile definitions incmake/EspRomMinizWorkaround.cmake, applied to whichever target compilesminiz.c, no-op off ESP-IDF.Deliberately not patched into
deps/miniz-3.1.2/miniz.h— that tree is vendored verbatim and re-synced from upstream, so an edit there would be silently dropped by the next miniz bump and resurrect this bug with no diff to point at.git diff master..HEAD -- deps/is empty. Onlyminiz.creferences these names, so target-scoped defines suffice.Regression check, cheap and no flashing required — must print nothing:
Worth re-running after any dep bump, and after adding any dep the ROM also ships — the
rom.ldfiles list them by group.2. drflac is reallocated once per hunk, and OOM was reported as a decode error
flac_decoder_reset()callsdrflac_open_with_metadata()per hunk, allocating a fresh decoder plus a STREAMINFO-sized sample buffer (~40KB for a CD-FLAC hunk) and freeing it on the next hunk. That failure surfaced asCHDERR_DECOMPRESSION_ERROR, indistinguishable from corrupt audio — which is exactly why it looked like more of the miniz bug above.drflac now goes through allocation callbacks that record failure, so cdfl/flac return
CHDERR_OUT_OF_MEMORY.Evidence it really is headroom and not data: the 3 affected titles are the 3 largest in the corpus by hunk count (11055 / 11271 / 11825) while the largest passing one is 7312 — a monotone boundary. Rawmap costs 12 bytes/hunk, so ~142KB at 11825 hunks, past which the 40KB no longer fits. All three decode fully on desktop in both LP64 and ILP32. Confirmed on hardware after the change:
alloc_failed=1on all three.Reusing one drflac instance across hunks would remove both the failure and the per-hunk malloc/free churn. Not attempted here.
Results
Same board, same card, before vs after:
The SD "after" run caps at 600 hunks/file (uncapped takes hours now that files actually decode). A cap can only hide failures past hunk 600 and all 16 pre-fix failures were at hunks 0–272, so it isn't manufacturing the result. The flash corpus is uncapped in both rows and is the like-for-like comparison.
Remaining SD failures are all RAM-capacity or corpus artifacts, none a decode defect: 76 OOM at open, 16 naomi GD-ROM parent/child sets that fail identically on desktop x86-64, 3 drflac OOM, 2 AVHuff. One reset in the whole run (power-on).
Testing
-m32) build also passes.T tinfl_decompress, confirming the workaround is inert off ESP-IDF.Review notes
contrib/esp32p4/idf-benchmark/is a new self-contained ESP-IDF app; it builds nothing by default and is not wired into CI.rom.ldfiles, not measured.🤖 Generated with Claude Code
https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG