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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partial fix for #1600: ALSA device can be selected using a combo box #2135

Merged
merged 5 commits into from Aug 26, 2015

Conversation

Projects
None yet
5 participants
@michaelgregorius
Contributor

michaelgregorius commented Jun 27, 2015

Implements the selection of ALSA devices using a combo box as proposed by @unfa in #1600. The ALSA configuration dialog looks as follows in this branch:

issue1600

Please note that the current implementation does not check whether the device works for the parameters that LMMS uses (SND_PCM_ACCESS_RW_INTERLEAVED, SND_PCM_FORMAT_S16_LE, etc.). I tried doing these checks while compiling the list of available devices but this led to strange effects which are likely caused by the fact that the PCM device has to be opened to query its capabilities. After testing the selection repeatedly I got error messages a la "Resource or device busy".

However, having a combo box to select devices from should be a good step forward compared to a simple line edit. 馃槂

To be able to react to combo box selection changes several changes were necessary. AudioDevice::setupWidget was moved into its own class AudioDeviceSetupWidget which logically should belong to the GUI (unfortunately the include structure does not make this obvious). This was needed so that inheriting classes can use the Q_OBJECT macro, e.g. to connect signals and slots.

For the ALSA driver there is an implementation AudioAlsaSetupWidget which provides a combo box for selection of the device. The available devices are pulled from a static method in the AudioAlsa class.

All other driver widgets have been changed to inherit from AudioDeviceSetupWidget but have not been changed otherwise.

SetupDialog has been adjusted to keep a map of AudioDeviceSetupWidgets now.

michaelgregorius added some commits Jun 25, 2015

Populate and show a combo box of available ALSA cards and devices
Shows a combo box with the available ALSA cards and devices instead of a
line edit. The problem currently is that the widgets are nested classes
of AudioDevice and therefore the macro Q_OBJECT does not work which
means that its not possible to define slots that react to retrieved
signals.
Some refactoring to enable signals in audio driver setup dialogs
Moved AudioDevice::setupWidget into its own class AudioDeviceSetupWidget
which logically should belong to the GUI (unfortunately the include
structure does not make this obvious).

For the ALSA driver there is an implementation AudioAlsaSetupWidget
which provides a combo box for selection of the card and device.

All other driver widgets have been changed to inherit from
AudioAlsaSetupWidget but have not been changed otherwise.

SetupDialog has been adjusted to keep a map of AudioAlsaSetupWidgets
now.
Final version that lets the user select the ALSA device with a combo box
This version lets the user select the ALSA device to use with a combo
box. It does not check whether the device works for the parameters that
LMMS uses (SND_PCM_ACCESS_RW_INTERLEAVED, SND_PCM_FORMAT_S16_LE, etc.).
Doing these checks while compiling the list of available devices led to
strange effects which are likely caused by the fact that the PCM device
has to be opened to query its capabilities. This in turn led to error
messages a la "Resource or device busy" when testing the new
functionality repeatedly.

However, having a combo box to select devices from should be a good step
forward compared to a simple line edit. :)
void onCurrentIndexChanged(int index);
private:
QComboBox * m_deviceComboBox;

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 27, 2015

Member

