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

Commit dd582929 authored by Phil Burk's avatar Phil Burk
Browse files

aaudio: cleanup thread handling

Fix problem when an attempt is made to join the callback from the
callback. It used to leave mHasThread in the wrong state.
This was fixed in AAudioThread and in AudioStream.

Add "_l" suffix to functions that need to be locked.

Simplify the way that reference counted objects are passed to threads
using incStrong() and decStrong().

Bug: 171296283
Test: atest AAudioTestCases
Change-Id: I034049c4cb9021c6073fff441e49214ee898b804
parent 9bcf4989
Loading
Loading
Loading
Loading
+19 −10
Original line number Original line Diff line number Diff line
@@ -75,6 +75,7 @@ AudioStreamInternal::AudioStreamInternal(AAudioServiceInterface &serviceInterfa
}
}


AudioStreamInternal::~AudioStreamInternal() {
AudioStreamInternal::~AudioStreamInternal() {
    ALOGD("%s() %p called", __func__, this);
}
}


aaudio_result_t AudioStreamInternal::open(const AudioStreamBuilder &builder) {
aaudio_result_t AudioStreamInternal::open(const AudioStreamBuilder &builder) {
@@ -270,21 +271,21 @@ aaudio_result_t AudioStreamInternal::open(const AudioStreamBuilder &builder) {
    return result;
    return result;


error:
error:
    releaseCloseFinal();
    safeReleaseClose();
    return result;
    return result;
}
}


// This must be called under mStreamLock.
// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::release_l() {
aaudio_result_t AudioStreamInternal::release_l() {
    aaudio_result_t result = AAUDIO_OK;
    aaudio_result_t result = AAUDIO_OK;
    ALOGV("%s(): mServiceStreamHandle = 0x%08X", __func__, mServiceStreamHandle);
    ALOGD("%s(): mServiceStreamHandle = 0x%08X", __func__, mServiceStreamHandle);
    if (mServiceStreamHandle != AAUDIO_HANDLE_INVALID) {
    if (mServiceStreamHandle != AAUDIO_HANDLE_INVALID) {
        aaudio_stream_state_t currentState = getState();
        aaudio_stream_state_t currentState = getState();
        // Don't release a stream while it is running. Stop it first.
        // Don't release a stream while it is running. Stop it first.
        // If DISCONNECTED then we should still try to stop in case the
        // If DISCONNECTED then we should still try to stop in case the
        // error callback is still running.
        // error callback is still running.
        if (isActive() || currentState == AAUDIO_STREAM_STATE_DISCONNECTED) {
        if (isActive() || currentState == AAUDIO_STREAM_STATE_DISCONNECTED) {
            requestStop();
            requestStop_l();
        }
        }


        logReleaseBufferState();
        logReleaseBufferState();
@@ -330,7 +331,7 @@ static void *aaudio_callback_thread_proc(void *context)
 * The processing code will then save the current offset
 * The processing code will then save the current offset
 * between client and server and apply that to any position given to the app.
 * between client and server and apply that to any position given to the app.
 */
 */
aaudio_result_t AudioStreamInternal::requestStart()
aaudio_result_t AudioStreamInternal::requestStart_l()
{
{
    int64_t startTime;
    int64_t startTime;
    if (mServiceStreamHandle == AAUDIO_HANDLE_INVALID) {
    if (mServiceStreamHandle == AAUDIO_HANDLE_INVALID) {
@@ -373,7 +374,7 @@ aaudio_result_t AudioStreamInternal::requestStart()
                              * AAUDIO_NANOS_PER_SECOND
                              * AAUDIO_NANOS_PER_SECOND
                              / getSampleRate();
                              / getSampleRate();
        mCallbackEnabled.store(true);
        mCallbackEnabled.store(true);
        result = createThread(periodNanos, aaudio_callback_thread_proc, this);
        result = createThread_l(periodNanos, aaudio_callback_thread_proc, this);
    }
    }
    if (result != AAUDIO_OK) {
    if (result != AAUDIO_OK) {
        setState(originalState);
        setState(originalState);
@@ -399,26 +400,29 @@ int64_t AudioStreamInternal::calculateReasonableTimeout() {
}
}


// This must be called under mStreamLock.
// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::stopCallback()
aaudio_result_t AudioStreamInternal::stopCallback_l()
{
{
    if (isDataCallbackSet()
    if (isDataCallbackSet()
            && (isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {
            && (isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {
        mCallbackEnabled.store(false);
        mCallbackEnabled.store(false);
        aaudio_result_t result = joinThread(NULL); // may temporarily unlock mStreamLock
        aaudio_result_t result = joinThread_l(NULL); // may temporarily unlock mStreamLock
        if (result == AAUDIO_ERROR_INVALID_HANDLE) {
        if (result == AAUDIO_ERROR_INVALID_HANDLE) {
            ALOGD("%s() INVALID_HANDLE, stream was probably stolen", __func__);
            ALOGD("%s() INVALID_HANDLE, stream was probably stolen", __func__);
            result = AAUDIO_OK;
            result = AAUDIO_OK;
        }
        }
        return result;
        return result;
    } else {
    } else {
        ALOGD("%s() skipped, isDataCallbackSet() = %d, isActive() = %d, getState()  = %d", __func__,
            isDataCallbackSet(), isActive(), getState());
        return AAUDIO_OK;
        return AAUDIO_OK;
    }
    }
}
}


// This must be called under mStreamLock.
// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::requestStop() {
aaudio_result_t AudioStreamInternal::requestStop_l() {
    aaudio_result_t result = stopCallback();
    aaudio_result_t result = stopCallback_l();
    if (result != AAUDIO_OK) {
    if (result != AAUDIO_OK) {
        ALOGW("%s() stop callback returned %d, returning early", __func__, result);
        return result;
        return result;
    }
    }
    // The stream may have been unlocked temporarily to let a callback finish
    // The stream may have been unlocked temporarily to let a callback finish
@@ -426,6 +430,7 @@ aaudio_result_t AudioStreamInternal::requestStop() {
    // Check to make sure the stream still needs to be stopped.
    // Check to make sure the stream still needs to be stopped.
    // See also AudioStream::safeStop().
    // See also AudioStream::safeStop().
    if (!(isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {
    if (!(isActive() || getState() == AAUDIO_STREAM_STATE_DISCONNECTED)) {
        ALOGD("%s() returning early, not active or disconnected", __func__);
        return AAUDIO_OK;
        return AAUDIO_OK;
    }
    }


@@ -805,11 +810,15 @@ int32_t AudioStreamInternal::getBufferCapacity() const {
    return mBufferCapacityInFrames;
    return mBufferCapacityInFrames;
}
}


// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::joinThread(void** returnArg) {
aaudio_result_t AudioStreamInternal::joinThread(void** returnArg) {
    return AudioStream::joinThread(returnArg, calculateReasonableTimeout(getFramesPerBurst()));
    return AudioStream::joinThread(returnArg, calculateReasonableTimeout(getFramesPerBurst()));
}
}


// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternal::joinThread_l(void** returnArg) {
    return AudioStream::joinThread_l(returnArg, calculateReasonableTimeout(getFramesPerBurst()));
}

bool AudioStreamInternal::isClockModelInControl() const {
bool AudioStreamInternal::isClockModelInControl() const {
    return isActive() && mAudioEndpoint->isFreeRunning() && mClockModel.isRunning();
    return isActive() && mAudioEndpoint->isFreeRunning() && mClockModel.isRunning();
}
}
+5 −3
Original line number Original line Diff line number Diff line
@@ -44,9 +44,9 @@ public:
    AudioStreamInternal(AAudioServiceInterface  &serviceInterface, bool inService);
    AudioStreamInternal(AAudioServiceInterface  &serviceInterface, bool inService);
    virtual ~AudioStreamInternal();
    virtual ~AudioStreamInternal();


    aaudio_result_t requestStart() override;
    aaudio_result_t requestStart_l() override;


    aaudio_result_t requestStop() override;
    aaudio_result_t requestStop_l() override;


    aaudio_result_t getTimestamp(clockid_t clockId,
    aaudio_result_t getTimestamp(clockid_t clockId,
                                       int64_t *framePosition,
                                       int64_t *framePosition,
@@ -117,7 +117,9 @@ protected:


    aaudio_result_t processCommands();
    aaudio_result_t processCommands();


    aaudio_result_t stopCallback();
    aaudio_result_t stopCallback_l();

    aaudio_result_t joinThread_l(void** returnArg);


    virtual void prepareBuffersForStart() {}
    virtual void prepareBuffersForStart() {}


+4 −4
Original line number Original line Diff line number Diff line
@@ -56,7 +56,7 @@ aaudio_result_t AudioStreamInternalPlay::open(const AudioStreamBuilder &builder)
                             getDeviceChannelCount());
                             getDeviceChannelCount());


        if (result != AAUDIO_OK) {
        if (result != AAUDIO_OK) {
            releaseCloseFinal();
            safeReleaseClose();
        }
        }
        // Sample rate is constrained to common values by now and should not overflow.
        // Sample rate is constrained to common values by now and should not overflow.
        int32_t numFrames = kRampMSec * getSampleRate() / AAUDIO_MILLIS_PER_SECOND;
        int32_t numFrames = kRampMSec * getSampleRate() / AAUDIO_MILLIS_PER_SECOND;
@@ -66,9 +66,9 @@ aaudio_result_t AudioStreamInternalPlay::open(const AudioStreamBuilder &builder)
}
}


// This must be called under mStreamLock.
// This must be called under mStreamLock.
aaudio_result_t AudioStreamInternalPlay::requestPause()
aaudio_result_t AudioStreamInternalPlay::requestPause_l()
{
{
    aaudio_result_t result = stopCallback();
    aaudio_result_t result = stopCallback_l();
    if (result != AAUDIO_OK) {
    if (result != AAUDIO_OK) {
        return result;
        return result;
    }
    }
@@ -83,7 +83,7 @@ aaudio_result_t AudioStreamInternalPlay::requestPause()
    return mServiceInterface.pauseStream(mServiceStreamHandle);
    return mServiceInterface.pauseStream(mServiceStreamHandle);
}
}


aaudio_result_t AudioStreamInternalPlay::requestFlush() {
aaudio_result_t AudioStreamInternalPlay::requestFlush_l() {
    if (mServiceStreamHandle == AAUDIO_HANDLE_INVALID) {
    if (mServiceStreamHandle == AAUDIO_HANDLE_INVALID) {
        ALOGW("%s() mServiceStreamHandle invalid", __func__);
        ALOGW("%s() mServiceStreamHandle invalid", __func__);
        return AAUDIO_ERROR_INVALID_STATE;
        return AAUDIO_ERROR_INVALID_STATE;
+2 −2
Original line number Original line Diff line number Diff line
@@ -35,9 +35,9 @@ public:


    aaudio_result_t open(const AudioStreamBuilder &builder) override;
    aaudio_result_t open(const AudioStreamBuilder &builder) override;


    aaudio_result_t requestPause() override;
    aaudio_result_t requestPause_l() override;


    aaudio_result_t requestFlush() override;
    aaudio_result_t requestFlush_l() override;


    bool isFlushSupported() const override {
    bool isFlushSupported() const override {
        // Only implement FLUSH for OUTPUT streams.
        // Only implement FLUSH for OUTPUT streams.
+52 −22
Original line number Original line Diff line number Diff line
@@ -42,16 +42,20 @@ AudioStream::AudioStream()
        : mPlayerBase(new MyPlayerBase())
        : mPlayerBase(new MyPlayerBase())
        , mStreamId(AAudio_getNextStreamId())
        , mStreamId(AAudio_getNextStreamId())
        {
        {
    // mThread is a pthread_t of unknown size so we need memset.
    memset(&mThread, 0, sizeof(mThread));
    setPeriodNanoseconds(0);
    setPeriodNanoseconds(0);
}
}


AudioStream::~AudioStream() {
AudioStream::~AudioStream() {
    // Please preserve this log because there have been several bugs related to
    // Please preserve these logs because there have been several bugs related to
    // AudioStream deletion and late callbacks.
    // AudioStream deletion and late callbacks.
    ALOGD("%s(s#%u) mPlayerBase strongCount = %d",
    ALOGD("%s(s#%u) mPlayerBase strongCount = %d",
            __func__, getId(), mPlayerBase->getStrongCount());
            __func__, getId(), mPlayerBase->getStrongCount());

    ALOGE_IF(pthread_equal(pthread_self(), mThread),
            "%s() destructor running in callback", __func__);

    ALOGE_IF(mHasThread, "%s() callback thread never join()ed", __func__);

    // If the stream is deleted when OPEN or in use then audio resources will leak.
    // If the stream is deleted when OPEN or in use then audio resources will leak.
    // This would indicate an internal error. So we want to find this ASAP.
    // This would indicate an internal error. So we want to find this ASAP.
    LOG_ALWAYS_FATAL_IF(!(getState() == AAUDIO_STREAM_STATE_CLOSED
    LOG_ALWAYS_FATAL_IF(!(getState() == AAUDIO_STREAM_STATE_CLOSED
@@ -164,7 +168,7 @@ aaudio_result_t AudioStream::systemStart() {
            return AAUDIO_ERROR_INVALID_STATE;
            return AAUDIO_ERROR_INVALID_STATE;
    }
    }


    aaudio_result_t result = requestStart();
    aaudio_result_t result = requestStart_l();
    if (result == AAUDIO_OK) {
    if (result == AAUDIO_OK) {
        // We only call this for logging in "dumpsys audio". So ignore return code.
        // We only call this for logging in "dumpsys audio". So ignore return code.
        (void) mPlayerBase->start();
        (void) mPlayerBase->start();
@@ -214,7 +218,7 @@ aaudio_result_t AudioStream::systemPause() {
            return AAUDIO_ERROR_INVALID_STATE;
            return AAUDIO_ERROR_INVALID_STATE;
    }
    }


    aaudio_result_t result = requestPause();
    aaudio_result_t result = requestPause_l();
    if (result == AAUDIO_OK) {
    if (result == AAUDIO_OK) {
        // We only call this for logging in "dumpsys audio". So ignore return code.
        // We only call this for logging in "dumpsys audio". So ignore return code.
        (void) mPlayerBase->pause();
        (void) mPlayerBase->pause();
@@ -239,7 +243,7 @@ aaudio_result_t AudioStream::safeFlush() {
        return result;
        return result;
    }
    }


    return requestFlush();
    return requestFlush_l();
}
}


aaudio_result_t AudioStream::systemStopFromCallback() {
aaudio_result_t AudioStream::systemStopFromCallback() {
@@ -299,11 +303,11 @@ aaudio_result_t AudioStream::safeStop() {
            return AAUDIO_ERROR_INVALID_STATE;
            return AAUDIO_ERROR_INVALID_STATE;
    }
    }


    return requestStop();
    return requestStop_l();
}
}


aaudio_result_t AudioStream::safeRelease() {
aaudio_result_t AudioStream::safeRelease() {
    // This get temporarily unlocked in the MMAP release() when joining callback threads.
    // This may get temporarily unlocked in the MMAP release() when joining callback threads.
    std::lock_guard<std::mutex> lock(mStreamLock);
    std::lock_guard<std::mutex> lock(mStreamLock);
    if (collidesWithCallback()) {
    if (collidesWithCallback()) {
        ALOGE("%s cannot be called from a callback!", __func__);
        ALOGE("%s cannot be called from a callback!", __func__);
@@ -322,7 +326,14 @@ aaudio_result_t AudioStream::safeReleaseClose() {
        ALOGE("%s cannot be called from a callback!", __func__);
        ALOGE("%s cannot be called from a callback!", __func__);
        return AAUDIO_ERROR_INVALID_STATE;
        return AAUDIO_ERROR_INVALID_STATE;
    }
    }
    releaseCloseFinal();
    releaseCloseFinal_l();
    return AAUDIO_OK;
}

aaudio_result_t AudioStream::safeReleaseCloseFromCallback() {
    // This get temporarily unlocked in the MMAP release() when joining callback threads.
    std::lock_guard<std::mutex> lock(mStreamLock);
    releaseCloseFinal_l();
    return AAUDIO_OK;
    return AAUDIO_OK;
}
}


@@ -403,23 +414,28 @@ void* AudioStream::wrapUserThread() {
    return procResult;
    return procResult;
}
}


// This is the entry point for the new thread created by createThread().

// This is the entry point for the new thread created by createThread_l().
// It converts the 'C' function call to a C++ method call.
// It converts the 'C' function call to a C++ method call.
static void* AudioStream_internalThreadProc(void* threadArg) {
static void* AudioStream_internalThreadProc(void* threadArg) {
    AudioStream *audioStream = (AudioStream *) threadArg;
    AudioStream *audioStream = (AudioStream *) threadArg;
    // Use an sp<> to prevent the stream from being deleted while running.
    // Prevent the stream from being deleted while being used.
    // This is just for extra safety. It is probably not needed because
    // this callback should be joined before the stream is closed.
    android::sp<AudioStream> protectedStream(audioStream);
    android::sp<AudioStream> protectedStream(audioStream);
    // Balance the incStrong() in createThread_l().
    protectedStream->decStrong(nullptr);
    return protectedStream->wrapUserThread();
    return protectedStream->wrapUserThread();
}
}


// This is not exposed in the API.
// This is not exposed in the API.
// But it is still used internally to implement callbacks for MMAP mode.
// But it is still used internally to implement callbacks for MMAP mode.
aaudio_result_t AudioStream::createThread(int64_t periodNanoseconds,
aaudio_result_t AudioStream::createThread_l(int64_t periodNanoseconds,
                                            aaudio_audio_thread_proc_t threadProc,
                                            aaudio_audio_thread_proc_t threadProc,
                                            void* threadArg)
                                            void* threadArg)
{
{
    if (mHasThread) {
    if (mHasThread) {
        ALOGE("createThread() - mHasThread already true");
        ALOGE("%s() - mHasThread already true", __func__);
        return AAUDIO_ERROR_INVALID_STATE;
        return AAUDIO_ERROR_INVALID_STATE;
    }
    }
    if (threadProc == nullptr) {
    if (threadProc == nullptr) {
@@ -429,10 +445,14 @@ aaudio_result_t AudioStream::createThread(int64_t periodNanoseconds,
    mThreadProc = threadProc;
    mThreadProc = threadProc;
    mThreadArg = threadArg;
    mThreadArg = threadArg;
    setPeriodNanoseconds(periodNanoseconds);
    setPeriodNanoseconds(periodNanoseconds);
    // Prevent this object from getting deleted before the thread has a chance to create
    // its strong pointer. Assume the thread will call decStrong().
    this->incStrong(nullptr);
    int err = pthread_create(&mThread, nullptr, AudioStream_internalThreadProc, this);
    int err = pthread_create(&mThread, nullptr, AudioStream_internalThreadProc, this);
    if (err != 0) {
    if (err != 0) {
        android::status_t status = -errno;
        android::status_t status = -errno;
        ALOGE("createThread() - pthread_create() failed, %d", status);
        ALOGE("%s() - pthread_create() failed, %d", __func__, status);
        this->decStrong(nullptr); // Because the thread won't do it.
        return AAudioConvert_androidToAAudioResult(status);
        return AAudioConvert_androidToAAudioResult(status);
    } else {
    } else {
        // TODO Use AAudioThread or maybe AndroidThread
        // TODO Use AAudioThread or maybe AndroidThread
@@ -452,17 +472,23 @@ aaudio_result_t AudioStream::createThread(int64_t periodNanoseconds,
    }
    }
}
}


aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanoseconds) {
    // This may get temporarily unlocked in the MMAP release() when joining callback threads.
    std::lock_guard<std::mutex> lock(mStreamLock);
    return joinThread_l(returnArg, timeoutNanoseconds);
}

// This must be called under mStreamLock.
// This must be called under mStreamLock.
aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanoseconds __unused)
aaudio_result_t AudioStream::joinThread_l(void** returnArg, int64_t /* timeoutNanoseconds */)
{
{
    if (!mHasThread) {
    if (!mHasThread) {
        ALOGE("joinThread() - but has no thread");
        ALOGD("joinThread() - but has no thread");
        return AAUDIO_ERROR_INVALID_STATE;
        return AAUDIO_ERROR_INVALID_STATE;
    }
    }
    aaudio_result_t result = AAUDIO_OK;
    aaudio_result_t result = AAUDIO_OK;
    // If the callback is stopping the stream because the app passed back STOP
    // If the callback is stopping the stream because the app passed back STOP
    // then we don't need to join(). The thread is already about to exit.
    // then we don't need to join(). The thread is already about to exit.
    if (pthread_self() != mThread) {
    if (!pthread_equal(pthread_self(), mThread)) {
        // Called from an app thread. Not the callback.
        // Called from an app thread. Not the callback.
        // Unlock because the callback may be trying to stop the stream but is blocked.
        // Unlock because the callback may be trying to stop the stream but is blocked.
        mStreamLock.unlock();
        mStreamLock.unlock();
@@ -477,11 +503,15 @@ aaudio_result_t AudioStream::joinThread(void** returnArg, int64_t timeoutNanosec
        if (err) {
        if (err) {
            ALOGE("%s() pthread_join() returns err = %d", __func__, err);
            ALOGE("%s() pthread_join() returns err = %d", __func__, err);
            result = AAudioConvert_androidToAAudioResult(-err);
            result = AAudioConvert_androidToAAudioResult(-err);
        }
        } else {
    }
            ALOGD("%s() pthread_join succeeded", __func__);
            // This must be set false so that the callback thread can be created
            // This must be set false so that the callback thread can be created
            // when the stream is restarted.
            // when the stream is restarted.
            mHasThread = false;
            mHasThread = false;
        }
    } else {
        ALOGD("%s() pthread_join() called on itself!", __func__);
    }
    return (result != AAUDIO_OK) ? result : mThreadRegistrationResult;
    return (result != AAUDIO_OK) ? result : mThreadRegistrationResult;
}
}


Loading