From f2a749b94a67f89f925e82778186ac1f12894a01 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Sun, 6 Sep 2026 11:25:14 +0200 Subject: [PATCH 1/3] Add an optional micro-flac backend for the FLAC decoder CHDR_FLAC_BACKEND=microflac swaps dr_flac for micro-flac behind the same flac_decoder interface, so exactly one backend is compiled and nothing else in the tree changes. dr_flac stays the default. micro-flac is Apache-2.0 and C++, so it is fetched at build time from a pinned commit rather than vendored: this tree stays BSD-3 for its other consumers, and a default build neither touches the network nor needs a C++ compiler - enable_language(CXX) is inside the backend's own branch, and the new workflow checks that by configuring a default build with a compiler path that does not exist. Point CHDR_MICROFLAC_SOURCE_DIR at a checkout to build offline. It fits one thing libchdr needs better than dr_flac does. dr_flac exposes no way to ask how many input bytes it consumed, so the dr_flac backend reconstructs that by reaching into the bit-reader's private cache state, and cdfl uses the answer to find the subcode that follows the audio in the hunk. micro-flac's streaming decode() returns bytes_consumed directly. It decodes into the caller's buffer, with no intermediate. micro-flac re-checks the output buffer against one whole block on every call, sized from the STREAMINFO block-size field - and the header libchdr synthesises carries block_size * num_channels where the format specifies inter-channel samples. Writing the specified value halves what micro-flac demands, which is what makes the caller's hunk large enough for every block of it. A buffer smaller than one block, or a final block that would overrun it, still falls back to a retained scratch; for CD geometry it is never allocated. That field cuts both ways and the comment says so: micro-flac also *rejects* any frame whose block size exceeds it, so the smaller value narrows what this backend accepts. It is safe against what chdman writes, because libchdr derives the field from the same geometry chdman encoded to, but it is not a margin. Measured on hardware, at the same point of the same image: 198 KB of heap in use against dr_flac's 210, and a largest free block of 52 KB against 39 - which is the figure that decides whether a later allocation of that size succeeds. On the desktop, peak heap on a cdfl image is 99488 bytes against dr_flac's 121640, excluding 73728 bytes of libstdc++ static initialisation the dynamic loader performs before main(); that is a hosted-link artifact and does not exist in a bare-metal link with the operator-new shim documented in contrib/tangcore-bl616/README.md. One incidental correctness gain. dr_flac writes int16_t straight into the caller's buffer, which is undefined on an odd pointer and is why chd_read documents a 2-byte alignment requirement. Under UBSan with a deliberately misaligned buffer, dr_flac aborts on all ten odd-aligned cases and micro-flac completes every one of them, with no sanitizer diagnostic at all. Three things this also fixes or closes: flac.h leaves the installed header set. It is included only from src/, and the backend makes struct _flac_decoder depend on a macro no consumer defines - so an installed copy would describe a different layout than the library was built with. flac_decoder_free() no longer destroys an object that was never constructed. chd_close() frees every codec slot, including ones whose init() never ran. generate.sh gains hd_flac.chd. The raw flac codec was reached only because it happens to be one of chdman's default hard-disk codecs, and relying on a default to cover a codec is how a codec stops being covered. Verified byte-identical against the dr_flac backend throughout: 18 seeds in three read orders, both settings of CHDR_CD_SCRATCH_BUFFER, and 4253 real images sampled 400 hunks each. 255 ASan/UBSan runs across seed x access pattern x alignment. AVHuff's audio streams also use this decoder and are not covered - that corpus is fetched rather than generated, so wiring it in is its own change. --- .github/workflows/microflac-backend.yml | 107 ++++++ .gitignore | 1 + CMakeLists.txt | 79 ++++- .../components/libchdr/CMakeLists.txt | 53 ++- contrib/rp2350/CMakeLists.txt | 39 ++- contrib/tangcore-bl616/README.md | 67 ++++ include/libchdr/flac.h | 33 ++ src/libchdr_flac_microflac.cpp | 308 ++++++++++++++++++ tests/corpus/generate.sh | 4 + 9 files changed, 687 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/microflac-backend.yml create mode 100644 src/libchdr_flac_microflac.cpp diff --git a/.github/workflows/microflac-backend.yml b/.github/workflows/microflac-backend.yml new file mode 100644 index 00000000..2670eb5b --- /dev/null +++ b/.github/workflows/microflac-backend.yml @@ -0,0 +1,107 @@ +name: micro-flac backend + +# Builds CHDR_FLAC_BACKEND=microflac and diffs its decoded output against the +# dr_flac backend byte-for-byte, in both settings of CHDR_CD_SCRATCH_BUFFER. +# +# This exists because the backend is opt-in, C++, and fetched at build time +# from a pinned commit rather than vendored - so nothing else in this repo +# compiles it, and it would rot silently. It also compiles against micro-flac's +# internals, which is why the fetch pins a SHA: an upstream change to its +# decode() contract has to fail here rather than in someone's firmware. +# +# The comparison is the point, not the build. micro-flac synthesises a smaller +# STREAMINFO block-size field than the dr_flac backend does, and unlike dr_flac +# it rejects any frame larger than that field - so this is the check that the +# narrower value still accepts everything chdman writes. +# +# It covers two of the three geometries that reach the FLAC decoder: +# cd_cdfl.chd (CD, 2 channels) and hd_flac.chd (the raw flac codec, 2 channels, +# hunk size not a multiple of the block so the last frame is short). AVHuff's +# audio streams also go through this decoder and are NOT covered here - that +# corpus is fetched, not generated, so wiring it in is its own change. + +on: + push: + branches: [master] + paths-ignore: + - 'docs/**' + - '**.md' + - 'LICENSE' + - '.gitignore' + - 'contrib/**' + pull_request: + paths-ignore: + - 'docs/**' + - '**.md' + - 'LICENSE' + - '.gitignore' + - 'contrib/**' + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.ref != 'refs/heads/master' }} + +env: + # micro-flac commit the backend is compiled against. Bump deliberately. + MICROFLAC_SHA: ffbe8a9ba5e78c16e535a4695a8b2418b0c091ee + +jobs: + microflac-backend: + runs-on: ubuntu-latest + timeout-minutes: 15 + + 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 both backends, both scratch settings + run: | + set -euo pipefail + for s in 0 1; do + cmake -B "build-drflac-$s" -DCMAKE_BUILD_TYPE=Release \ + -DCHDR_CD_SCRATCH_BUFFER=$s + cmake --build "build-drflac-$s" --target chd_dump_order -j$(nproc) + + cmake -B "build-microflac-$s" -DCMAKE_BUILD_TYPE=Release \ + -DCHDR_CD_SCRATCH_BUFFER=$s \ + -DCHDR_FLAC_BACKEND=microflac \ + -DCHDR_MICROFLAC_GIT_TAG="$MICROFLAC_SHA" + cmake --build "build-microflac-$s" --target chd_dump_order -j$(nproc) + done + + - name: Byte-identical decode against dr_flac + run: | + set -euo pipefail + fail=0 + total=0 + for s in 0 1; do + D="build-drflac-$s/tests/chd_dump_order" + M="build-microflac-$s/tests/chd_dump_order" + for f in tests/corpus/seeds/*.chd; do + for order in sequential reverse random:42; do + total=$((total+1)) + "$D" "$f" "$order" > /tmp/d.bin + "$M" "$f" "$order" > /tmp/m.bin + if ! cmp -s /tmp/d.bin /tmp/m.bin; then + echo "MISMATCH: $(basename "$f") $order CHDR_CD_SCRATCH_BUFFER=$s" + fail=$((fail+1)) + fi + done + done + done + echo "$total checks, $fail mismatches" + [ "$fail" -eq 0 ] + + - name: A default build still needs no C++ compiler + run: | + set -euo pipefail + # enable_language(CXX) must stay inside the microflac branch, or the + # opt-in backend stops being opt-in for anyone without a C++ toolchain. + cmake -B build-nocxx -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_CXX_COMPILER=/nonexistent-cxx + cmake --build build-nocxx --target chd_dump_order -j$(nproc) diff --git a/.gitignore b/.gitignore index 9ab031a5..9ce70fa1 100644 --- a/.gitignore +++ b/.gitignore @@ -22,5 +22,6 @@ contrib/esp32p4/idf-benchmark/main/embed/ contrib/esp32p4/idf-benchmark/main/embed_list.inc contrib/esp32p4/idf-benchmark/main/embed_includes.inc contrib/esp32p4/idf-benchmark/build/ +contrib/esp32p4/idf-benchmark/build-*/ contrib/esp32p4/idf-benchmark/sdkconfig contrib/esp32p4/idf-benchmark/sdkconfig.old diff --git a/CMakeLists.txt b/CMakeLists.txt index 72118140..3ab4f1a1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -27,6 +27,17 @@ option(CHDR_WANT_TESTS "Build tests for the library" ON) # linearly, and never read. set(CHDR_CD_SCRATCH_BUFFER "" CACHE STRING "Decode CD sectors via a private scratch buffer (ON/OFF; default follows CHDR_LOWRAM_TARGET)") +# FLAC backend. dr_flac is the default and needs nothing extra. microflac is +# faster on MCUs (measured 1.46x on ESP32-S3, 1.41x on ESP32-P4, bit-identical +# output) and smaller, but it is C++ and Apache-2.0, so it is fetched at build +# time rather than vendored - libchdr's own tree stays BSD-3 and a default +# build never touches the network. Point CHDR_MICROFLAC_SOURCE_DIR at a local +# checkout to build offline. +set(CHDR_FLAC_BACKEND "drflac" CACHE STRING "FLAC decoder backend: drflac or microflac") +set_property(CACHE CHDR_FLAC_BACKEND PROPERTY STRINGS drflac microflac) +set(CHDR_MICROFLAC_SOURCE_DIR "" CACHE PATH "Local micro-flac checkout (skips the fetch)") +set(CHDR_MICROFLAC_GIT_TAG "" CACHE STRING "micro-flac commit to fetch (40-char SHA)") + option(BUILD_LTO "Compile libchdr with link-time optimization if supported" OFF) if(BUILD_LTO) include(CheckIPOSupported) @@ -156,6 +167,47 @@ endif() # chdr #-------------------------------------------------- +if(CHDR_FLAC_BACKEND STREQUAL "microflac") + enable_language(CXX) + set(CHDR_FLAC_BACKEND_SOURCE src/libchdr_flac_microflac.cpp) + if(CHDR_MICROFLAC_SOURCE_DIR) + set(_microflac_dir "${CHDR_MICROFLAC_SOURCE_DIR}") + else() + if(CHDR_MICROFLAC_GIT_TAG AND NOT CHDR_MICROFLAC_GIT_TAG MATCHES "^[0-9a-fA-F]{40}$") + message(FATAL_ERROR + "CHDR_MICROFLAC_GIT_TAG must be a 40-character commit SHA, got: " + "${CHDR_MICROFLAC_GIT_TAG}") + endif() + if(NOT CHDR_MICROFLAC_GIT_TAG) + message(FATAL_ERROR + "CHDR_FLAC_BACKEND=microflac needs either CHDR_MICROFLAC_SOURCE_DIR (a local " + "checkout) or CHDR_MICROFLAC_GIT_TAG (a 40-character commit SHA). Branch names " + "are deliberately not accepted: the backend is compiled against micro-flac's " + "internals, so the build must be reproducible.") + endif() + include(FetchContent) + FetchContent_Declare(microflac + GIT_REPOSITORY https://github.com/esphome-libs/micro-flac.git + GIT_TAG ${CHDR_MICROFLAC_GIT_TAG}) + FetchContent_MakeAvailable(microflac) + set(_microflac_dir "${microflac_SOURCE_DIR}") + endif() + set(CHDR_MICROFLAC_SOURCES + ${_microflac_dir}/src/flac_decoder.cpp + ${_microflac_dir}/src/decorrelation.cpp + ${_microflac_dir}/src/frame_header.cpp + ${_microflac_dir}/src/pcm_packing.cpp + ${_microflac_dir}/src/crc.cpp + ${_microflac_dir}/src/lpc.cpp) + list(APPEND CHDR_DEFINES CHDR_FLAC_BACKEND_MICROFLAC MICRO_FLAC_DISABLE_OGG) +elseif(CHDR_FLAC_BACKEND STREQUAL "drflac") + set(CHDR_FLAC_BACKEND_SOURCE src/libchdr_flac.c) + set(CHDR_MICROFLAC_SOURCES "") + set(_microflac_dir "") +else() + message(FATAL_ERROR "CHDR_FLAC_BACKEND must be drflac or microflac, got: ${CHDR_FLAC_BACKEND}") +endif() + set(CHDR_SOURCES src/libchdr_bitstream.c src/libchdr_cdrom.c @@ -170,12 +222,34 @@ set(CHDR_SOURCES src/libchdr_codec_lzma.c src/libchdr_codec_zlib.c src/libchdr_codec_zstd.c - src/libchdr_flac.c + ${CHDR_FLAC_BACKEND_SOURCE} src/libchdr_huffman.c + ${CHDR_MICROFLAC_SOURCES} ) + +# Applies the micro-flac include paths and the C++ settings that keep its +# runtime footprint down. -fno-exceptions/-fno-rtti are what let a bare-metal +# link avoid pulling in libstdc++'s exception and RTTI machinery, and +# -fno-threadsafe-statics drops the __cxa_guard_* dependency. +function(chdr_apply_microflac target) + if(NOT CHDR_FLAC_BACKEND STREQUAL "microflac") + return() + endif() + # Public include dir only: micro-flac's src/ contains a crc.h that will + # shadow a consumer's own header of that name, and its sources reach their + # private headers relative to themselves anyway. + target_include_directories(${target} PRIVATE + ${_microflac_dir}/include) + target_compile_options(${target} PRIVATE + $<$:-fno-exceptions> + $<$:-fno-rtti> + $<$:-fno-threadsafe-statics>) +endfunction() + add_library(chdr-static STATIC ${CHDR_SOURCES}) target_include_directories(chdr-static INTERFACE include) +chdr_apply_microflac(chdr-static) target_link_libraries(chdr-static PRIVATE ${CHDR_LIBS} ${PLATFORM_LIBS}) # libchdr_codec_zlib.c calls tinfl_decompress() directly, so it must be renamed # alongside miniz's definition - otherwise on ESP-IDF this call binds to the @@ -207,6 +281,7 @@ endif() if (BUILD_SHARED_LIBS) add_library(chdr SHARED ${CHDR_SOURCES}) target_include_directories(chdr INTERFACE include) + chdr_apply_microflac(chdr) target_link_libraries(chdr PRIVATE ${CHDR_LIBS} ${PLATFORM_LIBS}) if(CHDR_NEEDS_MINIZ_RENAME) libchdr_apply_esp_rom_miniz_workaround(chdr) @@ -232,7 +307,7 @@ if (BUILD_SHARED_LIBS) set_target_properties(chdr PROPERTIES C_VISIBILITY_PRESET hidden) set_target_properties(chdr PROPERTIES VISIBILITY_INLINES_HIDDEN 1) - set_target_properties(chdr PROPERTIES PUBLIC_HEADER "include/libchdr/bitstream.h;include/libchdr/cdrom.h;include/libchdr/chd.h;include/libchdr/chdconfig.h;include/libchdr/coretypes.h;include/libchdr/flac.h;include/libchdr/huffman.h;include/libchdr/macros.h") + set_target_properties(chdr PROPERTIES PUBLIC_HEADER "include/libchdr/bitstream.h;include/libchdr/cdrom.h;include/libchdr/chd.h;include/libchdr/chdconfig.h;include/libchdr/coretypes.h;include/libchdr/huffman.h;include/libchdr/macros.h") set_target_properties(chdr PROPERTIES VERSION "${PROJECT_VERSION_MAJOR}.${PROJECT_VERSION_MINOR}" SOVERSION ${PROJECT_VERSION_MAJOR}) if (CMAKE_BUILD_TYPE MATCHES Release) diff --git a/contrib/esp32p4/idf-benchmark/components/libchdr/CMakeLists.txt b/contrib/esp32p4/idf-benchmark/components/libchdr/CMakeLists.txt index 9b1a8d44..f44794c2 100644 --- a/contrib/esp32p4/idf-benchmark/components/libchdr/CMakeLists.txt +++ b/contrib/esp32p4/idf-benchmark/components/libchdr/CMakeLists.txt @@ -20,6 +20,42 @@ endif() set(LIBCHDR_ROOT "${CMAKE_CURRENT_LIST_DIR}/../../../../..") +# FLAC backend, mirroring libchdr's own CHDR_FLAC_BACKEND. dr_flac is the +# default. microflac is measurably faster here - 1.46x on the S3 and 1.41x on +# the P4 decoding CD audio - but it is C++ and Apache-2.0, so it is not +# vendored: point CHDR_MICROFLAC_SOURCE_DIR at a checkout, or add +# esphome/micro-flac to the project's idf_component.yml and point this at +# ${CMAKE_BINARY_DIR}/../managed_components/esphome__micro-flac. +if(NOT DEFINED CHDR_FLAC_BACKEND) + set(CHDR_FLAC_BACKEND drflac) +endif() +if(CHDR_FLAC_BACKEND STREQUAL "microflac") + if(NOT CHDR_MICROFLAC_SOURCE_DIR) + message(FATAL_ERROR "CHDR_FLAC_BACKEND=microflac needs CHDR_MICROFLAC_SOURCE_DIR") + endif() + # micro-flac enables its Xtensa LPC assembly whenever the core has the + # required features, so the .S files must be compiled in on esp32/esp32s3 + # or the link fails on restore_lpc_*_asm. Worth ~1.064x on its own; the + # RISC-V parts take the C path and need nothing extra. + set(CHDR_MICROFLAC_ASM "") + if(IDF_TARGET STREQUAL "esp32" OR IDF_TARGET STREQUAL "esp32s3") + set(CHDR_MICROFLAC_ASM + "${CHDR_MICROFLAC_SOURCE_DIR}/src/xtensa/lpc_32_xtensa.S" + "${CHDR_MICROFLAC_SOURCE_DIR}/src/xtensa/lpc_64_xtensa.S") + endif() + set(CHDR_FLAC_SRCS + "${LIBCHDR_ROOT}/src/libchdr_flac_microflac.cpp" + ${CHDR_MICROFLAC_ASM} + "${CHDR_MICROFLAC_SOURCE_DIR}/src/flac_decoder.cpp" + "${CHDR_MICROFLAC_SOURCE_DIR}/src/decorrelation.cpp" + "${CHDR_MICROFLAC_SOURCE_DIR}/src/frame_header.cpp" + "${CHDR_MICROFLAC_SOURCE_DIR}/src/pcm_packing.cpp" + "${CHDR_MICROFLAC_SOURCE_DIR}/src/crc.cpp" + "${CHDR_MICROFLAC_SOURCE_DIR}/src/lpc.cpp") +else() + set(CHDR_FLAC_SRCS "${LIBCHDR_ROOT}/src/libchdr_flac.c") +endif() + idf_component_register( SRCS "${LIBCHDR_ROOT}/src/libchdr_bitstream.c" @@ -35,7 +71,7 @@ idf_component_register( "${LIBCHDR_ROOT}/src/libchdr_codec_lzma.c" "${LIBCHDR_ROOT}/src/libchdr_codec_zlib.c" "${LIBCHDR_ROOT}/src/libchdr_codec_zstd.c" - "${LIBCHDR_ROOT}/src/libchdr_flac.c" + ${CHDR_FLAC_SRCS} "${LIBCHDR_ROOT}/src/libchdr_huffman.c" "${LIBCHDR_ROOT}/deps/lzma-26.02/src/LzmaDec.c" "${LIBCHDR_ROOT}/deps/miniz-3.1.2/miniz.c" @@ -44,6 +80,21 @@ idf_component_register( "${LIBCHDR_ROOT}/include" ) +if(CHDR_FLAC_BACKEND STREQUAL "microflac") + # Only micro-flac's public include dir: its src/ holds a crc.h that will + # shadow a consumer's own header of that name. Its sources reach their + # private headers relative to themselves. + target_include_directories(${COMPONENT_LIB} PRIVATE + "${CHDR_MICROFLAC_SOURCE_DIR}/include") + target_compile_definitions(${COMPONENT_LIB} PRIVATE + CHDR_FLAC_BACKEND_MICROFLAC MICRO_FLAC_DISABLE_OGG) + # Keeps libstdc++'s exception, RTTI and guard machinery out of the image. + target_compile_options(${COMPONENT_LIB} PRIVATE + $<$:-fno-exceptions> + $<$:-fno-rtti> + $<$:-fno-threadsafe-statics>) +endif() + target_compile_definitions(${COMPONENT_LIB} PRIVATE WANT_RAW_DATA_SECTOR=1 WANT_SUBCODE=1 diff --git a/contrib/rp2350/CMakeLists.txt b/contrib/rp2350/CMakeLists.txt index f6192a21..b4f6b871 100644 --- a/contrib/rp2350/CMakeLists.txt +++ b/contrib/rp2350/CMakeLists.txt @@ -42,9 +42,32 @@ endif() # (+15,980 bytes per LZMA instance at CHD's lc=3/lp=0). Off by default. option(CHDR_LZMA_PROB32 "Use 32-bit LZMA probability model" OFF) +# FLAC backend, mirroring libchdr's own CHDR_FLAC_BACKEND option. dr_flac is +# the default; microflac needs CHDR_MICROFLAC_SOURCE_DIR pointing at a checkout. +if(NOT DEFINED CHDR_FLAC_BACKEND) + set(CHDR_FLAC_BACKEND drflac) +endif() +if(CHDR_FLAC_BACKEND STREQUAL "microflac") + enable_language(CXX) + if(NOT CHDR_MICROFLAC_SOURCE_DIR) + message(FATAL_ERROR "CHDR_FLAC_BACKEND=microflac needs CHDR_MICROFLAC_SOURCE_DIR") + endif() + set(CHDR_FLAC_SRC + ${LIBCHDR_ROOT}/src/libchdr_flac_microflac.cpp + ${CHDR_MICROFLAC_SOURCE_DIR}/src/flac_decoder.cpp + ${CHDR_MICROFLAC_SOURCE_DIR}/src/decorrelation.cpp + ${CHDR_MICROFLAC_SOURCE_DIR}/src/frame_header.cpp + ${CHDR_MICROFLAC_SOURCE_DIR}/src/pcm_packing.cpp + ${CHDR_MICROFLAC_SOURCE_DIR}/src/crc.cpp + ${CHDR_MICROFLAC_SOURCE_DIR}/src/lpc.cpp) +else() + set(CHDR_FLAC_SRC ${LIBCHDR_ROOT}/src/libchdr_flac.c) +endif() + add_executable(rp2350-bench benchmark_main.c hw_config.c + ${CHDR_FLAC_SRC} ${LIBCHDR_ROOT}/src/libchdr_bitstream.c ${LIBCHDR_ROOT}/src/libchdr_cdrom.c ${LIBCHDR_ROOT}/src/libchdr_chd.c @@ -58,7 +81,6 @@ add_executable(rp2350-bench ${LIBCHDR_ROOT}/src/libchdr_codec_lzma.c ${LIBCHDR_ROOT}/src/libchdr_codec_zlib.c ${LIBCHDR_ROOT}/src/libchdr_codec_zstd.c - ${LIBCHDR_ROOT}/src/libchdr_flac.c ${LIBCHDR_ROOT}/src/libchdr_huffman.c ${LIBCHDR_ROOT}/deps/lzma-26.02/src/LzmaDec.c ${LIBCHDR_ROOT}/deps/miniz-3.1.2/miniz.c @@ -67,8 +89,23 @@ add_executable(rp2350-bench target_include_directories(rp2350-bench PRIVATE ${LIBCHDR_ROOT}/include + ${LIBCHDR_ROOT}/src ${CMAKE_CURRENT_LIST_DIR} ) +if(CHDR_FLAC_BACKEND STREQUAL "microflac") + # Only micro-flac's public include dir goes on the target path. Its src/ + # dir must NOT: it holds a crc.h that shadows the SD driver's own, and a C + # translation unit then pulls in a C++ header. micro-flac's sources find + # their private headers relative to themselves. + target_include_directories(rp2350-bench PRIVATE + ${CHDR_MICROFLAC_SOURCE_DIR}/include) + target_compile_definitions(rp2350-bench PRIVATE + CHDR_FLAC_BACKEND_MICROFLAC MICRO_FLAC_DISABLE_OGG) + target_compile_options(rp2350-bench PRIVATE + $<$:-fno-exceptions> + $<$:-fno-rtti> + $<$:-fno-threadsafe-statics>) +endif() target_compile_definitions(rp2350-bench PRIVATE WANT_RAW_DATA_SECTOR=1 diff --git a/contrib/tangcore-bl616/README.md b/contrib/tangcore-bl616/README.md index ff0642a3..a8ca07e2 100644 --- a/contrib/tangcore-bl616/README.md +++ b/contrib/tangcore-bl616/README.md @@ -93,6 +93,73 @@ cp /path/to/libchdr/contrib/tangcore-bl616/chd/*.{h,c} chd/ make # BL_SDK_BASE defaults to ../bouffalo_sdk, TANG_BOARD defaults to console60k ``` +## Optional: the micro-flac backend + +libchdr can decode FLAC through [micro-flac](https://github.com/esphome-libs/micro-flac) +instead of dr_flac (`CHDR_FLAC_BACKEND=microflac`). Measured on hardware it is +1.46x faster on an ESP32-S3 and 1.41x on an ESP32-P4 with byte-identical +output, and most of that comes from its C rather than its Xtensa assembly, so +RV32 sees it too. It is not wired into the patch above, because the BL616 +integration selects sources with a glob and the backend needs three changes +that a glob cannot express. + +**Licensing.** micro-flac is Apache-2.0, including its `.S` files. That is +permissive and does not relicense libchdr, and TangCore's firmware is already +Apache-2.0 end to end (Bouffalo SDK and firmware-bl616 both), so it adds no +obligation here. Do not vendor it into libchdr's own tree - fetch it, so +libchdr stays BSD-3 for its other consumers. + +**1. Source selection.** `thirdparty/libchdr/src/*.c` globs `libchdr_flac.c`, +which is the dr_flac backend; compiling both is a duplicate-symbol error. List +the sources explicitly, or exclude that one file, and add: + + thirdparty/libchdr/src/libchdr_flac_microflac.cpp + thirdparty/micro-flac/src/{flac_decoder,decorrelation,frame_header,pcm_packing,crc,lpc}.cpp + +with `CHDR_FLAC_BACKEND_MICROFLAC` and `MICRO_FLAC_DISABLE_OGG` defined, and +`-fno-exceptions -fno-rtti -fno-threadsafe-statics -fno-use-cxa-atexit` on the +C++ sources (the last one is what ESP-IDF does; without it the objects also +reference `__cxa_atexit`, which newlib does provide, so it links either way). Put +only micro-flac's `include/` on the include path - its `src/` holds a `crc.h` +that will shadow another component's own header of that name. + +**2. Do not link with `g++`.** A plain C++ link pulls ~82 KB of libstdc++ even +with exceptions off: `std::__throw_length_error` drags in `cow-stdexcept`, +`tinfo`, `cp-demangle` and around fifteen `eh_*.o` objects. ESP-IDF hides this +by wrapping `__cxa_throw`; the BL616 toolchain does not. Link with the **`gcc` +driver** (the T-Head GCC is 10.2, which predates `-nostdlib++`) and supply the +handful of symbols micro-flac actually needs: + +```c +/* everything micro-flac wants from the C++ runtime */ +#include +void *operator new(unsigned sz) { return malloc(sz); } +void *operator new[](unsigned sz) { return malloc(sz); } +void operator delete(void *p) noexcept { free(p); } +void operator delete[](void *p) noexcept { free(p); } +void operator delete(void *p, unsigned) noexcept { free(p); } +void operator delete[](void *p, unsigned) noexcept { free(p); } +namespace std { void __throw_length_error(const char *) { abort(); } } +``` + +With those flags the only C++ runtime symbols left undefined are the three the +shim defines - verified by `nm -u` on the compiled objects at the real ABI. +Measured effect on a test image: 154,514 B of text down to 54,569 B, with zero +`libstdc++.a` members linked. + +**3. Use the vendor toolchain.** The T-Head GCC at +`toolchain_gcc_t-head_linux` and Debian's `gcc-riscv64-unknown-elf` share the +`riscv64-unknown-elf-` prefix, so PATH order decides which one you get, and +`CROSS_COMPILE ?= riscv64-unknown-elf-` does not disambiguate. Debian's ships +the `g++` driver but **no libstdc++ at all** - no `cstddef`, no `libstdc++.a` - +so micro-flac will not compile against it, for reasons that say nothing about +the BL616. Check with `riscv64-unknown-elf-gcc --version`: the vendor one +reports "Xuantie-900". + +micro-flac itself compiles clean at the real BL616 ABI +(`-march=rv32imafcpzpsfoperand_xtheade -mabi=ilp32f`, zero warnings) and its +object code is smaller than dr_flac's there: 36,304 B against 45,129 B. + ## Status (2026-08-25) Compiles and links clean, `LOWRAM_TARGET=1`. Real flash cost: +142.5KB (whole diff --git a/include/libchdr/flac.h b/include/libchdr/flac.h index 17adb1ce..49a3ba90 100644 --- a/include/libchdr/flac.h +++ b/include/libchdr/flac.h @@ -22,6 +22,37 @@ */ typedef struct _flac_decoder flac_decoder; + +#if defined(CHDR_FLAC_BACKEND_MICROFLAC) + +/* micro-flac backend. The decoder is a C++ object, but this header is included + * from C, so it is placement-new'd into an opaque buffer here rather than + * heap-allocated - the codecs embed flac_decoder by value and an allocation + * per codec instance is exactly what the dr_flac arena work removed. + * libchdr_flac_microflac.cpp static_asserts that the object fits. */ +struct _flac_decoder { + uint32_t sample_rate; + uint8_t channels; + uint8_t bits_per_sample; + int alloc_failed; /* set when a decode could not allocate */ + const uint8_t * payload; /* compressed data after the synthesised header */ + uint32_t payload_length; + uint32_t payload_consumed; /* what finish() reports */ + int header_done; /* STREAMINFO parsed for this stream */ + uint8_t custom_header[0x2a]; /* synthesised STREAMINFO */ + /* micro-flac sizes its output buffer as max_block_size * channels * + * bytes_per_sample, and libchdr's synthesised header carries + * block_size * channels in max_block_size - so it asks for twice the + * bytes a cdfl hunk's audio actually occupies. Decode into this and copy + * out. Retained across hunks, like the dr_flac backend's block. */ + int16_t * scratch; + uint32_t scratch_size; + int constructed; /* impl[] holds a live object */ + unsigned long long impl[64]; /* opaque, 8-byte aligned */ +}; + +#else + struct _flac_decoder { /* output state */ void * decoder; /* actual encoder */ @@ -48,6 +79,8 @@ struct _flac_decoder { uint8_t custom_header[0x2a]; /* custom header */ }; +#endif /* CHDR_FLAC_BACKEND_MICROFLAC */ + /* ======================> flac_decoder */ int flac_decoder_init(flac_decoder* decoder); diff --git a/src/libchdr_flac_microflac.cpp b/src/libchdr_flac_microflac.cpp new file mode 100644 index 00000000..b23a2307 --- /dev/null +++ b/src/libchdr_flac_microflac.cpp @@ -0,0 +1,308 @@ +/* license:BSD-3-Clause + * copyright-holders:Aaron Giles + *************************************************************************** + + libchdr_flac_microflac.cpp + + micro-flac backend for the FLAC decoder interface, as an alternative to + the dr_flac one in libchdr_flac.c. Selected at build time with + CHDR_FLAC_BACKEND=microflac; exactly one backend is compiled. + + Why it exists: measured on hardware against dr_flac, decoding 10 s of CD + audio at libchdr's cdfl geometry, micro-flac is 1.46x faster on an + ESP32-S3 and 1.41x on an ESP32-P4, and its object code is smaller + (27.7 KB vs 38.6 KB on RV32). Most of that comes from its C, not from its + Xtensa assembly, so RISC-V targets benefit too. + + It is also a better fit for one thing libchdr needs. dr_flac exposes no + way to ask how many input bytes it consumed, so flac_decoder_finish() in + the dr_flac backend reconstructs that by reaching into the bit-reader's + private cache state - and cdfl uses the answer to locate the subcode that + follows the audio in the hunk. micro-flac's streaming decode() returns + bytes_consumed directly. Verified equal on real cdfl blocks: both report + 6462, 6476, 6475, 6571 and 6798 bytes for the first five FLAC hunks of a + PC Engine CD image, with byte-identical audio. + +***************************************************************************/ + +#include +#include +#include + +extern "C" { +#include "../include/libchdr/chdconfig.h" +#include "../include/libchdr/flac.h" +} + +#include "micro_flac/flac_decoder.h" + +using micro_flac::FLACDecoder; + +/* The C struct reserves a fixed, aligned buffer for the C++ object so the + * codecs can keep embedding flac_decoder by value. If micro-flac grows past + * it this fails loudly at compile time rather than corrupting the struct. */ +static_assert(sizeof(FLACDecoder) <= sizeof(((flac_decoder *)0)->impl), + "flac_decoder::impl is too small for micro_flac::FLACDecoder"); +static_assert(alignof(FLACDecoder) <= alignof(unsigned long long), + "flac_decoder::impl is not aligned enough for micro_flac::FLACDecoder"); + +static FLACDecoder *impl_of(flac_decoder *decoder) +{ + return reinterpret_cast(decoder->impl); +} + +/*------------------------------------------------- + * flac_decoder_init - construct in place + *------------------------------------------------- + */ + +extern "C" int flac_decoder_init(flac_decoder *decoder) +{ + memset(decoder, 0, sizeof(*decoder)); + new (static_cast(decoder->impl)) FLACDecoder(); + decoder->constructed = 1; + /* libchdr verifies the per-hunk CRC itself where it is wanted, and the + * frame CRC is pure overhead on top of that. */ + impl_of(decoder)->set_crc_check_enabled(false); + return 0; +} + +/*------------------------------------------------- + * flac_decoder_free - destroy in place + *------------------------------------------------- + */ + +extern "C" void flac_decoder_free(flac_decoder *decoder) +{ + if (decoder == NULL) + return; + /* chd_close() frees every codec slot, including ones whose init() never + * ran - so impl[] may be the zeroed struct rather than a constructed + * object. Destroying that is undefined; init() sets the flag last. */ + if (!decoder->constructed) + return; + impl_of(decoder)->~FLACDecoder(); + decoder->constructed = 0; + if (decoder->scratch != NULL) { + free(decoder->scratch); + decoder->scratch = NULL; + decoder->scratch_size = 0; + } +} + +/*------------------------------------------------- + * flac_decoder_reset - start a new stream + *------------------------------------------------- + */ + +extern "C" int flac_decoder_reset(flac_decoder *decoder, uint32_t sample_rate, + uint8_t num_channels, uint32_t block_size, const void *buffer, uint32_t length) +{ + /* A CHD stores raw FLAC frames with no header, so one is manufactured per + * hunk. The dr_flac backend writes block_size * num_channels into the + * block-size fields; this one writes block_size, which is what the format + * actually specifies - inter-channel samples, not interleaved ones. + * + * This is not cosmetic in both directions. micro-flac sizes its buffers + * from this field and re-checks the caller's output buffer against it on + * every call, so the inflated value costs twice the memory and forces + * every hunk through a scratch copy. But it also *rejects* any frame + * whose block size exceeds the field, so writing the smaller value + * narrows what this backend accepts: a stream whose frames are larger + * than the block size libchdr computes now fails where dr_flac would + * have decoded it. + * + * That is safe against what chdman writes, because libchdr derives the + * field from the same geometry chdman encoded to and both halve until + * they are under the same cap. It is not a margin. A third-party encoder + * emitting larger frames than the hunk geometry implies would decode + * under dr_flac and fail here. + * + * Verified byte-identical against the dr_flac backend on the whole + * corpus, in both settings of CHDR_CD_SCRATCH_BUFFER. */ + static const uint8_t s_header_template[0x2a] = + { + 0x66, 0x4C, 0x61, 0x43, /* +00: 'fLaC' */ + 0x80, /* +04: STREAMINFO, last block */ + 0x00, 0x00, 0x22, /* +05: length 0x22 */ + 0x00, 0x00, /* +08: minimum block size */ + 0x00, 0x00, /* +0A: maximum block size */ + 0x00, 0x00, 0x00, /* +0C: minimum frame size */ + 0x00, 0x00, 0x00, /* +0F: maximum frame size */ + 0x0A, 0xC4, 0x42, 0xF0, 0x00, 0x00, 0x00, 0x00, /* +12: rate/channels/depth */ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* +1A: MD5 (none) */ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* +2A: stream data starts */ + }; + + FLACDecoder *dec = impl_of(decoder); + size_t consumed = 0, decoded = 0; + + memcpy(decoder->custom_header, s_header_template, sizeof(s_header_template)); + decoder->custom_header[0x08] = decoder->custom_header[0x0a] = (uint8_t)((block_size) >> 8); + decoder->custom_header[0x09] = decoder->custom_header[0x0b] = (uint8_t)((block_size) & 0xff); + decoder->custom_header[0x12] = (uint8_t)(sample_rate >> 12); + decoder->custom_header[0x13] = (uint8_t)(sample_rate >> 4); + decoder->custom_header[0x14] = (uint8_t)((sample_rate << 4) | ((num_channels - 1) << 1)); + + decoder->payload = (const uint8_t *)buffer; + decoder->payload_length = length; + decoder->payload_consumed = 0; + decoder->header_done = 0; + decoder->alloc_failed = 0; + + dec->reset(); + dec->set_crc_check_enabled(false); + + /* Feed the header on its own. It yields HEADER_READY without needing an + * output buffer, which is why the header and the payload do not have to + * be stitched into one contiguous block first. */ + { + const uint8_t *in = decoder->custom_header; + size_t left = sizeof(decoder->custom_header); + while (left > 0) { + micro_flac::FLACDecoderResult r = + dec->decode(in, left, (uint8_t *)NULL, (size_t)0, consumed, decoded); + in += consumed; + left -= consumed; + if (r == micro_flac::FLAC_DECODER_HEADER_READY) { + const micro_flac::FLACStreamInfo &si = dec->get_stream_info(); + decoder->sample_rate = si.sample_rate(); + decoder->channels = (uint8_t)si.num_channels(); + decoder->bits_per_sample = (uint8_t)si.bits_per_sample(); + decoder->header_done = 1; + break; + } + if (r == micro_flac::FLAC_DECODER_ERROR_MEMORY_ALLOCATION) { + decoder->alloc_failed = 1; + return 0; + } + if (r != micro_flac::FLAC_DECODER_NEED_MORE_DATA && consumed == 0) + return 0; + } + } + + return decoder->header_done; +} + +/*------------------------------------------------- + * flac_decoder_decode_interleaved - decode a hunk + *------------------------------------------------- + */ + +extern "C" int flac_decoder_decode_interleaved(flac_decoder *decoder, int16_t *samples, + uint32_t num_frames, int swap_endian) +{ + FLACDecoder *dec = impl_of(decoder); + const uint8_t *in = decoder->payload + decoder->payload_consumed; + size_t left = decoder->payload_length - decoder->payload_consumed; + uint32_t want = num_frames * decoder->channels; /* interleaved samples */ + uint32_t have = 0; + + if (!decoder->header_done) + return 0; + + /* micro-flac checks the output buffer against a whole block on every + * call - max_block_size * channels * bytes_per_sample - so it will write + * where the caller wants only while that much room is left. For a CD hunk + * there is, for every block, so nothing is copied and no scratch is + * allocated at all. The tail of a stream whose last block would overrun + * the caller's buffer, and any geometry whose buffer is smaller than one + * block, fall back to decoding a block aside and copying the part that + * fits. The scratch is allocated only if that happens, and then kept. */ + uint32_t block_bytes; + { + const micro_flac::FLACStreamInfo &si = dec->get_stream_info(); + block_bytes = si.max_block_size() * si.num_channels() * si.bytes_per_sample(); + } + + while (have < want) { + size_t consumed = 0, decoded = 0; + size_t room = (size_t)(want - have) * sizeof(int16_t); + int direct = (room >= block_bytes); + uint8_t *out; + size_t outsz; + + if (direct) { + out = (uint8_t *)(samples + have); + outsz = room; + } else { + if (decoder->scratch_size < block_bytes) { + int16_t *p = (int16_t *)realloc(decoder->scratch, block_bytes); + if (p == NULL) { + decoder->alloc_failed = 1; + return 0; + } + decoder->scratch = p; + decoder->scratch_size = block_bytes; + } + out = (uint8_t *)decoder->scratch; + outsz = decoder->scratch_size; + } + + micro_flac::FLACDecoderResult r = dec->decode(in, left, out, outsz, consumed, decoded); + + in += consumed; + left -= consumed; + decoder->payload_consumed += (uint32_t)consumed; + + if (r == micro_flac::FLAC_DECODER_SUCCESS) { + uint32_t take = (uint32_t)decoded; + if (have + take > want) + take = want - have; + if (!direct) + memcpy(samples + have, decoder->scratch, (size_t)take * sizeof(int16_t)); + have += take; + } else if (r == micro_flac::FLAC_DECODER_END_OF_STREAM) { + break; + } else if (r == micro_flac::FLAC_DECODER_NEED_MORE_DATA) { + if (left == 0) + break; + } else if (r == micro_flac::FLAC_DECODER_ERROR_MEMORY_ALLOCATION) { + decoder->alloc_failed = 1; + return 0; + } else if (r != micro_flac::FLAC_DECODER_HEADER_READY) { + return 0; + } + if (consumed == 0 && decoded == 0 && r != micro_flac::FLAC_DECODER_SUCCESS) + break; /* no progress; do not spin */ + } + + if (have != want) + return 0; + + /* CD audio is stored big-endian in a CHD, so the caller asks for a swap + * on a little-endian host. Same transform the dr_flac backend applies in + * its write callback. */ + if (swap_endian) { + uint32_t i; + for (i = 0; i < want; i++) { + uint16_t v = (uint16_t)samples[i]; + samples[i] = (int16_t)((uint16_t)(v << 8) | (uint16_t)(v >> 8)); + } + } + return 1; +} + +/*------------------------------------------------- + * flac_decoder_finish - bytes of payload consumed + *------------------------------------------------- + */ + +extern "C" uint32_t flac_decoder_finish(flac_decoder *decoder) +{ + /* cdfl uses this to find the subcode that follows the audio, so it must + * be the exact byte count - not a rounded-up frame boundary. */ + return decoder->payload_consumed; +} + +/*------------------------------------------------- + * flac_decoder_detect_native_endian + *------------------------------------------------- + */ + +extern "C" int flac_decoder_detect_native_endian(void) +{ + uint16_t native_endian = 0; + *(uint8_t *)(&native_endian) = 1; + return (native_endian & 1); +} diff --git a/tests/corpus/generate.sh b/tests/corpus/generate.sh index 034f11d3..4c2b2d2b 100755 --- a/tests/corpus/generate.sh +++ b/tests/corpus/generate.sh @@ -76,6 +76,10 @@ create_hd hd_lzma.chd -c lzma create_hd hd_huff.chd -c huff create_hd hd_zstd.chd -c zstd create_hd hd_multi.chd -c zlib,lzma,huff,zstd +# The raw FLAC codec, on its own. hd_default happens to include flac hunks +# because that 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. +create_hd hd_flac.chd -c flac # CD-ROM: default + per-codec. create_cd cd_default.chd From 5baeaa7dea81008f2b53d8e8d6b50d0415e5ced8 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Sun, 6 Sep 2026 14:28:08 +0200 Subject: [PATCH 2/3] BL616: say why the C++ shim exists, and why the toolchain is pinned The operator-new shim is a GCC 10.2 artifact - -nostdlib++ replaces it in one flag and has existed since GCC 11 - and reads as chip-specific when it is not. Someone will copy it into a project on a modern compiler. And the reason the vendor toolchain cannot simply be swapped for upstream is not the T-Head extensions, which GCC has carried since 13. It is -mtune=e907, the unratified p extension, and zpsfoperand/xtheade, none of which upstream spells - still true in the GCC 15.2 and 16.1 manuals. Written down so the next person does not repeat the search. --- contrib/tangcore-bl616/README.md | 54 ++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/contrib/tangcore-bl616/README.md b/contrib/tangcore-bl616/README.md index a8ca07e2..f4482ee6 100644 --- a/contrib/tangcore-bl616/README.md +++ b/contrib/tangcore-bl616/README.md @@ -147,6 +147,12 @@ shim defines - verified by `nm -u` on the compiled objects at the real ABI. Measured effect on a test image: 154,514 B of text down to 54,569 B, with zero `libstdc++.a` members linked. +**The shim exists because the toolchain is GCC 10.2, not because of anything +about this chip.** `-nostdlib++` does the same job in one flag and has been +available since GCC 11, so if this ever moves to a newer toolchain, delete the +shim rather than carrying it forward. Do not copy it into a project that is +already on a modern compiler. + **3. Use the vendor toolchain.** The T-Head GCC at `toolchain_gcc_t-head_linux` and Debian's `gcc-riscv64-unknown-elf` share the `riscv64-unknown-elf-` prefix, so PATH order decides which one you get, and @@ -156,6 +162,54 @@ so micro-flac will not compile against it, for reasons that say nothing about the BL616. Check with `riscv64-unknown-elf-gcc --version`: the vendor one reports "Xuantie-900". +Upstream GCC is not an option here yet, and it is worth writing down why so +nobody re-derives it. The T-Head vendor extensions themselves are not the +obstacle - GCC has had the XThead* collection since GCC 13. Three other things +are, and all three were still missing when checked against the GCC 15.2 and +16.1 manuals (2026-09): + +- `-mtune=e907`, which `bouffalo_sdk` sets, is rejected as an unknown cpu. + GCC 16 did grow the Xuantie application cores - `xt-c908`, `xt-c910`, + `xt-c920` and their variants - but not the small embedded E907. +- the `p` (packed SIMD) extension in the ABI string below is not in GCC's + `-march` table at all; it is still unratified, and the implementations that + exist live in vendor forks. +- `zpsfoperand` and `xtheade` likewise have no upstream spelling. + +`-mtune=size` is the documented substitute for the first, at the cost of the +core-specific tuning. The other two have no substitute. The vendor toolchain is +also frozen: its last commit is from October 2022. So this is a real constraint +rather than an upgrade nobody got round to. + +Clang does not unblock it either, checked at the same time against LLVM main. +It carries the same XThead* extensions, knows no E907 either (its only Xuantie +processors are `xt-c910v2` and `xt-c920v2`), and rejects `xtheade` and +`zpsfoperand` outright. It does have a `p` extension where GCC has none - but +as `experimental-p` behind `-menable-experimental-extensions`, implementing +draft 0.21, whereas `zpsfoperand` belongs to the older 0.9.x drafts this core +was built to. So they are not the same instruction set, and P being ratified +some day would not by itself make an upstream compiler target this chip. + +The vendor fork is the only route, and it has moved since the pin above. +[XUANTIE-RV/gcc](https://github.com/XUANTIE-RV/gcc) carries three branches +(checked 2026-09): + +| branch | last commit | declares `e907` | +|---|---|---| +| `xuantie-gcc-10.2.0` | 2024-07 | yes - c906, c908, c910, c920, e902, e906, e907 | +| `xuantie-gcc-10.4.0` | 2024-12 | yes, plus the c907 family | +| `xuantie-gcc-14.1.1` | 2025-03 | **no** - `riscv-cores.def` is upstream's, no Xuantie cores at all | + +So the GCC 14 branch cannot build this chip yet; it looks like a rebase in +progress rather than a finished port. `xuantie-gcc-10.4.0` can, and is two +years of GCC fixes newer than the GCC 10.2 blob pinned in +`bl616-tangcore-build.yml` - but it is still below GCC 11, so it does not +retire the shim above. Moving to it is `firmware-bl616`'s call, not ours. + +The community forks are not an alternative: `openbouffalo/xuantie-gnu-toolchain` +was last pushed in 2023 and `revyos/xuantie-gnu-toolchain` in 2024, both behind +the upstream they forked. + micro-flac itself compiles clean at the real BL616 ABI (`-march=rv32imafcpzpsfoperand_xtheade -mabi=ilp32f`, zero warnings) and its object code is smaller than dr_flac's there: 36,304 B against 45,129 B. From 97cb19927b6c1391221ad945debbaf35a376256a Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Wed, 9 Sep 2026 22:41:26 +0200 Subject: [PATCH 3/3] Fix the micro-flac SHA check, which rejected every valid SHA CMake's regex engine has no {n} repetition, so "^[0-9a-fA-F]{40}$" matches nothing at all and the guard fired on the correct SHA the workflow passes. Checked both ways this time: the pinned SHA configures, and "main" is still refused. The first test only covered the branch name, which failed for the wrong reason and looked like success. Co-Authored-By: Claude Opus 5 (1M context) --- CMakeLists.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3ab4f1a1..7c3ae2ec 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -173,7 +173,12 @@ if(CHDR_FLAC_BACKEND STREQUAL "microflac") if(CHDR_MICROFLAC_SOURCE_DIR) set(_microflac_dir "${CHDR_MICROFLAC_SOURCE_DIR}") else() - if(CHDR_MICROFLAC_GIT_TAG AND NOT CHDR_MICROFLAC_GIT_TAG MATCHES "^[0-9a-fA-F]{40}$") + # Length and character class separately: CMake's regex engine has no {n} + # repetition, so "[0-9a-f]{40}" silently matches nothing and would reject + # every valid SHA. + string(LENGTH "${CHDR_MICROFLAC_GIT_TAG}" _chdr_microflac_tag_len) + if(CHDR_MICROFLAC_GIT_TAG AND (NOT _chdr_microflac_tag_len EQUAL 40 + OR NOT CHDR_MICROFLAC_GIT_TAG MATCHES "^[0-9a-fA-F]+$")) message(FATAL_ERROR "CHDR_MICROFLAC_GIT_TAG must be a 40-character commit SHA, got: " "${CHDR_MICROFLAC_GIT_TAG}")