Skip to content
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

Jack MIDI portnames (fixes #944) #945

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

agraef
Copy link
Contributor

@agraef agraef commented Jul 19, 2023

Fixes up the Jack MIDI port names along the lines of Jack1 and a2jmidid, as discussed in #944.

More precisely, here's a quick rundown of what this PR does in its final version:

  • Jack MIDI port names are renamed, so that they match the ones from Jack1, for both -Xseq (rev. b17379e) and -Xraw (rev. 0b38fa4).
  • The 1st seqmidi alias now uses alsa_midi (instead of the misnamed alsa_pcm) as prefix (this is in rev. b17379e).
  • The 1st rawmidi alias had no device prefix at all, which qjackctl doesn't like, thus I added the alsa_midi prefix there as well, so that the alias displays correctly in qjackctl (rev. cdaf88c).
  • Both seqmidi aliases had the capture and playback labels in the port names the wrong way round, fixed by swapping them (rev. 3edde50).

So what this PR boils down to is to make the Jack MIDI port names compatible with Jack1 while keeping the existing aliases mostly intact (apart from some bugfixes).

@agraef
Copy link
Contributor Author

agraef commented Jul 19, 2023

I'm running this myself to put it through its paces, but use cases and MIDI gear vary wildly, so I'd appreciate it if anyone who's using Jack MIDI on Linux would give this a go. If you encounter any bugs, please post them here and I'll try to get them fixed asap. I'd really like this to go into the next release.

@agraef
Copy link
Contributor Author

agraef commented Jul 19, 2023

Here's how this PR will change the Jack MIDI port names if you're running Linux, using the ALSA backend.

QjackCtl with stock Jack2:
image

The Carla patchbay with stock Jack2:
image

If you have the mental capacity to remember what all these ports are, then good for you. :) I don't, that's why I created this PR.

QjackCtl with #945:
image

The Carla patchbay with #945:
image

Much better. The port naming is identical with current Jack1. If anyone has an idea on how to further improve it, please let me know. But for the time being, being compatible with Jack1 seems to be the right thing.

@agraef
Copy link
Contributor Author

agraef commented Jul 19, 2023

NB: I'm not sure how the MIDI port naming evolved during Jack1's lifetime. But I think it's possible that @sletz originally designed the port naming to be compatible with an older version of Jack1, or maybe just to be consistent with the naming of the audio ports. (@sletz, can you confirm this?) But in this day and age, with a bunch of MIDI devices in every home studio, this just doesn't do any more. This needs to be fixed in Jack2, and this PR does that.

NB2: I haven't looked at the other backends, it's possible that similar changes might be in order there. This PR is only concerned with Linux and ALSA MIDI since that's what I'm using Jack2 with.

@nedko
Copy link
Contributor

nedko commented Jul 20, 2023

IMO, at least by default, only ALSA MIDI hardware device(s) that is(are) on the same sound as the ALSA PCM device should be handled by the ALSA (seq or raw) MIDI code in JACK. This also implies the D-Bus device reservation mechanism is to be extended first so to define behaviour for MIDI devices too.

Also IMO, the first alias is better to be kept consistent with the alsa_pcm one (Already fixed in LADI/jack2), while the second alias is best to be kept for user-editable override (of the hw port name).

@agraef
Copy link
Contributor Author

agraef commented Jul 20, 2023

IMO, at least by default, only ALSA MIDI hardware device(s) that is(are) on the same sound as the ALSA PCM device should be handled by the ALSA (seq or raw) MIDI code in JACK.

In most cases that would be the empty set. I must be misunderstanding you; please elaborate. :)

Also, the scope of ALSA MIDI devices being exposed as Jack MIDI devices isn't the topic of the present PR. It is only concerned with the naming of the ports.

Also IMO, the first alias is better to be kept consistent with the alsa_pcm one

Yes, I need to review those aliases. I now realized that there can only be two of them, and the existing alsa_seqmidi.c code already uses both slots, so I need to think about which one to get rid of. That's a hard call, because the existing aliases both seem quite useful, whereas the old names aren't (that's the whole point of this PR). If backward compatibility is paramount, however, then it certainly makes sense to have the old system/midi_{capture,playback}_n names as one of the two aliases.

Is anyone actually using the existing aliases? Show of hands, please. :)

