Add Bungee Stretching/Keylock Engine - #5
Open
0cwa wants to merge 50 commits into
Open
Conversation
…I calls and wiring it into the keylock engine system
Summary of changes:
1. BUNGEE=ON remains default (CMakeLists.txt line 4594)
2. Created lib/bungee/.gitignore that excludes:
- Build artifacts (build/, *.a, *.so, etc.)
- CMake generated files
- Documentation (README.md, LICENSE, doxygen/, doc/, demos/)
- Command-line tool (cmd/)
- CI configuration (.github/, ci/)
- Eigen submodule extras (benchmarks, tests, docs)
- cxxopts submodule (not needed for library)
3. Files included in lib/bungee/ (434 files):
- bungee/ - 5 public headers (Bungee.h, Stream.h, Modes.h, Push.h, CommandLine.h)
- src/ - 26 source files + internal headers
- submodules/pffft/ - 5 files (pffft.c/h, fftpack.c/h, test_pffft.c)
- submodules/eigen/Eigen/ - ~390 header files (template library)
- .gitignore and .gitmodules
4. No custom CMakeLists.txt in lib/bungee/ - The build is handled entirely by the main CMakeLists.txt which creates the bungee and bungee-pffft targets.
5. Easy updates - To update bungee:
cd ../bungee && git pull && git submodule update --init
Fix std::min type mismatch in EngineBufferScaleBungee::processGrain
The Dual-threaded Stereo checkbox was incorrectly enabled for the Bungee keylock engine. This feature is only available with RubberBand engines (RubberBandFaster and RubberBandFiner). Changed the logic in updateKeylockDualThreadingCheckbox() to explicitly check for RubberBand engines only, rather than checking 'not SoundTouch' which incorrectly included Bungee. Now the checkbox is properly: - Enabled for: RubberBandFaster, RubberBandFiner - Disabled for: SoundTouch, Bungee The existing tooltip 'Dual threading mode is only available with RubberBand' now correctly appears for both Bungee and SoundTouch engines.
- Add CMake configuration to apply lib/bungee/0001-MSVC-compatibility.patch - Add MSVC-specific build configuration: * Conditional GCC flags for bungee-pffft * _USE_MATH_DEFINES for M_PI on Windows * Conditional BUNGEE_VISIBILITY definition - Keep lib/bungee source files in upstream state The patch provides: - Platform.h with BUNGEE_NOINLINE macro - Resample.h using BUNGEE_NOINLINE - Assert.cpp with Windows header support
Integrate MSVC compatibility patch for bungee library
…stereo-checkbox-for-bungee-key, check pr still builds
…ereo-checkbox-for-bungee-key fix: disable Dual-threaded Stereo checkbox for Bungee keylock engine
Auto-fixes from running pre-commit hooks: - Remove trailing whitespace in FindBungee.cmake - Fix code formatting in bungee-related engine files - Fix code formatting in bungee test file - Reorder includes and using statements alphabetically
Fix crash when clear() is called before stretcher is initialized by properly resetting output chunk state. Fix waveform lockup/jiggle when playing before track is fully loaded: - Use fixed grain size (kMaxGrainFrames) for consistent position tracking - Reset grain position on clear() to prevent drift - Don't invalidate position when input is unavailable, allowing retry - Initialize output chunk in onSignalChanged() even without valid signal
Fix crash when switching to Bungee while playing by removing the specifyGrain() call from clear(). The reset is now handled lazily on the next processGrain() call. Fix waveform lockup/jiggle when playing before track is fully loaded: - Use fixed grain size (kMaxGrainFrames) for consistent position tracking - Don't invalidate position when input is unavailable, allowing retry - Add signal validity checks in scaleBuffer() and processGrain() - Initialize output chunk in onSignalChanged() even without valid signal - Add null check for m_outputChunk.data before using it
Fix Bungee engine waveform lockup/jiggle and crash issues
This comment was marked as duplicate.
This comment was marked as duplicate.
0cwa
marked this pull request as ready for review
February 19, 2026 11:22
Review Summary by QodoAdd Bungee audio stretching engine with grain-based processing and comprehensive testing
WalkthroughsDescription• Implements EngineBufferScaleBungee, a new audio time-stretching and pitch-shifting engine using the Bungee library with grain-based processing • Integrates Bungee stretcher into the engine buffer system with conditional compilation support via __BUNGEE__ preprocessor flag • Adds comprehensive unit test suite with 13 test cases covering playback scenarios, variable speeds, keylock mode, pitch shifting, reverse playback, and edge cases • Implements complete Bungee stretcher pipeline including grain analysis/synthesis, phase vocoder with partial tracking, FFT operations using PFFFT, and windowing • Updates keylock engine dual-threading checkbox logic to explicitly support RubberBand variants while preparing for Bungee which lacks dual-threading support • Includes Eigen linear algebra library as a submodule dependency for matrix operations required by the Bungee stretcher File Changes1. src/test/enginebufferscalebungeetest.cpp
|
Owner
Author
|
/review --ignore.glob="['lib/bungee/**']" |
Fix Bungee keylock super fast speedup in Release builds
Owner
Author
|
@ronso0 Ok, I got my custom coding harness to work on it with some proper models and it seems to work well when I compile it now :). It should be good :D I'm gonna probably throughly test it tomorrow! |
Owner
Author
|
/review --ignore.glob="['lib/bungee/**']" |
|
Yeajii, did a quick test and it sounds good -- no obvious glitches anymore. And I can lower the audio buffer by one step (5ms currently) compared to RubberBand3 🎉 Will do a more thorough test soonish. Incl. side by side comaprison with RubberBand3 and SignalSmith (mixxxdj#15902) with this commit |
…analyseGrain overflow
Two related bugs caused a deterministic heap corruption crash detectable
via SIGABRT from malloc_printerr when Bungee keylock was active:
Bug 1 — discardBufferedInputBefore position tracking
When framePosition > m_bufferedInputEndFrame (all buffered data is before
the requested position — which happens at very high playback speeds where
the grain hops outrun the read buffer), the function discarded all frames
correctly via memmove(0 bytes) but then only advanced m_bufferedInputBeginFrame
to m_bufferedInputEndFrame (the old end) rather than all the way to
framePosition. This left a gap:
m_bufferedInputBeginFrame = m_bufferedInputEndFrame_old (WRONG)
m_bufferedInputEndFrame = m_bufferedInputBeginFrame (= old end)
On the next processGrain call, dataOffset was computed as
inputChunk.begin - m_bufferedInputBeginFrame
which equalled framePosition - m_bufferedInputEndFrame_old, a value that
could exceed (m_channelStride - grainSize). Bungee then created an Eigen
map starting at m_channelBufferPtrs[0] + dataOffset and iterated over the
full grainSize rows, reading past the end of m_contiguousChannelBuffer for
channel 1 and corrupting adjacent heap memory. Malloc detected this later
(in an unrelated thread cleaning up its arena) and called abort().
Fix: when remainingFrames <= 0 (all data discarded), set
m_bufferedInputBeginFrame = framePosition (not += bufferedFrames).
Bug 2 — DEBUG_ASSERT too weak
The existing assert only checked dataOffset <= m_channelStride, but the
actual requirement for the analyseGrain call is
dataOffset + grainSize <= m_channelStride.
Strengthened the assert and added a run-time guard that forces a reset
instead of overflowing if the invariant is ever violated.
Bonus — zero-initialise the input buffer
On the very first grain after reset (request.position = 0), Bungee's
InputChunk is {-halfFrames, +halfFrames}. We supply data for
[0, halfFrames) but the Eigen map covers the full [0, 2*halfFrames) range.
The upper half was previously uninitialized; zero-filling prevents Bungee
from seeing garbage floats (including possible NaN) in that region.
All 15 Bungee tests (12 unit + 3 integration) continue to pass.
Re-validate the BNG-12 final review claim against the post-87e48497c1
state of the integration code, and update the user-facing integration
doc so a future maintainer reading only docs/bungee-integration.md and
the current code can reconstruct the buffer-window invariant without
consulting commit history.
Code review findings (no source changes required):
* No stale comments or dead code paths from the crash-debugging era
remain in src/engine/bufferscalers/enginebufferscalebungee.{h,cpp}.
* The buffer-window state model
(m_bufferedInputBeginFrame / m_bufferedInputEndFrame / m_channelStride)
is now documented coherently in one place in the header (the new
"Buffer-window invariant (BNG-13)" section, added together with the
regression tests).
* discardBufferedInputBefore() and processGrain() honour a single
stated invariant: dataOffset + grainSize <= m_channelStride. Both
branches of discardBufferedInputBefore (partial / full discard) and
the runtime guard in processGrain are documented in the header.
* The original BNG-12 acceptance bullets ("focused diff, legible
commits, no misleading commentary") still hold once BNG-13 has
landed -- the regression test commit is the only addition and it
has its own ticket.
Doc updates (docs/bungee-integration.md):
- Add a "buffer-window invariant" subsection under the InputChunk
contract that states the post-BNG-13 contract explicitly:
begin <= chunk.begin and dataOffset + grainSize <= channelStride,
plus the partial- and full-discard branches and the
high-speed grain-outrun regime.
- Add a subsection explaining why m_contiguousChannelBuffer is
zero-initialised in onSignalChanged (muted half of the very first
post-reset grain).
- Extend the "What not to change casually" table with two new rows:
the discardBufferedInputBefore full-discard branch (pinned by
EngineBufferScaleBungeeBufferWindowTest) and the
m_contiguousChannelBuffer zero-init.
- Add a "Test surface" section enumerating the unit / regression /
integration test files and the .github/workflows/bungee-asan.yml
workflow so future maintainers can find the regression net
immediately.
Refs: TODO.md BNG-14. Depends on BNG-13.
…ckets Adds a structured plan under docs/plans/bungee-dependency-integration/ for replacing the checked-in lib/bungee/ source copy with a normal dependency integration, plus eight focused tickets (BNG-17 through BNG-24) in docs/tasks/ tracking each step. Key decisions captured: - BUNGEE defaults to ON for this PR. - Recommending CMake 3.30+ for Bungee-enabled builds is acceptable; the global CMake hard minimum must not be raised. - Future decision requests include pros/cons lists and a recommendation. BNG-18 research findings: - Every Bungee release (v2.1.8..v2.4.24) requires cmake_minimum 3.30..3.31. No older-CMake-compatible Bungee release exists. - bungee/Bungee.h is API-identical between v2.4.15 and v2.4.24. - mixxxdj/vcpkg does not yet have the Bungee port; microsoft/vcpkg added it in PR #50120 (merged 2026-03-19 at commit ae92331). Tickets BNG-17 (assumptions) and BNG-18 (version matrix) are done. BNG-19 (CMake normalization) and BNG-20 (vcpkg overlay) follow in the next two commits. BNG-21..24 are open for the next session.
… (BNG-19)
Restructures the BUNGEE block in CMakeLists.txt so that all provider paths
produce a single imported target named Bungee::Bungee. Downstream code never
needs to know whether Bungee came from a vcpkg port, a system package, or the
vendored fallback.
Discovery order:
1. find_package(Bungee CONFIG) - upstream config package (future)
2. find_package(unofficial-bungee CONFIG) - vcpkg PR #50120 port, wrapped
in a Bungee::Bungee INTERFACE IMPORTED target
3. find_package(Bungee MODULE) - pkg-config / system path via
cmake/modules/FindBungee.cmake
4. Vendored lib/bungee/ fallback - existing direct-source build,
now inside else() and normalized via add_library(Bungee::Bungee ALIAS
bungee). Marked for removal in BNG-22.
cmake/modules/FindBungee.cmake is updated to use the libbungee pkg-config
module name (matching the name the vcpkg port installs) and to search for
both 'bungee' and 'libbungee' library names.
BUNGEE=ON and BUNGEE=OFF both configure cleanly. BUNGEE_PETRIFY_DEBUG emits
a warning when Bungee comes from a package rather than the vendored tree.
…on C) mixxxdj/vcpkg does not yet contain the Bungee port. As a bridge until it does, this commit adds a verbatim copy of the microsoft/vcpkg Bungee port (PR #50120, merged 2026-03-19, commit ae92331) under: cmake/vcpkg-overlay-ports/bungee/ CMakeLists.txt appends that directory to VCPKG_OVERLAY_PORTS after the mixxxdj/vcpkg platform overlays (osx, windows), so if mixxxdj/vcpkg ever ships its own Bungee port it takes precedence automatically and this overlay becomes inert. The port installs Bungee 2.4.15 (eigen3 + pffft dependencies) and exposes: find_package(unofficial-bungee CONFIG REQUIRED) target_link_libraries(... PRIVATE unofficial::bungee::bungee) The BNG-19 discovery block wraps that target in Bungee::Bungee so all Mixxx code remains provider-agnostic. Removal plan (BNG-20 Option A): once a PR is merged into mixxxdj/vcpkg that cherry-picks the Bungee port from microsoft/vcpkg and a buildenv artifact is published, revert this entire commit and delete the cmake/vcpkg-overlay-ports/ tree. See cmake/vcpkg-overlay-ports/bungee/README.md for full provenance.
Discovery order is now: package CONFIG → vcpkg CONFIG → MODULE → ExternalProject_Add → vendored. Vendored stays as the absolute last resort until BNG-22 deletes it. The ExternalProject path is gated on a new option BUNGEE_FETCH_FALLBACK (default ON). It pins Bungee to v2.4.15 to match the vcpkg overlay port from BNG-20, with a hard-coded URL_HASH SHA256 and the libdjinterop-style forwarding of CMAKE_BUILD_TYPE / CMAKE_PREFIX_PATH (pipe-delimited) / CMAKE_FIND_ROOT_PATH / CMAKE_MODULE_PATH / CMAKE_TOOLCHAIN_FILE / OSX deploy + arch / CMAKE_SYSTEM_NAME + PROCESSOR. Patches reused verbatim from the vcpkg overlay (no duplication) plus one new Mixxx-only patch cmake/patches/bungee/lower-cmake-minimum.patch that drops Bungee's declared cmake_minimum_required(VERSION 3.30...3.31) to 3.21 (Mixxx's global minimum). A WARNING fires when host CMake < 3.30 telling users the source fallback is unsupported by Bungee maintainers and pointing at Kitware PPA / Homebrew / vcpkg as alternatives. Patches are applied via cmake/patches/bungee/apply-patches.cmake invoked from PATCH_COMMAND with cmake -P. It runs GNU patch -p1 -l -N because the upstream Bungee tarball ships trailing whitespace on lines that the upstream-derived patches normalise away (git apply --ignore-whitespace does not handle that). MSVC-only patches (assert-win32-compat, resample-msvc-noinline) are gated on a flag passed in by the parent CMake. If any prerequisite is missing — Eigen3 + pffft as CMake CONFIG packages, or GNU patch on PATH — we emit an actionable STATUS line and fall through to the vendored fallback so configure stays clean. The imported Bungee::Bungee STATIC target carries INTERFACE_LINK_LIBRARIES "pffft::pffft;Eigen3::Eigen" so the patched Bungee static library's transitive deps are resolved at the mixxx-lib link step. INSTALL_DIR layout matches the cmake-use-vcpkg-deps-and-install-layout patch (lib/<prefix>bungee<suffix>, include/bungee/*.h). Verified: - -DBUNGEE=ON configures clean (MODULE-mode picks up local Bungee or the ExternalProject path is silently skipped — both fine). - -DBUNGEE=ON with no system Bungee and no Eigen3/pffft falls back to vendored with the new STATUS message, configures clean. - -DBUNGEE=ON with stub Eigen3+pffft CONFIG packages exercises the ExternalProject branch; building bungee_external downloads the v2.4.15 tarball, applies all required patches, and the resulting CMakeLists has cmake_minimum_required(VERSION 3.21), include(GNUInstallDirs), and find_package(Eigen3/pffft CONFIG REQUIRED). - -DBUNGEE=OFF configures clean; __BUNGEE__ is not defined and enginebufferscalebungee.cpp.o is not in mixxx-lib's link line. Pending: full bungee_external build on a host with real Eigen3 + pffft installed — that exercise belongs in BNG-23 (CI / packaging walkthrough) where Mixxx's build environments either install the system packages or provide them via vcpkg. No changes to lib/bungee/.
BNG-23 was hitting all three difficulty signals: large body, multiple unrelated surfaces (CMake/Flatpak/CI), and the Eigen pin research changing the implementation contract. Split into four implementation children + one deferred placeholder so each lands as a focused PR. Children: - BNG-25 (new): add Eigen3 + pffft as ExternalProject_Add targets - BNG-26 (new): wire them into bungee_external; remove vendored fallthrough; this is the commit that unblocks BNG-22 - BNG-27 (new): Bungee/Eigen3/pffft as Flatpak modules - BNG-28 (new): GitHub Actions workflow updates - BNG-29 (new, deferred): patch reorganization to cmake/patches/, trigger condition documented Dependency adjustments: - BNG-22 blocked_by flipped from [BNG-19,BNG-20] to [BNG-21,BNG-26] (vendored deletion now correctly depends on the unblocking child) - BNG-23 rewritten as thin umbrella ticket; canonical pin research recorded here once so children don't re-derive - BNG-24 blocked_by expanded to all four implementation children Research corrections vs original handoff: - pffft upstream is bitbucket.org/jpommier/pffft.git (verified from Bungee v2.4.15 submodule pointer via git ls-tree v2.4.15:submodules), NOT marton78/pffft as previously suggested. Exact commit pin: 02fe7715a5bf8bfd914681c53429600f94e0f536 - Bungee v2.4.15 pins Eigen 3.4.90 (master snapshot, verified via EIGEN_WORLD/MAJOR/MINOR_VERSION macros in submodule headers). We deliberately deviate to Eigen 3.4.0 stable per Mixxx convention (libdjinterop, rubberband, etc. all use stable tarballs); BNG-25 smoke-tests this with documented c29c8001 commit-pin fallback if the upstream Bungee compile against 3.4.0 fails. - pffft CMakeLists.txt.in template mirrors Mixxx's verbatim vendored recipe (which itself matches Bungee upstream's own pffft recipe). - No Eigen3Config.cmake.in template needed - 3.4.0 ships its own. - No CMake-side Flatpak detection needed - Mixxx CMakeLists has zero Flatpak code; find_package's default search picks up /app/lib/cmake/* automatically when modules are listed before mixxx in the manifest. No code changes - tickets only. The runnable head of the chain is now BNG-25; manage_ticket_manifest action=list_runnable confirms.
Adds two ExternalProject_Add blocks inside the existing BUNGEE_FETCH_FALLBACK
branch:
- eigen3_external: Eigen 3.4.0 stable
URL: https://gitlab.com/libeigen/eigen/-/archive/3.4.0/eigen-3.4.0.tar.gz
SHA256=8586084f71f9bde545ee7fa6d00288b264a2b7ac3607b974e54d13e7162c1c72
Header-only — BUILD_COMMAND "" so the default INSTALL step copies
headers + Eigen3Config.cmake straight from CONFIGURE.
- pffft_external: jpommier upstream commit 02fe7715
URL: https://bitbucket.org/jpommier/pffft/get/02fe7715a5bf8bfd914681c53429600f94e0f536.tar.gz
SHA256=9adeb18ac7bb52e9fb921c31c0c6a4e9ae150cc6fcb20a899d4b3a2275176ded
pffft has no upstream CMake; PATCH_COMMAND drops in
cmake/patches/pffft/CMakeLists.txt.in (the same recipe Bungee
upstream uses in its own CMakeLists). No -march= flags — Mixxx's
top-level CMake injects -march=native or /arch:SSE2 already.
Both targets are EXCLUDE_FROM_ALL: BNG-26 wires them into bungee_external
and removes the find_package() + vendored fallthrough.
Eigen 3.4.0 vs c29c8001: Bungee v2.4.15's submodule pointer is c29c8001
(an Eigen master snapshot), but Bungee has zero Eigen version constraint
and Mixxx convention prefers stable releases. Smoke-test verified Bungee
v2.4.15 compiles cleanly against Eigen 3.4.0 stable headers, so no
fallback to the c29c8001 commit pin is needed. The fallback path is
documented in the BNG-25 ticket if a future Bungee bump regresses.
Smoke-test artifacts produced (with Bungee discovery forced to NOTFOUND
to exercise the BNG-21 fallback branch):
build/eigen3-install/share/eigen3/cmake/Eigen3Config.cmake
build/pffft-install/lib/cmake/pffft/pffftConfig.cmake
build/pffft-install/lib/libpffft.a
Acceptance-criteria validation:
- cmake -B build -DBUNGEE=ON exit 0 artifacts present
- cmake --build build --target eigen3_external pffft_external exit 0
- Bungee v2.4.15 against Eigen 3.4.0 (scratch) exit 0 builds clean
- cmake -B build -DBUNGEE=OFF exit 0
- cmake -B build -DBUNGEE=ON -DBUNGEE_FETCH_FALLBACK=OFF exit 0
(eigen3_external + pffft_external NOT registered — correct, they live
inside the BUNGEE_FETCH_FALLBACK branch)
- bungee_external untouched in this commit (BNG-26 wires it)
Adversarial code review: 4 findings — 1 nit fixed (misleading comment
about CONFIGURE vs INSTALL step ownership), 3 false positives about the
ticket-move workflow that the orchestration sequence handles between
review and commit (file moved + status flipped + transitions log
appended in this same commit).
Refs: docs/tasks/done/BNG-25.md
Unblocks: BNG-26
Wire eigen3_external and pffft_external into bungee_external using manual imported targets rather than configure-time find_package calls. The fallback now passes dependency install prefixes to Bungee's sub-build via CMAKE_PREFIX_PATH, hard-fails when GNU patch or any Bungee provider is unavailable, and leaves the vendored lib/bungee block physically present but unreachable for BNG-22 to delete. Validation: - cmake -B build_bng26_on -DBUNGEE=ON with local Bungee discovery disabled: passed - cmake --build build_bng26_on --target bungee_external -j2: passed - cmake --build build_bng26_on --target mixxx-lib -j4: passed - cmake -B build_bng26_off -DBUNGEE=OFF: passed - cmake -B build_bng26_pkgonly -DBUNGEE=ON -DBUNGEE_FETCH_FALLBACK=OFF: passed via local /home/x2/bungee module discovery - pre-commit run on changed files: passed Adversarial review: no blocking findings; one low-severity note on patch/gpatch search ordering accepted as non-blocking.
Delete the checked-in lib/bungee source tree and remove the now-dead direct-build CMake fallback, including bungee-pffft, the vendored bungee target, vendored include paths, the MSVC git-apply patch block, and the Bungee::Bungee alias for that target. The Bungee discovery order now ends at the ExternalProject/package providers, and live integration docs describe dependency-provider patching instead of a checked-in vendor tree. The source-fetch provider introduced in BNG-21/25/26 now requires a patch executable at configure time. Distro/package docs should list GNU patch (or an equivalent platform patch tool) as a prerequisite when relying on BUNGEE_FETCH_FALLBACK=ON. Validation: - cmake -B build_bng22_on -DBUNGEE=ON with local Bungee discovery disabled: passed - cmake --build build_bng22_on --target bungee_external -j4: passed - cmake --build build_bng22_on --target mixxx-lib -j4: passed - cmake -B build_bng22_off -DBUNGEE=OFF: passed - cmake -B build_bng22_pkgonly -DBUNGEE=ON -DBUNGEE_FETCH_FALLBACK=OFF: passed via local /home/x2/bungee module discovery - pre-commit run on changed files: passed Adversarial review: no blocking findings; accepted one wording nit in the BUNGEE_PETRIFY_DEBUG warning.
Summary: - Add Flatpak modules for Eigen3, pffft, and Bungee, then insert them before the mixxx module so Flatpak builds resolve Bungee as an installed package instead of using ExternalProject downloads - The Bungee module applies the existing Linux patches, installs a Flatpak-compatible unofficial-bungee CMake config, and uses shared-library output for runtime-safe linking; local validation covered YAML/pre-commit checks, tarball hashes, patch application, and a CMake find_package smoke test while flatpak-builder is deferred to CI because it is unavailable locally. Changed files: - packaging/flatpak/modules/bungee.yaml - packaging/flatpak/modules/eigen3.yaml - packaging/flatpak/modules/pffft.yaml - packaging/flatpak/org.mixxx.Mixxx.yaml Closes: BNG-27
Update the Bungee ASan workflow path filters for the non-vendored layout, replacing lib/bungee/** with the Bungee patch, vcpkg overlay, FindBungee, scaler, test, CMake, and workflow files that can affect the integration. Keep the existing Mixxx ASan flag (-DSANITIZE_ADDRESS=ON, verified from CMakeLists.txt) and add job-level CFLAGS/CXXFLAGS/LDFLAGS so ExternalProject_Add child CMake builds inherit sanitizer instrumentation from the runner environment without changing CMakeLists.txt. Verified build.yml needs no change: the Ubuntu matrix still configures -DBUNGEE=ON and the build step runs cmake --build without a target filter, so bungee_external/eigen3_external/pffft_external remain reachable through the normal mixxx-lib build chain. Validated with Ruby YAML parsing and pre-commit on the touched workflow/task files.
Add a draft PR description with dependency provenance, local spot checks, a focused validation matrix for the non-vendored Bungee dependency commits, and a full branch commit ledger for follow-up CI URLs. Record the experimental branch audit and explicitly document draft-only caveats found during adversarial review: legacy/pre-plan commits still need CI evidence or history linearization, nine merge commits remain in the branch, Flatpak/path-filter smoke tests require pushed CI, and Windows/macOS vcpkg overlay activation must be confirmed by CI. Close the BNG-23 umbrella now that BNG-25, BNG-26, BNG-27, and BNG-28 are complete. Validated the PR draft and task updates with pre-commit and a clean ticket manifest audit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 35
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/preferences/dialog/dlgprefsound.cpp (1)
806-817:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let availability disablement erase the saved RubberBand preference.
This function now disables the checkbox for Bungee and mono mix, but
DlgPrefSound::slotApply()still persistsisChecked() && isEnabled(). With Bungee selected, simply clicking Apply will silently writefalseand forget the user's previous RubberBand multithreading preference. Preserve the stored value independently from UI availability, and only gate whether the option can take effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/preferences/dialog/dlgprefsound.cpp` around lines 806 - 817, The UI currently disables keylockDualthreadedCheckBox based on currentEngine and monoMix, but DlgPrefSound::slotApply() persists the preference using keylockDualthreadedCheckBox->isChecked() && keylockDualthreadedCheckBox->isEnabled(), which causes the saved RubberBand multithreading preference to be overwritten when the checkbox is disabled (e.g., Bungee or mono). Change the logic so slotApply() reads and writes the stored preference independently of the widget enabled state (use only keylockDualthreadedCheckBox->isChecked() to update the stored setting, or better: keep the stored setting value separate and only use keylockDualthreadedCheckBox->isEnabled() to decide whether to apply it at runtime), and ensure the availability logic in the method that sets the tooltip/enablement (the code using currentEngine/EngineBuffer::KeylockEngine, monoMix, keylockDualthreadedCheckBox->setEnabled/setToolTip) does not clear or mutate the stored preference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Line 58: The CI currently only enables DBUNGEE=ON for Linux; add at least one
non-Linux matrix job that also sets DBUNGEE=ON (e.g., add macos-latest or
windows-latest to the workflow matrix) so a BUNGEE-enabled macOS/Windows lane
runs; update the matrix definition that contains the DBUNGEE=ON entry to include
the additional os value and ensure the env/strategy keys that reference
DBUNGEE=ON apply to that new job.
In @.github/workflows/bungee-asan.yml:
- Around line 28-48: The workflow's path filters in
.github/workflows/bungee-asan.yml are too narrow and may miss integration
changes; update the push and pull_request paths entries (the paths block in that
file) to broaden coverage—either add globs that capture all Bungee-related code
(for example include "src/**/bungee/**", "cmake/**/bungee/**", and any engine
integration directories) or remove the restrictive path filter so the ASan job
runs on relevant changes; ensure both the push and pull_request sections are
updated consistently.
In `@cmake/modules/FindBungee.cmake`:
- Around line 53-56: The pkg-config usage currently calls
pkg_check_modules(PC_Bungee) in variable mode and builds Bungee::Bungee manually
with only IMPORTED_LOCATION and INTERFACE_INCLUDE_DIRECTORIES, dropping
transitive link deps; change pkg_check_modules to create an imported target (use
pkg_check_modules(... IMPORTED_TARGET) so PkgConfig::PC_Bungee exists) and then
either target_link_libraries(Bungee::Bungee PUBLIC PkgConfig::PC_Bungee) or copy
PkgConfig::PC_Bungee's INTERFACE_LINK_LIBRARIES and INTERFACE_LINK_OPTIONS onto
Bungee::Bungee (in addition to include dirs) instead of only setting
IMPORTED_LOCATION, ensuring transitive link requirements like pffft/Eigen
propagate.
In `@cmake/patches/bungee/lower-cmake-minimum.patch`:
- Around line 20-21: The cmake_minimum_required call was changed to a bare
version which leaves CMake policies between 3.22 and 3.31 unset; revert to an
explicit range to preserve Bungee's tested policy set by changing the
cmake_minimum_required(...) invocation back to use "3.21...3.31" (i.e., update
the cmake_minimum_required symbol to include the upper bound) while keeping the
existing patch header/comment intact.
In
`@cmake/vcpkg-overlay-ports/bungee/cmake-use-vcpkg-deps-and-install-layout.patch`:
- Around line 110-117: Fix the leftover indentation and document the
Apple-framework caveat: unindent the two set() calls for PKGCONFIG_LINK_PATH and
PKGCONFIG_LINK_FLAG so they are at file-scope (they currently appear indented as
if inside an if(APPLE) block), and add a single-line comment near the
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/libbungee.pc.in ...) or before the
install(...) that states the generated libbungee.pc is intended for the
static/non-framework path (BUNGEE_BUILD_SHARED_LIBRARY=OFF) and therefore does
not include -F/-framework handling for Apple frameworks.
In `@cmake/vcpkg-overlay-ports/bungee/resample-msvc-noinline.patch`:
- Around line 9-13: The current BUNGEE_NOINLINE macro fallback unconditionally
uses __attribute__((noinline)) and should be guarded to avoid errors on
non-GCC/Clang compilers; update the macro definition around BUNGEE_NOINLINE so
the _MSC_VER branch stays the same but the non-MSVC branch first checks for
support (use __has_attribute(noinline) if available or compiler-specific macros)
and only defines BUNGEE_NOINLINE as __attribute__((noinline)) when supported,
otherwise define it as empty; adjust the preprocessor logic near the existing
BUNGEE_NOINLINE definition to perform the capability check and provide a safe
no-op fallback.
In `@CMakeLists.txt`:
- Around line 4636-4640: The CMake cache option BUNGEE_PETRIFY_DEBUG is dead and
only triggers a warning; remove the option declaration (the
option(BUNGEE_PETRIFY_DEBUG ...) block) and delete the code that consumes it
(the message(WARNING ...) that says it has no effect), or alternatively move it
into a dedicated removed-options section and replace the warning with a
message(FATAL_ERROR ...) so the build fails fast and users are forced to update
scripts; locate occurrences by the unique symbol BUNGEE_PETRIFY_DEBUG and the
related message(WARNING ...) and apply one of these two fixes consistently.
- Around line 2867-2868: CMake currently gates the entire
settingsmanager_test.cpp file behind the BUNGEE option which hides general
tests; update the build so only the Bungee-specific test is gated: either split
settingsmanager_test.cpp into two files (e.g., settingsmanager_bungee_test.cpp
containing SeedsBungeeKeylockEngineForFreshSettingsDirectory and
settingsmanager_test.cpp containing
DoesNotSeedBungeeKeylockEngineForExistingSettingsDirectory and
PreservesExplicitKeylockEngineInExistingSettingsDirectory) or change the CMake
entry to include the file unconditionally and guard only the
SeedsBungeeKeylockEngineForFreshSettingsDirectory test with preprocessor/macro
checks; ensure CMakeLists references the new file names (or the un-gated
filename) and that the Bungee-only test remains conditioned on BUNGEE.
In `@docs/bungee-integration.md`:
- Around line 73-83: The doc's formulas for muteHead/muteTail are inverted and
missing the availableEnd clamp compared to the implementation in processGrain
(enginebufferscalebungee.cpp); update the documentation to match the code by
showing the intermediate availableBegin = max(bufferedInputBeginFrame,
inputChunk.begin) and availableEnd = max(availableBegin,
min(bufferedInputEndFrame, inputChunk.end)) and then muteHead = availableBegin -
inputChunk.begin and muteTail = inputChunk.end - availableEnd (which yields zero
in the steady state), referencing the same variable names used in processGrain
(m_bufferedInputBeginFrame, m_bufferedInputEndFrame,
m_currentInputChunk.begin/end) so future readers see the exact logic and
clamping used by the implementation.
In `@docs/plans/bungee-dependency-integration/07-execution-tickets.md`:
- Around line 63-81: The strategy block’s fallback branch "if package discovery
fails and an approved source fallback exists" is invalid per BNG-18; update that
section by inserting a brief cross-reference note (e.g., an HTML comment)
immediately after that paragraph stating that BNG-18 found no
older-CMake-compatible Bungee releases and linking to bng-18-version-matrix.md
for details (suggested text: "<!-- superseded by BNG-18: no
older-cmake-compatible release exists — see bng-18-version-matrix.md -->"), so
readers won’t pursue the dead fallback path and the rest of the logic remains
unchanged.
In `@docs/plans/bungee-dependency-integration/bng-18-version-matrix.md`:
- Around line 136-142: The plan text (Option B) conflicts with the committed
change that applies lower-cmake-minimum.patch which edits Bungee's
cmake_minimum_required(3.30...3.31) to Mixxx's current minimum (see
lower-cmake-minimum.patch and pr-description.md reference); either update this
document to state that Option A was chosen and explain why the patch was
applied, or revert/remove the lower-cmake-minimum.patch and instead implement
Option B (replace the patch with a configure-time check + user-facing message
pointing to upgrade CMake or install a package). Ensure the decision record
explicitly names the chosen option (Option A or B), references
lower-cmake-minimum.patch and the cmake_minimum_required change, and documents
the rationale and next actions so implementation and recommendation no longer
contradict.
In `@docs/plans/bungee-dependency-integration/pr-description.md`:
- Around line 28-33: The documentation currently omits an explicit note that
lower-cmake-minimum.patch implements Option A (overriding Bungee's stated
minimum) contrary to the Recommendation in bng-18-version-matrix.md which
favored Option B; update the "Maintainer-facing decisions already applied"
section to state that lower-cmake-minimum.patch was chosen instead of Option B,
reference the patch name (lower-cmake-minimum.patch) and the recommendation
document (bng-18-version-matrix.md), and add a concise rationale for the
departure (e.g., build compatibility with Mixxx's global CMake minimum and the
ExternalProject fallback behavior) so reviewers see the intentional decision.
- Around line 149-152: Remove the conditional guard that wraps the Bungee
overlay block (the if(NOT DEFINED VCPKG_OVERLAY_PORTS) / endif) and instead
always perform list(APPEND VCPKG_OVERLAY_PORTS ...) to add the
cmake/vcpkg-overlay-ports/bungee overlay after the platform-specific overlays;
do not overwrite VCPKG_OVERLAY_PORTS (use list(APPEND) rather than set) so
existing pre-set values are preserved and find_package(unofficial-bungee CONFIG)
can locate the overlay.
In `@docs/tasks/done/BNG-17.md`:
- Around line 17-20: The Status line in the document body ("Status" block
currently showing "open") contradicts the YAML front matter's state; update the
body Status block to match the front matter (change "open" to "done") or
vice‑versa so both the YAML front matter and the "Status" section are
consistent—look for the "Status" heading and the YAML front matter at the top of
the file and make their values identical.
In `@docs/tasks/done/BNG-23.md`:
- Around line 37-54: The canonical SHA256 placeholders for pffft and Bungee in
the "Pin research (canonical home)" section were never backfilled; update the
SHA256 lines under the pffft entry (referencing pffft commit
`02fe7715a5bf8bfd914681c53429600f94e0f536`) to
9adeb18ac7bb52e9fb921c31c0c6a4e9ae150cc6fcb20a899d4b3a2275176ded and the SHA256
line under the Bungee v2.4.15 entry (referencing Bungee v2.4.15) to
aa94ffe8ba49bcb916f454c5221d3480ae880f199e1516de31585924398ca67a so the
canonical record in BNG-23 is complete and no longer points to "compute at
implementation time".
In `@docs/tasks/done/BNG-26.md`:
- Around line 68-73: The scope text contradicts itself about removing the
vendored fallthrough branch: update the BNG-26 task description to pick one
clear option (either remove the entire else() vendored branch now or defer its
deletion to BNG-22) and make the chosen plan consistent across the BUNGEE block
description and the references to BNG-22/BNG-26 in the CMakeLists notes;
explicitly state which commit will delete the vendored lib/bungee/ tree and
which will only wire the new path so reviewers and backport/cherry-pick scripts
are unambiguous.
In `@docs/tasks/done/BNG-27.md`:
- Around line 25-89: The markdown checklist in the PR description was marked
"done" while many individual checklist items (e.g., Create
`packaging/flatpak/modules/eigen3.yaml`, pffft.yaml, bungee.yaml, update
`org.mixxx.Mixxx.yaml`, Verify mixxx module config, Smoke test) remain
unchecked; update the document so completed items are explicitly checked (`-
[x]`) and any remaining work items stay as unchecked (`- [ ]`) or moved into a
separate open task list, ensuring each referenced item (eigen3.yaml, pffft.yaml,
bungee.yaml, org.mixxx.Mixxx.yaml changes, and Smoke test) accurately reflects
its current status.
In `@docs/tasks/done/BNG-28.md`:
- Around line 25-50: Update the task markdown to reconcile the front-matter
status by either checking the three scope checkboxes (change the three "- [ ]"
lines for ".github/workflows/build.yml — verify, no changes expected",
".github/workflows/bungee-asan.yml — path triggers", and
".github/workflows/bungee-asan.yml — build flags" to "- [x]") or add a brief
completion note under the "## Scope" section stating the tasks were completed
(include date and summary) so the YAML front-matter `status: done` / `completed:
2026-05-06` matches the checklist and preserves auditability.
- Around line 46-50: Replace the speculative "Likely `-DSANITIZERS=address`"
note under the “`.github/workflows/bungee-asan.yml — build flags`” bullet in
BNG-28 with the verified flag `-DSANITIZE_ADDRESS=ON` (as documented in
pr-description.md) and remove the "do not guess"/"Likely" phrasing so the item
states the confirmed flag unambiguously; ensure the bullet references
`-DSANITIZE_ADDRESS=ON` exactly and adjust the surrounding sentence so it no
longer suggests verification is required.
In `@docs/tasks/index.yaml`:
- Line 224: The YAML key features is currently a quoted string ("[]") instead of
an actual sequence; change the value to an empty YAML list (features: []) so
automation reads it as an empty sequence. Locate the features entry in the
index.yaml file (the key named "features") and remove the quotes around the
brackets to produce features: [].
In `@packaging/flatpak/modules/bungee.yaml`:
- Line 27: Replace the direct shell redirection with a safe write-then-install
flow: run your sed filter targetting the source filename
(unofficial-bungee-config.cmake) but write to a temporary file first, then call
install -Dm644 to copy the temp into
${FLATPAK_DEST}/lib/cmake/unofficial-bungee/unofficial-bungee-config.cmake so
failures in sed don’t leave a zero-byte file; update the packaging comment near
the dest-filename/unofficial-bungee-config.cmake note to explain that Flatpak
commands run with the build dir as cwd so the relative path is intentional.
In `@packaging/flatpak/modules/pffft.yaml`:
- Around line 15-17: The cleanup block in pffft.yaml currently removes /include
and /lib/cmake immediately after build which breaks normal CMake discovery for
dependents like bungee; update pffft.yaml to either (A) defer or remove those
cleanup entries so headers and pffft CMake files remain available to downstream
modules, (B) move installed headers/CMake files to a non-cleanup location that
persists for dependents, or (C) add a clear comment in pffft.yaml explaining
that downstream modules (e.g., bungee) intentionally patch pffft locations
(pffft-include-path.patch and cmake-use-vcpkg-deps-and-install-layout.patch) and
strip find_dependency(pffft CONFIG) and why this brittle workaround is used;
reference the cleanup: - /include and - /lib/cmake entries and the bungee
patches when making the chosen change.
In `@src/engine/bufferscalers/enginebufferscalebungee.cpp`:
- Around line 365-380: The initial assignments to m_remainingOutputFrames and
m_outputChunkConsumed right after the early-return are dead writes because they
are immediately overwritten below; remove the unnecessary lines that set
m_remainingOutputFrames = m_outputChunk.frameCount and m_outputChunkConsumed = 0
and rely on the later logic that computes framesToCopy, calls
copyOutputFrames(...) and then sets m_outputChunkConsumed and
m_remainingOutputFrames based on framesToCopy so state updates for
m_outputChunkConsumed and m_remainingOutputFrames are only done once and remain
observable.
- Around line 426-432: The manual de-interleave loop duplicates logic from
copyOutputFrames; replace the nested for-loops in the flush path with a call to
copyOutputFrames so you reuse its multi-channel branch and the
SampleUtil::interleaveBuffer stereo fast path; locate the block using
m_outputChunk and pOutput and remove the double-loop, invoking copyOutputFrames
with the same arguments used by the other caller (framesToCopy, channelCount and
pOutput) so the optimized path is used consistently.
- Around line 227-249: ensureInputForCurrentChunk fails for reverse playback
because it assumes chunk positions increase; update the function to branch on
m_bBackwards: when m_bBackwards==false keep the existing logic
(discardBufferedInputBefore when m_bufferedInputBeginFrame <
m_currentInputChunk.begin and loop while m_bufferedInputEndFrame <
m_currentInputChunk.end), but when m_bBackwards==true discard stale forward data
on the other side (implement or call a discardBufferedInputAfter-style behavior
when m_bufferedInputEndFrame > m_currentInputChunk.end), change the filling loop
to use the appropriate missing-frame check (while m_bufferedInputBeginFrame >
m_currentInputChunk.begin call appendInputFrames with missingFrames based on
begin, and for forward use the existing end-based loop), and compute
availableBegin/availableEnd using conditional min/max depending on direction so
the returned available frame count correctly reflects the intersection with
m_currentInputChunk. Ensure you reference and update
EngineBufferScaleBungee::ensureInputForCurrentChunk, m_bBackwards,
m_currentInputChunk, m_bufferedInputBeginFrame, m_bufferedInputEndFrame,
discardBufferedInputBefore/After (or add the after variant), and
appendInputFrames.
- Around line 408-444: The flush path violates Bungee's grain-call contract by
calling m_pStretcher->specifyGrain(...) followed directly by
m_pStretcher->synthesiseGrain(m_outputChunk) without the required analyseGrain
step; fix it by inserting a muted analyseGrain call
(m_pStretcher->analyseGrain(nullptr, m_channelStride, 0, 0)) between
specifyGrain and synthesiseGrain, or replace the explicit flush block with a
loop that calls processGrain() (or uses m_pStretcher->next() with NaN-position
requests) until m_pStretcher->isFlushed() is true so the normal
specify→analyse→synthesise sequence is preserved for m_outputChunk.
In `@src/engine/bufferscalers/enginebufferscalebungee.h`:
- Line 4: The header enginebufferscalebungee.h currently includes
<gtest/gtest_prod.h>, which forces consumers to have gtest available; replace
this by removing the direct include and adding a guarded fallback for the
FRIEND_TEST macro (e.g., ifdef/ifndef FRIEND_TEST or a build-config macro like
HAVE_GTEST) so that when gtest is unavailable the file defines a no-op
FRIEND_TEST macro locally; update enginebufferscalebungee.h to check for
existing FRIEND_TEST (or a HAVE_GTEST flag) before defining the fallback to
preserve test functionality when gtest is present.
In `@src/engine/enginebuffer.cpp`:
- Around line 332-333: The comment above df.close() is misleading—it says "close
the writer" but the code calls df.close() (the QFile), not writer (QTextStream);
update the comment to accurately state that the file is being closed (and that
closing the file will flush the QTextStream), or alternatively explicitly
flush/close writer (writer, a QTextStream) before calling df.close() if you
intended to close the stream; locate the df.close() call and the associated
writer (QTextStream) usage to make the consistent change.
In `@src/engine/enginebuffer.h`:
- Around line 361-363: Remove the duplicated FRIEND_TEST declarations for
EngineBufferBungeeTest (the three lines declaring BungeeEngineSelected,
BungeeKeylockToggleDoesNotCrash, and BungeeKeylockEngineSwitch) from the first
location and keep the single block that sits next to the other scaler-related
FRIEND_TEST entries; ensure only one set of FRIEND_TEST(EngineBufferBungeeTest,
...) remains (the block that is adjacent to the other EngineBufferTest friends)
so renames only require a single header edit.
In `@src/library/rekordbox/rekordboxfeature.cpp`:
- Line 97: The schema adds key_id but it is never selected back into the track
model; either fully implement retrieval or remove storage. If you want key
support, add "key_id" to the BaseTrackCache columns list and the SELECT mapping
used by the track model (update the columns array and the sort/mapping logic in
class BaseTrackCache) so that key_id is read where records are fetched; also
extend any sort mapping that needs musical-order key sorting. If you do not want
key support, remove "key_id" from the CREATE TABLE schema and from all INSERT
bindings in rekordboxfeature.cpp (the insert locations that bind key_id) to
avoid writing unused data. Ensure changes touch the symbols: key_id,
BaseTrackCache, and the INSERT code paths that currently bind key_id.
In `@src/test/enginebufferbungeetest.cpp`:
- Around line 80-81: The test dereferences pEB returned by
m_pChannel1->getEngineBuffer() without checking for null, which can SIGSEGV if
initialization changes; add a guard like ASSERT_NE(pEB, nullptr) (or
ASSERT_TRUE(pEB)) immediately after calling getEngineBuffer() in both the
current test and in BungeeKeylockEngineSwitch to ensure the test fails cleanly
and stops before any dereference of pEB or access to pEB->m_pScaleBungee /
pEB->m_pScaleKeylock.
In `@src/test/enginebufferscalebungeetest.cpp`:
- Around line 84-93: Replace the raw pointer members m_pReadAheadMock and
m_pScaler with std::unique_ptr members and update SetUp/TearDown to be
exception-safe: in SetUp() construct them with std::make_unique (or
unique_ptr::reset) for ReadAheadManagerMock and EngineBufferScaleBungee (passing
m_pReadAheadMock.get() into the EngineBufferScaleBungee constructor) and remove
the manual delete calls from TearDown(); also apply the same change to the other
fixture at the referenced location (lines ~490-499) so both fixtures use
unique_ptr for lifetime management.
- Around line 241-247: The stack allocation of large CSAMPLE arrays (e.g., the
readBuffer defined with constexpr SINT kBufferSize = 16384 and used with
m_pReadAheadMock->setReadBuffer) must be replaced with heap allocation to avoid
stack overflows; change these fixed-size arrays to std::vector<CSAMPLE>
vec(kBufferSize); fill vec via vec[i] or std::fill, then call
m_pReadAheadMock->setReadBuffer(vec.data(), vec.size()); apply the same
replacement for the other large arrays (8192/4096 elements referenced in this
file and the arrays around lines 397-402) to ensure consistency with
ReusesBufferedInputAcrossOverlappingGrains.
- Around line 119-140: The test BasicPlayback currently only asserts framesRead
> 0; strengthen it by validating actual output content: after calling
m_pScaler->scaleBuffer(pOutput, kOutputBufferSize) sample a few output frames
from pOutput (e.g., first, middle, last) and add assertions that each is finite
(not NaN/Inf) and within a reasonable range around the known input constant
(0.5f) using a small tolerance (e.g., fabs(sample - 0.5f) < 0.2) or
EXPECT_TRUE(std::isfinite(...)) plus EXPECT_NEAR; use the same pattern for other
tests listed. Keep references to the existing symbols (BasicPlayback,
m_pScaler->scaleBuffer, pOutput, SampleUtil::alloc, ClearBuffer,
m_pReadAheadMock/readBuffer) so changes are localized and do not alter buffer
setup or allocation semantics.
- Around line 68-72: Replace the legacy MOCK_METHOD4 usage with the modern
MOCK_METHOD syntax and add the override qualifier: change the
MOCK_METHOD4(getNextSamples, SINT(double dRate, CSAMPLE* buffer, SINT
requested_samples, mixxx::audio::ChannelCount channelCount)); declaration to use
MOCK_METHOD(SINT, getNextSamples, (double, CSAMPLE*, SINT,
mixxx::audio::ChannelCount), (override)); so the mock matches the virtual
signature in ReadAheadManager and properly marks the override.
---
Outside diff comments:
In `@src/preferences/dialog/dlgprefsound.cpp`:
- Around line 806-817: The UI currently disables keylockDualthreadedCheckBox
based on currentEngine and monoMix, but DlgPrefSound::slotApply() persists the
preference using keylockDualthreadedCheckBox->isChecked() &&
keylockDualthreadedCheckBox->isEnabled(), which causes the saved RubberBand
multithreading preference to be overwritten when the checkbox is disabled (e.g.,
Bungee or mono). Change the logic so slotApply() reads and writes the stored
preference independently of the widget enabled state (use only
keylockDualthreadedCheckBox->isChecked() to update the stored setting, or
better: keep the stored setting value separate and only use
keylockDualthreadedCheckBox->isEnabled() to decide whether to apply it at
runtime), and ensure the availability logic in the method that sets the
tooltip/enablement (the code using currentEngine/EngineBuffer::KeylockEngine,
monoMix, keylockDualthreadedCheckBox->setEnabled/setToolTip) does not clear or
mutate the stored preference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38d59c2a-e640-4f0e-9a89-d060e29f4bb8
⛔ Files ignored due to path filters (1)
docs/tasks/.pi/.ticket.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
.github/workflows/build.yml.github/workflows/bungee-asan.ymlCMakeLists.txtcmake/modules/FindBungee.cmakecmake/patches/bungee/apply-patches.cmakecmake/patches/bungee/lower-cmake-minimum.patchcmake/patches/pffft/CMakeLists.txt.incmake/vcpkg-overlay-ports/bungee/README.mdcmake/vcpkg-overlay-ports/bungee/assert-win32-compat.patchcmake/vcpkg-overlay-ports/bungee/cmake-use-vcpkg-deps-and-install-layout.patchcmake/vcpkg-overlay-ports/bungee/pffft-include-path.patchcmake/vcpkg-overlay-ports/bungee/portfile.cmakecmake/vcpkg-overlay-ports/bungee/resample-msvc-noinline.patchcmake/vcpkg-overlay-ports/bungee/unofficial-bungee-config.cmakecmake/vcpkg-overlay-ports/bungee/usagecmake/vcpkg-overlay-ports/bungee/vcpkg.jsondocs/bungee-integration.mddocs/plans/bungee-dependency-integration/00-current-state.mddocs/plans/bungee-dependency-integration/01-upstream-bungee.mddocs/plans/bungee-dependency-integration/02-vcpkg-buildenv.mddocs/plans/bungee-dependency-integration/03-mixxx-cmake.mddocs/plans/bungee-dependency-integration/04-packaging-ci.mddocs/plans/bungee-dependency-integration/05-validation-and-pr-strategy.mddocs/plans/bungee-dependency-integration/06-branch-ci-discipline.mddocs/plans/bungee-dependency-integration/07-execution-tickets.mddocs/plans/bungee-dependency-integration/HANDOFF.mddocs/plans/bungee-dependency-integration/README.mddocs/plans/bungee-dependency-integration/bng-18-version-matrix.mddocs/plans/bungee-dependency-integration/maintainer-questions.mddocs/plans/bungee-dependency-integration/pr-description.mddocs/tasks/done/BNG-17.mddocs/tasks/done/BNG-18.mddocs/tasks/done/BNG-19.mddocs/tasks/done/BNG-20.mddocs/tasks/done/BNG-21.mddocs/tasks/done/BNG-22.mddocs/tasks/done/BNG-23.mddocs/tasks/done/BNG-24.mddocs/tasks/done/BNG-25.mddocs/tasks/done/BNG-26.mddocs/tasks/done/BNG-27.mddocs/tasks/done/BNG-28.mddocs/tasks/index.yamldocs/tasks/open/BNG-29.mddocs/tasks/transitions.jsonlpackaging/flatpak/modules/bungee.yamlpackaging/flatpak/modules/eigen3.yamlpackaging/flatpak/modules/pffft.yamlpackaging/flatpak/org.mixxx.Mixxx.yamlsrc/engine/bufferscalers/enginebufferscalebungee.cppsrc/engine/bufferscalers/enginebufferscalebungee.hsrc/engine/enginebuffer.cppsrc/engine/enginebuffer.hsrc/library/rekordbox/rekordboxfeature.cppsrc/preferences/dialog/dlgprefsound.cppsrc/preferences/settingsmanager.cppsrc/test/enginebufferbungeetest.cppsrc/test/enginebufferscalebungeetest.cppsrc/test/settingsmanager_test.cpp
| -DQT6=ON | ||
| -DQML=ON | ||
| -DBULK=ON | ||
| -DBUNGEE=ON |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding one non-Linux BUNGEE-enabled CI lane.
Good to see Linux coverage enabled. Given recent platform-specific bugs in this PR thread, adding at least one macOS or Windows BUNGEE-on matrix job would reduce regression risk before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build.yml at line 58, The CI currently only enables
DBUNGEE=ON for Linux; add at least one non-Linux matrix job that also sets
DBUNGEE=ON (e.g., add macos-latest or windows-latest to the workflow matrix) so
a BUNGEE-enabled macOS/Windows lane runs; update the matrix definition that
contains the DBUNGEE=ON entry to include the additional os value and ensure the
env/strategy keys that reference DBUNGEE=ON apply to that new job.
Comment on lines
+28
to
+48
| paths: | ||
| - "cmake/patches/bungee/**" | ||
| - "cmake/patches/pffft/**" | ||
| - "cmake/vcpkg-overlay-ports/bungee/**" | ||
| - "cmake/modules/FindBungee.cmake" | ||
| - "src/engine/bufferscalers/enginebufferscalebungee.*" | ||
| - "src/test/enginebufferscalebungeetest.cpp" | ||
| - "src/test/enginebufferbungeetest.cpp" | ||
| - "CMakeLists.txt" | ||
| - ".github/workflows/bungee-asan.yml" | ||
| pull_request: | ||
| paths: | ||
| - "cmake/patches/bungee/**" | ||
| - "cmake/patches/pffft/**" | ||
| - "cmake/vcpkg-overlay-ports/bungee/**" | ||
| - "cmake/modules/FindBungee.cmake" | ||
| - "src/engine/bufferscalers/enginebufferscalebungee.*" | ||
| - "src/test/enginebufferscalebungeetest.cpp" | ||
| - "src/test/enginebufferbungeetest.cpp" | ||
| - "CMakeLists.txt" | ||
| - ".github/workflows/bungee-asan.yml" |
There was a problem hiding this comment.
Path filters are too narrow for the stated regression scope.
Lines 28-48 only include a subset of Bungee-related files, so changes in other integration points can bypass this ASan workflow entirely. That weakens the “catch regressions early” guarantee.
Suggested update
paths:
+ - "src/engine/enginebuffer.*"
+ - "src/engine/**/enginebuffer*bungee*.*"
+ - "src/preferences/settingsmanager.cpp"
+ - "src/test/settingsmanager_test.cpp"
- "cmake/patches/bungee/**"
- "cmake/patches/pffft/**"
- "cmake/vcpkg-overlay-ports/bungee/**"
- "cmake/modules/FindBungee.cmake"
- "src/engine/bufferscalers/enginebufferscalebungee.*"
- "src/test/enginebufferscalebungeetest.cpp"
- "src/test/enginebufferbungeetest.cpp"
- "CMakeLists.txt"
- ".github/workflows/bungee-asan.yml"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| paths: | |
| - "cmake/patches/bungee/**" | |
| - "cmake/patches/pffft/**" | |
| - "cmake/vcpkg-overlay-ports/bungee/**" | |
| - "cmake/modules/FindBungee.cmake" | |
| - "src/engine/bufferscalers/enginebufferscalebungee.*" | |
| - "src/test/enginebufferscalebungeetest.cpp" | |
| - "src/test/enginebufferbungeetest.cpp" | |
| - "CMakeLists.txt" | |
| - ".github/workflows/bungee-asan.yml" | |
| pull_request: | |
| paths: | |
| - "cmake/patches/bungee/**" | |
| - "cmake/patches/pffft/**" | |
| - "cmake/vcpkg-overlay-ports/bungee/**" | |
| - "cmake/modules/FindBungee.cmake" | |
| - "src/engine/bufferscalers/enginebufferscalebungee.*" | |
| - "src/test/enginebufferscalebungeetest.cpp" | |
| - "src/test/enginebufferbungeetest.cpp" | |
| - "CMakeLists.txt" | |
| - ".github/workflows/bungee-asan.yml" | |
| paths: | |
| - "src/engine/enginebuffer.*" | |
| - "src/engine/**/enginebuffer*bungee*.*" | |
| - "src/preferences/settingsmanager.cpp" | |
| - "src/test/settingsmanager_test.cpp" | |
| - "cmake/patches/bungee/**" | |
| - "cmake/patches/pffft/**" | |
| - "cmake/vcpkg-overlay-ports/bungee/**" | |
| - "cmake/modules/FindBungee.cmake" | |
| - "src/engine/bufferscalers/enginebufferscalebungee.*" | |
| - "src/test/enginebufferscalebungeetest.cpp" | |
| - "src/test/enginebufferbungeetest.cpp" | |
| - "CMakeLists.txt" | |
| - ".github/workflows/bungee-asan.yml" | |
| pull_request: | |
| paths: | |
| - "src/engine/enginebuffer.*" | |
| - "src/engine/**/enginebuffer*bungee*.*" | |
| - "src/preferences/settingsmanager.cpp" | |
| - "src/test/settingsmanager_test.cpp" | |
| - "cmake/patches/bungee/**" | |
| - "cmake/patches/pffft/**" | |
| - "cmake/vcpkg-overlay-ports/bungee/**" | |
| - "cmake/modules/FindBungee.cmake" | |
| - "src/engine/bufferscalers/enginebufferscalebungee.*" | |
| - "src/test/enginebufferscalebungeetest.cpp" | |
| - "src/test/enginebufferbungeetest.cpp" | |
| - "CMakeLists.txt" | |
| - ".github/workflows/bungee-asan.yml" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/bungee-asan.yml around lines 28 - 48, The workflow's path
filters in .github/workflows/bungee-asan.yml are too narrow and may miss
integration changes; update the push and pull_request paths entries (the paths
block in that file) to broaden coverage—either add globs that capture all
Bungee-related code (for example include "src/**/bungee/**",
"cmake/**/bungee/**", and any engine integration directories) or remove the
restrictive path filter so the ASan job runs on relevant changes; ensure both
the push and pull_request sections are updated consistently.
Comment on lines
+53
to
+56
| find_package(PkgConfig QUIET) | ||
| if(PkgConfig_FOUND) | ||
| pkg_check_modules(PC_Bungee QUIET libbungee) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For CMake's pkg_check_modules, which variables or imported targets carry transitive link dependencies from a .pcfile? If I wrap only the located library path in my own imported target, do I need to propagateINTERFACE_LINK_LIBRARIES/pkg-config link flags for static libraries?
💡 Result:
CMake’s pkg_check_modules transitive link dependencies from a.pc file are carried by the pkg-config “requires” graph as CMake target usage requirements, when you use the IMPORTED_TARGET (or later, cmake_pkg_config POPULATE/IMPORT) mechanism. 1) Variables that carry transitive link dependencies - In “variable mode” (no IMPORTED_TARGET), pkg_check_modules sets *_LIBRARIES / *_LINK_LIBRARIES and other flags like LDFLAGS, LDFLAGS_OTHER, LDFLAGS (and corresponding STATIC variants if pkg-config was invoked with --static). The FindPkgConfig docs explicitly state that these variables come in common and “static” sets ([1]). - However, those variables are just raw link/interface inputs; CMake won’t automatically interpret them as transitive dependencies of a target unless you manually propagate them via INTERFACE_LINK_LIBRARIES / target_link_libraries semantics. 2) Imported targets / target properties that carry transitive link dependencies - With pkg_check_modules(... IMPORTED_TARGET), CMake creates an imported target named PkgConfig:: that “encapsulate usage requirements for all specified modules at once” and can be passed to target_link_libraries ([2]). - Since CMake 3.15, non-library linker options from pkg-config are stored in the imported target’s INTERFACE_LINK_OPTIONS property ([2]). - The docs also indicate that the interface encapsulates usage requirements, including include dirs and other link flags, which is why linking to the imported target gives downstreams the correct transitive link behavior ([2]). - For the newer cmake_pkg_config(IMPORT/POPULATE) workflow, the docs state that imported pkg-config targets are exposed via the INTERFACE_LINK_LIBRARIES property of an IMPORT-generated target, and that pkg-config dependencies are resolved via the Requires graph ([3]). So, concretely, the “transitive link dependencies” end up in: - INTERFACE_LINK_LIBRARIES on the generated pkg-config imported target(s) (directly stated for cmake_pkg_config IMPORT-generated targets) [3] - INTERFACE_LINK_OPTIONS on the generated imported target (stated for pkg_check_modules since 3.15) [2] - plus other INTERFACE usage requirements (include dirs / compile options / etc.) that are also encapsulated in the imported target’s properties as part of its “usage requirements” ([2]). 3) Answer to your question about wrapping only the located library path If you wrap only the located library path in your own imported target and do not also propagate the pkg-config “usage requirements” (i.e., the link flags/options and the transitive requires graph), then you can miss dependencies needed for correct linkage—especially for static linking. Why: - The whole point of IMPORTED_TARGET / INTERFACE on the pkg-config target is to encapsulate the pkg-config usage requirements (including transitive Requires) so downstreams automatically get the right link line behavior via target_link_libraries ([2]; [3]). - In variable mode, you’re expected to propagate the right information yourself; CMake won’t infer transitive deps from a lone library path. What you should do for static libraries - If your goal is to support static linking, you must ensure you’re using the pkg-config flags that include transitive static dependencies. FindPkgConfig documents that there are separate variable sets for the “--static option” case, with STATIC suffixes ([1]). - Practically: link to the PkgConfig:: imported target (or create an equivalent imported target by consuming the imported target’s INTERFACE properties) rather than re-creating an imported target that only points at the main.a/.so path. 4) Practical recommendation - Prefer: target_link_libraries(yourTarget PRIVATE PkgConfig::) (or PUBLIC as appropriate). This uses the imported target that encapsulates transitive usage requirements ([2]). - If you must wrap: set your wrapper imported target’s INTERFACE_LINK_LIBRARIES and INTERFACE_LINK_OPTIONS (and related INTERFACE properties) from the PkgConfig:: target so that transitive link dependencies and required link options propagate the same way ([3]; [2]).
Citations:
- 1: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
- 2: http://www.cmake.org/cmake/help/latest/module/FindPkgConfig.html
- 3: http://www.cmake.org/cmake/help/latest/command/cmake_pkg_config.html
🏁 Script executed:
cat -n cmake/modules/FindBungee.cmake | sed -n '45,110p'Repository: 0cwa/mixxx
Length of output: 1944
Propagate pkg-config link requirements into Bungee::Bungee.
The current implementation uses variable-mode pkg_check_modules (without IMPORTED_TARGET) and creates a custom imported target with only IMPORTED_LOCATION and INTERFACE_INCLUDE_DIRECTORIES. This drops all transitive link dependencies that pkg-config provides, which breaks static Bungee builds when transitive dependencies like pffft and Eigen exist.
Use the IMPORTED_TARGET flag to create PkgConfig::PC_Bungee, then either link to it directly or propagate its INTERFACE_LINK_LIBRARIES and INTERFACE_LINK_OPTIONS to Bungee::Bungee.
Suggested approach
-find_package(PkgConfig QUIET)
-if(PkgConfig_FOUND)
- pkg_check_modules(PC_Bungee QUIET libbungee)
+find_package(PkgConfig QUIET)
+if(PkgConfig_FOUND)
+ pkg_check_modules(PC_Bungee IMPORTED_TARGET QUIET libbungee)
endif()
...
if(NOT TARGET Bungee::Bungee)
- add_library(Bungee::Bungee UNKNOWN IMPORTED)
- set_target_properties(
- Bungee::Bungee
- PROPERTIES
- IMPORTED_LOCATION "${Bungee_LIBRARY}"
- INTERFACE_INCLUDE_DIRECTORIES "${Bungee_INCLUDE_DIR}"
- )
+ if(TARGET PkgConfig::PC_Bungee)
+ add_library(Bungee::Bungee INTERFACE IMPORTED)
+ set_target_properties(Bungee::Bungee PROPERTIES
+ INTERFACE_LINK_LIBRARIES PkgConfig::PC_Bungee)
+ else()
+ add_library(Bungee::Bungee UNKNOWN IMPORTED)
+ set_target_properties(
+ Bungee::Bungee
+ PROPERTIES
+ IMPORTED_LOCATION "${Bungee_LIBRARY}"
+ INTERFACE_INCLUDE_DIRECTORIES "${Bungee_INCLUDE_DIR}"
+ )
+ endif()
endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/modules/FindBungee.cmake` around lines 53 - 56, The pkg-config usage
currently calls pkg_check_modules(PC_Bungee) in variable mode and builds
Bungee::Bungee manually with only IMPORTED_LOCATION and
INTERFACE_INCLUDE_DIRECTORIES, dropping transitive link deps; change
pkg_check_modules to create an imported target (use pkg_check_modules(...
IMPORTED_TARGET) so PkgConfig::PC_Bungee exists) and then either
target_link_libraries(Bungee::Bungee PUBLIC PkgConfig::PC_Bungee) or copy
PkgConfig::PC_Bungee's INTERFACE_LINK_LIBRARIES and INTERFACE_LINK_OPTIONS onto
Bungee::Bungee (in addition to include dirs) instead of only setting
IMPORTED_LOCATION, ensuring transitive link requirements like pffft/Eigen
propagate.
Comment on lines
+20
to
+21
| -cmake_minimum_required(VERSION 3.30...3.31) | ||
| +cmake_minimum_required(VERSION 3.21) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the difference in CMake policy handling between cmake_minimum_required(VERSION 3.21)andcmake_minimum_required(VERSION 3.21...3.31) when running on newer CMake versions?
💡 Result:
Difference in CMake policy handling: 1) cmake_minimum_required(VERSION 3.21) (single version) - This sets the “policy version” to 3.21. - All policies known to the running CMake and introduced in 3.21 or earlier are set to NEW behavior. - Policies introduced in later CMake versions are left unset; on those, newer CMake may warn and/or use its behavior for unset policies. [1][2] 2) cmake_minimum_required(VERSION 3.21...3.31) (range) - This sets the “policy version” to the range (i.e., minimum 3.21 and maximum/policy_max 3.31). - NEW behavior is requested for all policies introduced up through 3.31 (policies introduced in 3.31 or earlier become NEW). - Policies introduced after 3.31 remain unset. [1][2] What you observe on newer CMake versions - On CMake versions newer than 3.21 but up to and including 3.31, using 3.21...3.31 opts in to the NEW behavior for the policies introduced in that 3.21-to-3.31 window, so compatibility warnings for those policies should be avoided compared to using only 3.21. [1] - On CMake versions newer than 3.31, both forms still leave policies introduced after 3.31 unset, so warnings/OLD behavior for those later-introduced policies can still occur. [1] Related deprecation note - CMake 3.31+ deprecates calls to cmake_minimum_required(VERSION) or cmake_policy(VERSION) that do not specify at least 3.10 as their policy version (optionally via...). This is separate from the behavioral difference between 3.21 and 3.21...3.31; it’s about ensuring you provide an adequate policy version number. [2]
Citations:
- 1: https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html
- 2: https://cmake.org/cmake/help/latest/command/cmake_policy.html
🏁 Script executed:
# Check if the file exists and show its current content
if [ -f "cmake/patches/bungee/lower-cmake-minimum.patch" ]; then
cat -n cmake/patches/bungee/lower-cmake-minimum.patch
else
echo "File not found at that path"
# Search for similar patch files
find . -name "*lower-cmake*" -o -name "*cmake-minimum*" 2>/dev/null | head -20
fiRepository: 0cwa/mixxx
Length of output: 1219
🏁 Script executed:
# Also check if there's a CMakeLists.txt with the actual command to see current state
find . -name "CMakeLists.txt" -type f | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && grep -n "cmake_minimum_required" {} 2>/dev/null || true'Repository: 0cwa/mixxx
Length of output: 521
Preserve the upper policy range while lowering the minimum.
The patch correctly lowers the CMake minimum to 3.21 to allow Mixxx's source fallback to build on older hosts, and the patch header rightly notes that Bungee's actual commands work with 3.21+. However, using bare 3.21 instead of 3.21...3.31 changes policy behavior: on CMake versions 3.22–3.31, the bare version leaves policies introduced in that range unset, diverging from Bungee upstream's tested configuration. Since the goal is to relax the build floor while keeping the tested policy environment intact, use 3.21...3.31 instead. The warning Mixxx already emits for CMake below 3.30 remains effective.
Suggested fix
-cmake_minimum_required(VERSION 3.21)
+cmake_minimum_required(VERSION 3.21...3.31)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| -cmake_minimum_required(VERSION 3.30...3.31) | |
| +cmake_minimum_required(VERSION 3.21) | |
| cmake_minimum_required(VERSION 3.21...3.31) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmake/patches/bungee/lower-cmake-minimum.patch` around lines 20 - 21, The
cmake_minimum_required call was changed to a bare version which leaves CMake
policies between 3.22 and 3.31 unset; revert to an explicit range to preserve
Bungee's tested policy set by changing the cmake_minimum_required(...)
invocation back to use "3.21...3.31" (i.e., update the cmake_minimum_required
symbol to include the upper bound) while keeping the existing patch
header/comment intact.
Comment on lines
+110
to
+117
| +get_property(BUNGEE_LIBRARY_OUTPUT_NAME TARGET bungee_library PROPERTY OUTPUT_NAME) | ||
| + | ||
| + set(PKGCONFIG_LINK_PATH -L) | ||
| + set(PKGCONFIG_LINK_FLAG "-l") | ||
| + | ||
| +configure_file(${CMAKE_CURRENT_SOURCE_DIR}/libbungee.pc.in ${CMAKE_CURRENT_BINARY_DIR}/libbungee.pc @ONLY) | ||
| + | ||
| +install(FILES ${CMAKE_CURRENT_BINARY_DIR}/libbungee.pc DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Leftover indentation after collapsing the if(APPLE)/else() block.
The two set() calls for PKGCONFIG_LINK_PATH / PKGCONFIG_LINK_FLAG are still indented two spaces, even though the surrounding if(APPLE)/else()/endif() block has been removed and they are now at file-scope. This makes the patched Bungee CMakeLists.txt look like the lines belong inside a conditional that no longer exists, which is confusing for anyone diffing against upstream Bungee later.
Also note that removing the Apple branch unconditionally drops -F / -framework handling for the generated libbungee.pc — that is fine for the Mixxx static-link use case, but if anyone consumes the overlay port to build Bungee as an Apple framework via BUNGEE_BUILD_SHARED_LIBRARY=ON, the resulting .pc file will be wrong. Worth a one-line comment in the patch explaining that the .pc is only intended for the static/non-framework path used by Mixxx.
♻️ Proposed cleanup of indentation
- set(PKGCONFIG_LINK_PATH -L)
- set(PKGCONFIG_LINK_FLAG "-l")
+# Mixxx vcpkg overlay always builds Bungee as a static library, so the
+# pkg-config flags are intentionally non-Apple-framework specific.
+set(PKGCONFIG_LINK_PATH -L)
+set(PKGCONFIG_LINK_FLAG "-l")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@cmake/vcpkg-overlay-ports/bungee/cmake-use-vcpkg-deps-and-install-layout.patch`
around lines 110 - 117, Fix the leftover indentation and document the
Apple-framework caveat: unindent the two set() calls for PKGCONFIG_LINK_PATH and
PKGCONFIG_LINK_FLAG so they are at file-scope (they currently appear indented as
if inside an if(APPLE) block), and add a single-line comment near the
configure_file(${CMAKE_CURRENT_SOURCE_DIR}/libbungee.pc.in ...) or before the
install(...) that states the generated libbungee.pc is intended for the
static/non-framework path (BUNGEE_BUILD_SHARED_LIBRARY=OFF) and therefore does
not include -F/-framework handling for Apple frameworks.
Comment on lines
+80
to
+81
| EngineBuffer* pEB = m_pChannel1->getEngineBuffer(); | ||
| EXPECT_EQ(pEB->m_pScaleBungee, pEB->m_pScaleKeylock); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Guard against null pEB before dereferencing.
m_pChannel1->getEngineBuffer() is assumed non-null; if a future fixture refactor changes deck initialisation order, dereferencing pEB will SIGSEGV instead of producing a clean GTest failure. The same applies in BungeeKeylockEngineSwitch (Line 116). An ASSERT_NE(pEB, nullptr) (or ASSERT_TRUE(pEB)) right after the getEngineBuffer() call gives a readable failure and stops the test before it crashes the harness.
♻️ Proposed guard
EngineBuffer* pEB = m_pChannel1->getEngineBuffer();
+ ASSERT_NE(pEB, nullptr);
EXPECT_EQ(pEB->m_pScaleBungee, pEB->m_pScaleKeylock);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| EngineBuffer* pEB = m_pChannel1->getEngineBuffer(); | |
| EXPECT_EQ(pEB->m_pScaleBungee, pEB->m_pScaleKeylock); | |
| EngineBuffer* pEB = m_pChannel1->getEngineBuffer(); | |
| ASSERT_NE(pEB, nullptr); | |
| EXPECT_EQ(pEB->m_pScaleBungee, pEB->m_pScaleKeylock); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/enginebufferbungeetest.cpp` around lines 80 - 81, The test
dereferences pEB returned by m_pChannel1->getEngineBuffer() without checking for
null, which can SIGSEGV if initialization changes; add a guard like
ASSERT_NE(pEB, nullptr) (or ASSERT_TRUE(pEB)) immediately after calling
getEngineBuffer() in both the current test and in BungeeKeylockEngineSwitch to
ensure the test fails cleanly and stops before any dereference of pEB or access
to pEB->m_pScaleBungee / pEB->m_pScaleKeylock.
Comment on lines
+68
to
+72
| MOCK_METHOD4(getNextSamples, | ||
| SINT(double dRate, | ||
| CSAMPLE* buffer, | ||
| SINT requested_samples, | ||
| mixxx::audio::ChannelCount channelCount)); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm gtest/gmock major version pinned by the project (>=1.10 required for MOCK_METHOD).
rg -nP -C2 '(gtest|gmock).*[0-9]+\.[0-9]+'
fd -t f -i 'cmake.*googletest|googletest.*cmake'Repository: 0cwa/mixxx
Length of output: 36
🏁 Script executed:
# Search for CMakeLists.txt and other dependency files
fd -t f 'CMakeLists\.txt|conanfile|requirements|package\.json' | head -20Repository: 0cwa/mixxx
Length of output: 400
🏁 Script executed:
# Look at the specific file mentioned in the review
head -100 src/test/enginebufferscalebungeetest.cppRepository: 0cwa/mixxx
Length of output: 2726
🏁 Script executed:
# Search for gmock/gtest in common config locations
rg -l 'gmock|gtest' --max-count=20Repository: 0cwa/mixxx
Length of output: 4281
🏁 Script executed:
# Check main CMakeLists.txt for gmock/gtest version
head -200 CMakeLists.txt | grep -A5 -B5 -i 'gtest\|gmock'Repository: 0cwa/mixxx
Length of output: 36
🏁 Script executed:
# Find ReadAheadManager class definition
fd -t f 'readaheadmanager\.h' --exec head -100 {} \;Repository: 0cwa/mixxx
Length of output: 4224
🏁 Script executed:
# Also search for any version specifications in other config files
rg -i '(gtest|gmock)' --type cmake --max-count=30Repository: 0cwa/mixxx
Length of output: 647
🏁 Script executed:
# Look for version specifications in common dependency files
fd -t f '(conanfile|vcpkg|requirements|package-lock|version)' --exec grep -l 'gtest\|gmock' {} \;Repository: 0cwa/mixxx
Length of output: 36
🏁 Script executed:
# Check if there's a conanfile or vcpkg config
fd -t f 'conanfile\.(txt|py)|vcpkg\.json'Repository: 0cwa/mixxx
Length of output: 98
🏁 Script executed:
# Look at git history or tags for gmock version info (if available)
rg -i 'gtest.*[0-9]+\.[0-9]+|gmock.*[0-9]+\.[0-9]+' --type cmake --type txtRepository: 0cwa/mixxx
Length of output: 36
🏁 Script executed:
# Check the vcpkg.json file found
cat cmake/vcpkg-overlay-ports/bungee/vcpkg.jsonRepository: 0cwa/mixxx
Length of output: 476
🏁 Script executed:
# Look for root-level dependency specifications
cat vcpkg.json 2>/dev/null || cat conanfile.txt 2>/dev/null || echo "No root vcpkg/conan files"Repository: 0cwa/mixxx
Length of output: 80
🏁 Script executed:
# Check .gitmodules or git submodule info for gtest
cat .gitmodules 2>/dev/null || rg -i 'gtest|gmock' --max-count=10Repository: 0cwa/mixxx
Length of output: 15369
🏁 Script executed:
# Look at CMAKE code that includes GTest to understand how it's configured
rg -A10 'find_package.*GTest' CMakeLists.txtRepository: 0cwa/mixxx
Length of output: 350
Modernize to MOCK_METHOD syntax with override qualifier.
MOCK_METHOD4 is legacy gmock syntax. The modern MOCK_METHOD(SINT, getNextSamples, (double, CSAMPLE*, SINT, mixxx::audio::ChannelCount), (override)); syntax is preferred and properly adds the override qualifier, since the parent method in ReadAheadManager is declared virtual.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/enginebufferscalebungeetest.cpp` around lines 68 - 72, Replace the
legacy MOCK_METHOD4 usage with the modern MOCK_METHOD syntax and add the
override qualifier: change the MOCK_METHOD4(getNextSamples, SINT(double dRate,
CSAMPLE* buffer, SINT requested_samples, mixxx::audio::ChannelCount
channelCount)); declaration to use MOCK_METHOD(SINT, getNextSamples, (double,
CSAMPLE*, SINT, mixxx::audio::ChannelCount), (override)); so the mock matches
the virtual signature in ReadAheadManager and properly marks the override.
Comment on lines
+84
to
+93
| m_pReadAheadMock = new StrictMock<ReadAheadManagerMock>(); | ||
| m_pScaler = new EngineBufferScaleBungee(m_pReadAheadMock); | ||
| m_pScaler->setSignal(mixxx::audio::SampleRate(44100), | ||
| mixxx::audio::ChannelCount::stereo()); | ||
| } | ||
|
|
||
| void TearDown() override { | ||
| delete m_pScaler; | ||
| delete m_pReadAheadMock; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use std::unique_ptr for the SetUp/TearDown-managed objects.
Both fixtures use new/delete for m_pScaler and the mock. If SetUp() ever throws between the two new calls, the first allocation leaks because TearDown() is not invoked. std::unique_ptr makes this exception-safe and removes the manual delete in TearDown.
Also applies to: 490-499
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/enginebufferscalebungeetest.cpp` around lines 84 - 93, Replace the
raw pointer members m_pReadAheadMock and m_pScaler with std::unique_ptr members
and update SetUp/TearDown to be exception-safe: in SetUp() construct them with
std::make_unique (or unique_ptr::reset) for ReadAheadManagerMock and
EngineBufferScaleBungee (passing m_pReadAheadMock.get() into the
EngineBufferScaleBungee constructor) and remove the manual delete calls from
TearDown(); also apply the same change to the other fixture at the referenced
location (lines ~490-499) so both fixtures use unique_ptr for lifetime
management.
Comment on lines
+119
to
+140
| TEST_F(EngineBufferScaleBungeeTest, BasicPlayback) { | ||
| SetRate(1.0); | ||
|
|
||
| constexpr SINT kBufferSize = 4096; | ||
| CSAMPLE readBuffer[kBufferSize]; | ||
| for (SINT i = 0; i < kBufferSize; ++i) { | ||
| readBuffer[i] = static_cast<CSAMPLE>(i % 2 == 0 ? 0.5f : -0.5f); | ||
| } | ||
| m_pReadAheadMock->setReadBuffer(readBuffer, kBufferSize); | ||
|
|
||
| EXPECT_CALL(*m_pReadAheadMock, getNextSamples(_, _, _, _)) | ||
| .WillRepeatedly(Invoke(m_pReadAheadMock, &ReadAheadManagerMock::getNextSamplesFake)); | ||
|
|
||
| constexpr SINT kOutputBufferSize = 2048; | ||
| CSAMPLE* pOutput = SampleUtil::alloc(kOutputBufferSize); | ||
| ClearBuffer(pOutput, kOutputBufferSize); | ||
|
|
||
| const double framesRead = m_pScaler->scaleBuffer(pOutput, kOutputBufferSize); | ||
| EXPECT_GT(framesRead, 0.0); | ||
|
|
||
| SampleUtil::free(pOutput); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Most tests assert only framesRead > 0 — strengthen at least one with content/correctness checks.
BasicPlayback, VariableSpeeds, KeylockMode, PitchShifting, ReversePlayback, BufferClearing, SignalFormatChanges, and RapidParameterChanges all only assert that framesRead > 0. With the input buffer filled with a constant value (e.g. 0.5f, 0.3f, 0.4f, 0.6f, 0.8f), Bungee should produce an output close to that constant (modulo grain windowing). At minimum, sample a few output frames and verify they're finite and within a reasonable range, otherwise these tests will silently pass even if the scaler emits NaNs/Infs or zeros after a future regression.
Also applies to: 142-170, 172-192, 194-214, 216-236, 267-280, 282-307, 309-332, 334-366, 396-417
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/enginebufferscalebungeetest.cpp` around lines 119 - 140, The test
BasicPlayback currently only asserts framesRead > 0; strengthen it by validating
actual output content: after calling m_pScaler->scaleBuffer(pOutput,
kOutputBufferSize) sample a few output frames from pOutput (e.g., first, middle,
last) and add assertions that each is finite (not NaN/Inf) and within a
reasonable range around the known input constant (0.5f) using a small tolerance
(e.g., fabs(sample - 0.5f) < 0.2) or EXPECT_TRUE(std::isfinite(...)) plus
EXPECT_NEAR; use the same pattern for other tests listed. Keep references to the
existing symbols (BasicPlayback, m_pScaler->scaleBuffer, pOutput,
SampleUtil::alloc, ClearBuffer, m_pReadAheadMock/readBuffer) so changes are
localized and do not alter buffer setup or allocation semantics.
Comment on lines
+241
to
+247
| constexpr SINT kBufferSize = 16384; | ||
| CSAMPLE readBuffer[kBufferSize]; | ||
| for (SINT i = 0; i < kBufferSize; ++i) { | ||
| readBuffer[i] = static_cast<CSAMPLE>(0.7f); | ||
| } | ||
| m_pReadAheadMock->setReadBuffer(readBuffer, kBufferSize); | ||
|
|
There was a problem hiding this comment.
Move large fixed-size CSAMPLE arrays off the stack.
CSAMPLE readBuffer[16384] (~64 KB) and similar 8192-element arrays consume a significant fraction of the default thread stack on Windows (~1 MB) and embedded CI runners. The ReusesBufferedInputAcrossOverlappingGrains test already uses std::vector<CSAMPLE> — apply the same here for consistency and safety.
🛡️ Switch to heap allocation
- constexpr SINT kBufferSize = 16384;
- CSAMPLE readBuffer[kBufferSize];
+ constexpr SINT kBufferSize = 16384;
+ std::vector<CSAMPLE> readBuffer(kBufferSize);
for (SINT i = 0; i < kBufferSize; ++i) {
- readBuffer[i] = static_cast<CSAMPLE>(0.7f);
+ readBuffer[i] = static_cast<CSAMPLE>(0.7f);
}
- m_pReadAheadMock->setReadBuffer(readBuffer, kBufferSize);
+ m_pReadAheadMock->setReadBuffer(readBuffer.data(), kBufferSize);(apply the same change to the 8192/4096 arrays in the other tests).
Also applies to: 397-402
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/test/enginebufferscalebungeetest.cpp` around lines 241 - 247, The stack
allocation of large CSAMPLE arrays (e.g., the readBuffer defined with constexpr
SINT kBufferSize = 16384 and used with m_pReadAheadMock->setReadBuffer) must be
replaced with heap allocation to avoid stack overflows; change these fixed-size
arrays to std::vector<CSAMPLE> vec(kBufferSize); fill vec via vec[i] or
std::fill, then call m_pReadAheadMock->setReadBuffer(vec.data(), vec.size());
apply the same replacement for the other large arrays (8192/4096 elements
referenced in this file and the arrays around lines 397-402) to ensure
consistency with ReusesBufferedInputAcrossOverlappingGrains.
Coverage Report for CI Build 24553962548Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 30.981%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
|
This PR is marked as stale because it has been open 90 days with no activity. |
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
3 participants
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.
REVIEWERS TIP: In Files Changed you can hold Alt (or on mac: option) before clicking the arrow next to a file to collapse all changed files. Then you can scroll to the bottom to see the integration files and avoid looking at the bungee library files.
TODO: commits need to be prepared correctly for trunk-driven development. I'd like some feedback on a simple minimum acceptable way to implement that, have barely started at all here: #13
About the Library
I used Bungee because it's better than Signalsmith for realtime/on-the-fly audio. It sounds REALLY NICE.
Also, bungee uses submodules and so that needs to be considered with integrating this library, it's currently statically uploaded, that's were most of the added lines come from.
Bungee library is in
lib/bungeeThe only difference between that and bungee's github repo is that there's a patch for windows MSVC compilation in lib/bungee. (and the submodules have been pulled and some unnecessary files removed.) To update Bungee one only needs to use git pull, and maybe fix the patch if line numbers/implementations change.There are some optional bungee specific optimizations that would've complicated implementation and were therefore not used. It's still very performant.
Other Notes on the PR
Note: bungee is the default engine in this branch.
Dual threaded stereo not adapted/necessary for bungee, it has been disabled when bungee is enabled.