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

Commit 0bd745eb authored by Phil Burk's avatar Phil Burk
Browse files

aaudio: add thread safety annotation 

Also improve naming to clarify lock requirements.

And call some locked methods that were not called before.
Although they were protected with a different lock so
it should have no effect.

Bug: 171296283
Test: adb logcat *:F
Test: In another window:
Test: atest AAudioTestCases
Change-Id: I6e863cbdea9250188e3f4b8f8654ef71c8951e74
parent 11b45e9f

media/libaaudio/src/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ cc_library { 
    ],



    cflags: [

        "-Wthread-safety",

        "-Wno-unused-parameter",

        "-Wall",

        "-Werror",

media/libaaudio/src/client/AudioStreamInternal.cpp

+1 −11
Original line number Diff line number Diff line
@@ -418,7 +418,6 @@ aaudio_result_t AudioStreamInternal::stopCallback_l() 
    }

}



// This must be called under mStreamLock.

aaudio_result_t AudioStreamInternal::requestStop_l() {

    aaudio_result_t result = stopCallback_l();

    if (result != AAUDIO_OK) {
@@ -428,7 +427,7 @@ aaudio_result_t AudioStreamInternal::requestStop_l() { 
    // 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().

    // See also AudioStream::safeStop_l().

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

        ALOGD("%s() returning early, not active or disconnected", __func__);

        return AAUDIO_OK;
@@ -810,15 +809,6 @@ int32_t AudioStreamInternal::getBufferCapacity() const { 
    return mBufferCapacityInFrames;

}



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

    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 {

    return isActive() && mAudioEndpoint->isFreeRunning() && mClockModel.isRunning();

}

media/libaaudio/src/client/AudioStreamInternal.h

+4 −11
Original line number Diff line number Diff line
@@ -44,10 +44,6 @@ public: 
    AudioStreamInternal(AAudioServiceInterface  &serviceInterface, bool inService);

    virtual ~AudioStreamInternal();



    aaudio_result_t requestStart_l() override;



    aaudio_result_t requestStop_l() override;



    aaudio_result_t getTimestamp(clockid_t clockId,

                                       int64_t *framePosition,

                                       int64_t *timeNanoseconds) override;
@@ -56,8 +52,6 @@ public: 


    aaudio_result_t open(const AudioStreamBuilder &builder) override;



    aaudio_result_t release_l() override;



    aaudio_result_t setBufferSize(int32_t requestedFrames) override;



    int32_t getBufferSize() const override;
@@ -72,12 +66,9 @@ public: 


    aaudio_result_t unregisterThread() override;



    aaudio_result_t joinThread(void** returnArg);



    // Called internally from 'C'

    virtual void *callbackLoop() = 0;





    bool isMMap() override {

        return true;

    }
@@ -96,6 +87,10 @@ public: 
    }



protected:

    aaudio_result_t requestStart_l() REQUIRES(mStreamLock) override;

    aaudio_result_t requestStop_l() REQUIRES(mStreamLock) override;



    aaudio_result_t release_l() REQUIRES(mStreamLock) override;



    aaudio_result_t processData(void *buffer,

                         int32_t numFrames,
@@ -119,8 +114,6 @@ protected: 


    aaudio_result_t stopCallback_l();



    aaudio_result_t joinThread_l(void** returnArg);



    virtual void prepareBuffersForStart() {}



    virtual void advanceClientToMatchServerPosition(int32_t serverMargin = 0) = 0;

media/libaaudio/src/core/AudioStream.cpp

+7 −15
Original line number Diff line number Diff line
@@ -248,7 +248,7 @@ aaudio_result_t AudioStream::safeFlush() { 


aaudio_result_t AudioStream::systemStopFromCallback() {

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

    aaudio_result_t result = safeStop();

    aaudio_result_t result = safeStop_l();

    if (result == AAUDIO_OK) {

        // We only call this for logging in "dumpsys audio". So ignore return code.

        (void) mPlayerBase->stop();
@@ -262,7 +262,7 @@ aaudio_result_t AudioStream::systemStopFromApp() { 
        ALOGE("stream cannot be stopped by calling from a callback!");

        return AAUDIO_ERROR_INVALID_STATE;

    }

    aaudio_result_t result = safeStop();

    aaudio_result_t result = safeStop_l();

    if (result == AAUDIO_OK) {

        // We only call this for logging in "dumpsys audio". So ignore return code.

        (void) mPlayerBase->stop();
@@ -270,8 +270,7 @@ aaudio_result_t AudioStream::systemStopFromApp() { 
    return result;

}



// This must be called under mStreamLock.

aaudio_result_t AudioStream::safeStop() {

aaudio_result_t AudioStream::safeStop_l() {



    switch (getState()) {

        // Proceed with stopping.
@@ -472,15 +471,14 @@ aaudio_result_t AudioStream::createThread_l(int64_t periodNanoseconds, 
    }

}



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

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

    // 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);

    return joinThread_l(returnArg);

}



// This must be called under mStreamLock.

aaudio_result_t AudioStream::joinThread_l(void** returnArg, int64_t /* timeoutNanoseconds */)

{

aaudio_result_t AudioStream::joinThread_l(void** returnArg) {

    if (!mHasThread) {

        ALOGD("joinThread() - but has no thread");

        return AAUDIO_ERROR_INVALID_STATE;
@@ -492,13 +490,7 @@ aaudio_result_t AudioStream::joinThread_l(void** returnArg, int64_t /* timeoutNa 
        // 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;

        int err = pthread_timedjoin_np(mThread, returnArg, &abstime);

#else

        int err = pthread_join(mThread, returnArg);

#endif

        mStreamLock.lock();

        if (err) {

            ALOGE("%s() pthread_join() returns err = %d", __func__, err);
@@ -614,7 +606,7 @@ android::status_t AudioStream::MyPlayerBase::playerSetVolume() { 
    }

    if (audioStream) {

        // No pan and only left volume is taken into account from IPLayer interface

        audioStream->setDuckAndMuteVolume(mVolumeMultiplierL  /* * mPanMultiplierL */);

        audioStream->setDuckAndMuteVolume(mVolumeMultiplierL  /* mPanMultiplierL */);

    }

    return android::NO_ERROR;

}

media/libaaudio/src/core/AudioStream.h

+24 −21
Original line number Diff line number Diff line
@@ -20,11 +20,13 @@ 
#include <atomic>

#include <mutex>

#include <stdint.h>

#include <aaudio/AAudio.h>



#include <android-base/thread_annotations.h>

#include <binder/IServiceManager.h>

#include <binder/Status.h>

#include <utils/StrongPointer.h>



#include <aaudio/AAudio.h>

#include <media/AudioSystem.h>

#include <media/PlayerBase.h>

#include <media/VolumeShaper.h>
@@ -57,11 +59,6 @@ public: 


protected:



    /* Asynchronous requests.

     * Use waitForStateChange() to wait for completion.

     */

    virtual aaudio_result_t requestStart_l() = 0;



    /**

     * Check the state to see if Pause is currently legal.

     *
@@ -80,17 +77,22 @@ protected: 
        return false;

    }



    virtual aaudio_result_t requestPause_l() {

    /* Asynchronous requests.

     * Use waitForStateChange() to wait for completion.

     */

    virtual aaudio_result_t requestStart_l() REQUIRES(mStreamLock) = 0;



    virtual aaudio_result_t requestPause_l() REQUIRES(mStreamLock) {

        // Only implement this for OUTPUT streams.

        return AAUDIO_ERROR_UNIMPLEMENTED;

    }



    virtual aaudio_result_t requestFlush_l() {

    virtual aaudio_result_t requestFlush_l() REQUIRES(mStreamLock) {

        // Only implement this for OUTPUT streams.

        return AAUDIO_ERROR_UNIMPLEMENTED;

    }



    virtual aaudio_result_t requestStop_l() = 0;

    virtual aaudio_result_t requestStop_l() REQUIRES(mStreamLock) = 0;



public:

    virtual aaudio_result_t getTimestamp(clockid_t clockId,
@@ -130,11 +132,12 @@ public: 
     * The AAudioStream_close() method releases if needed and then closes.

     */



protected:

    /**

     * Free any hardware or system resources from the open() call.

     * It is safe to call release_l() multiple times.

     */

    virtual aaudio_result_t release_l() {

    virtual aaudio_result_t release_l() REQUIRES(mStreamLock) {

        setState(AAUDIO_STREAM_STATE_CLOSING);

        return AAUDIO_OK;

    }
@@ -143,7 +146,7 @@ public: 
     * Free any resources not already freed by release_l().

     * Assume release_l() already called.

     */

    virtual void close_l() {

    virtual void close_l() REQUIRES(mStreamLock) {

        // Releasing the stream will set the state to CLOSING.

        assert(getState() == AAUDIO_STREAM_STATE_CLOSING);

        // setState() prevents a transition from CLOSING to any state other than CLOSED.
@@ -151,6 +154,7 @@ public: 
        setState(AAUDIO_STREAM_STATE_CLOSED);

    }



public:

    // This is only used to identify a stream in the logs without

    // revealing any pointers.

    aaudio_stream_id_t getId() {
@@ -163,7 +167,7 @@ public: 
                                           aaudio_audio_thread_proc_t threadProc,

                                           void *threadArg);



    aaudio_result_t joinThread(void **returnArg, int64_t timeoutNanoseconds);

    aaudio_result_t joinThread(void **returnArg);



    virtual aaudio_result_t registerThread() {

        return AAUDIO_OK;
@@ -473,9 +477,8 @@ protected: 


    private:

        // Use a weak pointer so the AudioStream can be deleted.



        std::mutex               mParentLock;

        android::wp<AudioStream> mParent;

        android::wp<AudioStream> mParent GUARDED_BY(mParentLock);

        aaudio_result_t          mResult = AAUDIO_OK;

        bool                     mRegistered = false;

    };
@@ -533,7 +536,7 @@ protected: 
        mSessionId = sessionId;

    }



    aaudio_result_t joinThread_l(void **returnArg, int64_t timeoutNanoseconds);

    aaudio_result_t joinThread_l(void **returnArg) REQUIRES(mStreamLock);



    std::atomic<bool>    mCallbackEnabled{false};
@@ -601,14 +604,16 @@ protected: 


    std::string mMetricsId; // set once during open()



    std::mutex                 mStreamLock;



private:



    aaudio_result_t safeStop();

    aaudio_result_t safeStop_l() REQUIRES(mStreamLock);



    /**

     * Release then close the stream.

     */

    void releaseCloseFinal_l() {

    void releaseCloseFinal_l() REQUIRES(mStreamLock) {

        if (getState() != AAUDIO_STREAM_STATE_CLOSING) { // not already released?

            // Ignore result and keep closing.

            (void) release_l();
@@ -616,8 +621,6 @@ private: 
        close_l();

    }



    std::mutex                 mStreamLock;



    const android::sp<MyPlayerBase>   mPlayerBase;



    // These do not change after open().
@@ -656,8 +659,8 @@ private: 
    std::atomic<pid_t>          mErrorCallbackThread{CALLBACK_THREAD_NONE};



    // background thread ----------------------------------

    bool                        mHasThread = false;

    pthread_t                   mThread = {};

    bool                        mHasThread GUARDED_BY(mStreamLock) = false;

    pthread_t                   mThread  GUARDED_BY(mStreamLock) = {};



    // These are set by the application thread and then read by the audio pthread.

    std::atomic<int64_t>        mPeriodNanoseconds; // for tuning SCHED_FIFO threads