danomatika / ofxMidi

(maintained) Midi addon for openFrameworks
Other
262 stars 73 forks source link

Crash when ofxMidi sends messages from multiple thread #54

Closed hiroMTB closed 6 years ago

hiroMTB commented 6 years ago

ofxMidi always store MIDI message in local variable message.

ofxBaseMidiOut.h

vector<unsigned char> message;   //< message byte buffer

And then call MIDI API (e.g. RtAudio) to actually send data to device. This design is ok for single thread app.

But If we send MIDI from multiple thread, it won't work as expected. When we send MIDI message simultaneously from different thread, each thread overwrites local vector and create strange data packet. This is typical "data race" situation of multi thread application. To solve this problem, many changes are expected.

On OS X, with RtAudio, API itself looks like multithread friendly, so we can bypass message vector and call API directly like below.

Temporary solution Sending message directly without copy, via RtMidi native object(tested on OSX, works ok so far)

    ofxMidiOut mout;

    void sendNoteOn(int midiCh, int noteNum, int velocity, int duration){
        mout.sendNoteOn(midiCh, noteNum, velocity);  
        std::thread noteOffThread( &MidiSender::sendNoteOff, this, midiCh, noteNum, duration);
        noteOffThread.detach();
    }

    void sendNoteOff(int midiCh, int noteNum, int duration){
        if(duration>0)
            std::this_thread::sleep_for( chrono::microseconds(duration*1000) );

        vector<unsigned char> noteOff;
        noteOff.push_back(MIDI_NOTE_OFF+(midiCh-1));
        noteOff.push_back(noteNum);
        noteOff.push_back(0);
        ofPtr<ofxBaseMidiOut> ofOut = mout.midiOut;
        ofxRtMidiOut * rtOut = static_cast<ofxRtMidiOut*>(ofOut.get());
        rtOut->midiOut.sendMessage(&noteOff);
    }
Amitsegall commented 6 years ago

Hey ! I would love to implement your solution in my code as well , where should I paste this ? If i understand correctly this will allow me to send multiple notes out with actual note duration right ? thanks !

hiroMTB commented 6 years ago

yes! The code above can be used at anywhere in ofApp or your custom class. You can just call sendNoteOn with duration parameter and it automatically send noteOff later.

Please notice that some of ofxMidi variables are protected or private. (I guess mout.midiOut etc) And you will see compile error because of it. You can just move it to public in the meantime.

hiroMTB commented 6 years ago

MidiSender should be replaced with your class name.

std::thread noteOffThread( &YourClassNameHere::sendNoteOff, ...

Amitsegall commented 6 years ago

Thanks for writing back quickly - still I can't wrap my head around it (not much of a coder here) - I'm trying to write my own class that will include ofxMidi, and basically my class is your code above, i've found the mout.midiOut and set it to public but I still get two errors , one is when setting my class name in the line you mentioned std::thread noteOffThread( &MIDIOutClass::sendNoteOff, this, midiCh, noteNum, duration); where i get "used undeclared identifier 'MIDIOut Class' ... and another on the last line rtOut->midiOut.sendMessage(&noteOff); which says that midiOut is privet but i can't find it's origin to put it publicly. Thanks for the help !

hiroMTB commented 6 years ago

&MIDIOutClass should be the name of your class. which has sendNoteOff function. And I remember I exposed two or three private variables to public. Probably it's in the class of ofxMidiOut and ofxBaseMidiOut, will check later.

Amitsegall commented 6 years ago

Hey again, found all the hidden / private members and moved them to public , not I get the following _invoke(_VSTD::move(_VSTD::get<0>(__t)), _VSTD::move(_VSTD::get<_Indices>(__t))...); } and it says "attempt to use deleted function" any ideas ?

danomatika commented 6 years ago

This should now be fixed as I changed the internal byte buffer to only be used by the MIDI raw byte stream operator. This small caveat has been added to the documentation.

Please test.

Amitsegall commented 6 years ago

Hiroshi's solution works now ! thanks for sorting this out. Polyphony yes ! in the midi out example thought if you use 'S' since it is reserved you don't get a note off, but I've locally changed it and used Hiroshi's method to be able to set the duration for the note off. Thanks both !!

danomatika commented 6 years ago

What do you mean by 'S'?

Amitsegall commented 6 years ago

Sorry for the bad explanation, in the example provided with the addon, while using the keyboard to send midi notes and having the switch statement with 'S' case (the one for sysex msgs) you end up sending the sysex massages and midi note 60, but then you don't get note off when releasing. it's a very minor thing that acctually only happens if you use the keys to make notes and map them like you did in the example - I now created a new class like Hiroshi suggested in the beginning of this thread and make notes using the mouse so no problems actually - it's just in the example :) since i'm not a professional I tested the example first before writing my class and while playing with the keys spotted this. again I just changed the case in the switch state and the problem was solved.

danomatika commented 6 years ago

You might be seeing the result of a different bug.

'S' should map to SHIFT+s but modifier key handling is currently broken in OF 0.10.0 on macOS: https://github.com/openframeworks/openFrameworks/issues/6070

danomatika commented 6 years ago

If the solution is now working, please close the issue.

Amitsegall commented 6 years ago

I'm not sure I fully understand what you mean... at the moment i'm running 0.9.8 , and everything is working 👍

danomatika commented 6 years ago

Ignore the last post, then. I was making a guess without knowing what platform or OF version you are using.

The way the example's key handling is written, it should only trigger the example sysex sending when you press SHIFT+s. Pressing just 's' should just play a note. What is the issue that you see exactly? ie. "When I do ABC, XYZ happens when I thought DEF would."

danomatika commented 6 years ago

Nevermind. The issue is that shift+s is handled in keyReleased but not keyPressed. I added a filter now.