daniel/reasampler
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Close derived-bake-window review findings: NaN-guard remaining wire doubles, pin Trigger-span agreement, retire dead quantizer

Browse Source 
Guards params_payload.cpp's filter-tail seconds and both keyTrack sites against NaN; pins Voice::start's Trigger-span formula against trigger_seam; retires unused shortestDivisionAtLeast.
This commit is contained in:
Daniel Harvey
2026-08-01 21:32:05 -04:00
parent 65f6070348
commit eb6093e085
7 changed files with 84 additions and 92 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/core/instrument/map/params_payload.cpp
+14 -8
View File
@@ -244,6 +244,7 @@ void readBakeHold(ByteReader& r, InstrumentParams& p) {
// bipolar at EVERY version — a pre-v12 blob's y values are already valid bipolar ones, so its
// v12 domain re-tag needs no version branch (see component_state_io.h).
void readFilterTail(ByteReader& r, InstrumentParams& p) {
const FilterSeconds fallback; // the construction defaults, read rather than restated
FilterSeconds& f = p.play.filter;
f.enabled = (r.u8() != 0);
f.settings.cutoffNorm = static_cast<float>(bitsToDouble(r.u64()));
@@ -260,11 +261,11 @@ void readFilterTail(ByteReader& r, InstrumentParams& p) {
f.modAmount = std::isfinite(modAmount) ? modAmount : 0.0;
f.velAmount = std::isfinite(velAmount) ? velAmount : 0.0;
f.keyTrack = std::isfinite(keyTrack) ? keyTrack : 0.0;
f.env.attackSeconds = bitsToDouble(r.u64());
f.env.holdSeconds = bitsToDouble(r.u64());
f.env.decaySeconds = bitsToDouble(r.u64());
f.env.sustainLevel = bitsToDouble(r.u64());
f.env.releaseSeconds = bitsToDouble(r.u64());
f.env.attackSeconds = finiteOr(bitsToDouble(r.u64()), fallback.env.attackSeconds);
f.env.holdSeconds = finiteOr(bitsToDouble(r.u64()), fallback.env.holdSeconds);
f.env.decaySeconds = finiteOr(bitsToDouble(r.u64()), fallback.env.decaySeconds);
f.env.sustainLevel = finiteOr(bitsToDouble(r.u64()), fallback.env.sustainLevel);
f.env.releaseSeconds = finiteOr(bitsToDouble(r.u64()), fallback.env.releaseSeconds);
readCurveTail(r, f.velocityCurve, reasampler::instrument::engine::CurveDomain::Bipolar);
}
@@ -371,8 +372,10 @@ PayloadRead readLegacyZonePayload(ByteReader& r, std::uint32_t pv, double projec
readSecondsPlayTail(r, p, projectRate);
}
// A pre-v6 payload leaves keyTrack = 1.0 (100% ET), so an already-saved instance
// repitches BIT-IDENTICALLY. A pre-v7 payload leaves VelocityCurve::flat().
if (keyTrackTail) p.keyTrack = bitsToDouble(r.u64());
// repitches BIT-IDENTICALLY. A pre-v7 payload leaves VelocityCurve::flat(). A NaN
// reaches keyTrackedRatio -> baseRatio_ -> readPos_'s per-sample cast (voice.h) — the
// same guard the v8+ single-record reader applies to its own keyTrack below.
if (keyTrackTail) p.keyTrack = finiteOr(bitsToDouble(r.u64()), InstrumentParams{}.keyTrack);
if (curveTail) {
readCurveTail(r, p.velocityCurve,
reasampler::instrument::engine::CurveDomain::Unipolar);
@@ -500,7 +503,10 @@ PayloadRead readParamsPayload(ByteReader& r, double projectRate) {
const std::uint8_t hasStart = r.u8();
if (hasStart) p.startPoint = r.i64();
readSecondsPlayTail(r, p, projectRate);
p.keyTrack = bitsToDouble(r.u64());
// NaN reaches keyTrackedRatio (voice.h) -> baseRatio_ -> readPos_'s per-sample
// static_cast<std::int64_t> — UB on the per-sample path. Guarded here, codec-side, so
// that path needs no check of its own.
p.keyTrack = finiteOr(bitsToDouble(r.u64()), InstrumentParams{}.keyTrack);
readCurveTail(r, p.velocityCurve, reasampler::instrument::engine::CurveDomain::Unipolar);
if (pv >= kParamsFilterVersion) readFilterTail(r, p);
if (pv >= kParamsCurveVersion) readCurveStageTail(r, p);
src/core/instrument/note/musical_division.cpp
-21
View File
@@ -54,27 +54,6 @@ int divisionIndex(Division d) {
+ static_cast<int>(d.modifier());
}
Division shortestDivisionAtLeast(double beats) {
// Picker order is NOT length order — a rung's triplet is shorter than the previous rung's
// dotted — so both the fit and the fallback are found by scanning. 39 entries.
Division longest = divisionAt(0);
double longestBeats = divisionBeats(longest);
Division best = longest;
double bestBeats = 0.0;
bool found = false;
for (int i = 0; i < kDivisionCount; ++i) {
const Division d = divisionAt(i);
const double b = divisionBeats(d);
if (b > longestBeats) { longest = d; longestBeats = b; }
if (b >= beats && (!found || b < bestBeats)) { best = d; bestBeats = b; found = true; }
}
// NaN names no length to be at least, so it takes the never-short direction rather than
// the bottom rung: a corrupt value must not resolve to a near-instant note.
if (std::isnan(beats)) return longest;
if (!(beats > 0.0)) return divisionAt(0);
return found ? best : longest;
}
std::string divisionLabel(Division d) {
const int e = d.quarterExponent();
// Both branches meet at e == 2 ("1/1"): a division's written form is its length in
src/core/instrument/note/musical_division.h
-7
View File
@@ -69,13 +69,6 @@ double divisionBeats(Division d);
Division divisionAt(int index);
int divisionIndex(Division d);
// The shortest ladder length that is at least `beats` — for a caller quantizing a duration ONTO
// the ladder, where overshooting is the safe direction. It is NOT how a derived duration reaches
// the bake: nothing on a finite ladder covers an arbitrarily long source (see note/CLAUDE.md).
// Nothing long enough, or a non-finite `beats`, yields the top rung; a non-positive one yields
// the bottom.
Division shortestDivisionAtLeast(double beats);
// The notation divisions are named in: "1/16", "1/8.", "1/4t", "4/1".
std::string divisionLabel(Division d);
tests/test_component_state_io.cpp
+34 -3
View File
@@ -954,25 +954,41 @@ static void testV13HardFlagCountThatStrandsAlignmentLeavesTheHoldAbsentNotFabric
// Numeric domains are established at the DOOR, not at each consumer. A NaN pitch depth reaches
// the bake's pow() and the voice's ratio multiply; a NaN %-length and a NaN stage time reach
// narrowing casts that are undefined on one; and a root override outside MIDI range makes the
// narrowing casts that are undefined on one; a NaN keyTrack reaches keyTrackedRatio ->
// baseRatio_ -> readPos_'s per-sample static_cast<std::int64_t> (voice.h); a NaN v9 filter
// env second reaches secToFrames the same way the trigAhd/filter.trigEnv pair already covered
// by testNonFiniteAhdSecondsLiftToZero do; and a root override outside MIDI range makes the
// bake's render note and the sample's own root disagree, which is a read rate other than 1 and
// therefore a window sized in the truncating direction.
static void testOutOfDomainWireValuesAreBoundedAtTheCodec() {
const PlaySeconds defaults;
const InstrumentParams paramDefaults;
ComponentState in;
in.selectionId = "pad";
in.params.keyTrack = 0.5; // a neighbouring field, to show the guards are per-field
in.params.keyTrack = std::numeric_limits<double>::quiet_NaN();
in.params.rootOverride = 9999;
in.params.play.pitchEnv.peakSemitones = std::numeric_limits<double>::quiet_NaN();
in.params.play.trigger.lengthFraction = std::numeric_limits<double>::quiet_NaN();
in.params.play.adsr.releaseSeconds = std::numeric_limits<double>::infinity();
in.params.play.filter.enabled = true;
in.params.play.filter.env.attackSeconds = std::numeric_limits<double>::quiet_NaN();
in.params.play.filter.env.holdSeconds = std::numeric_limits<double>::infinity();
in.params.play.filter.env.decaySeconds = -std::numeric_limits<double>::infinity();
in.params.play.filter.env.sustainLevel = std::numeric_limits<double>::quiet_NaN();
in.params.play.filter.env.releaseSeconds = std::numeric_limits<double>::quiet_NaN();
const ComponentState out = deserializeComponentState(serializeComponentState(in), 48000.0);
CHECK(out.params.rootOverride && *out.params.rootOverride == 127);
CHECK(out.params.play.pitchEnv.peakSemitones == defaults.pitchEnv.peakSemitones);
CHECK(out.params.play.trigger.lengthFraction == defaults.trigger.lengthFraction);
CHECK(out.params.play.adsr.releaseSeconds == defaults.adsr.releaseSeconds);
CHECK(out.params.keyTrack == 0.5);
CHECK(out.params.keyTrack == paramDefaults.keyTrack);
CHECK(out.params.play.filter.enabled); // the fallback is per-field, not per-record
CHECK(out.params.play.filter.env.attackSeconds == defaults.filter.env.attackSeconds);
CHECK(out.params.play.filter.env.holdSeconds == defaults.filter.env.holdSeconds);
CHECK(out.params.play.filter.env.decaySeconds == defaults.filter.env.decaySeconds);
CHECK(out.params.play.filter.env.sustainLevel == defaults.filter.env.sustainLevel);
CHECK(out.params.play.filter.env.releaseSeconds == defaults.filter.env.releaseSeconds);
ComponentState low = in;
low.params.rootOverride = -5;
@@ -980,6 +996,20 @@ static void testOutOfDomainWireValuesAreBoundedAtTheCodec() {
CHECK(lowOut.params.rootOverride && *lowOut.params.rootOverride == 0);
}
// The keyTrack guard's OTHER site: a pre-v7 (legacy zone-list) blob's own keyTrack field
// (params_payload.cpp's readLegacyZonePayload, pv >= 6) is a second, independent read of the
// same wire double — same hazard, same fallback, must not be missed just because the v8+
// single-record reader above was fixed.
static void testLegacyZoneKeyTrackNaNLiftsToDefault() {
legacy::Zone z;
z.sampleId = "kick";
z.keyTrack = std::numeric_limits<double>::quiet_NaN();
const ComponentState out =
deserializeComponentState(legacy::envelopeWithZones("kick", {z}, 7), 48000.0);
CHECK(out.params.keyTrack == InstrumentParams{}.keyTrack);
}
// A hard-flag tail truncated mid-COUNT-FIELD (only 2 of its 4 length bytes present, and
// nothing else after) is a different failure shape than a declared-huge count: the u32 read
// itself fails, tripping r.ok inside readHardFlags rather than its own bound check. That must
@@ -1935,6 +1965,7 @@ int main() {
testV13HardFlagOutOfBoundsCountSurvivesWithoutWipingTheRecord();
testV13HardFlagCountThatStrandsAlignmentLeavesTheHoldAbsentNotFabricated();
testOutOfDomainWireValuesAreBoundedAtTheCodec();
testLegacyZoneKeyTrackNaNLiftsToDefault();
testV13HardFlagTailTruncatedMidCountSurvivesWithoutWipingTheRecord();
testBakeHoldRoundTripsAndDisturbsNothingElse();
testV13BlobLiftsToTheDefaultHold();
tests/test_musical_division.cpp
+2 -51
View File
@@ -3,15 +3,13 @@
//
// Covers: the beat length of all 39 divisions against a literal rung table (NOT the module's
// own exponent formula); the 1/64 and 64/1 extremes; the four named example divisions; the
// label notation; picker order and index round-trip; off-ladder clamping of BOTH persisted
// fields, measured through the readers rather than by comparing two clamped values; and
// shortestDivisionAtLeast's never-short contract across the rung boundaries.
// label notation; picker order and index round-trip; and off-ladder clamping of BOTH persisted
// fields, measured through the readers rather than by comparing two clamped values.
#include "../src/core/instrument/note/musical_division.h"
#include <cmath>
#include <cstdio>
#include <limits>
using namespace reasampler::instrument::note;
@@ -206,54 +204,7 @@ static void testOutOfRangeIndexClampsIntoTheSet() {
CHECK(divisionAt(kDivisionCount) == divisionAt(kDivisionCount - 1));
}
// --- shortestDivisionAtLeast --------------------------------------------------
// The contract is never-short: whatever comes back is at least the requested length, and
// nothing shorter on the ladder also is. Swept over every rung and over the gaps between them.
static void testShortestAtLeastIsNeverShortAndNeverLonger() {
for (int i = 0; i < kDivisionCount; ++i) {
const double target = divisionBeats(divisionAt(i));
for (const double want : {target, target * 0.99, target * 0.5}) {
const Division got = shortestDivisionAtLeast(want);
CHECK(divisionBeats(got) >= want - 1e-12);
// Nothing on the ladder is both long enough and shorter than the answer.
for (int j = 0; j < kDivisionCount; ++j) {
const double other = divisionBeats(divisionAt(j));
CHECK(!(other >= want && other < divisionBeats(got) - 1e-12));
}
}
}
}
// Picker order is NOT length order — a rung's triplet is shorter than the previous rung's
// dotted — so the answer is not simply "the next index up".
static void testShortestAtLeastCrossesRungBoundaries() {
// 5 beats: the 2/1 triplet (8 * 2/3 == 5.33) beats the dotted half (6) above it.
CHECK(shortestDivisionAtLeast(5.0) == makeDivision(3, DivisionModifier::Triplet));
// An exact rung length answers with that rung, not the one above it.
CHECK(shortestDivisionAtLeast(4.0) == makeDivision(2, DivisionModifier::Straight));
}
// Degenerate inputs land on an end rather than anywhere in the middle.
static void testShortestAtLeastDegenerateInputs() {
CHECK(shortestDivisionAtLeast(0.0) == divisionAt(0));
CHECK(shortestDivisionAtLeast(-10.0) == divisionAt(0));
// Past the longest programmable note (the dotted top rung), the top rung is the answer.
const Division longest = makeDivision(kMaxQuarterExponent, DivisionModifier::Dotted);
CHECK(almostEqual(divisionBeats(longest), kMaxDivisionBeats));
CHECK(shortestDivisionAtLeast(kMaxDivisionBeats * 2.0) == longest);
// A non-finite request names no length to be at least, so it takes the SAME never-short
// answer as one nothing covers. Landing on the bottom rung instead would turn a corrupt
// value into a near-instant note.
CHECK(shortestDivisionAtLeast(std::nan("")) == longest);
CHECK(shortestDivisionAtLeast(std::numeric_limits<double>::infinity()) == longest);
}
int main() {
testShortestAtLeastIsNeverShortAndNeverLonger();
testShortestAtLeastCrossesRungBoundaries();
testShortestAtLeastDegenerateInputs();
testLadderSpansSixtyfourthToSixtyFourWhole();
testEveryStraightRungHasItsWrittenBeatLength();
testDottedIsHalfAgainAndTripletIsTwoThirds();
tests/test_note_program.cpp
+4 -2
View File
@@ -219,8 +219,10 @@ static void testAnExactLengthCarriesDurationsPastTheLaddersTopRung() {
const Tempo t = at(120.0);
const double pastTheLadder = t.beatsToSeconds(kMaxDivisionBeats) * 3.0;
CHECK(almostEqual(noteLengthSeconds(lengthOfSeconds(pastTheLadder), t), pastTheLadder));
// …and the ladder really does saturate there, which is what makes the seam load-bearing.
CHECK(almostEqual(divisionBeats(shortestDivisionAtLeast(kMaxDivisionBeats * 3.0)),
// …and the ladder really does saturate there, which is what makes the seam load-bearing:
// its longest rung (the dotted top) IS kMaxDivisionBeats, so nothing on it reaches
// pastTheLadder.
CHECK(almostEqual(divisionBeats(makeDivision(kMaxQuarterExponent, DivisionModifier::Dotted)),
kMaxDivisionBeats));
}
tests/test_sampler_core.cpp
+30
View File
@@ -1380,6 +1380,34 @@ static void testTriggerEdgeCases() {
}
}
// --- Trigger out-of-domain lengthFraction: Voice::start's inline copy of trigger_seam's
// formula (map/trigger_seam.h) must clamp the same way the shared formula does — a
// frac > 1.0 never plays past the post-start span, and a NaN frees immediately rather than
// reaching the round()/static_cast<int64_t> undefined on a non-finite value. Pins the second
// implementation directly; test_trigger_seam.cpp pins the first.
static void testTriggerFracAboveOneClampsToSpan() {
// 150% length on a 200-frame sample -> clamped to the full 200-frame post-start span,
// not 300 frames (which would read off the end of the PCM).
SampleData km = (triggerSample(200, 1.5, 0, 0));
VoiceEngine eng(1, km);
eng.noteOn(60, 127);
std::vector<AudioSample> out;
eng.render(out, 220);
for (std::size_t i = 0; i < 200; ++i) CHECK(out[i] > 0.5f);
for (std::size_t i = 200; i < 220; ++i) CHECK(approx(out[i], 0.0, 1e-6));
CHECK(eng.activeVoiceCount() == 0); // frees exactly at frameCount
}
static void testTriggerFracNaNFreesImmediately() {
SampleData km = (triggerSample(200, std::nan(""), 5, 5));
VoiceEngine eng(1, km);
eng.noteOn(60, 127);
std::vector<AudioSample> out;
eng.render(out, 50);
for (float v : out) CHECK(approx(v, 0.0, 1e-6));
CHECK(eng.activeVoiceCount() == 0); // playLen 0 -> frees at start, no sound
}
// --- Trigger ignores note-off (S15): the one-shot plays through regardless. ---
static void testTriggerIgnoresNoteOff() {
SampleData km = (triggerSample(200, 0.5, 0, 0));
@@ -2908,6 +2936,8 @@ int main() {
testTriggerLengthWithStart();
testTriggerAhdFadeShape();
testTriggerEdgeCases();
testTriggerFracAboveOneClampsToSpan();
testTriggerFracNaNFreesImmediately();
testTriggerIgnoresNoteOff();
// S16 — pitch engine + pitch envelope.