VST3: only kResultFalse is end of stream, and the preset selector cannot be read before the host sends it

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<CLAPVST3StreamAdapter *>(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<uint64_t>(Steinberg::kMaxInt32)
+                             ? Steinberg::kMaxInt32
+                             : static_cast<Steinberg::int32>(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)
   {

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<Vst3Parameter *>(parameters.getParameter(_presetParamId)))
-    {
-      const auto index = static_cast<int64_t>(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<int64_t>(presetIndex), std::memory_order_relaxed);
+    return;
+  }
+
   if (static_cast<int64_t>(presetIndex) == _presetIndexInEffect.load(std::memory_order_relaxed))
   {
     return;

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<int64_t> _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<bool> _adoptNextPresetValue{false};
   // Set when the crawl finishes; onIdle() turns it into the host notification,
   // because notifyProgramListChange() is not for a background thread.
   std::atomic<bool> _presetListChanged{false};