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

Partially fix "Dummy" actually using ALSA #2376

Merged
merged 1 commit into from Nov 12, 2015

Conversation

Projects
None yet
3 participants
@M374LX
Contributor

M374LX commented Sep 24, 2015

Partial fix for #2349.

Even without this fix, an error opening the desired device seems to cause the setup dialog to be shown instead of simply falling back to Dummy. Also, the following message is shown on the console:

*** Error in `lmms': free(): invalid size: 0x00000000020169b0 ***
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Sep 24, 2015

Member

Looks great. Not sure about the re-showing of the dialog problem, but I suspect that it is an existing problem. Clean shuffling of operations here to honor an explicit Dummy selection.

The only problem that I have with this approach in general is that the Dummy state is undesired almost always, so we should have a fallback state AND a dummy state so that dummy is explicitly chosen when it is explicitly selected. As mentioned in the bug report, I don't think this is possible currently because we don't keep track of what is selected, but rather what is displayed, so we may decide to keep the counter-intuitive behavior in lieu of better sane defaults.

Member

tresf commented Sep 24, 2015

Looks great. Not sure about the re-showing of the dialog problem, but I suspect that it is an existing problem. Clean shuffling of operations here to honor an explicit Dummy selection.

The only problem that I have with this approach in general is that the Dummy state is undesired almost always, so we should have a fallback state AND a dummy state so that dummy is explicitly chosen when it is explicitly selected. As mentioned in the bug report, I don't think this is possible currently because we don't keep track of what is selected, but rather what is displayed, so we may decide to keep the counter-intuitive behavior in lieu of better sane defaults.

@M374LX

This comment has been minimized.

Show comment
Hide comment
@M374LX

M374LX Sep 24, 2015

Contributor

In MainWindow::finalize(), we have:

// look whether mixer could use a audio-interface beside AudioDummy
else if( Engine::mixer()->audioDevName() == AudioDummy::name() )
{
    // no, so we offer setup-dialog with audio-settings...
    SetupDialog sd( SetupDialog::AudioSettings );
    sd.exec();
}

As we can see, the settings dialog will be shown every time Dummy is selected by the program (either explicitly or as a fallback). A simple solution is to set a boolean variable to true inside Mixer when the fallback happens and read such value by a getter method.

Contributor

M374LX commented Sep 24, 2015

In MainWindow::finalize(), we have:

// look whether mixer could use a audio-interface beside AudioDummy
else if( Engine::mixer()->audioDevName() == AudioDummy::name() )
{
    // no, so we offer setup-dialog with audio-settings...
    SetupDialog sd( SetupDialog::AudioSettings );
    sd.exec();
}

As we can see, the settings dialog will be shown every time Dummy is selected by the program (either explicitly or as a fallback). A simple solution is to set a boolean variable to true inside Mixer when the fallback happens and read such value by a getter method.

@M374LX

This comment has been minimized.

Show comment
Hide comment
@M374LX

M374LX Sep 26, 2015

Contributor

In AudioDummy::run(), we have:

const surroundSampleFrame* b = mixer()->nextBuffer();
if( !b )
{
    break;
}

The cause of the error message when using DummyAudio and closing the program seems to be Mixer::nextBuffer() returning a wrong value.

Contributor

M374LX commented Sep 26, 2015

In AudioDummy::run(), we have:

const surroundSampleFrame* b = mixer()->nextBuffer();
if( !b )
{
    break;
}

The cause of the error message when using DummyAudio and closing the program seems to be Mixer::nextBuffer() returning a wrong value.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Oct 30, 2015

Member

Somehow I missed the additional work you did on this. Thanks. Once someone has time to test, we'll need you to squash the commits and then we can merge the PR.

Member

tresf commented Oct 30, 2015

Somehow I missed the additional work you did on this. Thanks. Once someone has time to test, we'll need you to squash the commits and then we can merge the PR.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 11, 2015

Member

Again... we can merge this once we get some valid testing and the commits are squashed.

Member

tresf commented Nov 11, 2015

Again... we can merge this once we get some valid testing and the commits are squashed.

@M374LX

This comment has been minimized.

Show comment
Hide comment
@M374LX

M374LX Nov 11, 2015

Contributor

Except for the message on the console, everything went fine on my own tests some time ago, but I am going to do new tests today.

I think the message on the console is a topic for a new issue.

Contributor

M374LX commented Nov 11, 2015

Except for the message on the console, everything went fine on my own tests some time ago, but I am going to do new tests today.

I think the message on the console is a topic for a new issue.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 11, 2015

Member

I am going to do new tests today.

Thanks for the prompt reply. I'll tag a few testers... @Umcaruje @Wallacoloo @Spekular

@musikBear if you had a dev environment setup, this would be a great one for you to test.

Member

tresf commented Nov 11, 2015

I am going to do new tests today.

Thanks for the prompt reply. I'll tag a few testers... @Umcaruje @Wallacoloo @Spekular

@musikBear if you had a dev environment setup, this would be a great one for you to test.

@M374LX M374LX closed this Nov 11, 2015

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 12, 2015

Member

@M374LX why did you close this?

Member

tresf commented Nov 12, 2015

@M374LX why did you close this?

@M374LX

This comment has been minimized.

Show comment
Hide comment
@M374LX

M374LX Nov 12, 2015

Contributor

@M374LX why did you close this?

Having trouble with Git, as usual.

Contributor

M374LX commented Nov 12, 2015

@M374LX why did you close this?

Having trouble with Git, as usual.

@M374LX M374LX reopened this Nov 12, 2015

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 12, 2015

Member

👍

Member

tresf commented Nov 12, 2015

👍

@M374LX

This comment has been minimized.

Show comment
Hide comment
@M374LX

M374LX Nov 12, 2015

Contributor

Finally, all changes in a single commit.

Here are my tests:

  1. LMMS started with ALSA selected; then I changed to Dummy.
  2. LMMS started with Dummy selected; it worked fine with no sound, as expected; then I changed to OSS; the same error message was shown on closing the program, but this is a topic for a new issue, as said earlier.
  3. OSS failed on my system; then the settings dialog was show while the audio device fell back to Dummy; I changed the device to ALSA; same results with Dummy.
  4. With ALSA selected on the previous test, ALSA worked fine again.
Contributor

M374LX commented Nov 12, 2015

Finally, all changes in a single commit.

Here are my tests:

  1. LMMS started with ALSA selected; then I changed to Dummy.
  2. LMMS started with Dummy selected; it worked fine with no sound, as expected; then I changed to OSS; the same error message was shown on closing the program, but this is a topic for a new issue, as said earlier.
  3. OSS failed on my system; then the settings dialog was show while the audio device fell back to Dummy; I changed the device to ALSA; same results with Dummy.
  4. With ALSA selected on the previous test, ALSA worked fine again.
@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 12, 2015

Member

@M374LX did you try to move/remove the ~/.lmmsrc.xml file too to confirm that behaves normally?

Member

tresf commented Nov 12, 2015

@M374LX did you try to move/remove the ~/.lmmsrc.xml file too to confirm that behaves normally?

@M374LX

This comment has been minimized.

Show comment
Hide comment
@M374LX

M374LX Nov 12, 2015

Contributor

@M374LX did you try to move/remove the ~/.lmmsrc.xml file too to confirm that behaves normally?

By removing the file, the settings dialog is shown, but it looks like the program uses the first audio device available (ALSA in my system). Knowing that the audio device is initialized before the settings dialog is shown, this is perfectly logical. Again, this is a topic for a new issue.

Contributor

M374LX commented Nov 12, 2015

@M374LX did you try to move/remove the ~/.lmmsrc.xml file too to confirm that behaves normally?

By removing the file, the settings dialog is shown, but it looks like the program uses the first audio device available (ALSA in my system). Knowing that the audio device is initialized before the settings dialog is shown, this is perfectly logical. Again, this is a topic for a new issue.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 12, 2015

Member

ALSA in my system

So SDL isn't available I take it? It should be first chosen via 1bb276b if it is available.

Member

tresf commented Nov 12, 2015

ALSA in my system

So SDL isn't available I take it? It should be first chosen via 1bb276b if it is available.

@M374LX

This comment has been minimized.

Show comment
Hide comment
@M374LX

M374LX Nov 12, 2015

Contributor

So SDL isn't available I take it? It should be first chosen via 1bb276b if it is available.

Somehow, SDL seems to have been not detected on my system during the build process. It is not even available on the settings dialog.

Contributor

M374LX commented Nov 12, 2015

So SDL isn't available I take it? It should be first chosen via 1bb276b if it is available.

Somehow, SDL seems to have been not detected on my system during the build process. It is not even available on the settings dialog.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 12, 2015

Member

Fair enough. Thanks for the fix, testing, squashing. Top notch. Merging.

Member

tresf commented Nov 12, 2015

Fair enough. Thanks for the fix, testing, squashing. Top notch. Merging.

tresf added a commit that referenced this pull request Nov 12, 2015

Merge pull request #2376 from M374LX/dummyaudio
Partially fix "Dummy" actually using ALSA

@tresf tresf merged commit 34821f9 into LMMS:master Nov 12, 2015

1 check passed

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

@M374LX M374LX deleted the M374LX:dummyaudio branch Nov 12, 2015

@musikBear

This comment has been minimized.

Show comment
Hide comment
@musikBear

musikBear Nov 12, 2015

@tresf sorry, i dont have it- My old box dont have room for a linux partition
I do have puppy linux on a rescue-disk, though, but absolutely NO idea how to neither get or use a linux distro of lmms, and does not know if a puppy will run lmms at all
I think there is too many unknowns in the equation, to actually make a valid result for anything.. sorry

musikBear commented Nov 12, 2015

@tresf sorry, i dont have it- My old box dont have room for a linux partition
I do have puppy linux on a rescue-disk, though, but absolutely NO idea how to neither get or use a linux distro of lmms, and does not know if a puppy will run lmms at all
I think there is too many unknowns in the equation, to actually make a valid result for anything.. sorry

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 12, 2015

Member
Member

tresf commented Nov 12, 2015

@musikBear

This comment has been minimized.

Show comment
Hide comment
@musikBear

musikBear Nov 13, 2015

Want me to ship you a hard drive?

@tresf
:P doubling as santa!
I expect changes i near future. :)

musikBear commented Nov 13, 2015

Want me to ship you a hard drive?

@tresf
:P doubling as santa!
I expect changes i near future. :)

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 13, 2015

Member

doubling as santa!

Just trying to get some help, that's all. 👍

Member

tresf commented Nov 13, 2015

doubling as santa!

Just trying to get some help, that's all. 👍

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