free-audio / clap-wrapper

Wrappers for using CLAP in other plugin environments
MIT License
107 stars 17 forks source link

Check active state before querying latency. Fixes #229 #230

Closed Schroedingers-Cat closed 6 months ago

Schroedingers-Cat commented 6 months ago

The VST3 wrapper simply forwards the host's latency request without checking whether the plugin is active. This causes a misbehaving host log when using the CLAP helpers (and is not allowed by CLAP's specs).

This PR fixes that problem by checking the plugin's active state before getting its latency. This PR also fixes #229.

baconpaul commented 6 months ago

This looks good to me but I think we should merge it into next then cherry pick it to main. But @defiantnerd i am unsure how next v main is tracking right now.

defiantnerd commented 6 months ago

I need to think about it - it avoids the protocol violation, but might lead to strange behavior with a wrapped host if it does not ask again for the latency.

Schroedingers-Cat commented 6 months ago

but might lead to strange behavior with a wrapped host if it does not ask again for the latency.

Even without this fix, the acquired latency will be of questionable correctness since the samplerate isn't known before the plugin is active.

I can update this PR next Monday with a behaviour that stores a missed latency call and handles that in the next activation of the plugin. That should cover the scenario of a plugin not reporting the latency on its own.

baconpaul commented 6 months ago

definitely if you ask an inactive plugin for latency you could queue a latency change update after the active

have we considered just activating the plugin here though also? what's the call path by which this happens?

defiantnerd commented 6 months ago

would this also be useful for the tail-extension?

 auto tailsize = this->_plugin->_ext._tail->get(_plugin->_plugin);
defiantnerd commented 6 months ago

is it possible that the necessary values (samplerate/blocksize) have been already set and only the setActive() call is missing and the host assumes that the (vst3-)plugin might now be able to tell the correct latency?

Schroedingers-Cat commented 6 months ago

have we considered just activating the plugin here though also? what's the call path by which this happens?

It's a direct call from the host (Reaper in my case, but Cubase 13 is also reported to instantly crash).

I've updated the PR so that a missed latency-call is stored and acted upon during setActive(). The changes hit an issue with the format checks but no error message is being printed. Any idea why?

EDIT: Seems my Xcode produced weird indentations. Fixed that failing check by reformatting the changed code with VS.

baconpaul commented 6 months ago

This looks good to me. @defiantnerd shall we include this in 0.7.2 also along with your other changes? If so I can do the double merge and move and stuff.

Schroedingers-Cat commented 6 months ago

I've added three additional commits in correspondence with @defiantnerd that fix additional crashes during the initialization phase (Reaper, Nuendo). I have no idea how I didn't have these issues last week on my Intel Mac, but these problems were happening on both my Win system and ARM Mac as well as on @defiantnerd 's Win system.

I'd recommend merging this PR instead of going forward with the incomplete merge via 06e49cfebb540680e18b3c850bdb9c64dc75719b as it's missing another misbehavior fix (bbe9f8994df83a0ac4ef77ae51a56e657aeb9eba) as well as the requested change to store a missed latency call (45b69bc01f77c848c745f0808df322a6d2a64858).

defiantnerd commented 6 months ago

it seems that the vst3 documentation contradicts itself and getLatency() is allowed between setupProcessing and setActive - I heard that at least Reaper and Nuendo are doing it that way. So we need to solve this differently by introducing a feature where the CLAP is being transitioned between the necessary states.

defiantnerd commented 6 months ago

I'll merge this now. We'll give it a makeover later.