while the second alias is best to be kept for user-editable override (of the hw port name)

I disagree. IMHO, any application which allows the user to edit aliases must be prepared to remove an existing alias with jack_port_unset_alias() to make room for the new alias if needed. (You need to do that anyway since the user may want to edit the alias that s/he created, so you'll be running out of available alias slots in any case.)

agraef added a commit to agraef/jack2 that referenced this pull request Jul 22, 2023
To these ends, swap the 1st and 2nd alias of rawmidi and seqmidi, so
that the old generic port names become the 1st alias.

This was suggested by @nedko in jackaudio#945 and seems to be a prudent choice in
order to offer as much backward compatibility as possible. (We keep the
2nd aliases around, too. If a Jack2 client wants to change these, it
should be prepared to first empty the slot anyway.)
@agraef
Copy link
Contributor Author

agraef commented Jul 22, 2023

Yes, I need to review those aliases.

Done. As @nedko suggested, the old port generic names are now always the 1st alias. In order to make room for the new alias in seqmidi, I had to sacrifice one of the existing aliases. I opted for the 1st of these which seemed less useful to me.

E.g., here's how the 1st seqmidi alias now looks like in qjackctl (the 1st rawmidi alias looks the same):
image

I also fixed up the existing rawmidi alias (now the 2nd alias) which wasn't showing in qjackctl because it lacked a device prefix. And I did some cosmetic changes to the 2nd seqmidi alias. In particular, I replaced the generic port names with the actual ALSA port names so that it becomes easier to identify the ports with devices which have many MIDI ports, such as some of the MIDI boxes like the BomeBox or the mioXM. Here's how the 2nd seqmidi alias now looks like in qjackctl:
image

I understand that such changes are always controversial because of potential backward compatibility issues, but I think that the changes here are all much-needed improvements, and I hope that everybody can at least live with them. If you still see any issues with the PR, please let me know and I'll do my best to fix them.

BTW, I also submitted a PR to Rui in order to fix the "invisible connections" issue with qjackctl's connections view when alias display is enabled. The PR fixes the issue so that you cannot just view those aliases, but actually use the connections window in that mode. :) (I mentioned this in #944 (comment).)

@agraef
Copy link
Contributor Author

agraef commented Jul 22, 2023

I had to remove that last commit again (the one with the non-backward-compatible seqmidi cosmetic changes) since those changes didn't work right with qjackctl for some reason. I'm not sure what's wrong with that commit, but those changes aren't really essential since the default view already provides the ALSA port names anyway, so there's no real need to replicate that information in the alias.

@nedko
Copy link
Contributor

nedko commented Jul 22, 2023

> IMO, at least by default, only ALSA MIDI hardware device(s) that is(are) on the same sound as the ALSA PCM device should be handled by the ALSA (seq or raw) MIDI code in JACK.

In most cases that would be the empty set. I must be misunderstanding you; please elaborate. :)

IMO, by default at least, the jack MIDI drivers should handle only ALSA and MIDI device of same sound card, the same that is configured for ALSA PCM . With unstable alsa card numbers used instead of alsa string identifiers, the alias is less usable. For machine automation it is unreliable input data. For humans it is more usable mostly because of the driver name being used (for alsa rawmidi at least) which could give the hint to recognize which device is which. With stable string identifier, it is also possible to limit the MIDI device reservation during JACK server start, to single MIDI device.

@agraef
Copy link
Contributor Author

agraef commented Jul 23, 2023

IMO, by default at least, the jack MIDI drivers should handle only ALSA and MIDI device of same sound card, the same that is configured for ALSA PCM .

Ok, so I did understand you correctly. I consider this impractical for most use cases. Even if you have a monster of an external sound card or digital mixer to be used as the Jack PCM device, it most likely has few if any MIDI ports, and even those might well go unused. Most studios I have seen use separate gear to connect MIDI devices, such as a powered USB hub or specialized MIDI interfaces.

