Fix VST parameter automation performance

[messmerd]

Fixes #8066

Before	After
https://github.com/user-attachments/assets/d9aa0f04-0cb7-4e08-b8b5-364bf7b29544	https://github.com/user-attachments/assets/e90050fc-cb89-464a-826d-1688e49388c9

The cause

Every time any VST parameter was modified, Vestige and VST effects would update the parameter labels and display text for all of their parameters.

This is bad because the parameter labels/display are only used by the text floats for parameter knobs, and exactly 0 or 1 text floats for knobs are visible at any given time globally. So it was making expensive calls into RemotePlugin to update the text for all parameters, even though this is only needed for at most 1 parameter and most of the time is not needed at all.

This performance regression was introduced by #5321, as @sakertooth explained here.

A solution

• I added RemoteVstPlugin op codes for updating a single parameter label or display text rather than all of them
• Removed some unnecessary string copies in RemotePlugin
• Refactored FloatModelEditorBase, replacing the displayValue() method with two methods for setting/updating the value of the SimpleTextFloat:

  virtual QString getCustomFloatingText();
  virtual std::optional<QString> getCustomFloatingTextUpdate();

  What these do is explained in the comments.
• Added VstPluginKnob class which implements Knob for Vestige/VST Effects.
  • This knob implementation limits the rate of updates to the floating text. Without rate limiting, the CPU spikes when the text float is visible and the parameter's value is changing rapidly, so I had to add this in. (Note: This solves a performance issue specific to RemotePlugin, not something that affects other knobs, so putting it in a Knob implementation for VSTs is appropriate.)
  • Uses the new RemoteVstPlugin op codes to only update a specific parameter's text rather than all of them
• Removed CustomTextKnob since a superset of its functionality is now supported by Knob as well as every other class that inherits from FloatModelEditorBase.

Bonus:

• I moved the weird volume knob hacks from FloatModelEditorBase into a new class called VolumeKnob which inherits from Knob. This isn't strictly necessary for this PR, but it makes FloatModelEditorBase significantly smaller in size (and therefore Knob as well). These changes are self-contained in this commit: 1e5ca51

[sakertooth]

Why the complication with the timers, Push/Pull mode and all? Isn't it just "when this knob's value changes, update its tooltip by sending a message and then using the updated string as the tooltip for the knob that was returned"? Though this does make stuff like automation less efficient since its rapidly changing (instead of the polling approach you used), though I wonder if that could suffice if only one tooltip is being updated, not for all the knobs. I feel like with a better IPC system the solution would be straightforward (if needed at all), but we're not there yet.

And I don't get why we have a separate SimpleTextFloat class (which I am guessing is the tooltip for the knob)?

[messmerd]

@sakertooth I agree the design was pretty ugly and convoluted. I just finished refactoring the PR, and now it's much cleaner and more straightforward. I no longer have any unease about the design.

Why the complication with the timers

Isn't it just "when this knob's value changes, update its tooltip by sending a message and then using the updated string as the tooltip for the knob that was returned"?

When I tested it without a timer to limit the rate of updates, it caused the CPU to spike whenever I rapidly adjusted the value of a parameter while the text float was visible. This is even after all the other optimizations. But with some simple rate limiting using a timer, there's no such problem.

Before I reworked the PR, the timer stuff was cluttering up the FloatModelEditorBase base class used by all knobs, but now it has been moved to a new class VstPluginKnob used only by VSTs, which is a much better place for it.

[sakertooth]

@messmerd, nice, I'll try to review again when I have time.

When I tested it without a timer to limit the rate of updates, it caused the CPU to spike whenever I rapidly adjusted the value of a parameter while the text float was visible. This is even after all the other optimizations. But with some simple rate limiting using a timer, there's no such problem.

Isn't this a bandage fix (and if so, should we make note of that)? Rate limiting wouldn't be necessary if the messages to update the parameters were sent on another queue for communication between the UI and the plugin, such that the audio thread wouldn't be blocked on messages it doesn't care about, right? Perhaps this is a more suitable approach to solving this problem, but if its too much to ask for here, we shouldn't need to worry much about the ideal.

[messmerd]

@sakertooth
There should definitely be separate channels for main/GUI thread messages and audio thread messages. For the audio thread, there should be a message queue since time-stamped events need to be pushed onto a queue during each processing period then sent to the plugin to read all at once during the process method.

For main/GUI thread messages, such as the messages for updating parameter text that this PR uses, maybe there shouldn't be a queue and the messages should just be sent synchronously as soon as possible. Of course, main/GUI thread messages and audio thread messages would be sent concurrently and not interfere with each other.

For the parameter text updates, it's different from other more important messages because it happens regularly and there is no significant penalty if it fails, so it makes sense to me to place a limit on how often it can be called. Even after an IPC rewrite, I think rate limiting could still be good for performance. Whether the rate limiting should be implemented in a generic fashion in RemotePlugin or AutomatableModel in future, I'm not sure, but if that happens, it will require some more thought and would have to happen during/after the IPC rewrite. And who knows, maybe after the IPC rewrite, performance would be so good that rate limiting won't be worth having due to the extra complexity. I don't know.

But for now, rate limiting for VST parameter text updates makes a noticeable improvement to performance, and the implementation is confined to VstPluginKnob, so nothing else depends on it and it's not difficult to remove if we ever decide to do so.

[sakertooth]

Tested it with some VST plugins. The CPU meter barely budged! This is awesome.

[sakertooth]

I was thinking all the rate limiting/timer logic would be in VstPluginKnob.

[messmerd]

This isn't related to rate limiting - it just makes the floating text visible and starts the timer to hide it if it hasn't already been started. I added this method while refactoring how FloatModelEditorBase uses SimpleTextFloat, but it may not be necessary anymore. I'll have to check.

[sakertooth]

On a separate note, a part of me questions why the code isn't just a simple call to setTooltip (i.e., why the SimpleTextFloat classes and such). I don't spend too much time in this part of the codebase, but it feels different than from what I was expecting.

[messmerd]

Our SimpleTextFloat allows specifying both a delay before the tooltip is shown and a display duration, while setToolTip/QToolTip only supports a display duration. And looking at QToolTip's documentation, it looks like it would be more difficult to adapt to our use cases than SimpleTextFloat.

One thing I don't like about SimpleTextFloat though, is how it's not a static class. We should probably look into changing that in a separate PR. And maybe rename it to ToolTip.

[messmerd]

I just realized this PR also fixes a bug where the tooltip text displayed by a knob when you hover over it without clicking it remains static and unchanging even when the knob is being automated or controlled.