m_deviceComboBox is only accessed within the context in which it was created (AudioAlsaSetupWidget's constructor), so could be made a local variable within the constructor instead of a member variable.

I don't know if this was intentional or not, as some people consider it a stylistic choice. Personally, I find extraneous member variables to be a bad thing. The more confined the scope of a variable, the fewer places you have to search through when debugging.

@Wallacoloo

Wallacoloo Jun 27, 2015

Member

m_deviceComboBox is only accessed within the context in which it was created (AudioAlsaSetupWidget's constructor), so could be made a local variable within the constructor instead of a member variable.

I don't know if this was intentional or not, as some people consider it a stylistic choice. Personally, I find extraneous member variables to be a bad thing. The more confined the scope of a variable, the fewer places you have to search through when debugging.

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Jun 27, 2015

Member

Hey @michaelgregorius, this is a fantastic idea and it fits right into the gui. I checked out your branch, and as you can probably also see with the Travis build logs, it fails:

[  2%] Building CXX object src/CMakeFiles/lmmsobjs.dir/core/Mixer.cpp.o
In file included from /home/colin/proj/lmms/src/core/Mixer.cpp:43:0:
/home/colin/proj/lmms/include/AudioPortAudio.h:85:2: error: expected class-name before 鈥榹鈥 token
  {
  ^

The error appears to be this code:

class AudioPortAudio : public AudioDevice
{
    ...
    class setupWidget : public AudioDevice::setupWidget
    {
    ...
}

Namely, you forgot to fix the parent class from AudioDevice::setupWidget to your new AudioDeviceSetupWidget.

Besides that though, I reviewed the code fairly thoroughly, primarily looking for possible memory errors, and it looked OK. It may be worth separating the implementation from the header in your new include/AudioDeviceSetupWidget.h file. That tends to help with organization (it's more clear that the class belongs to the gui since its implementation file resides in the gui directory) and it makes the file a bit easier to deal with down the road - small changes don't require long recompilations.

I also left a few inline comments for you. Great work though! Looking forward to integrating this into master, although I suspect it will have to wait until after 1.2 due to feature freeze :/

Member

Wallacoloo commented Jun 27, 2015

Hey @michaelgregorius, this is a fantastic idea and it fits right into the gui. I checked out your branch, and as you can probably also see with the Travis build logs, it fails:

[  2%] Building CXX object src/CMakeFiles/lmmsobjs.dir/core/Mixer.cpp.o
In file included from /home/colin/proj/lmms/src/core/Mixer.cpp:43:0:
/home/colin/proj/lmms/include/AudioPortAudio.h:85:2: error: expected class-name before 鈥榹鈥 token
  {
  ^

The error appears to be this code:

class AudioPortAudio : public AudioDevice
{
    ...
    class setupWidget : public AudioDevice::setupWidget
    {
    ...
}

Namely, you forgot to fix the parent class from AudioDevice::setupWidget to your new AudioDeviceSetupWidget.

Besides that though, I reviewed the code fairly thoroughly, primarily looking for possible memory errors, and it looked OK. It may be worth separating the implementation from the header in your new include/AudioDeviceSetupWidget.h file. That tends to help with organization (it's more clear that the class belongs to the gui since its implementation file resides in the gui directory) and it makes the file a bit easier to deal with down the road - small changes don't require long recompilations.

I also left a few inline comments for you. Great work though! Looking forward to integrating this into master, although I suspect it will have to wait until after 1.2 due to feature freeze :/

Fixes most of stuff found in Wallacoloo's code review for #1600
Removal of a superfluous include in AudioAlsaSetupWidget.cpp

Removal of the function "bool hasCapabilities(char *device_name)" which
was not used anyway. It implemented a test for ALSA device capabilities
needed by LMMS (SND_PCM_ACCESS_RW_INTERLEAVED, SND_PCM_FORMAT_S16_LE,
etc.).

Corrected header name in AudioAlsaSetupWidget.h.

Created an implementation file for AudioDeviceSetupWidget to make more
clear that it's part of the GUI.

Fix build for builds that use Port Audio. The setup widget of
AudioPortAudio.h still inherited from AudioDevice::setupWidget instead
of the new AudioDeviceSetupWidget.
@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Jun 27, 2015

Contributor

Hi @Wallacoloo! Thanks for the really thorough code review and the nice words! I have pushed a new commit that fixes everything except the member m_deviceComboBox. I think having the member gives a better overview of what the dialog is composed of.

Hope that the Travis build goes through now as I had to fix it blindly because I do not build with PortAudio.

Contributor

michaelgregorius commented Jun 27, 2015

Hi @Wallacoloo! Thanks for the really thorough code review and the nice words! I have pushed a new commit that fixes everything except the member m_deviceComboBox. I think having the member gives a better overview of what the dialog is composed of.

Hope that the Travis build goes through now as I had to fix it blindly because I do not build with PortAudio.

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Jun 28, 2015

Member

@michaelgregorius Great! I gave it a test, and it seems to work well enough for me ("default", "pulse" and "sysdefault" all produce the same output. "null" produces silence, as expected. The other ones shown produce silence and also cause for LMMS to show the audio device settings dialog upon restart, which I believe indicates a failure to open the device).

I believe this is the expected behavior, until it gets refined by fixing up the hasCapabilities function to where incompatible devices are hidden.

I also tested setting the device to "bogus" in master and then opened your alsa-combobox branch, and it seemed to handle things pretty well by automatically setting the ALSA device to "default". If I gave it something valid like "pulse", on the other hand, it preserved the device setting. So users shouldn't experience any upgrade problems 馃憤

Member

Wallacoloo commented Jun 28, 2015

@michaelgregorius Great! I gave it a test, and it seems to work well enough for me ("default", "pulse" and "sysdefault" all produce the same output. "null" produces silence, as expected. The other ones shown produce silence and also cause for LMMS to show the audio device settings dialog upon restart, which I believe indicates a failure to open the device).

I believe this is the expected behavior, until it gets refined by fixing up the hasCapabilities function to where incompatible devices are hidden.

I also tested setting the device to "bogus" in master and then opened your alsa-combobox branch, and it seemed to handle things pretty well by automatically setting the ALSA device to "default". If I gave it something valid like "pulse", on the other hand, it preserved the device setting. So users shouldn't experience any upgrade problems 馃憤

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Jun 28, 2015

Member

By the way, the LMMS PortAudio backend appears to be capable of narrowing down the compatible ALSA devices before selection. Their code may be of reference for getting hasCapabilities working right.

Member

Wallacoloo commented Jun 28, 2015

By the way, the LMMS PortAudio backend appears to be capable of narrowing down the compatible ALSA devices before selection. Their code may be of reference for getting hasCapabilities working right.

free(name);
free(description);

This comment has been minimized.

@8tab

8tab Jul 25, 2015

Contributor

Move those two 'free' calls inside above 'if' statement. If 'name' or 'description' are NULL, we have a crash.

@8tab

8tab Jul 25, 2015

Contributor

Move those two 'free' calls inside above 'if' statement. If 'name' or 'description' are NULL, we have a crash.

This comment has been minimized.

@midi-pascal

midi-pascal Jul 25, 2015

Contributor

馃憤 Nice catch!
However I would have tested against NULL or nullptr instead of 0 but this is pure personal taste :)

@midi-pascal

midi-pascal Jul 25, 2015

Contributor

馃憤 Nice catch!
However I would have tested against NULL or nullptr instead of 0 but this is pure personal taste :)

This comment has been minimized.

@Wallacoloo

Wallacoloo Jul 25, 2015

Member

I remember seeing this earlier & looking into it. Turns out it's perfectly valid to free a null pointer in C/C++: http://www.cplusplus.com/reference/cstdlib/free/

Additionally, moving BOTH calls to free into the if statement would cause a memory leak in the case that only one of them is null. So I vote to leave as-is.

@Wallacoloo

Wallacoloo Jul 25, 2015

Member

I remember seeing this earlier & looking into it. Turns out it's perfectly valid to free a null pointer in C/C++: http://www.cplusplus.com/reference/cstdlib/free/

Additionally, moving BOTH calls to free into the if statement would cause a memory leak in the case that only one of them is null. So I vote to leave as-is.

This comment has been minimized.

@michaelgregorius

michaelgregorius Jul 27, 2015

Contributor

Yes, that was the intention. That's also the reason why sometimes you can see code like this:

delete m_pointer;
m_pointer = 0;

It's ok to call delete on a null pointer but not ok to delete the same pointer (address) twice. By setting it to 0 after the deletion you make sure that you will only run into the valid case in case the code is called again without allocating something new for m_pointer.

@michaelgregorius

michaelgregorius Jul 27, 2015

Contributor

Yes, that was the intention. That's also the reason why sometimes you can see code like this:

delete m_pointer;
m_pointer = 0;

It's ok to call delete on a null pointer but not ok to delete the same pointer (address) twice. By setting it to 0 after the deletion you make sure that you will only run into the valid case in case the code is called again without allocating something new for m_pointer.

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Aug 26, 2015

Member

Somehow I forgot about this PR, with everything else going on. Sorry for that. I retested this after locally rebasing it against master, and it's still working great. So, merge time!

Member

Wallacoloo commented Aug 26, 2015

Somehow I forgot about this PR, with everything else going on. Sorry for that. I retested this after locally rebasing it against master, and it's still working great. So, merge time!

Wallacoloo added a commit that referenced this pull request Aug 26, 2015

Merge pull request #2135 from michaelgregorius/alsa-combobox
Partial fix for #1600: ALSA device can be selected using a combo box

@Wallacoloo Wallacoloo merged commit c99c6ee into LMMS:master Aug 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 26, 2015

Member

馃憤 Although next time we should consider a rebase first. :)

Member

tresf commented Aug 26, 2015

馃憤 Although next time we should consider a rebase first. :)

@michaelgregorius michaelgregorius deleted the michaelgregorius:alsa-combobox branch Aug 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment