Fix plugin midi input

[sgorpi]

Apparently the CMakeFiles.txt API changes with the JUCE versions and I had set the wrong flags... This PR fixes that, so that the plugin hosts actually send MIDI to CtrlrX... (tested with Bitwig on Linux).

I took the liberty to set some VST / VST3 / AU categories (AAX is also possible I saw). Let me know if you want to change any.

[damiensellier]

Hi Hedde, You should not change the categories for VST3 (currently Instrument|Tools, instrument being set by isSynth and tools from VST_Category) because when you export a panel as a VST3 plugin the Search and replace patching method won't work since it's looking for this very plugin category string in the code of the binary.

Same happens with plugin name and plugin manufacturer.
It has to remain as defined with the trailing spaces.

[damiensellier]

Also I noticed you set the isSynth parameter to false and you should leave it to true because this is this exact parameter that enables midi in and out in the juce declarations for midi I/o configuration In CtrlrProcessor.cpp

Also if you want to change the VST3 category you need to use a type that is compliant to the VST3 SDK and it's very strict or it will show as "Others"

The best in your case is just to open ctrlrx VST3 in your daw, open a blank panel, export it has an unrestricted instance with the proper VST3 category you'll find in the drop down menu in the global properties in the right pane.

Your exported instance will show up as whatever you defined in your daw and will allow to close and create new panels.

By default this property is set to Instruments|Synth

Also Ctrlr is not a midifx. Or it will not show up in the instrument list or audio insert FX in most of the DAWs.

What configuration you'd like precisely, I'll help you if needed?

[damiensellier]

Ah another last thing about the macos certificate failure. You need to select both certificated (application and installer) from your keychain and export them as a single p12 (note your p12 secret password). Then convert the p12 file to a base64 string to import in your GH secrets as well as the related password.
I updated this part in the yml script because it was failing to work with independent p12 certificates. That's so why I added a step for macos merged certificates.

EDIT: so yeah my mistake, the initial conditional checks for the apple certificates secrets are not good (deprecated) :

  HAVE_DEV_ID_APP_CERT: ${{ secrets.DEV_ID_APP_CERT != '' }}
  HAVE_DEV_ID_INSTALLER_CERT: ${{ secrets.DEV_ID_INSTALLER_CERT != '' }}

since I use a combined string and a single password I'll pass a new condition based on the availability of :
secrets.DEV_ID_COMBO_CERT
secrets.DEV_ID_COMBO_PASSWORD

i'll update that as well...

EDIT: FIXED

[sgorpi]

Hi Hedde, You should not change the categories for VST3 (currently Instrument|Tools, instrument being set by isSynth and tools from VST_Category) because when you export a panel as a VST3 plugin the Search and replace patching method won't work since it's looking for this very plugin category string in the code of the binary.

Same happens with plugin name and plugin manufacturer. It has to remain as defined with the trailing spaces.

Ok, restored 😄 Indeed the VST categories seem finicky (the CMake API.md already warns for that). I was thinking: maybe you should have a (unit)test for that... I see pamplejuce uses the catch2 framework... Is that something you prefer? Or would you rather use e.g. the googletest framework (seems leaner)?

Also I noticed you set the isSynth parameter to false and you should leave it to true because this is this exact parameter that enables midi in and out in the juce declarations for midi I/o configuration In CtrlrProcessor.cpp

That is not true, the midi i/o configuration is returned to the host by CtrlrProcessor::acceptsMidi , CtrlrProcessor::producesMidi and CtrlrProcessor::isMidiEffect. If you trace back the relevant #defines (JucePlugin_WantsMidiInput , JucePlugin_ProducesMidiOutput, and JucePlugin_IsMidiEffect respectively), you end up with NEEDS_MIDI_INPUT, NEEDS_MIDI_OUTPUT, and IS_MIDI_EFFECT respectively...

Also Ctrlr is not a midifx. Or it will not show up in the instrument list or audio insert FX in most of the DAWs.

What configuration you'd like precisely, I'll help you if needed?

If I remember, the original Ctrlr was a midi fx? I could instantiate it before a synth plugin or, more importantly, a HW instrument. The latter is a Bitwig device that opens the midi port and sends the notes/CC etc to it, and it allows you to select an audio input as 'return'. With Ctrlr before that HW instrument the Ctrlr panel sent the required CC / sysex (via the host) to the HW synth, while the sequencer (host) sent the notes.

