Bungee Engine, Memory Cues, and Beatgrid - #14
Open
0cwa wants to merge 90 commits into
Open
Conversation
Add downbeatOffset to track
Thanks for noticing @Veganachommunist
Not ideal but at least prevents accidently deleting a little better
…-glitches-based-on-identifie Fix Bungee audio glitches: correct buffer size and channel pointer handling
…ead of fixed advance
The Bungee buffer scaler was causing 8.18x speedup regardless of playback speed setting. A 3-minute song was playing in 22 seconds.
The Bungee library's `speed` parameter represents the **input-to-output frame ratio**, not the effective playback rate. The code was incorrectly multiplying `base_rate` into this speed parameter.
From Bungee's library documentation (Stream.h line 144):
```cpp
request.speed = inputFrameCount / outputFrameCount;
```
This means:
- `speed = 1.0`: No time stretch (normal playback)
- `speed = 2.0`: Output is 2x faster than input (compress time)
- `speed = 0.5`: Output is 2x slower than input (stretch time)
Bungee handles sample rate conversion internally via the `resampleMode` parameter, so `base_rate` should NOT be mixed into the speed parameter.
**Line 148-151 (setScaleParameters)**:
```cpp
// Before:
m_request.speed = m_dBaseRate * m_dTempoRatio;
// After:
// Bungee's speed parameter is the input/output frame ratio.
// Use only the tempo ratio (playback speed) without base_rate.
// Bungee handles sample rate conversion internally via resampleMode.
m_request.speed = m_dTempoRatio;
```
**Lines 222-230 (processGrain)**:
```cpp
// Before:
double speed = m_dBaseRate * m_dTempoRatio;
if (m_bBackwards) {
speed = -speed;
}
// After:
// For Bungee's request, use only the tempo ratio (input/output frame ratio)
double speed = m_dTempoRatio;
if (m_bBackwards) {
speed = -speed;
}
// Calculate effective rate for ReadAheadManager (includes base_rate for sample rate conversion)
const double effectiveRate = m_dBaseRate * m_dTempoRatio;
```
**Lines 262-266 (processGrain)**:
```cpp
// Before:
const SINT availableSamples = m_pReadAheadManager->getNextSamples(
speed,
m_interleavedReadBuffer.data(),
samplesNeeded,
getOutputSignal().getChannelCount());
// After:
const SINT availableSamples = m_pReadAheadManager->getNextSamples(
effectiveRate,
m_interleavedReadBuffer.data(),
samplesNeeded,
getOutputSignal().getChannelCount());
```
**Lines 391-398 (scaleBuffer flush path)**:
```cpp
// Before:
const SINT samplesToRead = getOutputSignal().frames2samples(kMaxGrainFrames);
const SINT availableSamples = m_pReadAheadManager->getNextSamples(
(m_bBackwards ? -1.0 : 1.0) * m_dBaseRate * m_dTempoRatio,
m_interleavedReadBuffer.data(),
samplesToRead,
getOutputSignal().getChannelCount());
// After:
const SINT samplesToRead = getOutputSignal().frames2samples(kMaxGrainFrames);
const double effectiveRate = (m_bBackwards ? -1.0 : 1.0) * m_dBaseRate * m_dTempoRatio;
const SINT availableSamples = m_pReadAheadManager->getNextSamples(
effectiveRate,
m_interleavedReadBuffer.data(),
samplesToRead,
getOutputSignal().getChannelCount());
```
1. **Bungee's speed parameter** now receives only the tempo ratio (playback speed), not including base_rate
2. **ReadAheadManager** receives the effective rate (base_rate * tempo_ratio) for proper position tracking
3. This ensures Bungee's time stretching calculations are correct while the ReadAheadManager still tracks the correct position
Expected behavior after fix:
- 1.0x speed: Normal playback
- 0.5x speed: Half-speed playback with correct pitch
- 2.0x speed: 2x faster playback with correct pitch
- 0.1x speed: 10% speed playback with correct pitch (should not have super-deep pitch issue)
This fix aligns with how RubberBand handles time ratios:
- RubberBand uses `setTimeRatio(1.0 / (base_rate * tempo_ratio))` - it calculates the inverse
- Bungee uses `speed = tempo_ratio` directly - it uses the ratio directly
The key difference is that RubberBand's `timeRatio` is stretched/unstretched duration ratio, while Bungee's `speed` is input/output frame ratio.
Fixed the issue where Bungee keylock causes super fast speedup in Release builds but works correctly in RelWithDebInfo builds. The problem was related to compiler optimization flags (-ffast-math -O3) affecting floating-point math in the scaleBuffer return value calculation. The fix uses m_effectiveRate instead of m_dBaseRate * m_dTempoRatio directly in the accumulation loop, matching the pattern used by RubberBand and SoundTouch scalers. This prevents floating-point reassociation issues that can occur with aggressive compiler optimizations. Changes: - Update m_effectiveRate in setScaleParameters() - Use m_effectiveRate in scaleBuffer() return value calculation - Reset m_effectiveRate in clear() for consistent state
…build wiring, docs and tests BNG-01 through BNG-05, BNG-07: - EngineBufferScaleBungee: replace per-grain isolated reads with a sliding deinterleaved planar input window aligned to Bungee InputChunk ranges. Correct muteHead/muteTail are computed and passed to analyseGrain(); the old 'muteHead=0 always' approach violated Bungee's streaming contract and was the root cause of the garbled/8x-speed keylock regression. - Remove the discarded-read fallback path that advanced ReadAheadManager without consuming the returned samples, causing playback-position drift. - Use Bungee::Stretcher::next() for grain-position stepping instead of a hand-rolled constant increment. - Preallocate all buffers in onSignalChanged(); no heap allocation in the steady-state engine path. - Document rate/position/reset semantics and the InputChunk sliding-window contract in enginebufferscalebungee.h so future maintainers need not rediscover them from branch archaeology. - Strengthen enginebufferscalebungeetest: ReadAheadManager mock tracks call counts and sample totals; new tests for zero-speed, buffer clearing, EOF/read-failure handling, overlapping-grain buffer reuse, rapid parameter changes, and signal-format changes. - CMakeLists.txt: gate the MSVC-only Bungee patch inside if(MSVC), and fix the WORKING_DIRECTORY to CMAKE_CURRENT_SOURCE_DIR so patch paths (lib/bungee/src/...) resolve correctly. Linux configure no longer dirties lib/bungee. All 23 EngineBuffer* tests pass (12 Bungee + 11 EngineBufferTest). lib/bungee sources are not modified; all changes are in Mixxx integration.
…selection (BNG-06) Three integration tests using the real EngineBufferScaleBungee (not a mock) to verify engine-level state management when Bungee is the active keylock engine: - BungeeEngineSelected: selecting KeylockEngine::Bungee wires m_pScaleKeylock to m_pScaleBungee, and enabling keylock advances m_pScale accordingly. - BungeeKeylockToggleDoesNotCrash: rapidly toggling keylock on/off while Bungee is active produces finite (non-NaN) audio and does not crash. - BungeeKeylockEngineSwitch: switching between SoundTouch and Bungee mid-playback keeps m_pScaleKeylock consistent and the output finite. Tests inherit BaseSignalPathTest directly (real CachingReader + real ReadAheadManager) without calling setScalerForTest(), so the normal engine-selection code path is exercised. Added FRIEND_TEST declarations to enginebuffer.h for the three new tests. Registered enginebufferbungeetest.cpp in CMakeLists.txt under the BUNGEE feature flag (same guard as enginebufferscalebungeetest.cpp). 44/44 EngineBuffer* tests pass.
…hrough BNG-12) BNG-08 — Output interleave optimisation: Add copyOutputFrames() helper to EngineBufferScaleBungee. For the common stereo case it delegates to SampleUtil::interleaveBuffer() (SIMD-optimised on x86/ARM); for N-channel audio it falls back to a scalar loop. Replaces two identical hand-rolled interleave loops in processGrain(). Also swap the bare <QtDebug> include for the proper util/assert.h (which includes <QtDebug> and defines DEBUG_ASSERT). BNG-09 — Prefs UI correctness: The dual-threading checkbox is already hidden and disabled for non-RubberBand engines (including Bungee) by the existing updateKeylockDualThreadingCheckbox() logic. Clarify the tooltip copy from "only available with RubberBand" to "only available with the RubberBand engine" so it reads correctly if the checkbox is ever made visible in future. BNG-10 — Maintainer documentation: Add docs/bungee-integration.md covering: architecture overview, rate/position semantics, the InputChunk sliding-window contract, why muteHead=0 was wrong, output interleave, reset/flush protocol, memory layout, the Windows patch scope, and what not to change casually. BNG-11 — CI regression gate: Add -DBUNGEE=ON to the Ubuntu 24.04 CI build so that EngineBufferScaleBungeeTest.* and EngineBufferBungeeTest.* run on every push. Bungee is vendored under lib/bungee/ so no new CI dependencies are needed. BNG-12 — Final cleanup: No stale theory comments remain in the Bungee sources. The overall diff across this branch tells a coherent story: root-cause identification to sliding-window rewrite to RAMAN accounting fix to CMake guard to tests to integration tests to performance and docs and CI. 15/15 Bungee tests pass (12 unit + 3 integration).
Fix Bungee keylock super fast speedup in Release builds
…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.
Merge resolution for src/library/rekordbox/rekordboxfeature.cpp: The conflict arose because both branches independently evolved the readAnalyze() memory-cue loop from the common ancestor (dd97c25): - main (2ede2e0 "Rekordbox hotcues stay hotcues distinct from memory cues") refactored the loop to use the new setMemoryCue() helper for all memory cues, but inadvertently stripped the setMainCuePosition() call that mapped the first chronological memory cue to Mixxx's main cue (the default playback-start marker). - feature/bungee (28c87ad "crash fix") kept the original mainCueFound + setMainCuePosition pattern from the ancestor, and correctly added null-checks for pMainCue and memoryCueOrLoop.color to fix a latent null-dereference. However it kept setHotCue() in the else branch, which predates setMemoryCue() and would regress the 2ede2e0 fix by giving subsequent memory cues the wrong cue type. Resolution: combine both improvements: - Restore bool mainCueFound / setMainCuePosition (lost from main) - Add pMainCue null-check and color optional-check (from feature/bungee) - Use setMemoryCue() in the else branch (from main) so subsequent memory cues and loops retain CueType::Memory / CueType::Loop, not HotCue Also kept from feature/bungee (non-conflicting, already staged): - key_id INTEGER column in the rekordbox temp table + KeyUtils binding (needed so columncache.cpp key-sort SQL works in the Rekordbox view) - #include track/keyutils.h Already staged from feature/bungee before conflict: - docs/bungee-integration.md markdown table formatting (d1006cb) - enginebufferscalebungee.cpp heap-corruption fix (87e4849)
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.
|
This PR is marked as stale because it has been open 90 days with no activity. |
Owner
Author
|
not stale :( |
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.
The coolest version of mixxx yet