From cfc4ab6ac342e5ff0711b206d24ec564da2feef7 Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Sun, 13 Sep 2026 20:20:30 +0200 Subject: [PATCH 1/5] AUV2-1: emit silence, not the last block, while the CLAP is restarting Render() gated its whole body on _initialized and otherwise returned noErr without touching the output. That is not silence. AUBase only refuses a call with kAudioUnitErr_Uninitialized while the *AU* is uninitialized, and the AU stays initialized right through a plugin requested restart, so the call reaches Render(); once it returns noErr, DoRenderBus copies the output element's cache into the host's buffer regardless. The cache still holds the last block the plugin rendered, so the host got that block again, once per cycle, for as long as onIdle() took to cycle the CLAP - tens of milliseconds for a plugin that reallocates its DSP, and an audible buzz under a playing Logic transport where a latency or oversampling change should have been a dropout. Zero every output element's buffer on that path and raise kAudioUnitRenderAction_OutputIsSilence. The zeroing is the part that matters: AUBase never reads the flag, so a host that ignores it would still play the stale cache. All elements are zeroed, not just the bus being rendered, because RenderBus answers busses 1..n from the cache without calling Render() at all. The comments at the restart site claimed these renders behave "exactly as they do before the AU is initialized". They do not, for the reason above; corrected here and at the two other places that repeated it. Not compiled: macOS-only source, developed on Windows. Uses only calls this file and the process adapter already make - Outputs(), Output(i), PrepareBuffer() and memset - rather than AUBufferList::ZeroBuffer, which has no other use in this wrapper and could not be checked here. --- src/wrapasauv2.cpp | 74 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 67 insertions(+), 7 deletions(-) diff --git a/src/wrapasauv2.cpp b/src/wrapasauv2.cpp index 63f3b8a2..efb2fd68 100644 --- a/src/wrapasauv2.cpp +++ b/src/wrapasauv2.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -1641,6 +1642,59 @@ OSStatus WrapAsAUV2::Render(AudioUnitRenderActionFlags &inFlags, const AudioTime // {} // ); } + else + { + // Nothing renders into the plugin this cycle: the CLAP underneath is down + // (onIdle() is cycling it for a plugin-requested restart, or activateCLAP() + // failed there and it stayed down), or the host handed over action flags + // this wrapper does not process (the assert above records the expectation + // that it never does). Either way the output still has to be written here, + // because AUBase will not do it. DoRender() only refuses a call with + // kAudioUnitErr_Uninitialized while the *AU* is uninitialized, and the AU + // stays initialized right through a restart; once a call gets past that + // check, DoRenderBus() (AUBase.h) copies the output element's cache into + // the host's buffer after every Render() that returns noErr. Returning + // without touching that cache is therefore not silence: it hands the host + // the last block the plugin rendered, once per cycle, for as long as the + // restart takes. With a plugin that reallocates its DSP on restart that is + // tens of milliseconds of one block looping under a playing Logic transport + // -- a buzz where a latency or oversampling change should be a dropout. + // + // Zero the buffers *and* raise kAudioUnitRenderAction_OutputIsSilence, and + // the zeroing is the part that matters. The flag is only a hint to the + // caller -- AUBase itself never reads it, DoRenderBus copies the cache + // regardless -- so a host that ignores it would still play the stale + // cache if the buffers were left alone. That is why ausdk's own + // AUEffectBase::Render ZeroBuffer()s its output whenever it reports silence + // rather than trusting the flag; the flag on top lets a host that does + // honour it skip the mix. inFlags is DoRender's ioActionFlags by reference, + // so the host sees it. + // + // Every output element is zeroed, not just the bus being rendered. On a + // multi-bus unit only the first bus rendered for a timestamp reaches + // Render(); AUBase::RenderBus answers the rest from the element cache + // (NeedsToRender()), so a cache left stale on bus 1 would repeat there even + // with bus 0 clean. PrepareBuffer() then memset is the same pair the + // process adapter uses: claim each element's cache, then silence the + // placeholder busses the plugin never writes (ProcessAdapter::process). + // DoRenderBus moves the result into the host's buffer afterwards exactly + // as it does after a real render. Deliberately not AUBufferList's own + // ZeroBuffer: nothing else in this wrapper calls it, and this file cannot + // be compiled on the machine the fix was written on. + // + // _renderedSinceIdle is deliberately left alone: nothing was carried in + // either direction, so the idle tick's flush still has to make up for it. + const auto numOutputs = Outputs().GetNumberOfElements(); + for (UInt32 i = 0; i < numOutputs; ++i) + { + AudioBufferList &buffers = Output(i).PrepareBuffer(inFrames); + for (UInt32 j = 0; j < buffers.mNumberBuffers; ++j) + { + std::memset(buffers.mBuffers[j].mData, 0, buffers.mBuffers[j].mDataByteSize); + } + } + inFlags |= kAudioUnitRenderAction_OutputIsSilence; + } return noErr; } @@ -1814,8 +1868,13 @@ void WrapAsAUV2::onIdle() // rebuild. Render holds it for its whole body, so once it is acquired no // render is inside the process adapter; clearing _initialized under it // keeps the ones that follow out while the plugin is torn down and stood - // back up. Those renders return without touching the buffers, exactly as - // they do before the AU is initialized. + // back up. Those renders are not rejected the way AUBase rejects one + // before Initialize (kAudioUnitErr_Uninitialized): the AU stays initialized + // throughout, so they reach Render(), which zeroes the output and flags it + // silent. It has to, because AUBase would otherwise hand the host the + // output element's cache -- the last block the plugin rendered -- once per + // cycle for as long as the rebuild takes, an audible buzz under a playing + // Logic transport (see the else branch in Render()). bool wasInitialized; { ClapWrapper::detail::shared::SpinLockGuard processGuard(_processLock); @@ -1827,8 +1886,10 @@ void WrapAsAUV2::onIdle() deactivateCLAP(); // Cannot fail for the format-pair reason Initialize guards against: // the formats have not changed since the last successful activation. - // If it fails anyway, _initialized stays false and renders return - // silence, the same state as before Initialize. + // If it fails anyway, _initialized stays false and every render from + // here on is silent (Render() zeroes the output) until request_process() + // below or the host's next Initialize stands the CLAP back up; parameter + // traffic keeps flowing through the deactivated-state flush meanwhile. if (!activateCLAP()) { LOGINFO("[clap-wrapper] restart: could not reactivate the plugin"); @@ -1848,9 +1909,8 @@ void WrapAsAUV2::onIdle() // activate/start_processing pair, which it drives from AU Initialize() -- // so if the AU is initialized and the CLAP is not running underneath it, // stand it back up. No lock is needed to decide that: activateCLAP() - // publishes _initialized last, and a render that reads it false returns - // without touching the plugin, exactly as it does before the AU is - // initialized at all. + // publishes _initialized last, and a render that reads it false outputs + // silence without touching the plugin (see the else branch in Render()). if (IsInitialized() && !_initialized) { activateCLAP(); From e9363e3ef40e8446572c25dccd18f3a1106b85a3 Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Sun, 13 Sep 2026 20:20:48 +0200 Subject: [PATCH 2/5] VST3-2, VST3-3, LIN-1: preset selector, state-restore race, Linux idle Three fixes in wrapasvst3 and the Linux helper. Committed together because VST3-3 and LIN-1 both change state declared in wrapasvst3.h and neither compiles without the other's half of that header. VST3-2. The CLAP_PARAM_RESCAN_INFO loop skipped only isMidi, and the preset selector is not a CLAP parameter either: createPresetSelector leaves param_index_for_clap_get_info at 0, so get_info handed back CLAP parameter 0 and renamed the host's program-change control after it. A plugin whose macros rename themselves and rescan on every change turned the Cubase program control into "Macro 1". Skip isPreset as well. VST3-3. setState() dropped a queued request and armed the adopt in two separate atomics, and process() does not take _mainThreadLock, so a request could be decided against the unarmed flag and then stored into the just-cleared slot. onIdle() found a request, found _presetIndexInEffect still -1 - a state that never came from the preset list names no preset - and loaded the selector's saved preset over the project that had just been restored. Cubase sends the selector in every process block, so the race was armed on every project load there. Both facts now live in one word: setState() arms and drops in a single store, the audio thread decides and publishes in a single compare-exchange that fails and re-decides if the state landed in between, and onIdle() drains only values >= 0 so it can no longer disarm the adopt on its way past. LIN-1. The stand-in idle thread could park forever with the editor closed, which is the one state it exists to cover. _iRunLoop was a plain pointer read on the helper thread and written on the host's main thread, so on a weakly ordered machine the helper could keep seeing the old non-null value and never resume ticking; and idleSourceChanged() notified without holding _standInLock, so a notify could land in the window after run() evaluated its predicate and before it registered as a waiter, where it is simply lost - and that branch waits unbounded. Made _iRunLoop atomic and moved the notify inside the lock. attachTimers() now notifies only on a null -> run-loop transition: register_timer can reach it from inside on_main_thread with _mainThreadLock already held, which the documented helper-then-plug-object lock order does not allow, and that transition is one register_timer cannot produce. Verified: VST3 target builds clean on Windows (MSVC, C++17) and on Linux (g++ 11.4), the latter being what actually compiles linux.cpp and every #if LIN block. clang-format clean. Reported, not changed: syncParameterValuesFromClap() and getParamValueByString() make the same isMidi-only assumption and call CLAP with the selector's invented id. Both are harmless today because the plugin rejects the unknown id, but they are the same latent class as VST3-2. --- src/detail/os/linux.cpp | 34 ++++++++++ src/wrapasvst3.cpp | 145 +++++++++++++++++++++++++++++----------- src/wrapasvst3.h | 66 +++++++++++++----- 3 files changed, 190 insertions(+), 55 deletions(-) diff --git a/src/detail/os/linux.cpp b/src/detail/os/linux.cpp index 6b26ced9..75e4180c 100644 --- a/src/detail/os/linux.cpp +++ b/src/detail/os/linux.cpp @@ -58,6 +58,13 @@ class LinuxHelper // Recursive, and paired with condition_variable_any, because it is held // across onIdle() and a plug object may come back through attach(), detach() // or idleSourceChanged() from inside its own idle. + // + // Also the lock that guards the wake-up handshake: run() tests its predicate + // and parks under it, so every change to what that predicate reads has to + // reach it through this lock - either made while holding it (_plugs, + // _standInRunning) or, for state a plug object publishes on its own + // (hasOwnIdleSource), followed by a notify_all() issued while holding it. + // A notify for an unlocked change can be lost. \see idleSourceChanged() std::recursive_mutex _standInLock; std::condition_variable_any _standInWakeup; std::thread _standInThread; @@ -230,6 +237,16 @@ void LinuxHelper::run() if (!anyoneWantsTicking()) { LOGDETAIL("clap-wrapper: every attached object has a run loop, pausing the idle thread"); + // Unbounded, and correct only under two conditions that the rest of this + // file has to keep true: the predicate reads state the plug objects + // publish atomically (hasOwnIdleSource), and every change that can turn + // its answer back to "tick me" reaches this thread through _standInLock + // - _plugs and _standInRunning are only written with it held, and a run + // loop going away is announced by idleSourceChanged(), which notifies + // with it held - so that no such change can slip between this predicate + // and this wait. Not a wait_for as a safety net, on purpose: a bounded + // wait would turn a missed wake-up from a dead plug-in into a merely + // late one, and hide the bug that caused it. _standInWakeup.wait(guard, [this] { return !_standInRunning || anyoneWantsTicking(); }); continue; } @@ -270,6 +287,23 @@ void LinuxHelper::detach(IPlugObject *plugobject) void LinuxHelper::idleSourceChanged() { + // The lock is not optional. run() evaluates anyoneWantsTicking() with + // _standInLock held and then parks on the condition variable in one step; + // a notify that is not itself serialised by the same lock can land in the + // window after the predicate came back "everyone has a run loop" and before + // the wait registered, and then it is simply lost. On the plug object's + // side that window is: helper reads _iRunLoop (non-null), host nulls it and + // notifies, helper parks - with an unbounded wait, forever. The plug-in then + // gets no idle at all for as long as its editor stays closed, which is the + // one situation this thread exists to cover. + // + // Taking the lock here means blocking until any idle that is in flight has + // finished, the same as attach() and detach() do. That is acceptable only + // because the caller holds no plug object lock: lock order is + // helper-then-plug-object, and onIdle() only ever try_locks its own, so the + // helper never blocks on a plug object and this can never be the far side + // of a deadlock. \see ClapAsVst3::_mainThreadLock + std::lock_guard guard(_standInLock); _standInWakeup.notify_all(); } diff --git a/src/wrapasvst3.cpp b/src/wrapasvst3.cpp index 0baf2881..9db403b5 100644 --- a/src/wrapasvst3.cpp +++ b/src/wrapasvst3.cpp @@ -325,15 +325,21 @@ tresult PLUGIN_API ClapAsVst3::setState(IBStream *state) // then the better authority rather than the worse one. if (result == kResultOk) { - // Drop a request the audio thread queued from a block that ran before the - // state did. It cannot close the window on its own - process() does not - // take _mainThreadLock, so a request can still be stored after this - but - // the adopt below is what actually decides, and onIdle() re-tests against - // _presetIndexInEffect before it loads anything. - _presetLoadRequest.store(-1); - - // Whatever the host sends next is where its selector stands, not a request - // to go there. \see onRequestPresetLoad(). + // One store does both: it drops a request the audio thread queued from a + // block that ran before the state did, and it arms the adopt, so that + // whatever the host sends next is where its selector stands, not a + // request to go there. \see onRequestPresetLoad(). + // + // It has to be one store. process() does not take _mainThreadLock, so a + // request can be published at any point during this function; when the + // drop and the arm were two separate atomics, a request could slip in + // between them - decided against the unarmed flag, stored into the + // cleared slot - and onIdle() would then load the selector's preset over + // the state just restored, because a state that never came from the + // preset list leaves _presetIndexInEffect at -1 for the re-test there to + // find. onRequestPresetLoad() publishes by compare-exchange against the + // value it decided on, so a request that races this store loses the + // exchange and is re-decided against the armed word. // // Reading the selector parameter here instead does not work, and it is // worth writing down why, because it looks like it should: at this point @@ -345,7 +351,7 @@ tresult PLUGIN_API ClapAsVst3::setState(IBStream *state) // therefore seeds 0 whatever the project said, which both misses every // preset except index 0 and makes index 0 itself unreachable for the life // of the instance. - _adoptNextPresetValue.store(true, std::memory_order_relaxed); + _presetLoadRequest.store(kAdoptNextPresetValue); } return result; @@ -1435,18 +1441,38 @@ void ClapAsVst3::onRequestPresetLoad(size_t presetIndex) // A second pick loads. That is the lesser of the two - the alternative // reloads the saved preset over every restored project, on every host that // streams, every time. - if (_adoptNextPresetValue.exchange(false, std::memory_order_relaxed)) - { - _presetIndexInEffect.store(static_cast(presetIndex), std::memory_order_relaxed); - return; - } + // + // Decide and publish in one compare-exchange, against the word this + // decision was made on. setState() can store kAdoptNextPresetValue at any + // moment during this function; if it does so after the load below and + // before the exchange, the exchange fails, `current` comes back armed, and + // the loop adopts instead of requesting. Two separate steps - test a flag, + // then store a request - left exactly that gap open, and a request that + // fell into it was loaded by onIdle() over the restored state. + // \see _presetLoadRequest. + const auto requested = static_cast(presetIndex); + auto current = _presetLoadRequest.load(); + for (;;) + { + if (current == kAdoptNextPresetValue) + { + if (!_presetLoadRequest.compare_exchange_weak(current, kNoPresetRequest)) continue; + _presetIndexInEffect.store(requested, std::memory_order_relaxed); + return; + } - if (static_cast(presetIndex) == _presetIndexInEffect.load(std::memory_order_relaxed)) - { - return; - } + if (requested == _presetIndexInEffect.load(std::memory_order_relaxed)) + { + return; + } - _presetLoadRequest.store(static_cast(presetIndex)); + // `current` is either kNoPresetRequest or an earlier request from this + // block, which this one coalesces away. A failure updates `current` and + // re-decides: onIdle() may have drained it (then it is kNoPresetRequest + // and the exchange simply goes again) or setState() may have armed it + // (then the branch above takes over). + if (_presetLoadRequest.compare_exchange_weak(current, requested)) return; + } } void ClapAsVst3::preset_loaded(uint32_t locationKind, const char *location, const char *loadKey) @@ -1564,7 +1590,12 @@ void ClapAsVst3::param_rescan(clap_param_rescan_flags flags) for (decltype(len) i = 0; i < len; ++i) { auto p = static_cast(parameters.getParameterByIndex(i)); - if (p->isMidi) continue; + // Neither kind names a CLAP parameter. The preset selector in particular + // is created with param_index_for_clap_get_info left at 0, so asking + // get_info for it hands back CLAP parameter 0 and renames the host's + // program-change control after it - "Macro 1" in Cubase, for a plug-in + // whose macros rename themselves and rescan on every change. + if (p->isMidi || p->isPreset) continue; clap_param_info_t info; if (_plugin->_ext._params->get_info(_plugin->_plugin, p->param_index_for_clap_get_info, &info)) { @@ -1745,9 +1776,9 @@ bool ClapAsVst3::unregister_timer(clap_id timer_id) to.period = 0; to.nexttick = 0; #if LIN - if (to.handler && _iRunLoop) + if (auto *const runLoop = _iRunLoop.load(); to.handler && runLoop) { - _iRunLoop->unregisterTimer(to.handler.get()); + runLoop->unregisterTimer(to.handler.get()); } to.handler.reset(); #endif @@ -1792,7 +1823,18 @@ void ClapAsVst3::onIdle() // A preset the host asked for on the audio thread, and a preset list that // filled in on the crawl thread. Both have to happen here: from_location() // is [main-thread], and so is notifyProgramListChange(). - if (auto requested = _presetLoadRequest.exchange(-1); requested >= 0) + // + // Take only an actual request. A plain exchange(-1) would also take the + // kAdoptNextPresetValue that setState() left there, and disarm the adopt + // before the audio thread ever saw it. The only change the audio thread can + // make to a word that already holds a request is to replace it with a newer + // one; the exchange then fails, `requested` picks up the newer index, and + // the loop takes that instead. + auto requested = _presetLoadRequest.load(); + while (requested >= 0 && !_presetLoadRequest.compare_exchange_weak(requested, kNoPresetRequest)) + { + } + if (requested >= 0) { Clap::PresetEntry entry; // Clamp anyway: stepCount matches the count at creation, but a host may @@ -1969,46 +2011,67 @@ void ClapAsVst3::attachTimers(Steinberg::Linux::IRunLoop *r) { if (r) { - _iRunLoop = r; + const auto previous = _iRunLoop.exchange(r); if (_idleHandler) { - _iRunLoop->unregisterTimer(_idleHandler.get()); + r->unregisterTimer(_idleHandler.get()); } else { _idleHandler = Steinberg::owned(new IdleHandler(this)); } - _iRunLoop->registerTimer(_idleHandler.get(), 30); + r->registerTimer(_idleHandler.get(), 30); for (auto &t : _timersObjects) { if (!t.handler) { t.handler = Steinberg::owned(new TimerHandler(this, t.timer_id)); - _iRunLoop->registerTimer(t.handler.get(), t.period); + r->registerTimer(t.handler.get(), t.period); } } - // the host's own main thread drives the idle from here on - os::idleSourceChanged(); + // The host's own main thread drives the idle from here on - but only say + // so when that is news, i.e. when there was no run loop before. This is + // reached from two places: the view's frame callback in createView(), + // which is where a run loop actually appears and which the host calls + // directly, holding nothing of ours; and register_timer(), which passes + // _iRunLoop back in to hang a new timer on an existing run loop. A + // plug-in may register a timer from inside on_main_thread(), which runs + // in onIdle() with _mainThreadLock held, and os::idleSourceChanged() + // takes the helper's lock - the wrong way round for the documented order + // (\see _mainThreadLock). Notifying only on the transition keeps the + // register_timer path clear of it, because a same-value exchange is not + // a transition. It also is not one the helper needs to hear about: it + // re-tests anyoneWantsTicking() every tick and parks on its own once + // this object answers hasOwnIdleSource(); only the other direction, a run + // loop going away, needs a wake-up. + if (previous == nullptr) + { + os::idleSourceChanged(); + } } } void ClapAsVst3::detachTimers(Steinberg::Linux::IRunLoop *r) { - if (r && r == _iRunLoop) + // Read once. The helper thread only reads _iRunLoop and never writes it, so + // a local copy cannot go stale under us; it is only atomic so that the + // helper's read is defined. + auto *const runLoop = _iRunLoop.load(); + if (r && r == runLoop) { if (_idleHandler) { - _iRunLoop->unregisterTimer(_idleHandler.get()); + runLoop->unregisterTimer(_idleHandler.get()); _idleHandler.reset(); } for (auto &t : _timersObjects) { if (t.handler) { - _iRunLoop->unregisterTimer(t.handler.get()); + runLoop->unregisterTimer(t.handler.get()); t.handler.reset(); } } @@ -2049,9 +2112,9 @@ bool ClapAsVst3::unregister_fd(int fd) if (it->fd == fd) { res = true; - if (_iRunLoop && it->handler) + if (auto *const runLoop = _iRunLoop.load(); runLoop && it->handler) { - _iRunLoop->unregisterEventHandler(it->handler.get()); + runLoop->unregisterEventHandler(it->handler.get()); } it->handler.reset(); it = _posixFDObjects.erase(it); @@ -2086,14 +2149,17 @@ void ClapAsVst3::attachPosixFD(Steinberg::Linux::IRunLoop *r) { if (r) { - _iRunLoop = r; + // No idleSourceChanged() here: the frame callback in createView() calls + // attachTimers() first, which announces the run loop, and register_fd() + // passes the run loop already in place. \see attachTimers() + _iRunLoop.store(r); for (auto &p : _posixFDObjects) { if (!p.handler) { p.handler = Steinberg::owned(new FDHandler(this, p.fd, p.flags)); - _iRunLoop->registerEventHandler(p.handler.get(), p.fd); + r->registerEventHandler(p.handler.get(), p.fd); } } } @@ -2101,13 +2167,14 @@ void ClapAsVst3::attachPosixFD(Steinberg::Linux::IRunLoop *r) void ClapAsVst3::detachPosixFD(Steinberg::Linux::IRunLoop *r) { - if (r && r == _iRunLoop) + auto *const runLoop = _iRunLoop.load(); + if (r && r == runLoop) { for (auto &p : _posixFDObjects) { if (p.handler) { - _iRunLoop->unregisterEventHandler(p.handler.get()); + runLoop->unregisterEventHandler(p.handler.get()); p.handler.reset(); } } diff --git a/src/wrapasvst3.h b/src/wrapasvst3.h index 5d809c73..e8018cbb 100644 --- a/src/wrapasvst3.h +++ b/src/wrapasvst3.h @@ -370,9 +370,14 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, #if LIN // While an editor is open the host's run loop drives onIdle() on the real // main thread and the Linux helper thread stands down. \see attachTimers() + // + // Read on the helper thread, written on the host's main thread, which is why + // _iRunLoop is atomic: a plain pointer read here is a data race, and on a + // weakly ordered machine (arm64) the helper could keep seeing the old + // non-null value after the editor closed and never resume ticking. bool hasOwnIdleSource() const override { - return _iRunLoop != nullptr; + return _iRunLoop.load() != nullptr; } #endif @@ -486,7 +491,42 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, // Set from the audio thread by onRequestPresetLoad(), drained in onIdle(). // Coalescing is correct: three program changes in one block should load the // last preset, not three. - std::atomic _presetLoadRequest{-1}; + // + // One word carries two facts, on purpose: + // + // >= 0 a preset the host asked for, not yet loaded + // kNoPresetRequest nothing pending + // kAdoptNextPresetValue armed by a successful setState(): the next + // selector value the host sends is where its + // selector stands, not a preset to load. A + // restored project hands back the value it was + // saved with, and obeying it reloads that preset + // over the state that has just been restored. + // + // The two used to be separate atomics, a request and an "adopt next" flag, + // and that left a hole exactly the width of the audio thread's decision: + // process() could test the flag (not armed), setState() could then drop the + // request and arm the flag, and process() could go on to publish its + // request into the just-cleared slot. onIdle() found a request, found + // _presetIndexInEffect still at -1 (a state that never came from the preset + // list names none), and loaded the selector's preset over the project. + // Cubase sends the selector in every process block, so on that host the + // race was armed on every project load. With both facts in one word, + // setState() arms and drops in a single store, and the audio thread decides + // and publishes in a single compare-exchange, which fails and retries if + // the state landed in between (\see onRequestPresetLoad). + // + // Armed by setState() and nowhere else. A fresh instance has no state to + // protect, so its first program change is a real one and is obeyed - which + // is also why the armed state cannot be inferred from _presetIndexInEffect + // being -1, a condition the two cases share. + // + // Written on the main thread (setState, onIdle) and on the audio thread + // (onRequestPresetLoad); the two main-thread writers are serialised by + // _mainThreadLock, the audio thread only ever changes it by compare-exchange. + static constexpr int64_t kNoPresetRequest = -1; + static constexpr int64_t kAdoptNextPresetValue = -2; + std::atomic _presetLoadRequest{kNoPresetRequest}; // The selector value already in effect: the index onIdle() last acted on, // or the one preset_loaded() resolved. A parameter change is only a request // when it names something else. @@ -502,19 +542,6 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, // Written on the main thread (onIdle, preset_loaded), read on the audio // thread (onRequestPresetLoad). std::atomic _presetIndexInEffect{-1}; - // Armed by a successful setState(): the next selector value the host sends - // is where its selector stands, not a preset to load. A restored project - // hands back the value it was saved with, and obeying it reloads that preset - // over the state that has just been restored. - // - // Armed there and nowhere else. A fresh instance has no state to protect, so - // its first program change is a real one and is obeyed - which is also why - // this cannot be inferred from _presetIndexInEffect being -1, a condition the - // two cases share. - // - // Written on the main thread (setState), cleared on the audio thread - // (onRequestPresetLoad). - std::atomic _adoptNextPresetValue{false}; // Set when the crawl finishes; onIdle() turns it into the host notification, // because notifyProgramListChange() is not for a background thread. std::atomic _presetListChanged{false}; @@ -544,7 +571,14 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, void attachTimers(Steinberg::Linux::IRunLoop *); void detachTimers(Steinberg::Linux::IRunLoop *); - Steinberg::Linux::IRunLoop *_iRunLoop{nullptr}; + // The host's run loop, for as long as an editor is open, or null. Written on + // the host's main thread (attachTimers/attachPosixFD from the view's frame, + // nulled in createView's callbacks); read from the helper thread through + // hasOwnIdleSource(). Every write that takes it to null is followed by + // os::idleSourceChanged(), which is what wakes the parked helper - the + // atomic alone makes the read well-defined, the notify is what makes it + // timely. \see os::LinuxHelper::run() + std::atomic _iRunLoop{nullptr}; #endif #if LIN From 8c3f02dc4cd443616049888f703f211b67fc30fa Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:10:12 +0200 Subject: [PATCH 3/5] Trim comment noise in the VST3/Linux/AUv2 0.16 fixes Comment-only change: the explanatory blocks are cut to the non-obvious why. --- src/detail/os/linux.cpp | 38 +++----------------- src/wrapasauv2.cpp | 65 +++++++--------------------------- src/wrapasvst3.cpp | 78 ++++++++++------------------------------- src/wrapasvst3.h | 46 +++++------------------- 4 files changed, 45 insertions(+), 182 deletions(-) diff --git a/src/detail/os/linux.cpp b/src/detail/os/linux.cpp index 75e4180c..a056e6ea 100644 --- a/src/detail/os/linux.cpp +++ b/src/detail/os/linux.cpp @@ -58,13 +58,6 @@ class LinuxHelper // Recursive, and paired with condition_variable_any, because it is held // across onIdle() and a plug object may come back through attach(), detach() // or idleSourceChanged() from inside its own idle. - // - // Also the lock that guards the wake-up handshake: run() tests its predicate - // and parks under it, so every change to what that predicate reads has to - // reach it through this lock - either made while holding it (_plugs, - // _standInRunning) or, for state a plug object publishes on its own - // (hasOwnIdleSource), followed by a notify_all() issued while holding it. - // A notify for an unlocked change can be lost. \see idleSourceChanged() std::recursive_mutex _standInLock; std::condition_variable_any _standInWakeup; std::thread _standInThread; @@ -237,16 +230,8 @@ void LinuxHelper::run() if (!anyoneWantsTicking()) { LOGDETAIL("clap-wrapper: every attached object has a run loop, pausing the idle thread"); - // Unbounded, and correct only under two conditions that the rest of this - // file has to keep true: the predicate reads state the plug objects - // publish atomically (hasOwnIdleSource), and every change that can turn - // its answer back to "tick me" reaches this thread through _standInLock - // - _plugs and _standInRunning are only written with it held, and a run - // loop going away is announced by idleSourceChanged(), which notifies - // with it held - so that no such change can slip between this predicate - // and this wait. Not a wait_for as a safety net, on purpose: a bounded - // wait would turn a missed wake-up from a dead plug-in into a merely - // late one, and hide the bug that caused it. + // Unbounded on purpose: every change that can turn the predicate back to + // "tick me" reaches this thread under _standInLock, so none can be lost. _standInWakeup.wait(guard, [this] { return !_standInRunning || anyoneWantsTicking(); }); continue; } @@ -287,22 +272,9 @@ void LinuxHelper::detach(IPlugObject *plugobject) void LinuxHelper::idleSourceChanged() { - // The lock is not optional. run() evaluates anyoneWantsTicking() with - // _standInLock held and then parks on the condition variable in one step; - // a notify that is not itself serialised by the same lock can land in the - // window after the predicate came back "everyone has a run loop" and before - // the wait registered, and then it is simply lost. On the plug object's - // side that window is: helper reads _iRunLoop (non-null), host nulls it and - // notifies, helper parks - with an unbounded wait, forever. The plug-in then - // gets no idle at all for as long as its editor stays closed, which is the - // one situation this thread exists to cover. - // - // Taking the lock here means blocking until any idle that is in flight has - // finished, the same as attach() and detach() do. That is acceptable only - // because the caller holds no plug object lock: lock order is - // helper-then-plug-object, and onIdle() only ever try_locks its own, so the - // helper never blocks on a plug object and this can never be the far side - // of a deadlock. \see ClapAsVst3::_mainThreadLock + // The lock is not optional: run() tests its predicate and parks in one step, + // so an unlocked notify can be lost, leaving the helper parked forever. Safe + // to block on: lock order is helper-then-plug-object, onIdle() only try_locks. std::lock_guard guard(_standInLock); _standInWakeup.notify_all(); } diff --git a/src/wrapasauv2.cpp b/src/wrapasauv2.cpp index efb2fd68..0c715ebc 100644 --- a/src/wrapasauv2.cpp +++ b/src/wrapasauv2.cpp @@ -1644,46 +1644,14 @@ OSStatus WrapAsAUV2::Render(AudioUnitRenderActionFlags &inFlags, const AudioTime } else { - // Nothing renders into the plugin this cycle: the CLAP underneath is down - // (onIdle() is cycling it for a plugin-requested restart, or activateCLAP() - // failed there and it stayed down), or the host handed over action flags - // this wrapper does not process (the assert above records the expectation - // that it never does). Either way the output still has to be written here, - // because AUBase will not do it. DoRender() only refuses a call with - // kAudioUnitErr_Uninitialized while the *AU* is uninitialized, and the AU - // stays initialized right through a restart; once a call gets past that - // check, DoRenderBus() (AUBase.h) copies the output element's cache into - // the host's buffer after every Render() that returns noErr. Returning - // without touching that cache is therefore not silence: it hands the host - // the last block the plugin rendered, once per cycle, for as long as the - // restart takes. With a plugin that reallocates its DSP on restart that is - // tens of milliseconds of one block looping under a playing Logic transport - // -- a buzz where a latency or oversampling change should be a dropout. - // - // Zero the buffers *and* raise kAudioUnitRenderAction_OutputIsSilence, and - // the zeroing is the part that matters. The flag is only a hint to the - // caller -- AUBase itself never reads it, DoRenderBus copies the cache - // regardless -- so a host that ignores it would still play the stale - // cache if the buffers were left alone. That is why ausdk's own - // AUEffectBase::Render ZeroBuffer()s its output whenever it reports silence - // rather than trusting the flag; the flag on top lets a host that does - // honour it skip the mix. inFlags is DoRender's ioActionFlags by reference, - // so the host sees it. - // - // Every output element is zeroed, not just the bus being rendered. On a - // multi-bus unit only the first bus rendered for a timestamp reaches - // Render(); AUBase::RenderBus answers the rest from the element cache - // (NeedsToRender()), so a cache left stale on bus 1 would repeat there even - // with bus 0 clean. PrepareBuffer() then memset is the same pair the - // process adapter uses: claim each element's cache, then silence the - // placeholder busses the plugin never writes (ProcessAdapter::process). - // DoRenderBus moves the result into the host's buffer afterwards exactly - // as it does after a real render. Deliberately not AUBufferList's own - // ZeroBuffer: nothing else in this wrapper calls it, and this file cannot - // be compiled on the machine the fix was written on. - // - // _renderedSinceIdle is deliberately left alone: nothing was carried in - // either direction, so the idle tick's flush still has to make up for it. + // Nothing rendered this cycle (the CLAP is down while onIdle() cycles it + // for a restart), and AUBase will not silence anything for us: the AU stays + // initialized throughout, so DoRenderBus copies the output element's cache + // into the host's buffer after every noErr Render and returning without + // writing replays the last block. Zero every output element, not just the + // bus being rendered - RenderBus answers the others from their caches - and + // the silence flag on top is only a hint. _renderedSinceIdle is left alone: + // the idle tick's flush still has to make up for this block. const auto numOutputs = Outputs().GetNumberOfElements(); for (UInt32 i = 0; i < numOutputs; ++i) { @@ -1868,13 +1836,8 @@ void WrapAsAUV2::onIdle() // rebuild. Render holds it for its whole body, so once it is acquired no // render is inside the process adapter; clearing _initialized under it // keeps the ones that follow out while the plugin is torn down and stood - // back up. Those renders are not rejected the way AUBase rejects one - // before Initialize (kAudioUnitErr_Uninitialized): the AU stays initialized - // throughout, so they reach Render(), which zeroes the output and flags it - // silent. It has to, because AUBase would otherwise hand the host the - // output element's cache -- the last block the plugin rendered -- once per - // cycle for as long as the rebuild takes, an audible buzz under a playing - // Logic transport (see the else branch in Render()). + // back up. The AU stays initialized throughout, so those renders still + // reach Render(), which zeroes the output (see the else branch there). bool wasInitialized; { ClapWrapper::detail::shared::SpinLockGuard processGuard(_processLock); @@ -1886,10 +1849,8 @@ void WrapAsAUV2::onIdle() deactivateCLAP(); // Cannot fail for the format-pair reason Initialize guards against: // the formats have not changed since the last successful activation. - // If it fails anyway, _initialized stays false and every render from - // here on is silent (Render() zeroes the output) until request_process() - // below or the host's next Initialize stands the CLAP back up; parameter - // traffic keeps flowing through the deactivated-state flush meanwhile. + // If it fails anyway, _initialized stays false and renders are silent + // until request_process() below or the host's next Initialize recovers. if (!activateCLAP()) { LOGINFO("[clap-wrapper] restart: could not reactivate the plugin"); @@ -1910,7 +1871,7 @@ void WrapAsAUV2::onIdle() // so if the AU is initialized and the CLAP is not running underneath it, // stand it back up. No lock is needed to decide that: activateCLAP() // publishes _initialized last, and a render that reads it false outputs - // silence without touching the plugin (see the else branch in Render()). + // silence without touching the plugin. if (IsInitialized() && !_initialized) { activateCLAP(); diff --git a/src/wrapasvst3.cpp b/src/wrapasvst3.cpp index 9db403b5..733448ed 100644 --- a/src/wrapasvst3.cpp +++ b/src/wrapasvst3.cpp @@ -325,21 +325,10 @@ tresult PLUGIN_API ClapAsVst3::setState(IBStream *state) // then the better authority rather than the worse one. if (result == kResultOk) { - // One store does both: it drops a request the audio thread queued from a - // block that ran before the state did, and it arms the adopt, so that - // whatever the host sends next is where its selector stands, not a - // request to go there. \see onRequestPresetLoad(). - // - // It has to be one store. process() does not take _mainThreadLock, so a - // request can be published at any point during this function; when the - // drop and the arm were two separate atomics, a request could slip in - // between them - decided against the unarmed flag, stored into the - // cleared slot - and onIdle() would then load the selector's preset over - // the state just restored, because a state that never came from the - // preset list leaves _presetIndexInEffect at -1 for the re-test there to - // find. onRequestPresetLoad() publishes by compare-exchange against the - // value it decided on, so a request that races this store loses the - // exchange and is re-decided against the armed word. + // One store drops a request the audio thread queued before the state and + // arms the adopt. It has to be one store: process() takes no lock, so a + // request published between a separate drop and arm would survive both and + // be loaded over the state just restored. \see onRequestPresetLoad(). // // Reading the selector parameter here instead does not work, and it is // worth writing down why, because it looks like it should: at this point @@ -1442,14 +1431,8 @@ void ClapAsVst3::onRequestPresetLoad(size_t presetIndex) // reloads the saved preset over every restored project, on every host that // streams, every time. // - // Decide and publish in one compare-exchange, against the word this - // decision was made on. setState() can store kAdoptNextPresetValue at any - // moment during this function; if it does so after the load below and - // before the exchange, the exchange fails, `current` comes back armed, and - // the loop adopts instead of requesting. Two separate steps - test a flag, - // then store a request - left exactly that gap open, and a request that - // fell into it was loaded by onIdle() over the restored state. - // \see _presetLoadRequest. + // Decide and publish in one compare-exchange: setState() can arm at any + // moment here, and a failed exchange re-decides against the armed value. const auto requested = static_cast(presetIndex); auto current = _presetLoadRequest.load(); for (;;) @@ -1466,11 +1449,7 @@ void ClapAsVst3::onRequestPresetLoad(size_t presetIndex) return; } - // `current` is either kNoPresetRequest or an earlier request from this - // block, which this one coalesces away. A failure updates `current` and - // re-decides: onIdle() may have drained it (then it is kNoPresetRequest - // and the exchange simply goes again) or setState() may have armed it - // (then the branch above takes over). + // `current` is kNoPresetRequest or an earlier request this one coalesces. if (_presetLoadRequest.compare_exchange_weak(current, requested)) return; } } @@ -1590,11 +1569,8 @@ void ClapAsVst3::param_rescan(clap_param_rescan_flags flags) for (decltype(len) i = 0; i < len; ++i) { auto p = static_cast(parameters.getParameterByIndex(i)); - // Neither kind names a CLAP parameter. The preset selector in particular - // is created with param_index_for_clap_get_info left at 0, so asking - // get_info for it hands back CLAP parameter 0 and renames the host's - // program-change control after it - "Macro 1" in Cubase, for a plug-in - // whose macros rename themselves and rescan on every change. + // Neither names a CLAP parameter: the selector's index is left at 0, so + // get_info would rename the host's program control after parameter 0. if (p->isMidi || p->isPreset) continue; clap_param_info_t info; if (_plugin->_ext._params->get_info(_plugin->_plugin, p->param_index_for_clap_get_info, &info)) @@ -1824,12 +1800,8 @@ void ClapAsVst3::onIdle() // filled in on the crawl thread. Both have to happen here: from_location() // is [main-thread], and so is notifyProgramListChange(). // - // Take only an actual request. A plain exchange(-1) would also take the - // kAdoptNextPresetValue that setState() left there, and disarm the adopt - // before the audio thread ever saw it. The only change the audio thread can - // make to a word that already holds a request is to replace it with a newer - // one; the exchange then fails, `requested` picks up the newer index, and - // the loop takes that instead. + // Take only an actual request: a plain exchange would also take the + // kAdoptNextPresetValue setState() left there and disarm the adopt. auto requested = _presetLoadRequest.load(); while (requested >= 0 && !_presetLoadRequest.compare_exchange_weak(requested, kNoPresetRequest)) { @@ -2032,21 +2004,10 @@ void ClapAsVst3::attachTimers(Steinberg::Linux::IRunLoop *r) } } - // The host's own main thread drives the idle from here on - but only say - // so when that is news, i.e. when there was no run loop before. This is - // reached from two places: the view's frame callback in createView(), - // which is where a run loop actually appears and which the host calls - // directly, holding nothing of ours; and register_timer(), which passes - // _iRunLoop back in to hang a new timer on an existing run loop. A - // plug-in may register a timer from inside on_main_thread(), which runs - // in onIdle() with _mainThreadLock held, and os::idleSourceChanged() - // takes the helper's lock - the wrong way round for the documented order - // (\see _mainThreadLock). Notifying only on the transition keeps the - // register_timer path clear of it, because a same-value exchange is not - // a transition. It also is not one the helper needs to hear about: it - // re-tests anyoneWantsTicking() every tick and parks on its own once - // this object answers hasOwnIdleSource(); only the other direction, a run - // loop going away, needs a wake-up. + // The host's main thread drives the idle from here on, but announce it + // only on the transition: register_timer() also lands here, and it may run + // from on_main_thread() with _mainThreadLock held, where taking the + // helper's lock would invert the documented order. if (previous == nullptr) { os::idleSourceChanged(); @@ -2056,9 +2017,7 @@ void ClapAsVst3::attachTimers(Steinberg::Linux::IRunLoop *r) void ClapAsVst3::detachTimers(Steinberg::Linux::IRunLoop *r) { - // Read once. The helper thread only reads _iRunLoop and never writes it, so - // a local copy cannot go stale under us; it is only atomic so that the - // helper's read is defined. + // Read once: the helper thread only reads _iRunLoop, never writes it. auto *const runLoop = _iRunLoop.load(); if (r && r == runLoop) { @@ -2149,9 +2108,8 @@ void ClapAsVst3::attachPosixFD(Steinberg::Linux::IRunLoop *r) { if (r) { - // No idleSourceChanged() here: the frame callback in createView() calls - // attachTimers() first, which announces the run loop, and register_fd() - // passes the run loop already in place. \see attachTimers() + // No idleSourceChanged() here: the frame callback calls attachTimers() + // first, which announces the run loop. \see attachTimers() _iRunLoop.store(r); for (auto &p : _posixFDObjects) diff --git a/src/wrapasvst3.h b/src/wrapasvst3.h index e8018cbb..46edccbe 100644 --- a/src/wrapasvst3.h +++ b/src/wrapasvst3.h @@ -371,10 +371,7 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, // While an editor is open the host's run loop drives onIdle() on the real // main thread and the Linux helper thread stands down. \see attachTimers() // - // Read on the helper thread, written on the host's main thread, which is why - // _iRunLoop is atomic: a plain pointer read here is a data race, and on a - // weakly ordered machine (arm64) the helper could keep seeing the old - // non-null value after the editor closed and never resume ticking. + // Called on the helper thread, which is why _iRunLoop is atomic. bool hasOwnIdleSource() const override { return _iRunLoop.load() != nullptr; @@ -497,33 +494,13 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, // >= 0 a preset the host asked for, not yet loaded // kNoPresetRequest nothing pending // kAdoptNextPresetValue armed by a successful setState(): the next - // selector value the host sends is where its - // selector stands, not a preset to load. A - // restored project hands back the value it was - // saved with, and obeying it reloads that preset - // over the state that has just been restored. + // selector value the host sends is where the + // host's selector stands, not a preset to load // - // The two used to be separate atomics, a request and an "adopt next" flag, - // and that left a hole exactly the width of the audio thread's decision: - // process() could test the flag (not armed), setState() could then drop the - // request and arm the flag, and process() could go on to publish its - // request into the just-cleared slot. onIdle() found a request, found - // _presetIndexInEffect still at -1 (a state that never came from the preset - // list names none), and loaded the selector's preset over the project. - // Cubase sends the selector in every process block, so on that host the - // race was armed on every project load. With both facts in one word, - // setState() arms and drops in a single store, and the audio thread decides - // and publishes in a single compare-exchange, which fails and retries if - // the state landed in between (\see onRequestPresetLoad). - // - // Armed by setState() and nowhere else. A fresh instance has no state to - // protect, so its first program change is a real one and is obeyed - which - // is also why the armed state cannot be inferred from _presetIndexInEffect - // being -1, a condition the two cases share. - // - // Written on the main thread (setState, onIdle) and on the audio thread - // (onRequestPresetLoad); the two main-thread writers are serialised by - // _mainThreadLock, the audio thread only ever changes it by compare-exchange. + // One word is what closes the race: setState() arms and drops in a single + // store, the audio thread decides and publishes in a single compare-exchange + // (\see onRequestPresetLoad). Armed by setState() and nowhere else, so a + // fresh instance's first program change is a real one. static constexpr int64_t kNoPresetRequest = -1; static constexpr int64_t kAdoptNextPresetValue = -2; std::atomic _presetLoadRequest{kNoPresetRequest}; @@ -571,13 +548,8 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, void attachTimers(Steinberg::Linux::IRunLoop *); void detachTimers(Steinberg::Linux::IRunLoop *); - // The host's run loop, for as long as an editor is open, or null. Written on - // the host's main thread (attachTimers/attachPosixFD from the view's frame, - // nulled in createView's callbacks); read from the helper thread through - // hasOwnIdleSource(). Every write that takes it to null is followed by - // os::idleSourceChanged(), which is what wakes the parked helper - the - // atomic alone makes the read well-defined, the notify is what makes it - // timely. \see os::LinuxHelper::run() + // Written on the host's main thread, read from the helper thread; every + // write to null is followed by os::idleSourceChanged() to wake the helper. std::atomic _iRunLoop{nullptr}; #endif From bbe7db305e412a803a46ec2d8316a049a524d86b Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Sun, 13 Sep 2026 23:29:03 +0200 Subject: [PATCH 4/5] PD-2, PD-4, PD-6, VST3-1: size the preset selector once, grow it in place Four findings with one root cause, and they interlocked: the obvious fix for one armed another. VST3-1's teardown branch needs _presetListChanged set while _presetParamId is still kNoParamId, and the listener that sets that flag was only registered after the id was assigned - so PD-2, the early return that skipped registering it, was the only reason VST3-1 was not firing. Moving that registration up, which is what PD-2 asks for on its own, would have made a use-after-free live and frequent. So the teardown goes instead of being worked around. PD-2. setupPresets() no longer returns before registering the completion listener. The selector is created whatever the crawl has found so far - hidden, stepCount 0, when that is nothing - and the listener is registered straight after, so a registered listener always has a selector to grow. A plugin whose folder crawl outlived initialize() used to get no program list for the life of that instance. PD-4. The selector's stepCount was frozen at creation while getProgramListInfo reported the live index size, so a host listed 2100 programs against a 99-step parameter and picking #1500 clamped to 1.0 and loaded preset 99 - silently, and not the preset chosen. One number now: Vst3Parameter::presetCount(), stepCount+1 or 0 while hidden, read by getProgramListInfo, getProgramName, the onIdle clamp and preset_loaded. The live size is never published, and only a complete crawl is published at all. PD-6. waitUntilComplete(1000) is gone from initialize(). VST3-1. onIdle no longer calls setupParameters(), so parameters.removeAll() can no longer run while the component is active and destroy Parameter objects the process adapter is dereferencing on the audio thread. The completion path instead writes stepCount, min/max_value and the hidden flag on the existing parameter under the same spin lock process() takes, then announces kParamTitlesChanged - which the SDK defines as covering "titles, default values, stepCount or flags" - alongside notifyProgramListChange. param_rescan already rewrites getInfo().title in place and announces it the same way. setupParameters() is now reached only from initialize() and from param_rescan(RESCAN_ALL), which CLAP forbids while active. Two things the design did not anticipate, both found while implementing: A no-op guard. The index is shared per module, so from the second instance on the crawl is already complete at initialize() and the listener fires straight back with nothing to say. Restarting the component for that on every instantiation is not free on hosts that rebuild parameter views. Selector catch-up. A preset the plugin loads from its own UI while the crawl is still running has no slot yet, so the selector stays at 0. On a host that streams the selector every block that 0 becomes a load request the moment the list grows, putting preset 0 over the user's choice. After the grow, onIdle moves the selector to _presetIndexInEffect when it is now in range. The bracketed edit tail was factored out of preset_loaded and shared. asVst3Value() returns 0 for a zero-width range rather than dividing. That also stops a CLAP parameter declaring min == max from handing the host a NaN. Verified: builds clean (MSVC, C++17), clang-format clean. Not verified in a host, and it needs to be - see the PR for what to check. --- src/detail/vst3/parameter.cpp | 25 ++++- src/detail/vst3/parameter.h | 34 +++++- src/wrapasvst3.cpp | 188 +++++++++++++++++++++++++++------- src/wrapasvst3.h | 16 ++- 4 files changed, 221 insertions(+), 42 deletions(-) diff --git a/src/detail/vst3/parameter.cpp b/src/detail/vst3/parameter.cpp index e21139e0..e6270b40 100644 --- a/src/detail/vst3/parameter.cpp +++ b/src/detail/vst3/parameter.cpp @@ -198,12 +198,33 @@ Vst3Parameter *Vst3Parameter::createPresetSelector(Steinberg::Vst::ParamID id, i // load rebuilds a plugin's state, which is not something to draw a curve // through. v.flags = Vst::ParameterInfo::kIsProgramChange | Vst::ParameterInfo::kIsList; - v.stepCount = presetCount > 1 ? presetCount - 1 : 0; + v.stepCount = 0; auto *p = new Vst3Parameter(v, 0, 0, 0); p->isMidi = false; p->isPreset = true; p->min_value = 0; - p->max_value = v.stepCount; + p->max_value = 0; + // One place decides what a count of N looks like on the parameter, whether + // it is the count at creation or the one the crawl delivers later. + p->resizePresetSelector(presetCount); return p; } + +void Vst3Parameter::resizePresetSelector(int32_t presetCount) +{ + auto &v = getInfo(); + // A host reads stepCount+1 as the number of programs (the SDK's own preset + // sample sets kNumPrograms-1), so one preset is stepCount 0 - which is also + // what an empty list is. The flag is what tells the two apart. + v.stepCount = presetCount > 1 ? presetCount - 1 : 0; + if (presetCount > 0) + v.flags &= ~Vst::ParameterInfo::kIsHidden; + else + v.flags |= Vst::ParameterInfo::kIsHidden; + // The plain range asClapValue()/asVst3Value() convert against: index 0 to + // the last index. With no presets it collapses to 0..0, which asVst3Value() + // guards against dividing by. + min_value = 0; + max_value = v.stepCount; +} diff --git a/src/detail/vst3/parameter.h b/src/detail/vst3/parameter.h index c55f14c5..fb4582c3 100644 --- a/src/detail/vst3/parameter.h +++ b/src/detail/vst3/parameter.h @@ -72,7 +72,14 @@ class Vst3Parameter : public Steinberg::Vst::Parameter { return floor(clapvalue - min_value) / float(info.stepCount); } - return (clapvalue - min_value) / (max_value - min_value); + // A zero-width range has exactly one plain value, and that value is + // normalized 0. The preset selector sits there while its list is empty + // (stepCount 0, min_value == max_value == 0), and a CLAP parameter may + // legitimately declare min == max as well; dividing here would hand the + // host a NaN for either. + const auto range = max_value - min_value; + if (range <= 0.0) return 0.0; + return (clapvalue - min_value) / range; } static Vst3Parameter *create(const clap_param_info_t *info, std::function getUnitId); @@ -83,7 +90,32 @@ class Vst3Parameter : public Steinberg::Vst::Parameter // separate kind because the process adapter turns every isMidi program // change into an actual 0xC0 message, which is emphatically not what // selecting a preset should do. + // + // Created hidden and with stepCount 0 when presetCount is 0, rather than + // not at all: the parameter COUNT of a VST3 component must not change while + // it is active (the process adapter holds a raw pointer into the parameter + // container), so the selector has to exist from the first setupParameters() + // on, whatever the crawl has found by then. What may change afterwards is + // its stepCount and its flags - see resizePresetSelector(). static Vst3Parameter *createPresetSelector(Steinberg::Vst::ParamID id, int32_t presetCount); + // Re-sizes an existing selector to a list of presetCount entries, in place: + // stepCount, max_value and the kIsHidden flag. Nothing is allocated and no + // pointer moves, so the process adapter's references stay valid; the caller + // must nevertheless hold whatever excludes process() while the fields are + // written, and afterwards announce kParamTitlesChanged, which the SDK + // defines as "titles, default values, stepCount or flags have changed". + void resizePresetSelector(int32_t presetCount); + // The number of programs a selector currently publishes: 0 while hidden + // (list empty or crawl not finished), stepCount+1 otherwise - the reading a + // host makes of stepCount, and the one number every preset path in the + // wrapper has to agree on. The live index may be larger while a crawl is + // still running; that size is not published until it is complete. + int32_t presetCount() const + { + auto &info = this->getInfo(); + if (!isPreset || (info.flags & Steinberg::Vst::ParameterInfo::kIsHidden)) return 0; + return info.stepCount + 1; + } // copies from the clap_param_info_t uint32_t param_index_for_clap_get_info = 0; clap_id id = 0; diff --git a/src/wrapasvst3.cpp b/src/wrapasvst3.cpp index 733448ed..11c68529 100644 --- a/src/wrapasvst3.cpp +++ b/src/wrapasvst3.cpp @@ -1340,9 +1340,10 @@ void ClapAsVst3::setupPresets() _presetParamId = Vst::kNoParamId; _presetUnitId = Vst::kRootUnitId; - // setupParameters() can run more than once (param_rescan, and the rebuild in - // onIdle below), and the index is shared and long-lived - so drop any - // listener from a previous pass rather than stacking another onto it. + // setupParameters() can run more than once (initialize, and a + // param_rescan(RESCAN_ALL), which CLAP forbids while active), and the index + // is shared and long-lived - so drop any listener from a previous pass rather + // than stacking another onto it. if (_presetIndex && _presetIndexToken) { _presetIndex->removeCompletionListener(_presetIndexToken); @@ -1366,15 +1367,27 @@ void ClapAsVst3::setupPresets() Vst::ParamID id = 0xc00000; while (parameters.getParameter(id)) ++id; - // The list has to be sized now, because a parameter's stepCount is fixed at - // creation and a host reads stepCount+1 as the program count (the SDK's own - // preset sample sets kNumPrograms-1). So wait briefly for the crawl rather - // than announce a size that is wrong: an embedded container resolves in - // milliseconds, and a folder crawl that outlasts the wait is still covered - // by the rescan onIdle() asks for when it completes. - _presetIndex->waitUntilComplete(1000); - const auto presetCount = _presetIndex->size(); - if (presetCount == 0) return; // nothing to show; the rescan will come back + // The selector is created whatever the crawl has found so far - with zero + // presets it is hidden and has stepCount 0 - because the one thing about it + // that cannot change afterwards is whether it exists. This runs from + // initialize(), and from here on the parameter COUNT is fixed for the life of + // the instance: the process adapter dereferences the parameter container + // through a raw pointer from the audio thread, so rebuilding the parameters + // once the component is active (which is what an earlier version did from + // onIdle() when the crawl came in late) destroys objects that thread is + // using. Growing the existing parameter in place is what onIdle() does + // instead when the crawl completes. + // + // Not waited for, deliberately. This is the host's main thread inside + // initialize(); a folder crawl can take seconds, and even a short wait would + // mean the in-place path only ever runs for slow crawls, leaving it the + // least exercised path in the file. It runs for every instance instead. + // + // Only a completed crawl is published. Entries land per provider, so a + // size read mid-crawl is some providers' worth of presets, and a host that + // caches its program list at this point would keep showing that fragment. + // AUv2's rebuildPresetCache() draws the same line. + const auto presetCount = _presetIndex->isComplete() ? _presetIndex->size() : 0; auto *selector = Vst3Parameter::createPresetSelector(id, (int32_t)presetCount); @@ -1396,11 +1409,22 @@ void ClapAsVst3::setupPresets() _presetParamId = id; _presetUnitId = Vst::kRootUnitId; - // The crawl may already be done - addCompletionListener() calls straight - // back in that case, which is why _presetParamId is set before this. + // Registered unconditionally - there is no size that makes it unnecessary. + // An earlier version returned before this line when the count was zero, so + // a plug-in whose crawl was still running at initialize() never got a + // program list for the life of that instance. The crawl may also already be + // done - addCompletionListener() calls straight back in that case, which is + // why _presetParamId is set before this: a registered listener always finds + // a selector to grow, and onIdle() relies on that. _presetIndexToken = _presetIndex->addCompletionListener([this]() { onPresetIndexComplete(); }); } +Vst3Parameter *ClapAsVst3::presetSelector() const +{ + if (_presetParamId == Vst::kNoParamId) return nullptr; + return static_cast(parameters.getParameter(_presetParamId)); +} + void ClapAsVst3::onPresetIndexComplete() { // Called from the index's crawl thread. Nothing that talks to the host may @@ -1466,13 +1490,30 @@ void ClapAsVst3::preset_loaded(uint32_t locationKind, const char *location, cons size_t index = 0; if (!_presetIndex->indexOf(locationKind, location, loadKey, index)) return; - auto *param = (Vst3Parameter *)parameters.getParameter(_presetParamId); + auto *param = presetSelector(); if (!param) return; _presetIndexInEffect.store(static_cast(index), std::memory_order_relaxed); - const auto normalized = param->asVst3Value(static_cast(index)); - if (param->getNormalized() == normalized) + // The index came from the live list; the selector publishes only the size + // the completed crawl had (\see Vst3Parameter::presetCount). Between the + // two - a preset the plug-in loaded on its own while the crawl was still + // running - there is no slot to point the host at yet, and asVst3Value() on + // an index past stepCount is a normalized value above 1.0, which a host is + // entitled to reject or clamp to the wrong program. The in-effect store + // above still stands: it is what the plug-in holds, and onIdle() moves the + // selector to it once the list has grown to include it. + if (index >= static_cast(param->presetCount())) return; + + moveSelectorTo(*param, index); +} + +void ClapAsVst3::moveSelectorTo(Vst3Parameter ¶m, size_t index) +{ + // [main-thread]. The caller has checked that index is within what the + // selector publishes. + const auto normalized = param.asVst3Value(static_cast(index)); + if (param.getNormalized() == normalized) { // Already where the host put it, which is the usual case: this is the // confirmation of a load the host itself asked for. Reporting it as an @@ -1480,7 +1521,7 @@ void ClapAsVst3::preset_loaded(uint32_t locationKind, const char *location, cons return; } - param->setNormalized(normalized); + param.setNormalized(normalized); if (componentHandler) { // Bracketed, like any value a plugin originates: an unbracketed @@ -1514,10 +1555,15 @@ Steinberg::tresult PLUGIN_API ClapAsVst3::getProgramListInfo(Steinberg::int32 li if (_presetParamId == Vst::kNoParamId || listIndex != inherited) return Steinberg::kResultFalse; info.id = (Vst::ProgramListID)_presetParamId; - // What the host will show as the number of slots. Reporting the count found - // so far (rather than the parameter's 128 steps) keeps a browser from - // listing empty entries while the crawl is still running. - info.programCount = _presetIndex ? (Steinberg::int32)_presetIndex->size() : 0; + // What the host will show as the number of slots. This is the selector's + // own reading of itself - stepCount+1, or 0 while the crawl has not finished + // and the parameter is hidden - and not the live size of the index. The two + // must agree: a host lists programCount entries and moves a parameter with + // stepCount steps, and when the list is the longer of the two, picking any + // program past stepCount clamps to 1.0 and loads the last preset the + // parameter knows about. Silently, and not the one the user chose. + auto *selector = presetSelector(); + info.programCount = selector ? selector->presetCount() : 0; stringconv::convert(std::string("Presets"), info.name); return Steinberg::kResultOk; } @@ -1528,8 +1574,15 @@ Steinberg::tresult PLUGIN_API ClapAsVst3::getProgramName(Vst::ProgramListID list { if (!isPresetProgramList(listId)) return super::getProgramName(listId, programIndex, name); + // Bounded by what getProgramListInfo() published, not by what the index + // holds: mid-crawl the index may already know names for slots the host has + // not been told exist. + auto *selector = presetSelector(); + if (!selector || programIndex < 0 || programIndex >= selector->presetCount()) + return Steinberg::kResultFalse; + Clap::PresetEntry entry; - if (!_presetIndex || programIndex < 0 || !_presetIndex->presetAt((size_t)programIndex, entry)) + if (!_presetIndex || !_presetIndex->presetAt((size_t)programIndex, entry)) return Steinberg::kResultFalse; stringconv::convert(entry.displayName(), name); @@ -1809,10 +1862,14 @@ void ClapAsVst3::onIdle() if (requested >= 0) { Clap::PresetEntry entry; - // Clamp anyway: stepCount matches the count at creation, but a host may - // still have a stale value from before a rescan. - const auto count = _presetIndex ? _presetIndex->size() : 0; - if (count > 0) + // Clamp anyway, and against the count the selector publishes rather than + // the index's live size: that is the range the host was told about, and + // the one asClapValue() decoded the request against. A value can still be + // out of it - a host replaying a stale value from before the crawl grew + // the parameter, or a block decoded against the old stepCount. + auto *selector = presetSelector(); + const auto count = selector ? (size_t)selector->presetCount() : 0; + if (count > 0 && _presetIndex) { const auto index = std::min((size_t)requested, count - 1); @@ -1841,21 +1898,76 @@ void ClapAsVst3::onIdle() if (_presetListChanged.exchange(false)) { - if (_presetParamId == Vst::kNoParamId) + // The crawl finished. The selector already exists - setupPresets() creates + // it before it registers the listener that set this flag, hidden and with + // stepCount 0 if the crawl was still running then - so what happens here + // is to grow that parameter in place, never to rebuild the parameters. + // Rebuilding (parameters.removeAll() via setupParameters()) while the + // component is active destroys Parameter objects the process adapter is + // dereferencing on the audio thread through a raw ParameterContainer*. + // That path is gone from here on purpose; do not bring it back. + // + // The in-place change is what the SDK provides kParamTitlesChanged for + // (ivsteditcontroller.h: "Parameter titles (title, shortTitle and units), + // default values, stepCount or flags (ParameterFlags) have changed. The + // host invalidates all caches of parameter infos and asks the edit + // controller for the current infos"), and param_rescan() already rewrites + // getInfo().title in place and announces it the same way. + // + // isComplete() is what the listener fires on, so it holds whenever the + // flag was set; tested anyway, because publishing a partial size is the + // one thing this must never do (\see setupPresets). + auto *selector = presetSelector(); + if (selector && _presetIndex && _presetIndex->isComplete() && + (Steinberg::int32)_presetIndex->size() != selector->presetCount()) { - // The crawl finished after setupPresets() had nothing to size the list - // with, so there is no selector parameter at all yet. Only rebuilding - // the parameters can introduce one; restartComponent is what makes the - // host re-read them. - setupParameters(_plugin->_plugin, _plugin->_ext._params); + // The size test above is not only an optimisation. When the index was + // already complete at initialize() - the usual case for the second and + // every later instance of a plug-in, since the index is shared per + // module - addCompletionListener() fires straight back, the selector + // was sized from the same count a moment earlier, and there is nothing + // to tell the host. Restarting the component for that on every + // instantiation is not free on hosts that rebuild their parameter + // views for it. + const auto presetCount = _presetIndex->size(); + { + // process() decodes the selector's value against stepCount and + // min/max_value on the audio thread (asClapValue), so the writes are + // excluded against it the way the flush below is: the same spin lock, + // held for the field writes and nothing else - no allocation, no host + // call, nothing that can block. + ClapWrapper::detail::shared::SpinLockGuard growLock(_processOrFlushLock); + selector->resizePresetSelector((int32_t)presetCount); + } + + // Both notifications, and in this order. restartComponent() makes the + // host re-read the parameter info - the new stepCount, and the hidden + // flag now cleared - and notifyProgramListChange() makes it re-read the + // list that parameter selects from; a host that has cached either one + // shows the old count. kParamValuesChanged as well, so a host that + // interprets the selector's normalized value against the new stepCount + // refreshes its display of it. if (componentHandler) componentHandler->restartComponent(Vst::RestartFlags::kParamTitlesChanged | Vst::RestartFlags::kParamValuesChanged); - } - else if (auto unitHandler = Steinberg::FUnknownPtr(componentHandler)) - { - // -1: every program in the list changed, not one of them. - unitHandler->notifyProgramListChange((Vst::ProgramListID)_presetParamId, -1); + if (auto unitHandler = Steinberg::FUnknownPtr(componentHandler)) + { + // -1: every program in the list changed, not one of them. + unitHandler->notifyProgramListChange((Vst::ProgramListID)_presetParamId, -1); + } + + // A preset the plug-in loaded on its own while the crawl was still + // running (its own browser, say) had no slot to be shown in, and + // preset_loaded() left the selector where it was. It has one now; move + // the selector there. Not cosmetic: Cubase sends the selector's value in + // every process block, and left at 0 it would be read as a request to + // load preset 0 over what the user chose the moment the list grew, + // because _presetIndexInEffect says something else is in effect. + const auto inEffect = _presetIndexInEffect.load(std::memory_order_relaxed); + if (inEffect >= 0 && inEffect < static_cast(presetCount)) + { + moveSelectorTo(*selector, static_cast(inEffect)); + } } } diff --git a/src/wrapasvst3.h b/src/wrapasvst3.h index 46edccbe..7eaee3cf 100644 --- a/src/wrapasvst3.h +++ b/src/wrapasvst3.h @@ -52,6 +52,7 @@ namespace Clap { class ProcessAdapter; } +class Vst3Parameter; class queueEvent { @@ -470,9 +471,22 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, // Built in setupParameters() when the plugin implements preset-load. The // index itself is shared per module and crawls on a background thread, so // the list can be empty here and fill in later; onPresetIndexComplete() is - // what tells the host to look again. + // what tells the host to look again. The selector parameter itself always + // exists once an index does - hidden, with stepCount 0, while the crawl is + // running - and is grown in place by onIdle() when the crawl completes. The + // parameter count never changes for that; \see setupPresets(). void setupPresets(); void onPresetIndexComplete(); + // The selector parameter, or nullptr when there is none. Its stepCount and + // hidden flag are the one source of truth for how many presets the host has + // been told about (\see Vst3Parameter::presetCount) - getProgramListInfo(), + // the clamp in onIdle() and preset_loaded() all read that, none of them the + // live size of _presetIndex. + Vst3Parameter *presetSelector() const; + // Moves the selector to `index` and tells the host, as a bracketed edit. For + // a load the plug-in originated - preset_loaded(), and the catch-up in + // onIdle() once the list has grown to include what the plug-in holds. + void moveSelectorTo(Vst3Parameter ¶m, size_t index); bool isPresetProgramList(Vst::ProgramListID listId) const { return _presetParamId != Vst::kNoParamId && listId == (Vst::ProgramListID)_presetParamId; From cc9cc786f17ba61c3bd2c44001d17327fad42073 Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Mon, 14 Sep 2026 19:10:23 +0200 Subject: [PATCH 5/5] Trim comment noise in the preset selector sizing fixes Keep the host quirks and the audio-thread hazards, drop the narration. --- src/detail/vst3/parameter.cpp | 10 +-- src/detail/vst3/parameter.h | 30 +++----- src/wrapasvst3.cpp | 131 ++++++++-------------------------- src/wrapasvst3.h | 17 ++--- 4 files changed, 44 insertions(+), 144 deletions(-) diff --git a/src/detail/vst3/parameter.cpp b/src/detail/vst3/parameter.cpp index e6270b40..56f0ccae 100644 --- a/src/detail/vst3/parameter.cpp +++ b/src/detail/vst3/parameter.cpp @@ -205,8 +205,6 @@ Vst3Parameter *Vst3Parameter::createPresetSelector(Steinberg::Vst::ParamID id, i p->isPreset = true; p->min_value = 0; p->max_value = 0; - // One place decides what a count of N looks like on the parameter, whether - // it is the count at creation or the one the crawl delivers later. p->resizePresetSelector(presetCount); return p; } @@ -214,17 +212,13 @@ Vst3Parameter *Vst3Parameter::createPresetSelector(Steinberg::Vst::ParamID id, i void Vst3Parameter::resizePresetSelector(int32_t presetCount) { auto &v = getInfo(); - // A host reads stepCount+1 as the number of programs (the SDK's own preset - // sample sets kNumPrograms-1), so one preset is stepCount 0 - which is also - // what an empty list is. The flag is what tells the two apart. + // A host reads stepCount+1 as the program count, so a single preset and an + // empty list are both stepCount 0 - kIsHidden is what tells them apart. v.stepCount = presetCount > 1 ? presetCount - 1 : 0; if (presetCount > 0) v.flags &= ~Vst::ParameterInfo::kIsHidden; else v.flags |= Vst::ParameterInfo::kIsHidden; - // The plain range asClapValue()/asVst3Value() convert against: index 0 to - // the last index. With no presets it collapses to 0..0, which asVst3Value() - // guards against dividing by. min_value = 0; max_value = v.stepCount; } diff --git a/src/detail/vst3/parameter.h b/src/detail/vst3/parameter.h index fb4582c3..944deb1d 100644 --- a/src/detail/vst3/parameter.h +++ b/src/detail/vst3/parameter.h @@ -72,11 +72,8 @@ class Vst3Parameter : public Steinberg::Vst::Parameter { return floor(clapvalue - min_value) / float(info.stepCount); } - // A zero-width range has exactly one plain value, and that value is - // normalized 0. The preset selector sits there while its list is empty - // (stepCount 0, min_value == max_value == 0), and a CLAP parameter may - // legitimately declare min == max as well; dividing here would hand the - // host a NaN for either. + // min == max - an empty preset selector, or a CLAP parameter that declares + // it - has exactly one plain value, and dividing would hand the host a NaN. const auto range = max_value - min_value; if (range <= 0.0) return 0.0; return (clapvalue - min_value) / range; @@ -91,25 +88,14 @@ class Vst3Parameter : public Steinberg::Vst::Parameter // change into an actual 0xC0 message, which is emphatically not what // selecting a preset should do. // - // Created hidden and with stepCount 0 when presetCount is 0, rather than - // not at all: the parameter COUNT of a VST3 component must not change while - // it is active (the process adapter holds a raw pointer into the parameter - // container), so the selector has to exist from the first setupParameters() - // on, whatever the crawl has found by then. What may change afterwards is - // its stepCount and its flags - see resizePresetSelector(). + // Created even for presetCount 0 (hidden, stepCount 0): the parameter count + // must not change while the component is active - see resizePresetSelector(). static Vst3Parameter *createPresetSelector(Steinberg::Vst::ParamID id, int32_t presetCount); - // Re-sizes an existing selector to a list of presetCount entries, in place: - // stepCount, max_value and the kIsHidden flag. Nothing is allocated and no - // pointer moves, so the process adapter's references stay valid; the caller - // must nevertheless hold whatever excludes process() while the fields are - // written, and afterwards announce kParamTitlesChanged, which the SDK - // defines as "titles, default values, stepCount or flags have changed". + // Resizes an existing selector in place. The caller must exclude process() + // while the fields are written, and afterwards announce kParamTitlesChanged. void resizePresetSelector(int32_t presetCount); - // The number of programs a selector currently publishes: 0 while hidden - // (list empty or crawl not finished), stepCount+1 otherwise - the reading a - // host makes of stepCount, and the one number every preset path in the - // wrapper has to agree on. The live index may be larger while a crawl is - // still running; that size is not published until it is complete. + // Programs published to the host: 0 while hidden, stepCount+1 otherwise. The + // live index may be larger while a crawl is still running. int32_t presetCount() const { auto &info = this->getInfo(); diff --git a/src/wrapasvst3.cpp b/src/wrapasvst3.cpp index 11c68529..d71f9f73 100644 --- a/src/wrapasvst3.cpp +++ b/src/wrapasvst3.cpp @@ -1340,10 +1340,8 @@ void ClapAsVst3::setupPresets() _presetParamId = Vst::kNoParamId; _presetUnitId = Vst::kRootUnitId; - // setupParameters() can run more than once (initialize, and a - // param_rescan(RESCAN_ALL), which CLAP forbids while active), and the index - // is shared and long-lived - so drop any listener from a previous pass rather - // than stacking another onto it. + // setupParameters() can run more than once, and the index is shared and + // long-lived - drop any listener from a previous pass rather than stack one. if (_presetIndex && _presetIndexToken) { _presetIndex->removeCompletionListener(_presetIndexToken); @@ -1367,26 +1365,10 @@ void ClapAsVst3::setupPresets() Vst::ParamID id = 0xc00000; while (parameters.getParameter(id)) ++id; - // The selector is created whatever the crawl has found so far - with zero - // presets it is hidden and has stepCount 0 - because the one thing about it - // that cannot change afterwards is whether it exists. This runs from - // initialize(), and from here on the parameter COUNT is fixed for the life of - // the instance: the process adapter dereferences the parameter container - // through a raw pointer from the audio thread, so rebuilding the parameters - // once the component is active (which is what an earlier version did from - // onIdle() when the crawl came in late) destroys objects that thread is - // using. Growing the existing parameter in place is what onIdle() does - // instead when the crawl completes. - // - // Not waited for, deliberately. This is the host's main thread inside - // initialize(); a folder crawl can take seconds, and even a short wait would - // mean the in-place path only ever runs for slow crawls, leaving it the - // least exercised path in the file. It runs for every instance instead. - // - // Only a completed crawl is published. Entries land per provider, so a - // size read mid-crawl is some providers' worth of presets, and a host that - // caches its program list at this point would keep showing that fragment. - // AUv2's rebuildPresetCache() draws the same line. + // Created whatever the crawl has found, and never waited for: the parameter + // count must not change once the component is active (the process adapter + // holds a raw pointer into the container), so onIdle() grows it in place. + // Only a completed crawl is published; a mid-crawl size is a fragment. const auto presetCount = _presetIndex->isComplete() ? _presetIndex->size() : 0; auto *selector = Vst3Parameter::createPresetSelector(id, (int32_t)presetCount); @@ -1409,13 +1391,8 @@ void ClapAsVst3::setupPresets() _presetParamId = id; _presetUnitId = Vst::kRootUnitId; - // Registered unconditionally - there is no size that makes it unnecessary. - // An earlier version returned before this line when the count was zero, so - // a plug-in whose crawl was still running at initialize() never got a - // program list for the life of that instance. The crawl may also already be - // done - addCompletionListener() calls straight back in that case, which is - // why _presetParamId is set before this: a registered listener always finds - // a selector to grow, and onIdle() relies on that. + // The crawl may already be done - addCompletionListener() calls straight + // back in that case, which is why _presetParamId is set before this. _presetIndexToken = _presetIndex->addCompletionListener([this]() { onPresetIndexComplete(); }); } @@ -1495,14 +1472,9 @@ void ClapAsVst3::preset_loaded(uint32_t locationKind, const char *location, cons _presetIndexInEffect.store(static_cast(index), std::memory_order_relaxed); - // The index came from the live list; the selector publishes only the size - // the completed crawl had (\see Vst3Parameter::presetCount). Between the - // two - a preset the plug-in loaded on its own while the crawl was still - // running - there is no slot to point the host at yet, and asVst3Value() on - // an index past stepCount is a normalized value above 1.0, which a host is - // entitled to reject or clamp to the wrong program. The in-effect store - // above still stands: it is what the plug-in holds, and onIdle() moves the - // selector to it once the list has grown to include it. + // The index is from the live list, which can be ahead of what the selector + // publishes; an index past stepCount normalizes above 1.0. onIdle() moves the + // selector to _presetIndexInEffect once the list has grown to include it. if (index >= static_cast(param->presetCount())) return; moveSelectorTo(*param, index); @@ -1510,8 +1482,7 @@ void ClapAsVst3::preset_loaded(uint32_t locationKind, const char *location, cons void ClapAsVst3::moveSelectorTo(Vst3Parameter ¶m, size_t index) { - // [main-thread]. The caller has checked that index is within what the - // selector publishes. + // [main-thread] index must be within what the selector publishes. const auto normalized = param.asVst3Value(static_cast(index)); if (param.getNormalized() == normalized) { @@ -1555,13 +1526,9 @@ Steinberg::tresult PLUGIN_API ClapAsVst3::getProgramListInfo(Steinberg::int32 li if (_presetParamId == Vst::kNoParamId || listIndex != inherited) return Steinberg::kResultFalse; info.id = (Vst::ProgramListID)_presetParamId; - // What the host will show as the number of slots. This is the selector's - // own reading of itself - stepCount+1, or 0 while the crawl has not finished - // and the parameter is hidden - and not the live size of the index. The two - // must agree: a host lists programCount entries and moves a parameter with - // stepCount steps, and when the list is the longer of the two, picking any - // program past stepCount clamps to 1.0 and loads the last preset the - // parameter knows about. Silently, and not the one the user chose. + // The slots the host will show: the selector's own stepCount+1, not the live + // index size. If the list were the longer of the two, picking a program past + // stepCount clamps to 1.0 and silently loads the wrong preset. auto *selector = presetSelector(); info.programCount = selector ? selector->presetCount() : 0; stringconv::convert(std::string("Presets"), info.name); @@ -1574,9 +1541,8 @@ Steinberg::tresult PLUGIN_API ClapAsVst3::getProgramName(Vst::ProgramListID list { if (!isPresetProgramList(listId)) return super::getProgramName(listId, programIndex, name); - // Bounded by what getProgramListInfo() published, not by what the index - // holds: mid-crawl the index may already know names for slots the host has - // not been told exist. + // Bounded by what getProgramListInfo() published: mid-crawl the index knows + // names for slots the host has not been told exist. auto *selector = presetSelector(); if (!selector || programIndex < 0 || programIndex >= selector->presetCount()) return Steinberg::kResultFalse; @@ -1862,11 +1828,8 @@ void ClapAsVst3::onIdle() if (requested >= 0) { Clap::PresetEntry entry; - // Clamp anyway, and against the count the selector publishes rather than - // the index's live size: that is the range the host was told about, and - // the one asClapValue() decoded the request against. A value can still be - // out of it - a host replaying a stale value from before the crawl grew - // the parameter, or a block decoded against the old stepCount. + // Clamp against what the selector publishes - the range asClapValue() + // decoded against; a host may still replay a value from an older stepCount. auto *selector = presetSelector(); const auto count = selector ? (size_t)selector->presetCount() : 0; if (count > 0 && _presetIndex) @@ -1898,55 +1861,24 @@ void ClapAsVst3::onIdle() if (_presetListChanged.exchange(false)) { - // The crawl finished. The selector already exists - setupPresets() creates - // it before it registers the listener that set this flag, hidden and with - // stepCount 0 if the crawl was still running then - so what happens here - // is to grow that parameter in place, never to rebuild the parameters. - // Rebuilding (parameters.removeAll() via setupParameters()) while the - // component is active destroys Parameter objects the process adapter is - // dereferencing on the audio thread through a raw ParameterContainer*. - // That path is gone from here on purpose; do not bring it back. - // - // The in-place change is what the SDK provides kParamTitlesChanged for - // (ivsteditcontroller.h: "Parameter titles (title, shortTitle and units), - // default values, stepCount or flags (ParameterFlags) have changed. The - // host invalidates all caches of parameter infos and asks the edit - // controller for the current infos"), and param_rescan() already rewrites - // getInfo().title in place and announces it the same way. - // - // isComplete() is what the listener fires on, so it holds whenever the - // flag was set; tested anyway, because publishing a partial size is the - // one thing this must never do (\see setupPresets). + // Grow the existing selector in place; never rebuild the parameters here - + // that destroys Parameter objects the process adapter is dereferencing on + // the audio thread. kParamTitlesChanged is the SDK's channel for this. auto *selector = presetSelector(); if (selector && _presetIndex && _presetIndex->isComplete() && (Steinberg::int32)_presetIndex->size() != selector->presetCount()) { - // The size test above is not only an optimisation. When the index was - // already complete at initialize() - the usual case for the second and - // every later instance of a plug-in, since the index is shared per - // module - addCompletionListener() fires straight back, the selector - // was sized from the same count a moment earlier, and there is nothing - // to tell the host. Restarting the component for that on every - // instantiation is not free on hosts that rebuild their parameter - // views for it. + // The size test is not just an optimisation: with the shared index already + // complete there is nothing to tell the host, and a restart is not free. const auto presetCount = _presetIndex->size(); { - // process() decodes the selector's value against stepCount and - // min/max_value on the audio thread (asClapValue), so the writes are - // excluded against it the way the flush below is: the same spin lock, - // held for the field writes and nothing else - no allocation, no host - // call, nothing that can block. + // process() decodes the selector against stepCount and min/max_value on + // the audio thread, so exclude it for the field writes and nothing else. ClapWrapper::detail::shared::SpinLockGuard growLock(_processOrFlushLock); selector->resizePresetSelector((int32_t)presetCount); } - // Both notifications, and in this order. restartComponent() makes the - // host re-read the parameter info - the new stepCount, and the hidden - // flag now cleared - and notifyProgramListChange() makes it re-read the - // list that parameter selects from; a host that has cached either one - // shows the old count. kParamValuesChanged as well, so a host that - // interprets the selector's normalized value against the new stepCount - // refreshes its display of it. + // Both, in this order: the parameter info, then the list it selects from. if (componentHandler) componentHandler->restartComponent(Vst::RestartFlags::kParamTitlesChanged | Vst::RestartFlags::kParamValuesChanged); @@ -1956,13 +1888,8 @@ void ClapAsVst3::onIdle() unitHandler->notifyProgramListChange((Vst::ProgramListID)_presetParamId, -1); } - // A preset the plug-in loaded on its own while the crawl was still - // running (its own browser, say) had no slot to be shown in, and - // preset_loaded() left the selector where it was. It has one now; move - // the selector there. Not cosmetic: Cubase sends the selector's value in - // every process block, and left at 0 it would be read as a request to - // load preset 0 over what the user chose the moment the list grew, - // because _presetIndexInEffect says something else is in effect. + // Cubase sends the selector's value in every process block, so a preset the + // plug-in loaded mid-crawl must be caught up to or preset 0 wins over it. const auto inEffect = _presetIndexInEffect.load(std::memory_order_relaxed); if (inEffect >= 0 && inEffect < static_cast(presetCount)) { diff --git a/src/wrapasvst3.h b/src/wrapasvst3.h index 7eaee3cf..606716e4 100644 --- a/src/wrapasvst3.h +++ b/src/wrapasvst3.h @@ -471,21 +471,14 @@ class ClapAsVst3 : public Steinberg::Vst::SingleComponentEffect, // Built in setupParameters() when the plugin implements preset-load. The // index itself is shared per module and crawls on a background thread, so // the list can be empty here and fill in later; onPresetIndexComplete() is - // what tells the host to look again. The selector parameter itself always - // exists once an index does - hidden, with stepCount 0, while the crawl is - // running - and is grown in place by onIdle() when the crawl completes. The - // parameter count never changes for that; \see setupPresets(). + // what tells the host to look again. The selector always exists once an index + // does - hidden while the crawl runs - and onIdle() grows it in place. void setupPresets(); void onPresetIndexComplete(); - // The selector parameter, or nullptr when there is none. Its stepCount and - // hidden flag are the one source of truth for how many presets the host has - // been told about (\see Vst3Parameter::presetCount) - getProgramListInfo(), - // the clamp in onIdle() and preset_loaded() all read that, none of them the - // live size of _presetIndex. + // The selector parameter, or nullptr. Its presetCount() - never the live size + // of _presetIndex - is what the host has been told about. Vst3Parameter *presetSelector() const; - // Moves the selector to `index` and tells the host, as a bracketed edit. For - // a load the plug-in originated - preset_loaded(), and the catch-up in - // onIdle() once the list has grown to include what the plug-in holds. + // Moves the selector to `index` and tells the host, as a bracketed edit. void moveSelectorTo(Vst3Parameter ¶m, size_t index); bool isPresetProgramList(Vst::ProgramListID listId) const {