File-backed PCM: refuse loop configs, and honor note-off

amy/test.py

@@ -1299,6 +1299,33 @@ def run(self):
     amy_send_at(time=500, osc=0, preset=1024, wave=amy.PCM_MIX, vel=2, note=50)
 
 
+class TestDiskSampleStopsOnNoteOff(AmyTest):
+  """A streamed preset must honor the immediate-stop note-off like an
+  in-memory one does. pcm_note_off stops by seeking phase to the end, but
+  render_pcm resets phase to 0 every block for file presets, so the stop
+  used to be discarded and the clip played on to end-of-file. Note-off at
+  100ms, sample is ~256ms, so a regression shows up as extra audio."""
+
+  def run(self):
+    amy.disk_sample('sounds/partial_sources/CL SHCI A3.wav', preset=1024, midinote=57)
+    amy_send_at(time=50, osc=0, preset=1024, wave=amy.PCM_MIX, vel=2, note=57)
+    amy_send_at(time=100, osc=0, vel=0)
+
+
+class TestDiskSampleLoopModeRefused(AmyTest):
+  """A streamed preset can't loop (no seekable table), so asking for PCM_LOOP
+  is refused outright rather than accepted and quietly not looped. The mode
+  is the half that gets dropped, so the osc keeps the preset and its default
+  PCM_PLAY_STOP -- which means note-off stops it. Output should therefore be
+  identical to TestDiskSampleStopsOnNoteOff, which asks for no mode at all."""
+
+  def run(self):
+    amy.disk_sample('sounds/partial_sources/CL SHCI A3.wav', preset=1024, midinote=57)
+    amy_send_at(time=50, osc=0, preset=1024, wave=amy.PCM_MIX, vel=2, note=57,
+                mode=amy.PCM_LOOP)
+    amy_send_at(time=100, osc=0, vel=0)
+
+
 class TestDiskSampleStereo(AmyTest):
 
   def run(self):

docs/synth.md

