From b0df674cfd20a36231f3b6807ccce65771360d9b Mon Sep 17 00:00:00 2001 From: defiantnerd <97224712+defiantnerd@users.noreply.github.com> Date: Sun, 13 Sep 2026 16:36:05 +0200 Subject: [PATCH] VST3: only kResultFalse means end of stream, and the selector cannot be read before the host sends it Two corrections to #543, both found in review. The stream adapter reported EVERY non-kResultOk result as end of file, not just the kResultFalse a host uses to say "that ran past the end of the chunk". kOutOfMemory - which the SDK's own MemoryStream::read returns when its buffer could not be allocated - then reads as a clean EOF: a plug-in that treats "no bytes" as "no state" reports a successful load, and the next save writes its defaults over the user's project. Worse, a reader that loops until it has the bytes it asked for never terminates, because the call that used to break the loop now returns 0 forever - and that loop is the pattern in this repository's own conformance fixtures. A real error is a real error again. The size argument is also clamped rather than cast: IBStream counts in int32, and a reader with no length asking for more than that is not making a mistake - the extension gives it no way to ask how much there is. setState seeded _presetIndexInEffect from the selector parameter, which cannot work: at that point the parameter still holds createPresetSelector's default of 0. A host replays a program list parameter AFTER the component state, the selector's value is not in the state chunk, and syncParameterValuesFromClap cannot fill it either - the selector's id is a tag this wrapper invented and no CLAP plug-in owns, so get_value fails for it. The seed was therefore always 0, which missed every preset except index 0 and made index 0 itself unreachable for the life of the instance: the first entry in every program list, dead after any setState. What works instead is to adopt the first value the host sends after a state load, whatever it says, and load nothing. It is armed only by a setState that succeeded - a fresh instance has no state to protect, and a load that failed leaves the host's selector the better authority - which is also why it cannot be inferred from _presetIndexInEffect being -1, a condition both cases share. The cost is stated at the guard: on a host that sends this parameter only when it changes, the user's first pick after a project load is spent on the adopt and the second one loads. --- src/detail/vst3/state.h | 48 +++++++++++++++++++------------ src/wrapasvst3.cpp | 62 ++++++++++++++++++++++++++++++----------- src/wrapasvst3.h | 13 +++++++++ 3 files changed, 88 insertions(+), 35 deletions(-) diff --git a/src/detail/vst3/state.h b/src/detail/vst3/state.h index 543d6345..c3d2a079 100644 --- a/src/detail/vst3/state.h +++ b/src/detail/vst3/state.h @@ -26,28 +26,40 @@ class CLAPVST3StreamAdapter static int64_t read(const struct clap_istream *stream, void *buffer, uint64_t size) { auto self = static_cast(stream->ctx); + + // IBStream counts in int32. A caller asking for more than that is not an + // error - it is a reader with no length, which is every clap_istream + // reader - so give it what fits and let it come back for the rest. + const int32 wanted = size > static_cast(Steinberg::kMaxInt32) + ? Steinberg::kMaxInt32 + : static_cast(size); + Steinberg::int32 bytesRead = 0; - const auto result = self->vst_stream->read(buffer, (int32)size, &bytesRead); + const auto result = self->vst_stream->read(buffer, wanted, &bytesRead); if (kResultOk == result) return bytesRead; - // Not every host reports the end of a stream the way the SDK's own - // IBStream implementations do. Cubase answers a read that runs past the - // end of the plug-in's chunk with kResultFalse rather than with kResultOk - // and a short count - and a reader asking for more than is left is the - // normal way to read a stream whose length it does not know, which is - // every reader clap_istream has. - // - // CLAP has one code for "there is nothing more": 0. Reporting -1 instead - // says "this stream is broken", and a plug-in that believes it throws away - // the project state it was in the middle of restoring - which is exactly - // what it did, on the very first read, so nothing was ever restored under - // VST3 at all. + // kResultFalse, and ONLY kResultFalse, is how a host may phrase "that ran + // past the end of the chunk". Cubase answers that way rather than with + // kResultOk and a short count, and asking for more than is left is the + // normal way to read a stream whose length you were never told - the + // extension gives a reader no way to ask. // - // A real I/O failure ends up here too and is reported as a short read. The - // reader gets a truncated chunk and rejects it, which is where it was - // going to end up anyway; what it does not do is lose a whole project to a - // host that phrased the end of a stream differently. - return bytesRead > 0 ? bytesRead : 0; + // CLAP has one code for "there is nothing more" and it is 0 + // (clap/stream.h: "0 indicates end of file and -1 a read error"), so that + // is what this returns. Reporting -1 for it said "this stream is broken", + // and a plug-in that believed it threw away the project state it was in + // the middle of restoring - on the very first read, so under VST3 nothing + // was ever restored at all. + if (kResultFalse == result) return bytesRead; + + // Anything else is a real failure - kOutOfMemory, kInternalError, + // kInvalidArgument - and has to stay one. Reporting end-of-file for these + // is worse than reporting nothing: a reader that loops until it has the + // bytes it needs (the pattern in this repository's own conformance + // fixtures) never terminates, and one that treats "no bytes" as "no state" + // reports a successful load and lets the next save write its defaults over + // the user's project. + return -1; } static int64_t write(const struct clap_ostream *stream, const void *buffer, uint64_t size) { diff --git a/src/wrapasvst3.cpp b/src/wrapasvst3.cpp index 83f20dd7..0baf2881 100644 --- a/src/wrapasvst3.cpp +++ b/src/wrapasvst3.cpp @@ -320,21 +320,32 @@ tresult PLUGIN_API ClapAsVst3::setState(IBStream *state) // the preset list - dropped in, or edited from the plugin's own default // - names none, and leaves nothing for the guard to work with. // - // The second is why the selector's own value is read here. Where the host's - // list stands is not a request to go there - it is where the restored - // content came from - so it is what is in effect. Only when the state named - // no preset at all: if it named one, preset_loaded() has run during the load - // above and has put the right index there already. - _presetLoadRequest.store(-1); - - if (_presetParamId != Vst::kNoParamId && - _presetIndexInEffect.load(std::memory_order_relaxed) < 0) + // Only when there is a restored state to protect. A load that failed leaves + // the plug-in holding whatever it held before, and the host's selector is + // then the better authority rather than the worse one. + if (result == kResultOk) { - if (auto *param = static_cast(parameters.getParameter(_presetParamId))) - { - const auto index = static_cast(param->asClapValue(param->getNormalized()) + 0.5); - if (index >= 0) _presetIndexInEffect.store(index, std::memory_order_relaxed); - } + // 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(). + // + // 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 + // the parameter still holds createPresetSelector's default of 0. The + // host replays a program list parameter AFTER the component state, the + // selector's value is not in the state chunk, and syncParameterValuesFromClap + // cannot fill it either - the selector's id is a tag this wrapper invented + // and no CLAP plug-in owns, so get_value fails for it. Seeding from it + // 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); } return result; @@ -1410,9 +1421,26 @@ void ClapAsVst3::onRequestPresetLoad(size_t presetIndex) // parameter stream carries the selector's value, not its edges, and acting // on every arrival makes loading a preset a permanent state of reloading it // (\see _presetIndexInEffect). - // A state load establishes the same thing without a preset ever being - // loaded, which is what keeps a restored project from reloading the preset - // it was saved from over itself. \see setState(). + // Neither is the first value to arrive after a state load, whatever it says. + // The state is the newer fact about what the plug-in holds; the selector is + // a label on where that content started, and a host restoring a project + // hands back the label it stored with it. Obeying that reloads the preset + // over everything the project just restored. So adopt the value as the one + // in effect and load nothing - an actual move by the user still names + // something else, and is still obeyed. \see setState(). + // + // The cost, stated plainly: on a host that sends this parameter only when it + // changes rather than in every process block, nothing arrives to be adopted + // until the user picks a preset, and that first pick is spent on the adopt. + // 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; + } + if (static_cast(presetIndex) == _presetIndexInEffect.load(std::memory_order_relaxed)) { return; diff --git a/src/wrapasvst3.h b/src/wrapasvst3.h index 0ad018f2..5d809c73 100644 --- a/src/wrapasvst3.h +++ b/src/wrapasvst3.h @@ -502,6 +502,19 @@ 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};