Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions include/AudioJack.h
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@
#endif

class QLineEdit;
class QMenu;
class QToolButton;

namespace lmms
{
Expand Down Expand Up @@ -79,8 +81,19 @@ class AudioJack : public QObject, public AudioDevice
setupWidget(QWidget* parent);
void saveSettings() override;

private:
std::vector<std::string> getAudioPortNames(JackPortFlags portFlags) const;
std::vector<std::string> getAudioInputNames() const;
std::vector<std::string> getAudioOutputNames() const;
static QMenu* buildMenu(QToolButton* toolButton, const std::vector<std::string>& names, const QString& filteredLMMSClientName);

private:
QLineEdit* m_clientName;
// Because we do not have access to a JackAudio driver instance we have to be our own client to display inputs and outputs...
jack_client_t* m_client;

std::vector<QToolButton*> m_outputDevices;
std::vector<QToolButton*> m_inputDevices;
};

private slots:
Expand All @@ -90,6 +103,10 @@ private slots:
bool initJackClient();
void resizeInputBuffer(jack_nframes_t nframes);

void attemptToConnect(size_t index, const char *lmms_port_type, const char *source_port, const char *destination_port);
void attemptToReconnectOutput(size_t outputIndex, const QString& targetPort);
void attemptToReconnectInput(size_t inputIndex, const QString& sourcePort);

void startProcessing() override;
void stopProcessing() override;

Expand Down
279 changes: 257 additions & 22 deletions src/core/audio/AudioJack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,12 @@
#ifdef LMMS_HAVE_JACK

#include <QFormLayout>
#include <QLabel>
#include <QLineEdit>
#include <QMenu>
#include <QMessageBox>
#include <QToolButton>
#include <QStringList>

#include "AudioEngine.h"
#include "ConfigManager.h"
Expand All @@ -37,9 +41,42 @@
#include "MainWindow.h"
#include "MidiJack.h"


namespace
{
static const QString audioJackClass("audiojack");
static const QString clientNameKey("clientname");
static const QString disconnectedRepresentation("-");

QString getOutputKeyByChannel(size_t channel)
{
return "output" + QString::number(channel + 1);
}

QString getInputKeyByChannel(size_t channel)
{
return "input" + QString::number(channel + 1);
}

}

namespace lmms
{

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);
}
Comment on lines +66 to +79
Copy link
Member

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()?

Copy link
Contributor Author

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.

Copy link
Member

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


AudioJack::AudioJack(bool& successful, AudioEngine* audioEngineParam)
: AudioDevice(
Expand Down Expand Up @@ -128,11 +165,9 @@ AudioJack* AudioJack::addMidiClient(MidiJack* midiClient)
}