That's why I said that we're basically talking about the empty set of MIDI devices here. Jack MIDI would appear to be severely broken if this was the default, and most existing Jack MIDI clients, including Ardour, wouldn't work any more.

With unstable alsa card numbers used instead of alsa string identifiers, the alias is less usable.

I completely agree with you on that. But the present PR solves this as well. It uses the actual ALSA device and port names as Jack MIDI port names, with both raw and seq midi, which gets rid of this problem. And the port names (not the aliases) are compatible with Jack1, so if you use these for persistent storage in a session manager or patchbay client, you can even switch between Jack1 and Jack2 and it should just work.

@agraef
Copy link
Contributor Author

agraef commented Jul 23, 2023

Rui has thankfully merged my fixes for the alias display option in qjackctl, so the following little screencast shows how all the different views of the Jack MIDI ports in this PR (with -Xseq) work in qjackctl's connection window. It first shows the default view (the real Jack MIDI port names, these are the same as in Jack1), then the first alias with the old generic port names, and finally the second alias (which is the same as before):

PR945-screencast

@nedko
Copy link
Contributor

nedko commented Jul 23, 2023

As @nedko suggested, the old port generic names are now always the 1st alias.

I actually suggest to use generic names as port names (system:), use improved first alias (that uses string id of the card instead of unstable card index, and that match the alsa pcm aliases), and leave the second alias empty by default.

Here is how it looks like with LADI/jack1 and LADI/jack2:

# jack_lsp -A
system:capture_1
   alsa_pcm:hw:U192k:out1
system:capture_2
   alsa_pcm:hw:U192k:out2
system:capture_3
   alsa_pcm:hw:U192k:out3
system:capture_4
   alsa_pcm:hw:U192k:out4
system:playback_1
   alsa_pcm:hw:U192k:in1
system:playback_2
   alsa_pcm:hw:U192k:in2
system:playback_3
   alsa_pcm:hw:U192k:in3
system:playback_4
   alsa_pcm:hw:U192k:in4
system:midi_capture_1
   alsa_midi:hw:U192k:0_0_UMC404HD_192k_MIDI_1_in
system:midi_playback_1
   alsa_midi:hw:U192k:0_0_UMC404HD_192k_MIDI_1_out

The driver part, "UMC404HD_192k", is probably redundant now. Previously it was useful to identify device MIDI ports that were using card indexes. As the string identifier of the ALSA card is used now instead (in LADI/jack), the driver name probably only makes string longer with no real benefit anymore.

@agraef
Copy link
Contributor Author

agraef commented Jul 23, 2023

Ok, then I misunderstood your remark. Maybe the idea to keep the old names around as an alias was misguided. If nobody wants or needs those, then I can just get rid of them and keep the existing aliases as they are. This will make the PR simpler as well.

I don't really care much about the aliases, this PR is concerned first and foremost with the actual Jack MIDI device names. And I'm striving for compatibility with (mainline) Jack1 there, not with LADI Jack.

@agraef
Copy link
Contributor Author

agraef commented Jul 23, 2023

Alright guys, I stripped this down as much as I could now. Jack MIDI device names are the ones from Jack1, for both -Xseq and -Xraw. The aliases I left largely untouched, except:

  • The 1st seqmidi alias now uses alsa_midi (instead of the misnamed alsa_pcm) as prefix (this is in b17379e).
  • The 1st rawmidi alias had no device prefix at all, which qjackctl doesn't like, thus I added the alsa_midi prefix there as well, so that the alias displays correctly in qjackctl (cdaf88c).

So what this PR now boils down to is to make the Jack MIDI port names compatible with Jack1 while keeping the existing aliases intact (mostly).

@nedko Thanks for your comments, this helped clarify my thoughts about what the scope of this PR should be, even though I'm taking a different route here than what you've done with LADI Jack.

@falkTX You promised that you'd consider this, can you please have a look? :) Feel free to cherry-pick -- I mostly care about the 1st commit (b17379e) since I don't use -Xraw much.

@agraef
Copy link
Contributor Author

agraef commented Jul 23, 2023

