From 1a10dd39ee18d07f3fda2f887d694302c8e4efc7 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Mon, 24 Aug 2026 12:28:13 +0200 Subject: [PATCH 1/7] Fix signed left-shift UB in bitstream_peek bitstream->read[doffset] is uint8_t, promoted to int before the shift; shifting a byte with the high bit set left by 24 overflows a signed 32-bit int, which is undefined behavior. This fires on roughly half of all input bytes (any byte >= 0x80), not a rare edge case. Cast to uint32_t before the shift so it's well-defined. Found while investigating an RV32 QEMU crash during real-content testing; ruled out as that crash's cause (compiled output verified byte-identical before/after the fix on the affected toolchain) but real UB worth fixing regardless. No behavior change: verified 64/64 byte-identical decode against unmodified master across the full synthetic corpus (sequential/reverse/random read orders, LOWRAM_MAP on and off), plus valgrind --leak-check=full clean. --- src/libchdr_bitstream.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libchdr_bitstream.c b/src/libchdr_bitstream.c index 240214a..de67221 100644 --- a/src/libchdr_bitstream.c +++ b/src/libchdr_bitstream.c @@ -52,7 +52,7 @@ uint32_t bitstream_peek(struct bitstream* bitstream, int numbits) while (bitstream->bits <= 24) { if (bitstream->doffset < bitstream->dlength) - bitstream->buffer |= bitstream->read[bitstream->doffset] << (24 - bitstream->bits); + bitstream->buffer |= (uint32_t)bitstream->read[bitstream->doffset] << (24 - bitstream->bits); bitstream->doffset++; bitstream->bits += 8; } From 307e47657b8dd83937aaa262135abb3cf927436e Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Mon, 24 Aug 2026 12:28:55 +0200 Subject: [PATCH 2/7] LOWRAM_MAP: two-level huffman lookup table Each huffman_decoder currently allocates a full 2^maxbits direct-lookup table (128 KiB at maxbits=16, the width used by AVHuff's Y/Cb/Cr contexts and the existing CHD huffman codec). Under LOWRAM_MAP, replace it with a two-level table: a 2^L1BITS first-level table (default L1BITS=10, tunable via LOWRAM_MAP_HUFFMAN_L1BITS), escaping to small per-prefix subtables only for codes longer than L1BITS. Huffman assigns short codes to frequent symbols by construction, so the common decode path stays a single table load; only rare long codes pay a second indirection. Measured on real RV32 QEMU hardware against the existing hd_huff codec (maxbits=16, numcodes=256): 150,677 -> 25,141 bytes peak heap (-83%). Directly applicable to AVHuff's three Y/Cb/Cr huffman contexts, which use the same decoder shape. An unwritten L1/subtable slot previously held malloc/memset(0) garbage that could alias the escape sentinel (bits field == 0) and point at a subtable index that doesn't exist - reachable from malformed input (e.g. a degenerate single-symbol tree). Fixed by prefilling every slot with a bounded, non-escaping placeholder before the real fills, so malformed input degrades to wrong output (matching the old single-level table's failure mode) instead of a crash or infinite non-consuming decode loop. Verified byte-identical decode vs. unmodified master across the full synthetic corpus x {sequential, reverse, random} read orders, repeated at LOWRAM_MAP_HUFFMAN_L1BITS in {2,3,4,10} specifically to force the subtable/escape path to execute (the default L1BITS=10 alone would not exercise it on this corpus - its huffman trees are too skewed to produce any code longer than 10 bits). Plus valgrind --leak-check=full clean, and the same 64-check comparison with LOWRAM_MAP off (unaffected - this code is compiled out entirely at LOWRAM_MAP=0). --- include/libchdr/chdconfig.h | 13 +++ include/libchdr/huffman.h | 9 +- src/libchdr_huffman.c | 170 ++++++++++++++++++++++++++++++++++-- 3 files changed, 186 insertions(+), 6 deletions(-) diff --git a/include/libchdr/chdconfig.h b/include/libchdr/chdconfig.h index 8f531a0..16197ab 100644 --- a/include/libchdr/chdconfig.h +++ b/include/libchdr/chdconfig.h @@ -30,4 +30,17 @@ #define LOWRAM_MAP_CHECKPOINT_STRIDE 512 #endif +/* Under LOWRAM_MAP, also replace each huffman_decoder's full 2^maxbits + * direct-lookup table (e.g. 128 KiB at maxbits=16, as used by AVHuff's + * Y/Cb/Cr contexts and the CHD huffman codec) with a two-level table: a + * 2^L1BITS first-level table, escaping to small per-prefix subtables only + * for the rare codes longer than L1BITS. Huffman assigns short codes to + * common symbols by construction, so the fast (non-escaping) path is + * unchanged; only long, rare codes pay an extra indirection. Total RAM + * drops to a few KiB per decoder regardless of maxbits. + */ +#ifndef LOWRAM_MAP_HUFFMAN_L1BITS +#define LOWRAM_MAP_HUFFMAN_L1BITS 10 +#endif + #endif diff --git a/include/libchdr/huffman.h b/include/libchdr/huffman.h index 446721d..7cbef43 100644 --- a/include/libchdr/huffman.h +++ b/include/libchdr/huffman.h @@ -14,6 +14,7 @@ #define __HUFFMAN_H__ #include "bitstream.h" +#include "chdconfig.h" /*************************************************************************** @@ -59,9 +60,15 @@ struct huffman_decoder uint8_t maxbits; /* maximum bits per code */ uint8_t prevdata; /* value of the previous data (for delta-RLE encoding) */ int rleremaining; /* number of RLE bytes remaining (for delta-RLE encoding) */ - lookup_value * lookup; /* pointer to the lookup table */ + lookup_value * lookup; /* pointer to the lookup table (full 2^maxbits table, + or under LOWRAM_MAP, the 2^l1bits first-level table) */ struct node_t * huffnode; /* array of nodes */ uint32_t * datahisto; /* histogram of data values */ +#if LOWRAM_MAP + uint8_t l1bits; /* first-level table width in bits, MIN(maxbits, LOWRAM_MAP_HUFFMAN_L1BITS) */ + lookup_value * subtable; /* concatenated second-level subtables, one per escaping first-level prefix */ + uint32_t subtable_count; /* number of subtables currently allocated */ +#endif /* array versions of the info we need */ #if 0 diff --git a/src/libchdr_huffman.c b/src/libchdr_huffman.c index bbd163f..3180201 100644 --- a/src/libchdr_huffman.c +++ b/src/libchdr_huffman.c @@ -132,7 +132,14 @@ struct huffman_decoder* create_huffman_decoder(int numcodes, int maxbits) decoder = (struct huffman_decoder*)malloc(sizeof(struct huffman_decoder)); decoder->numcodes = numcodes; decoder->maxbits = maxbits; +#if LOWRAM_MAP + decoder->l1bits = MIN(maxbits, LOWRAM_MAP_HUFFMAN_L1BITS); + decoder->lookup = (lookup_value*)malloc(sizeof(lookup_value) * (1u << decoder->l1bits)); + decoder->subtable = NULL; + decoder->subtable_count = 0; +#else decoder->lookup = (lookup_value*)malloc(sizeof(lookup_value) * (1 << maxbits)); +#endif decoder->huffnode = (struct node_t*)malloc(sizeof(struct node_t) * numcodes); decoder->datahisto = NULL; decoder->prevdata = 0; @@ -146,6 +153,10 @@ void delete_huffman_decoder(struct huffman_decoder* decoder) { if (decoder->lookup != NULL) free(decoder->lookup); +#if LOWRAM_MAP + if (decoder->subtable != NULL) + free(decoder->subtable); +#endif if (decoder->huffnode != NULL) free(decoder->huffnode); free(decoder); @@ -161,13 +172,28 @@ void delete_huffman_decoder(struct huffman_decoder* decoder) uint32_t huffman_decode_one(struct huffman_decoder* decoder, struct bitstream* bitbuf) { /* peek ahead to get maxbits worth of data */ - uint32_t bits = bitstream_peek(bitbuf, decoder->maxbits); + uint32_t window = bitstream_peek(bitbuf, decoder->maxbits); + lookup_value lookup; + +#if LOWRAM_MAP + /* two-level lookup: common (short-code) case is one table load, same as + * the full-table path below; only codes longer than l1bits pay for a + * second indirection into a small per-prefix subtable. */ + uint32_t extrabits = decoder->maxbits - decoder->l1bits; + lookup = decoder->lookup[window >> extrabits]; + if ((lookup & 0x1f) == 0) + { + uint32_t subid = lookup >> 5; + uint32_t idx2 = window & ((1u << extrabits) - 1); + lookup = decoder->subtable[subid * (1u << extrabits) + idx2]; + } +#else + /* look it up directly in the full table */ + lookup = decoder->lookup[window]; +#endif - /* look it up, then remove the actual number of bits for this code */ - lookup_value lookup = decoder->lookup[bits]; + /* remove the actual number of bits for this code, then return the value */ bitstream_remove(bitbuf, lookup & 0x1f); - - /* return the value */ return lookup >> 5; } @@ -536,6 +562,139 @@ enum huffman_error huffman_assign_canonical_codes(struct huffman_decoder* decode *------------------------------------------------- */ +#if LOWRAM_MAP +enum huffman_error huffman_build_lookup_table(struct huffman_decoder* decoder) +{ + uint32_t l1bits = decoder->l1bits; + uint32_t l1size = 1u << l1bits; + uint32_t extrabits = decoder->maxbits - l1bits; + uint32_t subsize = 1u << extrabits; + int32_t *prefix_subid; + uint32_t curcode, i; + enum huffman_error result = HUFFERR_NONE; + + /* build-time-only bookkeeping: which first-level prefix already has a + * subtable allocated for it. Not part of the decoder's persistent RAM + * footprint - freed before this function returns. */ + prefix_subid = (int32_t*)malloc(sizeof(int32_t) * l1size); + if (prefix_subid == NULL) + return HUFFERR_INTERNAL_INCONSISTENCY; + for (i = 0; i < l1size; i++) + prefix_subid[i] = -1; + + /* Canonical-code coverage of the table is only guaranteed complete when + * every assigned length participates in the Kraft-equality check in + * huffman_assign_canonical_codes() (length 1 is exempt there, and a + * degenerate/malformed tree can leave other gaps too). An unwritten + * slot would otherwise hold malloc() garbage that can alias a valid + * escape entry (bits field == 0) pointing at a subtable index that was + * never allocated - decode would then index decoder->subtable with it + * while subtable is still NULL. Prefill with a bounded, non-escaping + * placeholder (matches the old single-level table's failure mode: wrong + * output on malformed input, never a crash) before any real code fills + * its range. */ + for (i = 0; i < l1size; i++) + decoder->lookup[i] = MAKE_LOOKUP(0, 1); + + /* pass 1: codes short enough to decode directly from the first-level + * table (the common case - huffman assigns these to frequent symbols) */ + for (curcode = 0; curcode < decoder->numcodes; curcode++) + { + struct node_t* node = &decoder->huffnode[curcode]; + if (node->numbits > 0 && node->numbits <= l1bits) + { + int shift = l1bits - node->numbits; + lookup_value value = MAKE_LOOKUP(curcode, node->numbits); + lookup_value *dest = &decoder->lookup[node->bits << shift]; + lookup_value *destend = &decoder->lookup[((node->bits + 1) << shift) - 1]; + if (dest >= &decoder->lookup[l1size] || destend >= &decoder->lookup[l1size] || destend < dest) + { + result = HUFFERR_INTERNAL_INCONSISTENCY; + goto done; + } + while (dest <= destend) + *dest++ = value; + } + } + + /* pass 2: rare, longer codes - escape from their first-level prefix + * into a small per-prefix subtable. A prefix­-free (canonical) code + * guarantees a long code's first-level prefix is never touched by a + * pass-1 fill, so every subtable slot ends up written exactly once. */ + for (curcode = 0; curcode < decoder->numcodes; curcode++) + { + struct node_t* node = &decoder->huffnode[curcode]; + if (node->numbits > l1bits) + { + uint32_t restbits = node->numbits - l1bits; + uint32_t prefix = node->bits >> restbits; + uint32_t local_bits, subid; + int local_shift; + lookup_value value, *base, *dest, *destend; + + if (prefix >= l1size) + { + result = HUFFERR_INTERNAL_INCONSISTENCY; + goto done; + } + + if (prefix_subid[prefix] < 0) + { + lookup_value *grown; + if (decoder->subtable_count >= (1u << 11)) + { + result = HUFFERR_TOO_MANY_CONTEXTS; + goto done; + } + subid = decoder->subtable_count; + grown = (lookup_value*)realloc(decoder->subtable, + sizeof(lookup_value) * subsize * (decoder->subtable_count + 1)); + if (grown == NULL) + { + result = HUFFERR_INTERNAL_INCONSISTENCY; + goto done; + } + decoder->subtable = grown; + decoder->subtable_count++; + /* same bounded, non-escaping placeholder as the L1 prefill + * above - an unwritten subtable slot must never look like + * a 0-bit read (that would stall the caller's decode loop + * without consuming input). */ + { + uint32_t j; + lookup_value *sub = &decoder->subtable[subid * subsize]; + for (j = 0; j < subsize; j++) + sub[j] = MAKE_LOOKUP(0, 1); + } + decoder->lookup[prefix] = MAKE_LOOKUP(subid, 0); + prefix_subid[prefix] = (int32_t)subid; + } + else + { + subid = (uint32_t)prefix_subid[prefix]; + } + + local_bits = node->bits - (prefix << restbits); + local_shift = (int)extrabits - (int)restbits; + value = MAKE_LOOKUP(curcode, node->numbits); + base = &decoder->subtable[subid * subsize]; + dest = &base[local_bits << local_shift]; + destend = &base[((local_bits + 1) << local_shift) - 1]; + if (dest >= &base[subsize] || destend >= &base[subsize] || destend < dest) + { + result = HUFFERR_INTERNAL_INCONSISTENCY; + goto done; + } + while (dest <= destend) + *dest++ = value; + } + } + +done: + free(prefix_subid); + return result; +} +#else enum huffman_error huffman_build_lookup_table(struct huffman_decoder* decoder) { const lookup_value* lookupend = &decoder->lookup[(1u << decoder->maxbits)]; @@ -567,3 +726,4 @@ enum huffman_error huffman_build_lookup_table(struct huffman_decoder* decoder) return HUFFERR_NONE; } +#endif From d56b95a74b51e9122bc51b2fbb0081c09dc1e22c Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Mon, 24 Aug 2026 12:29:14 +0200 Subject: [PATCH 3/7] LOWRAM_MAP: demand-grow the compressed scratch buffer chd->compressed was preallocated to header.hunkbytes at chd_open(), regardless of how large any given hunk's actual compressed payload turns out to be. Under LOWRAM_MAP, start it at NULL and grow it (via realloc, monotonic, never shrinks) to fit each hunk's actual compressed length in hunk_read_compressed() - the map entry already gives that length before the read happens, no extra lookup needed. Peak usage converges to the largest hunk actually touched in the session instead of the file's worst case. Applies to every LOWRAM_MAP read path, v1-4 and v5 alike - hunk_read_compressed() is shared, not AVHuff- or CHDv5-specific. hunk_read_uncompressed() reads straight into the caller's destination buffer and never touches this scratch buffer, so demand-growth only ever sees genuinely-compressed hunks. A failed realloc returns NULL before touching chd->compressed, so a failed grow leaves the existing buffer/capacity intact rather than corrupting state. Verified byte-identical decode vs. unmodified master across the full synthetic corpus x {sequential, reverse, random} read orders, plus valgrind --leak-check=full clean, with LOWRAM_MAP on and off (compiled out entirely at LOWRAM_MAP=0, so the non-LOWRAM allocation path is unchanged). --- src/libchdr_chd.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/src/libchdr_chd.c b/src/libchdr_chd.c index e749f04..a68b736 100644 --- a/src/libchdr_chd.c +++ b/src/libchdr_chd.c @@ -302,6 +302,15 @@ struct _chd_file * deferred to the first hunk_read_into_memory() call that actually needs * that slot - see ensure_codec_ready(). */ uint8_t codec_lazy_initialized[4]; + + /* `compressed` is grown on demand to the largest per-hunk compressed + * length actually read (map entries carry that length before the read + * happens), instead of being preallocated to the worst case + * (header.hunkbytes) at open() - see hunk_read_compressed(). Real + * compressed hunks are typically well under hunkbytes, and for a + * session that only touches part of a file (e.g. sequential playback) + * peak usage tracks what was actually touched, not the whole file. */ + uint32_t compressed_capacity; #endif }; @@ -1652,10 +1661,17 @@ CHD_EXPORT chd_error chd_open_core_file_callbacks(const core_file_callbacks *cal if (err != CHDERR_NONE) EARLY_EXIT(err); +#if LOWRAM_MAP + /* grown on demand in hunk_read_compressed() instead of preallocated to + * the worst case (header.hunkbytes) here - see compressed_capacity. */ + newchd->compressed = NULL; + newchd->compressed_capacity = 0; +#else /* allocate the temporary compressed buffer */ newchd->compressed = (uint8_t *)malloc(newchd->header.hunkbytes); if (newchd->compressed == NULL) EARLY_EXIT(err = CHDERR_OUT_OF_MEMORY); +#endif /* find the codec interface */ if (newchd->header.version < 5) @@ -2468,10 +2484,26 @@ static uint8_t* hunk_read_compressed(chd_file *chd, uint64_t offset, size_t size } else { - /* make sure it isn't larger than the compressed buffer */ + /* make sure it isn't larger than a legitimate hunk could ever be */ if (size > chd->header.hunkbytes) return NULL; +#if LOWRAM_MAP + /* grow the scratch buffer to fit this hunk's actual compressed + * length instead of always carrying a hunkbytes-sized buffer (see + * compressed_capacity). Monotonic: only grows, never shrinks, so a + * realloc failure here leaves the existing buffer/capacity intact + * and this call simply fails - the chd_file stays consistent. */ + if (size > chd->compressed_capacity) + { + uint8_t *grown = (uint8_t*)realloc(chd->compressed, size); + if (grown == NULL) + return NULL; + chd->compressed = grown; + chd->compressed_capacity = (uint32_t)size; + } +#endif + if (!seek_and_read(chd, offset, chd->compressed, size)) return NULL; return chd->compressed; From 81cb70ffdbd06857c9332fdefbe7d752a4dfdff3 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Mon, 24 Aug 2026 12:29:40 +0200 Subject: [PATCH 4/7] LOWRAM_MAP: raise default checkpoint stride 512 -> 2048 LOWRAM_MAP_CHECKPOINT_STRIDE controls the resident checkpoint table size (header.hunkcount / STRIDE entries) for the v5 compressed-map lazy-decode path. Only that path reads this macro (grepped every reference: build_v5_map_checkpoints/v5_map_get_entry/ ensure_v5_map_window in src/libchdr_chd.c) - the legacy v1-4 lazy path (map_read_one_legacy) does a direct fixed-size per-hunk seek/read with no checkpoint concept, so this change has no effect there. Measured on real RV32 QEMU hardware across 5 real local CHDs (2 PSP titles, a SegaCD title, a PCE-CD title, an HD-style BIOS image; hunkbytes 2,448-19,584, totalhunks 32,300-271,328) with mallinfo()-based peak tracking (not --wrap - see below) under a sequential + reverse + 2-seed-random read sweep: peak heap immediately after chd_open() drops 60-76% across every file (e.g. Castlevania X: 39,992 -> 13,656 bytes). Full-sweep peak improvement is smaller and file-dependent (-1.2% to -11.8%), since it's diluted by dest-buffer/decoder/scratch costs this change doesn't touch. Also measured, and reverted out of this branch: reusing the compressed- map window buffer across checkpoint-bucket crossings (realloc instead of malloc/free per crossing) to avoid allocator churn. Measured cost, not benefit - it pinned both pass1/pass2 windows at their all-time-max size simultaneously, costing +2-3.5KB peak with no measured upside (the claimed fragmentation benefit isn't observable with either measurement method used). Dropped after data disagreed with the original rationale. Measurement note: initial real-content testing at this stride value hit RV32 crashes and spurious decode failures under the --wrap=malloc/free/ realloc/calloc peak-tracking harness used earlier in this session. Traced to the harness, not this change: ASan (host x86-64 and -m32), valgrind --track-origins=yes, and doubling the firmware stack all found nothing on the identical access sequence; switching the same RV32 firmware from --wrap interposition to mallinfo().uordblks polling made every failure disappear. All numbers in this message and verified above are from the mallinfo()-based measurement. Verified byte-identical decode vs. unmodified master across the full synthetic corpus x {sequential, reverse, random} read orders, plus valgrind --leak-check=full clean, with LOWRAM_MAP on and off. --- include/libchdr/chdconfig.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/libchdr/chdconfig.h b/include/libchdr/chdconfig.h index 16197ab..08c9fa4 100644 --- a/include/libchdr/chdconfig.h +++ b/include/libchdr/chdconfig.h @@ -27,7 +27,7 @@ #endif #ifndef LOWRAM_MAP_CHECKPOINT_STRIDE -#define LOWRAM_MAP_CHECKPOINT_STRIDE 512 +#define LOWRAM_MAP_CHECKPOINT_STRIDE 2048 #endif /* Under LOWRAM_MAP, also replace each huffman_decoder's full 2^maxbits From c75a8293b30f524e25ee4a6863ba6212bff598b8 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Mon, 24 Aug 2026 13:23:26 +0200 Subject: [PATCH 5/7] Rename LOWRAM_MAP -> LOWRAM_TARGET LOWRAM_MAP no longer describes what the flag gates - it now controls three independent levers (checkpointed map decode, two-level huffman table, demand-grown compressed scratch buffer), not just the map. LOWRAM_TARGET reads correctly for all three ("this build targets low RAM"). Renamed the macro itself, both sub-knobs (LOWRAM_TARGET_CHECKPOINT_STRIDE, LOWRAM_TARGET_HUFFMAN_L1BITS), the CMake option (CHDR_LOWRAM_TARGET), and every reference across CMakeLists.txt, chdconfig.h, huffman.h, bitstream.h, libchdr_chd.c, libchdr_huffman.c, and the rv32-ram-budget CI workflow. Also rewrote chdconfig.h's LOWRAM_TARGET comment block, stale since it predated the huffman-table and scratch-buffer levers - it described only the map tradeoff. Mechanical rename, no behavior change. Verified two ways: the 64-check byte-identical corpus comparison (passes regardless of whether the rename broke the flag, since it exercises default-off either way), and a peak-RSS delta on a real file (5,272 KB off vs 1,840 KB on, castlevania X, matching the pre-rename gap) to confirm CHDR_LOWRAM_TARGET=ON still compiles the LOWRAM code path in rather than silently building the default (an unknown -D is otherwise ignored by CMake, which would make CI go green while testing nothing). Also grepped the CI workflow yml explicitly post-rename to catch exactly that failure mode. Valgrind --leak-check=full clean. --- .github/workflows/rv32-ram-budget.yml | 8 ++-- CMakeLists.txt | 8 ++-- include/libchdr/bitstream.h | 2 +- include/libchdr/chdconfig.h | 38 ++++++++++------ include/libchdr/huffman.h | 6 +-- src/libchdr_chd.c | 64 +++++++++++++-------------- src/libchdr_huffman.c | 10 ++--- 7 files changed, 73 insertions(+), 63 deletions(-) diff --git a/.github/workflows/rv32-ram-budget.yml b/.github/workflows/rv32-ram-budget.yml index 1c95d0b..7de9bf5 100644 --- a/.github/workflows/rv32-ram-budget.yml +++ b/.github/workflows/rv32-ram-budget.yml @@ -11,16 +11,16 @@ name: RV32 RAM budget # saves another ~3% via cross-TU dead-code elimination - both measured and # CRC-verified correct under qemu-system-riscv32. # -# Also builds with CHDR_LOWRAM_MAP=ON: replaces the fully-materialized +# Also builds with CHDR_LOWRAM_TARGET=ON: replaces the fully-materialized # per-hunk map (12B/hunk for CHDv5, ~24B/hunk legacy - scales with total # hunk count, independent of codec/hunkbytes) with a checkpointed on-demand # decode. Barely visible on this workflow's tiny synthetic corpus (few # hunks/file), but on real full-size discs it's the dominant RAM cost - # measured 48-74% peak-heap reduction on real GD-ROM/UMD CHDs (naomi, -# dreamcast, psp), CRC-verified byte-identical against the non-LOWRAM_MAP +# dreamcast, psp), CRC-verified byte-identical against the non-LOWRAM_TARGET # build across sequential/reverse/random-order reads. Real-ROM validation # isn't reproducible in CI (copyrighted files), so this workflow's job is -# proving LOWRAM_MAP=ON doesn't regress correctness or blow past budget on +# proving LOWRAM_TARGET=ON doesn't regress correctness or blow past budget on # what CI *can* see - the real-file numbers were measured locally. on: [push, pull_request] @@ -44,7 +44,7 @@ jobs: cmake -B build-rv32 -DCMAKE_TOOLCHAIN_FILE=${{github.workspace}}/cmake/toolchain-rv32imafc.cmake -DBUILD_SHARED_LIBS=OFF -DINSTALL_STATIC_LIBS=OFF - -DCMAKE_BUILD_TYPE=MinSizeRel -DCHDR_LOWRAM_MAP=ON + -DCMAKE_BUILD_TYPE=MinSizeRel -DCHDR_LOWRAM_TARGET=ON - name: Build chdr-static run: cmake --build build-rv32 --target chdr-static -j$(nproc) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8ca775c..6bb2846 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -11,7 +11,7 @@ option(WITH_SYSTEM_ZSTD "Use system provided zstd library" OFF) option(CHDR_WANT_RAW_DATA_SECTOR "Output ECC data and sync header" ON) option(CHDR_WANT_SUBCODE "Output CD subchannel data" ON) option(CHDR_VERIFY_BLOCK_CRC "Verify integrity of decoded data" ON) -option(CHDR_LOWRAM_MAP "Trade CPU for RAM on the per-hunk map (checkpointed on-demand decode instead of fully materializing it at chd_open) - for memory-constrained targets" OFF) +option(CHDR_LOWRAM_TARGET "Trade CPU for RAM on the per-hunk map (checkpointed on-demand decode instead of fully materializing it at chd_open) - for memory-constrained targets" OFF) option(CHDR_WANT_TESTS "Build tests for the library" ON) option(BUILD_LTO "Compile libchdr with link-time optimization if supported" OFF) @@ -93,10 +93,10 @@ else() list(APPEND CHDR_DEFINES VERIFY_BLOCK_CRC=0) endif() -if(CHDR_LOWRAM_MAP) - list(APPEND CHDR_DEFINES LOWRAM_MAP=1) +if(CHDR_LOWRAM_TARGET) + list(APPEND CHDR_DEFINES LOWRAM_TARGET=1) else() - list(APPEND CHDR_DEFINES LOWRAM_MAP=0) + list(APPEND CHDR_DEFINES LOWRAM_TARGET=0) endif() #-------------------------------------------------- diff --git a/include/libchdr/bitstream.h b/include/libchdr/bitstream.h index afc1f7e..60f2064 100644 --- a/include/libchdr/bitstream.h +++ b/include/libchdr/bitstream.h @@ -40,7 +40,7 @@ void bitstream_remove(struct bitstream* bitstream, int numbits); uint32_t bitstream_flush(struct bitstream* bitstream); /* exact bit-granular position/seek, independent of byte alignment - used to - * checkpoint/resume mid-stream decode (see LOWRAM_MAP in libchdr_chd.c) */ + * checkpoint/resume mid-stream decode (see LOWRAM_TARGET in libchdr_chd.c) */ uint64_t bitstream_position_bits(struct bitstream* bitstream); void bitstream_seek_bits(struct bitstream* bitstream, uint64_t bitpos); diff --git a/include/libchdr/chdconfig.h b/include/libchdr/chdconfig.h index 08c9fa4..c371f56 100644 --- a/include/libchdr/chdconfig.h +++ b/include/libchdr/chdconfig.h @@ -15,22 +15,32 @@ #define VERIFY_BLOCK_CRC 1 #endif -/* Trade CPU for RAM on the per-hunk map: instead of fully materializing it - * at chd_open() (12 bytes/hunk for CHDv5, ~24 bytes/hunk for legacy v1-v4 - - * scales with total hunk count, independent of codec/hunkbytes choice, and - * can reach multiple MB on full-size CD/GD-ROM/UMD images), keep only a - * sparse checkpoint index and re-derive individual entries on demand. For - * memory-constrained targets. See LOWRAM_MAP_CHECKPOINT_STRIDE to tune the - * RAM/CPU tradeoff. */ -#ifndef LOWRAM_MAP -#define LOWRAM_MAP 0 +/* Trade CPU for RAM across several independent levers, for + * memory-constrained targets (e.g. the BL616/RV32 port, 480KB SRAM): + * + * 1. Per-hunk map: instead of fully materializing it at chd_open() + * (12 bytes/hunk for CHDv5, ~24 bytes/hunk for legacy v1-v4 - scales + * with total hunk count, independent of codec/hunkbytes choice, and + * can reach multiple MB on full-size CD/GD-ROM/UMD images), keep only + * a sparse checkpoint index and re-derive individual entries on + * demand. See LOWRAM_TARGET_CHECKPOINT_STRIDE to tune the RAM/CPU + * tradeoff. + * 2. Each huffman_decoder's lookup table: a two-level table instead of + * a full 2^maxbits direct table - see LOWRAM_TARGET_HUFFMAN_L1BITS + * below. + * 3. The compressed-hunk scratch buffer (chd->compressed, + * src/libchdr_chd.c): grown on demand to the largest hunk actually + * read instead of preallocated to header.hunkbytes at chd_open(). + */ +#ifndef LOWRAM_TARGET +#define LOWRAM_TARGET 0 #endif -#ifndef LOWRAM_MAP_CHECKPOINT_STRIDE -#define LOWRAM_MAP_CHECKPOINT_STRIDE 2048 +#ifndef LOWRAM_TARGET_CHECKPOINT_STRIDE +#define LOWRAM_TARGET_CHECKPOINT_STRIDE 2048 #endif -/* Under LOWRAM_MAP, also replace each huffman_decoder's full 2^maxbits +/* Under LOWRAM_TARGET, also replace each huffman_decoder's full 2^maxbits * direct-lookup table (e.g. 128 KiB at maxbits=16, as used by AVHuff's * Y/Cb/Cr contexts and the CHD huffman codec) with a two-level table: a * 2^L1BITS first-level table, escaping to small per-prefix subtables only @@ -39,8 +49,8 @@ * unchanged; only long, rare codes pay an extra indirection. Total RAM * drops to a few KiB per decoder regardless of maxbits. */ -#ifndef LOWRAM_MAP_HUFFMAN_L1BITS -#define LOWRAM_MAP_HUFFMAN_L1BITS 10 +#ifndef LOWRAM_TARGET_HUFFMAN_L1BITS +#define LOWRAM_TARGET_HUFFMAN_L1BITS 10 #endif #endif diff --git a/include/libchdr/huffman.h b/include/libchdr/huffman.h index 7cbef43..8bfacd0 100644 --- a/include/libchdr/huffman.h +++ b/include/libchdr/huffman.h @@ -61,11 +61,11 @@ struct huffman_decoder uint8_t prevdata; /* value of the previous data (for delta-RLE encoding) */ int rleremaining; /* number of RLE bytes remaining (for delta-RLE encoding) */ lookup_value * lookup; /* pointer to the lookup table (full 2^maxbits table, - or under LOWRAM_MAP, the 2^l1bits first-level table) */ + or under LOWRAM_TARGET, the 2^l1bits first-level table) */ struct node_t * huffnode; /* array of nodes */ uint32_t * datahisto; /* histogram of data values */ -#if LOWRAM_MAP - uint8_t l1bits; /* first-level table width in bits, MIN(maxbits, LOWRAM_MAP_HUFFMAN_L1BITS) */ +#if LOWRAM_TARGET + uint8_t l1bits; /* first-level table width in bits, MIN(maxbits, LOWRAM_TARGET_HUFFMAN_L1BITS) */ lookup_value * subtable; /* concatenated second-level subtables, one per escaping first-level prefix */ uint32_t subtable_count; /* number of subtables currently allocated */ #endif diff --git a/src/libchdr_chd.c b/src/libchdr_chd.c index a68b736..0942e9b 100644 --- a/src/libchdr_chd.c +++ b/src/libchdr_chd.c @@ -206,11 +206,11 @@ struct _metadata_entry uint8_t flags; /* flag bits */ }; -#if LOWRAM_MAP +#if LOWRAM_TARGET /* one v5-map checkpoint: everything needed to resume both decode passes * (pass 1: per-hunk compression-type byte, Huffman+RLE; pass 2: per-hunk * length/offset/crc, fixed-width fields) at hunk `hunknum` without having - * decoded any of hunks [0, hunknum). See LOWRAM_MAP in chdconfig.h. */ + * decoded any of hunks [0, hunknum). See LOWRAM_TARGET in chdconfig.h. */ typedef struct _v5_map_checkpoint v5_map_checkpoint; struct _v5_map_checkpoint { @@ -291,13 +291,13 @@ struct _chd_file uint8_t * file_cache; /* cache of underlying file */ -#if LOWRAM_MAP +#if LOWRAM_TARGET v5_lowram_map lowram_map; /* CHDv5 compressed-map lazy-decode state */ /* CHDv5 can list up to 4 alternate codecs (header.compression[]); chdman * picks whichever compresses best per hunk, so a normal build must * init()/allocate all of them up front just in case a hunk needs one. - * Under LOWRAM_MAP, codecintf[] is still resolved eagerly (cheap - just + * Under LOWRAM_TARGET, codecintf[] is still resolved eagerly (cheap - just * matching a tag to a codec_interface pointer) but init() itself is * deferred to the first hunk_read_into_memory() call that actually needs * that slot - see ensure_codec_ready(). */ @@ -348,7 +348,7 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t /* internal map access */ static chd_error map_read(chd_file *chd); -#if LOWRAM_MAP +#if LOWRAM_TARGET static chd_error map_read_one_legacy(chd_file *chd, uint32_t hunknum, map_entry *entry); static chd_error build_v5_map_checkpoints(chd_file *chd, chd_header *header, uint64_t file_base, uint64_t mapbytes, uint64_t firstoffs, uint8_t lengthbits, uint8_t selfbits, uint8_t parentbits, uint16_t mapcrc); @@ -669,7 +669,7 @@ static CHDR_INLINE int map_size_v5(chd_header* header, size_t *size) /*------------------------------------------------- crc16_update - calculate CRC16 (from hashing.cpp), continuing from a prior partial - result - lets LOWRAM_MAP verify the map CRC + result - lets LOWRAM_TARGET verify the map CRC across chunks without materializing the whole buffer at once -------------------------------------------------*/ @@ -740,7 +740,7 @@ static CHDR_INLINE int chd_compressed(chd_header* header) { static chd_error decompress_v5_map(chd_file* chd, chd_header* header) { -#if !LOWRAM_MAP +#if !LOWRAM_TARGET uint32_t hunknum; int repcount = 0; uint8_t lastcomp = 0; @@ -769,7 +769,7 @@ static chd_error decompress_v5_map(chd_file* chd, chd_header* header) if ((header->mapoffset + rawmapsize) >= chd->file_size || (header->mapoffset + rawmapsize) < header->mapoffset) return CHDERR_INVALID_FILE; -#if LOWRAM_MAP +#if LOWRAM_TARGET /* not entropy-coded - each entry is independently seekable, so there's * nothing to materialize. v5_map_get_entry() reads it lazily. */ return CHDERR_NONE; @@ -797,7 +797,7 @@ static chd_error decompress_v5_map(chd_file* chd, chd_header* header) if ((header->mapoffset + mapbytes) < header->mapoffset || (header->mapoffset + mapbytes) >= chd->file_size) return CHDERR_INVALID_FILE; -#if LOWRAM_MAP +#if LOWRAM_TARGET /* build_v5_map_checkpoints() owns reading the compressed blob (in small * rolling chunks, not all mapbytes at once) and creating the huffman * decoder itself - nothing to set up here. */ @@ -957,14 +957,14 @@ static chd_error decompress_v5_map(chd_file* chd, chd_header* header) #endif } -#if LOWRAM_MAP +#if LOWRAM_TARGET /*------------------------------------------------- build_v5_map_checkpoints - run both v5 map decode passes once (same as decompress_v5_map's - non-LOWRAM_MAP path), but instead of writing a + non-LOWRAM_TARGET path), but instead of writing a 12-byte entry per hunk into a fully materialized buffer, record a resumable checkpoint every - LOWRAM_MAP_CHECKPOINT_STRIDE hunks. Verifies the + LOWRAM_TARGET_CHECKPOINT_STRIDE hunks. Verifies the same map CRC as the normal path, computed incrementally instead of over one big buffer. @@ -1067,7 +1067,7 @@ static chd_error build_v5_map_checkpoints(chd_file *chd, chd_header *header, uin uint64_t pass1_symbols_start_bits; chd_error err; - checkpoint_capacity = header->hunkcount / LOWRAM_MAP_CHECKPOINT_STRIDE + 1; + checkpoint_capacity = header->hunkcount / LOWRAM_TARGET_CHECKPOINT_STRIDE + 1; checkpoints = (v5_map_checkpoint*)malloc(sizeof(v5_map_checkpoint) * checkpoint_capacity); if (checkpoints == NULL) return CHDERR_OUT_OF_MEMORY; @@ -1095,7 +1095,7 @@ static chd_error build_v5_map_checkpoints(chd_file *chd, chd_header *header, uin checkpoint_count = 0; for (hunknum = 0; hunknum < header->hunkcount; hunknum++) { - if (hunknum % LOWRAM_MAP_CHECKPOINT_STRIDE == 0) + if (hunknum % LOWRAM_TARGET_CHECKPOINT_STRIDE == 0) { checkpoints[checkpoint_count].hunknum = hunknum; checkpoints[checkpoint_count].pass1_bitpos = v5_build_stream_position_bits(&pass1); @@ -1169,7 +1169,7 @@ static chd_error build_v5_map_checkpoints(chd_file *chd, chd_header *header, uin uint16_t crc = 0; uint8_t entry[12]; - if (hunknum % LOWRAM_MAP_CHECKPOINT_STRIDE == 0) + if (hunknum % LOWRAM_TARGET_CHECKPOINT_STRIDE == 0) { checkpoints[checkpoint_count].pass2_bitpos = v5_build_stream_position_bits(&pass2); checkpoints[checkpoint_count].curoffset = curoffset; @@ -1281,7 +1281,7 @@ static chd_error build_v5_map_checkpoints(chd_file *chd, chd_header *header, uin checkpoint `cpidx`. A no-op if it already does (the common case for sequential access - most lookups stay within the same checkpoint bucket, - up to LOWRAM_MAP_CHECKPOINT_STRIDE hunks); re-reads + up to LOWRAM_TARGET_CHECKPOINT_STRIDE hunks); re-reads just that small range from the file otherwise. -------------------------------------------------*/ @@ -1512,7 +1512,7 @@ static chd_error ensure_codec_ready(chd_file *chd, size_t slot, void *codec) chd->codec_lazy_initialized[slot] = 1; return CHDERR_NONE; } -#endif /* LOWRAM_MAP */ +#endif /* LOWRAM_TARGET */ /*------------------------------------------------- map_extract_old - extract a single map @@ -1661,7 +1661,7 @@ CHD_EXPORT chd_error chd_open_core_file_callbacks(const core_file_callbacks *cal if (err != CHDERR_NONE) EARLY_EXIT(err); -#if LOWRAM_MAP +#if LOWRAM_TARGET /* grown on demand in hunk_read_compressed() instead of preallocated to * the worst case (header.hunkbytes) here - see compressed_capacity. */ newchd->compressed = NULL; @@ -1700,7 +1700,7 @@ CHD_EXPORT chd_error chd_open_core_file_callbacks(const core_file_callbacks *cal else { size_t decompnum; -#if !LOWRAM_MAP +#if !LOWRAM_TARGET int needsinit; #endif @@ -1720,7 +1720,7 @@ CHD_EXPORT chd_error chd_open_core_file_callbacks(const core_file_callbacks *cal if (newchd->codecintf[decompnum] == NULL && newchd->header.compression[decompnum] != 0) EARLY_EXIT(err = CHDERR_UNSUPPORTED_FORMAT); -#if !LOWRAM_MAP +#if !LOWRAM_TARGET /* ensure we don't try to initialize the same codec twice */ /* this is "normal" for chds where the user overrides the codecs, it'll have none repeated */ needsinit = (newchd->codecintf[decompnum]->init != NULL); @@ -1951,7 +1951,7 @@ CHD_EXPORT void chd_close(chd_file *chd) if (codec) { -#if LOWRAM_MAP +#if LOWRAM_TARGET /* never lazily init()ed (no hunk ever selected this slot) - * most codec free() implementations assume init() ran first */ if (chd->codec_lazy_initialized[i]) @@ -1964,7 +1964,7 @@ CHD_EXPORT void chd_close(chd_file *chd) if (chd->header.rawmap != NULL) free(chd->header.rawmap); -#if LOWRAM_MAP +#if LOWRAM_TARGET if (chd->lowram_map.pass1_window.data != NULL) free(chd->lowram_map.pass1_window.data); if (chd->lowram_map.pass2_window.data != NULL) @@ -2488,7 +2488,7 @@ static uint8_t* hunk_read_compressed(chd_file *chd, uint64_t offset, size_t size if (size > chd->header.hunkbytes) return NULL; -#if LOWRAM_MAP +#if LOWRAM_TARGET /* grow the scratch buffer to fit this hunk's actual compressed * length instead of always carrying a hunkbytes-sized buffer (see * compressed_capacity). Monotonic: only grows, never shrinks, so a @@ -2554,7 +2554,7 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t if (chd->header.version < 5) { -#if LOWRAM_MAP +#if LOWRAM_TARGET map_entry entry_storage; map_entry *entry = &entry_storage; if ((err = map_read_one_legacy(chd, hunknum, entry)) != CHDERR_NONE) @@ -2624,7 +2624,7 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t #if VERIFY_BLOCK_CRC uint16_t blockcrc; #endif -#if LOWRAM_MAP +#if LOWRAM_TARGET uint8_t rawmap_storage[12]; uint8_t *rawmap = rawmap_storage; if ((err = v5_map_get_entry(chd, hunknum, rawmap)) != CHDERR_NONE) @@ -2711,7 +2711,7 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t } if (codec==NULL) return CHDERR_CODEC_ERROR; -#if LOWRAM_MAP +#if LOWRAM_TARGET if ((err = ensure_codec_ready(chd, rawmap[0], codec)) != CHDERR_NONE) return err; #endif @@ -2794,17 +2794,17 @@ static chd_error map_read(chd_file *chd) uint8_t cookie[MAP_ENTRY_SIZE]; chd_error err; uint32_t i; -#if LOWRAM_MAP +#if LOWRAM_TARGET map_entry extracted[MAP_STACK_ENTRIES]; #endif /* legacy (v1-v4) map entries are fixed-size and independently seekable - - * under LOWRAM_MAP, don't materialize the whole array, just validate it + * under LOWRAM_TARGET, don't materialize the whole array, just validate it * (cookie + maxoffset, same as always) and re-read the single entry * needed on each hunk_read_into_memory() call instead. chd->map stays * NULL; hunknum*entrysize + chd->header.length recovers any entry's * file offset without storing anything. */ -#if !LOWRAM_MAP +#if !LOWRAM_TARGET chd->map = (map_entry *)malloc(sizeof(chd->map[0]) * chd->header.totalhunks); if (!chd->map) return CHDERR_OUT_OF_MEMORY; @@ -2816,7 +2816,7 @@ static chd_error map_read(chd_file *chd) { /* compute how many entries this time */ int entries = chd->header.totalhunks - i, j; -#if !LOWRAM_MAP +#if !LOWRAM_TARGET map_entry *dest = &chd->map[i]; #else map_entry *dest = extracted; @@ -2858,7 +2858,7 @@ static chd_error map_read(chd_file *chd) return CHDERR_NONE; cleanup: -#if !LOWRAM_MAP +#if !LOWRAM_TARGET if (chd->map) free(chd->map); chd->map = NULL; @@ -2866,7 +2866,7 @@ static chd_error map_read(chd_file *chd) return err; } -#if LOWRAM_MAP +#if LOWRAM_TARGET /*------------------------------------------------- map_read_one_legacy - lazily fetch a single v1-v4 map entry directly from the file diff --git a/src/libchdr_huffman.c b/src/libchdr_huffman.c index 3180201..b22380b 100644 --- a/src/libchdr_huffman.c +++ b/src/libchdr_huffman.c @@ -132,8 +132,8 @@ struct huffman_decoder* create_huffman_decoder(int numcodes, int maxbits) decoder = (struct huffman_decoder*)malloc(sizeof(struct huffman_decoder)); decoder->numcodes = numcodes; decoder->maxbits = maxbits; -#if LOWRAM_MAP - decoder->l1bits = MIN(maxbits, LOWRAM_MAP_HUFFMAN_L1BITS); +#if LOWRAM_TARGET + decoder->l1bits = MIN(maxbits, LOWRAM_TARGET_HUFFMAN_L1BITS); decoder->lookup = (lookup_value*)malloc(sizeof(lookup_value) * (1u << decoder->l1bits)); decoder->subtable = NULL; decoder->subtable_count = 0; @@ -153,7 +153,7 @@ void delete_huffman_decoder(struct huffman_decoder* decoder) { if (decoder->lookup != NULL) free(decoder->lookup); -#if LOWRAM_MAP +#if LOWRAM_TARGET if (decoder->subtable != NULL) free(decoder->subtable); #endif @@ -175,7 +175,7 @@ uint32_t huffman_decode_one(struct huffman_decoder* decoder, struct bitstream* b uint32_t window = bitstream_peek(bitbuf, decoder->maxbits); lookup_value lookup; -#if LOWRAM_MAP +#if LOWRAM_TARGET /* two-level lookup: common (short-code) case is one table load, same as * the full-table path below; only codes longer than l1bits pay for a * second indirection into a small per-prefix subtable. */ @@ -562,7 +562,7 @@ enum huffman_error huffman_assign_canonical_codes(struct huffman_decoder* decode *------------------------------------------------- */ -#if LOWRAM_MAP +#if LOWRAM_TARGET enum huffman_error huffman_build_lookup_table(struct huffman_decoder* decoder) { uint32_t l1bits = decoder->l1bits; From c59a79dcb196b4e27eeb74e9d8b71eba15f486e5 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Mon, 24 Aug 2026 18:29:12 +0200 Subject: [PATCH 6/7] LOWRAM_TARGET: O(1) sequential-access fast path for the v5 map v5_map_get_entry() previously resumed from the nearest checkpoint on every single call - O(stride/2) huffman+RLE decode work per lookup on average (stride now defaults to 2048, so up to ~1024 hunks of re-decode per lookup in the worst case). For sequential access, the access pattern that dominates real playback, that's wasted work: the previous call already decoded up to hunknum-1 and left bs1/bs2 sitting exactly where hunknum's own decode needs to continue from. Cache that in-flight state (bitstream positions, lastcomp/repcount/ curoffset/last_self/last_parent, and the next expected hunknum) and reuse it when the next call asks for exactly that hunk. Falls back to the existing checkpoint-based decode on any cache miss (non-sequential access, first call, or a checkpoint-bucket crossing - see below) - strictly additive, the slow path is untouched. Fixed-size cost, no growth: one v5_resume_cache struct per chd_file, 96 bytes on x86-64 / 80 bytes on ilp32 (RV32/32-bit hosts), independent of file size or hunk count. Real host benchmark (5 real local CHDs, full sequential decode, same files/methodology as the earlier stride commit): closes most or all of the throughput regression LOWRAM_TARGET introduced. Castlevania X (the worst case: 271,328 small hunks, lookup-dominated) improves from -38% vs LOWRAM_TARGET=off to -22%. Mega Man Maverick Hunter X, Battlecorps, and Bonk III now measure LOWRAM_TARGET=on as fast as or faster than off outright. mac755 remains slower (-27%) - it's the one file of the five that uses the CD huffman codec for actual payload decode (not just the map's compression-type stream), so it pays the two-level lookup table's extra indirection on every decoded byte, not just on map lookups; unrelated to this change, not investigated further here. Bug caught during verification, not from static review: an initial version had no bucket-boundary check, so sequential access silently corrupted the decode at every checkpoint crossing. pass1_window/ pass2_window are each sized to exactly one checkpoint bucket's byte range; continuing to read past that range via cached bitstream state doesn't error, it silently reads zeros once bitstream_peek's doffset >= dlength (its bounds check just stops supplying real bytes, it's not equipped to signal "need more data"). Missed by the 64-check synthetic corpus (too few hunks per file to cross a stride-2048 boundary) but caught immediately by an ASan sweep against 5 real local files - "decompression error"/"requires parent" starting consistently around the first checkpoint boundary (~hunk 4096) on every file. Fixed by tracking each cached state's next_boundary (the first hunknum outside its checkpoint bucket) and forcing the slow path - which correctly refetches both windows - whenever hunknum reaches it. Verified after the fix: 80/80 byte-identical checks (5 corpora orders x LOWRAM_TARGET on/off) vs. unmodified master, valgrind --leak-check=full clean, zero ASan/UBSan findings re-running the exact real-file sweep that caught the bug, plus full (uncapped) sequential/reverse/random decode of two real files (271,328 and 32,300 hunks - dozens of checkpoint-boundary crossings each) byte-identical against master. --- src/libchdr_chd.c | 140 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 106 insertions(+), 34 deletions(-) diff --git a/src/libchdr_chd.c b/src/libchdr_chd.c index 0942e9b..f16a0fe 100644 --- a/src/libchdr_chd.c +++ b/src/libchdr_chd.c @@ -238,6 +238,40 @@ struct _v5_map_window uint32_t checkpoint_idx; /* which checkpoint this window covers; UINT32_MAX if none cached */ }; +/* v5_map_get_entry() normally resumes from the nearest checkpoint on every + * call - O(stride/2) huffman+RLE work per lookup on average. For sequential + * access (the common case: playback reading hunk N, N+1, N+2, ...) that's + * wasted work, since the previous call already decoded up to hunknum-1 and + * left bs1/bs2 sitting exactly where hunknum's own decode needs to start. + * Caching that in-flight state turns the sequential case into O(1) per + * lookup instead. bs1/bs2's `read` pointers alias pass1_window/pass2_window + * - safe to cache as long as nothing reallocates those buffers in between, + * which holds precisely because this fast path never calls + * ensure_v5_map_window() itself (see v5_map_get_entry()). Fixed-size, no + * growth - a few dozen bytes, independent of file size or hunk count. + * + * `next_boundary` exists because pass1_window/pass2_window are each sized + * to exactly one checkpoint bucket's byte range - continuing to read past + * that range without refetching would silently read past bitstream->dlength + * (bitstream_peek's bounds check just stops supplying real bytes at that + * point, it doesn't error), corrupting the decode right at every bucket + * crossing. Forcing hunknum == next_boundary back onto the slow path - + * which correctly refetches both windows for the new bucket - is what + * keeps this fast path safe. */ +typedef struct _v5_resume_cache v5_resume_cache; +struct _v5_resume_cache +{ + uint8_t valid; /* 0 until the first successful decode populates this */ + uint32_t cur_hunk; /* next hunknum this state can resume into */ + uint32_t next_boundary; /* first hunknum outside the current checkpoint bucket */ + struct bitstream bs1, bs2; + uint8_t lastcomp; + int32_t repcount; + uint64_t curoffset; + uint32_t last_self; + uint64_t last_parent; +}; + /* resident state for lazily re-deriving v5 map entries. Replaces the fully * materialized `header->rawmap` (totalhunks*12 bytes) with a sparse * checkpoint table plus, per lookup, a small on-demand window of the @@ -257,6 +291,7 @@ struct _v5_lowram_map * a single fixed point for the whole file, not per-checkpoint */ v5_map_window pass1_window; v5_map_window pass2_window; + v5_resume_cache resume; /* sequential-access fast path, see v5_resume_cache */ }; #endif @@ -1330,7 +1365,7 @@ static chd_error v5_map_get_entry(chd_file *chd, uint32_t hunknum, uint8_t out[1 v5_lowram_map *lm = &chd->lowram_map; chd_header *header = &chd->header; struct bitstream bs1, bs2; - uint32_t cur_hunk; + uint32_t cur_hunk, start_hunk, next_boundary; uint8_t lastcomp; int32_t repcount; uint64_t curoffset; @@ -1351,43 +1386,66 @@ static chd_error v5_map_get_entry(chd_file *chd, uint32_t hunknum, uint8_t out[1 if (hunknum >= header->hunkcount || lm->checkpoint_count == 0) return CHDERR_INVALID_PARAMETER; - /* checkpoints are hunknum-ascending; find the nearest one <= hunknum */ - cpidx = 0; - for (i = 1; i < lm->checkpoint_count && lm->checkpoints[i].hunknum <= hunknum; i++) - cpidx = i; - + if (lm->resume.valid && lm->resume.cur_hunk == hunknum && hunknum < lm->resume.next_boundary) { - uint64_t pass1_start_bit = lm->checkpoints[cpidx].pass1_bitpos; - uint64_t pass1_end_bit = (cpidx + 1 < lm->checkpoint_count) ? - lm->checkpoints[cpidx + 1].pass1_bitpos : lm->pass1_end_bitpos; - uint64_t pass2_start_bit = lm->checkpoints[cpidx].pass2_bitpos; - uint64_t pass2_end_bit = (cpidx + 1 < lm->checkpoint_count) ? - lm->checkpoints[cpidx + 1].pass2_bitpos : (uint64_t)lm->mapbytes * 8; - chd_error werr; - - if ((werr = ensure_v5_map_window(chd, &lm->pass1_window, pass1_start_bit / 8, - (pass1_end_bit + 7) / 8, cpidx)) != CHDERR_NONE) - return werr; - if ((werr = ensure_v5_map_window(chd, &lm->pass2_window, pass2_start_bit / 8, - (pass2_end_bit + 7) / 8, cpidx)) != CHDERR_NONE) - return werr; - - bs1.buffer = 0; bs1.bits = 0; bs1.read = lm->pass1_window.data; bs1.doffset = 0; - bs1.dlength = lm->pass1_window.byte_len; - bitstream_seek_bits(&bs1, pass1_start_bit - (uint64_t)lm->pass1_window.byte_start * 8); - - bs2.buffer = 0; bs2.bits = 0; bs2.read = lm->pass2_window.data; bs2.doffset = 0; - bs2.dlength = lm->pass2_window.byte_len; - bitstream_seek_bits(&bs2, pass2_start_bit - (uint64_t)lm->pass2_window.byte_start * 8); + /* fast path: the previous call already decoded up to hunknum-1 and + * left this exact state ready to continue from hunknum - skip the + * checkpoint lookup and window setup entirely. Safe to reuse + * bs1/bs2 as-is: nothing reallocates pass1_window/pass2_window + * between one call finishing and the next one starting, since this + * path never calls ensure_v5_map_window(). */ + bs1 = lm->resume.bs1; + bs2 = lm->resume.bs2; + lastcomp = lm->resume.lastcomp; + repcount = lm->resume.repcount; + curoffset = lm->resume.curoffset; + last_self = lm->resume.last_self; + last_parent = lm->resume.last_parent; + start_hunk = hunknum; + next_boundary = lm->resume.next_boundary; } + else + { + /* checkpoints are hunknum-ascending; find the nearest one <= hunknum */ + cpidx = 0; + for (i = 1; i < lm->checkpoint_count && lm->checkpoints[i].hunknum <= hunknum; i++) + cpidx = i; - lastcomp = lm->checkpoints[cpidx].lastcomp; - repcount = lm->checkpoints[cpidx].repcount; - curoffset = lm->checkpoints[cpidx].curoffset; - last_self = lm->checkpoints[cpidx].last_self; - last_parent = lm->checkpoints[cpidx].last_parent; + { + uint64_t pass1_start_bit = lm->checkpoints[cpidx].pass1_bitpos; + uint64_t pass1_end_bit = (cpidx + 1 < lm->checkpoint_count) ? + lm->checkpoints[cpidx + 1].pass1_bitpos : lm->pass1_end_bitpos; + uint64_t pass2_start_bit = lm->checkpoints[cpidx].pass2_bitpos; + uint64_t pass2_end_bit = (cpidx + 1 < lm->checkpoint_count) ? + lm->checkpoints[cpidx + 1].pass2_bitpos : (uint64_t)lm->mapbytes * 8; + chd_error werr; + + if ((werr = ensure_v5_map_window(chd, &lm->pass1_window, pass1_start_bit / 8, + (pass1_end_bit + 7) / 8, cpidx)) != CHDERR_NONE) + return werr; + if ((werr = ensure_v5_map_window(chd, &lm->pass2_window, pass2_start_bit / 8, + (pass2_end_bit + 7) / 8, cpidx)) != CHDERR_NONE) + return werr; + + bs1.buffer = 0; bs1.bits = 0; bs1.read = lm->pass1_window.data; bs1.doffset = 0; + bs1.dlength = lm->pass1_window.byte_len; + bitstream_seek_bits(&bs1, pass1_start_bit - (uint64_t)lm->pass1_window.byte_start * 8); + + bs2.buffer = 0; bs2.bits = 0; bs2.read = lm->pass2_window.data; bs2.doffset = 0; + bs2.dlength = lm->pass2_window.byte_len; + bitstream_seek_bits(&bs2, pass2_start_bit - (uint64_t)lm->pass2_window.byte_start * 8); + } - for (cur_hunk = lm->checkpoints[cpidx].hunknum; cur_hunk <= hunknum; cur_hunk++) + lastcomp = lm->checkpoints[cpidx].lastcomp; + repcount = lm->checkpoints[cpidx].repcount; + curoffset = lm->checkpoints[cpidx].curoffset; + last_self = lm->checkpoints[cpidx].last_self; + last_parent = lm->checkpoints[cpidx].last_parent; + start_hunk = lm->checkpoints[cpidx].hunknum; + next_boundary = (cpidx + 1 < lm->checkpoint_count) ? lm->checkpoints[cpidx + 1].hunknum : header->hunkcount; + } + + for (cur_hunk = start_hunk; cur_hunk <= hunknum; cur_hunk++) { uint8_t type; uint64_t offset; @@ -1469,6 +1527,20 @@ static chd_error v5_map_get_entry(chd_file *chd, uint32_t hunknum, uint8_t out[1 } } + /* cache resumable state for a potential hunknum+1 fast-path call. Only + * reached after a fully successful decode - any early error return + * above leaves the previous cache entry untouched. */ + lm->resume.valid = 1; + lm->resume.cur_hunk = hunknum + 1; + lm->resume.next_boundary = next_boundary; + lm->resume.bs1 = bs1; + lm->resume.bs2 = bs2; + lm->resume.lastcomp = lastcomp; + lm->resume.repcount = repcount; + lm->resume.curoffset = curoffset; + lm->resume.last_self = last_self; + lm->resume.last_parent = last_parent; + return CHDERR_NONE; } From 0015a869f247e1c3bd42598fdd58fd50c3b078b0 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Mon, 24 Aug 2026 21:52:36 +0200 Subject: [PATCH 7/7] Add CI regression test for the LOWRAM_TARGET checkpoint-boundary bug The resume-cache fast path added in c59a79d silently corrupted decode at every LOWRAM_TARGET_CHECKPOINT_STRIDE checkpoint boundary until a next_boundary check was added to fix it (same commit). That bug was found by a manual ASan sweep against large real-world CHDs this session, not by anything in the repo - the existing synthetic corpus (largest seed: a few hundred hunks) is too small to ever cross a stride-2048 boundary, so nothing here would have caught it, and nothing stops the same bug class coming back silently. Adds: - tests/chd_dump_order.c: reads a CHD's hunks in a given order (sequential/reverse/random:SEED) and dumps them to stdout, so two builds can be diffed byte-for-byte instead of only relying on the internal per-hunk CRC check. - tests/corpus/generate.sh: a new raw_boundary.chd seed - 2,200 hunks of 64 B (140,800 B raw), deliberately sized to cross the default 2048-hunk checkpoint boundary. Every other seed in this corpus is too small to. - .github/workflows/lowram-correctness.yml: builds default config and CHDR_LOWRAM_TARGET=ON, diffs chd_dump_order's output for every corpus seed x {sequential, reverse, random:1, random:42}, fails on any mismatch. Verified the fixture actually catches the class of bug it's meant to catch, not just that it currently passes: reverted only the next_boundary check locally and reran - raw_boundary.chd's sequential order fails immediately (decompression error at hunk 2048, the exact checkpoint boundary), every other seed/order stays green. Confirms this would have caught the original bug and will catch a regression of it. Restored the fix, reverified 68/68 clean before committing. --- .github/workflows/lowram-correctness.yml | 61 ++++++++++++++++++ tests/CMakeLists.txt | 7 ++ tests/chd_dump_order.c | 81 ++++++++++++++++++++++++ tests/corpus/generate.sh | 17 +++++ 4 files changed, 166 insertions(+) create mode 100644 .github/workflows/lowram-correctness.yml create mode 100644 tests/chd_dump_order.c diff --git a/.github/workflows/lowram-correctness.yml b/.github/workflows/lowram-correctness.yml new file mode 100644 index 0000000..6654ed1 --- /dev/null +++ b/.github/workflows/lowram-correctness.yml @@ -0,0 +1,61 @@ +name: LOWRAM_TARGET correctness + +# Builds libchdr twice (default config and CHDR_LOWRAM_TARGET=ON) and diffs +# decoded output byte-for-byte across the full corpus in several read +# orders, instead of only relying on the internal per-hunk CRC check. +# +# This exists because of a real bug: LOWRAM_TARGET's sequential resume-cache +# (v5_map_get_entry's fast path) silently corrupted the decode at every +# LOWRAM_TARGET_CHECKPOINT_STRIDE checkpoint boundary until a next_boundary +# check was added. The existing synthetic corpus is too small to ever cross +# a stride-2048 boundary, so it did not catch this - only a manual sweep +# against large real-world CHDs did. tests/corpus/generate.sh's +# raw_boundary.chd (2,200 hunks of 64 B, deliberately crossing the default +# stride) closes that gap; this workflow is what actually runs it on every +# push/PR. + +on: [push, pull_request] + +jobs: + lowram-correctness: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v7 + + - name: Install chdman + run: sudo apt-get update -qq && sudo apt-get install -y --no-install-recommends mame-tools + + - name: Generate corpus seeds + run: tests/corpus/generate.sh + + - name: Build (default config) + run: | + cmake -B build-default -DCMAKE_BUILD_TYPE=Release + cmake --build build-default --target chd_dump_order -j$(nproc) + + - name: Build (CHDR_LOWRAM_TARGET=ON) + run: | + cmake -B build-lowram -DCMAKE_BUILD_TYPE=Release -DCHDR_LOWRAM_TARGET=ON + cmake --build build-lowram --target chd_dump_order -j$(nproc) + + - name: Byte-identical decode, every seed x every read order + run: | + set -euo pipefail + D=build-default/tests/chd_dump_order + L=build-lowram/tests/chd_dump_order + fail=0 + total=0 + for f in tests/corpus/seeds/*.chd; do + for order in sequential reverse random:1 random:42; do + total=$((total+1)) + "$D" "$f" "$order" > /tmp/d.bin + "$L" "$f" "$order" > /tmp/l.bin + if ! cmp -s /tmp/d.bin /tmp/l.bin; then + echo "MISMATCH: $(basename "$f") $order" + fail=$((fail+1)) + fi + done + done + echo "$total checks, $fail mismatches" + [ "$fail" -eq 0 ] diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b4da3ed..c8496ae 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,6 +1,13 @@ add_executable(chdr-benchmark benchmark.c) target_link_libraries(chdr-benchmark PRIVATE chdr-static) +# reads a CHD's hunks in a given order and dumps them to stdout, so two +# builds (e.g. LOWRAM_TARGET on vs off) can be diffed byte-for-byte instead +# of only relying on the internal per-hunk CRC check - see +# lowram-correctness.yml +add_executable(chd_dump_order chd_dump_order.c) +target_link_libraries(chd_dump_order PRIVATE chdr-static) + # fuzzing if(BUILD_FUZZER) add_executable(chdr-fuzz fuzz.c) diff --git a/tests/chd_dump_order.c b/tests/chd_dump_order.c new file mode 100644 index 0000000..b2aac84 --- /dev/null +++ b/tests/chd_dump_order.c @@ -0,0 +1,81 @@ +/* Reads a CHD's hunks in a given order (sequential, reverse, or a random + * permutation seeded from argv) and writes them concatenated to stdout, so + * two builds (e.g. LOWRAM_MAP=0 vs =1) can be diffed byte-for-byte - not + * just relying on the internal CRC check. */ +#include +#include +#include +#include + +int main(int argc, char **argv) +{ + if (argc < 3) { + fprintf(stderr, "usage: %s [max_hunks]\n", argv[0]); + return 2; + } + + chd_file *chd = NULL; + chd_error err = chd_open(argv[1], CHD_OPEN_READ, NULL, &chd); + if (err != CHDERR_NONE) { + fprintf(stderr, "chd_open: %s\n", chd_error_string(err)); + return 1; + } + + const chd_header *h = chd_get_header(chd); + uint32_t n = h->totalhunks; + uint32_t *order; + + if (strncmp(argv[2], "sample:", 7) == 0) { + /* scattered sample across the FULL [0,totalhunks) range - for large + * real-world files, exercises many checkpoints without decoding the + * whole (possibly multi-GB) file. format: sample:COUNT:SEED */ + char *rest = argv[2] + 7; + uint32_t count = (uint32_t)strtoul(rest, &rest, 10); + unsigned seed = (rest[0] == ':') ? (unsigned)strtoul(rest + 1, NULL, 10) : 0; + if (count > n) count = n; + srand(seed); + order = malloc(sizeof(uint32_t) * count); + for (uint32_t i = 0; i < count; i++) + order[i] = (uint32_t)((double)rand() / ((double)RAND_MAX + 1) * n); + n = count; + } else { + if (argc >= 4) { + uint32_t cap = (uint32_t)strtoul(argv[3], NULL, 10); + if (cap < n) n = cap; + } + order = malloc(sizeof(uint32_t) * n); + for (uint32_t i = 0; i < n; i++) order[i] = i; + + if (strncmp(argv[2], "reverse", 7) == 0) { + for (uint32_t i = 0; i < n; i++) order[i] = n - 1 - i; + } else if (strncmp(argv[2], "random:", 7) == 0) { + unsigned seed = (unsigned)strtoul(argv[2] + 7, NULL, 10); + srand(seed); + for (uint32_t i = n; i > 1; i--) { + uint32_t j = rand() % i; + uint32_t tmp = order[i - 1]; + order[i - 1] = order[j]; + order[j] = tmp; + } + } + /* else: sequential, already set up */ + } + + uint8_t *buf = malloc(h->hunkbytes); + for (uint32_t i = 0; i < n; i++) { + err = chd_read(chd, order[i], buf); + if (err != CHDERR_NONE) { + fprintf(stderr, "chd_read(hunk %u) failed: %s\n", order[i], chd_error_string(err)); + return 1; + } + /* prefix each hunk with its logical index so the diff pinpoints which + * hunk mismatched, regardless of read order */ + fwrite(&order[i], sizeof(uint32_t), 1, stdout); + fwrite(buf, 1, h->hunkbytes, stdout); + } + + free(buf); + free(order); + chd_close(chd); + return 0; +} diff --git a/tests/corpus/generate.sh b/tests/corpus/generate.sh index 4d04547..35023a4 100755 --- a/tests/corpus/generate.sh +++ b/tests/corpus/generate.sh @@ -33,6 +33,15 @@ EOF RAW="$TMP/tiny.raw" dd if=/dev/urandom of="$RAW" bs=4096 count=16 status=none +# Raw for a small-hunk, many-hunk file - 2,200 hunks of 64 B (140,800 B +# total) specifically to cross LOWRAM_TARGET_CHECKPOINT_STRIDE's default +# 2048-hunk checkpoint boundary. LOWRAM_TARGET's sequential resume-cache +# only exercises its bucket-boundary handling when a real checkpoint +# crossing happens mid-file - every other seed here is too small to ever +# hit one. See lowram-correctness.yml. +RAW_BOUNDARY="$TMP/boundary.raw" +dd if=/dev/urandom of="$RAW_BOUNDARY" bs=64 count=2200 status=none + create_hd () { local name="$1"; shift chdman createhd -f -o "$CORPUS/$name" -i "$RAW_HD" --chs 4,16,2 -ss 512 "$@" >/dev/null 2>&1 || true @@ -48,6 +57,11 @@ create_raw () { chdman createraw -f -o "$CORPUS/$name" -i "$RAW" -hs 4096 -us 512 "$@" >/dev/null 2>&1 || true } +create_raw_boundary () { + local name="$1"; shift + chdman createraw -f -o "$CORPUS/$name" -i "$RAW_BOUNDARY" -hs 64 -us 64 "$@" >/dev/null 2>&1 || true +} + # Hard disk: default codecs + flavor variants. create_hd hd_default.chd create_hd hd_none.chd -c none @@ -70,6 +84,9 @@ create_raw raw_default.chd create_raw raw_none.chd -c none create_raw raw_zstd.chd -c zstd +# Raw, many small hunks - crosses a LOWRAM_TARGET checkpoint boundary. +create_raw_boundary raw_boundary.chd -c zstd + # Summary. echo "generated $(ls -1 "$CORPUS"/*.chd 2>/dev/null | wc -l) CHD samples:" ls -lhS "$CORPUS"/*.chd 2>/dev/null | awk '{print " " $5 " " $NF}' | sed "s|$CORPUS/||"