-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Output combo boxes for Jack driver #7919
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
Output combo boxes for Jack driver #7919
Conversation
Add two combo boxes and a Jack client to the Jack setup dialog. The combo boxes display the available Jack inputs. Please note that in Jack terminology these are called "outputs". The own Jack client is necessary because the setup dialog does not have any access to the actual driver. So a new client is created when the dialog is opened and deleted when it is closed, i.e. when the dialog is deleted itself. The available inputs are collected via `AudioJack::setupWidget::getAudioInputNames` and are then put into the combo boxes via `AudioJack::setupWidget::populateComboBox`. `AudioJack::setupWidget::saveSettings` saves the selections that have been made in the combo boxes into the configuration. Add the method `handleRegistrationEvent` to `AudioJack` and register it in `AudioJack::initJackClient` via `jack_set_port_registration_callback`. Currently it will only output information about the port via `printf` but not do anything else. It can likely be removed again because the `AudioJack::setupWidget` uses it's own client and must register its own callbacks in case it wants to react to changed inputs and outputs while the setup dialog is open. * Do the same for the inputs * Read the information stored in the configuration when the driver is initialized. Gracefully handle when selections are not available, e.g. because the device is not plugged in. * Decide if the setup dialog should react to changed ports while it is open. This might complicate selections that are currently made, i.e. it has to be ensured that the user experience is good. * Remove printf statements
Extend the Jack setup dialog with combo boxes that present the available outputs. Save the selected outputs in the configuration. Add `AudioJack::setupWidget::getAudioPortNames` which takes the type of port and then collects all port names which match. Make the new `getAudioOutputNames` and `getAudioInputNames` delegate to that method with the appropriate type. This also hides the different terminologies a bit.
Attempt to reconnect the inputs and outputs from the configuration during startup of the Jack driver. Nothing will be done for inputs and outputs that are not available at startup. Example: the users might have saved some inputs when a device was available. The device is then disconnected and LMMS restarted. The stored inputs cannot be used anymore. To give the users the least surprise nothing is done. `AudioJack::attemptToConnect` does the actual reconnection and also prints some information for now. `attemptToReconnectOutput` and `attemptToReconnectInput` delegate to `attemptToConnect` with the right parameters.
When populating the combo boxes select the current input/output as saved in the configuration.
Show a warning message in the Jack setup dialog if some inputs or outputs that are set in the configuration are missing in the system. This might for example be caused by disconnected devices. Let `populateComboBox` return a boolean which indicates if the selected port can be found in the list of displayed ports or not.
Remove `AudioJack::handleRegistrationEvent` as it was only used for debugging so far.
Replace repeated hard-coded strings with references to static constant variables in the anonymous namespace. This should prevent subtle mistakes when working with the configuration values of the Jack driver.
Input selection does not make sense for master now because recording is not implemented/merged yet. However, being able to select the inputs from a combo box instead of having to use an external connection manager already brings benefit to users. Hence we remove input selection for now. There will be a separate branch/commit which will revert this commit, i.e. which will add input selection again once it makes sense for master.
Introduce input selection for the Jack driver similar to how it is done for the outputs, i.e. saving to configuration, reconnection at driver start, etc.
Present the available inputs and outputs in a hierarchical sorted menu which shows clients with their ports. The heavy lifting of creating the menu for the tool button is done in the new method `buildMenu`. It takes the input/output names in Jack's "Client name:Port name" format. If an input/output name can be successfully split into the client name and port name then a sub menu with the client name is created (if it was not already created before) and the port name is added as an entry. If the name cannot be split into exactly two components then it is simply added to the top level menu as is. Ports of the LMMS client are filtered out to prevent loops. The menu starts with the client's sub menus in alphabetical order. Then the top level entries are added in alphabetical order as well. The callbacks for the `QAction` instances are implemented with lambdas because MOC does not support nested classes like the setup widget is. For now the used lambda only sets the text of the `QToolButton` as these are used for persisting the configuration anyway. Replace the `QComboxBox` instances with `QToolButton`.
|
I made a few tests. Some first points I noted:
|
|
Thanks for checking @JohannesLorenz! Regarding your observations:
That's a fundamental shortcoming of the current
Yes, it is assumed that there are some main outputs/inputs. More complex setups would have to be created with a session manager because IMO it does not make sense to implement a full-fledged session manager with all possibilities in LMMS. 😅
Yes, it is intended as a default setting that is reestablished on each start.
The LMMS client is already filtered in my local branch that's shown in #7918 (comment). It has some further improvements like for example grouping by Jack client, sorting, etc. I wonder if I should ditch this PR, push the changes from the comment into #7920, make that PR the main one and comment/define out everything related to inputs. This would mean that if recording is merged the inputs could be reinstated quickly.
Do you mean the labels? IMO they should not change according to the selection. However, your comment made me come to the conclusion that it might be better to change the labels to "Output L", "Output R", "Input L" and "Input R". What do you think? Potential fixes for the Setup DialogThere are several options to fix the restart problem of the setup dialog:
|
I don't have a strong opinion here. Though, this remark of mine (numbered "4") was a very minor observation, so it does not need to give rise to change the branching strategy.
I mean, in So, if qjackctl shows you Then LMMS should IMO show you the setup dialogue as (The
I think there may be another solution. |
JohannesLorenz
left a comment
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.
Finished 1st review:
- Checked code
- Checked style
- Tested
Let the tool buttons stretch so that they look uniform and use all the available space.
Remove the printing of the audio port names in `getAudioPortNames`.
Show the technical output and input port names used by LMMS in the setup dialog. Note: these are the names that are shown in tools like `qjackctl` or `qpwgraph`. This was proposed in a review. Personally I like the non-technical names better but let's see what's accepted.
Switch the popup mode of the tool buttons from `MenuButtonPopup` back to `InstantPopup` so that users can click everywhere. This works better now because the buttons use all the space and the little triangle can be seen better.
Make CodeFactor happy.
|
@JohannesLorenz: I have merged the changes of the now closed PR #7920 into this PR. The rationale is that the SDL driver dialog already sports input selection as well although the feature itself is not implemented yet. Here's what the feature currently looks like: The dialog now also sports the technical Jack output/input names. I am not a big fan of that but let's see what's the option of others. In case the input selection should be hidden until the feature is implemented then this could be done with a handful of |
|
Moving some of the discussion here so that we are not confined to that small code review discussion.
Yes, it was much too hot today. 😅 It can give great flexibility if one is allowed to route the output of one or more tracks into another track (as long as there are no feedback loops I guess). For example here are the options that are provided for the output of a track in Bitwig: In this example the output of track "Audio 2" is routed to the track "Audio 3". As can be seen in the menu it is even possible to route it to other applications which should come natural for a PipeWire or Jack driver. In general it is for example also possible to route track A, B and C to track D, enable recording for D and then create a quick bounce while doing so and then to continue working with that new sample. I assume that this is not possible in LMMS if it routes tracks to the mixer with no other options. Another use case could be to create effect tracks/busses by routing tracks accordingly.
Because it's a very huge undertaking. 😅 One would have to implement some kind of interface which would provide at least the following functionality:
That interface would need to abstract lots of the stuff provided by the different drivers and provide it in a uniform way to LMMS' code. All drivers would be affected by that design change because every driver would need to cater to that interface as good as possible, e.g. each driver would need to map its own inputs and outputs to the inputs and outputs provided by the staging area. Each driver would need to copy its buffers to the staging area, etc. The interface would also need to be designed in such a way that everything still works even if some drivers cannot provide all features. It might for example be the case that some drivers do not allow for devices to appear or disappear during runtime. Because its a large undertaking this PR only intends to (slowly) move towards this direction by providing some rudimentary routing functionality which can then be used by the also rudimentary sample recording functionality/branch. |
Generalize the number of inputs and outputs by using for loops. This affects the number of widgets that are created and the amount of configuration that is stored. This change is a result of a code review discussion. In my opinion it adds unnecessary complexity to something that should later be implemented completely different anyway. It is for example now necessary to compute the key names that are used during the saving of the configuration based on the channel number. The commit exists so that its changes can be discussed further. It might be reverted in one of the next commits.
JohannesLorenz
left a comment
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.
Code and style OK
|
Okay, ran it now. I feel like setting both the L and R is a bit overkill. Also, most input devices will be microphones, which are mono. The mono thing isn't that bit of an issue, TBH, you just set the mono signal to be both the left and right input. But still, what's your opinion on someone needing to set both the L and R? I feel like the mapping should be automatic and be like:
Let's start from that. |
Jack does not have a concept of mono and stereo channels. Therefore heuristics would have to be used to get an idea of which channels might belong together and form a stereo pair. Therefore I think this should be something for a later improvement down the line. |
|
Alright, thanks for elaborating! I'll test it some more and approve when I'm happy. As you can see from the QJackCtl graph, it seems to work great! |
|
The pin connector from #7459 might be a good fit for selecting JACK outputs and routing to them, though if we wanted to try that, it could probably be done later to avoid holding up this PR. The benefit would be more flexible routing, support for JACK outputs with any number of channels, and the same familiar pin connector UI used by instruments and effects. |
The pin connector goes into the direction that i have already alluded to in some comments for some other PRs. In the mid/long term the configuration options should be removed from the driver dialog and moved into the tracks. The pin connectors would allow the users to do so. The pin connector would have to work with all available drivers which have different capabilities. So the pin connector will likely have to work on some internal LMMS representations instead of a specific Jack/PipeWire implementation. However, it seems that this is already handled by your PR. |
|
Okay, I'm back! Tested again, works flawlessly! AFAIC, this can be merged immediately, maybe minus the debug messages on the terminal, but then again, those might also be useful, so no real reason to drop them. Maybe consider adding a "None" option in the dropdown, in case someone doesn't want to specify some input, but if the user doesn't need an input, they can just specify whatever, LOL! |
Output the success/failure messages with regard to the reconnects only in debug builds.
Add the option to keep inputs/outputs disconnected. The disconnected state is represented by the string "-" which is also what is saved into the configuration in this case. For now the representation in the GUI and of the save state is the same as it has the advantage that no translation is necessary and thus not mapping between display text and save state is necessary.
Make the output unconditional again because it interfered with some release builds. They complained that the `result` variable in `AudioJack::attemptToConnect` was unused because it was only further used in debug builds. The alternative would have been to duplicate the `jack_connect` call. However, that would have been a worse solution because it would have duplicated critical code which might have deviated due to the duplication.
|
Cool! Imma wait for the builds to finish and retest. |
Thanks for testing, @bratpeki!
With commit b04ff1b I have attempted to make them output only in debug builds but that lead to some problems with the release builds. So I reverted the changes in commit e270c71.
Commit 776d0be adds a disconnected state in the form of a "-" entry at the end of the list. 🙂 Can you please do a final test with regards to the new option? I would then merge after that. |
|
I'll be able to do that next week, probably on Tuesday 🫡 |
bratpeki
left a comment
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.
Works great!
Happy to hear that. 😃 I will merge then. Thanks for testing this again! |
|
@irrenhaus3, who also experienced this issue, figured out that installing JACK2 fixes the segfault. However, if jack isnt running, the PR introduces a little stutter of maybe half a second when opening the settings menu which is slightly annoying
|
Edit: fixed with pull request #8026. @AW1534, following through with #7659 I have finally archived my LMMS working copy. However, I can try to assist with some fix. It seems that you are in a better position to reproduce the bug anyway because I have always activated JACK via PipeWire and don't want to muck around with a working Linux audio system. 😅 Can you please check the state of If it is not I assume that the problem with the stutter is also rather caused by the underlying libraries as I have not implemented some explicit waiting in case the server cannot be reached. |
|
Ok, so I have installed Debian in a virtual machine and have fixed the bug with pull request #8026. |
@AW1534, this is now fixed in the current master. Thanks for bringing the problem to my attention! |
messmerd
left a comment
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 the first that I've looked at this PR's code.
While this is by no means a thorough review, there are some functional issues that immediately jump out at me, plus some breaks with the style guidelines.
And it appears the PR title and description are inaccurate, and the screenshot out-of-date? Because there are input selection combo boxes too.
| } | ||
|
|
||
| auto outputName = jack_port_name(m_outputPorts[outputIndex]); | ||
| auto targetName = targetPort.toLatin1().constData(); |
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.
The .toLatin1() method creates and returns a QByteArray by value, then its .constData() method returns a const char* pointer to its internal buffer. But the QByteArray is a temporary and will be destroyed at the end of this statement, so targetName becomes a dangling pointer.
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.
It does not really seem like a problem to me because the pointer is not really "dangling". The pointer value of targetName is passed as a parameter into the attemptToConnect method which does its job and does not store the value in any way. I assume that jack_connect doesn't store it either.
Because the value of targetName is not stored anywhere or returned to the caller it does not really have the character of a dangling pointer to me. It shouldn't create a memory leak either, because as you have noted toLatin1 returns a value which is destroyed at the end of the method. Therefore everything should be good and clean at the end of attemptToReconnectOutput.
Or am I missing something here?
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 not about whether it's an owning raw pointer or whether the function it's passed to stores it. This is about the pointer being used after the lifetime of the data it points to ends.
Like all containers, QByteArray uses RAII to clean up the data it contains when it goes out of scope. And just like std::vector<T>::data() and std::string::c_str(), QByteArray::constData() returns a non-owning pointer to the internal data the container manages. That pointer is only valid for as long as the data it points to is valid.
In this case, the temporary QByteArray goes out of scope at the end of line 258, and although QByteArray is copy-on-write, its reference count would be 1 when it goes out of scope so the data will be deallocated by the destructor.
So by line 260 where targetName is used, it is a pointer that points to memory that is no longer valid - a dangling pointer - so dereferencing it and using it is undefined behavior.
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.
Ah, you're right. For some reason I believed that the temporary would live until the end of the method which of course it does not. I have opened pull request #8032 which fixes this problem.
| } | ||
|
|
||
| auto inputName = jack_port_name(m_inputPorts[inputIndex]); | ||
| auto sourceName = sourcePort.toLatin1().constData(); |
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.
Same here
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.
| static QString buildChannelSuffix(ch_cnt_t ch) | ||
| { | ||
| return (ch % 2 ? "R" : "L") + QString::number(ch / 2 + 1); | ||
| } | ||
|
|
||
| static QString buildOutputName(ch_cnt_t ch) | ||
| { | ||
| return QString("master out ") + buildChannelSuffix(ch); | ||
| } | ||
|
|
||
| static QString buildInputName(ch_cnt_t ch) | ||
| { | ||
| return QString("master in ") + buildChannelSuffix(ch); | ||
| } |
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 have static functions here when you added an anonymous namespace block just a few lines above?
Should the strings here be translated using QObject::tr()?
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 have static functions here when you added an anonymous namespace block just a few lines above?
IIRC, I did this because the ch_cnt_t type is "LMMSy" and therefore I have put it into the LMMS namespace.
Should the strings here be translated using
QObject::tr()?
The strings are also used for the JACK port names. It has been proposed in one of the comments in the PR to also show these technical names in the dialog. Previously they have not been translated (see line 174 in the original code before this PR) and I am not sure if they should be translated due to their rather technical nature.
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.
An anonymous namespace can be nested within other namespaces such as the lmms namespace, just as an FYI
Yes, the screenshot is out of date due to the evolution that can be followed through the PR's comments. It should look somewhat like in this comment: #7919 (comment). Do other people keep their original PR descriptions up-to-date with regards to all latter changes? |
Yes, otherwise it's misleading to people trying to understand what the PR does. (Like me as I'm writing July's progress report) I ended up looking though the code changes to find out what this PR did because I couldn't trust the description and I didn't feel like reading though the whole conversation under the PR. Also, GitHub saves edits to the PR description and comments, so anyone who wants to see the initial state of the PR for whatever reason can do so that way. |
I have now adjusted the PR's description according to the current state. Thanks for doing the progress reports by the way! I find them very valuable and it's great that they are still done regularly! |






Implements #7918 and additionally provides an input configuration so that feature-wise it becomes on par with the SDL driver dialog which already provides an input selection as well although the feature itself is not implemented yet.
Here's what the feature looks like:

The dialog mirrors the technical input/output names that are used for the JACK ports.
It's possible to leave an input/output disconnected by selecting the "-" option.