Fix SoundTouch producing silence through the read-ahead time-stretch path - #405
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #405 +/- ##
===========================================
+ Coverage 58.74% 58.96% +0.22%
===========================================
Files 564 564
Lines 78879 78920 +41
Branches 12330 12331 +1
===========================================
+ Hits 46338 46539 +201
+ Misses 32541 32381 -160 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Pushed 62bd6b5 after the first CI run went red. The new alignment test also covers RubberBand, which CI enables by adding the rubberband submodule at build time. That exposed a second, unrelated latent bug in the non-read-ahead
This is why Verified locally with the rubberband submodule added: all 128 alignment cases (8 test setups x soundtouchNormal/Better + rubberbandMelodic/Percussive x read-ahead on/off) now run clean. Before the fix it aborted on the first Separately, the |
|
CI after 62bd6b5: 19 pass / 17 fail, and every remaining failure is a build failure that never reaches the test stage. Fixed by this push (all were red before):
The 17 that stay red are all pre-existing toolchain/API rot on the current runner images, none of it in code this PR touches:
Master last went green in August 2025 on older runner images. Happy to open a separate issue for the CI refresh if useful. Also verified locally with the rubberband submodule added, Debug: 128/128 alignment cases clean and doctest 22/22 pass - including |
|
I don't think this is the correct approach as it breaks the contract of getFramesNeeded(). That should always return the number of frames required to generate at least one block. Perhaps the SoundTouchStretcher needs to be fixed so getMaxFramesNeeded() and getFramesNeeded() work as advertised? |
|
Opened #407 for the CI breakage so it does not get conflated with this change. |
62bd6b5 to
91197c6
Compare
|
Rebased onto |
…d reader SoundTouchStretcher::getFramesNeeded() did not honour its contract of returning enough frames to produce at least one block. SoundTouch buffers its initial latency worth of input (~4k frames at 44.1kHz, up to ~13k at 96kHz with a 0.5 speed ratio) before it emits anything, but the stretcher only ever reported a single block's worth, so the first processData call after a reset returned nothing. The non-read-ahead TimeStretchReader loops until output appears and hid this; ReadAheadTimeStretchReader pushes getFramesNeeded() frames, pops one block and treats an empty pop as end-of-data, so playback stayed silent whenever read-ahead was enabled with a SoundTouch mode. The hard-coded getMaxFramesNeeded() of 8192 was also smaller than the priming requirement at 96kHz. - SoundTouchStretcher::getFramesNeeded() now includes SoundTouch's own SETTING_INITIAL_LATENCY until the first batch has been produced since the last reset, clamped to getMaxFramesNeeded(), mirroring how RubberBandStretcher handles priming. - getMaxFramesNeeded() is computed in the constructor from the initial latency at the slowest supported speed ratio (0.25) plus a block's input at the fastest (4), so it is sample-rate aware. - Removed the TimeStretchReader assertion comparing output FIFO space against an input frame count; processData only writes up to chunkSize frames, which the following assertion already covers, and RubberBand's priming request trips it as soon as any output is queued. - Documented the getFramesNeeded() contract on TimeStretcher. - Added runFramesNeededContractTest, checking for every enabled stretcher that pushing exactly getFramesNeeded() frames yields output immediately after initialisation and after a reset, and that it never exceeds getMaxFramesNeeded(). The syncTestModes latency-compensation test now runs against both ReadAhead values, and the read-ahead playback test's guard covers any enabled algorithm rather than RubberBand only.
91197c6 to
c7078cf
Compare
|
Agreed, that was the wrong layer. Reworked: the reader-side retry is gone and |
Problem
TimeStretcher::Mode::soundtouchBetterproduces completely silent output when used viaWaveNodeRealTime::ReadAhead::yes, i.e. whenever a host returnstruefromEngineBehaviour::enableReadAheadForTimeStretchNodes()(as the TestRunner does). RMS is exactly 0 across the whole buffer, at every sample rate. Without read-ahead SoundTouch works and is well aligned.Cause
SoundTouchStretcher::getFramesNeeded()did not honour its contract. SoundTouch buffers its initial latency worth of input before it emits anything, butgetFramesNeeded()only ever reported one block's worth (samplesPerBlock * ioRatio, 256-2048 frames). Measured input needed before the first output for the "better" settings:SETTING_INITIAL_LATENCYSo the first
processDatacall after a reset returned 0 frames. The non-read-aheadTimeStretchReaderloops until output appears and hides this;ReadAheadTimeStretchReaderpushesgetFramesNeeded()frames, pops one block, and treats an empty pop as end-of-data, so playback stayed silent. The hard-codedgetMaxFramesNeeded() == 8192was also wrong: at 96 kHz the priming requirement exceeds it.Fix
Make
SoundTouchStretcherreport what SoundTouch actually needs, following the patternRubberBandStretcheralready uses while priming:getFramesNeeded(): until the first batch has been produced since the last reset, returnSETTING_INITIAL_LATENCY + one block's input - numUnprocessedSamples(), clamped togetMaxFramesNeeded(). Afterwards the existing per-block formula applies. AhasProducedOutputflag (cleared inreset()) tracks this.getMaxFramesNeeded(): computed once in the constructor fromSETTING_INITIAL_LATENCYat the slowest supported speed (0.25, tempo 4) plus a block's input at the fastest (4), so it is sample-rate aware rather than a magic number. Speed ratios outside 0.25-4 still work but may need more than one process call for the first block.SETTING_INITIAL_LATENCYis re-read insetSpeedAndPitchsince it depends on tempo/rate.No reader changes are needed.
TimeStretchReaderloses itsassert (outputFifo.getFreeSpace() >= numThisTime), which compared output space against an input count -processDataonly writes up tochunkSizeframes, which the next assertion already covers, and it fires as soon as RubberBand's 8192-frame priming request coincides with queued output.TimeStretcher::getFramesNeeded()docs now state the contract: this many frames must yield at least one block, including the first call after a reset, and never exceedgetMaxFramesNeeded().Tests
runFramesNeededContractTestintracktion_TimeStretch.test.cpp: for 44.1/96 kHz x 64/512 block x five speed/pitch combinations, pushing exactlygetFramesNeeded()frames once must produce output, both straight after initialisation and again afterreset(), andgetFramesNeeded() <= getMaxFramesNeeded()throughout. Run for SoundTouch, RubberBand and Signalsmith.syncTestModeslatency-compensation block inrunTimestretchedTests()now runs every enabled algorithm against bothReadAhead::noandReadAhead::yes.Playback single audio clip using read-aheadguard relaxed from RubberBand-only to any enabled algorithm.Verified locally on macOS (Debug, RubberBand + Signalsmith + SoundTouch):
--source-file="*TimeStretch.test.cpp": 542/542 assertions (was 62 before the contract test).--source-file="*WaveNode.test.cpp": 3/3 cases, 1118/1118 assertions; all three algorithms run 8 setups x both read-ahead modes.SoundTouchStretcherchange reverted: 40 contract assertions fail (every SoundTouch combination, first push and post-reset push) and the 8soundtouchBetter, read-aheadWaveNode cases fail withrms == 0- the reported symptom. RubberBand and Signalsmith pass the contract unchanged.