Merge pull request #191 from Tracktion/dev/discrete-params-validation-b488be

Add a discrete parameter state restoration test and fix false failures in the existing one

Author: drowaudio

.github/workflows/acceptance.yml

@@ -156,7 +156,7 @@ jobs:
         shell: bash
         run: |
           check() { echo "$1: got '$2', expected '$3'"; [ "$2" = "$3" ] || exit 1; }
-          check "validate passed"            "${{ steps.validate.outputs.passed }}"           3
+          check "validate passed"            "${{ steps.validate.outputs.passed }}"           4
           check "validate failed"            "${{ steps.validate.outputs.failed }}"           0
           check "validate-missing outcome"   "${{ steps.validate-missing.outcome }}"          failure
           check "validate-missing passed"    "${{ steps.validate-missing.outputs.passed }}"   1

.github/workflows/build.yaml

@@ -128,7 +128,9 @@ jobs:
         shell: bash
         run: |
           # Paths must be single quoted for bash not to escape the Windows backslash character \ used in absolute paths
-          ${{ env.APP_DIR }}/${{ matrix.test-binary }} validate --strictness-level 10 '${{ env.PLUGIN_CACHE_PATH }}/DSPModulePluginDemo_artefacts/Release/VST3/DSPModulePluginDemo.vst3'
+          # "Discrete parameter state restoration" is disabled for DSPModulePluginDemo until juce::AudioParameterBool::setValue
+          # snaps to 0 or 1: https://forum.juce.com/t/pluginval-can-set-an-audioparameterbools-to-odd-values/50971
+          ${{ env.APP_DIR }}/${{ matrix.test-binary }} validate --strictness-level 10 --disabled-tests "Discrete parameter state restoration" '${{ env.PLUGIN_CACHE_PATH }}/DSPModulePluginDemo_artefacts/Release/VST3/DSPModulePluginDemo.vst3'
           ${{ env.APP_DIR }}/${{ matrix.test-binary }} validate --strictness-level 10 '${{ env.PLUGIN_CACHE_PATH }}/MultiOutSynthPlugin_artefacts/Release/VST3/MultiOutSynthPlugin.vst3'
 
       - name: Validate JUCE Plugin examples (AU)
@@ -143,7 +145,7 @@ jobs:
           killall -9 AudioComponentRegistrar # kick the AU registrar
           # Intentionally uses the deprecated "--validate" flat flag to keep the
           # backwards-compatible alias covered (this step is continue-on-error).
-          ${{ env.APP_DIR }}/${{ matrix.test-binary }} --strictness-level 10 --validate  ~/Library/Audio/Plug-Ins/Components/DSPModulePluginDemo.component
+          ${{ env.APP_DIR }}/${{ matrix.test-binary }} --strictness-level 10 --disabled-tests "Discrete parameter state restoration" --validate  ~/Library/Audio/Plug-Ins/Components/DSPModulePluginDemo.component
 
       - name: Codesign (macOS)
         if: ${{ matrix.name == 'macOS' }}

CHANGELIST.md

@@ -17,6 +17,10 @@
 - **Breaking:** invalid option values now error instead of silently falling back (e.g. `--rtcheck banana`, `--strictness-level abc`)
 - **Breaking:** falsey flag environment variables now mean off (e.g. `SKIP_GUI_TESTS=0` no longer enables skipping)
 - `--help` output is now generated by CLI11
+- Added a "Discrete parameter state restoration" test (strictness 8) which checks a discrete parameter has the same value after restoring a state, whatever value it held beforehand. This fails for parameters that keep the raw normalised value from the host instead of mapping it to a step (e.g. `juce::AudioParameterBool`)
+- Fixed false failures in the "Plugin state restoration" test where the first parameter was compared against the last value sent by a previous test rather than the plugin's own value
+- Below strictness level 8, two-step (on/off) parameters are now only sent 0 or 1 when tests randomise parameter values. Arbitrary values are still sent to them at level 8 and above, and to all other parameters at every level
+- Fixed the "Plugin state restoration" test's exact binary state comparison never running. It was documented as running at strictness level 8 and above but checked the test's own level instead of the run's
 - Enabled the editor stress and extreme (real-time allocation / oversized block) tests that were present in the source tree but had not been compiled into the build
 
 ### 1.0.5

