VST3 non optimal calls #9619

[bonsembiante]

Follow issue tracking and context in: https://tracker.ardour.org/view.php?id=9619

[x42]

I am not really a fan of adding the the value to the Changed signal here. That is a huge commitment, particularly since it is unused except for VST3, and basically a workaround for yabrdge/VST3 only.

Do you think we could instead cache that value on VST3Plugin or the implementation VST3PI for a quick lookup instead?

[bonsembiante]

Hey Robin! Thanks for your answer.

Sure, I could try another approach, I thought that this was the best way because it seems logic to me that the value created with the event goes with the signal.

Regarding caching the value, how does it come to your mind?
One way could be using a sort of stack or queue, but how could it be mantained? Delete the value after one read? What would happend to the other calls to "get_value" caused by the same event? They will lose the chance to get a cached value.

Another way could be caching the value with a timestamp, but I'm thinking that it could be too complex for something like this.

Or caching it during the performEdit until the next beginEdit, but maybe there is a time when we need genuinely fetch the actual value through "get_value" and I could get a cached old one.

What do you think?

Also, which are the cons for passing the value in the signal?

Regards

[bonsembiante]

This is an old comment that kept as Pending. You can discard it
I'm thinking that caching the value in a queue/stack as the "_update_ctrl" vector could be a good option. After read a cached value it is removed from the stack, and next time "get_value" is called we fetch the value as always. It shouldn't be a problem because the signal emitted by the Changed from a performEdit only creates one unnecessary call to the VST plugin. It could be fine.
I will do the change.

[bonsembiante]

This is an old comment that kept as Pending. You can discard it
I did some tests and yes, it works, but because every performEdit ends triggering 3 calls to "get_value", caching the value and "uncaching" it after the next read only save us only 1 call to normalizedToPlain.

Do you know if it will be good to mantain the value cached during all the edit? I mean, save it in the cache when the "beginEdit" call comes, and delete it at the "endEdit" call. I think that way could cover the rest of the cases but I don't know how could that impact in the plugin side. Also, in my analysis of the issue I detected calls to normalizedToPlain after the "endEdit" call, and in these cases the cache option wouldn't be right, because the "endEdit" arrives, we remove the value from the cache queue, but some Changed signal handler is still being executed and it won't reach a cached value, still creating unnecessary calls to the plugin.

[x42]

Also, which are the cons for passing the value in the signal?

The purpose of the signal is to just inform others that something has changed. That is semantically different from passing a ValueChange. Also, when an optional value is passed, all API users (notably control surfaces) will need to check if its boost::none, and provide a fall back. Then the value may change more rapidly than a given UI can update.

Regarding caching the value, how does it come to your mind?

The basic idea I had is to cache the normalized value at the point when the Changed() signal is emitted (here VST3PI::ParamValueChanged). Then return the last cache value in VST3PI::get_parameter.

As for the the implementation my first thought that for every call
_shadow_data[p] = value;
we can add
_shadow_data_plain[p] = normalizedParamToPlain (id, _shadow_data[p]);

--

Then again for other plugins (some have 1000+ controls) this approach may add a bit of load if a given value is never used.

[bonsembiante]

(I did this comment a few days ago and it kept in Pending. Also found that other older comments never got published)

Hi Robin! Again, thank you very much for your time reviewing this PR, beside all your effort mantaining Ardour.

Yes, that was actually what I tried. And this cache aproach still gives us two unnecessary calls to "normalizedToPlain" (or actually Control::get_value, because I'm doing my tests with a VST3 but the unnecesarry calls are being made to "get_value" method).

I was making some more tests to understand the reason around this extra calls with the cache solution and I found new information. In my tests, when 1 Changed signal is emitted, 3 handlers are called (so only the first handler can use the cached value and the other 2 fetch the value again).

When I change a param in the plugin GUI, this is approximalety the sequence of calls:

- There is a change in the plugin GUI
- OnParameterChange signal is emitted
- > VST3Plugin::parameter_change_handler handles OnParameterChange
- > it calls Plugin::parameter_changed_externally
- > Plugin::ParameterChangedExternally signal is emitted
- > > PluginInsert::parameter_changed_externally handles Plugin::ParameterChangedExternally
- > > it ends calling AutomationControl::actually_set_value
- > > Controllable::Changed signal is emitted
- > > > ProcessorEntry::Control::control_changed handles Controllable::Changed signal at least 3 times
- > > > Control::get_value is called for each signal handling.

I think that the main reason is that ProcessorEntry::Control::control_changed is registered to handling c->Changed signal more than one time because there are different instances of ProcessorEntry::Control.

So I think that we can't cache the value because for one Changed signal emission could be more than one handler.

With this additional information, now I think that passing the value through the signal have even more sense, because each handler (that could be "n" qty of handlers) will recieve the value that cause that signal.

I would like to add that my decision to add an argument in the signal emission was based on other signals that also pass the value like OnParameterChange and ParameterChangedExternally, just to mention the closest examples. So I don't find it as a bad pattern.

Also, when doing a signal connection and registering a handler function, it is specified which signal arguments will be used, and if anyone uses this 3rd argument, this fix will be completely transparent for those cases.

What do you think?

I'll wait for your comments. Have a great day.

[bonsembiante]

Hey robin! How are you?
I was thinking that maybe we can skip the changes in OnChanged if you are not convinced to merge it, and at least fix the other unnecessary calls. What do you think?

[x42]

Sorry for not replying sooner. Somehow this dropped below my radar.

So just change the VST3 OnParameterChange signal to pass along the value (and cache it)?
That sounds reasonable.