File-backed PCM: refuse loop configs, and honor note-off - #1012
Conversation
Fixes #1011. What actually happens today, since the issue asks: nothing crashes, hangs or leaks an osc -- but two things go wrong quietly. 1. A PCM_LOOP* mode on a disk_sample() preset is accepted and then ignored. render_pcm guards its loop-back on `preset->type != AMY_PCM_TYPE_FILE`, so the clip plays once and stops with no indication the loop was dropped. 2. Worse, and not limited to looping: the immediate-stop modes don't stop a streamed preset at all. pcm_note_off implements "stop now" by seeking phase to the end of the sample, but render_pcm resets phase to 0 every block for file presets -- it refills a sliding window from the file rather than indexing a table -- so the seek is thrown away and the clip plays on to end-of-file. PCM_PLAY_STOP is the DEFAULT mode, so this hit every disk_sample() note-off, not just the LOOP ones. Measured with a 256ms clip and note-off at 100ms: an in-memory preset stopped at 100ms, a streamed one ran to 300ms. So: stop the osc outright on note-off for file presets, and degrade the loop modes to their nearest non-looping equivalent at note-on -- PCM_LOOP/PCM_LOOP_FOREVER to PCM_PLAY, PCM_LOOP_STOP to PCM_PLAY_STOP -- with a one-time warning per preset naming the file, so asking for a loop tells you why you didn't get one. Verified the degraded PCM_LOOP render is bit-identical to a plain PCM_PLAY of the same clip. Whole-file looping is not out of reach if we want it later: pcm_note_on already rewinds a file preset with fseek + a header re-parse, so EOF could do the same instead of stopping. What a stream can never honor is the loopstart/loopend marks, which is the part the docs now steer to load_sample for. Two regression tests, both golden-file: one pins the note-off stop, one pins the degraded loop render. 124 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
We haven't measured this yet but it was always my assumption that the esp32 flash or sd card reader can't really support random access reading of a file quickly enough in a audio block. maybe future products could , IDK |
|
Rather than graceful fallback of behavior, I would rather refuse to let the user set up this situation. So, when the user sets the loop mode on an oscillator that is using a file-backed sample, or when they add a file-backed sample to an oscillator with looping enabled, AMY prints an error message and does not perform the command; the user is then prompted to modify their requested operation to be compliant. This avoids the complication of re-interpreting the loop mode at note-on time, and removes the need for an extra flag in each sample to track whether the user has already been warned (both of which struck me as regrettable additional complication to the common-case code). |
Reworks the #1011 fix per review: rather than accepting a PCM_LOOP* mode on a streamed preset and quietly substituting a non-looping one at note-on, AMY now refuses the command that would create the impossible configuration, so the state never reaches something it can't honor. Both halves are checked as they are set -- pcm_loop_config_allowed() is called from the MODE and the PRESET delta with the proposed value -- and the offending command is dropped after a warning naming the file. Only PCM oscillators are considered; mode means nothing for other waves. PRESET is now emitted before MODE, and that ordering is load-bearing. The check refuses whichever of the pair arrives second, and one message asking for both a streamed preset and a loop mode is the common way to hit this. With preset first, the MODE is what gets dropped, leaving a usable sample that plays once; the old order dropped the PRESET and left a loop mode pointing at nothing. No reference audio changed, so nothing else depended on that order. Kept from the original fix: the note-off repair. pcm_note_off stopped an osc by seeking phase to the end, but render_pcm resets phase to 0 every block for file presets, so the stop was discarded and the clip ran to end-of-file. That was the DEFAULT mode, so it affected every disk_sample() note-off. TestDiskSampleLoopModeDegrades becomes TestDiskSampleLoopModeRefused, and its render is bit-identical to TestDiskSampleStopsOnNoteOff, which asks for no mode at all -- proof the refused mode never took effect. 124 tests pass, 118 bit-exact. Also brings the branch up to date with main (54 commits) and converts its two tests off the time= kwarg removed in the meantime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reworked per review: instead of accepting a What changed
One thing worth reviewing: PRESET now precedes MODE
The check refuses whichever half arrives second. The common way to hit this is a single message asking for both a streamed preset and a loop mode — and I measured the old order doing the wrong thing there: With No reference audio changed as a result of the reorder, so nothing else depended on it. Kept from the original fixThe note-off repair, which was the more serious of the two bugs and is independent of this: Testing
Behavior across the four cases:
Also brought the branch up to date with 🤖 Generated with Claude Code |
The immediate-stop branch had two mechanisms glued together: seek phase past the end of the sample for in-memory presets, plus a SYNTH_OFF for file-backed ones because the seek didn't survive their per-block refill. SYNTH_OFF subsumes the seek. Both paths silence the same block -- the seek only took effect when render_pcm next saw base_index >= sample_length, which is the block the note-off lands in -- so setting the status directly says what we mean and covers both preset types with one line. partial_note_off() already ends a note this way. That also removes the reason the function looked the preset up at all: the get_preset_for_preset_number() walk existed solely to read length for the seek, so every PCM note-off was paying for a linked-list lookup it no longer needs. Verified rather than assumed: 124 tests pass with all 118 waveform comparisons bit-exact, so the substitution is not merely equivalent in the cases the issue was about but in every PCM case the suite covers. Checked one edge that looked like it might differ: PCM_LOOP_STOP where loopend < length. render_pcm tests base_index >= sample_length BEFORE the loop-back, so a phase seeked to length stopped rather than wrapping -- the old code was right there too, just doing it the long way round. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two params were handled by separate ifs several lines apart, each calling pcm_loop_config_allowed() with its own hand-built argument pair. Fold them into one block that works out which half this delta carries and checks once. Purely structural: play_delta handles a single delta, so only one of the two ifs could ever fire and their position in the chain never mattered. What decides which half gets refused is the delta QUEUE order, set by the EVENT_TO_DELTA emission order above, which is unchanged. 124 tests pass, 118 bit-exact, and all four configuration cases still behave as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🎛️ AMY HW CI (AMYboard bench)Flashed this PR's AMY (LoadTestChord: 6-voice Juno ✅ PASS — the bench ran the test to completion.
Full chord settled render μs: 2754 (was 2762, Δ -0.3%) (peak 2757, 39 samples) ⬇️ Artifacts: serial log · load trace · report Self-hosted bench (amyboardci). FAIL means only that the test could not run — the load values are informational, with no threshold and no audio compare. See |
⛓️ tulipcc integration PR openedThis merge was pinned into tulipcc for full-system CI: shorepine/tulipcc#1277 Test it there and merge that PR to move tulipcc onto this AMY. |
Fixes #1011.
What happens today
The issue asks what currently happens, so I measured it first. Nothing crashes, hangs, or leaks an oscillator — but two things go wrong quietly, and the second one is worse than the reported problem.
1.
PCM_LOOP*on adisk_sample()preset is accepted, then ignored.render_pcmguards its loop-back onpreset->type != AMY_PCM_TYPE_FILE, so the clip plays through once and stops with no indication the loop was dropped.2. The immediate-stop modes don't stop a streamed preset at all.
pcm_note_offimplements "stop now" by seekingphaseto the end of the sample — butrender_pcmresetsphase = 0every block for file presets (it refills a sliding window from the file rather than indexing a table), so the seek is thrown away and the clip keeps playing to end-of-file.PCM_PLAY_STOPis the default mode, so this affected everydisk_sample()note-off, not just theLOOPones.Measured with a 256 ms clip and note-off at 100 ms:
load_sample(memory)disk_sample(streamed)The fix
Note-off: for file presets in
PCM_PLAY_STOP/PCM_LOOP_STOP, stop the osc outright instead of relying on the phase seek that the next block overwrites.Loop modes: degrade at note-on to the nearest non-looping equivalent —
PCM_LOOP/PCM_LOOP_FOREVER→PCM_PLAY,PCM_LOOP_STOP→PCM_PLAY_STOP— with a one-time warning per preset naming the file, so asking for a loop tells you why you didn't get one:Verified the degraded
PCM_LOOPrender is bit-identical to a plainPCM_PLAYof the same clip.Note on doing it properly later
Whole-file looping isn't out of reach if you want it:
pcm_note_onalready rewinds a file preset withfseek+ a header re-parse, so EOF could do the same instead of stopping. What a stream can never honor is theloopstart/loopendmarks — that's the partdocs/synth.mdnow steers toload_samplefor. Happy to build that if you'd prefer it over degrading; I went conservative since the issue framed these as unsupportable.Testing
Two golden-file regression tests —
TestDiskSampleStopsOnNoteOffpins the note-off stop,TestDiskSampleLoopModeDegradespins the degraded loop render.make test: 124 tests pass.🤖 Generated with Claude Code