-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for using jack midi input #2038
Conversation
* | ||
* by Shane Ambler | ||
* | ||
* This file is part of Linux MultiMedia Studio - http://lmms.sourceforge.net |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be edited to match all our other file descriptions. Example here.
Hi. Thanks for your contribution. The audio output of LMMS depends on input to the instruments, therefore also any incoming MIDI signals. The order in which JACK clients are run depends on the connections between clients. However, In this design the MIDI and audio clients are separate, and the only connection between them is internal to LMMS, thus invisible to JACK. There is also no way (that I know of) to set any kind of priority to JACK clients in the same process. Depending on how JACK manages to build the processing order there will be latency of 1-2 periods. Ideally both the audio and MIDI processing should share the same client. For example, patching MIDI support behaviour into the audio driver, but passing the MIDI data to the MIDI driver, or something like that. |
249032b
to
9916fa5
Compare
I came up with a way to use an existing AudioJack connection. If jack is not used for audio then midiJack still manages it's own connection, this works better than always creating an AudioJack object to manage the connection as the mixer still sees the jack audio connection and tries to use it, this seemed less work than adjusting the mixer/audioJack relationship. |
@sambler it seems that your build is failing with Qt5: Could you look into that? |
#ifdef LMMS_HAVE_JACK | ||
if( client_name == MidiJack::name() || client_name == "" ) | ||
{ | ||
MidiJack * moss = new MidiJack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nitpicking, but moss
-> mjack
because this is not the OSS midi client.
On the other hand, maybe the code for testing all the different backends should just use a generic name for these temporary variables anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nitpicking but better to be consistent with rest of the code.
if( isRunning() ) | ||
{ | ||
m_quit = true; | ||
wait( 1000 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this wait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is in the destructor, set m_quit to true, wait a little for thread to stop then terminate, that code was copied from MidiOss.cpp and is also used in MidiAlsaRaw.cpp
Build is passing in Qt5 now. Anything in particular that is holding up this PR? |
8c45c1f
to
4da73f3
Compare
Not that I'm aware of although the commits should be squashed down first. |
@sambler could you please squash the commits so this can be merged? |
@sambler I'm curious, why are the port names |
That would be transmit and receive - I think I picked them as hydrogen uses TX/RX |
previous changes updated to compile on master compiles but not tested
to match changes in 697aebc
Rebased against master and added changes to match config widget changes in 697aebc |
Nice one, @sambler - can't wait to see this merged. |
@grejppi how does this PR look? Could this be merged? |
* master: (90 commits) Grey out muted patterns in the BB editor Crash at clearing path in settings manager Clear buffer of dummy instruments. Should fix LMMS#2682 Get rid of another copy constructor call to prevent Qt5 crashes Enables style sheets for knob line colors for all knob types Kicker 'version' 0 on first save Add C++11 compile flag to the carla plugin as well Elide channel names to prevent text overflow in FxLine Replace every use of the foreach macro with a C++11 range-based for loop Compile several plugins with -std=c++0x to support range-based for loops Make lb302 include math.h so we can switch it to C++11 File browser, factory files off by one Fix regression caused by fcec8dd Fix BBtrack updating; Fix the Pattern tooltip Rewrote ProjectVersionTest.cpp to use QVERIFY and indeed fail when it's supposed to fail, and added 2 tests in this test suite. data/locale: zh_CN.ts not zh.ts White-space formatting Add gig player to win32 builds Update Chinese translations Fix locale generation for win32 builds Closes LMMS#2577 ...
…rt has been dropped.
Is there still something that needs to be done before this can be merged? |
I don't know, as I am not competent enough to review the code. I do think this is an excellent addition to LMMS, and should be accepted. @LMMS/developers please can someone look at this? |
Also @sambler the merge conflicts need to be resolved |
* master: (213 commits) Update Pattern and AutomationPattern length (LMMS#3037) Refresh i18n strings Hint text update Drop notes with length zero (LMMS#3031) Background tweak Background Update Flanger Exclude .ts files from the Github linguist Redesign Multitap echo (LMMS#3008) Update i18n source strings Extended arpeggiator functions (LMMS#2130) Fix sample track playback in BB tracks (LMMS#3023) Sort plug-in embedded resources (LMMS#3014) Implement version major.minor.release-stage.build (LMMS#3011) Fix regressions on loading broken projects (LMMS#3013) Improved file input validation. (LMMS#2523) Fix sample track view in BB editor (LMMS#3002) Request change in model when dropping a track (LMMS#3000) Add LocklessAllocator and use it in LocklessList (LMMS#2998) Drop forceStep in AutomatableModel (LMMS#3010) ...
rebased with master |
@sambler Thanks for rebasing. I'm not that good with C(++) so can't thoroughly review the code, but wouldn't it make sense to have a Jackclient interface which the JackAudio and JackMidi implement? Or a singleton? Also, what I can do is build and run these changes to give feedback on if/how well it works.
|
Allright, I just built this and tried it out and the MIDI in works perfect! (at least for the +-5 minutes of midi torture I sent through it ;)). Thanks @sambler! The setup I'm using:
I tried to push as many MIDI messages through it and it kept working perfectly. Tried it with a Kicker, Sample, TripleOscillator and ZynAddSubFX. Is there any way to get debug message about if there are warnings or errors like buffer overflows or something? I'm not seeing anything in the console, but I don't know if that's because it's working 100% or if I simply have no output of these kinds of messages. The only minor niggle might be the name of the MIDI ports as presented to JACK: I've included some other apps + a ladish room for reference. As you can see there are already a lot of different naming schemes :P So I'd like to suggest to adopt one of the existing options :) |
The jack connection is made in the JackAudio class and used by JackMidi. This could be separated but I'm not sure it would be any benefit as it is only used in two places and the code in those places isn't very big, you would be adding more lines of code to define the new class than is used to make the connection. There is only one jack midi port open. At first I tried to open multiple ports but at the time I couldn't figure out how to get it working. Jack allows multiple devices to be connected to the one port, so doesn't prevent connecting any number of midi devices. If we could get multiple ports working, I expect that it would allow more complex setups with more than one device using the same midi channel but "wired" into different instruments. Either jack midi out doesn't work or I didn't set it up right, I was hoping someone would know how to get that working, if no-one knows how to do that then opening the midi out port should be disabled until it is working. With naming, I only picked rx/tx as hydrogen uses that, I guess LMMS uses in/out for the audio port names so copying that would give some consistency. |
Makes sense, was just a question without much background knowledge :)
Having 1 JACK MIDI in port is already infinitely better than having none ;) and should serve the majority of use cases, so I'd suggest to just merge this and only if there are actually people who want/need/use multiple JACK MIDI ports try to fix it in a new change/PR.
👍
|
* master: Remove remnant WANT_SYSTEM_SR (LMMS#3050) Add Persian and Phrygian dominant scales (LMMS#3045) change automation editor window title on rename automation pattern piano roll title changed on pattern renaming minor changes/code conventions no reason for patternRenamed() is public rename Piano Roll window title on Instrument Track rename/remove adjust window title on rename lfo controller Window title will be changed on rename track, now
Disable jack midi out port until it is functional Add comments about midi out not implemented
Rx/Tx labels are industry standards for receiving/transmitting lines and it goes way back, probably 60's, and is also used in the MIDI specifications but isn't very descriptive. Midi in/out is better. |
I've been running this branch for the last 8 days and never had a problem with the MIDI in, so from a functional perspective this PR is working fine (on my machine). |
Seems like this PR go idle? |
@LMMS/developers Bump! Can someone, competent, please review this. It's been tested and updated on multiple occasions. I vote merge within 48 hours if no one says no... |
There's a lot of very minor formatting inconsistencies... For example: if( m_jackAudio == NULL && m_jackClient == NULL)
+ ^--- space
- ^--- no space From a code quality perspective, perhaps @jasp00 has some time to chime in or @rageboge (since he recently wrote some Midi support for Apple).
That's fine too. :) |
I have given the branch a short test run and the Jack MIDI is working fine on my machine. |
@sambler Thanks for the PR and your patience! 🐳 |
* Support jack midi input * If jack is used for audio then use the same connection for midi as well * Remove old FreeBSD adjustment for portaudio as multiple version support has been dropped. * Disable jack midi out port until it is functional
While lmms has jack audio support there is currently no support for jack midi connections. On FreeBSD jack midi is the only solution I have found that supports usb midi input. This patch adds jack midi input support.
I originally made these adjustments against 0.4.x and have seen no issues with my low usage, a while back I made a small adjustment to update it for 1.0 release and have now merged against master and run a short test.
Further work - jack midi output is incomplete, an output port is created but no data is sent. I made an attempt to enable multiple input ports but couldn't get it to work. This may be desirable to allow multiple devices to be connected simultaneously as I believe other midi inputs allow. While jack allows multiple devices to be connected to one input, clearly separating them within lmms would be better.