Missing check for automation order of operations?


#1

I didn’t know how to title this, but basically:

in AudioProcessor the two begin/endParameterChangeGesture() methods have a block of code that checks for the consistency of their calling order (for example, no calling of begin twice without calling an end in between for the same parameter).

My question is: why isn’t the same checking code also present in sendParamChangeMessageToListeners(), to make sure that a proper begin has been previously called for the parameter being changed?

I think it would make sense to enforce such consistency, and for non-conformant code it is always possible to define the JUCE_DISABLE_AUDIOPROCESSOR_BEGIN_END_GESTURE_CHECKING macro anyway.

EDIT: just to be clear, I’m referring to the following code:

void AudioProcessor::sendParamChangeMessageToListeners (const int parameterIndex, const float newValue)
{ 
    if (isPositiveAndBelow (parameterIndex, getNumParameters()))
    {

    /* yfede: These 3 lines below are those I think should be added */
    #if JUCE_DEBUG && ! JUCE_DISABLE_AUDIOPROCESSOR_BEGIN_END_GESTURE_CHECKING
            jassert (changingParams [parameterIndex]);
    #endif

        for (int i = listeners.size(); --i >= 0;)
            if (AudioProcessorListener* l = getListenerLocked (i))
                l->audioProcessorParameterChanged (this, parameterIndex, newValue);
    }
    else
    {
        jassertfalse; // called with an out-of-range parameter index!
    }
}

#2

What if you’re loading a saved state or a preset? In this case you don’t have any gestures and your code will trigger an assertion.


#3

I don’t agree: In my opinion when loading a previously saved preset, the plug-in should signal to the host the fact that it is updating all those parameters by calling begin/end for each.

Otherwise, there are some hosts that will simply ignore the new loaded values and will internally keep the previous value for each automated parameter, essentially going out of sync with what is displayed on the interface


#4

The AudioParameterValueTreeState code doesn’t start and end a gesture when restoring parameter values (via setParameterNotifyingHost, which triggers sendParamChangeMessageToListeners). We’ve not had any bug reports to suggest this is failing anywhere, but perhaps we have been lucky.

Otherwise JUCE users are free to call setParameterNotifyingHost as they wish. I’m a bit concerned that imposing a debug check that each call is sandwiched between begin/endParameterChangeGesture may break otherwise valid code, but it sounds like it would be a positive step for full cross-host compatibility.


#5

@yfede could you please provide an example where restoring parameter values from a saved state without gestures fails? I’ve done a fair bit of testing with AudioParameterValueTreeState and everything appears to work correctly.


#6

I’m a little busy at the moment but I will certainly follow up your request when I get time.

Can you point me to the most appropriate example project in JUCE to test with? I don’t know in which of those the AudioParameterValueTreeState is used


#7

Thank you!

The tutorial code is the most battle hardened: https://www.juce.com/doc/projects/tutorial_audio_processor_value_tree_state.zip


#8

Ok I have managed to reproduce this on Mac with Logic Pro X.

First, I have added a Button to the tutorial code (download the changed file here: GainEditor.h).

The button stores the initial state of the plug-in with a getStateInformation() upon construction, then each time it is pressed, it restores it back into the plug-in with a setStateInformation(), effectively recalling the starting condition.

Then, to recreate the setup I have in the video linked below, I did the following:

  • Start Logic
  • Create an Audio track
  • Insert there the plug-in
  • show automation envelopes (press the ‘A’ key on the keyboard)
  • set automation mode to ‘Latch’, to easily record all automation movements
  • display the two automation lanes for Gain and Invert Phase parameters (which at the beginning will appear empty)

At this point I started recording the video and showed that, during playback of the session:

  • If you act on the widgets on the GUI, the changes will be correctly recorded also in the automation lanes
  • If, at any point, you click on the “Recall” button to bring both parameters back to their default values (obviously you should do that when they don’t match their initial values) you will see that those changes are not properly recorded in the automation lanes

In my opinion, this proves that those begin/end gesture notifications are required at all times, even when restoring state with setStateInformation(), and my proposed additional jassert above should help in enforcing that.

https://dl.dropboxusercontent.com/u/591622/automation%20issue.mov


#9

Thank you for that.

The setStateInformation and getStateInformation functions are designed for the host to call, rather than the plug-in. When they’re called by the host (when restoring a saved state or loading a preset, for example) then the host knows that parameter changes are expected and should handle them correctly. If you really want to (ab)use set/getStateInformation like this then you should also call updateHostDisplay to let the host know that something special has occurred.

When I was testing I was looking for errors in host-initiated actions, but I didn’t find any.

Apologies if I seem overly conservative, but there’s a real danger of dropping assertions into otherwise valid code here. Gestures are used to indicate to the host that sequential parameter changes constitute a single (usually undoable) user action, and I think there are a few places where they’re not mandatory.


#10

Well, that may be the case regarding the setStateInformation and getStateInformation functions, but I didn’t mean to intend that I use them like that in actual production code. Mine was just a quick and dirty hack on top of the tutorial code to show the faulty behavior that happens when updating parameters in bulk without surrounding them with proper begin/end gesture callbacks

In fact, I am still able to reproduce the problem using that button I added to trigger the undo feature of the AudioProcessorValueTreeState. I believe that to have a button on the GUI that does a step back in the undo history is perfectly legitimate, but still that fails to update the automation recorded in Logic, as shown below.

This is the code:

GainProcessor.cpp (5.8 KB)
GainEditor.h (3.5 KB)

and below is the video:

https://dl.dropboxusercontent.com/u/591622/automation%20issue%20undo.mov


#11

I agree with @t0m here. if you are updating multiple parameters at once then this is not really a gesture of any one slider. You should be calling AudioProcessor::updateHostDisplay in those situations and which will inform the host to update the automation data correctly.