bool AudioJack::initJackClient()
{
QString clientName = ConfigManager::inst()->value("audiojack", "clientname");
QString clientName = ConfigManager::inst()->value(audioJackClass, clientNameKey);
if (clientName.isEmpty()) { clientName = "lmms"; }

const char* serverName = nullptr;
Expand Down Expand Up @@ -171,11 +206,11 @@ bool AudioJack::initJackClient()

for (ch_cnt_t ch = 0; ch < channels(); ++ch)
{
QString name = QString("master out ") + ((ch % 2) ? "R" : "L") + QString::number(ch / 2 + 1);
const QString name = buildOutputName(ch);
m_outputPorts.push_back(
jack_port_register(m_client, name.toLatin1().constData(), JACK_DEFAULT_AUDIO_TYPE, JackPortIsOutput, 0));

QString input_name = QString("master in ") + ((ch % 2) ? "R" : "L") + QString::number(ch / 2 + 1);
const QString input_name = buildInputName(ch);
m_inputPorts.push_back(jack_port_register(m_client, input_name.toLatin1().constData(), JACK_DEFAULT_AUDIO_TYPE, JackPortIsInput, 0));

if (m_outputPorts.back() == nullptr)
Expand All @@ -196,7 +231,50 @@ void AudioJack::resizeInputBuffer(jack_nframes_t nframes)
m_inputFrameBuffer.resize(nframes);
}

void AudioJack::attemptToConnect(size_t index, const char *lmms_port_type, const char *source_port, const char *destination_port)
{
printf("Attempting to reconnect %s port %u: %s -> %s", lmms_port_type, static_cast<unsigned int>(index), source_port, destination_port);
if (!jack_connect(m_client, source_port, destination_port))
{
printf(" - Success!\n");
}
else
{
printf(" - Failure\n");
}
}

void AudioJack::attemptToReconnectOutput(size_t outputIndex, const QString& targetPort)
{
if (outputIndex > m_outputPorts.size()) return;

if (targetPort == disconnectedRepresentation)
{
printf("Output port %u is not connected.\n", static_cast<unsigned int>(outputIndex));
return;
}

auto outputName = jack_port_name(m_outputPorts[outputIndex]);
auto targetName = targetPort.toLatin1().constData();
Copy link
Member

@messmerd messmerd Aug 6, 2025

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.


attemptToConnect(outputIndex, "output", outputName, targetName);
}

void AudioJack::attemptToReconnectInput(size_t inputIndex, const QString& sourcePort)
{
if (inputIndex > m_inputPorts.size()) return;

if (sourcePort == disconnectedRepresentation)
{
printf("Input port %u is not connected.\n", static_cast<unsigned int>(inputIndex));
return;
}

auto inputName = jack_port_name(m_inputPorts[inputIndex]);
auto sourceName = sourcePort.toLatin1().constData();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


attemptToConnect(inputIndex, "input", sourceName, inputName);
}


void AudioJack::startProcessing()
Expand All @@ -218,26 +296,20 @@ void AudioJack::startProcessing()
// try to sync JACK's and LMMS's buffer-size
// jack_set_buffer_size( m_client, audioEngine()->framesPerPeriod() );

const char** ports = jack_get_ports(m_client, nullptr, nullptr, JackPortIsPhysical | JackPortIsInput);
if (ports == nullptr)
const auto cm = ConfigManager::inst();

const auto numberOfChannels = channels();
for (size_t i = 0; i < numberOfChannels; ++i)
{
printf("no physical playback ports. you'll have to do "
"connections at your own!\n");
attemptToReconnectOutput(i, cm->value(audioJackClass, getOutputKeyByChannel(i)));
}
else

for (size_t i = 0; i < numberOfChannels; ++i)
{
for (ch_cnt_t ch = 0; ch < channels(); ++ch)
{
if (jack_connect(m_client, jack_port_name(m_outputPorts[ch]), ports[ch]))
{
printf("cannot connect output ports. you'll "
"have to do connections at your own!\n");
}
}
attemptToReconnectInput(i, cm->value(audioJackClass, getInputKeyByChannel(i)));
}

m_stopped = false;
jack_free(ports);
}


Expand Down Expand Up @@ -411,21 +483,184 @@ void AudioJack::shutdownCallback(void* udata)
AudioJack::setupWidget::setupWidget(QWidget* parent)
: AudioDeviceSetupWidget(AudioJack::name(), parent)
{
const char* serverName = nullptr;
jack_status_t status;
m_client = jack_client_open("LMMS-Setup Dialog", JackNullOption, &status, serverName);

QFormLayout * form = new QFormLayout(this);

QString cn = ConfigManager::inst()->value("audiojack", "clientname");
// Set the field growth policy to allow fields to expand horizontally
form->setFieldGrowthPolicy(QFormLayout::ExpandingFieldsGrow);

const auto cm = ConfigManager::inst();
QString cn = cm->value(audioJackClass, clientNameKey);
if (cn.isEmpty()) { cn = "lmms"; }
m_clientName = new QLineEdit(cn, this);

form->addRow(tr("Client name"), m_clientName);
}

auto buildToolButton = [this](QWidget* parent, const QString& currentSelection, const std::vector<std::string>& names, const QString& filteredLMMSClientName)
{
auto toolButton = new QToolButton(parent);
// Make sure that the tool button will fill out the available space in the form layout
toolButton->setSizePolicy(QSizePolicy::Expanding, QSizePolicy::Preferred);
toolButton->setPopupMode(QToolButton::InstantPopup);
toolButton->setText(currentSelection);
auto menu = AudioJack::setupWidget::buildMenu(toolButton, names, filteredLMMSClientName);
toolButton->setMenu(menu);

return toolButton;
};

// Outputs
const auto audioOutputNames = getAudioOutputNames();

constexpr size_t numberOfOutputChannels = 2;
for (size_t i = 0; i < numberOfOutputChannels; ++i)
{
const auto outputKey = getOutputKeyByChannel(i);
const auto outputValue = cm->value(audioJackClass, outputKey);
auto outputDevice = buildToolButton(this, outputValue, audioOutputNames, cn);
form->addRow(buildOutputName(i) + ":", outputDevice);
m_outputDevices.push_back(outputDevice);
}

// Inputs
const auto audioInputNames = getAudioInputNames();