CLAUDE.md

@@ -117,7 +117,8 @@ pluginval/
 │   ├── test_plugins/         # Test plugin files
 │   │   ├── tone_generator/   # Deterministic dogfood generator (PLUGINVAL_BUILD_TEST_PLUGINS)
 │   │   ├── gain/             # Deterministic dogfood gain effect (for the input.audio path)
-│   │   └── playhead_probe/   # Writes the host transport to output (for the playhead path)
+│   │   ├── playhead_probe/   # Writes the host transport to output (for the playhead path)
+│   │   └── discrete_params/  # Bool/choice-only plugin (for the discrete parameter state test)
 │   ├── acceptance/           # Acceptance self-tests: configs (*.json.in), inputs/, checked-in refs/ WAVs
 │   ├── mac_tests/            # macOS-specific tests
 │   └── windows_tests.bat     # Windows test scripts
@@ -379,7 +380,7 @@ struct Requirements {
 
 | File | Tests Included |
 |------|----------------|
-| `BasicTests.cpp` | PluginInfo, Programs, Editor, AudioProcessing, PluginState, Automation, auval, VST3validator |
+| `BasicTests.cpp` | PluginInfo, Programs, Editor, AudioProcessing, PluginState, DiscreteParameterState, Automation, auval, VST3validator |
 | `BusTests.cpp` | Bus layout, channel configuration |
 | `EditorTests.cpp` | Editor creation, resizing |
 | `ParameterFuzzTests.cpp` | Random parameter value testing |
@@ -443,6 +444,7 @@ static MyNewTest myNewTest;
 
 Located in `TestUtilities.h`:
 - `getNonBypassAutomatableParameters()` - Get automatable params
+- `getRandomParameterValue()` - Random value to set on a param (only 0 or 1 for two-step params below strictness 8)
 - `fillNoise()` - Fill buffer with random audio
 - `countNaNs()`, `countInfs()`, `countSubnormals()` - Audio validation
 - `ScopedEditorShower` - RAII editor creation/destruction

CMakeLists.txt

@@ -289,5 +289,6 @@ if (PLUGINVAL_BUILD_TEST_PLUGINS)
     add_subdirectory(tests/test_plugins/tone_generator)
     add_subdirectory(tests/test_plugins/gain)
     add_subdirectory(tests/test_plugins/playhead_probe)
+    add_subdirectory(tests/test_plugins/discrete_params)
     add_subdirectory(tests/acceptance)
 endif()

source/TestUtilities.h

@@ -53,6 +53,23 @@ static inline juce::Array<juce::AudioProcessorParameter*> getNonBypassAutomatabl
     return parameters;
 }
 
+/** Returns a random normalised value to set on a parameter.
+
+    Below strictness level 8, two-step (on/off) parameters are only sent 0 or 1.
+    Any value is valid, but some plugins keep the values in between rather than
+    mapping them to a step, which the state restoration tests then pick up.
+*/
+static inline float getRandomParameterValue (juce::Random& r, const juce::AudioProcessorParameter& parameter, int strictnessLevel)
+{
+    // N.B. hosted VST3 parameters don't report isBoolean so also check the number of steps
+    const bool isTwoStep = parameter.isBoolean() || parameter.getNumSteps() == 2;
+
+    if (isTwoStep && strictnessLevel < 8)
+        return r.nextBool() ? 1.0f : 0.0f;
+
+    return r.nextFloat();
+}
+
 //==============================================================================
 template<typename UnaryFunction>
 void iterateAudioBuffer (juce::AudioBuffer<float>& ab, UnaryFunction fn)

source/tests/BasicTests.cpp

