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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
88cbddb
This change FIX Issue #2638 (Crash after rendering when using soundio).
Sep 20, 2020
0d7041f
This change FIX the issue #5664 "LMMS freezes at 100% on export" (JAC…
firewall1110 Sep 21, 2020
d5103a2
Better variable name & stderr message only if error.
firewall1110 Sep 21, 2020
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
1 change: 1 addition & 0 deletions include/AudioSoundIo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class AudioSoundIo : public AudioDevice
fpp_t m_outBufFrameIndex;

bool m_stopped;
bool m_outstreamStarted;

int m_disconnectErr;
void onBackendDisconnect(int err);
Expand Down
1 change: 1 addition & 0 deletions src/core/audio/AudioJack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

return;
}

Expand Down
39 changes: 34 additions & 5 deletions src/core/audio/AudioSoundIo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ AudioSoundIo::AudioSoundIo( bool & outSuccessful, Mixer * _mixer ) :
m_outBufFrameIndex = 0;
m_outBufFramesTotal = 0;
m_stopped = true;
m_outstreamStarted = false;

m_soundio = soundio_create();
if (!m_soundio)
Expand Down Expand Up @@ -196,6 +197,12 @@ void AudioSoundIo::onBackendDisconnect(int err)
AudioSoundIo::~AudioSoundIo()
{
stopProcessing();

if (m_outstream)
{
soundio_outstream_destroy(m_outstream);
}

if (m_soundio)
{
soundio_destroy(m_soundio);
Expand All @@ -205,28 +212,50 @@ AudioSoundIo::~AudioSoundIo()

void AudioSoundIo::startProcessing()
{
int err;

m_outBufFrameIndex = 0;
m_outBufFramesTotal = 0;
m_outBufSize = mixer()->framesPerPeriod();

m_outBuf = new surroundSampleFrame[m_outBufSize];

if (! m_outstreamStarted)
{
if ((err = soundio_outstream_start(m_outstream)))
{
fprintf(stderr,
"AudioSoundIo::startProcessing() :: soundio unable to start stream: %s\n",
soundio_strerror(err));
} else {
m_outstreamStarted = true;
}
}

m_stopped = false;
int err;
if ((err = soundio_outstream_start(m_outstream)))

if ((err = soundio_outstream_pause(m_outstream, false)))
{
m_stopped = true;
fprintf(stderr, "soundio unable to start stream: %s\n", soundio_strerror(err));
fprintf(stderr,
"AudioSoundIo::startProcessing() :: resuming result error: %s\n",
soundio_strerror(err));
}
}

void AudioSoundIo::stopProcessing()
{
int err;

m_stopped = true;
if (m_outstream)
{
soundio_outstream_destroy(m_outstream);
m_outstream = NULL;
if (err = soundio_outstream_pause(m_outstream, true))
{
fprintf(stderr,
"AudioSoundIo::stopProcessing() :: pausing result error: %s\n",
soundio_strerror(err));
}
}

if (m_outBuf)
Expand Down