constexpr size_t numberOfInputChannels = 2;
for (size_t i = 0; i < numberOfInputChannels; ++i)
{
const auto inputKey = getInputKeyByChannel(i);
const auto inputValue = cm->value(audioJackClass, inputKey);
auto inputDevice = buildToolButton(this, inputValue, audioInputNames, cn);
form->addRow(buildInputName(i) + ":", inputDevice);
m_inputDevices.push_back(inputDevice);
}

if (m_client != nullptr)
{
jack_deactivate(m_client);
jack_client_close(m_client);
}
}


void AudioJack::setupWidget::saveSettings()
{
ConfigManager::inst()->setValue("audiojack", "clientname", m_clientName->text());
ConfigManager::inst()->setValue(audioJackClass, clientNameKey, m_clientName->text());

for (size_t i = 0; i < m_outputDevices.size(); ++i)
{
ConfigManager::inst()->setValue(audioJackClass, getOutputKeyByChannel(i), m_outputDevices[i]->text());
}

for (size_t i = 0; i < m_inputDevices.size(); ++i)
{
ConfigManager::inst()->setValue(audioJackClass, getInputKeyByChannel(i), m_inputDevices[i]->text());
}
}

std::vector<std::string> AudioJack::setupWidget::getAudioPortNames(JackPortFlags portFlags) const
{
std::vector<std::string> audioPorts;

const char **inputAudioPorts = jack_get_ports(m_client, nullptr, JACK_DEFAULT_AUDIO_TYPE, portFlags);
if (inputAudioPorts)
{
for (int i = 0; inputAudioPorts[i] != nullptr; ++i)
{
auto currentPortName = inputAudioPorts[i];

audioPorts.push_back(currentPortName);
}
jack_free(inputAudioPorts); // Remember to free after use
}

return audioPorts;
}

std::vector<std::string> AudioJack::setupWidget::getAudioOutputNames() const
{
return getAudioPortNames(JackPortIsInput);
}

std::vector<std::string> AudioJack::setupWidget::getAudioInputNames() const
{
return getAudioPortNames(JackPortIsOutput);
}

QMenu* AudioJack::setupWidget::buildMenu(QToolButton* toolButton, const std::vector<std::string>& names, const QString& filteredLMMSClientName)
{
auto menu = new QMenu(toolButton);
QMap<QString, QMenu*> clientNameToSubMenuMap;
QList<QAction*> topLevelActions;
for (const auto& currentName : names)
{
const auto clientNameWithPortName = QString::fromStdString(currentName);

auto actionLambda = [toolButton, clientNameWithPortName](bool checked)
{
toolButton->setText(clientNameWithPortName);
};

// Split into individual client name and port name
const auto list = clientNameWithPortName.split(":");
if (list.size() == 2)
{
const auto& clientName = list[0];
const auto& portName = list[1];

if (clientName == filteredLMMSClientName)
{
// Prevent loops by not adding port of the LMMS client to the menu
continue;
}

QMenu* clientSubMenu = nullptr;

auto it = clientNameToSubMenuMap.find(clientName);
if (it == clientNameToSubMenuMap.end())
{
clientSubMenu = new QMenu(menu);
clientSubMenu->setTitle(clientName);
clientNameToSubMenuMap.insert(clientName, clientSubMenu);
}
else
{
clientSubMenu = *it;
}

auto action = new QAction(portName, clientSubMenu);
connect(action, &QAction::triggered, actionLambda);
clientSubMenu->addAction(action);
}
else
{
// We cannot split into client and port name. Add the whole thing to the top level menu
auto action = new QAction(QString::fromStdString(currentName), menu);
connect(action, &QAction::triggered, actionLambda);
topLevelActions.append(action);
}
}

// First add the sub menus. By iterating the map they will be sorted automatically
for (auto it = clientNameToSubMenuMap.begin(); it != clientNameToSubMenuMap.end(); ++it)
{
menu->addMenu(it.value());
}

// Now add potential top level actions, i.e. the entries which cannot be split at exactly one ":"
// They must be sorted explicitly
std::sort(topLevelActions.begin(), topLevelActions.end(), [](QAction* a, QAction* b) { return a->text() < b->text(); });
menu->addActions(topLevelActions);

// Add the menu entry which represents the disconnected state at the very end
auto disconnectedAction = new QAction(disconnectedRepresentation, menu);
connect(disconnectedAction, &QAction::triggered, [toolButton](bool checked) { toolButton->setText(disconnectedRepresentation); });
menu->addAction(disconnectedAction);

return menu;
}


Expand Down
Loading