MOD property notifications

[rerdavies]

Send additional PATCH_Set message when state is loaded, and when freshly created, in order to update MOD filenames.

[rerdavies]

Tested on Carla; not tested on MOD.

I have included changes that allow loading of state from factory presets, if you want to explore that. Search for isAbsolutePath() in nam_plugin.cpp if you don't feel it's necessary.

Contrary to falcTXx's advice, I'm still sending the stateChange notifications, which I think are correct, and are, at worse, harmless.

[mikeoliphant]

Thanks!

There are more code changes here than I was expecting - I'm going to need some time to make sure there are no unintended consequences.

I'm willing to merge to expedite if @falkTX signs off.

[falkTX]

honestly this doesnt seem like the best approach to me.
the file swap should never rely on class variables for one, and setting a property to true during instantiate feels wrong (for sure not needed).

since MOD wants to have this plugin soon I can dedicate some time for this tomorrow and do a review of the whole deal for patch-set / file changes.

[rerdavies]

Agreed. It's a bit sketchy, but it's a bit sketchy because you have a sketchy expectation that the plugin should send notifications without an explicit request to do so. That requires a comment, and hanging the comment on code in the initializer seems much better than putting in the header where it won't be seen.

The file swap does not rely on class variables. The filename is passed in the data written into the schedule request -- as it always has been. I changed setting of the class variable so that it gets set immediately as the request starts, rather than asynchronously when the load completes. That avoids a racy asynchronous delay between the time state is restored, and the time that the schedule request completes, for the benefit of hosts that do the right thing and use PATCH_Get to get the value, when and if UI attaches to the plugin,. instead of relying on unsolicited notification.

The specific race condition that needs to be avoided in the old code:

1. Plugin is initialized.
2. state is loaded.
3. Plugin starts running, which triggers the scheduler request.
4. Host attaches a UI, which does the right thing and issues a Patch_Get. to get the current value.
5. The plugin responds with the class variable which has not yet been updated.
6. The async load completes, and sets the class variable.

Result: the plugin has the wrong value.

I am firmly of the opinion that you need that change, in case you choose to fix MOD (or your MOD UI code) to do the right thing in future, as you run into more plugins that use path properties..

[rominator1983]

I confirm that my described loading issue of #31 does not occur with the code of this PR on Ubuntu.

Don't know about the MOD devices.

[falkTX]

Agreed. It's a bit sketchy, but it's a bit sketchy because you have a sketchy expectation that the plugin should send notifications without an explicit request to do so.

I do not think it is sketchy, the LV2 docs in https://lv2plug.in/ns/ext/state#StateChanged state:

Plugins SHOULD emit such an event whenever a change has occurred that would result in a different state being saved, but not when the host explicitly makes a change which it knows is likely to have that effect

so:

1. such event is for when it "result[s] in different state being saved", which is weird to apply this on a state load
2. the host knows that such event happened, as it is the host that triggered the state load (the host could even go deep as look into the project state and parse the parameters there, it is ttl data so it could be manually parsed if needed)

it works for now you in some hosts because you are abusing the state-changed notification, forcing hosts to reload plugin state.
on hosts such as carla that do not have a concept of "dirty state" this no longer works.

and in my opinion this "state-changed" is for hosts to know the plugin has internal "dirty" state and needs saving in order to fully preserve its state. as I said before on another ticket, it should not be plugged together with reloading of patch parameters as those are completely separate LV2 extensions anyhow.

The specific race condition that needs to be avoided in the old code:

1. Plugin is initialized.

2. state is loaded.

3. Plugin starts running, which triggers the scheduler request.

4. Host attaches a UI, which does the right thing and issues a Patch_Get. to get the current value.

5. The plugin responds with the class variable which has not yet been updated.

6. The async load completes, and sets the class variable.

Result: the plugin has the wrong value.

this is only an issue if the plugin does not report parameter changes after a state load.
when the plugin reports parameter values after it finishes loading a new file, the host will know about it and be in sync with the plugin.

it results in 2 notifications received by the host, but that is a minor thing. for the MOD case the loading of pedalboards happens in steps - 1st the plugin is loaded, then its state is restored, sometimes while the plugin is already active and running.
this is not really an issue in the end, and has been confirmed to work with all plugins so far that use LV2 files and patch parameters inside the MOD systems.

[rerdavies]

If you have a change to make, please propose it. The comment was made about a change that was made with respect to when the class's filename was updated. - The class filename used to be updated after the load completes, which causes problems, especially when implementing the MOD feature. Inherited behaviour. It had to be fixed. - And it now gets updated when state is restored (or a property set is received. Which is correct. Comments with respect to state change noted, and taken under advisement. For the moment, PiPedal requires it, so I'm going to leave it in, but I may propose a change to remove it in future. It seems risk-free. The host gets notified (perhaps redundantly) that the state has changed, when the state has changed. Further to previous comments made: - A state change is NOT send after a state restore (never was, and currently is not sent). - The background load does not use the value stored in the class variable. - Only one PATCH_Set gets sent. At no time does the host receive two notifications. As for as MOD's requirements go, MOD also knows when the state was restored and is perfectly capable of deciding whether a PATCH_Get should be sent to get the result. The variable to do that on load is set in the initializer, because I don't expect the MOD requirement to continue, and it needs an explicit comment to indicate that ONLY MOD needs that behavior. (Other hosts do not).

[falkTX]

my apologies, but I also found it was rude to agree on something and then a bit later say something else which invalidates a previous discussion.

my comment on the implementation takes into account the previous implementation, which in my opinion needs/needed to be better. so we ended up with some hacks, which is understandable.
I have a PR coming up very soon that hopefully makes this better, though of course could still have issues, will need some testing.

in general I just think for the lv2 workers they should never use class variables, only the final "work response" perhaps because it runs directly in sync with the audio and thus its execution can be thread-safe

I should have looked more into the code before commenting, only did so today for some cleanup.
and really makes me think that relying on the current/old implementation was not a good idea.

once the PR is up, feel free to ostracize it 😅

[rerdavies]

And my apologies as well. I have edited the previous response to tone it down a bit.

For the record, the LV2 worker does not (and has never) used the class variable. Please review the code. The worker thread uses the filename passed in the message package. The point at which the class variable is updated did change (and needed to be changed to implement the MOD requirements).

[falkTX]

The worker uses stagedModel which is a class variable, so maybe we are talking about different things?
The class variable usage comes from the previous implementation, not yours of course.
My comments were for the overall implementation so far, with old code + your PR.

[rerdavies]

Oh. Excellent point. My apologies.

[mikeoliphant]

Resolved via #34

@rerdavies thanks for your help on this. If the changes cause any problems on your end, please let me know.