aaudio: stop calling virtual methods from destructor

Was calling close(), which is abstract and virtual.
This may have been related to some audioserver crashes.
Also cleaned up some strong pointer handling.

Bug: 63390734
Bug: 63353455
Test: run CTS nativemedia/aaudio many times
Change-Id: Ib95aed60a64771b64395c67f0921c67146f9d10f

diff --git a/services/oboeservice/AAudioService.cpp b/services/oboeservice/AAudioService.cpp
index 8f89051..4ccd2f6 100644 (file)
--- a/services/oboeservice/AAudioService.cpp
+++ b/services/oboeservice/AAudioService.cpp
@@ -150,17 +150,18 @@ aaudio_handle_t AAudioService::openStream(const aaudio::AAudioStreamRequest &req
 }
 
 aaudio_result_t AAudioService::closeStream(aaudio_handle_t streamHandle) {
-    // Check permission first.
-    AAudioServiceStreamBase *serviceStream = convertHandleToServiceStream(streamHandle);
+    // Check permission and ownership first.
+    sp<AAudioServiceStreamBase> serviceStream = convertHandleToServiceStream(streamHandle);
     if (serviceStream == nullptr) {
         ALOGE("AAudioService::startStream(), illegal stream handle = 0x%0x", streamHandle);
         return AAUDIO_ERROR_INVALID_HANDLE;
     }
 
+    ALOGD("AAudioService.closeStream(0x%08X)", streamHandle);
+    // Remove handle from tracker so that we cannot look up the raw address any more.
     serviceStream = (AAudioServiceStreamBase *)
             mHandleTracker.remove(AAUDIO_HANDLE_TYPE_STREAM,
                                   streamHandle);
-    ALOGD("AAudioService.closeStream(0x%08X)", streamHandle);
     if (serviceStream != nullptr) {
         serviceStream->close();
         pid_t pid = serviceStream->getOwnerProcessId();

diff --git a/services/oboeservice/AAudioServiceStreamBase.cpp b/services/oboeservice/AAudioServiceStreamBase.cpp
index c88038f..2e20287 100644 (file)
--- a/services/oboeservice/AAudioServiceStreamBase.cpp
+++ b/services/oboeservice/AAudioServiceStreamBase.cpp
@@ -41,8 +41,11 @@ AAudioServiceStreamBase::AAudioServiceStreamBase()
 }
 
 AAudioServiceStreamBase::~AAudioServiceStreamBase() {
-    close();
-    ALOGD("AAudioServiceStreamBase::~AAudioServiceStreamBase() destroyed %p", this);
+    ALOGD("AAudioServiceStreamBase::~AAudioServiceStreamBase() destroying %p", this);
+    // If the stream is deleted without closing then audio resources will leak.
+    // Not being closed here would indicate an internal error. So we want to find this ASAP.
+    LOG_ALWAYS_FATAL_IF(mState != AAUDIO_STREAM_STATE_CLOSED,
+                        "service stream not closed, state = %d", mState);
 }
 
 std::string AAudioServiceStreamBase::dump() const {
@@ -71,11 +74,13 @@ aaudio_result_t AAudioServiceStreamBase::open(const aaudio::AAudioStreamRequest
 }
 
 aaudio_result_t AAudioServiceStreamBase::close() {
-    stop();
-    std::lock_guard<std::mutex> lock(mLockUpMessageQueue);
-    delete mUpMessageQueue;
-    mUpMessageQueue = nullptr;
-
+    if (mState != AAUDIO_STREAM_STATE_CLOSED) {
+        stopTimestampThread();
+        std::lock_guard<std::mutex> lock(mLockUpMessageQueue);
+        delete mUpMessageQueue;
+        mUpMessageQueue = nullptr;
+        mState = AAUDIO_STREAM_STATE_CLOSED;
+    }
     return AAUDIO_OK;
 }
 
@@ -106,9 +111,8 @@ aaudio_result_t AAudioServiceStreamBase::stop() {
     aaudio_result_t result = AAUDIO_OK;
     if (isRunning()) {
         // TODO wait for data to be played out
-        sendCurrentTimestamp();
-        mThreadEnabled.store(false);
-        result = mAAudioThread.stop();
+        sendCurrentTimestamp(); // warning - this calls a virtual function
+        result = stopTimestampThread();
         if (result != AAUDIO_OK) {
             disconnect();
             return result;
@@ -119,6 +123,15 @@ aaudio_result_t AAudioServiceStreamBase::stop() {
     return result;
 }
 
+aaudio_result_t AAudioServiceStreamBase::stopTimestampThread() {
+    aaudio_result_t result = AAUDIO_OK;
+    // clear flag that tells thread to loop
+    if (mThreadEnabled.exchange(false)) {
+        result = mAAudioThread.stop();
+    }
+    return result;
+}
+
 aaudio_result_t AAudioServiceStreamBase::flush() {
     sendServiceEvent(AAUDIO_SERVICE_EVENT_FLUSHED);
     mState = AAUDIO_STREAM_STATE_FLUSHED;
@@ -141,6 +154,7 @@ void AAudioServiceStreamBase::run() {
             nextTime = timestampScheduler.nextAbsoluteTime();
         } else  {
             // Sleep until it is time to send the next timestamp.
+            // TODO Wait for a signal with a timeout so that we can stop more quickly.
             AudioClock::sleepUntilNanoTime(nextTime);
         }
     }

diff --git a/services/oboeservice/AAudioServiceStreamBase.h b/services/oboeservice/AAudioServiceStreamBase.h
index 0bd2276..eed1a03 100644 (file)
--- a/services/oboeservice/AAudioServiceStreamBase.h
+++ b/services/oboeservice/AAudioServiceStreamBase.h
@@ -78,6 +78,8 @@ public:
      */
     virtual aaudio_result_t stop();
 
+    aaudio_result_t stopTimestampThread();
+
     /**
      *  Discard any data held by the underlying HAL or Service.
      */
@@ -143,6 +145,7 @@ public:
     }
 
 protected:
+
     aaudio_result_t writeUpMessageQueue(AAudioServiceMessage *command);
 
     aaudio_result_t sendCurrentTimestamp();

diff --git a/services/oboeservice/AAudioServiceStreamMMAP.cpp b/services/oboeservice/AAudioServiceStreamMMAP.cpp
index da7cdf7..da18f44 100644 (file)
--- a/services/oboeservice/AAudioServiceStreamMMAP.cpp
+++ b/services/oboeservice/AAudioServiceStreamMMAP.cpp
@@ -49,16 +49,18 @@ AAudioServiceStreamMMAP::AAudioServiceStreamMMAP(uid_t serviceUid)
         , mCachedUserId(serviceUid) {
 }
 
-AAudioServiceStreamMMAP::~AAudioServiceStreamMMAP() {
-    close();
-}
-
 aaudio_result_t AAudioServiceStreamMMAP::close() {
-    mMmapStream.clear(); // TODO review. Is that all we have to do?
-    // Apparently the above close is asynchronous. An attempt to open a new device
-    // right after a close can fail. Also some callbacks may still be in flight!
-    // FIXME Make closing synchronous.
-    AudioClock::sleepForNanos(100 * AAUDIO_NANOS_PER_MILLISECOND);
+    if (mState == AAUDIO_STREAM_STATE_CLOSED) {
+        return AAUDIO_OK;
+    }
+
+    if (mMmapStream != 0) {
+        mMmapStream.clear(); // TODO review. Is that all we have to do?
+        // Apparently the above close is asynchronous. An attempt to open a new device
+        // right after a close can fail. Also some callbacks may still be in flight!
+        // FIXME Make closing synchronous.
+        AudioClock::sleepForNanos(100 * AAUDIO_NANOS_PER_MILLISECOND);
+    }
 
     if (mAudioDataFileDescriptor != -1) {
         ::close(mAudioDataFileDescriptor);

diff --git a/services/oboeservice/AAudioServiceStreamMMAP.h b/services/oboeservice/AAudioServiceStreamMMAP.h
index 337252d..257bea9 100644 (file)
--- a/services/oboeservice/AAudioServiceStreamMMAP.h
+++ b/services/oboeservice/AAudioServiceStreamMMAP.h
@@ -44,8 +44,7 @@ class AAudioServiceStreamMMAP
 
 public:
     AAudioServiceStreamMMAP(uid_t serviceUid);
-    virtual ~AAudioServiceStreamMMAP();
-
+    virtual ~AAudioServiceStreamMMAP() = default;
 
     aaudio_result_t open(const aaudio::AAudioStreamRequest &request,
                                  aaudio::AAudioStreamConfiguration &configurationOutput) override;

diff --git a/services/oboeservice/AAudioServiceStreamShared.cpp b/services/oboeservice/AAudioServiceStreamShared.cpp
index 1978ddd..9cc5cbc 100644 (file)
--- a/services/oboeservice/AAudioServiceStreamShared.cpp
+++ b/services/oboeservice/AAudioServiceStreamShared.cpp
@@ -44,10 +44,6 @@ AAudioServiceStreamShared::AAudioServiceStreamShared(AAudioService &audioService
     {
 }
 
-AAudioServiceStreamShared::~AAudioServiceStreamShared() {
-    close();
-}
-
 int32_t AAudioServiceStreamShared::calculateBufferCapacity(int32_t requestedCapacityFrames,
                                                            int32_t framesPerBurst) {
 
@@ -260,21 +256,27 @@ aaudio_result_t AAudioServiceStreamShared::flush()  {
 }
 
 aaudio_result_t AAudioServiceStreamShared::close()  {
-    pause();
-    // TODO wait for pause() to synchronize
-    AAudioServiceEndpoint *endpoint = mServiceEndpoint;
-    if (endpoint != nullptr) {
-        sp<AAudioServiceStreamShared> keep(this);
-        endpoint->unregisterStream(keep);
+    if (mState == AAUDIO_STREAM_STATE_CLOSED) {
+        return AAUDIO_OK;
+    }
 
-        AAudioEndpointManager &mEndpointManager = AAudioEndpointManager::getInstance();
-        mEndpointManager.closeEndpoint(endpoint);
-        mServiceEndpoint = nullptr;
+    AAudioServiceEndpoint *endpoint = mServiceEndpoint;
+    if (endpoint == nullptr) {
+        return AAUDIO_ERROR_INVALID_STATE;
     }
+    endpoint->stopStream(this);
+
+    endpoint->unregisterStream(this);
+
+    AAudioEndpointManager &mEndpointManager = AAudioEndpointManager::getInstance();
+    mEndpointManager.closeEndpoint(endpoint);
+    mServiceEndpoint = nullptr;
+
     if (mAudioDataQueue != nullptr) {
         delete mAudioDataQueue;
         mAudioDataQueue = nullptr;
     }
+
     return AAudioServiceStreamBase::close();
 }
 

diff --git a/services/oboeservice/AAudioServiceStreamShared.h b/services/oboeservice/AAudioServiceStreamShared.h
index 9de502b..742c5af 100644 (file)
--- a/services/oboeservice/AAudioServiceStreamShared.h
+++ b/services/oboeservice/AAudioServiceStreamShared.h
@@ -44,7 +44,7 @@ class AAudioServiceStreamShared : public AAudioServiceStreamBase {
 
 public:
     AAudioServiceStreamShared(android::AAudioService &aAudioService);
-    virtual ~AAudioServiceStreamShared();
+    virtual ~AAudioServiceStreamShared() = default;
 
     aaudio_result_t open(const aaudio::AAudioStreamRequest &request,
                          aaudio::AAudioStreamConfiguration &configurationOutput) override;