Rui has just brought to my attention that the seqmidi port aliases are incorrectly labelled in Jack2. With the pcm device and with the (old) default Jack MIDI port names, capture is always on the left, playback on the right. In the seqmidi aliases, it's the other way round. which makes no sense. Let me fix this.

@agraef
Copy link
Contributor Author

agraef commented Jul 23, 2023

Yep, that change is 15 years old, see alsa_seqmidi.c:146. This is part of b9395c1 which replaces in with playback and out with capture, which is the wrong way round. Well, I guess it depends on how you look at it. :)

Fixed in 3edde50.

@agraef
Copy link
Contributor Author

agraef commented Jul 23, 2023

A related bit of Jack trivia: The in and out labelling for the Jack MIDI port names in Jack1 (which I pilfered for this PR) was also swapped around some 7 years ago, see jackaudio/jack1@0b0953d. The way it is now is that capture is the device's out port, while playback is in. (Which actually makes the most sense if you think about how the physical MIDI IN and OUT ports will be labelled on a device with traditional serial MIDI ports.)

@agraef
Copy link
Contributor Author

agraef commented Jul 24, 2023

Edited the description so that it summarizes the changes in the final version of this PR.

@agraef
Copy link
Contributor Author

agraef commented Jul 24, 2023

I had to remove that last commit again (the one with the non-backward-compatible seqmidi cosmetic changes) since those changes didn't work right with qjackctl for some reason.

I figured that out, it was a silly mistake in the formatting of the port aliases. Based on this I've also given the aliases a complete facelift now. Here's how the first seqmidi and rawmidi aliases look with these changes (this uses a2jmidid-style port names):
image

Here's the updated second seqmidi alias (this uses the ALSA port names):
image

This is what I'm actually using now, I think that these are a vast improvement over the current aliases. But YMMV.

@falkTX I have the improved aliases in a separate branch based on this PR for now, see agraef/jack2@jack-midi-portnames...agraef:jack2:jack-midi-aliases. I can still add it to this PR, or submit it separately, just let me know.

@falkTX
Copy link
Member

falkTX commented Jul 25, 2023

This is a tricky change, we dont want to break backwards compatibility with people relying on the old behaviour, even if slightly odd.
Renaming system:midi_playback_# into alsa_midi:<name> is not something I am comfortable pushing, as that is for sure going to break stuff. But if we simply set aliases on those ports then we can still show them in qjackctl and other tools while keeping backwards compat. (Carla does not show aliases, so we should just ignore it)

FYI Lots of stuff happening on my side, so reviewing such changes is going to get a bit delayed, will try to come back to it next month.

@agraef
Copy link
Contributor Author

agraef commented Jul 25, 2023

Ok, so then I'll have to keep the old port names. I'll have to change the existing aliases then, there's no way around that, given that we only have two slots for those. If you're fine with that, then I can update the PR accordingly, I'll look at that later this week.

@falkTX
Copy link
Member

falkTX commented Jul 25, 2023

yes that is what I mean. keep port names, change aliases for being more useful.
aliases are typically meant for extra names of the ports, though we can also set the "pretty name" metadata if the strings are non-ascii or very long. for now changing the aliases is more than fine and welcome.

@agraef
Copy link
Contributor Author

agraef commented Jul 25, 2023

Ok, agreed, I'm on it.

@agraef
Copy link
Contributor Author

agraef commented Jul 25, 2023

Ok, there you go. Default port name is unchanged:
image
Alias 1 is Jack1 port name under alsa_midi, for both rawmidi and seqmidi:
image
Alias 2 for seqmidi is similar to what it was, but using the ALSA device and port names:
image
Let me know what you think.

@agraef
Copy link
Contributor Author

agraef commented Jul 25, 2023

