Expand the documentation on the parameter cookie

[baconpaul]

per conversation in discussino #173, expand the cookie documentation to make it clear that it is required and provided by the plugin.

[robbert-vdh]

If supporting this is indeed supposed to be mandatory, then the parameter events should probably also be updated to more clearly indicate that the cookie field must be set to the same value previously provided by the plugin for that parameter.

[baconpaul]

If supporting this is indeed supposed to be mandatory, then the parameter events should probably also be updated to more clearly indicate that the cookie field must be set to the same value previously provided by the plugin for that parameter.

Supporting this is not mandatory, inasmuch as a host doesn't have to support the params extension.

But a host which supports the params extension which doesn't reflect cookies back to the plugin is not correct. It is a mandatory part of the params api.

I will update the event field to refer to the comment here. Good ideas!

[baconpaul]

Oh actually I don't have to do that. The cookie in the events already points at this documentation point.

include/clap/events.h:   void   *cookie;   // @ref clap_param_info.cookie
include/clap/events.h:   void   *cookie;   // @ref clap_param_info.cookie

[baconpaul]

oh and a way to convince yourself cookie is mandatory.

if it is optional and you have to code defensively you need to maintain two code paths: one with a cookie and one with the data structures that the cookie means you don't need. Since you have to do both, and that's error prone, then you are best to not assume the cookie is ever valid. Therefore we should remove the cookie from the spec.

Removing the cookie from the spec will make most of the plugins running to date quite a lot less efficient on every parameter update, which is undesirable.

Therefore the cookie should be mandatory is the best solution! Especially since host count << plugin count.

[robbert-vdh]

It indeed does already refer to the param_info struct, but it probably doesn't hurt to be extra specific about it. Especially since clap-juce-extensions dereferences the cookie pointer without checking for null pointers, so if the host forgets to set it any JUCE plugins using CJE will trigger UB.

[baconpaul]

It indeed does already refer to the param_info struct, but it probably doesn't hurt to be extra specific about it. Especially since clap-juce-extensions dereferences the cookie pointer without checking for null pointers, so if the host forgets to set it any JUCE plugins using CJE will trigger UB.

again: the clap juce extension has already checked if the cookie is null before it gives it to the host.

but yes if the host sends random values back to the plugins they will crash.

I'll update the doc to be more specific in place.

[baconpaul]

OK updated the documentation in the use points also.

[abique]

https://github.com/free-audio/clap-plugins/blob/main/plugins/core-plugin.cc#L454

The ref plugins uses a defensive approach and it is the right thing to do. The plugin must program this defensively. The only requirement would be for the host to initialize the cookie to null if it doesn't have it.

The defensive cost only apply if the cookie isn't set, otherwise it is a fast path which you can annotate with [[likely]]. It is easy enough and convenient to write a method:

MyParam *getParam(clap_id paramId, void *cookie) const noexcept
{
   if (cookie) [[likely]]
      return static_cast<MyParam*>(cookie);
   return findParamById(paramId);
}

If a plugin decides to entirely discard the cookie then it is its loss, but I would not do that.

[abique]

To add some context, you can consider a VST3 wrapper, which may not have the cookie.

[baconpaul]

wow.

well that's a really bad choice I think, but i'll go fix every plugin which uses JUCE

Why would the VST3 wrapper not maintain the cookie?

[baconpaul]

just FYI: Here's how the actual VST3 wrapper we have today keeps and handles the cookie.

https://github.com/defiantnerd/clap-wrapper/blob/7e77f069d9ac8bc2ac5f575d796079b9bf33ff39/src/detail/vst3/process.cpp#L282

[baconpaul]

The fact that, defacto, 80% of the clap plugins we have in production assume cookie is good also should weigh on our minds here I think. Are you 100% sure every plugin author in the world has to maintain a dual map just so some host does not?

[baconpaul]

basically - the defensive runtime cost is only if the cookie isn't set. The defensive dev and test time cost is real and high though. You are placing a high cost on every plugin.

Why even bother using cookies then? Save a lookup on a map you have to maintain and debug anyway? Basically "two code paths have to work perfectly but one of them is never called by any host you have but might be in the future" seems like a totally foot-pointing gun.

[robbert-vdh]

The fact that, defacto, 80% of the clap plugins we have in production assume cookie is good also should weigh on our minds here I think. Are you 100% sure every plugin author in the world has to maintain a dual map just so some host does not?

You still need this map for any other operation that works on parameter IDs. Like getting the text representation for a value. Or getting a parameter's current value.

[wrl]

@abique Is there a compelling reason this should be considered optional for the host? For example, does it add particular complexity to the host-side implementation?

[baconpaul]

Yes. The juce wrappers even have the ID for some of the juce internals

        jassert(paramPtrByClapID.find(info->id) != paramPtrByClapID.end());
        jassert(&paramPtrByClapID.find(info->id)->second == info->cookie);

