sendBlockOfMessages() not sending more than first message

General JUCE discussion
1

Hi there,

I am trying to send a MIDI Note On and Note Off with a gap in between them. I’m doing this…

int lengthInMs = 1000;

MidiBuffer m;
m.addEvent (MidiMessage::noteOn (channel, noteNumber, velocity / 127.f), 0);
m.addEvent (MidiMessage::noteOff (channel, noteNumber), lengthInMs);

// later on...
midiOutput->sendBlockOfMessages (m, Time::getMillisecondCounter(), 1000.0);

However, when I monitor for MIDI events using Protokol, I only see the first MIDI note on.

Is there anything wrong with this? I’m certain this was working previously in our app.

I am creating the midiOutput like this…

midiOutput = MidiOutput::createNewDevice ("My MIDI Output");


if (midiOutput)
    midiOutput->startBackgroundThread();

Thanks,

Adam

2

My approach to understanding what is going wrong, would be to follow the code down into JUCE, to at least determine where the failure is.

There is a loop in sendBlockOfMessages, which adds the events to the outputThread’s queue of events to send. You can verify if is working, as well as examine the timestamps that are being created for each event.

Then you could check the output thread, although I don’t quite understand it, because there are no comments in it. but, there several conditions in there which seem like they would drop events. so that could hold some clues too.

3

Thanks @cpr2323.

After some investigation I have identified a bug in JUCE with respect to sending blocks of messages.

I first re-produced the issue in a fresh test project (see attached file), for which the relevant code is below…

MainComponent.h:

//==============================================================================
class MainComponent  : public juce::Component
{
public:
       //==============================================================================
    MainComponent();
    ~MainComponent() override;

    //==============================================================================
    void resized() override;

private:

    //==============================================================================
    void sendMidiMessages();
    
    TextButton midiButton;
    std::unique_ptr<MidiOutput> myMidiOutput;

    JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (MainComponent)
};

MainComponent.cpp:

#include "MainComponent.h"

//==============================================================================
MainComponent::MainComponent()
{
    myMidiOutput = MidiOutput::createNewDevice ("My MIDI Out");
    
    if (myMidiOutput)
        myMidiOutput->startBackgroundThread();
    
    midiButton.setButtonText ("Send Messages");
    addAndMakeVisible (midiButton);
    midiButton.onClick = [this]()
    {
        sendMidiMessages();
    };
    
    setSize (600, 400);
}

//==============================================================================
MainComponent::~MainComponent()
{
    if (myMidiOutput != nullptr)
    {
        myMidiOutput->stopBackgroundThread();
        myMidiOutput.reset();
    }
}

//==============================================================================
void MainComponent::sendMidiMessages()
{
    constexpr int channel = 1;
    constexpr int noteNumber = 60;
    constexpr int velocity = 100;
    constexpr int lengthInMs = 1000;
    
    MidiBuffer m;
    m.addEvent (MidiMessage::noteOn (channel, noteNumber, velocity / 127.f), 0);
    m.addEvent (MidiMessage::noteOff (channel, noteNumber), lengthInMs);

    myMidiOutput->sendBlockOfMessages (m, Time::getMillisecondCounter(), 1000.0);
}

//==============================================================================
void MainComponent::resized()
{
    Rectangle<int> r (getLocalBounds());
    midiButton.setBounds (r.withSizeKeepingCentre (150, 20));
}

Bug Details and Suggested Fix

As mentioned in the first post - sending messages with time offsets means only the first message gets sent (use Protokol or MIDI Monitor to verify this).

In the class ScheduledEventThread in JUCE there is a loop dispatching events in the run() function.

The relevant code is below:

            const auto now = Time::getMillisecondCounter();
            const auto event = *pendingMessages.begin();
            pendingMessages.erase (pendingMessages.begin());

            const auto timestamp = event.getTimeStamp();

            if (timestamp > now + 20)
            {
                const auto millis = static_cast<int64_t> (timestamp - (now + 20));
                condvar.wait_for (lock, std::chrono::milliseconds (millis));
                continue;
            }

            if (timestamp > now)
                Time::waitForMillisecondCounter ((uint32) timestamp);

            if (timestamp > now - 200)
                outputCallback (event);

Note in the above that the statement if (timestamp > now + 20) causes the code to continue if the timestamp is more than 20ms in the future.

However - the line pendingMessages.erase (pendingMessages.begin()); has already happened so messages later than 20ms are discarded.

I think that the fix is just to move the line:

pendingMessages.erase (pendingMessages.begin());

to after the if (timestamp > now + 20) statement has finished, that way we are only erasing elements we consume (if i understand the code correctly). This fixed the issue for me in my test project.

If it was possible to get a fix for this soon that would be great as it is an issue in a live product,

MidiMessageIssue.zip (5.6 KB)

Thanks,

Adam

4 Likes
4

Excellent! I had looked at the thread run function and thought it looked like there was a case where messages could be dropped, but since it lacked any comments, it was unclear if this was intended. Thanks for putting the time in to uncover the actual issue, and hopefully improve the library. :nerd_face:

1 Like
5

that’s quite a find!

1 Like
6

Thanks for the detailed report and test case, that’s fixed here:

2 Likes
7

Thank you!