docs: add !processing requirement to clap_plugin->deactivate

[Quant1um]

Currently, deactivate is "allowed" to be called without a stop_processing call, which is likely not intentional as most implementations (including clap-helpers) rely on all state transitions being explicit.

This PR adds a !processing requirement to deactivate, which requires hosts to call stop_processing explicitly before deactivate.

#515

[defiantnerd]

clarification is a good thing.

[abique]

I think it is OK to call deactivate without stop processing.

stop processing sends the plugin to sleep but the plugin is still active.
stop processing is called from the audio thread, and if for some reason your audio callback isn't happening anymore, maybe you don't execute stuff on audio thread anymore. So calling deactivate without stop processing seems fine to me.

Here's is the clap helpers:

   template <MisbehaviourHandler h, CheckingLevel l>
   void Plugin<h, l>::clapDeactivate(const clap_plugin *plugin) noexcept {
      auto &self = from(plugin);
      self.ensureInitialized("deactivate");
      self.ensureMainThread("clap_plugin.deactivate");

      if (l >= CheckingLevel::Minimal) {
         if (!self._isActive) {
            self.hostMisbehaving("The plugin was deactivated twice.");
            return;
         }
      }

      self.deactivate();
      self._isActive = false;
      self._sampleRate = 0;
   }
   
   template <MisbehaviourHandler h, CheckingLevel l>
   void Plugin<h, l>::clapStopProcessing(const clap_plugin *plugin) noexcept {
      auto &self = from(plugin);
      self.ensureInitialized("stop_processing");
      self.ensureAudioThread("clap_plugin.stop_processing");

      if (l >= CheckingLevel::Minimal) {
         if (!self._isActive) {
            self.hostMisbehaving(
               "Host called clap_plugin.stop_processing() on a deactivated plugin");
            return;
         }

         if (!self._isProcessing) {
            self.hostMisbehaving("Host called clap_plugin.stop_processing() twice");
            return;
         }
      }

      self.stopProcessing();
      self._isProcessing = false;
   }

And as you can see you can deactivate even if the plugin is still processing.

A possible improvement would be to set _isProcessing = false; in deactivate().

[defiantnerd]

The clap-helpers implement the docs, but the docs are imprecise and most of us think it should be mandatory to stop processing before deactivate. it is a proper way to communicate the transition, especially since it is from audio thread to main thread.

[baconpaul]

yeah what does it mean to be an 'inactive processing' unit?

[abique]

yeah what does it mean to be an 'inactive processing' unit?

The mistake here is to read the processing state when inactive.
When the plugin is inactive, the processing state is irrelevant and reading it is a bug.

[defiantnerd]

but this exactly is not clear by the documentation. therefore the enhancement of the documentation making it mandatory. CLAP is not VST2.

making it super-obvious will clear the confusion and provide a reference point.

[baconpaul]

yeah what does it mean to be an 'inactive processing' unit?

The mistake here is to read the processing state when inactive. When the plugin is inactive, the processing state is irrelevant and reading it is a bug.

Here's two alternatives.

Alternative 1:

A call to deactivate results in !processing no matter the processing state of the plugin. A host must use some mechanism to make sure that deactivate is not called on the main thread when the audio thread has entered a call to processing which has not completed, and may not start a call to processing after entering deactivate.

As a result, there are two paths in the state diagram from processing to !processing one is stop_processing the other is deactivate and a plugin cant guarantee that stop_processing is called before deactivation so shouldn't rely on it. stop_processing is useful only as a hint.

Alternative 2:

deactivate requires !processing so a host must trigger stop_processing on the audio thread before deactivate. A plugin can rely on stop_processing being called before deactivate for any processor which had start_processing called

I know which I prefer, but either Alternative 1 or Alternative 2 should be the spec and in the documentation. The 'leave it ambiguous' or 'leave it implicit' is bad.

[abique]

@Quant1um please update activate() to specify:

• after successful activate(), the plugin is !processing
• the processing state is only relevant while activated

Please update deactivate() to mention that after this call, the processing state isn't relevant anymore.

About requiring the host to call stop_processing() before deactivate():

• we can encourage the host in doing that
• plugins should be aware that there are valid host that may call deactivate() without calling stop_processing()

That wasn't a requirement before, so there are valid clap host that may not do it; because of that I don't think we can require hosts built against older CLAP version to implement it. The only thing you could do is to require host built using CLAP > 1.x to honor this behavior, and plugins would have to check the CLAP version to know which behavior to expect from the host??? I can't see that as a good solution, and it seems to me that it is simpler as a plugin to check in deactivate() if the plugin was processing and perform the necessary tasks.

As a reminder, both start_processing() and stop_processing() have real-time requirements, which means that you should not do resource allocation, I/O, ... All the heavy resource management happens in activate() and deactivate().

If you really really really want to enforce stop_processing() before deactivate() then you should specify as well that if a plugin counts on it, it should then return NULL from clap_plugin_factory.create_plugin() if host->clap_version < 1.x. But I don't think we want to go there right? ;-)

In regards to host validation, I think it is fine to report a missing call to stop_processing() as a warning, but not an error.

EDIT:
@Quant1um maybe you can add something to clarify that there can't be a race between activate()/deactivate() and start_processing()/stop_processing(), they can't overlap. The host MUST guarantee it.

Thanks 👍

[Quant1um]

@abique Done! Removed the !processing requirement from deactivate, added a comment that stop_processing is implied, and the concurrency requirements clarification as per this comment. Let me know what you think

[defiantnerd]

I am not happy with this.

[Quant1um]

I personally prefer the !processing requirement solution to the problem, but this is indeed risky as it would technically cause breakage with host implementors that decided to not call stop_processing, which is unfortunate but is spec-compliant. On the other hand, some already assume that it has to be called (clap-helpers emits a host-misbehavior message if stop_processing is skipped currently).

In my opinion it's not a big deal (a plugin can track processing state internally and emit a stop_processing on deactivate if needed), but it still should be specified in docs one way or the other.

[abique]

@Quant1um thank you for the PR 👍
I wanted to word things differently so I did the change myself in fcebab3