With CtrlrX being an instrument, it seems to not pass thru the MIDI notes (while https://github.com/gbevin/ShowMIDI does, and the latter can also show that no midi ends up after inserting CtrlrX on the channel).

Next to that, I noticed the midi timing is very off using CtrlrX, but I think that is a different issue. Probably the output when CtrlrX owns the midi port is not compensating for the 'time' of a midi event. Only noticeable with large audio buffers I guess...

[damiensellier]

I remember I tried all combination to fit most of the DAWs and ended up with those settings.
Especially for protools it was a big bummer, only this particular config is working or I had ctrlrx only showing up in the offline processors not as real time fx.

Most DAWs don't like midifx true, audiounit was failing to validate with it.

You can try many settings to get it work with bitwig and Linux.
I think we'll be able to do a custom layout with the macros and juce filters.

Let me know when you have something ;)

About the timing it's probably coming from Linux because when I check on macos with midi monitor and the timestamp on the Ctrlr input monitor I get the same.

But I remember someone was talking about it and asked to implement another type of drivers on the Linux version (Jack) But I don't know how to do that .

Have a nice weekend!

[damiensellier]

Btw can you please check as VST2, it's less strict than VST3?
And yes you're right, isSynth is not enabling or disabling the midi IO, proof is I have some panels running as audio inserts on my audio tracks. to control multiFX hardwares as FX|Reverb for exemple.

Let me know if you find a working setup for bitwig.

[sgorpi]

So, with the current set of flags, I tested* that indeed acceptsMidi() and producesMidi() return true. After a reboot (maybe that was it, or some other magical thing?) Bitwig indeed shows CtrlrX (VST3 instance) also outputting midi data to the host. So I can use this HW instrument. So for me the flags as in this PR work. If they also work for you, then maybe you can approve the PR 😄

*) On the testing:
I've tried a bit with the framework that Pamplejuce provided by uncommenting include(Tests) in the CMakeLists.txt, and copying the tests directory from Pamplejuce into the repository directory... With that I can run some tests:

PluginBasics.cpp

However, it recompiles the code for the testing, so I'm not yet a big fan (would prefer just linking to the production (SharedCode static library).

I'll play a bit with google test and cmake, see if that does not need recompiling. I'm more familiar with the gtest framework personally, but do you have any preference?

[damiensellier]

Hi Hedde,
Cool if it works!

I already noticed thatIS_MIDI_EFFECT = FALSE is actually very important, I set it as false in the projucer to have the AAX and AU working or they would not just show up.

CMakeLists.txt did not have this option and I did not notice it.

I'll definitely add it so that we get the same config as with the projucer :

# --- CORE PLUGIN CHARACTERISTICS FROM PROJUCER ---
PLUGIN_IS_SYNTH TRUE
IS_MIDI_EFFECT FALSE
PLUGIN_WANTS_MIDI_INPUT TRUE
PLUGIN_PRODUCES_MIDI_OUTPUT TRUE
PLUGIN_EDITOR_REQUIRES_KEYS TRUE
# -------------------------------------------------

I'll take a look at the other mods you did in the PR to see if it's ok with macOS and Windows.

Concerning include(Tests) I'm not sure if it's useful since we have pluginval which directly tests the plugin thoroughfully.

I saw my latest updated with fallbacks if the GH secrets are not available are now OK, that's super cool.

Enjoy!

Damien

[sgorpi]

