Timing: backwards playback for plug-in hosts, Patternist, and grid parity with halp - #2163
Conversation
a32d2a9 to
a783c93
Compare
Adversarial review pass: every
|
0acd683 to
46f5832
Compare
a6dcbb0 to
8e17d2a
Compare
6de876c to
365c542
Compare
All three wrapped their whole run() in `if(tk.date > tk.prev_date)`, so a negative speed left the output port untouched and the effect chain went completely silent. exec_state_facade::timings() hands the same buffer span it would going forward, with time-reversed contents, which is what a sequencer is expected to do; only a paused tick has nothing to process. VST3 also ignored the tick offset: it sized the port channels to the tick length and read and wrote them from index 0, so an interval that does not start on a buffer boundary landed at the wrong place with the rest of the buffer missing. Size the ports to the buffer and offset the channel pointers, like the VST2 node already does. While in there, the float path's copy back to the outlet never advanced float_k, so every output channel got bus 0 / channel 0's samples. LV2 still ignores the offset the same way: pre-existing, and equally wrong in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The node asked timings() for [tick_start ; tick_start + d[ and then used index 0, so an interval that does not start on a buffer boundary was read and written at the wrong buffer position. It also resized the input channel to the tick length, truncating a buffer it does not own, and always resized the output with default_init - which hands out whatever the audio buffer pool last left there for the samples a partial tick does not overwrite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Port timestamps are relative to the audio buffer, but the plug-in is handed only [tick_start ; tick_start + samples[ of it and indexes its own buffers from 0. MIDI, note and MIDI2 event times were passed through verbatim, so an interval that does not start on a buffer boundary handed the plug-in an event time past the end of the block it was given - and clap_event_header time is uint32, so a timestamp below tick_start would have wrapped to around four billion. Parameter events were clamped into the block but not rebased either. Rebase by the tick start on the way in, clamp into [0 ; samples[, and undo it on the way out so the plug-in's output events land at the right buffer position. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
token_request::metronome() now reports bar and quarter crossings in both directions, with the sample offset clamped inside the tick, so the node no longer needs to sit out backward ticks. Only a paused tick has nothing to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
std::clamp with lo > hi is undefined, and clamp(t, 0, samples - 1) is that whenever a tick covers no whole sample. Not reachable today: all three hosts return on samples <= 0 before any clamp site. It is hardening against the shape, not a fix for a live bug, and once events are held back rather than dropped these sites see only non-empty blocks.
A tick covering no whole sample returned before dispatching its MIDI, so a note landing in one was dropped. Calling the plug-in with an empty block instead is not an option: CLAP bounds the frame count at [1, INT32_MAX], and JUCE turns a zero-sample VST3 process into a parameter flush that skips the event conversion entirely. Keep the events and send them at offset 0 of the next block that does cover a sample. Under one floor map from model time to samples this is exact rather than approximate: an empty span means its start and end land on the same sample, every date in the tick maps to that sample, and the next non-empty span begins there - so offset 0 of the next block is the sample the event belongs to, across a buffer boundary too. Faust needs nothing: it applies MIDI before its own early return. VST2 is handled separately, in its own commit.
run() returns on a zero-length span before dispatchMidi is ever reached, and the graph clears the inlet each tick, so a note landing in a tick that covers no whole sample was dropped - the same defect already fixed for VST3, CLAP and ysfx, not an exception to it as I claimed there. Keep the events and send them at offset 0 of the next block that does cover a sample. The event count also has to drop when a SysEx message is skipped: it was counted into numEvents but never written into the array, so the plug-in was handed a count larger than the entries behind it.
samplePos, projectTimeSamples and the LV2 frame field all mean where the playhead is on the timeline. They were fed m_processed_frames, which accumulates span lengths - and a span length is never negative, so the value only ever rose. Rewinding ten seconds and playing back told the plug-in twenty had passed and that it was ten seconds further along than it was, and the error never settled because the counter never came down. Use the transport position instead, which follows the timeline in both directions. The counter keeps its own meaning for anything that wants a steady frame count.
Two paths clear in_flight without producing a note-off the downstream can actually see, which leaves the synth holding notes forever while the step sequencer keeps sending note-ons: - the end_discontinuous branch stamps its note-offs at 0. That is outside [tick_start; tick_start + frames[ as soon as the interval does not begin on a buffer boundary, so every consumer that windows on the tick drops them - avendish does exactly this in port_run_preprocess.hpp. in_flight is cleared right after, so the notes are never released again. - all_notes_off() did not clear in_flight at all, so the same notes were offed a second time on the next step. Both now go through release_all(), which stamps at the start of the tick. Also: - channelChanged assigned the raw 1-16 model value to the 0-15 wire channel, off by one against the constructor. Changing the channel while notes were held also sent their note-offs on the new channel, stranding them on the old one: remember the channel notes were started on and release everything before switching. - guard pattern.length <= 0 (modulo by zero) and tk.speed == 0. - the default pattern declared length 4 for 16-step lanes, so three quarters of the built-in rhythm never played. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pattern_node lived in PatternExecutor.cpp, behind the Execution component machinery, so none of it could be reached from a test. Moved to a PatternNode.hpp the executor includes; no behaviour change. The tests drive it tick by tick. Leaving the musical fields of the token at zero makes get_quantification_date() return prev_date, so one tick is one step - which keeps them about note pairing rather than about the quantization arithmetic that token_request owns. Covered, including what the accompanying fix changes: - a step releases what the previous one held, for a single lane, for notes alternating between two lanes, and for a note repeated on one lane - legato holds a note across a step and releases it when the lane rests - a legato step strikes the note if it was not already held - end_discontinuous releases inside the tick rather than at 0 - all_notes_off clears the in-flight set, so the next step does not release the same notes a second time - changing channel releases the held notes on the channel they were struck on - channel conversion and clamping - a zero pattern length and a zero speed do not divide by zero - steps past the end of a shorter lane still release what is held - lanes above the MIDI range (accent, slide) are not struck Also updates the midi node expectation in MidiMessageTest: a tick now comes out in chronological order, so the note-on of the second note precedes the note-off of the first, which ends later. Needs ossia/libossia#914. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
node_process::stop() calls all_notes_off(), which wrote the note-offs straight into the outlet. But that runs outside of a tick, and init_outlet() clears every outlet before the node runs again - so nothing ever read them. Stopping a pattern left the synth holding whatever the last step struck. ossia::nodes::midi does not have the problem because midi_node_process::stop() requests a tick and raises a flag the node consumes from run(), where the outlet is live. Same shape here: all_notes_off() now only raises mustStop, pattern_node_process requests the tick, and the flush happens in run(). The requested token is a default-constructed one, so mustStop has to be handled before the empty-tick early return. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_quantification_date() reports only the first quantification point of a tick, so every other step in it was dropped without a trace. A small division, a large buffer or a high tempo are enough: a quarter note of music at a sixteenth division is four steps, of which one was played. Iterating get_quantification_dates() instead plays them all, each stamped at its own date inside the buffer. The step body moves to play_step() unchanged. Needs ossia/libossia#916. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The file did not compile there at all: ScopedIgnoreSigtrap uses SIGTRAP, which the mingw CRT does not define. Score itself already knows better - Debug.hpp maps DEBUG_BREAK to DebugBreak() on Windows and raise(SIGTRAP) elsewhere - the test only mirrored the POSIX half. The Windows side installs a vectored exception handler that swallows EXCEPTION_BREAKPOINT. It has to step the instruction pointer over the trap by hand: the context is reported at the trapping instruction, not past it, so resuming as-is runs the same int3 forever. Found by running it - the first version spun. Renamed to ScopedIgnoreDebugBreak, since it is no longer about a signal. All 202 assertions pass on Windows now, which also covers the note ordering expectation of the executor case somewhere other than Linux CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The step came from a counter that only ever incremented, so a rewinding timeline still marched the sequence forwards: the grid points the tick crosses were reported in decreasing musical order, and the pattern advanced through them anyway. Step back through the pattern instead when the tick runs backwards, and step before playing rather than after, so going out and back over the same ground crosses the same steps in the opposite order and returns to where it started.
ossia::token_request and halp::tick_musical are documented as behaviourally identical; nothing compared them. Drive both with the token streams time_interval emits and require, strictly: exactly-once delivery of every grid point over consecutive ticks (the regression guard for the boundary double-fire fixed in libossia), exact metronome agreement, and per-point agreement of count, index and frame within one sample. Exact frame equality does not hold today: ossia truncates the musical position to a whole-flick date which its consumers floor into frames, halp floors the position into frames directly, and on ~2e-5 of the points the two land one frame apart. A [!mayfail] case states the ideal and records the current distance from it, so movement in either direction shows up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XGta2LPAedDP3Txvyq8kTW
std::signal(SIGTRAP, ...) does not compile on Windows - there is no SIGTRAP. MidiMessageTest already learnt this and installs a vectored handler instead; these two only ever ignored the signal defensively, so guard the call on the macro existing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XGta2LPAedDP3Txvyq8kTW
…te-off std::signal(SIGTRAP, ...) does not compile on Windows; give the fuzz guard the same vectored-exception treatment MidiMessageTest already has, so the DEBUG_BREAK the malformed buffers trigger stays survivable there too. First time the midi propagation case runs against the current libossia: a note that starts and ends inside one tick emits its note-off in that same tick (midi::run's second stop_finished_notes pass), not in the next one. The expectation of a lone note-on was written against an older libossia; assert the pair and their ordering instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XGta2LPAedDP3Txvyq8kTW
…ition Both derived the sample from the point's date, which is truncated to a whole flick, so flooring it rounded a second time and put the event a sample before the metronome click on the very bar line it was quantized to. Map the point's position through token_request::physical_position instead - the single map the metronome and halp already use. Patternist keeps reconstructing the span start from the tick offset when the producer carried none, and for a null speed, where the model -> sample map is undefined; it prefers the carried span otherwise. The parity test that pinned the ossia/halp divergence as [!mayfail] is now strict and passes: over five signatures, eight rates, four speeds and two buffer lengths the two implementations agree exactly on the count, the index and the frame of every point. A second case holds the distinction in place by measuring that the date-derived frame is still the one that differs, so a consumer switched back to it fails here rather than drifting off the metronome by a sample. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XGta2LPAedDP3Txvyq8kTW
7c08c99 to
fc38ae2
Compare
<windows.h> defines _WIN32_WINNT itself, through <sdkddkver.h>, whenever it is not already set, so leaving it alone does not mean "no minimum" - it means each translation unit gets one depending on whether it reached <windows.h> at all. Headers that branch on it configure themselves differently from one file to the next - Asio reads it to decide BOOST_ASIO_HAS_STD_ATOMIC_WAIT - which is an ODR violation that nothing reports, because both spellings mangle the same. The defaults also disagree between toolchains: the Windows Kits header picks 0x0A00 while mingw-w64 picks _WIN32_WINNT_WS03, so a mingw build has been configuring itself for Server 2003 wherever this was left alone. 0x0A00 is already what MSVC gets by default, so this changes nothing there. libossia enables Asio's version namespace on Windows, to stop its global symbols claiming the names standalone Asio uses - score links both, through the LSL addon. That namespace is an inline namespace, so it is part of libossia's ABI: it is baked into the mangled name of every Asio type its headers expose, and libossia explicitly instantiates resolve_sync_v4 for boost::asio::ip::udp and ::tcp. A translation unit here that disagrees about it names a different specialisation and does not link. It comes with the ossia target too; it is repeated here for the targets that reach Asio without linking ossia. win32.store.build.cmd meant to set the target version and could not: it passed -D_WIN32_WINNT_=0x0A00, with a trailing underscore, which is not a macro. So that job set WINVER to Windows 10 while _WIN32_WINNT quietly stayed at mingw's default - the two macros that are meant to move together, left disagreeing. Both flags go: the definitions below cover every target, and WINVER derives itself. In ScoreConfiguration, before any add_subdirectory, and global rather than PUBLIC on a target: the dependencies that compile Asio are siblings of the libraries consuming it rather than consumers themselves. Not scoped to a compiler either - the divergence is not a property of one. WINVER and NTDDI_VERSION are deliberately not set: sdkddkver.h derives WINVER from _WIN32_WINNT and NTDDI_VERSION from the SDK, in both toolchains, so setting them by hand only creates a way for them to disagree.
cd7a354 to
c840d16
Compare
…n unit All three were set per-target and so reached about three quarters of the build, which means the remaining quarter was compiled against a different configuration of the same headers. BOOST_NO_RTTI is the one that matters. Boost changes what it declares depending on it - typeid use, and with it the layout of the types that carry a std::type_info around - so two translation units that disagree about it disagree about a Boost class while spelling its name identically. libossia sets it PUBLIC on the ossia target, which covered what links ossia and left 583 of 2173 translation units here without it. Not guarded by WIN32; nothing about it is Windows-specific. NOMINMAX and WIN32_LEAN_AND_MEAN decide whether min and max are macros and how much of the Windows API is declared, so which of them a header saw depended on which target it happened to be compiled into. They move to ScoreConfiguration beside the rest, and out of the MSVC branch of the top-level CMakeLists: what they change is what <windows.h> declares, which is a property of the platform and not of the compiler. Spelled without a value, which is what libossia uses and what every consistent translation unit here already had. For C++ only, though. Neither changes a type layout - they change which declarations are visible - so the consistency worth having is between the translation units that share C++ types, and the vendored C we build has been written against the unabridged <windows.h>. libpd's pd~.c reaches malloc and errno through it and stops compiling, on the toolchains that reject undeclared library functions, once the lean header is imposed on it. Counted over compile_commands.json, before and after: BOOST_NO_RTTI 1590 -> 2173 / 2173 NOMINMAX 1691 -> every C++ translation unit, one spelling WIN32_LEAN_AND_MEAN 1523 -> every C++ translation unit What is left disagreeing is vendored C - wiiuse pins _WIN32_WINNT to 0x0501 and Servus' dnssd spells WIN32_LEAN_AND_MEAN as 1 - which cannot take part in a C++ ODR violation, and UNICODE spelled both ways in three vendored C++ libraries, where both spellings are truthy and mean the same thing.
The score changes here call token_request::physical_position, get_quantification_point and the musical position a quantification point now carries, and QuantificationParityTest compiles halp's grid alongside ossia's. None of that exists in the submodule commits master records, so CI built the new code against the old libraries and every job failed. libossia also carries the Windows fixes this branch needs to link on MSVC: the _WIN32_WINNT pin and Asio's version namespace. Repoint both to their commits on libossia master and avendish main once those land, and drop this commit if they are already in by the time this merges.
c840d16 to
2853cce
Compare
#2163 landed with these pinned to the branches it was developed against, which was right while those branches were what carried the changes. They have since been merged, and rebased in the merging, so the commits master records here do not exist on libossia master or avendish main - they exist only on fix/backwards-playback-dataflow and fix/quantification-backwards. That resolves today, because those branches are still there. It stops resolving the moment either is deleted, and a fresh clone of score can no longer check out its submodules. Point them at the same work as it landed upstream: libossia b167d39b7 -> cdeb2619e build: version Boost.Asio's global symbols avendish 948d21d25 -> cb3d29191 halp: one quantification grid, walked once The source is identical either way - the only difference between the branch tips and these is .github/workflows, which upstream changed while the branches were open, so this also stops score pinning both projects to their pre-merge CI.
18 commits, one per defect. Requires ossia/libossia#917; pairs with celtera/avendish#185. Supersedes and closes #2158, every commit of which is carried here.
Plug-in hosts under a rewinding timeline
VST, VST3, LV2 and ysfx all returned early on a backward tick, so a plug-in went silent the moment the timeline ran backwards. They now process; ysfx additionally honours the tick offset instead of assuming the buffer starts at 0.
Events on an empty tick were dropped. A tick can legitimately cover zero frames, and the hosts called the plug-in with a 0-frame block or skipped it, losing whatever MIDI landed in it — a dropped note-off is a note stuck forever. VST/VST3/CLAP/ysfx now hold those events and deliver them at offset 0 of the next non-empty block, deferred-events-first and stably sorted.
Also: an event offset is now clamped only against a non-empty range (
std::clamp(x, 0, -1)is UB — not reachable today, since every host returns before it, but it sat one refactor away), and CLAP's held-back events are delivered once, in order, without dangling.Transport position
samplePos,projectTimeSamplesand the LV2time_frameall mean where the playhead is. They were fedm_processed_frames, which accumulates span lengths — and a span length is never negative, so the value only ever rose. Rewinding ten seconds told the plug-in twenty had passed, and the error never settled. They now get the transport position, which follows the timeline both ways.Patternist
current = (current + 1) % lengthis a free-running counter: rewinding over the same ground played 36, 37 where it must play 39, 38. It now steps back first when the timeline does, so going out and back crosses the same steps;floor(50 / -1) = -50— a negative timestamp that every consumer windowing on[tick_start; tick_start + frames[drops, stranding the note forever.One grid, one sample
Patternist and Looper derived a step's sample from the grid point's date, which libossia truncates to a whole flick — so flooring it rounded a second time and put the event a sample before the metronome click on the very bar line it was quantized to. Both now map the point's musical position through
token_request::physical_position, the single map the metronome and halp already use.QuantificationParityTestdrivesossia::token_requestandhalp::tick_musicalside by side from the token streamstime_intervalactually emits. They are documented as behaviourally identical and nothing compared them; they were not. Over five signatures, eight rates, four speeds and two buffer lengths they now agree exactly on the count, the index and the frame of every point — 1,529,184 assertions. A second case asserts the date-derived frame is still the one that differs, so a consumer switched back to it fails loudly instead of drifting a sample off the metronome.Windows test coverage
StateSerializationTest,ProcessModelTest,DataflowValueTestandMidiMessageTestnever built on Windows at all (SIGTRAP), so they were never running here. Fixed — and one immediately exposed a stale expectation about a same-tick note-off, corrected.Known open, not fixed here
Component.cpp:318-326, code-confirmed, not reproducible in-harness);