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

This change FIX Issue #2638 (Crash after rendering when using soundio). #5681

Merged
merged 3 commits into from
Oct 30, 2020
Merged

This change FIX Issue #2638 (Crash after rendering when using soundio). #5681

merged 3 commits into from
Oct 30, 2020

Conversation

firewall1110
Copy link
Contributor

No description provided.

@LmmsBot
Copy link

LmmsBot commented Sep 20, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8914-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-712%2Bgd7a2841-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8914?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8916-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-712%2Bgd7a2841aa-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8916?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8917-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-712%2Bgd7a2841aa-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8917?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/8d9jr8dr5qiu72c3/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35326996"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/q6ngslys0ol7sgby/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35326996"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://8918-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-712%2Bgd7a2841aa-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8918?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "d5103a2623f13069020abec545d31041662c278f"}

@firewall1110
Copy link
Contributor Author

It seems that LmmsBot compile AppImage without JACK and soundio ... How testers will be able to test? They will have to compile source code ...

@@ -204,6 +204,7 @@ void AudioJack::startProcessing()
{
if( m_active || m_client == NULL )
{
m_stopped = false;
Copy link
Member

Choose a reason for hiding this comment

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

This will make sense if the client is already active, but how about the null client case?

Copy link
Contributor Author

@firewall1110 firewall1110 Sep 26, 2020

Choose a reason for hiding this comment

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

Good question!

In modified stable code I tried to make situation with m_client == NULL true:

  • if there is no jackd (package is not installed in system), then this "driver" fails to start and instead starts dummy "driver";
  • if jackd available (package is installed in system), then driver start jackd itself ;
  • if jackd is already started with qjackctl , then driver use started jackd; and if I stop jackd with qjackctl, then "driver" simply restart jackd (if it was connected, than restart was correct, but if I disconnect lmms before stopping jackd, than restart ended with endless loop trying start jackd; I was able close gui, but application was closed only after terminal closing).

But anyway I can not model situation than AudioJack::startProcessing() called with m_client == NULL true . It seems that m_client == NULL always false than AudioJack::startProcessing() is called.

But from another point of view, it seems that nothing happens if AudioJack::processCallback will be called when m_client == NULL true . In this case (m_client == NULL true) if
m_stopped = false; will be replaced (for example) with if (m_client != NULL) {m_stopped = false;} ,
than LMMS will hung after exporting project in file.

So, if "driver" can work with m_client == NULL true - and "eat" audio data from Mixer, than code should be as it is in PR now. But if "driver" can not work in this case , than we should replace
if( m_active || m_client == NULL )
with something like:

assert(m_client != NULL);
if( m_active )

{I really don't know how to check this ...} - is not true (see below)

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 seems, that the sentence:
"In LMMS m_client == NULL always false than AudioJack::startProcessing() is called."
can be proved "mathematically".

If this is really true, than current PR simply fix bug.
All other changes are not needed for bug fix , but are "some code cleaning".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On modified stable code I have checked situation when m_client == NULL is true (I simply change initialization code to leave object uninitialized).

"Driver" can not work properly (when m_client == NULL is true) - it is not "eating" audio data from mixer, so application hung on file export start.

So question to maintainers: "Should I add additional lines of code to handle situation, that can not happen ?"
:

Copy link
Member

Choose a reason for hiding this comment

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

I looked into the JACK source code. That situation can only happen when JACK calls AudioJack::shutdownCallback() on failures. In that case, AudioJack::restartAfterZombified() will try to restart the client. If that fails, m_client will be null.
Then, AudioJack won't consume audio anymore.

Copy link
Contributor Author

@firewall1110 firewall1110 Sep 28, 2020

Choose a reason for hiding this comment

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

You are right, but after this AudioJack::startProcessing() is not called anymore ...
Even if we try export audio data, than LMMS will hung before real audio export process.

P.S.

But "in real life" restart the client always not fail , except when user

  • starts jackd;
  • starts LMMS;
  • uninstall jackd package (if this can be done);
  • quit jackd using ( what ...., after that user have no qjackctl) command line with su.

There for I mentioned infinite loop trying restart client.

P.S.
But theoretically (not in current LMMS situation) m_client can be null. So

assert(m_client != NULL);
if( m_active )

is not the option!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My be should be added something like:

if (m_client == NULL)
{
    // In this case LMMS can not work and will hung on some operation, but user still able save project.
    // If this is happened user is warned (IN AudioJack::restartAfterZombified).
    // TODO : when will be possible change driver "on the fly" this code should be rewritten ! 
    fprintf(stderr, "AudioJack::startProcessing : ERROR : m_client == NULL : driver can not be started !!!\n");
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is no change request from maintainers, than this PR version is final - ready to merge.

P.S.

AudioJack.cpp code should be improved but not in bug fix context. (for example, in "When Jack is used, start / stop jack transport" context)

Copy link
Member

Choose a reason for hiding this comment

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

It seems like m_stopped doesn't change anything if the client is null. I think it's safe to merge.

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