Well, this version suddenly crashes as soon as I launch Qsynth, so there must be something wrong with it. :( Probably a silly mistake. If anyone can spot it, please let me know. I'm giving up for now. Maybe I can have another look later this week.

- The 1st alias is now the Jack1 MIDI port name with alsa_midi prefix.

- This 2nd alias is basically the same as the 1st alias with the
  alsa_midi prefix stripped, so that devices are listed under the ALSA
  names.

Also fixed the "capture" and "playback" port type names which were the
wrong way round.
Like in alsa_seqmidi.c, the 1st alias is now the Jack1 MIDI port name
with alsa_midi prefix.
@agraef
Copy link
Contributor Author

agraef commented Jul 26, 2023

As I suspected, a silly editing blunder. Fixed, works again now.

@agraef
Copy link
Contributor Author

agraef commented Jul 26, 2023

Ok, @falkTX, I'm done with this, it's as much of an improvement as I can make it given the backward compatibility constraints. I understand that you're not willing to rock the boat too much, but I'd still prefer a more radical change like what I have in https://github.com/agraef/jack2/tree/jack-midi-aliases. Maybe you can have a look at that, too, before you decide.

As it stands, this PR is merely a cosmetic change of the aliases now. It is an improvement, but the aliases won't help much with qjackctl patchbay editing, since that always uses the default Jack MIDI port names. Maybe we can still swap the default name and 1st alias in this PR at a later time, to work towards better Jack1 compatibility? Something to think about.

@falkTX
Copy link
Member

falkTX commented Jul 26, 2023

if this is about qjackctl patching, why not also set the pretty-name metadata too then? afaik it will show up as replacement in qjackctl, at least it does so for audio ports.
and it is already in use for audio ports, so we can copy the code in place for those

@agraef
Copy link
Contributor Author

agraef commented Jul 26, 2023

I'm not sure what you mean by "pretty-name metadata". If you mean jack_port_set_default_metadata(), that's already there. But this doesn't make a difference in the patchbay window, I only see the proper Jack MIDI port names in there, never any aliases or metadata, no matter what I do:

image

@lucianoiam
Copy link
Contributor

FYI, I recall working on this #498 . Forgot the details though 🙂

@agraef
Copy link
Contributor Author

agraef commented Jul 26, 2023

I see an Enable JACK client/port pretty names (metadata) option in qjackctl's display setup page, but this doesn't seem to make any difference, neither in the connections window nor in the patchbay window.

If what you mean is jack_set_property() and friends (apparently, that's what #489 is about), I see that this is being defined but not called anywhere, and I'm not sure how to use it. I'm very confused. :)

@agraef
Copy link
Contributor Author

agraef commented Jul 26, 2023

Also, this seems like an awful lot of hoops to jump through just to get proper port names which is something that should just work out of the box, IMHO. I can also edit port names in qjackctl, but I never do this. Who has time for such a chore? ;-)

This isn't a grave issue with the audio part of Jack because, well, that's just a single device (normally) with some number of audio channels, no problem at all. But it is a severe problem with any non-trivial MIDI setup. The ALSA system already has proper names for all these MIDI ports, so why not just show them? By default, without having to jump through all these hoops? If Jack2 presents me with a gazillion of port names for my MIDI devices which are as descriptive as "system:midi_playback_32" then that's a clear UX issue for me, I don't think that there's another way to look at it.

Nuff said, I need to work on other things.

@falkTX
Copy link
Member

falkTX commented Jul 26, 2023

there is a problem of breaking existing setups. if this was a new development we could do whatever, but it is not the case.

we can see it in another way, the port names are unique identifiers and aliases/pretty-name give these identifiers a proper name.

@agraef
Copy link
Contributor Author

agraef commented Jul 26, 2023

there is a problem of breaking existing setups.

Agreed. Which has to be weighed against usability. But you've probably heard all that a thousand times before, and I already made my point, so I'll shut up now. :)

This PR is ready to be merged. It's an improvement IMHO, and I followed your guideline. The rest is up to you now.

@agraef
Copy link
Contributor Author

agraef commented Jul 26, 2023

if this is about qjackctl patching, why not also set the pretty-name metadata too then? afaik it will show up as replacement in qjackctl, at least it does so for audio ports.

Ok, after a brief lecture from Rui, I think I understand this metadata stuff now. (Thanks, Rui!)

But I don't see any pretty-name metadata on the audio ports:

$ for i in 1 2; do jack_property -p system:capture_$i -l; done
(no output)

I see that there is pretty-name metadata on the Jack MIDI ports:

