This change FIX Issue #2638 (Crash after rendering when using soundio). #5681
Conversation
🤖 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"} |
|
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; | |||
There was a problem hiding this comment.
This will make sense if the client is already active, but how about the null client case?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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 ?"
:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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");
}
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
It seems like m_stopped doesn't change anything if the client is null. I think it's safe to merge.
No description provided.