the problem isn't coding it. The problem is requiring everyone to be defensive around this and only this part of the API

But that being said, i never thought. Is it guaranteed a host will call process with the param change events having an ID for an ID in our space? What other parts of my input event should I check for validity?

We can close this, though, if we instead add something like "A well behaved host will send valid parameter information for all events, except cookie, which might be null even if you advertised a cookie, so that, and only that, field in all parameter events needs to be checked and compensated for" or some such.

I don't feel good about that sentence. But if that's the spec lets put it in!

[abique]

Why would the VST3 wrapper not maintain the cookie?

I said may, so the impl is free to do whatever, with vst3, you don't have the cookie when you receive the parameter change, so either the wrapper has its own cache id->cookie or it just passes the id with null cookie. Both approach works.

The problem is requiring everyone to be defensive around this and only this part of the API

Plugins and Hosts must be programmed defensively. Even if the spec says that the cookie is mandatory, then there will be hosts which do not honor that aspect of the spec and if the plugin isn't written defensively it crashes, with all the bad consequences it can have (for the user) when the host doesn't have plugin sandboxing.

To say it differently, making it mandatory would not change your code, because you will still program it defensively:

• there will be plugins which crash with missing cookies
• there will be plugins which don't crash with missing cookies
• guess what the user will think?

Is it difficult for the host? I don't think so, but caching data is always more complicated than it seems. Validation plugins could strongly suggest to support the cookie.

If you want to simplify your QA by stripping one code path, then don't use the cookie at all and you have a single code path. There is no need to change the spec for that. And if it turns out that the benefits of using the cookie out-weight the cost of the QA then go for it.

[wrl]

The issue in my mind with just saying "code defensively" is that if the host is misbehaving with parameter cookies, it's possible that the host will not zero out the param event structures and leave the cookie set to uninitialised data, so simply checking against NULL/nullptr is not sufficient.

[sagantech]

Right now a host that ignores cookies, because they are optional, will crash inside certain existing plug-ins for no apparent reason.

[baconpaul]

Right I think this is a mistake, but so be it. I think we should write another PR which clarifies which parts of the spec are optional and which aren't. For instance do I have to be defensive around param id? Or is that going to be a parameter which I issued always?

My approach I'm currently thinking of is

1. Make the juce wrappers deal with the null case and associated performance degradation silently. I'll get to that next time I change them. @abique can you modify clap-host to have a mode where it ignores cookies optionally so I can test?
2. Patch surge so that it does an explicit std::terminate on a null cookie which tells that host developer to fix their host

I think it is a bit odd, honestly, that theres an object with 2^64 values, the plugin tells you the correct one, and then we expect either that one or another one, but none of the other 2^64-2 values, and call that coding defensively, and I think cookie being optional is a mistake, but only so much you can spend time on.

[baconpaul]

Actually @abique I added that mode to clap-host just now.

https://github.com/free-audio/clap-host/pull/25

I'll close this.

[wrl]

I think this issue should be reopened.

I think unless there is a compelling reason why hosts would have difficulty implementing this, it should be mandatory for the host. I am wholly unconvinced by the answer that the plugin should "code defensively" as a justification for why a host would opt not to implement it.

[abique]

I think this issue should be reopened.

I think unless there is a compelling reason why hosts would have difficulty implementing this, it should be mandatory for the host. I am wholly unconvinced by the answer that the plugin should "code defensively" as a justification for why a host would opt not to implement it.

So you won't program it defensively and you'll never back-out on that statement? You will take the time to investigate all plugin crashes with lesser known hosts you encounter and educate the host devs about the cookie? What if for some reasons, the host always sets the cookie to null, and no one can deploy a fix for the host because X and Y?

[baconpaul]

Why is it the cookie and only the cookie thst host devs can implement incorrectly Alex? That‘x what we don’t get.

[baconpaul]

Like when I get a param event with an id do I have to check that id is a valid param and code defensively to that?

or a million other examples

[baconpaul]

If cookie was a uint64_t it would look like the rest of the api. And the host can’t just set those to 0 if it feels like it can it?

[wrl]

If a host mistakenly leaves the param event struct partially uninitialised and cookie is not a valid pointer, there is no amount of defensive programming that will protect against this (unless the plugin is expected to walk its entire parameter space to verify the pointer, which completely defeats the point of the cookie in the first place).

If it is acceptable for the host to set it to NULL, then the documentation should be clear about this.

Agreeing with Paul that there are plenty of other places in the API where the host can misbehave. Passing through a pointer unmodified from the parameter info to param events seems like a simple operation.

Even if the plugin is coding defensively and checking for NULL, why not make this mandatory?