pluginval will load the plugin and do some basic operations on it, but it doesn't have the 'internal knowledge' of CtrlrX... So it will try stuff, but won't check if your plugin has certain capabilities or certain categories. The tests in this PR do a bit of that, and you can probably think of more 😄
(I just noticed that pluginval crashed on windows with the latest commit? But I didn't change the production code since the last successful build)

I've just updated the PR with some google tests as an example. Locally they run (of course...) but in the github runner they seem to be not found, so I'll be doing some debugging on that the coming days...

[sgorpi]

So, I'm adding quite a bit of testing around the MIDI i/o stuff... I noticed that, when I set the 'block size' in bitwig to 2048 (ok, quite extreme, but still), the midi timing isn't very accurate...

(the orange is a sound by a bitwig drumrack, the blue is the sound from an external device (airbase99) triggered via CtrlrX

I started chasing possible causes there. One of them is the usage of JUCE's MidiMessageCollector ... when you call its addMessageToQueue it doesn't look at the 'sample position' of the incoming midi message, but it looks at it's timestamp, in relation to when the 'removeNextBlockOfMessages' was last called (set with Time::getMillisecondCounterHiRes()) ...

That seems to have improved some things, but then yesterday I've noticed that, when I send the midi via bitwig instead of via Ctrlr, there are 2 note triggers in the output... One of the things i found is that the panel's panelMidiThruH2H is completely ignored, as the CtrlrProcessor::processBlock will always copy the input midi messages straight to the midiCollector... I'm suspecting that the panel then also receives the message and will again copy that to the midiCollector, but I haven't seen that in testing yet, so I'm diving further in the rabbit hole 😉

Also notice that the tests are running in the pipeline in the "Test & Benchmark" step. Currently I've set the pipeline to ignore a failure of that step, but you could consider making it more strict in the future.

[damiensellier]

Hi Hedde, I did not know about that. I never noticed midi could be out of sync. According to your waveform, it's quite groovy :)

That would be interesting to check the timing at different sample rate.

I have absolutely no time to spend on all that in the next few weeks, maybe even months.

So, feel free to take a look at it and keep up with the commits in this particular PR. I'll do a merge once you have something tried and tested.

Good luck 🤞

Damien

[sgorpi]

Cheers, the December period is always busy. Hope you'll have a good time the coming months!

I'll keep working on this PR and maybe open a few small ones with other small fixes/improvements that aren't related.

[sgorpi]

Small update: I found a nice tool to create overview diagrams: https://clang-uml.github.io/ Some example diagrams below.
From the sequence diagram we can see that when the CtrlrPanelProcessor gets triggered to processBlock, it'll at some point ask the CtrlrPanel to sendMidi() from the midi messages from the host, which in turn then will ask the CtrlrProcessor to addMidiToOutputQueue. But, in CtrlrProcessor::processBlock() the same messages were already added to the output queue...

1. I haven't been able to reproduce this 'double midi messages' situation yet, but I can see it from code analysis and the diagrams, so I'll try to find the setup in which it occurs.
2. If you like I can make a pull-request with a github action to generate diagrams?

class diagram of CtrlrProcessor:

sequence diagram of CtrlrProcessor::processBlock()

[damiensellier]

Hi Hedde, that's interesting! Another user started to map all the functions but it was never completed because done by hand.
If it can be generated automatically from a GH action that would be interesting to have it somewhere available on the repository.

Sure you can do another PR I'll merge it asap.

Have a nice day

Damien

[damiensellier]

Now I remember someone telling about those duplicated messages and when it occurred. I'll try to recall and post the link about it here.

[sgorpi]

So, I also haven't found much time lately... but today I found at least a way to reproduce the 'duplicate notes' scenario: The "Host 2 Device" flag has to be true/checked, and then midi messages are being duplicated (to the host at least). This is captured in test2.panel in the tests, which you can see in the Test & Benchmarks workflow step. So, now we can investigate it....

Now, to solve at least the midi timing issues, I think that it probably requires quite some rewriting of the midi message handling. I'm not fully sure how to approach yet, but among others,

• the midiCollector in CtrlrProcessor should probably be a MidiBuffer i.s.o. a MidiMessageCollector to address timing
• the origin of the midi message should be stored and the logic should take it into account to help solve the duplicate midi issue
• there's some odd logic with a leftoverBuffer that's returned by a panel. If you have multiple panels open, only the last one that processed gets to return its 'leftover' buffer to the host (might also partly contribute to the duplicate midi issue

Are you OK with such major changes? And if so: maybe it's good to do that in multiple pull requests, so that the incremental steps are small (easier review/less merge conflicts)?

[damiensellier]

Hi Hedde, congrats for finding the case where duplicate messages are present!

If ever you want to update the message handling, just do a branch for that such as 5.6.35-Maintenance-MIDI-Buffer-FIX and do a separate PR, I'll also do a separate branch for testing.

I'll follow your progress and try it with the apps compiled online with the GH actions.

Right now I only pulled your valgrind debugging commit that I implemented in a temp branch https://github.com/damiensellier/CtrlrX/tree/5.6.35-Maintenance-Memory-Valgrind-FIX. If you confirm it just improves the software I'll merge my temp branch to the higher level 5.6.35-Maintenance one. I did not pulled your commits improving the testing yet.

Thanks a lot and enjoy the holidays, have a good time!

Damien