Merge "SoundPool: Clean up Stream::play_l logic for garbage collection" am: 3c44833a91

Original change: https://android-review.googlesource.com/c/platform/frameworks/base/+/1922978

Change-Id: I0f776fce8b1012c31c15153efe1fd46ead7131e3

media/jni/soundpool/Stream.cpp

@@ -274,12 +274,6 @@ Stream* Stream::playPairStream(std::vector<std::any>& garbage) {
 void Stream::play_l(const std::shared_ptr<Sound>& sound, int32_t nextStreamID,
         float leftVolume, float rightVolume, int32_t priority, int32_t loop, float rate,
         std::vector<std::any>& garbage)
-{
-    // oldTrack and newTrack are placeholders to be released by garbage without the lock.
-    sp<AudioTrack> oldTrack;
-    sp<AudioTrack> newTrack;
-    status_t status = NO_ERROR;
-
 {
     ALOGV("%s(%p)(soundID=%d, streamID=%d, leftVolume=%f, rightVolume=%f,"
             " priority=%d, loop=%d, rate=%f)",
@@ -300,16 +294,20 @@ void Stream::play_l(const std::shared_ptr<Sound>& sound, int32_t nextStreamID,
         frameCount = sound->getSizeInBytes() / frameSize;
     }
 
-        // check if the existing track has the same sound id.
+    if (mAudioTrack != nullptr) {
-        if (mAudioTrack != nullptr && mSoundID == sound->getSoundID()) {
+        if (mSoundID == sound->getSoundID()
+                && mAudioTrack->setSampleRate(sampleRate) == NO_ERROR) {
+            // Reuse the old track if the soundID matches.
             // the sample rate may fail to change if the audio track is a fast track.
-            if (mAudioTrack->setSampleRate(sampleRate) == NO_ERROR) {
-                newTrack = mAudioTrack;
             ALOGV("%s: reusing track %p for sound %d",
                     __func__, mAudioTrack.get(), sound->getSoundID());
+        } else {
+            // If reuse not possible, move mAudioTrack to garbage, set to nullptr.
+            garbage.emplace_back(std::move(mAudioTrack));
+            mAudioTrack.clear(); // move should have cleared the sp<>, but we clear just in case.
         }
     }
-        if (newTrack == nullptr) {
+    if (mAudioTrack == nullptr) {
         // mToggle toggles each time a track is started on a given stream.
         // The toggle is concatenated with the Stream address and passed to AudioTrack
         // as callback user data. This enables the detection of callbacks received from the old
@@ -330,7 +328,7 @@ void Stream::play_l(const std::shared_ptr<Sound>& sound, int32_t nextStreamID,
         attributionSource.packageName = mStreamManager->getOpPackageName();
         attributionSource.token = sp<BBinder>::make();
         // TODO b/182469354 make consistent with AudioRecord, add util for native source
-            newTrack = new AudioTrack(streamType, sampleRate, sound->getFormat(),
+        mAudioTrack = new AudioTrack(streamType, sampleRate, sound->getFormat(),
                 channelMask, sound->getIMemory(), AUDIO_OUTPUT_FLAG_FAST,
                 staticCallback, userData,
                 0 /*default notification frames*/, AUDIO_SESSION_ALLOCATE,
@@ -340,26 +338,30 @@ void Stream::play_l(const std::shared_ptr<Sound>& sound, int32_t nextStreamID,
                 false /*doNotReconnect*/, 1.0f /*maxRequiredSpeed*/);
         // Set caller name so it can be logged in destructor.
         // MediaMetricsConstants.h: AMEDIAMETRICS_PROP_CALLERNAME_VALUE_SOUNDPOOL
-            newTrack->setCallerName("soundpool");
+        mAudioTrack->setCallerName("soundpool");
-            oldTrack = mAudioTrack;
+
-            status = newTrack->initCheck();
+        if (status_t status = mAudioTrack->initCheck();
-            if (status != NO_ERROR) {
+            status != NO_ERROR) {
-                ALOGE("%s: error creating AudioTrack", __func__);
+            ALOGE("%s: error %d creating AudioTrack", __func__, status);
-                // newTrack goes out of scope, so reference count drops to zero
+            // TODO: should we consider keeping the soundID and reusing the old track?
-                goto exit;
+            mState = IDLE;
+            mSoundID = 0;
+            mSound.reset();
+            garbage.emplace_back(std::move(mAudioTrack)); // remove mAudioTrack.
+            mAudioTrack.clear(); // move should have cleared the sp<>, but we clear just in case.
+            return;
         }
         // From now on, AudioTrack callbacks received with previous toggle value will be ignored.
         mToggle = toggle;
-            mAudioTrack = newTrack;
         ALOGV("%s: using new track %p for sound %d",
-                    __func__, newTrack.get(), sound->getSoundID());
+                __func__, mAudioTrack.get(), sound->getSoundID());
     }
     if (mMuted) {
-            newTrack->setVolume(0.0f, 0.0f);
+        mAudioTrack->setVolume(0.f, 0.f);
     } else {
-            newTrack->setVolume(leftVolume, rightVolume);
+        mAudioTrack->setVolume(leftVolume, rightVolume);
     }
-        newTrack->setLoop(0, frameCount, loop);
+    mAudioTrack->setLoop(0, frameCount, loop);
     mAudioTrack->start();
     mSound = sound;
     mSoundID = sound->getSoundID();
@@ -373,22 +375,6 @@ void Stream::play_l(const std::shared_ptr<Sound>& sound, int32_t nextStreamID,
     mStreamID = nextStreamID;  // prefer this to be the last, as it is an atomic sync point
 }
 
-exit:
-    ALOGV("%s: delete oldTrack %p", __func__, oldTrack.get());
-    if (status != NO_ERROR) {
-        // TODO: should we consider keeping the soundID if the old track is OK?
-        // Do not attempt to restart this track (should we remove the stream id?)
-        mState = IDLE;
-        mSoundID = 0;
-        mSound.reset();
-        mAudioTrack.clear();  // actual release from garbage
-    }
-
-    // move tracks to garbage to be released later outside of lock.
-    if (newTrack) garbage.emplace_back(std::move(newTrack));
-    if (oldTrack) garbage.emplace_back(std::move(oldTrack));
-}
-
 /* static */
 void Stream::staticCallback(int event, void* user, void* info)
 {