MidiOutput::sendBlockOfMessages() queue problem

General JUCE discussion
1

Hi there,

this is my first post here, so first of all - Juce is great, after 8 years coding Java it helps make coming back to C++ a pleasure (mostly!).

I’ve found with MidiOutput that when using sendBlockOfMessages() the outcome is not always as I would expect. I could be my expectations that are off, not the code.

What I do is this - I queue up a NoteOn and a NoteOff, then if I use sendBlockOfMessages again for another pair of NoteOn/Off it works as long as the On message is scheduled for later than the previous Off. Otherwise it gets delayed until after the previous Off has finished, then generally ignored - the result is an output sequence of Note On, Off, Off.
I.e. sendBlockOfMessages won’t put a message at the head of the queue if another message is already scheduled, even if that message has a later dispatch time than the new one.

At least part of the problem is here:
while (mm->next != 0 && mm->next->message.getTimeStamp() <= eventTime)
mm = mm->next;

the queue insertion code looks ahead one step, ignoring the timestamp of the first message.

I have a fix, let me know if you want it posted. Likewise if I’m just using the code wrong.

best regards,

/m

2

Ah yes, well spotted. I guess a fix would be:

if (firstMessage == 0 || firstMessage->message.getTimeStamp() > eventTime) { m->next = firstMessage; firstMessage = m; } else

3

Yep, that works fine.

I started off changing the run() method before I spotted what the problem really was - I thought it was Time::waitForMillisecondCounter (eventTime) that didn’t wake up on notify().

So I ended up simplifying somewhat to this:

void MidiOutput::run()
{
    uint32 now;
    uint32 eventTime;
    PendingMessage* message = 0;

    while (! threadShouldExit())
    {
        while(firstMessage == 0)
            wait(500);
        now = Time::getMillisecondCounter();
        lock.enter();
        message = firstMessage;
        eventTime = message->message.getTimeStamp();
        if (eventTime > now + 20){
            lock.exit();
            wait (eventTime - now - 10);
        }else{
            firstMessage = message->next;
            lock.exit();
            // only send message if it is not too old already
            if(eventTime > now - 200)
                sendMessageNow (message->message);
            delete message;
        }
    }

    clearAllPendingMessages();
}

By the way, it would be nice if (as mentioned elsewhere in the forums) it was possible to subclass MidiOutput/Input, and perhaps have a common base class. I’ve got two reasons in my current app:
I’ve got a MultiMidiOutput which forwards messages to a list of actual MidiDevices. I’d like to use polymorphism so that my client classes don’t have to know that they’re getting a MultiMidiOutput, however if I derive from MidiOutput I don’t know what unwanted implementation I get with the interface.
I also want to create a MidiInput that generates sync messages, but which can be swapped out for an external MidiInput sync at any time.
If there already is a way to do this, pls let me know! Otherwise, separating the implementation into a separate MidiOutputImpl would be fab.

cheers,

/m

4

Glad that works now! Good request about the midioutput subclassing too, it’s no problem to make those virtual.

5

I think the following change, shown below, is correct. I’ve commented out a line with dvb09, and the one after it is the correction.

The meaning of samplesPerSecondForBuffer is really samplesPerMillisecondForBuffer, the time is in samples, and we are computing milliseconds again.

Y’think?


void MidiOutput::sendBlockOfMessages (const MidiBuffer& buffer,
                                      const double millisecondCounterToStartAt,
                                      double samplesPerSecondForBuffer) throw()
{
    // You've got to call startBackgroundThread() for this to actually work..
    jassert (isThreadRunning());

    // this needs to be a value in the future - RTFM for this method!
    jassert (millisecondCounterToStartAt > 0);

    samplesPerSecondForBuffer *= 0.001;

    MidiBuffer::Iterator i (buffer);

    const uint8* data;
    int len, time;

    while (i.getNextEvent (data, len, time))
    {
//dvb09        const double eventTime = millisecondCounterToStartAt + samplesPerSecondForBuffer * time;
        const double eventTime = millisecondCounterToStartAt + time / samplesPerSecondForBuffer;

        PendingMessage* const m
            = new PendingMessage (data, len, eventTime);

        const ScopedLock sl (lock);

        if (firstMessage == 0 || firstMessage->message.getTimeStamp() > eventTime)
        {
            m->next = firstMessage;
            firstMessage = m;
        }
        else
        {
            PendingMessage* mm = firstMessage;

            while (mm->next != 0 && mm->next->message.getTimeStamp() <= eventTime)
                mm = mm->next;

            m->next = mm->next;
            mm->next = m;
        }
    }

    notify();
}
6

wow - you’re right. It looks like what I ‘meant’ to do was actually this in the earlier line:

    //samplesPerSecondForBuffer *= 0.001; 
    samplesPerSecondForBuffer = 1000.0 / samplesPerSecondForBuffer;

…which I think amounts to the same thing.

Thanks for spotting it!