@@ -347,7 +347,7 @@ struct PluginStateTest  : public PluginTest
 
         // Set random parameter values
         for (auto parameter : getNonBypassAutomatableParameters (instance))
-            parameter->setValue (r.nextFloat());
+            parameter->setValue (getRandomParameterValue (r, *parameter, ut.getOptions().strictnessLevel));
 
         // Restore original state
         callSetStateInformationOnMessageThreadIfVST3 (instance, originalState);
@@ -378,13 +378,17 @@ struct PluginStateTestRestoration   : public PluginTest
         // Read state
         auto originalState = callGetStateInformationOnMessageThreadIfVST3 (instance);
 
+        // Hosted parameters report the last value sent to them until a state is
+        // restored, so restore once here to start from the plugin's own values
+        callSetStateInformationOnMessageThreadIfVST3 (instance, originalState);
+
         const auto tolaratedDiff = 0.1f;
 
         // Set random parameter values
         for (auto parameter : getNonBypassAutomatableParameters(instance))
         {
 			const auto originalValue = parameter->getValue();
-            parameter->setValue(r.nextFloat());
+            parameter->setValue (getRandomParameterValue (r, *parameter, ut.getOptions().strictnessLevel));
 
             // Restore original state
             callSetStateInformationOnMessageThreadIfVST3(instance, originalState);
@@ -394,7 +398,7 @@ struct PluginStateTestRestoration   : public PluginTest
                 parameter->getName(1024) + juce::String(" not restored on setStateInformation"));
         }
 