@@ -359,6 +359,26 @@ amy.send(wave=amy.PCM, preset=99, vel=1, mode=amy.PCM_LOOP_FOREVER, eg0='0,1,100
 amy.send(vel=0) # note off
 ```
 
+**Looping needs an in-memory sample.** Presets loaded with `disk_sample` stream
+from the file through a small buffer instead of sitting in memory, so there is
+nothing to loop back into and the loop marks can't be honored.
+
+AMY therefore refuses to enter that configuration rather than accepting it and
+quietly not looping. Both halves are checked as they are set — when you change
+`mode`, and when you change `preset` — and the command that would create the
+impossible pair is dropped with a warning naming the file:
+
+```
+amy: osc 0 preset 1024 streams from drums/kick.wav, which cannot loop;
+     ignoring mode=2. Use load_sample() to loop.
+```
+
+The `mode` is the half dropped when you set both at once, so the sample still
+plays (once, honoring note-off) rather than leaving you a loop mode pointing at
+nothing. Setting a streamed preset on an oscillator that is *already* in a loop
+mode drops the `preset` instead, and says so. Load the sample with
+`load_sample` if you need it to loop.
+
 ### Sampler (aka Memory PCM)
 
 You can also load your own samples into AMY memory at runtime by sending PCM data over the wire protocol. Use `load_sample` in `amy.py` as an example:

src/amy.c

@@ -723,8 +723,14 @@ void amy_event_to_deltas_queue(amy_event *e, uint16_t base_osc, struct delta **q
             amy_global.highest_bus = e->bus;
     }
     EVENT_TO_DELTA_I(wave, WAVE)
-    EVENT_TO_DELTA_I(mode, MODE)
+    // PRESET before MODE, and it matters: pcm_loop_config_allowed() refuses
+    // whichever of the pair arrives second and makes the configuration
+    // impossible. A single message asking for a file-backed preset AND a
+    // PCM_LOOP* mode is the common way to hit that, and refusing the *mode*
+    // leaves something useful (the sample, playing once) where refusing the
+    // preset would leave a loop mode pointing at nothing.
     EVENT_TO_DELTA_I(preset, PRESET)
+    EVENT_TO_DELTA_I(mode, MODE)
     EVENT_TO_DELTA_F(midi_note, MIDI_NOTE)
     EVENT_TO_DELTA_COEFS(amp_coefs, AMP)
     EVENT_TO_DELTA_FREQ_COEFS(freq_coefs, FREQ)
@@ -1366,7 +1372,21 @@ void play_delta(struct delta *d) {
             sine_note_on(d->osc, freq_of_logfreq(synth[d->osc]->logfreq_coefs[COEF_CONST]));
         }
     }
-    DELTA_TO_SYNTH_I(MODE, mode)
+    // MODE and PRESET are assigned together, because neither is valid on its
+    // own: a PCM_LOOP* mode on a file-backed preset is a configuration AMY
+    // cannot honor. Whichever of the pair this delta carries is checked
+    // against the value already on the osc, and refused with a warning rather
+    // than accepted and quietly doing something else at note-on. (Only one
+    // param matches per delta, so this reads as two cases but runs as one.)
+    if (d->param == MODE || d->param == PRESET) {
+        bool setting_mode = (d->param == MODE);
+        uint16_t mode = setting_mode ? (uint16_t)d->data.i : synth[d->osc]->mode;
+        uint16_t preset = setting_mode ? synth[d->osc]->preset : (uint16_t)d->data.i;
+        if (pcm_loop_config_allowed(d->osc, mode, preset, setting_mode)) {
+            if (setting_mode) synth[d->osc]->mode = mode;
+            else synth[d->osc]->preset = preset;
+        }
+    }
     DELTA_TO_SYNTH_I(BUS, bus)
     DELTA_TO_SYNTH_F(FEEDBACK, feedback)
     DELTA_TO_SYNTH_F(RATIO, logratio)
@@ -1375,9 +1395,6 @@ void play_delta(struct delta *d) {
     DELTA_TO_SYNTH_I(NOTE_SOURCE_CHANNEL, s_note_source_channel)
     DELTA_TO_SYNTH_I(EG0_TYPE, eg_type[0])
     DELTA_TO_SYNTH_I(EG1_TYPE, eg_type[1])
-    if (d->param == PRESET) {
-        synth[d->osc]->preset = (uint16_t)d->data.i;
-    }
     if (d->param == PORTAMENTO) synth[d->osc]->portamento_alpha = portamento_ms_to_alpha(d->data.i);
     if (d->param == PHASE) {
         // Phase sets the *initial* phase of the osc.

src/amy.h

@@ -1235,6 +1235,12 @@ extern void custom_mod_trigger(uint16_t osc);
 extern int16_t * pcm_load(uint16_t preset_number, uint32_t length, uint32_t samplerate, uint8_t channels, uint8_t midinote, uint32_t loopstart, uint32_t loopend);
 extern const int16_t *pcm_get_sample_ram_for_preset(uint16_t preset_number, uint32_t *length);
 extern int pcm_load_file();
+// Guard against configuring a PCM loop on a file-backed (streamed) preset,
+// which can never loop. Called with the PROPOSED mode and preset as each is
+// set; returns false if that command should be dropped (having warned).
+// mode_is_the_new_part picks which of the two the message blames.
+extern bool pcm_loop_config_allowed(uint16_t osc, uint16_t mode, uint16_t preset_number,
+                                    bool mode_is_the_new_part);
 extern void pcm_unload_preset(uint16_t preset_number);
 extern void pcm_unload_all_presets();
 

src/pcm.c

@@ -160,6 +160,57 @@ static void fclose_if_file(memorypcm_preset_t *preset) {
     }
 }
 
+static bool mode_is_looping(uint16_t mode) {
+    return mode == PCM_LOOP || mode == PCM_LOOP_STOP || mode == PCM_LOOP_FOREVER;
+}
+
+// True if `preset_number` streams from a file rather than sitting in memory.
+// `filename_out`, when non-NULL, receives the file name for the message.
+static bool preset_is_file(uint16_t preset_number, const char **filename_out) {
+    if (AMY_IS_UNSET(preset_number)) return false;
+    memorypcm_preset_t rom_local;
+    memorypcm_preset_t *preset = get_preset_for_preset_number(preset_number, &rom_local);
+    if (preset == NULL || preset->type != AMY_PCM_TYPE_FILE) return false;
+    if (filename_out != NULL) *filename_out = preset->filename;
+    return true;
+}
+
+// A file-backed preset streams through a small sliding buffer rather than
+// sitting in a table we can index freely, so there is nothing to loop back
+// into: render_pcm refills from the file each block and rewinds phase to the
+// top of the fresh buffer. A PCM_LOOP* mode on such a preset can never do
+// what it says.
+//
+// Rather than accept the command and quietly do something else at note-on,
+// refuse it where it is issued -- when the mode changes, and when the preset
+// number changes -- so the configuration never reaches a state it can't
+// honor, and the user hears about it while the offending command is still in
+// front of them. Called with the *proposed* mode and preset; returns false if
+// the command should be dropped.
+//
+// (Whole-file looping *would* be implementable on top of the fseek+re-parse
+// rewind pcm_note_on already does; what a stream can never honor is the
+// loopstart/loopend marks. That's a bigger change than this one.)
+bool pcm_loop_config_allowed(uint16_t osc, uint16_t mode, uint16_t preset_number,
+                             bool mode_is_the_new_part) {
+    // mode means nothing outside PCM, so don't second-guess other waves.
+    if (synth[osc]->wave != PCM) return true;
+    if (!mode_is_looping(mode)) return true;
+    const char *filename = NULL;
+    if (!preset_is_file(preset_number, &filename)) return true;
+    if (mode_is_the_new_part) {
+        fprintf(stderr, "amy: osc %d preset %d streams from %s, which cannot loop; "
+                        "ignoring mode=%d. Use load_sample() to loop.\n",
+                osc, preset_number, filename ? filename : "a file", mode);
+    } else {
+        fprintf(stderr, "amy: preset %d streams from %s, which cannot loop, but osc %d "
+                        "is in mode=%d; ignoring preset=%d. Set a non-loop mode first, "
+                        "or use load_sample().\n",
+                preset_number, filename ? filename : "a file", osc, mode, preset_number);
+    }
+    return false;
+}
+
 void pcm_note_on(uint16_t osc) {
     if(AMY_IS_SET(synth[osc]->preset)) {
         memorypcm_preset_t rom_local;
@@ -205,17 +256,20 @@ void pcm_mod_trigger(uint16_t osc) {
 
 void pcm_note_off(uint16_t osc) {
     if(AMY_IS_SET(synth[osc]->preset)) {
-        uint32_t length = 0;
-        memorypcm_preset_t rom_local;
-        memorypcm_preset_t *preset =
-            get_preset_for_preset_number(synth[osc]->preset, &rom_local);
-        if(preset != NULL) {
-            length = preset->length;
-        }
         if (msynth[osc]->state == PCM_PLAY_STOP
             || msynth[osc]->state == PCM_LOOP_STOP) {
-            // PCM mode where note off causes immediate stop: Set phase to the end
-            synth[osc]->phase = F2P(length / (float)(1 << PCM_INDEX_BITS));
+            // PCM mode where note off causes immediate stop.
+            //
+            // This used to seek phase past the end of the sample and let
+            // render_pcm notice on the next block. That worked only for
+            // in-memory presets: a streamed one refills from the file and
+            // resets phase to 0 every block, so the seek was thrown away and
+            // the clip played on to end-of-file, ignoring note-off entirely.
+            // PCM_PLAY_STOP is the DEFAULT mode, so that hit every
+            // disk_sample() note-off. Stopping the osc says what we mean and
+            // works for both kinds -- and it no longer needs the preset
+            // lookup that the seek needed just to find the sample length.
+            synth[osc]->status = SYNTH_OFF;
         } else if (msynth[osc]->state == PCM_LOOP_FOREVER) {
             // Sending one note-off to a LOOP_FOREVER loop downgrades it to a stoppable loop.
             msynth[osc]->state = PCM_LOOP;