Restore vocal-chord-emulation ("linear") pitch scaling - #9
Open
austek wants to merge 1 commit into
Open
Conversation
sonicSetChordPitch/sonicGetChordPitch were no-op stubs (same as upstream) -- the setter had nothing to write to, so the CLI's -c flag printed "Scaling pitch linearly" but silently produced byte-identical output to not passing -c at all. Restores adjustPitch, overlapAddWithSeparation, the useChordPitch struct field, and its two call sites in processStreamInput -- all byte-identical to what waywardgeek/sonic had before its 2022 rewrite dropped them, and all the helpers they depend on (moveNewSamplesToPitchBuffer, findPitchPeriod, enlargeOutputBufferIfNeeded, overlapAdd, removePitchSamples, the pitchBuffer/numPitchSamples struct fields) are unchanged and shared with changeSpeed's speed-change path. Clamps newPeriod (period / pitch) to [1, stream->maxRequired]: at this fork's current SONIC_MIN/MAX_PITCH_SETTING range (0.05-20.0), an unclamped newPeriod can hit 0 (dividing by it in overlapAdd) or exceed how many samples are actually buffered ahead of the current position (overlapAddWithSeparation reading past pitchBuffer) -- a heap-buffer-overflow, confirmed via ASan, that the pre-2022 code was never exercised against. Adds tests/chord_pitch_test.c, wired into this fork's tests/Makefile: - sonicTestChordPitchChangesOutput: enabling chord pitch must change output for the same pitch setting. - sonicTestChordPitchExtremeRatioDoesNotOverflow: processes audio at both ends of the pitch range with chord pitch enabled without crashing. Also drops sonic.h's "DEPRECATED... never used by anyone" comment on the getter/setter, now inaccurate. Ported from waywardgeek/sonic#71, which fixes the same bug (also present here, byte-identical processStreamInput/sonicCreateStream and the removed helpers' call sites) upstream.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Same bug, same fix as waywardgeek/sonic#71 — this repo's
sonicSetChordPitch/sonicGetChordPitchare the same no-op stubs, and the surrounding code (processStreamInput,sonicCreateStream, the struct definition) is byte-identical, so the fix ports directly.sonicSetChordPitch/sonicGetChordPitchhad nothing to write to — the CLI's-cflag prints "Scaling pitch linearly" but silently produces byte-identical output to not passing-cat all.Restores
adjustPitch,overlapAddWithSeparation, theuseChordPitchstruct field, and its two call sites inprocessStreamInput— all the helpers they depend on (moveNewSamplesToPitchBuffer,findPitchPeriod,enlargeOutputBufferIfNeeded,overlapAdd,removePitchSamples, thepitchBuffer/numPitchSamplesstruct fields) are unchanged and shared withchangeSpeed's speed-change path.Clamps
newPeriod(period / pitch) to[1, stream->maxRequired]: at this fork's current pitch-setting range (0.05–20.0), an unclampednewPeriodcan hit 0 (dividing by it inoverlapAdd) or exceed how many samples are actually buffered ahead of the current position (overlapAddWithSeparationreading pastpitchBuffer) — a heap-buffer-overflow, confirmed via ASan.Adds
tests/chord_pitch_test.c, wired into this fork'stests/Makefile.Also drops
sonic.h's "DEPRECATED... never used by anyone" comment on the getter/setter, now inaccurate.Test plan
cd tests && make runtests && ./runtests— passes, new tests included-fsanitize=address,undefined— no finding from this path; only the separate, already-knownfindSincCoefficientfinding remainssonicCLI from this fork's code directly and reproduced the fix end-to-end:-c -p 0.5and-p 0.5now produce different output (previously byte-identical)