0.16 review: size the preset selector once and grow it in place (PD-2, PD-4, PD-6, VST3-1) - #548
Merged
Merged
Conversation
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.
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.
Comment-only change: the explanatory blocks are cut to the non-obvious why.
…lace 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.
Keep the host quirks and the audio-thread hazards, drop the narration.
defiantnerd
force-pushed
the
0.16-fix-preset-knot
branch
from
September 14, 2026 17:19
d6a2282 to
cc9cc78
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fourth and last code batch from the 0.16 release review of
main...next.Based on #546, not on
next— VST3-1's fix lands inonIdle, which #546 also changes. GitHub will retarget this tonextwhen #546 merges. Review #546 first.Why these four are one change
They interlock, and fixing one on its own makes another worse.
VST3-1's teardown branch requires
_presetListChangedset while_presetParamIdis stillkNoParamId. 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 exactly what PD-2 asks for in isolation, would have made a use-after-free live and frequent.That also corrects the review: VST3-1's stated failure scenario could not actually happen, because seeing zero presets returned early without registering any listener at all.
So the teardown is removed rather than worked around.
What changed
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 registered straight after, so a registered listener always has a selector to grow. Previously, a plugin whose folder crawl outlivedinitialize()got no program list for the life of that instance. (AUv2 already did this correctly.)PD-4.
stepCountwas frozen at creation whilegetProgramListInforeported the live index size. A host listed 2100 programs against a 99-step parameter; picking #1500 clamped to 1.0 and loaded preset 99 — silently. There is now one number,Vst3Parameter::presetCount(), read bygetProgramListInfo,getProgramName, theonIdleclamp andpreset_loaded. The live size is never published, and only a complete crawl is published at all.PD-6.
waitUntilComplete(1000)is gone frominitialize().VST3-1.
onIdleno longer callssetupParameters(), soparameters.removeAll()can no longer destroyParameterobjects while the process adapter dereferences them on the audio thread. The completion path writesstepCount,min/max_valueand the hidden flag on the existing parameter under the same spin lockprocess()takes, then announceskParamTitlesChanged— whichivsteditcontroller.hdefines as covering "titles, default values, stepCount or flags" — alongsidenotifyProgramListChange.param_rescanalready rewritesgetInfo().titlein place and announces it the same way.setupParameters()is now reachable only frominitialize()andparam_rescan(RESCAN_ALL), which CLAP forbids while active.kReloadComponentwas considered and rejected: the SDK's own wrapper says// kReloadComponent is Not supported(basewrapper.cpp:1418).Two things found while implementing
A no-op guard. The index is shared per module, so from the second instance onward the crawl is already complete at
initialize()and the listener fires straight back with nothing to say. Without the guard every instantiation wouldrestartComponentfor nothing.Selector catch-up. A preset the plugin loads from its own UI during the crawl has no slot yet, so the selector stays at 0 — and 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. This is a bug the fix would have introduced. After the grow,
onIdlemoves the selector to_presetIndexInEffectwhen it is now in range.