Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 13d3d83b authored by Phil Burk's avatar Phil Burk
Browse files

aaudio: fix STOP hang by unlocking around join() 

Unlock and lock around the join() because the callback
thread that we are joining may be blocked by the lock.
Check for DISCONNECTED state so the error callback is
also joined correctly.
Remove unnecessary waitForStateChange that could cause
a timeout delay on DISCONNECT.

Bug: 134963902
Test: test_stop_hang (see bug report)
Change-Id: I5080fa655da35b3221234a2bcafb6ccd71d3ca27
parent e81a326d

media/libaaudio/src/client/AudioStreamInternal.cpp

+18 −11
Original line number Diff line number Diff line
@@ -241,22 +241,18 @@ error: 
    return result;

}



// This must be called under mStreamLock.

aaudio_result_t AudioStreamInternal::close() {

    aaudio_result_t result = AAUDIO_OK;

    ALOGV("%s(): mServiceStreamHandle = 0x%08X", __func__, mServiceStreamHandle);

    if (mServiceStreamHandle != AAUDIO_HANDLE_INVALID) {

        // Don't close a stream while it is running.

        aaudio_stream_state_t currentState = getState();

        if (isActive()) {

        // Don't close a stream while it is running. Stop it first.

        // If DISCONNECTED then we should still try to stop in case the

        // error callback is still running.

        if (isActive() || currentState == AAUDIO_STREAM_STATE_DISCONNECTED) {

            requestStop();

            aaudio_stream_state_t nextState;

            int64_t timeoutNanoseconds = MIN_TIMEOUT_NANOS;

            result = waitForStateChange(currentState, &nextState,

                                                       timeoutNanoseconds);

            if (result != AAUDIO_OK) {

                ALOGW("%s() waitForStateChange() returned %d %s",

                __func__, result, AAudio_convertResultToText(result));

            }

        }

        setState(AAUDIO_STREAM_STATE_CLOSING);

        aaudio_handle_t serviceStreamHandle = mServiceStreamHandle;
@@ -357,21 +353,31 @@ int64_t AudioStreamInternal::calculateReasonableTimeout() { 
    return calculateReasonableTimeout(getFramesPerBurst());

}



// This must be called under mStreamLock.

aaudio_result_t AudioStreamInternal::stopCallback()

{

    if (isDataCallbackActive()) {

    if (isDataCallbackSet()

            && (isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {

        mCallbackEnabled.store(false);

        return joinThread(NULL);

        return joinThread(NULL); // may temporarily unlock mStreamLock

    } else {

        return AAUDIO_OK;

    }

}



// This must be called under mStreamLock.

aaudio_result_t AudioStreamInternal::requestStop() {

    aaudio_result_t result = stopCallback();

    if (result != AAUDIO_OK) {

        return result;

    }

    // The stream may have been unlocked temporarily to let a callback finish

    // and the callback may have stopped the stream.

    // Check to make sure the stream still needs to be stopped.

    // See also AudioStream::safeStop().

    if (!(isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {

        return AAUDIO_OK;

    }



    if (mServiceStreamHandle == AAUDIO_HANDLE_INVALID) {

        ALOGW("%s() mServiceStreamHandle invalid = 0x%08X",
@@ -728,6 +734,7 @@ int32_t AudioStreamInternal::getFramesPerBurst() const { 
    return mFramesPerBurst;

}



// This must be called under mStreamLock.

aaudio_result_t AudioStreamInternal::joinThread(void** returnArg) {

    return AudioStream::joinThread(returnArg, calculateReasonableTimeout(getFramesPerBurst()));

}

media/libaaudio/src/client/AudioStreamInternalPlay.cpp

+1 −0
Original line number Diff line number Diff line
@@ -58,6 +58,7 @@ aaudio_result_t AudioStreamInternalPlay::open(const AudioStreamBuilder &builder) 
    return result;

}



// This must be called under mStreamLock.

aaudio_result_t AudioStreamInternalPlay::requestPause()

{

    aaudio_result_t result = stopCallback();

media/libaaudio/src/core/AudioStream.cpp

+6 −0
Original line number Diff line number Diff line
@@ -211,6 +211,7 @@ aaudio_result_t AudioStream::systemStopFromApp() { 
    return result;

}



// This must be called under mStreamLock.

aaudio_result_t AudioStream::safeStop() {



    switch (getState()) {
@@ -247,6 +248,7 @@ aaudio_result_t AudioStream::safeStop() { 
}



aaudio_result_t AudioStream::safeClose() {

    // This get temporarily unlocked in the close when joining callback threads.

    std::lock_guard<std::mutex> lock(mStreamLock);

    if (collidesWithCallback()) {

        ALOGE("%s cannot be called from a callback!", __func__);
@@ -363,6 +365,7 @@ aaudio_result_t AudioStream::createThread(int64_t periodNanoseconds, 
    }

}



// This must be called under mStreamLock.

aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanoseconds __unused)

{

    if (!mHasThread) {
@@ -374,6 +377,8 @@ aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanosec 
    // then we don't need to join(). The thread is already about to exit.

    if (pthread_self() != mThread) {

        // Called from an app thread. Not the callback.

        // Unlock because the callback may be trying to stop the stream but is blocked.

        mStreamLock.unlock();

#if 0

        // TODO implement equivalent of pthread_timedjoin_np()

        struct timespec abstime;
@@ -381,6 +386,7 @@ aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanosec 
#else

        int err = pthread_join(mThread, returnArg);

#endif

        mStreamLock.lock();

        if (err) {

            ALOGE("%s() pthread_join() returns err = %d", __func__, err);

            result = AAudioConvert_androidToAAudioResult(-err);