And, for the record, I do not use cookie in my plugins at all, but I do check to make sure the param_id maps to a valid parameter in the event the host has clobbered my IDs somehow.

[baconpaul]

I check cookie and id in debug asserts also. Just I turn those off at production time

[abique]

Why is it the cookie and only the cookie that host devs can implement incorrectly Alex? That‘x what we don’t get.

Host will have bugs and mis-understanding everywhere.

My point is that making it mandatory changes nothing in the plugin impl, because you'll have to be defensive (and you should be most of the time, see how clap-helper checks most input).

@wrl junk data is different. There is nothing you can do against a random cookie, or very little like rejecting a non-aligned pointer.

Yes the specification is not explicit about the null option, and this can be improved; at the same time if you read the documentation and take some time to think about it, it is easy to conclude that if the host can't provide the cookie, then it should set it to null.

[baconpaul]

But my view is either we make this mandatory (best) or we make an explicit comment thst it is ptional then update the test host and validator to do scans with the cookie blanked to null and go update all the plugins which assume it was mandatory. Which is all juce plugins at least.

[abique]

Yes we should clarify the doc.

[baconpaul]

Yes the specification is not explicit about the null option, and this can be improved; at the same time if you read the documentation and take some time to think about it, it is easy to conclude that if the host can't provide the cookie, then it should set it to null.

This is not easy to conclude at all! I concluded that if the host can’t fulfill the api it shouldn’t call the api. So clearly we should disambiguate the documentation to allow cookie and only cookie to have a special value if the host wants, but the host has to do everything else right.

[abique]

   // This value is optional and set by the plugin.
   // Its purpose is to provide a fast access to the plugin parameter:
   //
   //    Parameter *p = findParameter(param_id);
   //    param_info->cookie = p;
   //
   //    /* and later on */
   //    Parameter *p = (Parameter *)cookie;
   //
   // It is invalidated on clap_host_params->rescan(CLAP_PARAM_RESCAN_ALL) and when the plugin is
   // destroyed.
   void *cookie;

If the cookie is junk it will necessarily lead to a crash, so it can't be. And null is the common thing to check with pointers. But I get your confusion now, you saw it as optional for the plugin but not for the host.

The thing is that we have the param_id which is sufficient to address a param but it is not fast, hence the cookie but the cookie isn't required because we already have the param_id; the cookie is just an optional optimization. That's how it was designed and thought.

You can port your host to clap, and wire later on the cookie. This makes it easy to hack, because depending on the host complexity and number of layers (think Bitwig: java app <-> C++ engine <-> plugin host <-> plugin), keeping the cookie cache valid may not be trivial, so the host may want to skip that part first to advance quicker in the impl and deal with optimizations when the foundation is solid.

[wrl]

Seconding that I absolutely read the documentation as "optional for the plugin, mandatory for the host."

it is easy to conclude that if the host can't provide the cookie, then it should set it to null.

100% disagree.

[baconpaul]

Seconding that I absolutely read the documentation as "optional for the plugin, mandatory for the host."

it is easy to conclude that if the host can't provide the cookie, then it should set it to null.

100% disagree.

I read all parts of the spec as “mandatory unless indicated otherwise”. Are there other things which are optional and not marked optional Alex?

anyway we disagree but luckily the majority of plugins require cookie to work so it will end up defacto mandatory. If we can make clap host and the validator work with null cookies we can all go do whatever fix we think is appropriate. I think I can make surge pop a user facing dialog explaining why they should swap to a host which supports cookies even!

[baconpaul]

oh its also kinda funny that the example code in the spec isnt' defensive. Like, it does the same thing I do in juce which led to the question which led to this issue.

Luckily I have a popular enough clap where I can merge changes that I bet I can get people to implement the cookie anyway with a sufficiently irritating user facing message :)

[robbert-vdh]

For the record I am in favor of mandating that hosts implement the cookie. But I still think you should at least have a hard assertion there, or ignore the event, or look up the parameter from your hash map if the cookie is not set.

[baconpaul]

For the record I am in favor of mandating that hosts implement the cookie. But I still think you should at least have a hard assertion there, or ignore the event, or look up the parameter from your hash map if the cookie is not set.

To be painfully pedantic: you don't mean if the cookie is not set. You mean if the cookie is set to nullptr, right?

[robbert-vdh]

To be painfully pedantic: you don't mean if the cookie is not set. You mean if the cookie is set to nullptr, right?

Exactly. But yeah the param event struct docs should definitely still be updated to clearly indicate that setting the cookie field is either mandatory (which is probably the right call) or optional (in which case the host could set it to a null pointer).

[defiantnerd]

+1 for mandatory

I also strongly agree that a host should always preserve the parameter cookie for the plugin. And it does not even matter if it is a nullptr or not - the cookie should be preserved at all times.