-        if (strictnessLevel >= 8)
+        if (ut.getOptions().strictnessLevel >= 8)
         {
             // Read state again and compare to what we set
             auto duplicateState = callGetStateInformationOnMessageThreadIfVST3 (instance);
@@ -417,6 +421,67 @@ struct PluginStateTestRestoration   : public PluginTest
 static PluginStateTestRestoration pluginStateTestRestoration;
 
 
+//==============================================================================
+/**
+    Tests that a discrete parameter reports the same value after a state restore
+    regardless of the value it held beforehand.
+
+    A host can send any normalised value to a discrete parameter and only the
+    plugin knows which step that maps to. If the plugin keeps the raw value for
+    a single parameter change but saves/restores the step, the value it reports
+    depends on how it got there.
+    See https://forum.juce.com/t/pluginval-can-set-an-audioparameterbools-to-odd-values/50971
+*/
+struct DiscreteParameterStateTest   : public PluginTest
+{
+    DiscreteParameterStateTest()
+        : PluginTest ("Discrete parameter state restoration", 8)
+    {
+    }
+
+    void runTest (PluginTests& ut, juce::AudioPluginInstance& instance) override
+    {
+        auto r = ut.getRandom();
+
+        for (auto parameter : getNonBypassAutomatableParameters (instance))
+        {
+            if (! parameter->isDiscrete())
+                continue;
+
+            // Any normalised value is valid so use ones that are unlikely to be on a step
+            const auto value = 0.1f + 0.8f * r.nextFloat();
+            const auto otherValue = value < 0.5f ? value + 0.4f : value - 0.4f;
+
+            parameter->setValue (value);
+            const auto state = callGetStateInformationOnMessageThreadIfVST3 (instance);
+
+            // Restore the state with the parameter still at the value it was saved with
+            callSetStateInformationOnMessageThreadIfVST3 (instance, state);
+            const auto restoredFromValue = parameter->getValue();
+
+            // Then restore it again from a different value
+            parameter->setValue (otherValue);
+            callSetStateInformationOnMessageThreadIfVST3 (instance, state);
+            const auto restoredFromOtherValue = parameter->getValue();
+
+            ut.logVerboseMessage (parameter->getName (1024) + ": set " + juce::String (value) + ", restored as " + juce::String (restoredFromValue)
+                                  + "; set " + juce::String (otherValue) + ", restored as " + juce::String (restoredFromOtherValue));
+
+            ut.expectWithinAbsoluteError (restoredFromOtherValue, restoredFromValue, 0.0001f,
+                                          parameter->getName (1024) + " has a different value after restoring the same state");
+        }
+    }
+
+    std::vector<TestDescription> getDescription (int) const override
+    {
+        return { { name, "For each discrete param: sets a random value, saves state, then restores that state twice - "
+                         "once unchanged and once after setting a different value - and expects the same value both times" } };
+    }
+};
+
+static DiscreteParameterStateTest discreteParameterStateTest;
+
+
 //==============================================================================
 struct AutomationTest  : public PluginTest
 {
@@ -474,7 +539,7 @@ struct AutomationTest  : public PluginTest
                         for (int i = 0; i < juce::jmin (10, parameters.size()); ++i)
                         {
                             const int paramIndex = r.nextInt (parameters.size());
-                            parameters[paramIndex]->setValue (r.nextFloat());
+                            parameters[paramIndex]->setValue (getRandomParameterValue (r, *parameters[paramIndex], ut.getOptions().strictnessLevel));
                         }
                     }
 
@@ -552,7 +617,7 @@ struct EditorAutomationTest : public PluginTest
         while (--numBlocks >= 0)
         {
             for (auto parameter : parameters)
-                parameter->setValue (r.nextFloat());
+                parameter->setValue (getRandomParameterValue (r, *parameter, ut.getOptions().strictnessLevel));
 
             ut.resetTimeout();
             juce::Thread::sleep (10);
@@ -702,7 +767,7 @@ struct BackgroundThreadStateTest    : public PluginTest
 
         // Set random parameter values
         for (auto parameter : parameters)
-            parameter->setValue (r.nextFloat());
+            parameter->setValue (getRandomParameterValue (r, *parameter, ut.getOptions().strictnessLevel));
 
         // Restore original state
         callSetStateInformationOnMessageThreadIfVST3 (instance, originalState);
@@ -739,6 +804,7 @@ struct ParameterThreadSafetyTest    : public PluginTest
         auto parameters = getNonBypassAutomatableParameters (instance);
         const bool isPluginInstrument = instance.getPluginDescription().isInstrument;
         const int numBlocks = 500;
+        const int runStrictnessLevel = ut.getOptions().strictnessLevel;
 
         // This emulates the plugin itself setting a value for example from a slider within its UI
         juce::MessageManager::callAsync ([&, threadRandom = r]() mutable
@@ -747,7 +813,7 @@ struct ParameterThreadSafetyTest    : public PluginTest
 
                                        for (int i = 0; i < numBlocks; ++i)
                                            for (auto param : parameters)
-                                               param->setValueNotifyingHost (threadRandom.nextFloat());
+                                               param->setValueNotifyingHost (getRandomParameterValue (threadRandom, *param, runStrictnessLevel));
 
                                        endWaiter.signal();
                                    });
@@ -777,7 +843,7 @@ struct ParameterThreadSafetyTest    : public PluginTest
                 addNoteOff (mb, noteChannel, noteNumber, 0);
 
             for (auto param : parameters)
-                param->setValue (r.nextFloat());
+                param->setValue (getRandomParameterValue (r, *param, runStrictnessLevel));
 
             fillNoise (ab);
 

source/tests/ParameterFuzzTests.cpp

@@ -44,7 +44,7 @@ struct FuzzParametersTest  : public PluginTest
 
         for (int i = 0; i < 5; ++i)
         {
-            const float value = r.nextFloat();
+            const float value = getRandomParameterValue (r, parameter, ut.getOptions().strictnessLevel);
 
             parameter.setValue (value);
             const float v = parameter.getValue();

tests/test_plugins/discrete_params/CMakeLists.txt

@@ -0,0 +1,37 @@
+# A minimal plugin with only discrete (bool / choice) parameters, used to
+# dogfood the "Discrete parameter state restoration" test.
+# Built only when PLUGINVAL_BUILD_TEST_PLUGINS is ON.
+
+set(discrete_params_formats VST3)
+
+if (APPLE)
+    list(APPEND discrete_params_formats AU)
+endif()
+
+juce_add_plugin(pluginval_discrete_params
+    PRODUCT_NAME                "pluginval Discrete Params"
+    COMPANY_NAME                Tracktion
+    BUNDLE_ID                   com.Tracktion.pluginvalDiscreteParams
+    PLUGIN_MANUFACTURER_CODE    Trkt
+    PLUGIN_CODE                 Pdp1
+    FORMATS                     ${discrete_params_formats}
+    IS_SYNTH                    FALSE
+    NEEDS_MIDI_INPUT            FALSE
+    COPY_PLUGIN_AFTER_BUILD     FALSE)
+
+target_sources(pluginval_discrete_params PRIVATE
+    DiscreteParamsPlugin.cpp
+    DiscreteParamsPlugin.h)
+
+target_compile_features(pluginval_discrete_params PRIVATE cxx_std_20)
+
+target_compile_definitions(pluginval_discrete_params PRIVATE
+    JUCE_WEB_BROWSER=0
+    JUCE_USE_CURL=0
+    JUCE_VST3_CAN_REPLACE_VST2=0)
+
+target_link_libraries(pluginval_discrete_params PRIVATE
+    juce::juce_audio_utils
+    juce::juce_audio_plugin_client
+    juce::juce_recommended_config_flags
+    juce::juce_recommended_warning_flags)

tests/test_plugins/discrete_params/DiscreteParamsPlugin.cpp

@@ -0,0 +1,65 @@
+/*==============================================================================
+
+  Copyright 2018 by Tracktion Corporation.
+  For more information visit www.tracktion.com
+
+   You may also use this code under the terms of the GPL v3 (see
+   www.gnu.org/licenses).
+
+   pluginval IS PROVIDED "AS IS" WITHOUT ANY WARRANTY, AND ALL WARRANTIES, WHETHER
+   EXPRESSED OR IMPLIED, INCLUDING MERCHANTABILITY AND FITNESS FOR PURPOSE, ARE
+   DISCLAIMED.
+
+ ==============================================================================*/
+
+#include "DiscreteParamsPlugin.h"
+
+//==============================================================================
+DiscreteParamsProcessor::DiscreteParamsProcessor()
+    : juce::AudioProcessor (BusesProperties().withInput  ("Input",  juce::AudioChannelSet::stereo(), true)
+                                             .withOutput ("Output", juce::AudioChannelSet::stereo(), true))
+{
+    for (int i = 1; i <= 3; ++i)
+    {
+        bools.push_back (new juce::AudioParameterBool (juce::ParameterID ("bool" + juce::String (i), 1),
+                                                       "Bool " + juce::String (i), false));
+        addParameter (bools.back());
+    }
+
+    for (int i = 1; i <= 2; ++i)
+    {
+        choices.push_back (new juce::AudioParameterChoice (juce::ParameterID ("choice" + juce::String (i), 1),
+                                                           "Choice " + juce::String (i),
+                                                           { "One", "Two", "Three", "Four" }, 0));
+        addParameter (choices.back());
+    }
+}
+
+//==============================================================================
+void DiscreteParamsProcessor::getStateInformation (juce::MemoryBlock& destData)
+{
+    juce::MemoryOutputStream mos (destData, false);
+
+    for (auto p : bools)
+        mos.writeBool (p->get());
+
+    for (auto p : choices)
+        mos.writeInt (p->getIndex());
+}
+
+void DiscreteParamsProcessor::setStateInformation (const void* data, int sizeInBytes)
+{
+    juce::MemoryInputStream mis (data, (size_t) sizeInBytes, false);
+
+    for (auto p : bools)
+        *p = mis.readBool();
+
+    for (auto p : choices)
+        *p = mis.readInt();
+}
+
+//==============================================================================
+juce::AudioProcessor* JUCE_CALLTYPE createPluginFilter()
+{
+    return new DiscreteParamsProcessor();
+}