$ for i in 1 2 3 4 5; do jack_property -p system:midi_capture_$i http://jackaudio.org/metadata/pretty-name -l; done
Midi Through
APC mini mk2
APC mini mk2
AG06/AG03
MPK mini Plus

This is generated with jack_port_set_default_metadata() which is in both alsa_rawmidi.c and alsa_seqmidi.c.

But it seems that those pretty-names aren't shown in the connections window. I don't know whether that's a bug in qjackctl or intentional, but the pretty-names are shown alright in qjackctl's graph window:

image

BTW, right now those pretty-names only comprise the device name, not the actual port name. Which looks weird with devices having many different ports, because just the device name gets repeated over and over again (as you can see with the mioXM device in the screenshot above). That should be easy to fix, though, I can still add it to this PR.

The rawmidi and seqmidi pretty-name metadata now uses the same Jack1
port name as the 1st alias, without the alsa_midi: prefix.
@agraef
Copy link
Contributor Author

agraef commented Jul 27, 2023

Fixed in c3b6d56:

$ for i in 1 2 3 4 5; do jack_property -p system:midi_capture_$i http://jackaudio.org/metadata/pretty-name -l; done
Midi Through Port 0 (out)
APC mini mk2 APC mini mk2 Contr (out)
APC mini mk2 APC mini mk2 Notes (out)
AG06/AG03 MIDI 1 (out)
MPK mini Plus MIDI 1 (out)

This is how it looks like in qjackctl's graph window now:
image

This matches the display for Jack1 fairly closely (apart from the fact that the Jack1 MIDI devices are under the alsa_midi instead of the system prefix, and that they are the real Jack1 portnames, not pretty-names):
image

I'm happy with that, I think that this is much better than the current pretty-names. Opinions?

@falkTX
Copy link
Member

falkTX commented Jul 27, 2023

Seems the best of both, we keep backwards compat while still making things look nice.

@agraef
Copy link
Contributor Author

agraef commented Jul 27, 2023

Exactly! I'll just have to get used to qjackctl's graph instead of the connections window. ;-)

@agraef
Copy link
Contributor Author

agraef commented Jul 27, 2023

Actually, I'm sure that we could make the default pretty-names work with qjackctl's connection window, too, and that Rui would accept such a change. I'll look into it some time. But for the time being I can also learn to like the graph window.

The qjackctl patchbay is another matter. But most people will probably just maintain a snapshot of the current connections and never directly edit the patchbay by hand (which I sometimes do), and then it doesn't really matter whether its contents look like gobbledegook to human eyes.

@agraef
Copy link
Contributor Author

agraef commented Jul 27, 2023

Actually, I'm sure that we could make the default pretty-names work with qjackctl's connection window, too, and that Rui would accept such a change. I'll look into it some time.

Fixed in rncbc/qjackctl#198:
image

Note that "Enable JACK client/port pretty-names (metadata)" needs to be enabled in Setup-Display to get that view in the qjackctl connections window.

@agraef
Copy link
Contributor Author

agraef commented Aug 5, 2023

@falkTX I hope that you've not forgotten about this? Better merge before it falls victim to bitrot. :)

@falkTX
Copy link
Member

falkTX commented Aug 5, 2023

I did not, as I said before I hope to make some time for some jack2 things this month. Have a few things to sort out first, then hopefully I can see about a bunch of jack2 PRs all at once

@agraef
Copy link
Contributor Author

agraef commented Aug 6, 2023

Fair enough, I'll try to be more patient. 😃 (And I'm on vacation the next three weeks anyway.)

@falkTX
Copy link
Member

falkTX commented Aug 29, 2023

this is a tough one, have been thinking about it and for sure this is going to break stuff.
that said, previous behaviour was a weird one and the new one seems logically better.

nothing in the code seems bad to me, only minor oddities that come from the existing code and not new one.
also a quick check through valgrind shows no memory errors.

so let's merge it, but we will have to write about this breaking change on the release notes.

@falkTX falkTX merged commit b83d234 into jackaudio:develop Aug 29, 2023
9 checks passed
@agraef agraef deleted the jack-midi-portnames branch September 20, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants