Declare the real FLAC block size in the synthesised STREAMINFO - #188
Merged
Conversation
The header libchdr builds ahead of a FLAC stream put block_size * channels in the block-size fields, but STREAMINFO counts inter-channel samples, so for stereo it claimed twice the real maximum. dr_flac sizes its decoded-sample buffer from that value, so every FLAC decoder instance carried a buffer twice the size it needs. Over-declaring is otherwise harmless - the value only sizes that buffer and rejects frames larger than it - which is why this survived. The corrected value is exact, not merely safe: chdman picks the encoder's block size with the same function the codecs here call to derive this argument (chd_cd_flac_compressor::blocksize and chd_flac_compressor::blocksize in MAME's chdcodec.cpp), and MAME's own decoder writes block_size unmultiplied. The AVHuff path passes one channel, so it is unaffected either way. Peak heap on rv32imafc under qemu, LOWRAM_TARGET=ON: cd_cdfl 95145 -> 76329 (-18816, -19.8%) hd_flac 32530 -> 24338 ( -8192, -25.2%) Neither codec was in the RAM-budget firmware, which is how an oversized allocation in the largest per-instance buffer of any codec went unmeasured; both are added here, across all three budget targets. Every other codec's peak is unchanged to the byte. Decoded output is byte-identical across the seed corpus, the v3/v4 fixtures, three real CHDv4 images, six real discs and an AVHuff image, in the default build, with CHDR_CD_SCRATCH_BUFFER=OFF and with CHDR_LOWRAM_TARGET=ON - 108 comparisons, no difference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01846EhHuAFk5qvxwEA5Gq6y
hd_default happens to contain flac hunks because flac is one of chdman's default hard-disk codecs, but relying on a default to cover a codec is how a codec stops being covered. The RAM-budget firmware needs the seed by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01846EhHuAFk5qvxwEA5Gq6y
This was referenced Sep 10, 2026
pull Bot
pushed a commit
to sysfce2/libchdr
that referenced
this pull request
Sep 10, 2026
libchdr's own default stays dr_flac, because a desktop consumer vendoring src/ must not have to fetch anything and must not be handed a C++ dependency. An MCU integrator is already cloning an SDK, a toolchain and a filesystem library, so one more pinned checkout costs them nothing - and on every board this was tried on, micro-flac is faster, uses less peak heap on most images, and leaves a larger worst-case free block. Measured, all post-rtissera#188. ESP32-S3 with I/O excluded: 1.198x on a CD-FLAC hunk, 1.233x on raw FLAC. Eleven real discs on the same board: 1.072x overall, 1.21x where the image is FLAC-heavy, 0.988x on one profile where FLAC barely appears. RP2350 Cortex-M33, seven real discs, two runs each within 0.17%: 1.032x overall, 1.12-1.14x on two of them, worst-case largest-free-block 51 KB against dr_flac's 47 KB. BL616 and ESP32-P4 have not been measured - said so in both READMEs. The three targets now fetch micro-flac at a pinned commit when no local checkout is given, the same shape libchdr's root CMakeLists already uses. CHDR_FLAC_BACKEND=drflac goes back, and CI builds both ways on all three so neither path rots. Two things this ran into: FetchContent's SOURCE_SUBDIR names a directory that does not exist on purpose. Without it MakeAvailable runs micro-flac's own CMakeLists, which configures its targets for the host and joins them to a cross build - visible as "microFLAC: Building for host platform" in the RP2350 configure. ESP-IDF runs every component CMakeLists in script mode first to collect REQUIRES, and FetchContent is not scriptable there: define_property fails. The backend does not affect requirements, so that pass takes the in-tree source and the fetch happens once, in the real configure. On BL616 the integration patch now carries micro-flac, and firmware-bl616-libchdr-drflac.patch reverts it for anyone Apache-2.0 does not suit. Both were generated against the pinned firmware-bl616 commit and checked to apply, rather than written by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01846EhHuAFk5qvxwEA5Gq6y
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.
flac_decoder_reset()builds a STREAMINFO block ahead of the compressed stream so a raw FLAC frame sequence can be handed to a normal decoder. It wroteblock_size * num_channelsinto the minimum and maximum block-size fields, but STREAMINFO counts inter-channel samples, so for stereo it claimed twice the real maximum.dr_flac sizes its decoded-sample buffer from that value, so every FLAC decoder instance has been carrying a buffer twice as large as it needs. Over-declaring is otherwise harmless — the field only sizes that buffer and rejects frames larger than it — which is why nothing ever failed because of it.
The corrected value is exact rather than merely safe. chdman picks the encoder's block size with the same function the codecs here call to derive this argument (
chd_cd_flac_compressor::blocksizeandchd_flac_compressor::blocksize), so the declared maximum now matches what the stream actually contains. MAME's own decoder writesblock_sizeunmultiplied. The AVHuff path passes one channel and is unaffected either way.Effect
Peak heap on rv32imafc under qemu,
CHDR_LOWRAM_TARGET=ON:cd_cdflhd_flacEvery other codec's peak is unchanged to the byte. On x86-64 the same delta shows up under massif:
cd_cdfl105136 -> 86320,cd_default152956 -> 134140,hd_flac107088 -> 98896.Test coverage
Neither FLAC codec was in the RAM-budget firmware, even though
cd_cdflholds the largest per-instance buffer of any codec there — second only tocd_cdzsin total peak. That is how this went unmeasured. Both are added, across all three budget targets, with ceilings in line with the existing entries.Verification
Decoded output is byte-identical across the seed corpus, the CHDv3/v4 fixtures, three real CHDv4 images, six real discs and an AVHuff image, in the default build, with
CHDR_CD_SCRATCH_BUFFER=OFFand withCHDR_LOWRAM_TARGET=ON— 108 comparisons, no difference.Note for #186: it edits the same threshold tables and will need the two new codecs added there as well.
🤖 Generated with Claude Code